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

[Dirk Behme <dirk.behme@xxxxxxxxxxxx>]

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?
While I agree that the patch theoretically incomplete, if nobody has a 
real world example I would think that from practical point of view it's 
sufficient in a first step.
If this is the case, I'd propose to fix the practical issue in a first 
step with a patch (this one) which is sufficient to fix the issues the 
Xen users have. And update the code for theoretical future issues in a 
second step.
Best regards

Dirk

[1] http://bugs.xenproject.org/xen/bug/45


> 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?
>
> 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?
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel