Re: [PATCH for-6.2 09/25] clock: Provide builtin multiplier/divider

[Philippe Mathieu-Daudé]

On 8/16/21 11:36 AM, Peter Maydell wrote:
> On Mon, 16 Aug 2021 at 10:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 8/16/21 11:05 AM, Peter Maydell wrote:
>>> On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4bug@amsat.org> 
>>> wrote:
>>>> I only wonder if we shouldn't check clock_is_enabled() here.
>>>> Maybe not assert, but at least report a GUEST_ERROR?
>>>
>>> Setting the multiplier on a disabled clock doesn't seem like
>>> an error to me. I would expect a common way for the guest to
>>> program a clock-controller would be "first set the divider
>>> and any other parameters; finally, enable the clock".
>>
>> Eh sorry I meant the other way around :/ It is usually either
>> illegal or undefined behavior on real hw to change a clock scale
>> while it is active. Personally I'd be interested to catch guests
>> doing so. I was thinking of:
>>
>>     if (clock_is_enabled(clk)) {
>>         qemu_log_mask(LOG_GUEST_ERROR,
>>                       "Changing scale of ENABLED clock '%s'\n'",
>>                       CLOCK_PATH(clk));
>>     }
> 
> I think if particular clock-controller hardware has that
> restriction we should be logging guest errors there. (Doing that
> also has the advantage that we can make the error clearer by being
> specific about what guest hardware register/device is being
> mis-programmed.)

OK.

> I don't think we can be certain enough that it's
> always wrong to change the divider on a running clock to put the error
> in the common clock API code.

OK.

> (Among other things, I suspect a warning
> here would be easy to trigger incorrectly when connecting up hard-wired
> clock dividers at startup.)

Yes, probably.

Thanks for the clarifications.

Preferably with a trace-event:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>