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

[Peter Maydell <peter.maydell@linaro.org>]

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

thanks
-- PMM