| Message ID | 20211020084956.83041-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings: mfd: rohm,bd71847-pmic: Document rohm,clock-output-is-critical property | expand |
Hi dee Ho Marek, Thanks! I like the idea and appreciate the improvement. On 10/20/21 11:49, Marek Vasut wrote: > Add the possibility to configure PMIC 32kHz output clock as CRITICAL, > so that they are never gated off. This is useful in case those clock > supply some vital clock net, which requires the clock to always run. > > The iMX8M RTC XTAL input is one such example, if the clock are ever > gated off, the system locks up completely. The clock must be present > and enabled even if the RTC is unused. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: devicetree@vger.kernel.org > Cc: linux-power@fi.rohmeurope.com > To: linux-clk@vger.kernel.org > --- > Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml > index 5d531051a153..2497ade2bbd0 100644 > --- a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml > @@ -41,6 +41,11 @@ properties: > clock-output-names: > maxItems: 1 > > + rohm,clock-output-is-critical: I wonder if this really is something specific to ROHM ICs? Do you think this would warrant a generic, non vendor specific property? I am Ok with the ROHM specific property too but it just seems to me this might not be unique to ROHM IC(s). By the way, the very same clk driver where you implemented the property reading (patch 2/2) is used by few other ROHM PMICs. At least by BD71837, BD71828, BD71815, BD9576 and BD9573. So the code change here adds support for this property to all of those PMICs. I wonder if the property should be mentioned in all of the binding docs... That could be another argument for making this a generic property and describing it in clk yaml ;) Well, just my 10 Cents - I am ok with this change as you presented it here if you don't think this should be generic one. > + description: > + Never gate off C32K_OUT clock. > + type: boolean > + > # The BD71847 abd BD71850 support two different HW states as reset target > # states. States are called as SNVS and READY. At READY state all the PMIC > # power outputs go down and OTP is reload. At the SNVS state all other logic > Best Regards Matti Vaittinen
On 10/20/21 12:14 PM, Vaittinen, Matti wrote: [...] > I wonder if this really is something specific to ROHM ICs? Do you think > this would warrant a generic, non vendor specific property? I am Ok with > the ROHM specific property too but it just seems to me this might not be > unique to ROHM IC(s). > > By the way, the very same clk driver where you implemented the property > reading (patch 2/2) is used by few other ROHM PMICs. At least by > BD71837, BD71828, BD71815, BD9576 and BD9573. So the code change here > adds support for this property to all of those PMICs. I wonder if the > property should be mentioned in all of the binding docs... That could be > another argument for making this a generic property and describing it in > clk yaml ;) > > Well, just my 10 Cents - I am ok with this change as you presented it > here if you don't think this should be generic one. I think we need something like gpio-hog, except for clock. Some clk-hog maybe ? That would be useful not only here, but also for things where some output generates clock for random stuff which cannot be described in the DT for whatever reason (like e.g. the SoC is used as a substitute for CPLD XTAL and the CPLD isn't connected to the SoC in any other way).
On Wed, Oct 20, 2021 at 01:06:13PM +0200, Marek Vasut wrote: > On 10/20/21 12:14 PM, Vaittinen, Matti wrote: > [...] > > > I wonder if this really is something specific to ROHM ICs? Do you think > > this would warrant a generic, non vendor specific property? I am Ok with > > the ROHM specific property too but it just seems to me this might not be > > unique to ROHM IC(s). I imagine we debated the need for a DT property when critical clocks was added to the kernel. > > By the way, the very same clk driver where you implemented the property > > reading (patch 2/2) is used by few other ROHM PMICs. At least by > > BD71837, BD71828, BD71815, BD9576 and BD9573. So the code change here > > adds support for this property to all of those PMICs. I wonder if the > > property should be mentioned in all of the binding docs... That could be > > another argument for making this a generic property and describing it in > > clk yaml ;) > > > > Well, just my 10 Cents - I am ok with this change as you presented it > > here if you don't think this should be generic one. > > I think we need something like gpio-hog, except for clock. Some clk-hog > maybe ? That would be useful not only here, but also for things where some > output generates clock for random stuff which cannot be described in the DT > for whatever reason (like e.g. the SoC is used as a substitute for CPLD XTAL > and the CPLD isn't connected to the SoC in any other way). The justification given in this patch was for an SoC input which should get described so that the clock is handled and kept enabled properly. The CPLD case would be more interesting, but is there an actual need or just a possible case? You could use the 'protected-clocks' property here. Maybe that's a bit overloaded between can't access and don't turn off. But what it means is really up the clock controller. Rob
On 10/29/21 00:24, Rob Herring wrote: > On Wed, Oct 20, 2021 at 01:06:13PM +0200, Marek Vasut wrote: >> On 10/20/21 12:14 PM, Vaittinen, Matti wrote: >> [...] >> >>> I wonder if this really is something specific to ROHM ICs? Do you think >>> this would warrant a generic, non vendor specific property? I am Ok with >>> the ROHM specific property too but it just seems to me this might not be >>> unique to ROHM IC(s). > > I imagine we debated the need for a DT property when critical clocks was > added to the kernel. Sorry. I guess I've missed this. Maybe this was done back when I spent my days tinkering with strictly proprietary systems - or then I have just missed it. >>> By the way, the very same clk driver where you implemented the property >>> reading (patch 2/2) is used by few other ROHM PMICs. At least by >>> BD71837, BD71828, BD71815, BD9576 and BD9573. So the code change here >>> adds support for this property to all of those PMICs. I wonder if the >>> property should be mentioned in all of the binding docs... That could be >>> another argument for making this a generic property and describing it in >>> clk yaml ;) >>> >>> Well, just my 10 Cents - I am ok with this change as you presented it >>> here if you don't think this should be generic one. >> >> I think we need something like gpio-hog, except for clock. Some clk-hog >> maybe ? That would be useful not only here, but also for things where some >> output generates clock for random stuff which cannot be described in the DT >> for whatever reason (like e.g. the SoC is used as a substitute for CPLD XTAL >> and the CPLD isn't connected to the SoC in any other way). > > The justification given in this patch was for an SoC input which should > get described so that the clock is handled and kept enabled properly. > > The CPLD case would be more interesting, but is there an actual need or > just a possible case? > > You could use the 'protected-clocks' property here. Maybe that's a bit > overloaded between can't access and don't turn off. But what it means is > really up the clock controller. I think I have seen some patch series which are aimed to providing common implementation for the 'protected-clocks'. It seems to me the intended 'protected-clocks' handling is simply not registering these clocks. I don't see this handling in-tree yet though and I did not find any discussion as to why the generic support has not been merged. So, if the 'protected-clocks' is to be supported by the driver, then I wonder if the handling should be 'ensure enabled and don't register to clock core' or plain 'don't register to clock core'? Best Regards --Matti Vaittinen
On 10/28/21 11:24 PM, Rob Herring wrote: > On Wed, Oct 20, 2021 at 01:06:13PM +0200, Marek Vasut wrote: >> On 10/20/21 12:14 PM, Vaittinen, Matti wrote: >> [...] >> >>> I wonder if this really is something specific to ROHM ICs? Do you think >>> this would warrant a generic, non vendor specific property? I am Ok with >>> the ROHM specific property too but it just seems to me this might not be >>> unique to ROHM IC(s). > > I imagine we debated the need for a DT property when critical clocks was > added to the kernel. Have you got some reference to this debate ? I think something like clk-hog , similar to gpio-hog , would be useful, since we could also configure the critical clock frequency in DT. >>> By the way, the very same clk driver where you implemented the property >>> reading (patch 2/2) is used by few other ROHM PMICs. At least by >>> BD71837, BD71828, BD71815, BD9576 and BD9573. So the code change here >>> adds support for this property to all of those PMICs. I wonder if the >>> property should be mentioned in all of the binding docs... That could be >>> another argument for making this a generic property and describing it in >>> clk yaml ;) >>> >>> Well, just my 10 Cents - I am ok with this change as you presented it >>> here if you don't think this should be generic one. >> >> I think we need something like gpio-hog, except for clock. Some clk-hog >> maybe ? That would be useful not only here, but also for things where some >> output generates clock for random stuff which cannot be described in the DT >> for whatever reason (like e.g. the SoC is used as a substitute for CPLD XTAL >> and the CPLD isn't connected to the SoC in any other way). > > The justification given in this patch was for an SoC input which should > get described so that the clock is handled and kept enabled properly. This is the case I had here, yes. Although I've been running into similar requirements repeatedly for almost a decade, I'm surprised nobody implemented something like this yet. > The CPLD case would be more interesting, but is there an actual need or > just a possible case? This is an iMX53 board from 2012 or so, where they figured they don't need an XTal for the CPLD because the SoC has this OSC_OUT and that can be used to supply clock to the CPLD at just the frequency they need. So the SoC is a clock source for the CPLD, and that's all there is to it. So far I hacked it in the clock driver to keep the clock running at specific rate, but that hack has been a thorn in my side for long enough. > You could use the 'protected-clocks' property here. Maybe that's a bit > overloaded between can't access and don't turn off. But what it means is > really up the clock controller. This does not seem to describe what is needed here, protected-clock are used to tell OS not to touch certain clock because they are protected by e.g. firmware access restriction, it does not say anything about whether the clock are critical. Also, it seems to be a non-generic property only for some qualcomm clock driver. commit 48d7f160b10711f014bf07b574c73452646c9fdd [...] dt-bindings: clk: Introduce 'protected-clocks' property Add a generic clk property for clks which are not intended to be used by the OS due to security restrictions put in place by firmware. For example, on some Qualcomm firmwares reading or writing certain clk registers causes the entire system to reboot, but on other firmwares reading and writing those same registers is required to make devices like QSPI work. Rather than adding one-off properties each time a new set of clks appears to be protected, let's add a generic clk property to describe any set of clks that shouldn't be touched by the OS. This way we never need to register the clks or use them in certain firmware configurations.
diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml index 5d531051a153..2497ade2bbd0 100644 --- a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml @@ -41,6 +41,11 @@ properties: clock-output-names: maxItems: 1 + rohm,clock-output-is-critical: + description: + Never gate off C32K_OUT clock. + type: boolean + # The BD71847 abd BD71850 support two different HW states as reset target # states. States are called as SNVS and READY. At READY state all the PMIC # power outputs go down and OTP is reload. At the SNVS state all other logic
Add the possibility to configure PMIC 32kHz output clock as CRITICAL, so that they are never gated off. This is useful in case those clock supply some vital clock net, which requires the clock to always run. The iMX8M RTC XTAL input is one such example, if the clock are ever gated off, the system locks up completely. The clock must be present and enabled even if the RTC is unused. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Stephen Boyd <sboyd@kernel.org> Cc: devicetree@vger.kernel.org Cc: linux-power@fi.rohmeurope.com To: linux-clk@vger.kernel.org --- Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml | 5 +++++ 1 file changed, 5 insertions(+)