Re: [PATCH v8 02/14] hw/misc: Add NPCM7xx Clock Controller device model

[Havard Skinnemoen <hskinnemoen@google.com>]

On Mon, Sep 7, 2020 at 6:40 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 9/5/20 12:38 AM, Havard Skinnemoen wrote:
> > On Fri, Sep 4, 2020 at 3:02 PM Havard Skinnemoen <hskinnemoen@google.com> wrote:
> >>
> >> On Fri, Sep 4, 2020 at 2:32 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>>
> >>> On 8/25/20 2:16 AM, Havard Skinnemoen via wrote:
> >>>> Enough functionality to boot the Linux kernel has been implemented. This
> >>>> includes:
> >>>>
> >>>>   - Correct power-on reset values so the various clock rates can be
> >>>>     accurately calculated.
> >>>>   - Clock enables stick around when written.
> >>>>
> >>>> In addition, a best effort attempt to implement SECCNT and CNTR25M was
> >>>> made even though I don't think the kernel needs them.
> >>>>
> >>>> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> >>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
> >>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> >>>> ---
> [...]
> >>>> +static void npcm7xx_clk_write(void *opaque, hwaddr offset,
> >>>> +                              uint64_t v, unsigned size)
> >>>> +{
> >>>> +    uint32_t reg = offset / sizeof(uint32_t);
> >>>> +    NPCM7xxCLKState *s = opaque;
> >>>> +    uint32_t value = v;
> >>>> +
> >>>> +    trace_npcm7xx_clk_write(offset, value);
> >>>> +
> >>>> +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> >>>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>>> +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> >>>> +                      __func__, offset);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    switch (reg) {
> >>>> +    case NPCM7XX_CLK_SWRSTR:
> >>>> +        qemu_log_mask(LOG_UNIMP, "%s: SW reset not implemented: 0x%02x\n",
> >>>> +                      __func__, value);
> >>>
> >>> Isn't this sufficient?
> >>>
> >>>            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >>
> >> It's not quite that easy; this register holds 4 bits, each of which
> >> maps to a separate register which defines the modules to reset. It's
> >> not that hard, but a little more than I'd like to add to the series at
> >> this point. I'll send a followup patch once the initial series is in.
> >
> > Actually, I'm not sure if this would have any effect on being able to
> > reboot. Running with -d unimp shows:
> >
> > reboot: Restarting system
> > npcm7xx_timer_write: WTCR write not implemented: 0x00000083
> > Reboot failed -- System halted
> >
> > So we need to implement watchdog support, which is something we were
> > planning to do fairly soon.
>
> Well this seems a guest implementation decision to prefer
> wait the watchdog to kick (hard reset?) rather than doing
> a soft reset.
>
> Two different issues IMO. Anyway this is certainly not
> blocking your series to get merged.

I agree, we need to implement both. I just wanted to note that
implementing SWRST alone may not be enough to make reboots work with
the current image. The SW reset registers are actually very similar to
the WD reset registers, so implementing WD reset should make it almost
trivial to add SW reset as well.

It looks like the kernel has a driver that can use SW reset to reboot,
but the DT is missing a property needed to set up the restart handler.

https://elixir.bootlin.com/linux/v5.8.7/source/drivers/reset/reset-npcm.c#L269

Havard