On Thu, 14 Jul 2016, Dirk Behme wrote:

On 14.07.2016 12:38, Stefano Stabellini wrote:

quoted

On Thu, 14 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 23:03, Michael Turquette wrote:

quoted

Quoting Dirk Behme (2016-07-13 11:56:30)

quoted

On 13.07.2016 20:43, Stefano Stabellini wrote:

quoted

On Wed, 13 Jul 2016, Dirk Behme wrote:

quoted

On 13.07.2016 00:26, Michael Turquette wrote:

quoted

Quoting Dirk Behme (2016-07-12 00:46:45)

quoted

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, 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 or if we need something different to solve the
problem completely.

Once we understand that, I am happy to accept a partial implementation
in Linux, as long as it is a step in the right direction. Does it make
sense?

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

quoted

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?