Re: [Qemu-devel] [PATCH v14 1/9] target-avr: AVR cores support is added.

[Michael Rolnik]

I remember I did it. So it seems to be a wrong review.

On Mon, Aug 15, 2016, 6:28 PM Peter Maydell <address@hidden>
wrote:

> On 29 July 2016 at 16:32, Michael Rolnik <address@hidden> wrote:
> >     1. basic CPU structure
> >     2. registers
> >     3. no instructions
> >     4. saving sreg, rampD, rampX, rampY, rampD, eind in HW representation
> >
> > Signed-off-by: Michael Rolnik <address@hidden>
>
> > diff --git a/configure b/configure
> > index f57fcc6..c4d58b4 100755
> > --- a/configure
> > +++ b/configure
> > @@ -5641,6 +5641,8 @@ case "$target_name" in
> >    x86_64)
> >      TARGET_BASE_ARCH=i386
> >    ;;
> > +  avr)
> > +  ;;
>
> I asked you to move this in my review of v12, but you haven't.
>
> >    alpha)
> >    ;;
> >    arm|armeb)
> > @@ -5837,6 +5839,9 @@ disas_config() {
> >
> >  for i in $ARCH $TARGET_BASE_ARCH ; do
> >    case "$i" in
> > +  avr)
> > +    disas_config "AVR"
> > +  ;;
>
> Ditto.
>
> >    alpha)
> >      disas_config "ALPHA"
> >    ;;
>
> > +#ifndef QEMU_AVR_CPU_QOM_H
> > +#define QEMU_AVR_CPU_QOM_H
> > +
> > +#include "qom/cpu.h"
> > +
> > +#define TYPE_AVR_CPU "avr"
> > +
> > +#define AVR_CPU_CLASS(klass) \
> > +                    OBJECT_CLASS_CHECK(AVRCPUClass, (klass),
> TYPE_AVR_CPU)
> > +#define AVR_CPU(obj) \
> > +                    OBJECT_CHECK(AVRCPU, (obj), TYPE_AVR_CPU)
> > +#define AVR_CPU_GET_CLASS(obj) \
> > +                    OBJECT_GET_CLASS(AVRCPUClass, (obj), TYPE_AVR_CPU)
> > +
> > +/**
> > +*  AVRCPUClass:
> > +*  @parent_realize: The parent class' realize handler.
> > +*  @parent_reset: The parent class' reset handler.
> > +*  @vr: Version Register value.
> > +*
> > +*  A AVR CPU model.
> > + */
>
> The spacing here is still wrong.
>
> > +typedef struct AVRCPUClass {
> > +    CPUClass parent_class;
> > +
> > +    DeviceRealize parent_realize;
> > +    void (*parent_reset)(CPUState *cpu);
> > +} AVRCPUClass;
> > +
> > +/**
> > +*  AVRCPU:
> > +*  @env: #CPUAVRState
> > +*
> > +*  A AVR CPU.
> > +*/
>
> Ditto.
>
> > +typedef struct AVRCPU {
> > +    /* < private > */
> > +    CPUState parent_obj;
> > +    /* < public > */
> > +
> > +    CPUAVRState env;
> > +} AVRCPU;
>
> > +static bool avr_cpu_has_work(CPUState *cs)
> > +{
> > +    AVRCPU *cpu = AVR_CPU(cs);
> > +    CPUAVRState *env = &cpu->env;
> > +
> > +    return (cs->interrupt_request
> > +                &   (CPU_INTERRUPT_HARD
> > +                    | CPU_INTERRUPT_RESET))
> > +            &&  cpu_interrupts_enabled(env);
>
> I asked you to fix this spacing, but it's still all over the place.
>
> If you could check that you've addressed all the comments
> from previous patch review rounds that would make things
> faster.
>
> thanks
> -- PMM
>