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

[Philippe Mathieu-Daudé]

On 1/15/20 12:04 AM, Niek Linnenbank wrote:
> On Tue, Jan 14, 2020 at 12:14 AM Philippe Mathieu-Daudé 
> <address@hidden <mailto:address@hidden>> 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 :)