Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Quoting Michael Turquette (2015-12-04 16:46:28) > Heiko Stübner wrote: > > Hi Mike, > > > > Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette: > > > This is an alternative solution to Lee's "clk: Provide support for > > > always-on clocks" series[0]. > > > > > > The first two patches introduce run-time checks to ensure that clock > > > consumer drivers are respecting the clk.h api. The former patch checks > > > for prepare and enable imbalances. The latter checks for calls to > > > clk_put without first disabling and unpreparing the clk. > > > > > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > > > prepares and enables a clk at registration-time. The reference counts > > > (prepare & enable) are transferred to the first clock consumer driver > > > that clk_get's the clk with this flag set AND calls clk_prepare or > > > clk_enable. > > > > > > The net result is that a clock with this flag set will be enabled at > > > boot and neither the clk_disable_unused garbage collector or the > > > "sibling clock disables a shared parent" scenario will cause the flagged > > > clock to be disabled. The first driver to come along and explicitly > > > claim, prepare and enable this clock will inherit those reference > > > counts. No change to clock consumer drivers is required for this to > > > work. Please continue to use the clk.h api properly. > > > > just out of curiosity, did this move anywhere yet? (Last message from > > october > > 1st it seems) > > > > It looks like it is needed to fix the orphan-deferral I need on Rockchip > > that > > breaks sunxi in its current state. > > Yes, I'm preparing another version. Sorry for high latency, but I've been > traveling for more than 2 months non-stop. Done. See: http://lkml.kernel.org/r/<1455225554-13267-1-git-send-email-mturque...@baylibre.com> Regards, Mike > > Regards, > Mike > > > > > > > Thanks > > Heiko > >
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Quoting Michael Turquette (2015-12-04 16:46:28) > Heiko Stübner wrote: > > Hi Mike, > > > > Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette: > > > This is an alternative solution to Lee's "clk: Provide support for > > > always-on clocks" series[0]. > > > > > > The first two patches introduce run-time checks to ensure that clock > > > consumer drivers are respecting the clk.h api. The former patch checks > > > for prepare and enable imbalances. The latter checks for calls to > > > clk_put without first disabling and unpreparing the clk. > > > > > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > > > prepares and enables a clk at registration-time. The reference counts > > > (prepare & enable) are transferred to the first clock consumer driver > > > that clk_get's the clk with this flag set AND calls clk_prepare or > > > clk_enable. > > > > > > The net result is that a clock with this flag set will be enabled at > > > boot and neither the clk_disable_unused garbage collector or the > > > "sibling clock disables a shared parent" scenario will cause the flagged > > > clock to be disabled. The first driver to come along and explicitly > > > claim, prepare and enable this clock will inherit those reference > > > counts. No change to clock consumer drivers is required for this to > > > work. Please continue to use the clk.h api properly. > > > > just out of curiosity, did this move anywhere yet? (Last message from > > october > > 1st it seems) > > > > It looks like it is needed to fix the orphan-deferral I need on Rockchip > > that > > breaks sunxi in its current state. > > Yes, I'm preparing another version. Sorry for high latency, but I've been > traveling for more than 2 months non-stop. Done. See: http://lkml.kernel.org/r/<1455225554-13267-1-git-send-email-mturque...@baylibre.com> Regards, Mike > > Regards, > Mike > > > > > > > Thanks > > Heiko > >
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Heiko Stübner wrote: > Hi Mike, > > Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette: > > This is an alternative solution to Lee's "clk: Provide support for > > always-on clocks" series[0]. > > > > The first two patches introduce run-time checks to ensure that clock > > consumer drivers are respecting the clk.h api. The former patch checks > > for prepare and enable imbalances. The latter checks for calls to > > clk_put without first disabling and unpreparing the clk. > > > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > > prepares and enables a clk at registration-time. The reference counts > > (prepare & enable) are transferred to the first clock consumer driver > > that clk_get's the clk with this flag set AND calls clk_prepare or > > clk_enable. > > > > The net result is that a clock with this flag set will be enabled at > > boot and neither the clk_disable_unused garbage collector or the > > "sibling clock disables a shared parent" scenario will cause the flagged > > clock to be disabled. The first driver to come along and explicitly > > claim, prepare and enable this clock will inherit those reference > > counts. No change to clock consumer drivers is required for this to > > work. Please continue to use the clk.h api properly. > > just out of curiosity, did this move anywhere yet? (Last message from october > 1st it seems) > > It looks like it is needed to fix the orphan-deferral I need on Rockchip that > breaks sunxi in its current state. Yes, I'm preparing another version. Sorry for high latency, but I've been traveling for more than 2 months non-stop. Regards, Mike > > > Thanks > Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Heiko Stübner wrote: > Hi Mike, > > Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette: > > This is an alternative solution to Lee's "clk: Provide support for > > always-on clocks" series[0]. > > > > The first two patches introduce run-time checks to ensure that clock > > consumer drivers are respecting the clk.h api. The former patch checks > > for prepare and enable imbalances. The latter checks for calls to > > clk_put without first disabling and unpreparing the clk. > > > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > > prepares and enables a clk at registration-time. The reference counts > > (prepare & enable) are transferred to the first clock consumer driver > > that clk_get's the clk with this flag set AND calls clk_prepare or > > clk_enable. > > > > The net result is that a clock with this flag set will be enabled at > > boot and neither the clk_disable_unused garbage collector or the > > "sibling clock disables a shared parent" scenario will cause the flagged > > clock to be disabled. The first driver to come along and explicitly > > claim, prepare and enable this clock will inherit those reference > > counts. No change to clock consumer drivers is required for this to > > work. Please continue to use the clk.h api properly. > > just out of curiosity, did this move anywhere yet? (Last message from october > 1st it seems) > > It looks like it is needed to fix the orphan-deferral I need on Rockchip that > breaks sunxi in its current state. Yes, I'm preparing another version. Sorry for high latency, but I've been traveling for more than 2 months non-stop. Regards, Mike > > > Thanks > Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Hi Mike, Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette: > This is an alternative solution to Lee's "clk: Provide support for > always-on clocks" series[0]. > > The first two patches introduce run-time checks to ensure that clock > consumer drivers are respecting the clk.h api. The former patch checks > for prepare and enable imbalances. The latter checks for calls to > clk_put without first disabling and unpreparing the clk. > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > prepares and enables a clk at registration-time. The reference counts > (prepare & enable) are transferred to the first clock consumer driver > that clk_get's the clk with this flag set AND calls clk_prepare or > clk_enable. > > The net result is that a clock with this flag set will be enabled at > boot and neither the clk_disable_unused garbage collector or the > "sibling clock disables a shared parent" scenario will cause the flagged > clock to be disabled. The first driver to come along and explicitly > claim, prepare and enable this clock will inherit those reference > counts. No change to clock consumer drivers is required for this to > work. Please continue to use the clk.h api properly. just out of curiosity, did this move anywhere yet? (Last message from october 1st it seems) It looks like it is needed to fix the orphan-deferral I need on Rockchip that breaks sunxi in its current state. Thanks Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Hi Mike, Am Freitag, 7. August 2015, 12:09:27 schrieb Michael Turquette: > This is an alternative solution to Lee's "clk: Provide support for > always-on clocks" series[0]. > > The first two patches introduce run-time checks to ensure that clock > consumer drivers are respecting the clk.h api. The former patch checks > for prepare and enable imbalances. The latter checks for calls to > clk_put without first disabling and unpreparing the clk. > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > prepares and enables a clk at registration-time. The reference counts > (prepare & enable) are transferred to the first clock consumer driver > that clk_get's the clk with this flag set AND calls clk_prepare or > clk_enable. > > The net result is that a clock with this flag set will be enabled at > boot and neither the clk_disable_unused garbage collector or the > "sibling clock disables a shared parent" scenario will cause the flagged > clock to be disabled. The first driver to come along and explicitly > claim, prepare and enable this clock will inherit those reference > counts. No change to clock consumer drivers is required for this to > work. Please continue to use the clk.h api properly. just out of curiosity, did this move anywhere yet? (Last message from october 1st it seems) It looks like it is needed to fix the orphan-deferral I need on Rockchip that breaks sunxi in its current state. Thanks Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
On Wed, Sep 30, 2015 at 05:36:49AM -0700, Michael Turquette wrote: > Quoting Maxime Ripard (2015-08-28 20:55:57) > > 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? > > Yes. I discussed this face to face with Lee last week at Linaro Connect. > I proposed the following: > > 1) support an always-on clk, which is enabled by the framework at > registration-time and can never be claimed and gated > > 2) support a hand-off clk, which is enabled by the framework at > registration-time and whose reference counts are handed off to the first > driver to get, prepare and enable this clk > > 3) forget about knowledgeable drivers because nobody needs this (yet) > > Lee was happy with this. Does it sound OK to you? Yep, it sounds good! See you in Dublin, Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
On Wed, Sep 30, 2015 at 05:36:49AM -0700, Michael Turquette wrote: > Quoting Maxime Ripard (2015-08-28 20:55:57) > > 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? > > Yes. I discussed this face to face with Lee last week at Linaro Connect. > I proposed the following: > > 1) support an always-on clk, which is enabled by the framework at > registration-time and can never be claimed and gated > > 2) support a hand-off clk, which is enabled by the framework at > registration-time and whose reference counts are handed off to the first > driver to get, prepare and enable this clk > > 3) forget about knowledgeable drivers because nobody needs this (yet) > > Lee was happy with this. Does it sound OK to you? Yep, it sounds good! See you in Dublin, Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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@984 {
> ...
> clocks = <_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
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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@984 {
...
clocks = <_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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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@984 {
...
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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@984 {
...
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
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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 signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
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. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Hi Mike,
On Tue, Aug 11, 2015 at 6:41 PM, Michael Turquette
wrote:
> Quoting Geert Uytterhoeven (2015-08-11 02:20:12)
>> On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
>> wrote:
>> > This is an alternative solution to Lee's "clk: Provide support for
>> > always-on clocks" series[0].
>> I gave it a try on r8a7791/koelsch, where I replaced the hack from
>> "[PATCH/RFC 1/5] clk: shmobile: mstp: Never disable INTC-SYS"
>> (http://www.spinics.net/lists/linux-sh/msg41107.html) by setting
>>
>> init.flags |= CLK_ENABLE_HAND_OFF
>>
>> in cpg_mstp_clock_register() for "intc-sys".
>>
>> The end result is fine (the "intc-sys" clock is never disabled), but I get
>> a few annoying lockdep splats like below (one for the "intc-sys" clock,
>> and one more for each parent up to the root clock):
>>
>> [ cut here ]
>> WARNING: CPU: 0 PID: 0 at drivers/clk/clk.c:745 clk_core_enable+0x6c/0xdc()
> Thanks much for testing! I forgot to hold the enable lock in __clk_init
> (we already hold the prepare lock). Can you tell me if this diff fixes
> it?
Yes it does. Thanks for the quick fix!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Hi Mike,
On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
wrote:
> This is an alternative solution to Lee's "clk: Provide support for
> always-on clocks" series[0].
>
> The first two patches introduce run-time checks to ensure that clock
> consumer drivers are respecting the clk.h api. The former patch checks
> for prepare and enable imbalances. The latter checks for calls to
> clk_put without first disabling and unpreparing the clk.
>
> The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
> prepares and enables a clk at registration-time. The reference counts
> (prepare & enable) are transferred to the first clock consumer driver
> that clk_get's the clk with this flag set AND calls clk_prepare or
> clk_enable.
>
> The net result is that a clock with this flag set will be enabled at
> boot and neither the clk_disable_unused garbage collector or the
> "sibling clock disables a shared parent" scenario will cause the flagged
> clock to be disabled. The first driver to come along and explicitly
> claim, prepare and enable this clock will inherit those reference
> counts. No change to clock consumer drivers is required for this to
> work. Please continue to use the clk.h api properly.
>
> In time this approach can probably replace the CLK_IGNORE_UNUSED flag
> and hopefully reduce the number of users of the clk_ignore_unused boot
> parameter.
Thanks for your series!
I gave it a try on r8a7791/koelsch, where I replaced the hack from
"[PATCH/RFC 1/5] clk: shmobile: mstp: Never disable INTC-SYS"
(http://www.spinics.net/lists/linux-sh/msg41107.html) by setting
init.flags |= CLK_ENABLE_HAND_OFF
in cpg_mstp_clock_register() for "intc-sys".
The end result is fine (the "intc-sys" clock is never disabled), but I get
a few annoying lockdep splats like below (one for the "intc-sys" clock,
and one more for each parent up to the root clock):
[ cut here ]
WARNING: CPU: 0 PID: 0 at drivers/clk/clk.c:745 clk_core_enable+0x6c/0xdc()
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.2.0-rc6-koelsch-04462-g27bac5e25174da01-dirty #1507
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
r6:c05c249d r5:0009 r4: r3:0020
[] (show_stack) from [] (dump_stack+0x78/0x94)
[] (dump_stack) from [] (warn_slowpath_common+0x90/0xbc)
r4: r3:
[] (warn_slowpath_common) from []
(warn_slowpath_null+0x24/0x2c)
r8:eec10d00 r7:0001 r6:eec0f8c0 r5: r4:eec0f8c0
[] (warn_slowpath_null) from [] (clk_core_enable+0x6c/0xdc)
[] (clk_core_enable) from [] (clk_register+0x504/0x62c)
r5: r4:eec0f8c0
[] (clk_register) from [] (cpg_mstp_clocks_init+0x240/0x310)
r10:c05c2f26 r9:0008 r8:eec10d00 r7:c0ff3755 r6:eec0f7c0 r5:ef1df1cc
r4:eec09080
[] (cpg_mstp_clocks_init) from [] (of_clk_init+0xe0/0x188)
r10:0002 r9:eec09100 r8:eec09108 r7:0001 r6:eec09140 r5:c0643f48
r4:
[] (of_clk_init) from [] (rcar_gen2_timer_init+0x108/0x120)
r10:c0633ae4 r9:c0644400 r8: r7: r6:ef7fca00 r5:f0006000
r4:00989680
[] (rcar_gen2_timer_init) from [] (time_init+0x24/0x38)
r5:c067c000 r4:
[] (time_init) from [] (start_kernel+0x268/0x378)
[] (start_kernel) from [<40008090>] (0x40008090)
r10: r9:413fc0f2 r8:40007000 r7:c0647fe8 r6:c0633ae0 r5:c0644480
r4:c067c394
---[ end trace cb88537fdc8fa200 ]---
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
On Mon, 10 Aug 2015, Michael Turquette wrote: > Quoting Lee Jones (2015-08-10 08:36:38) > > On Fri, 07 Aug 2015, Michael Turquette wrote: > > > This is an alternative solution to Lee's "clk: Provide support for > > > always-on clocks" series[0]. > > > > So after all the hours of work that's been put in and all of the > > versions that have been authored, you're just going to write your own > > solution anyway, without any further discussion? > > This certainly seems like "further discussion" to me. In fact the > capital C in RFC pretty much announces that this is a big, wide open > discussion. > > If I had ninja-merged the patches then I would understand your > frustration, but that clearly is not the case. This doesn't have anything to do with merging code. When contributors submit patches to subsystem I maintain, I provide reviews, advice and guidance. If I think an approach is wrong, I state why I think that and provide ideas for an alternative approach if required. If instead of providing feedback I just replied with a patch-set containing my own alternative method, I would expect the original submitter to be justifiably cross. > > That's one way to make your contributors feel valued. > > Lee, your contributions are very much valued. You and I both know that > there are times when a concept is far better communicated with code than > with prose. This is one of those times. The DT-centric solution that > requires a clock consumer driver to call clk_disable() before calling > clk_enable() (an obvious violation of the clk.h api) is just plain wrong > and this implementation hopes to get it back on the right course. See my previous email about what I think about "bastardizing the clk.h api". > > > The first two patches introduce run-time checks to ensure that clock > > > consumer drivers are respecting the clk.h api. The former patch checks > > > for prepare and enable imbalances. The latter checks for calls to > > > clk_put without first disabling and unpreparing the clk. > > > > Are these real issues that people are having trouble with, or just > > hypothetical ones? I can understand the need for them if they're > > causing people some pain, but if they are unnecessarily hardening the > > API, then it makes it difficult for proper issues (such as critical > > clocks) to be solved easily. > > They are deeply related. Per-user accounting gives us a graceful method > to implement hand-off, which is the real feature that is needed here. The hand-off feature was mentioned a week ago, as part of an 8 month discussion. This only aids knowledgeable consumers to turn of a critical clock after the introduction of the API tightening which happens in _this_ set. So I disagree that this is the "real feature that is needed here". The _real_ feature that's required is a generic way to keep critical clocks enabled and a way for them to be gated if another driver knows best. Granted this method is pretty clean for drivers which have all of their platform data in driver code, but it's still not ideal. > > > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > > > prepares and enables a clk at registration-time. The reference counts > > > (prepare & enable) are transferred to the first clock consumer driver > > > that clk_get's the clk with this flag set AND calls clk_prepare or > > > clk_enable. > > > > > > The net result is that a clock with this flag set will be enabled at > > > boot and neither the clk_disable_unused garbage collector or the > > > "sibling clock disables a shared parent" scenario will cause the flagged > > > clock to be disabled. > > > > I'm not sure if the third patch actually solves the _real_ issue you > > allude to here. The original critical (then called always-on) clock > > patch-set solved it just fine. However others were concerned about > > how you would then turn the clock off if some knowledgeable consumer > > (who knew the full consequences of their actions) came along and had a > > good reason to gate it. That, the turning off the clock if still > > desired is what the third patch _actually_ solves. > > > > Now we are back where we started however, and still need to figure out > > a generic method to mark the clocks (either by setting a FLAG or > > actually calling enable()) as critical. > > The third patch: > > 1) implements an always-on behavior > 2) allows knowledgeable drivers to gate the clock > 3) marks such clocks with a flag > > What is missing? The difficuly is marking the clock in the first place. We already have drivers doing 1), just not in a generic way. This solution is more generic (as was mine), but it's still not completely generic. 2) is a new requirement that would have required a little more thought. My solution to this was only in V1/RFC and admitted would have required a little more discussion. 3) isn't really a 'thing'. > > > The first driver to come along and explicitly > > > claim, prepare and enable this clock
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Hi Mike,
On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
mturque...@baylibre.com wrote:
This is an alternative solution to Lee's clk: Provide support for
always-on clocks series[0].
The first two patches introduce run-time checks to ensure that clock
consumer drivers are respecting the clk.h api. The former patch checks
for prepare and enable imbalances. The latter checks for calls to
clk_put without first disabling and unpreparing the clk.
The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which
prepares and enables a clk at registration-time. The reference counts
(prepare enable) are transferred to the first clock consumer driver
that clk_get's the clk with this flag set AND calls clk_prepare or
clk_enable.
The net result is that a clock with this flag set will be enabled at
boot and neither the clk_disable_unused garbage collector or the
sibling clock disables a shared parent scenario will cause the flagged
clock to be disabled. The first driver to come along and explicitly
claim, prepare and enable this clock will inherit those reference
counts. No change to clock consumer drivers is required for this to
work. Please continue to use the clk.h api properly.
In time this approach can probably replace the CLK_IGNORE_UNUSED flag
and hopefully reduce the number of users of the clk_ignore_unused boot
parameter.
Thanks for your series!
I gave it a try on r8a7791/koelsch, where I replaced the hack from
[PATCH/RFC 1/5] clk: shmobile: mstp: Never disable INTC-SYS
(http://www.spinics.net/lists/linux-sh/msg41107.html) by setting
init.flags |= CLK_ENABLE_HAND_OFF
in cpg_mstp_clock_register() for intc-sys.
The end result is fine (the intc-sys clock is never disabled), but I get
a few annoying lockdep splats like below (one for the intc-sys clock,
and one more for each parent up to the root clock):
[ cut here ]
WARNING: CPU: 0 PID: 0 at drivers/clk/clk.c:745 clk_core_enable+0x6c/0xdc()
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.2.0-rc6-koelsch-04462-g27bac5e25174da01-dirty #1507
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace:
[c00138b4] (dump_backtrace) from [c0013aac] (show_stack+0x18/0x1c)
r6:c05c249d r5:0009 r4: r3:0020
[c0013a94] (show_stack) from [c045fea4] (dump_stack+0x78/0x94)
[c045fe2c] (dump_stack) from [c002ba18] (warn_slowpath_common+0x90/0xbc)
r4: r3:
[c002b988] (warn_slowpath_common) from [c002bae8]
(warn_slowpath_null+0x24/0x2c)
r8:eec10d00 r7:0001 r6:eec0f8c0 r5: r4:eec0f8c0
[c002bac4] (warn_slowpath_null) from [c0344fbc] (clk_core_enable+0x6c/0xdc)
[c0344f50] (clk_core_enable) from [c0346e44] (clk_register+0x504/0x62c)
r5: r4:eec0f8c0
[c0346940] (clk_register) from [c0625cd8] (cpg_mstp_clocks_init+0x240/0x310)
r10:c05c2f26 r9:0008 r8:eec10d00 r7:c0ff3755 r6:eec0f7c0 r5:ef1df1cc
r4:eec09080
[c0625a98] (cpg_mstp_clocks_init) from [c0624f7c] (of_clk_init+0xe0/0x188)
r10:0002 r9:eec09100 r8:eec09108 r7:0001 r6:eec09140 r5:c0643f48
r4:
[c0624e9c] (of_clk_init) from [c060ea20] (rcar_gen2_timer_init+0x108/0x120)
r10:c0633ae4 r9:c0644400 r8: r7: r6:ef7fca00 r5:f0006000
r4:00989680
[c060e918] (rcar_gen2_timer_init) from [c0609334] (time_init+0x24/0x38)
r5:c067c000 r4:
[c0609310] (time_init) from [c0606bb0] (start_kernel+0x268/0x378)
[c0606948] (start_kernel) from [40008090] (0x40008090)
r10: r9:413fc0f2 r8:40007000 r7:c0647fe8 r6:c0633ae0 r5:c0644480
r4:c067c394
---[ end trace cb88537fdc8fa200 ]---
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
On Mon, 10 Aug 2015, Michael Turquette wrote: Quoting Lee Jones (2015-08-10 08:36:38) On Fri, 07 Aug 2015, Michael Turquette wrote: This is an alternative solution to Lee's clk: Provide support for always-on clocks series[0]. So after all the hours of work that's been put in and all of the versions that have been authored, you're just going to write your own solution anyway, without any further discussion? This certainly seems like further discussion to me. In fact the capital C in RFC pretty much announces that this is a big, wide open discussion. If I had ninja-merged the patches then I would understand your frustration, but that clearly is not the case. This doesn't have anything to do with merging code. When contributors submit patches to subsystem I maintain, I provide reviews, advice and guidance. If I think an approach is wrong, I state why I think that and provide ideas for an alternative approach if required. If instead of providing feedback I just replied with a patch-set containing my own alternative method, I would expect the original submitter to be justifiably cross. That's one way to make your contributors feel valued. Lee, your contributions are very much valued. You and I both know that there are times when a concept is far better communicated with code than with prose. This is one of those times. The DT-centric solution that requires a clock consumer driver to call clk_disable() before calling clk_enable() (an obvious violation of the clk.h api) is just plain wrong and this implementation hopes to get it back on the right course. See my previous email about what I think about bastardizing the clk.h api. The first two patches introduce run-time checks to ensure that clock consumer drivers are respecting the clk.h api. The former patch checks for prepare and enable imbalances. The latter checks for calls to clk_put without first disabling and unpreparing the clk. Are these real issues that people are having trouble with, or just hypothetical ones? I can understand the need for them if they're causing people some pain, but if they are unnecessarily hardening the API, then it makes it difficult for proper issues (such as critical clocks) to be solved easily. They are deeply related. Per-user accounting gives us a graceful method to implement hand-off, which is the real feature that is needed here. The hand-off feature was mentioned a week ago, as part of an 8 month discussion. This only aids knowledgeable consumers to turn of a critical clock after the introduction of the API tightening which happens in _this_ set. So I disagree that this is the real feature that is needed here. The _real_ feature that's required is a generic way to keep critical clocks enabled and a way for them to be gated if another driver knows best. Granted this method is pretty clean for drivers which have all of their platform data in driver code, but it's still not ideal. The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which prepares and enables a clk at registration-time. The reference counts (prepare enable) are transferred to the first clock consumer driver that clk_get's the clk with this flag set AND calls clk_prepare or clk_enable. The net result is that a clock with this flag set will be enabled at boot and neither the clk_disable_unused garbage collector or the sibling clock disables a shared parent scenario will cause the flagged clock to be disabled. I'm not sure if the third patch actually solves the _real_ issue you allude to here. The original critical (then called always-on) clock patch-set solved it just fine. However others were concerned about how you would then turn the clock off if some knowledgeable consumer (who knew the full consequences of their actions) came along and had a good reason to gate it. That, the turning off the clock if still desired is what the third patch _actually_ solves. Now we are back where we started however, and still need to figure out a generic method to mark the clocks (either by setting a FLAG or actually calling enable()) as critical. The third patch: 1) implements an always-on behavior 2) allows knowledgeable drivers to gate the clock 3) marks such clocks with a flag What is missing? The difficuly is marking the clock in the first place. We already have drivers doing 1), just not in a generic way. This solution is more generic (as was mine), but it's still not completely generic. 2) is a new requirement that would have required a little more thought. My solution to this was only in V1/RFC and admitted would have required a little more discussion. 3) isn't really a 'thing'. The first driver to come along and explicitly claim, prepare and enable this clock will inherit those reference counts. No change to clock consumer drivers is required for this to work. Do you mean it won't break
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
Hi Mike,
On Tue, Aug 11, 2015 at 6:41 PM, Michael Turquette
mturque...@baylibre.com wrote:
Quoting Geert Uytterhoeven (2015-08-11 02:20:12)
On Fri, Aug 7, 2015 at 9:09 PM, Michael Turquette
mturque...@baylibre.com wrote:
This is an alternative solution to Lee's clk: Provide support for
always-on clocks series[0].
I gave it a try on r8a7791/koelsch, where I replaced the hack from
[PATCH/RFC 1/5] clk: shmobile: mstp: Never disable INTC-SYS
(http://www.spinics.net/lists/linux-sh/msg41107.html) by setting
init.flags |= CLK_ENABLE_HAND_OFF
in cpg_mstp_clock_register() for intc-sys.
The end result is fine (the intc-sys clock is never disabled), but I get
a few annoying lockdep splats like below (one for the intc-sys clock,
and one more for each parent up to the root clock):
[ cut here ]
WARNING: CPU: 0 PID: 0 at drivers/clk/clk.c:745 clk_core_enable+0x6c/0xdc()
Thanks much for testing! I forgot to hold the enable lock in __clk_init
(we already hold the prepare lock). Can you tell me if this diff fixes
it?
Yes it does. Thanks for the quick fix!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
On Fri, 07 Aug 2015, Michael Turquette wrote: > This is an alternative solution to Lee's "clk: Provide support for > always-on clocks" series[0]. So after all the hours of work that's been put in and all of the versions that have been authored, you're just going to write your own solution anyway, without any further discussion? That's one way to make your contributors feel valued. > The first two patches introduce run-time checks to ensure that clock > consumer drivers are respecting the clk.h api. The former patch checks > for prepare and enable imbalances. The latter checks for calls to > clk_put without first disabling and unpreparing the clk. Are these real issues that people are having trouble with, or just hypothetical ones? I can understand the need for them if they're causing people some pain, but if they are unnecessarily hardening the API, then it makes it difficult for proper issues (such as critical clocks) to be solved easily. > The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which > prepares and enables a clk at registration-time. The reference counts > (prepare & enable) are transferred to the first clock consumer driver > that clk_get's the clk with this flag set AND calls clk_prepare or > clk_enable. > > The net result is that a clock with this flag set will be enabled at > boot and neither the clk_disable_unused garbage collector or the > "sibling clock disables a shared parent" scenario will cause the flagged > clock to be disabled. I'm not sure if the third patch actually solves the _real_ issue you allude to here. The original critical (then called always-on) clock patch-set solved it just fine. However others were concerned about how you would then turn the clock off if some knowledgeable consumer (who knew the full consequences of their actions) came along and had a good reason to gate it. That, the turning off the clock if still desired is what the third patch _actually_ solves. Now we are back where we started however, and still need to figure out a generic method to mark the clocks (either by setting a FLAG or actually calling enable()) as critical. > The first driver to come along and explicitly > claim, prepare and enable this clock will inherit those reference > counts. No change to clock consumer drivers is required for this to > work. Do you mean it won't break anything? I think to make use of the functionality each of the providers still have to figure out which of their clocks are critical (which may change from platform to platform) then mark them as such before registration. > Please continue to use the clk.h api properly. > > In time this approach can probably replace the CLK_IGNORE_UNUSED flag > and hopefully reduce the number of users of the clk_ignore_unused boot > parameter. > > Finally, a quick note on comparing this series to Lee's. I went with the > simplest approach to solve a real problem: preventing critical clocks > from being spuriously disabled at boot, or before a their parent clock > becomes accidentally disabled by a sibling. This wasn't the problem. Platforms are already doing this. The _real_ problem was doing it in a generic way, so vendors didn't have to roll their own 'gather' and 'mark' code, which unfortunately they still have to do with this solution. > All of the other kitchen sink stuff (DT binding, The DT binding will still be required, at least for ST, as they have gone for the more Linuxy approach of having a generic set of clock drivers and provide all of the platform specific information in platform code (i.e. DT). So to identify which clocks are critical on each platform the DT will need to be interrogated in some way. > passing the flag back to the framework when the clock consumer > driver calls clk_put) was left I'm not sure what this is. > 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. > > [0] > http://lkml.kernel.org/r/<1437570255-21049-1-git-send-email-lee.jo...@linaro.org> > > Michael Turquette (3): > clk: per-user clk prepare & enable ref counts > clk: clk_put WARNs if user has not disabled clk > clk: introduce CLK_ENABLE_HAND_OFF flag > > drivers/clk/clk.c| 79 > +--- > include/linux/clk-provider.h | 3 ++ > 2 files changed, 78 insertions(+), 4 deletions(-) > -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC RFT 0/3] clk: detect per-user enable imbalances and implement hand-off
On Fri, 07 Aug 2015, Michael Turquette wrote: This is an alternative solution to Lee's clk: Provide support for always-on clocks series[0]. So after all the hours of work that's been put in and all of the versions that have been authored, you're just going to write your own solution anyway, without any further discussion? That's one way to make your contributors feel valued. The first two patches introduce run-time checks to ensure that clock consumer drivers are respecting the clk.h api. The former patch checks for prepare and enable imbalances. The latter checks for calls to clk_put without first disabling and unpreparing the clk. Are these real issues that people are having trouble with, or just hypothetical ones? I can understand the need for them if they're causing people some pain, but if they are unnecessarily hardening the API, then it makes it difficult for proper issues (such as critical clocks) to be solved easily. The third patch introduces a new flag, CLK_ENABLE_HAND_OFF, which prepares and enables a clk at registration-time. The reference counts (prepare enable) are transferred to the first clock consumer driver that clk_get's the clk with this flag set AND calls clk_prepare or clk_enable. The net result is that a clock with this flag set will be enabled at boot and neither the clk_disable_unused garbage collector or the sibling clock disables a shared parent scenario will cause the flagged clock to be disabled. I'm not sure if the third patch actually solves the _real_ issue you allude to here. The original critical (then called always-on) clock patch-set solved it just fine. However others were concerned about how you would then turn the clock off if some knowledgeable consumer (who knew the full consequences of their actions) came along and had a good reason to gate it. That, the turning off the clock if still desired is what the third patch _actually_ solves. Now we are back where we started however, and still need to figure out a generic method to mark the clocks (either by setting a FLAG or actually calling enable()) as critical. The first driver to come along and explicitly claim, prepare and enable this clock will inherit those reference counts. No change to clock consumer drivers is required for this to work. Do you mean it won't break anything? I think to make use of the functionality each of the providers still have to figure out which of their clocks are critical (which may change from platform to platform) then mark them as such before registration. Please continue to use the clk.h api properly. In time this approach can probably replace the CLK_IGNORE_UNUSED flag and hopefully reduce the number of users of the clk_ignore_unused boot parameter. Finally, a quick note on comparing this series to Lee's. I went with the simplest approach to solve a real problem: preventing critical clocks from being spuriously disabled at boot, or before a their parent clock becomes accidentally disabled by a sibling. This wasn't the problem. Platforms are already doing this. The _real_ problem was doing it in a generic way, so vendors didn't have to roll their own 'gather' and 'mark' code, which unfortunately they still have to do with this solution. All of the other kitchen sink stuff (DT binding, The DT binding will still be required, at least for ST, as they have gone for the more Linuxy approach of having a generic set of clock drivers and provide all of the platform specific information in platform code (i.e. DT). So to identify which clocks are critical on each platform the DT will need to be interrogated in some way. passing the flag back to the framework when the clock consumer driver calls clk_put) was left I'm not sure what this is. 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. [0] http://lkml.kernel.org/r/1437570255-21049-1-git-send-email-lee.jo...@linaro.org Michael Turquette (3): clk: per-user clk prepare enable ref counts clk: clk_put WARNs if user has not disabled clk clk: introduce CLK_ENABLE_HAND_OFF flag drivers/clk/clk.c| 79 +--- include/linux/clk-provider.h | 3 ++ 2 files changed, 78 insertions(+), 4 deletions(-) -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
