From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dirk Behme Subject: Re: [Xen-devel] [PATCH v4] xen/arm: Add a clock property Date: Thu, 28 Jul 2016 16:35:52 +0200 Message-ID: References: <1468309605-19522-1-git-send-email-dirk.behme@de.bosch.com> <146836236759.73491.11707389619985827497@resonance> <57868EDE.6060406@gmail.com> <146844380895.73491.4867379517577413421@resonance> <7df784ab-d0c0-939b-393e-214535c4b191@de.bosch.com> <146914607316.8780.7961396342647226841@resonance> <1a486d3b-0dda-d545-ca6a-031a8bf932e9@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1a486d3b-0dda-d545-ca6a-031a8bf932e9@arm.com> Sender: linux-clk-owner@vger.kernel.org To: Julien Grall , Michael Turquette Cc: Stefano Stabellini , Dirk Behme , Mark Rutland , devicetree@vger.kernel.org, Stephen Boyd , xen-devel@lists.xenproject.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: dirk.behme@gmail.com (Dirk Behme) Date: Thu, 28 Jul 2016 16:35:52 +0200 Subject: [Xen-devel] [PATCH v4] xen/arm: Add a clock property In-Reply-To: <1a486d3b-0dda-d545-ca6a-031a8bf932e9@arm.com> References: <1468309605-19522-1-git-send-email-dirk.behme@de.bosch.com> <146836236759.73491.11707389619985827497@resonance> <57868EDE.6060406@gmail.com> <146844380895.73491.4867379517577413421@resonance> <7df784ab-d0c0-939b-393e-214535c4b191@de.bosch.com> <146914607316.8780.7961396342647226841@resonance> <1a486d3b-0dda-d545-ca6a-031a8bf932e9@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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