Re: [PATCH v3 06/17] hw/arm/allwinner: add CPU Configuration module

From: Niek Linnenbank <nieklinnenbank@gmail.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH v3 06/17] hw/arm/allwinner: add CPU Configuration module
Date: Sat, 18 Jan 2020 23:17:27 +0100	[thread overview]
Message-ID: <CAPan3WpLzg0yzkgj29t3QdFczNsFwqubMUa1_xnqrO2RJMgxaA@mail.gmail.com> (raw)
In-Reply-To: <5760dae5-f7cc-5736-7275-e6461f985dbd@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]

On Sat, Jan 18, 2020 at 10:06 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 1/15/20 12:04 AM, Niek Linnenbank wrote:
> > On Tue, Jan 14, 2020 at 12:14 AM Philippe Mathieu-Daudé
> > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> >
> >     On 1/8/20 9:00 PM, Niek Linnenbank wrote:
> >      > Various Allwinner System on Chip designs contain multiple
> processors
> >      > that can be configured and reset using the generic CPU
> Configuration
> >      > module interface. This commit adds support for the Allwinner CPU
> >      > configuration interface which emulates the following features:
> >      >
> >      >   * CPU reset
> >      >   * CPU status
> >      >   * Shared 64-bit timer
> >      >
> [...]
> >      > +    case REG_CPU0_CTRL:         /* CPU#0 Control */
> >      > +    case REG_CPU1_CTRL:         /* CPU#1 Control */
> >      > +    case REG_CPU2_CTRL:         /* CPU#2 Control */
> >      > +    case REG_CPU3_CTRL:         /* CPU#3 Control */
> >      > +    case REG_CPU0_STATUS:       /* CPU#0 Status */
> >      > +    case REG_CPU1_STATUS:       /* CPU#1 Status */
> >      > +    case REG_CPU2_STATUS:       /* CPU#2 Status */
> >      > +    case REG_CPU3_STATUS:       /* CPU#3 Status */
> >      > +    case REG_CLK_GATING:        /* CPU Clock Gating */
> >      > +    case REG_GEN_CTRL:          /* General Control */
> >      > +        s->gen_ctrl = val;
> >      > +        break;
> >      > +    case REG_SUPER_STANDBY:     /* Super Standby Flag */
> >      > +        s->super_standby = val;
> >      > +        break;
> >      > +    case REG_ENTRY_ADDR:        /* Reset Entry Address */
> >      > +        s->entry_addr = val;
> >      > +        break;
> >      > +    case REG_DBG_EXTERN:        /* Debug External */
> >      > +        break;
> >      > +    case REG_CNT64_CTRL:        /* 64-bit Counter Control */
> >      > +        s->counter_ctrl = val;
> >      > +        break;
> >      > +    case REG_CNT64_LOW:         /* 64-bit Counter Low */
> >      > +    case REG_CNT64_HIGH:        /* 64-bit Counter High */
> >
> >     You forgot to set these. Maybe you can add a int64_t cnt64_diff, set
> it
> >     here to the difference with qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> and
> >     in the read() function return cnt64_diff +
> >     qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL).
> >
> > OK I'll need to look into that. Currently this timer is not used by
> > Linux, NetBSD or U-Boot as far
> > as I know. But since it is there, it should be correct indeed.
>
> You might reduce this patch by simply using LOG_UNIMP for these
> registers. Than add the patch when you find some use.
>
> We are more confident when reviewing code when we have a way to test it :)
>
>
True indeed. I'll just remove the 64-bit counter from this patch. Thanks!

Regards,
Niek

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 4134 bytes --]