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

[Philippe Mathieu-Daudé]

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.

Regards,

Phil.