From: Dirk Behme <dirk.behme@de.bosch.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
devicetree@vger.kernel.org,
Stefano Stabellini <sstabellini@kernel.org>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@codeaurora.org>,
Dirk Behme <dirk.behme@gmail.com>,
xen-devel@lists.xenproject.org, linux-clk@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4] xen/arm: Add a clock property
Date: Fri, 15 Jul 2016 09:53:09 +0200 [thread overview]
Message-ID: <2bc46969-544d-3e07-a7f6-83895703d11a__8081.32558963752$1468569289$gmane$org@de.bosch.com> (raw)
In-Reply-To: <5787C88A.8030103@arm.com>
On 14.07.2016 19:14, Julien Grall wrote:
> Hello,
>
> On 14/07/16 17:30, Dirk Behme wrote:
>> On 14.07.2016 17:55, Stefano Stabellini wrote:
>>> On Thu, 14 Jul 2016, Dirk Behme wrote:
>>>> On 14.07.2016 12:38, Stefano Stabellini wrote:
>>>>> 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.
>>>>
>>>>
>>>> Let me ask then: Do we have a practical example where it's not
>>>> sufficient
>>>> practically?
>>>>
>>>> To my understanding, Xen people have been happy with the
>>>> "clk_ignore_unused"
>>>> workaround since ~2 years, now [1]. And I think the "clk_ignore_unused"
>>>> workaround does mainly the same like the patch discussed here. It
>>>> doesn't care
>>>> regarding clk_set_rate from changing properties, too?
>>>
>>> Let me premise that I appreciate what you are trying to achieve with
>>> this patch and I don't want to feature-creep it.
>>>
>>> However we are defining a new Device Tree binding,
>>
>>
>> I don't think so.
>>
>> We are just using the existing one
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n66
>>
>>
>>
>> , pick it from other device tree nodes (e.g. serial, timer etc) and add
>> it to the hypervisor node. And then use this existing one with the
>> existing well defined clock API.
>>
>>
>>> one which will have
>>> to be supported for a long time by both Xen and Linux, so at the very
>>> least we need to have the full picture. We need to understand if the
>>> binding if sufficient
>>
>>
>> Even if it's not sufficient, you can't change it.
>
> I think you misunderstood Stefano's comment. Whilst the clock bindings
> is set in stone, the binding you are adding in this patch is not yet
> fixed.
It has to be
a) identical what we pick from UART, timer, etc
b) compatible to the kernel's clock API
No?
With these two requirements I have some difficulties to imagine how it
could be different to the clock binding from clock-bindings.txt?
> There is no requirement to follow what was defined in
> clock-bindings.txt. I agree it would be convenient, but as mentioned by
> Stefano this will need to be supported for a long time by Xen, Linux,
> and any other consumer (i.e BSD kernels).
Sounds like an additional argument for clock-bindings.txt ;)
> So we have to be careful on
> how it has been defined.
>
> I would wait the answer of Michael on Stefano's question before taking
> any decision here.
Fine with me :)
Best regards
Dirk
>>> or if we need something different to solve the
>>> problem completely.
>>
>>
>> You might need anything additionally. E.g. an extension of the Linux
>> kernel clock API to be able to modify the flags was proposed.
>
> The Linux kernel is not the only consumer of the device tree bindings.
> This is also used by other OS such as FreeBSD where you might already be
> able (I have not actually checked) to forbid a user to change the clock
> rate.
>
>>
>> Best regards
>>
>> Dirk
>>
>> P.S.: I still would be interested if we do have a practical example
>> where it's not sufficient practically?
>
> Very easy. What does prevent a driver to change the clock rate? Nothing
> but the flags mentioned by Michael. There are already drivers which
> modify the clock rate, thankfully those clocks are not shared with the
> UART for now.
>
> But we cannot rule out that it will not be possible in the future. Think
> about a clock that would be used by another guest (I know it is still
> theoretical as we have not yet solved the problem).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-07-15 7:53 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1468309605-19522-1-git-send-email-dirk.behme@de.bosch.com>
2016-07-12 22:26 ` [PATCH v4] xen/arm: Add a clock property Michael Turquette
[not found] ` <146836236759.73491.11707389619985827497@resonance>
2016-07-13 8:35 ` Dirk Behme
[not found] ` <c320f820-8749-f4a4-435e-ddd9379655b5@de.bosch.com>
2016-07-13 18:43 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1607131941200.26575@sstabellini-ThinkPad-X260>
2016-07-13 18:56 ` Dirk Behme
[not found] ` <57868EDE.6060406@gmail.com>
2016-07-13 21:03 ` Michael Turquette
[not found] ` <146844380895.73491.4867379517577413421@resonance>
2016-07-14 6:31 ` Dirk Behme
[not found] ` <7df784ab-d0c0-939b-393e-214535c4b191@de.bosch.com>
2016-07-14 10:14 ` Julien Grall
[not found] ` <57876612.3030407@arm.com>
2016-07-14 10:32 ` Dirk Behme
2016-07-14 10:38 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1607141132000.26575@sstabellini-ThinkPad-X260>
2016-07-14 10:49 ` Dirk Behme
[not found] ` <86951b96-0a3e-79b1-1a2f-88dfeba9b76b@de.bosch.com>
2016-07-14 15:55 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1607141647350.22290@sstabellini-ThinkPad-X260>
2016-07-14 16:30 ` Dirk Behme
[not found] ` <5787BE0B.8060504@gmail.com>
2016-07-14 17:14 ` Julien Grall
[not found] ` <5787C88A.8030103@arm.com>
2016-07-15 7:53 ` Dirk Behme [this message]
2016-07-22 0:07 ` Michael Turquette
[not found] ` <146914607316.8780.7961396342647226841@resonance>
2016-07-22 1:16 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1607211804000.6667@sstabellini-ThinkPad-X260>
2016-07-27 5:05 ` Dirk Behme
[not found] ` <dbd46fe5-6f53-d11e-6363-a2c9905d6c8e@gmail.com>
2016-07-28 11:17 ` Julien Grall
[not found] ` <1a486d3b-0dda-d545-ca6a-031a8bf932e9@arm.com>
2016-07-28 14:35 ` Dirk Behme
2016-07-14 10:25 ` Stefano Stabellini
2016-07-13 18:35 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1607131932530.26575@sstabellini-ThinkPad-X260>
2016-07-13 18:47 ` Dirk Behme
[not found] ` <57868CB4.1090504@gmail.com>
2016-07-13 19:07 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1607132002000.26575@sstabellini-ThinkPad-X260>
2016-07-14 6:11 ` Dirk Behme
[not found] ` <094be367-ff91-aec7-cbd6-cfb8dc711c54@de.bosch.com>
2016-07-14 10:28 ` Stefano Stabellini
2016-07-20 9:43 ` Geert Uytterhoeven
[not found] ` <CAMuHMdU9yP2SMYa5ssrQSu1vwSN2gM4mD-EVYSGr2Pqf=jmCgw@mail.gmail.com>
2016-07-20 11:01 ` Julien Grall
[not found] ` <cc28ef7e-dce3-fe5f-ab80-b662244bcbd0@arm.com>
2016-07-20 11:49 ` Geert Uytterhoeven
[not found] ` <CAMuHMdWeJjcuBHjnjppc5Ys5Ew8VssXzg=dLpLPCOQaBnUo_7Q@mail.gmail.com>
2016-07-20 12:10 ` Julien Grall
[not found] ` <04d9dbfb-196d-a775-7fbb-526aba8085f4@arm.com>
2016-07-20 12:46 ` Geert Uytterhoeven
[not found] ` <CAMuHMdWdqdT4jnae8h9tDDzzh8ZAbJJwiMW-scPBNVwmjYSiWQ@mail.gmail.com>
2016-07-20 12:53 ` Dirk Behme
2016-07-20 13:21 ` Julien Grall
[not found] ` <457c61fd-4a64-b050-cd87-845b7e22ec39@arm.com>
2016-07-22 0:14 ` Michael Turquette
2016-07-12 7:46 Dirk Behme
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='2bc46969-544d-3e07-a7f6-83895703d11a__8081.32558963752$1468569289$gmane$org@de.bosch.com' \
--to=dirk.behme@de.bosch.com \
--cc=devicetree@vger.kernel.org \
--cc=dirk.behme@gmail.com \
--cc=julien.grall@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mturquette@baylibre.com \
--cc=sboyd@codeaurora.org \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).