| Message ID | 20220517235919.200375-1-marex@denx.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3,1/2] dt-bindings: clk: Introduce 'critical-clocks' property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success |
On Wed, May 18, 2022 at 01:59:18AM +0200, Marek Vasut wrote: > Some platforms require select clock to be always running, e.g. because > those clock supply vital devices which are not otherwise attached to > the system and thus do not have a matching DT node and clock consumer. > > An example is a system where the SoC serves as a crystal oscillator > replacement for a programmable logic device. The "critical-clocks" > property of a clock controller allows listing clock which must never > be turned off. > > Clock listed in the "critical-clocks" property may have other consumers > in DT, listing the clock in "critical-clocks" only assures those clock > are never turned off, and none of these optional additional consumers > can turn the clock off either. > > The implementation is modeled after "protected-clocks". > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: devicetree@vger.kernel.org > To: linux-clk@vger.kernel.org > --- > V2: Update the commit message to clarify the behavior > V3: s@Some platforms require clock@Some platforms require some clocks@ > --- > .../devicetree/bindings/clock/clock-bindings.txt | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) This file is removed upstream as it is replaced by the schema in dtschema. But I'd wait to see what Stephen says. I'm fine with the addition. Rob
Quoting Marek Vasut (2022-05-17 16:59:18) > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt > index f2ea53832ac63..d7f7afe2cbd0c 100644 > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt > @@ -169,6 +169,22 @@ a shared clock is forbidden. > Configuration of common clocks, which affect multiple consumer devices can > be similarly specified in the clock provider node. > > +==Critical clocks== > + > +Some platforms require some clocks to be always running, e.g. because those > +clock supply devices which are not otherwise attached to the system. One > +example is a system where the SoC serves as a crystal oscillator replacement > +for a programmable logic device. The critical-clocks property of a clock > +controller allows listing clock which must never be turned off. > + > + clock-controller@a000f000 { > + compatible = "vendor,clk95; > + reg = <0xa000f000 0x1000> > + #clocks-cells = <1>; > + ... > + critical-clocks = <UART3_CLK>, <SPI5_CLK>; Historically "critical" is overloaded in the clk framework. We should avoid using that name. What does "critical" even mean? Instead I'd prefer "always-on-clocks" here, so we can indicate that these clks should always be on. It would also parallel the property in the regulator framework.
Quoting Marek Vasut (2022-05-17 16:59:19) > Some platforms require select clock to be always running, e.g. because > those clock supply vital devices which are not otherwise attached to > the system and thus do not have a matching DT node and clock consumer. > > An example is a system where the SoC serves as a crystal oscillator > replacement for a programmable logic device. The "critical-clocks" > property of a clock controller allows listing clock which must never > be turned off. > > Clock listed in the "critical-clocks" property may have other consumers > in DT, listing the clock in "critical-clocks" only assures those clock > are never turned off, and none of these optional additional consumers > can turn the clock off either. This is achieved by adding CLK_IS_CRITICAL > flag to these critical clock. > > This flag has thus far been added to select clock by hard-coding it in > various clock drivers, this patch provides generic DT interface to add > the flag to arbitrary clock that may be critical. > > The implementation is modeled after "protected-clocks", except the protected > clock property is currently driver specific. This patch attempts to provide > a generic implementation of "critical-clocks" instead. > > Unlike "assigned-clocks", the "critical-clocks" must be parsed much earlier > in __clk_register() to assign CLK_IS_CRITICAL flag to clk_init_data .flags > field. Why? Instead of using the CLK_IS_CRITICAL flag to enable at registration time for this, why can't we parse the property when a clk provider is registered and enable those clks manually and then set the CLK_IS_CRITICAL flag? Ideally we don't implement another clk_op for this. > > The new match_clkspec() callback is used to determine whether struct clk_hw > that is currently being registered matches the clock specifier in the DT > "critical-clocks" property, and if so, then the CLK_IS_CRITICAL is added to > these newly registered clock. This callback can only be driver specific. > > Signed-off-by: Marek Vasut <marex@denx.de>
On 6/15/22 22:10, Stephen Boyd wrote: > Quoting Marek Vasut (2022-05-17 16:59:18) >> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt >> index f2ea53832ac63..d7f7afe2cbd0c 100644 >> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt >> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt >> @@ -169,6 +169,22 @@ a shared clock is forbidden. >> Configuration of common clocks, which affect multiple consumer devices can >> be similarly specified in the clock provider node. >> >> +==Critical clocks== >> + >> +Some platforms require some clocks to be always running, e.g. because those >> +clock supply devices which are not otherwise attached to the system. One >> +example is a system where the SoC serves as a crystal oscillator replacement >> +for a programmable logic device. The critical-clocks property of a clock >> +controller allows listing clock which must never be turned off. >> + >> + clock-controller@a000f000 { >> + compatible = "vendor,clk95; >> + reg = <0xa000f000 0x1000> >> + #clocks-cells = <1>; >> + ... >> + critical-clocks = <UART3_CLK>, <SPI5_CLK>; > > Historically "critical" is overloaded in the clk framework. We should > avoid using that name. What does "critical" even mean? It means those clock must not be turned off, but there is no consumer described in DT. > Instead I'd prefer "always-on-clocks" here, so we can indicate that > these clks should always be on. It would also parallel the property in > the regulator framework. This property name is derived from protected-clock which you introduced. I think it would be better to stay consistent within the clock framework property names ?
Quoting Marek Vasut (2022-06-15 14:55:17) > On 6/15/22 22:10, Stephen Boyd wrote: > > Quoting Marek Vasut (2022-05-17 16:59:18) > >> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt > >> index f2ea53832ac63..d7f7afe2cbd0c 100644 > >> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt > >> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt > >> @@ -169,6 +169,22 @@ a shared clock is forbidden. > >> Configuration of common clocks, which affect multiple consumer devices can > >> be similarly specified in the clock provider node. > >> > >> +==Critical clocks== > >> + > >> +Some platforms require some clocks to be always running, e.g. because those > >> +clock supply devices which are not otherwise attached to the system. One > >> +example is a system where the SoC serves as a crystal oscillator replacement > >> +for a programmable logic device. The critical-clocks property of a clock > >> +controller allows listing clock which must never be turned off. > >> + > >> + clock-controller@a000f000 { > >> + compatible = "vendor,clk95; > >> + reg = <0xa000f000 0x1000> > >> + #clocks-cells = <1>; > >> + ... > >> + critical-clocks = <UART3_CLK>, <SPI5_CLK>; > > > > Historically "critical" is overloaded in the clk framework. We should > > avoid using that name. What does "critical" even mean? > > It means those clock must not be turned off, but there is no consumer > described in DT. So it means "always on". > > > Instead I'd prefer "always-on-clocks" here, so we can indicate that > > these clks should always be on. It would also parallel the property in > > the regulator framework. > > This property name is derived from protected-clock which you introduced. > I think it would be better to stay consistent within the clock framework > property names ? protected-clocks is based on assigned-clocks. There isn't a CLK_IS_PROTECTED flag. I'm not following your argument at all here, sorry.
On 6/16/22 00:22, Stephen Boyd wrote: > Quoting Marek Vasut (2022-06-15 14:55:17) >> On 6/15/22 22:10, Stephen Boyd wrote: >>> Quoting Marek Vasut (2022-05-17 16:59:18) >>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt >>>> index f2ea53832ac63..d7f7afe2cbd0c 100644 >>>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt >>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt >>>> @@ -169,6 +169,22 @@ a shared clock is forbidden. >>>> Configuration of common clocks, which affect multiple consumer devices can >>>> be similarly specified in the clock provider node. >>>> >>>> +==Critical clocks== >>>> + >>>> +Some platforms require some clocks to be always running, e.g. because those >>>> +clock supply devices which are not otherwise attached to the system. One >>>> +example is a system where the SoC serves as a crystal oscillator replacement >>>> +for a programmable logic device. The critical-clocks property of a clock >>>> +controller allows listing clock which must never be turned off. >>>> + >>>> + clock-controller@a000f000 { >>>> + compatible = "vendor,clk95; >>>> + reg = <0xa000f000 0x1000> >>>> + #clocks-cells = <1>; >>>> + ... >>>> + critical-clocks = <UART3_CLK>, <SPI5_CLK>; >>> >>> Historically "critical" is overloaded in the clk framework. We should >>> avoid using that name. What does "critical" even mean? >> >> It means those clock must not be turned off, but there is no consumer >> described in DT. > > So it means "always on". > >> >>> Instead I'd prefer "always-on-clocks" here, so we can indicate that >>> these clks should always be on. It would also parallel the property in >>> the regulator framework. >> >> This property name is derived from protected-clock which you introduced. >> I think it would be better to stay consistent within the clock framework >> property names ? > > protected-clocks is based on assigned-clocks. There isn't a > CLK_IS_PROTECTED flag. I'm not following your argument at all here, > sorry. critical-clock property name is based on protected-clock property name. There is also no CLK_IS_ALWAYS_ON flag , but there is CLK_IS_CRITICAL flag . Sure, there is no CLK_IS_PROTECTED flag because the protected clock is implemented only by a single driver (qualcomm). I think it makes sense to align the DT property name and the flag name, and the critical-clock is aligned with both other DT property names in the clock framework and the flag name.
Quoting Marek Vasut (2022-06-15 16:42:25) > On 6/16/22 00:22, Stephen Boyd wrote: > > Quoting Marek Vasut (2022-06-15 14:55:17) > >> > >> It means those clock must not be turned off, but there is no consumer > >> described in DT. > > > > So it means "always on". > > > >> > >>> Instead I'd prefer "always-on-clocks" here, so we can indicate that > >>> these clks should always be on. It would also parallel the property in > >>> the regulator framework. > >> > >> This property name is derived from protected-clock which you introduced. > >> I think it would be better to stay consistent within the clock framework > >> property names ? > > > > protected-clocks is based on assigned-clocks. There isn't a > > CLK_IS_PROTECTED flag. I'm not following your argument at all here, > > sorry. > > critical-clock property name is based on protected-clock property name. Got that. > > There is also no CLK_IS_ALWAYS_ON flag , but there is CLK_IS_CRITICAL > flag . Sure, there is no CLK_IS_PROTECTED flag because the protected > clock is implemented only by a single driver (qualcomm). Alright, thanks for clarifying the argument. > > I think it makes sense to align the DT property name and the flag name, > and the critical-clock is aligned with both other DT property names in > the clock framework and the flag name. No, it does not make sense to align the CLK_IS_CRITICAL flag with the DT property name. The property should be named what it is, i.e. always on. The internal clk framework details could be changed at any time whereas the DT binding is essentially forever. I agree using a similar name may help connect the code and the DT, but that is nowhere near as important as choosing a name that expresses what is happening in a binding that we need to maintain for the foreseeable future.
diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt index f2ea53832ac63..d7f7afe2cbd0c 100644 --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt @@ -169,6 +169,22 @@ a shared clock is forbidden. Configuration of common clocks, which affect multiple consumer devices can be similarly specified in the clock provider node. +==Critical clocks== + +Some platforms require some clocks to be always running, e.g. because those +clock supply devices which are not otherwise attached to the system. One +example is a system where the SoC serves as a crystal oscillator replacement +for a programmable logic device. The critical-clocks property of a clock +controller allows listing clock which must never be turned off. + + clock-controller@a000f000 { + compatible = "vendor,clk95; + reg = <0xa000f000 0x1000> + #clocks-cells = <1>; + ... + critical-clocks = <UART3_CLK>, <SPI5_CLK>; + }; + ==Protected clocks== Some platforms or firmwares may not fully expose all the clocks to the OS, such
Some platforms require select clock to be always running, e.g. because those clock supply vital devices which are not otherwise attached to the system and thus do not have a matching DT node and clock consumer. An example is a system where the SoC serves as a crystal oscillator replacement for a programmable logic device. The "critical-clocks" property of a clock controller allows listing clock which must never be turned off. Clock listed in the "critical-clocks" property may have other consumers in DT, listing the clock in "critical-clocks" only assures those clock are never turned off, and none of these optional additional consumers can turn the clock off either. The implementation is modeled after "protected-clocks". Signed-off-by: Marek Vasut <marex@denx.de> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Stephen Boyd <sboyd@kernel.org> Cc: devicetree@vger.kernel.org To: linux-clk@vger.kernel.org --- V2: Update the commit message to clarify the behavior V3: s@Some platforms require clock@Some platforms require some clocks@ --- .../devicetree/bindings/clock/clock-bindings.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)