Re: [PATCH v4] xen/arm: Add a clock property

[Dirk Behme <dirk.behme@gmail.com>]

Hi Michael, Stefano and Julien,

On 22.07.2016 03:16, Stefano Stabellini wrote:
> On Thu, 21 Jul 2016, Michael Turquette wrote:
>> Quoting Stefano Stabellini (2016-07-14 03:38:04)
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 13.07.2016 23:03, Michael Turquette wrote:
>>>>> Quoting Dirk Behme (2016-07-13 11:56:30)
>>>>>> On 13.07.2016 20:43, Stefano Stabellini wrote:
>>>>>>> On Wed, 13 Jul 2016, Dirk Behme wrote:
>>>>>>>> On 13.07.2016 00:26, Michael Turquette wrote:
>>>>>>>>> Quoting Dirk Behme (2016-07-12 00:46:45)
>>>>>>>>>> Clocks described by this property are reserved for use by Xen, and
>>>>>>>>>> the OS
>>>>>>>>>> must not alter their state any way, such as disabling or gating a
>>>>>>>>>> clock,
>>>>>>>>>> or modifying its rate. Ensuring this may impose constraints on
>>>>>>>>>> parent
>>>>>>>>>> clocks or other resources used by the clock tree.
>>>>>>>>>
>>>>>>>>> Note that clk_prepare_enable will not prevent the rate from changing
>>>>>>>>> (clk_set_rate) or a parent from changing (clk_set_parent). The only
>>>>>>>>> way
>>>>>>>>> to do this currently would be to set the following flags on the
>>>>>>>>> effected
>>>>>>>>> clocks:
>>>>>>>>>
>>>>>>>>>     CLK_SET_RATE_GATE
>>>>>>>>>     CLK_SET_PARENT_GATE
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Regarding setting flags, I think we already talked about that. I think
>>>>>>>> the
>>>>>>>> conclusion was that in our case its not possible to manipulate the
>>>>>>>> flags in
>>>>>>>> the OS as this isn't intended to be done in cases like ours. Therefore
>>>>>>>> no API
>>>>>>>> is exported for this.
>>>>>>>>
>>>>>>>> I.e. if we need to set these flags, we have to do that in Xen where we
>>>>>>>> add the
>>>>>>>> clocks to the hypervisor node in the device tree. And not in the
>>>>>>>> kernel patch
>>>>>>>> discussed here.
>>>>>>>
>>>>>>> These are internal Linux flags, aren't they?
>>>>>>
>>>>>>
>>>>>> I've been under the impression that you can set clock "flags" via the
>>>>>> device tree. Seems I need to re-check that ;)
>>>>>
>>>>> Right, you cannot set flags from the device tree. Also, setting these
>>>>> flags is done by the clock provider driver, not a consumer. Xen is the
>>>>> consumer.
>>>>
>>>>
>>>> Ok, thanks, then I think we can forget about using flags for the issue we are
>>>> discussing here.
>>>>
>>>> Best regards
>>>>
>>>> Dirk
>>>>
>>>> P.S.: Would it be an option to merge the v4 patch we are discussing here,
>>>> then? From the discussion until here, it sounds to me that it's the best
>>>> option we have at the moment. Maybe improving it in the future, then.
>>>
>>> It might be a step in the right direction, but it doesn't really prevent
>>> clk_set_rate from changing properties of a clock owned by Xen.  This
>>> patch is incomplete. We need to understand at least what it would take
>>> to have a complete solution.
>>>
>>> Michael, do you have any suggestions on how it would be possible to set
>>> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for those clocks in a proper
>>> way?
>>
>> No, there is no way for a consumer to do that. The provider must do it.
>
> All right. But could we design a new device tree binding which the Xen
> hypervisor would use to politely ask the clock provider in Linux to set
> CLK_SET_RATE_GATE and CLK_SET_PARENT_GATE for a given clock?
>
> Xen would have to modify the DTB before booting Linux with the new
> binding.
>
>
>>> Like you wrote, I would imagine it needs to be done by the clock
>>> provider driver. Maybe to do that, it would be easier to have a new
>>> device tree property on the clock node, rather than listing phandle and
>>> clock-specifier pairs under the Xen node?
>>
>> Upon further reflection, I think that your clock consumer can probably
>> use clk_set_rate_range() to "lock" in a rate. This is good because it is
>> exactly what a clock consumer should do:
>>
>> 1) get the clk
>> 2) enable the clk
>> 3) set the required rate for the clock
>> 4) set rate range constraints, or conversely,
>> 5) lock in an exact rate; set the min/max rate to the same value
>>
>> The problem with this solution is that it requires the consumer to have
>> knowledge of the rates that it wants for that clock, which I guess is
>> something that Linux kernels in a Xen setup do not want/need?
>
> Who is usually the component with knowledge of the clock rate to set? If
> it's a device driver, then neither the Xen hypervisor, nor the Xen core
> drivers in Linux would know anything about it. (Unless the clock rate is
> specified on device tree via assigned-clock-rates of course.)
>
>
>> Is it correct that you would prefer some sort of never_touch_this_clk()
>> api?
>
>>From my understading, yes, never_touch_this_clk() would make things easier.


Would it be somehow worth to wait for anything like this 
never_touch_this_clk() api? Or should we try to proceed with 
clk_prepare_enable() like done in this patch for the moment?

Best regards

Dirk

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel