[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 02/14] hw/misc: Add NPCM7xx Clock Controller device model
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v8 02/14] hw/misc: Add NPCM7xx Clock Controller device model |
Date: |
Mon, 7 Sep 2020 15:40:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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.