Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Julien Grall <julien.grall@arm.com>]

Hi,

On 30/06/16 16:18, Mark Rutland wrote:
> On Thu, Jun 30, 2016 at 04:56:40PM +0200, Dirk Behme wrote:
>> On 30.06.2016 16:21, Mark Rutland wrote:
>>> On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote:
>>>> +- clocks: one or more clocks to be registered.
>>>> +  Xen hypervisor drivers might replace native drivers, resulting in
>>>> +  clocks not registered by these native drivers. To avoid that these
>>>> +  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
>>>> +  register them in the hypervisor node.
>>>> +  An example for this are the clocks of the serial driver. If the clocks
>>>> +  used by the serial hardware interface are not registered by the serial
>>>> +  driver the serial output might stop once clk_disable_unused() is called.
>>>
>>> This shouldn't be described in terms of the Linux implementation details
>>> like clk_disable_unused. Those can change at any time, and don't define
>>> the contract as such.
>>
>> Ok. I remove that from the description. Thanks!
>
> Great, thanks.
>
>>> What exactly is the contract here? I assume that you don't want the
>>> kernel to alter the state of these clocks in any way,
>>
>> I don't think so. I think what we want is that the kernel just don't
>> disable the (from kernel's point of view) "unused" clocks. I think
>> we get this from the fact that using 'clk_ignore_unused' is a
>> working workaround for this issue.
>
> What if the clock (or a parent thereof) is shared with another device?
>
> Surely you don't want that other driver to change the rate (changing the
> rate of the UART behind Xen's back)?
>
> I appreciate that clk_ignore_unused might work on platforms today, but
> that relies on the behaviour of drivers, which might change. It may also
> not work on other platforms in future, in cases like the above.
>
>>> e.g. the rate cannot be changed, they must be left always on, parent
>>> clocks cannot be altered if that would result in momentary jitter.
>>>
>>> I suspect it may be necessary to do more to ensure that, but I'm not
>>> familiar enough with the clocks subsystem to say.
>>>
>>> Are we likely to need to care about regulators, GPIOs, resets, etc? I
>>> worry that this may be the tip of hte iceberg
>>
>> I don't think so. If there is a user in the kernel of the clock,
>> fine. Then hopefully the user in the kernel knows what he is doing
>> with the clock and then he should do what ever he wants.
>
> I'm not sure that's true. A user of the clock may know nothing about
> other users. As far as I can see, nothing prevents it from changing the
> clock rate.
>
> While drivers in the same kernel can co-ordinate to avoid problems such
> as that, we can't do that if we know nothing about the user hidden by
> Xen.
>
>  From looking at the Xen bug tracker, it's not clear which
> platforms/devices this is necessary for, so it's still not clear to me
> if we need to care about regulators, GPIOs, resets, and so on.
>
> Do we know which platforms in particular we need this for?

I believe this is necessary for all the platforms where the UART has a 
clock described in the firmware.

This workaround was first required on the arndale. Note that UART on 
Juno r2 has a clock described but 'clk_ignore_unused' is not necessary. 
I am wondering why it is working.

The number of devices used by Xen is very limited (UART, timer and 
SMMU). However we may also want to consider device passthrough where a 
clock would be handled by another guest.

Regards,

-- 
Julien Grall