[1/2] dt-bindings: mfd: rohm,bd71847-pmic: Document rohm,clock-output-is-critical property

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(+)

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.