[Xen-devel] [PATCH v4] xen/arm: Add a clock property

[Dirk Behme]

On 28.07.2016 13:17, Julien Grall wrote:
> Hi Dirk,
>
> On 27/07/16 06:05, Dirk Behme wrote:
>> 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?
>
> I am not sure who will write the new api never_touch_this_clk(). Could
> you suggest an implementation based on the discussion?


As this was a proposal from Michael, I'm hoping for Michael here, 
somehow ;) At least for a hint if anything like never_touch_this_clk() 
would be realistic to get accepted. And if so, how this could look like.

If this is unrealistic, I think we should go the proposed 
clk_prepare_enable() way, as it seems this is the best we could do at 
the moment without never_touch_this_clk().

Best regards

Dirk