[RFC PATCH 1/3] clk: bd718x7: Clean up the code, no functional changes

[RFC PATCH 1/3] clk: bd718x7: Clean up the code, no functional changes

[Michael Trimarchi]

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 drivers/clk/clk-bd718x7.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
index ac40b669d60b..04cc0beb67df 100644
--- a/drivers/clk/clk-bd718x7.c
+++ b/drivers/clk/clk-bd718x7.c
@@ -81,27 +81,28 @@ static int bd71837_clk_probe(struct platform_device *pdev)
 	struct bd718xx_clk *c;
 	int rval = -ENOMEM;
 	const char *parent_clk;
+	struct device *dev = &pdev->dev;
 	struct device *parent = pdev->dev.parent;
 	struct clk_init_data init = {
 		.name = "bd718xx-32k-out",
 		.ops = &bd71837_clk_ops,
+		.num_parents = 1,
 	};
 	enum rohm_chip_type chip = platform_get_device_id(pdev)->driver_data;
 
-	c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
 	if (!c)
 		return -ENOMEM;
 
-	c->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	c->regmap = dev_get_regmap(parent, NULL);
 	if (!c->regmap)
 		return -ENODEV;
 
-	init.num_parents = 1;
 	parent_clk = of_clk_get_parent_name(parent->of_node, 0);
 
 	init.parent_names = &parent_clk;
 	if (!parent_clk) {
-		dev_err(&pdev->dev, "No parent clk found\n");
+		dev_err(dev, "No parent clk found\n");
 		return -EINVAL;
 	}
 	switch (chip) {
@@ -119,7 +120,7 @@ static int bd71837_clk_probe(struct platform_device *pdev)
 		c->mask = CLK_OUT_EN_MASK;
 		break;
 	default:
-		dev_err(&pdev->dev, "Unknown clk chip\n");
+		dev_err(dev, "Unknown clk chip\n");
 		return -EINVAL;
 	}
 	c->pdev = pdev;
@@ -128,15 +129,15 @@ static int bd71837_clk_probe(struct platform_device *pdev)
 	of_property_read_string_index(parent->of_node,
 				      "clock-output-names", 0, &init.name);
 
-	rval = devm_clk_hw_register(&pdev->dev, &c->hw);
+	rval = devm_clk_hw_register(dev, &c->hw);
 	if (rval) {
-		dev_err(&pdev->dev, "failed to register 32K clk");
+		dev_err(dev, "failed to register 32K clk");
 		return rval;
 	}
-	rval = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_simple_get,
+	rval = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
 					   &c->hw);
 	if (rval)
-		dev_err(&pdev->dev, "adding clk provider failed\n");
+		dev_err(dev, "adding clk provider failed\n");
 
 	return rval;
 }
-- 
2.25.1

Re: [RFC PATCH 1/3] clk: bd718x7: Clean up the code, no functional changes

[Matti Vaittinen]

On 6/5/22 19:57, Michael Trimarchi wrote:
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>   drivers/clk/clk-bd718x7.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
> index ac40b669d60b..04cc0beb67df 100644
> --- a/drivers/clk/clk-bd718x7.c
> +++ b/drivers/clk/clk-bd718x7.c
> @@ -81,27 +81,28 @@ static int bd71837_clk_probe(struct platform_device *pdev)
>   	struct bd718xx_clk *c;
>   	int rval = -ENOMEM;
>   	const char *parent_clk;
> +	struct device *dev = &pdev->dev;

I am not a fan of assigning pointers to struct members to local 
variables unless they're shortening lines to fit on one row instead of 
using two. Whenever we add such a variable we hide information. After 
that being said - in this particular case the device 'dev' points to is 
quite obvious so I am not completely against the change if other see the 
value.

>   	struct device *parent = pdev->dev.parent;
>   	struct clk_init_data init = {
>   		.name = "bd718xx-32k-out",
>   		.ops = &bd71837_clk_ops,
> +		.num_parents = 1,

I like this. Thanks.

>   	};

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

Best Regards
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

[RFC PATCH 2/3] clk: bd718x7: Enable the possibility to mark the clock as critical

[Michael Trimarchi]

If the clock is used to generate the osc_32k, we need to mark
as critical. clock-critical has no binding description at the moment
but it's defined in linux kernel

bd71847: pmic@4b {
...
	rohm,reset-snvs-powered;

	#clock-cells = <0>;
	clock-critical = <1>;
	clocks = <&osc_32k 0>;
	clock-output-names = "clk-32k-out";
...
}

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 drivers/clk/clk-bd718x7.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
index 04cc0beb67df..5088379ec2b7 100644
--- a/drivers/clk/clk-bd718x7.c
+++ b/drivers/clk/clk-bd718x7.c
@@ -83,6 +83,7 @@ static int bd71837_clk_probe(struct platform_device *pdev)
 	const char *parent_clk;
 	struct device *dev = &pdev->dev;
 	struct device *parent = pdev->dev.parent;
+	unsigned long flags;
 	struct clk_init_data init = {
 		.name = "bd718xx-32k-out",
 		.ops = &bd71837_clk_ops,
@@ -100,6 +101,9 @@ static int bd71837_clk_probe(struct platform_device *pdev)
 
 	parent_clk = of_clk_get_parent_name(parent->of_node, 0);
 
+	of_clk_detect_critical(dev->of_node, 0, &flags);
+	init.flags = flags;
+
 	init.parent_names = &parent_clk;
 	if (!parent_clk) {
 		dev_err(dev, "No parent clk found\n");
-- 
2.25.1

Re: [RFC PATCH 2/3] clk: bd718x7: Enable the possibility to mark the clock as critical

[Matti Vaittinen]

Hi Michael,

On 6/5/22 19:57, Michael Trimarchi wrote:
> If the clock is used to generate the osc_32k, we need to mark
> as critical. clock-critical has no binding description at the moment
> but it's defined in linux kernel
> 
> bd71847: pmic@4b {
> ...
> 	rohm,reset-snvs-powered;
> 
> 	#clock-cells = <0>;
> 	clock-critical = <1>;
> 	clocks = <&osc_32k 0>;
> 	clock-output-names = "clk-32k-out";
> ...
> }
> 
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>   drivers/clk/clk-bd718x7.c | 4 ++++

//snip

> @@ -100,6 +101,9 @@ static int bd71837_clk_probe(struct platform_device *pdev)
>   
>   	parent_clk = of_clk_get_parent_name(parent->of_node, 0);
>   
> +	of_clk_detect_critical(dev->of_node, 0, &flags);

Purely judging the kerneldoc for of_clk_detect_critical - you may have 
hard time getting this accepted.

I think you're working on a very valid problem though. Maybe you could 
see if you could align your effort with Marek?

https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564

Best Regards
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Re: [RFC PATCH 2/3] clk: bd718x7: Enable the possibility to mark the clock as critical

[Michael Nazzareno Trimarchi]

Hi

On Mon, Jun 6, 2022 at 7:26 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> Hi Michael,
>
> On 6/5/22 19:57, Michael Trimarchi wrote:
> > If the clock is used to generate the osc_32k, we need to mark
> > as critical. clock-critical has no binding description at the moment
> > but it's defined in linux kernel
> >
> > bd71847: pmic@4b {
> > ...
> >       rohm,reset-snvs-powered;
> >
> >       #clock-cells = <0>;
> >       clock-critical = <1>;
> >       clocks = <&osc_32k 0>;
> >       clock-output-names = "clk-32k-out";
> > ...
> > }
> >
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > ---
> >   drivers/clk/clk-bd718x7.c | 4 ++++
>
> //snip
>
> > @@ -100,6 +101,9 @@ static int bd71837_clk_probe(struct platform_device *pdev)
> >
> >       parent_clk = of_clk_get_parent_name(parent->of_node, 0);
> >
> > +     of_clk_detect_critical(dev->of_node, 0, &flags);
>
> Purely judging the kerneldoc for of_clk_detect_critical - you may have
> hard time getting this accepted.
>

This is the reason for RFC. I have already seen the usage of this in

ainline/master:drivers/clk/st/clk-flexgen.c:
of_clk_detect_critical(np, i, &flex_flags);
mainline/master:drivers/clk/st/clkgen-fsyn.c:
of_clk_detect_critical(np, fschan, &flags);
mainline/master:drivers/clk/st/clkgen-pll.c:
of_clk_detect_critical(np, 0, &pll_flags);
mainline/master:drivers/clk/st/clkgen-pll.c:
of_clk_detect_critical(np, odf, &odf_flags);

> I think you're working on a very valid problem though. Maybe you could
> see if you could align your effort with Marek?
>
> https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
>

I have seen the Marek patcheset now. I don't know if they works anyway
for a clock that is a part of MFD controller

Michael

> Best Regards
>         -- Matti
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~
>
> Discuss - Estimate - Plan - Report and finally accomplish this:
> void do_work(int time) __attribute__ ((const));



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [RFC PATCH 2/3] clk: bd718x7: Enable the possibility to mark the clock as critical

[Michael Nazzareno Trimarchi]

Hi Stephen

On Mon, Jun 6, 2022 at 7:26 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> Hi Michael,
>
> On 6/5/22 19:57, Michael Trimarchi wrote:
> > If the clock is used to generate the osc_32k, we need to mark
> > as critical. clock-critical has no binding description at the moment
> > but it's defined in linux kernel
> >
> > bd71847: pmic@4b {
> > ...
> >       rohm,reset-snvs-powered;
> >
> >       #clock-cells = <0>;
> >       clock-critical = <1>;
> >       clocks = <&osc_32k 0>;
> >       clock-output-names = "clk-32k-out";
> > ...
> > }
> >
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > ---
> >   drivers/clk/clk-bd718x7.c | 4 ++++
>
> //snip
>
> > @@ -100,6 +101,9 @@ static int bd71837_clk_probe(struct platform_device *pdev)
> >
> >       parent_clk = of_clk_get_parent_name(parent->of_node, 0);
> >
> > +     of_clk_detect_critical(dev->of_node, 0, &flags);
>
> Purely judging the kerneldoc for of_clk_detect_critical - you may have
> hard time getting this accepted.
>
> I think you're working on a very valid problem though. Maybe you could
> see if you could align your effort with Marek?
>
> https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
>

Old thread but same problem. Is there any way to make this acceptable?
any suggestion?

Michael

> Best Regards
>         -- Matti
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~
>
> Discuss - Estimate - Plan - Report and finally accomplish this:
> void do_work(int time) __attribute__ ((const));



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [RFC PATCH 2/3] clk: bd718x7: Enable the possibility to mark the clock as critical

[Matti Vaittinen]

On 8/14/24 19:00, Michael Nazzareno Trimarchi wrote:
> Hi Stephen
> 
> On Mon, Jun 6, 2022 at 7:26 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> Hi Michael,
>>
>> On 6/5/22 19:57, Michael Trimarchi wrote:
>>> If the clock is used to generate the osc_32k, we need to mark
>>> as critical. clock-critical has no binding description at the moment
>>> but it's defined in linux kernel
>>>
>>> bd71847: pmic@4b {
>>> ...
>>>        rohm,reset-snvs-powered;
>>>
>>>        #clock-cells = <0>;
>>>        clock-critical = <1>;
>>>        clocks = <&osc_32k 0>;
>>>        clock-output-names = "clk-32k-out";
>>> ...
>>> }
>>>
>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>>> ---
>>>    drivers/clk/clk-bd718x7.c | 4 ++++
>>
>> //snip
>>
>>> @@ -100,6 +101,9 @@ static int bd71837_clk_probe(struct platform_device *pdev)
>>>
>>>        parent_clk = of_clk_get_parent_name(parent->of_node, 0);
>>>
>>> +     of_clk_detect_critical(dev->of_node, 0, &flags);
>>
>> Purely judging the kerneldoc for of_clk_detect_critical - you may have
>> hard time getting this accepted.
>>
>> I think you're working on a very valid problem though. Maybe you could
>> see if you could align your effort with Marek?
>>
>> https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
>>
> 
> Old thread but same problem. Is there any way to make this acceptable?
> any suggestion?

Hi Michael. I'm not sure what is the correct way but I think there are a 
few tricks people have used to fix (or paper over) the problem. One was 
suggested by Sebastian:

https://lore.kernel.org/all/20220913152140.iikckob5h3ecagfi@mercury.elektranox.org/

No one shouted for implementing this fix though.

It also seems to me that there is a way to 'make things work' by 
modelling the clock dependencies in the DT in certain way, AND having 
correct drivers enabled. This understanding came just by reading mails 
Marek sent in this discussion:

https://lore.kernel.org/all/20220924174603.458956-1-marex@denx.de/

I've not tested any of this myself - but I hope you can use these as 
pointers to a solution that works for you...

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

[RFC PATCH 3/3] arm64: dts: imx8mn-bsh-smm-s2/pro: Add pmic clock connection

[Michael Trimarchi]

pmic clock is connected to svns_rtc using RTC_XTALI pin so we
need to mark this clock as critical. The clock source even the
wifi/bluetooth chipset

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2-common.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2-common.dtsi b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2-common.dtsi
index c11895d9d582..072d2c480f20 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2-common.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2-common.dtsi
@@ -28,6 +28,8 @@ usdhc2_pwrseq: usdhc2-pwrseq {
 		pinctrl-names = "default";
 		pinctrl-0 = <&pinctrl_usdhc2_pwrseq>;
 		reset-gpios = <&gpio4 27 GPIO_ACTIVE_LOW>;
+		clocks = <&bd71847 0>;
+		clock-names = "ext_clock";
 	};
 };
 
@@ -92,6 +94,7 @@ bd71847: pmic@4b {
 		rohm,reset-snvs-powered;
 
 		#clock-cells = <0>;
+		clock-critical = <1>;
 		clocks = <&osc_32k 0>;
 		clock-output-names = "clk-32k-out";
 
@@ -235,6 +238,8 @@ bluetooth {
 		shutdown-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
 		device-wakeup-gpios = <&gpio1 18 GPIO_ACTIVE_HIGH>;
 		host-wakeup-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+		clocks = <&bd71847 0>;
+		clock-names = "lpo";
 		max-speed = <3000000>;
 	};
 };
-- 
2.25.1