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

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

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?

Regards,

>
> Best regards
>
> Dirk
>

-- 
Julien Grall