Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1210586 > unrolled thread

Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

Started byMaxime Ripard <maxime.ripard@free-electrons.com>
First post2015-08-20 17:20 +0200
Last post2015-08-29 06:00 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Maxime Ripard <maxime.ripard@free-electrons.com> - 2015-08-20 17:20 +0200
    Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Lee Jones <lee.jones@linaro.org> - 2015-08-26 09:00 +0200
      Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Maxime Coquelin <maxime.coquelin@st.com> - 2015-08-26 10:50 +0200
        Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Lee Jones <lee.jones@linaro.org> - 2015-08-26 11:20 +0200
          Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Maxime Coquelin <maxime.coquelin@st.com> - 2015-08-26 11:40 +0200
            Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Lee Jones <lee.jones@linaro.org> - 2015-08-26 22:50 +0200
      Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Maxime Ripard <maxime.ripard@free-electrons.com> - 2015-08-29 05:50 +0200
    Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and  implement hand-off Maxime Ripard <maxime.ripard@free-electrons.com> - 2015-08-29 06:00 +0200

#1210586 — Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2015-08-20 17:20 +0200
SubjectRe: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Message-ID<pZzNg-fs-5@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> Quoting Maxime Ripard (2015-08-18 08:45:52)
> > Hi Mike,
> > 
> > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > to the framework when the clock consumer driver calls clk_put) was left
> > > out because I do not see a real use case for it. If one can demonstrate
> > > a real use case (and not a hypothetical one) then this patch series can
> > > be expanded further.
> > 
> > I think there is a very trivial use case for passing back the
> > reference to the framework, if during the probed, we have something
> > like:
> > 
> > clk = clk_get()
> > clk_prepare_enable(clk)
> > foo_framework_register()
> > 
> > if foo_framework_register fails, the sensible thing to do would be to
> > call clk_disable_unprepare. If the clock was a critical clock, you
> > just gated it.
> 
> Hmm, a good point. Creating the "pass the reference back" call is not
> hard technically. But how to keep from abusing it? E.g. I do not want
> that call to become an alternative to correct use of clk_enable.
> 
> Maybe I'll need a Coccinelle script or just some regular sed to
> occasionally search for new users of this api and audit them?
> 
> I was hoping to not add any new consumer api at all :-/

I don't think there's any abuse that can be done with the current API,
nor do I think you need to have new functions either.

If the clock is critical, when the customer calls
clk_unprepare_disable on it, simply take back the reference you gave
in the framework, and you're done. Or am I missing something?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [next] | [standalone]


#1213586

FromLee Jones <lee.jones@linaro.org>
Date2015-08-26 09:00 +0200
Message-ID<q1CQG-6AJ-15@gated-at.bofh.it>
In reply to#1210586
On Tue, 25 Aug 2015, Michael Turquette wrote:

> Quoting Maxime Ripard (2015-08-20 08:15:10)
> > On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> > > Quoting Maxime Ripard (2015-08-18 08:45:52)
> > > > Hi Mike,
> > > > 
> > > > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > > > to the framework when the clock consumer driver calls clk_put) was left
> > > > > out because I do not see a real use case for it. If one can demonstrate
> > > > > a real use case (and not a hypothetical one) then this patch series can
> > > > > be expanded further.
> > > > 
> > > > I think there is a very trivial use case for passing back the
> > > > reference to the framework, if during the probed, we have something
> > > > like:
> > > > 
> > > > clk = clk_get()
> > > > clk_prepare_enable(clk)
> > > > foo_framework_register()
> > > > 
> > > > if foo_framework_register fails, the sensible thing to do would be to
> > > > call clk_disable_unprepare. If the clock was a critical clock, you
> > > > just gated it.
> > > 
> > > Hmm, a good point. Creating the "pass the reference back" call is not
> > > hard technically. But how to keep from abusing it? E.g. I do not want
> > > that call to become an alternative to correct use of clk_enable.
> > > 
> > > Maybe I'll need a Coccinelle script or just some regular sed to
> > > occasionally search for new users of this api and audit them?
> > > 
> > > I was hoping to not add any new consumer api at all :-/
> > 
> > I don't think there's any abuse that can be done with the current API,
> > nor do I think you need to have new functions either.
> > 
> > If the clock is critical, when the customer calls
> > clk_unprepare_disable on it, simply take back the reference you gave
> > in the framework, and you're done. Or am I missing something?
> 
> Maybe I am the one missing something? My goal was to allow the consumer
> driver to gate the critical clock. So we need clk_disable_unused to
> actually disable the clock for that to work.
> 
> I think you are suggesting that clk_disable_unused should *not* disable
> the clock if it is critical. Can you confirm that?

My take is that a critical clock should only be disabled when a
knowledgeable driver wants to gate it for a specific purpose [probably
using clk_disable()].  Once the aforementioned driver no longer has a
use for the clock [whether that happens with clk_unprepare_disable()
or clk_put() ...] the clock should be ungated and be provided with
critical status once more.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1213671

FromMaxime Coquelin <maxime.coquelin@st.com>
Date2015-08-26 10:50 +0200
Message-ID<q1Ez8-Dv-3@gated-at.bofh.it>
In reply to#1213586
Hi Lee,

On 08/26/2015 08:54 AM, Lee Jones wrote:
> On Tue, 25 Aug 2015, Michael Turquette wrote:
>
>
>> Maybe I am the one missing something? My goal was to allow the consumer
>> driver to gate the critical clock. So we need clk_disable_unused to
>> actually disable the clock for that to work.
>>
>> I think you are suggesting that clk_disable_unused should *not* disable
>> the clock if it is critical. Can you confirm that?
> My take is that a critical clock should only be disabled when a
> knowledgeable driver wants to gate it for a specific purpose [probably
> using clk_disable()].  Once the aforementioned driver no longer has a
> use for the clock [whether that happens with clk_unprepare_disable()
> or clk_put() ...] the clock should be ungated and be provided with
> critical status once more.
>
How do you differentiate between a knowledgeable and non-knowledgeable 
driver?
Let's take the example of the clock used by the i2c on STi SoCs.
This clock is used by i2c, and is also critical to the system, but only 
i2c takes it.

At first transfer, the i2c will enable the clock and then disables it.

What we would expect here is that the clk_disable does not gate the 
clock, even if only user since the hand-off flag has been set.
Else, system will freeze.

Maxime
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1213688

FromLee Jones <lee.jones@linaro.org>
Date2015-08-26 11:20 +0200
Message-ID<q1F2a-1qN-21@gated-at.bofh.it>
In reply to#1213671
On Wed, 26 Aug 2015, Maxime Coquelin wrote:

> Hi Lee,
> 
> On 08/26/2015 08:54 AM, Lee Jones wrote:
> >On Tue, 25 Aug 2015, Michael Turquette wrote:
> >
> >
> >>Maybe I am the one missing something? My goal was to allow the consumer
> >>driver to gate the critical clock. So we need clk_disable_unused to
> >>actually disable the clock for that to work.
> >>
> >>I think you are suggesting that clk_disable_unused should *not* disable
> >>the clock if it is critical. Can you confirm that?
> >My take is that a critical clock should only be disabled when a
> >knowledgeable driver wants to gate it for a specific purpose [probably
> >using clk_disable()].  Once the aforementioned driver no longer has a
> >use for the clock [whether that happens with clk_unprepare_disable()
> >or clk_put() ...] the clock should be ungated and be provided with
> >critical status once more.
> >
> How do you differentiate between a knowledgeable and
> non-knowledgeable driver?
> Let's take the example of the clock used by the i2c on STi SoCs.
> This clock is used by i2c, and is also critical to the system, but
> only i2c takes it.
> 
> At first transfer, the i2c will enable the clock and then disables it.
> 
> What we would expect here is that the clk_disable does not gate the
> clock, even if only user since the hand-off flag has been set.
> Else, system will freeze.

The I2C driver in this instance is not a knowledgeable driver and
should not be taking a reference to a critical clock.

In the example you provide, the real issue is that the I2C driver uses
one of the critical clock's siblings.  Without this framework, if it
gives up the reference to its own clock and there are no users of any
sibling clocks, the parent is gated.  This has the unfortunate effect
of gating the entire family, critical clock included.

These sorts of issues are precisely what we're trying to fix here.

For clarification, a knowledgeable driver is one that requests an
actual (not a sibling of) critical clock.  It's knowledgeable in the
fact that it knows what gating the clock will do to the system, but it
"knows best" that this is actually fine.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1213705

FromMaxime Coquelin <maxime.coquelin@st.com>
Date2015-08-26 11:40 +0200
Message-ID<q1Flw-1N4-7@gated-at.bofh.it>
In reply to#1213688

On 08/26/2015 11:09 AM, Lee Jones wrote:
> On Wed, 26 Aug 2015, Maxime Coquelin wrote:
>
>> Hi Lee,
>>
>> On 08/26/2015 08:54 AM, Lee Jones wrote:
>>> On Tue, 25 Aug 2015, Michael Turquette wrote:
>>>
>>>
>>>> Maybe I am the one missing something? My goal was to allow the consumer
>>>> driver to gate the critical clock. So we need clk_disable_unused to
>>>> actually disable the clock for that to work.
>>>>
>>>> I think you are suggesting that clk_disable_unused should *not* disable
>>>> the clock if it is critical. Can you confirm that?
>>> My take is that a critical clock should only be disabled when a
>>> knowledgeable driver wants to gate it for a specific purpose [probably
>>> using clk_disable()].  Once the aforementioned driver no longer has a
>>> use for the clock [whether that happens with clk_unprepare_disable()
>>> or clk_put() ...] the clock should be ungated and be provided with
>>> critical status once more.
>>>
>> How do you differentiate between a knowledgeable and
>> non-knowledgeable driver?
>> Let's take the example of the clock used by the i2c on STi SoCs.
>> This clock is used by i2c, and is also critical to the system, but
>> only i2c takes it.
>>
>> At first transfer, the i2c will enable the clock and then disables it.
>>
>> What we would expect here is that the clk_disable does not gate the
>> clock, even if only user since the hand-off flag has been set.
>> Else, system will freeze.
> The I2C driver in this instance is not a knowledgeable driver and
> should not be taking a reference to a critical clock.
This is the case:
         i2c@9840000 {
             ...
             clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
             clock-names = "ssc";
             ...
         }

CLK_EXT2F_A9 is a critical clock I think.
Indeed, this clock corresponds to output 13 of clockgen c0.
This ouput has several clock names in the datasheet, but is in reality 
the same clock from HW point of view (i.e. "same wire"):

- CLK_ICN_REG
- CLK_TRACE_A9
- CLK_PTI_STM
- CLK_EXT2F_A9

I'm pretty sure CLK_ICN_REG is a critical clock.

Try to gate it without gating its parent, and see if system is still alive.


>
> In the example you provide, the real issue is that the I2C driver uses
> one of the critical clock's siblings.  Without this framework, if it
> gives up the reference to its own clock and there are no users of any
> sibling clocks, the parent is gated.  This has the unfortunate effect
> of gating the entire family, critical clock included.
I don't see why a clock used by i2c could not be a critical clock, if it 
is used by other parts of the system that cannot be represented as 
drivers, and rely on the clock to be always on.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1214182

FromLee Jones <lee.jones@linaro.org>
Date2015-08-26 22:50 +0200
Message-ID<q1PNU-8kE-13@gated-at.bofh.it>
In reply to#1213705
Mike, Maxime, Maxime,

On Wed, 26 Aug 2015, Maxime Coquelin wrote:
> On 08/26/2015 11:09 AM, Lee Jones wrote:
> >On Wed, 26 Aug 2015, Maxime Coquelin wrote:
> >>On 08/26/2015 08:54 AM, Lee Jones wrote:
> >>>On Tue, 25 Aug 2015, Michael Turquette wrote:
> >>>
> >>>>Maybe I am the one missing something? My goal was to allow the consumer
> >>>>driver to gate the critical clock. So we need clk_disable_unused to
> >>>>actually disable the clock for that to work.
> >>>>
> >>>>I think you are suggesting that clk_disable_unused should *not* disable
> >>>>the clock if it is critical. Can you confirm that?
> >>>My take is that a critical clock should only be disabled when a
> >>>knowledgeable driver wants to gate it for a specific purpose [probably
> >>>using clk_disable()].  Once the aforementioned driver no longer has a
> >>>use for the clock [whether that happens with clk_unprepare_disable()
> >>>or clk_put() ...] the clock should be ungated and be provided with
> >>>critical status once more.
> >>>
> >>How do you differentiate between a knowledgeable and
> >>non-knowledgeable driver?
> >>Let's take the example of the clock used by the i2c on STi SoCs.
> >>This clock is used by i2c, and is also critical to the system, but
> >>only i2c takes it.
> >>
> >>At first transfer, the i2c will enable the clock and then disables it.
> >>
> >>What we would expect here is that the clk_disable does not gate the
> >>clock, even if only user since the hand-off flag has been set.
> >>Else, system will freeze.
> >The I2C driver in this instance is not a knowledgeable driver and
> >should not be taking a reference to a critical clock.
> This is the case:
>         i2c@9840000 {
>             ...
>             clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
>             clock-names = "ssc";
>             ...
>         }
> 
> CLK_EXT2F_A9 is a critical clock I think.
> Indeed, this clock corresponds to output 13 of clockgen c0.
> This ouput has several clock names in the datasheet, but is in
> reality the same clock from HW point of view (i.e. "same wire"):
> 
> - CLK_ICN_REG
> - CLK_TRACE_A9
> - CLK_PTI_STM
> - CLK_EXT2F_A9
> 
> I'm pretty sure CLK_ICN_REG is a critical clock.
> 
> Try to gate it without gating its parent, and see if system is still alive.
> 
> >In the example you provide, the real issue is that the I2C driver uses
> >one of the critical clock's siblings.  Without this framework, if it
> >gives up the reference to its own clock and there are no users of any
> >sibling clocks, the parent is gated.  This has the unfortunate effect
> >of gating the entire family, critical clock included.
> 
> I don't see why a clock used by i2c could not be a critical clock,
> if it is used by other parts of the system that cannot be
> represented as drivers, and rely on the clock to be always on.

This is actually a great point and one that slipped my mind recently.
Handing-off a critical clock to the first requester will break our
platform.  It's part of the reason I set-up a special API.

To summarise:

In the beginning we were faced with an issue where unclaimed, but
still required clocks were being gated on start-up.  This was due to
the 'disable-unused' functionality used for power saving.  This was
tackled in one of two ways; either turn it off completely using the
'clk_ignore_unused' kernel command line parameter or on a per-clock
bases using a flag in C code.

Even with 'clk_ignore_unused' provided, drivers were able to gate
clocks critical to the running of the system by either gating their
siblings or the critical clock itself if it was shared with other
users.  It was this issue that prompted the creation of this set's
predecessor.

Although the original set worked, there were two shortcomings.
Firstly, it created a imbalance in the internal framework reference
counting.  Something that wasn't an issue at the time, but would
become an issue once Mike had authored and submitted his per-clock
reference counting patch set.  It also didn't allow "knowledgeable"
drivers (ones which knew the risks of gating a critical clock, but
knew better, and that it was okay to do so) to adopt the clock in
order to disable it.

So now we have this new set, where the priorities seem to have been
reversed.  It solves the issue of clock adoption by knowledgeable
drivers, but it suffers from the same symptoms as the ones which
prompted this functionality in the first place.  If, let's call it
an "uninformed" driver requests a critical clock using the current
API, the critical clock will be handed over, then the uninformed
driver is free to gate and ungate it as it sees fit.  The issue is
that the first call to clk_disable() will bork the running platform
irrecoverably.

We've already made it quite clear that we shouldn't be coding for
hypothetical situations.  So why do we even have this hand-off
feature?  I'm not aware of any knowledgeable drivers which do think
it's a good idea to gate a critical clock.  Are there any?

The hand-off feature was only mentioned because we were marking clocks
as critical in DT.  And due to the fact that DTBs sometimes get
separated (out dated) from the kernel, we needed this as a fall-back
plan to gate clocks previously thought to be critical at a later date.
However, this implementation doesn't even have DT support.  So to fix
this problem you could just un-flag the clock as critical, no?

From a personal PoV, this set has all the features we don't need and
none of the ones we do.

I would like to suggest once more that if you wanted to keep this
adoption/hand-off feature that we do so in a cleaner (i.e. have proper
functions that deal with this stuff as opposed to shoehorning extra
code into existing functions) and more deliberate (i.e. insist that a
driver identify itself as 'knowledgeable', rather than 'uninformed' by
way of a specific call, clk_get_critical() for instance) way.

How do you propose we move forward?  Would be be okay with me having
another stab at this?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1215674

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2015-08-29 05:50 +0200
Message-ID<q2Fjr-6Q1-11@gated-at.bofh.it>
In reply to#1213586

[Multipart message — attachments visible in raw view] — view raw

On Wed, Aug 26, 2015 at 07:54:23AM +0100, Lee Jones wrote:
> On Tue, 25 Aug 2015, Michael Turquette wrote:
> 
> > Quoting Maxime Ripard (2015-08-20 08:15:10)
> > > On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> > > > Quoting Maxime Ripard (2015-08-18 08:45:52)
> > > > > Hi Mike,
> > > > > 
> > > > > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > > > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > > > > to the framework when the clock consumer driver calls clk_put) was left
> > > > > > out because I do not see a real use case for it. If one can demonstrate
> > > > > > a real use case (and not a hypothetical one) then this patch series can
> > > > > > be expanded further.
> > > > > 
> > > > > I think there is a very trivial use case for passing back the
> > > > > reference to the framework, if during the probed, we have something
> > > > > like:
> > > > > 
> > > > > clk = clk_get()
> > > > > clk_prepare_enable(clk)
> > > > > foo_framework_register()
> > > > > 
> > > > > if foo_framework_register fails, the sensible thing to do would be to
> > > > > call clk_disable_unprepare. If the clock was a critical clock, you
> > > > > just gated it.
> > > > 
> > > > Hmm, a good point. Creating the "pass the reference back" call is not
> > > > hard technically. But how to keep from abusing it? E.g. I do not want
> > > > that call to become an alternative to correct use of clk_enable.
> > > > 
> > > > Maybe I'll need a Coccinelle script or just some regular sed to
> > > > occasionally search for new users of this api and audit them?
> > > > 
> > > > I was hoping to not add any new consumer api at all :-/
> > > 
> > > I don't think there's any abuse that can be done with the current API,
> > > nor do I think you need to have new functions either.
> > > 
> > > If the clock is critical, when the customer calls
> > > clk_unprepare_disable on it, simply take back the reference you gave
> > > in the framework, and you're done. Or am I missing something?
> > 
> > Maybe I am the one missing something? My goal was to allow the consumer
> > driver to gate the critical clock. So we need clk_disable_unused to
> > actually disable the clock for that to work.
> > 
> > I think you are suggesting that clk_disable_unused should *not* disable
> > the clock if it is critical. Can you confirm that?
> 
> My take is that a critical clock should only be disabled when a
> knowledgeable driver wants to gate it for a specific purpose [probably
> using clk_disable()].  Once the aforementioned driver no longer has a
> use for the clock [whether that happens with clk_unprepare_disable()
> or clk_put() ...] the clock should be ungated and be provided with
> critical status once more.

Agreed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]


#1215681

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2015-08-29 06:00 +0200
Message-ID<q2Ft8-71h-11@gated-at.bofh.it>
In reply to#1210586

[Multipart message — attachments visible in raw view] — view raw

Hi Mike,

On Tue, Aug 25, 2015 at 02:50:51PM -0700, Michael Turquette wrote:
> Quoting Maxime Ripard (2015-08-20 08:15:10)
> > On Tue, Aug 18, 2015 at 09:43:56AM -0700, Michael Turquette wrote:
> > > Quoting Maxime Ripard (2015-08-18 08:45:52)
> > > > Hi Mike,
> > > > 
> > > > On Fri, Aug 07, 2015 at 12:09:27PM -0700, Michael Turquette wrote:
> > > > > All of the other kitchen sink stuff (DT binding, passing the flag back
> > > > > to the framework when the clock consumer driver calls clk_put) was left
> > > > > out because I do not see a real use case for it. If one can demonstrate
> > > > > a real use case (and not a hypothetical one) then this patch series can
> > > > > be expanded further.
> > > > 
> > > > I think there is a very trivial use case for passing back the
> > > > reference to the framework, if during the probed, we have something
> > > > like:
> > > > 
> > > > clk = clk_get()
> > > > clk_prepare_enable(clk)
> > > > foo_framework_register()
> > > > 
> > > > if foo_framework_register fails, the sensible thing to do would be to
> > > > call clk_disable_unprepare. If the clock was a critical clock, you
> > > > just gated it.
> > > 
> > > Hmm, a good point. Creating the "pass the reference back" call is not
> > > hard technically. But how to keep from abusing it? E.g. I do not want
> > > that call to become an alternative to correct use of clk_enable.
> > > 
> > > Maybe I'll need a Coccinelle script or just some regular sed to
> > > occasionally search for new users of this api and audit them?
> > > 
> > > I was hoping to not add any new consumer api at all :-/
> > 
> > I don't think there's any abuse that can be done with the current API,
> > nor do I think you need to have new functions either.
> > 
> > If the clock is critical, when the customer calls
> > clk_unprepare_disable on it, simply take back the reference you gave
> > in the framework, and you're done. Or am I missing something?
> 
> Maybe I am the one missing something? My goal was to allow the consumer
> driver to gate the critical clock. So we need clk_disable_unused to
> actually disable the clock for that to work.

Yeah, but I guess the consumer driver clock gating is not the default
mode of operations.

Under normal circumstances, it should just always leave the clock
enabled, all the time.

> I think you are suggesting that clk_disable_unused should *not* disable
> the clock if it is critical. Can you confirm that?

By default, yes.

Now, we also have the knowledgeable driver case wanting to force the
clock gating. I think it's an orthogonal issue, we might have the same
use case for non-critical clocks, and since it's hard to get that done
with the current API, and that we don't really know what a
knowledgeable driver will look like at this point, maybe we can just
delay this entirely until we actually have one in front of us?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web