From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52524) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ReFOh-0001Gx-JB for qemu-devel@nongnu.org; Fri, 23 Dec 2011 19:23:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ReFOg-0001wz-3w for qemu-devel@nongnu.org; Fri, 23 Dec 2011 19:23:35 -0500 Received: from mail-qw0-f52.google.com ([209.85.216.52]:59432) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ReFOf-0001wW-W0 for qemu-devel@nongnu.org; Fri, 23 Dec 2011 19:23:34 -0500 Received: by qadc11 with SMTP id c11so6109877qad.4 for ; Fri, 23 Dec 2011 16:23:32 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1324578014-24746-2-git-send-email-mark.langsdorf@calxeda.com> References: <1324578014-24746-1-git-send-email-mark.langsdorf@calxeda.com> <1324578014-24746-2-git-send-email-mark.langsdorf@calxeda.com> Date: Sat, 24 Dec 2011 00:23:32 +0000 Message-ID: From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 1/9] arm: add missing scu registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark Langsdorf Cc: kwolf@redhat.com, qemu-devel@nongnu.org, Rob Herring , paul@codesourcery.com On 22 December 2011 18:20, Mark Langsdorf wrot= e: > From: Rob Herring > > Add power control and non-secure access ctrl registers Commit message says it's adding the non-secure access control register, but the patch is only doing power control. > > Signed-off-by: Rob Herring > Signed-off-by: Mark Langsdorf > --- > Changes from v1: > =C2=A0 =C2=A0 =C2=A0 =C2=A0Added VMState support > =C2=A0 =C2=A0 =C2=A0 =C2=A0Checked alignment of writes to the power contr= ol register > > =C2=A0hw/a9mpcore.c | =C2=A0 34 ++++++++++++++++++++++++++++++++-- > =C2=A01 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/hw/a9mpcore.c b/hw/a9mpcore.c > index cd2985f..875ae98 100644 > --- a/hw/a9mpcore.c > +++ b/hw/a9mpcore.c > @@ -29,6 +29,7 @@ gic_get_current_cpu(void) > =C2=A0typedef struct a9mp_priv_state { > =C2=A0 =C2=A0 gic_state gic; > =C2=A0 =C2=A0 uint32_t scu_control; > + =C2=A0 =C2=A0uint32_t scu_status; > =C2=A0 =C2=A0 uint32_t old_timer_status[8]; > =C2=A0 =C2=A0 uint32_t num_cpu; > =C2=A0 =C2=A0 qemu_irq *timer_irq; > @@ -48,7 +49,13 @@ static uint64_t a9_scu_read(void *opaque, target_phys_= addr_t offset, > =C2=A0 =C2=A0 case 0x04: /* Configuration */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (((1 << s->num_cpu) - 1) << 4) | (s->n= um_cpu - 1); > =C2=A0 =C2=A0 case 0x08: /* CPU Power Status */ > - =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return s->scu_status; > + =C2=A0 =C2=A0case 0x09: /* CPU status. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return s->scu_status >> 8; > + =C2=A0 =C2=A0case 0x0a: /* CPU status. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return s->scu_status >> 16; > + =C2=A0 =C2=A0case 0x0b: /* CPU status. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return s->scu_status >> 24; > =C2=A0 =C2=A0 case 0x0c: /* Invalidate All Registers In Secure State */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0; > =C2=A0 =C2=A0 case 0x40: /* Filtering Start Address Register */ > @@ -73,6 +80,29 @@ static void a9_scu_write(void *opaque, target_phys_add= r_t offset, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; > =C2=A0 =C2=A0 case 0x4: /* Configuration: RO */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; > + =C2=A0 =C2=A0case 0x08: /* Power Control =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status =3D value; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0break; This does the wrong thing for a byte or halfword write to 0x8. (Byte writes in particular are going to be the common case for this register for obvious reasons.) > + =C2=A0 =C2=A0case 0x09: /* Power Control =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status &=3D ~(0xff << 8); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status |=3D (value & 0xff) << 8; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0break; > + =C2=A0 =C2=A0case 0x0A: /* Power Control =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (size =3D=3D 1) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status &=3D ~(0xff << 1= 6); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status |=3D (value & 0x= ff) << 16; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else if (size =3D=3D 2) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status &=3D ~(0xffff <<= 16); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status |=3D (value & 0x= ffff) << 16; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* illegal number of bytes */ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0 =C2=A0 =C2=A0break; > + =C2=A0 =C2=A0case 0x0B: /* Power Control =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status &=3D ~(0xff << 24); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->scu_status |=3D (value & 0xff) << 24; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0break; How about: switch (size) { case 1: mask =3D 0xff; break; case 2: mask =3D 0xffff; break; case 4: mask =3D 0xffffffff; break; } Then the register write code simplifies to: case 0x08: case 0x09: case 0xa: case 0xb: shift =3D (offset - 0x8) * 8; s->scu_status &=3D ~(mask << shift); s->scu_status |=3D ((value & mask) << shift); break; (written on the hoof and untested!) > =C2=A0 =C2=A0 case 0x0c: /* Invalidate All Registers In Secure State */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* no-op as we do not implement caches */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; > @@ -80,7 +110,6 @@ static void a9_scu_write(void *opaque, target_phys_add= r_t offset, > =C2=A0 =C2=A0 case 0x44: /* Filtering End Address Register */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* RAZ/WI, like an implementation with only o= ne AXI master */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; > - =C2=A0 =C2=A0case 0x8: /* CPU Power Status */ > =C2=A0 =C2=A0 case 0x50: /* SCU Access Control Register */ > =C2=A0 =C2=A0 case 0x54: /* SCU Non-secure Access Control Register */ > =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* unimplemented, fall through */ > @@ -173,6 +202,7 @@ static const VMStateDescription vmstate_a9mp_priv =3D= { > =C2=A0 =C2=A0 .minimum_version_id =3D 1, > =C2=A0 =C2=A0 .fields =3D (VMStateField[]) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_UINT32(scu_control, a9mp_priv_state), > + =C2=A0 =C2=A0 =C2=A0 =C2=A0VMSTATE_UINT32(scu_status, a9mp_priv_state), > =C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_UINT32_ARRAY(old_timer_status, a9mp_p= riv_state, 8), > =C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_END_OF_LIST() > =C2=A0 =C2=A0 } This breaks migration compatibility. You want: * set .version_id to 2 * leave .minimum_version_id as 1 * the new field should go at the end of the list and be: VMSTATE_UINT32_V(scu_status, a9mp_priv_state, 2), (which says it only exists in version 2). -- PMM