[PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Marek Vasut]

The PMIC is the 32 kHz clock source for the RTC_XTALI input of the SoC
on this system. The RTC_XTALI input is used to supply 32 kHz clock to
the SVNS RTC per "i.MX 8M Mini Applications Processor Reference Manual,
Rev. 3, 11/2020" page 759 "The 32KHz XTAL module uses a different IP and
it is used as the clock source for the RTC, located in the SNVS." The
PMIC has its own dedicated 32 kHz XTAL on input.

Model the connection in DT.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Fabio Estevam <festevam@denx.de>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Shawn Guo <shawnguo@kernel.org>
To: linux-arm-kernel@lists.infradead.org
---
 .../dts/freescale/imx8mm-data-modul-edm-sbc.dts     | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
index 6ff30cbb32fb2..575d5632296c5 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
@@ -53,6 +53,12 @@ clk_xtal25: clk-xtal25 {
 		clock-frequency = <25000000>;
 	};
 
+	clk_xtal32k: clk-xtal32k {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <32768>;
+	};
+
 	panel: panel {
 		backlight = <&backlight>;
 		power-supply = <&reg_panel_vcc>;
@@ -276,6 +282,9 @@ &i2c1 {
 	pmic: pmic@4b {
 		compatible = "rohm,bd71847";
 		reg = <0x4b>;
+		#clock-cells = <0>;
+		clocks = <&clk_xtal32k 0>;
+		clock-output-names = "clk-32k-out";
 		pinctrl-names = "default";
 		pinctrl-0 = <&pinctrl_pmic>;
 		interrupt-parent = <&gpio1>;
@@ -942,6 +951,10 @@ &sai5 {
 	status = "disabled";
 };
 
+&snvs_rtc {
+	clocks = <&pmic>;
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1>;
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Peng Fan]

On 9/25/2022 1:46 AM, Marek Vasut wrote:
> The PMIC is the 32 kHz clock source for the RTC_XTALI input of the SoC
> on this system. The RTC_XTALI input is used to supply 32 kHz clock to
> the SVNS RTC per "i.MX 8M Mini Applications Processor Reference Manual,
> Rev. 3, 11/2020" page 759 "The 32KHz XTAL module uses a different IP and
> it is used as the clock source for the RTC, located in the SNVS." The
> PMIC has its own dedicated 32 kHz XTAL on input.
> 
> Model the connection in DT.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> To: linux-arm-kernel@lists.infradead.org
> ---
>   .../dts/freescale/imx8mm-data-modul-edm-sbc.dts     | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> index 6ff30cbb32fb2..575d5632296c5 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> @@ -53,6 +53,12 @@ clk_xtal25: clk-xtal25 {
>   		clock-frequency = <25000000>;
>   	};
>   
> +	clk_xtal32k: clk-xtal32k {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <32768>;
> +	};
> +
>   	panel: panel {
>   		backlight = <&backlight>;
>   		power-supply = <&reg_panel_vcc>;
> @@ -276,6 +282,9 @@ &i2c1 {
>   	pmic: pmic@4b {
>   		compatible = "rohm,bd71847";
>   		reg = <0x4b>;
> +		#clock-cells = <0>;
> +		clocks = <&clk_xtal32k 0>;
> +		clock-output-names = "clk-32k-out";
>   		pinctrl-names = "default";
>   		pinctrl-0 = <&pinctrl_pmic>;
>   		interrupt-parent = <&gpio1>;
> @@ -942,6 +951,10 @@ &sai5 {
>   	status = "disabled";
>   };
>   
> +&snvs_rtc {
> +	clocks = <&pmic>;
> +};
> +
>   &uart1 {
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&pinctrl_uart1>;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Tim Harvey]

On Sat, Sep 24, 2022 at 10:47 AM Marek Vasut <marex@denx.de> wrote:
>
> The PMIC is the 32 kHz clock source for the RTC_XTALI input of the SoC
> on this system. The RTC_XTALI input is used to supply 32 kHz clock to
> the SVNS RTC per "i.MX 8M Mini Applications Processor Reference Manual,
> Rev. 3, 11/2020" page 759 "The 32KHz XTAL module uses a different IP and
> it is used as the clock source for the RTC, located in the SNVS." The
> PMIC has its own dedicated 32 kHz XTAL on input.
>
> Model the connection in DT.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> To: linux-arm-kernel@lists.infradead.org
> ---
>  .../dts/freescale/imx8mm-data-modul-edm-sbc.dts     | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> index 6ff30cbb32fb2..575d5632296c5 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
> @@ -53,6 +53,12 @@ clk_xtal25: clk-xtal25 {
>                 clock-frequency = <25000000>;
>         };
>
> +       clk_xtal32k: clk-xtal32k {
> +               compatible = "fixed-clock";
> +               #clock-cells = <0>;
> +               clock-frequency = <32768>;
> +       };
> +
>         panel: panel {
>                 backlight = <&backlight>;
>                 power-supply = <&reg_panel_vcc>;
> @@ -276,6 +282,9 @@ &i2c1 {
>         pmic: pmic@4b {
>                 compatible = "rohm,bd71847";
>                 reg = <0x4b>;
> +               #clock-cells = <0>;
> +               clocks = <&clk_xtal32k 0>;
> +               clock-output-names = "clk-32k-out";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_pmic>;
>                 interrupt-parent = <&gpio1>;
> @@ -942,6 +951,10 @@ &sai5 {
>         status = "disabled";
>  };
>
> +&snvs_rtc {
> +       clocks = <&pmic>;
> +};
> +
>  &uart1 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_uart1>;
> --
> 2.35.1
>
>

Marek,

The modeling here makes sense, but I tried this on the boards I have
with the rohm,bd71847 and it did not bump the clk_enable_count for
clk-32k-out and thus drivers/clk/clk-bd718x7.c still disables the
clock. Is something else required to make that happen?

Best Regards,

Tim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Marek Vasut]

On 9/27/22 21:43, Tim Harvey wrote:

Tim,

> Marek,
> 
> The modeling here makes sense, but I tried this on the boards I have
> with the rohm,bd71847 and it did not bump the clk_enable_count for
> clk-32k-out and thus drivers/clk/clk-bd718x7.c still disables the
> clock. Is something else required to make that happen?

The only thing I can think of is, do you have SNVS_RTC driver enabled 
and compiled in, just like the PMIC, or are they maybe modules ?

You can always try and add a printk() into the snvs rtc driver and see 
whether the clk_get there doesn't fail for some reason, and what the 
error code is.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Tim Harvey]

On Tue, Sep 27, 2022 at 1:10 PM Marek Vasut <marex@denx.de> wrote:
>
> On 9/27/22 21:43, Tim Harvey wrote:
>
> Tim,
>
> > Marek,
> >
> > The modeling here makes sense, but I tried this on the boards I have
> > with the rohm,bd71847 and it did not bump the clk_enable_count for
> > clk-32k-out and thus drivers/clk/clk-bd718x7.c still disables the
> > clock. Is something else required to make that happen?
>
> The only thing I can think of is, do you have SNVS_RTC driver enabled
> and compiled in, just like the PMIC, or are they maybe modules ?
>
> You can always try and add a printk() into the snvs rtc driver and see
> whether the clk_get there doesn't fail for some reason, and what the
> error code is.

Marek,

Thanks! I did 'not' have CONFIG_RTC_DRV_SNVS enabled in this test case
and as soon as I enable that it does bump the count and enable the
clock. We actually have a separate watchdog on our boards so I
typically disable the SNVS one to avoid the confusion of having two
watchdogs for our users. I tried adding 'clocks = <&pmic>' to the wdog
node and disalbing the RTC_DRV_SNVS again but it fails to enable that
clock.

Also I wonder if your patch deserves a 'Fixes: acb01032e11a5 ("arm64:
defconfig: Enable clock driver for ROHM BD718x7 PMIC")' tag?

Best Regards,

Tim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Marek Vasut]

On 9/27/22 22:23, Tim Harvey wrote:
> On Tue, Sep 27, 2022 at 1:10 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 9/27/22 21:43, Tim Harvey wrote:
>>
>> Tim,
>>
>>> Marek,
>>>
>>> The modeling here makes sense, but I tried this on the boards I have
>>> with the rohm,bd71847 and it did not bump the clk_enable_count for
>>> clk-32k-out and thus drivers/clk/clk-bd718x7.c still disables the
>>> clock. Is something else required to make that happen?
>>
>> The only thing I can think of is, do you have SNVS_RTC driver enabled
>> and compiled in, just like the PMIC, or are they maybe modules ?
>>
>> You can always try and add a printk() into the snvs rtc driver and see
>> whether the clk_get there doesn't fail for some reason, and what the
>> error code is.
> 
> Marek,

Tim,

> Thanks! I did 'not' have CONFIG_RTC_DRV_SNVS enabled in this test case
> and as soon as I enable that it does bump the count and enable the
> clock. We actually have a separate watchdog on our boards so I
> typically disable the SNVS one to avoid the confusion of having two
> watchdogs for our users. I tried adding 'clocks = <&pmic>' to the wdog
> node and disalbing the RTC_DRV_SNVS again but it fails to enable that
> clock.

I believe the 32 kHz fed into the SNVS RTC are mandatory, they must be 
supplied to the MX8M otherwise the SoC hangs. So whatever supplies the 
RTC_XTALI on your board should be connected to the SNVS RTC node clock 
and the SNVS RTC should be enabled.

> Also I wonder if your patch deserves a 'Fixes: acb01032e11a5 ("arm64:
> defconfig: Enable clock driver for ROHM BD718x7 PMIC")' tag?

No, since the current board DT without this patch is not really broken. 
Without the clock parts in the PMIC node, the PMIC just supplies 32 kHz 
clock and does not disable those clock, because Linux is not even aware 
of them, so everything works just fine even if the PMIC driver is 
enabled. This could potentially by a Fixes for this specific board DT, 
but I am don't think it's worth it either.

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Tim Harvey]

On Tue, Sep 27, 2022 at 1:31 PM Marek Vasut <marex@denx.de> wrote:
>
> On 9/27/22 22:23, Tim Harvey wrote:
> > On Tue, Sep 27, 2022 at 1:10 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 9/27/22 21:43, Tim Harvey wrote:
> >>
> >> Tim,
> >>
> >>> Marek,
> >>>
> >>> The modeling here makes sense, but I tried this on the boards I have
> >>> with the rohm,bd71847 and it did not bump the clk_enable_count for
> >>> clk-32k-out and thus drivers/clk/clk-bd718x7.c still disables the
> >>> clock. Is something else required to make that happen?
> >>
> >> The only thing I can think of is, do you have SNVS_RTC driver enabled
> >> and compiled in, just like the PMIC, or are they maybe modules ?
> >>
> >> You can always try and add a printk() into the snvs rtc driver and see
> >> whether the clk_get there doesn't fail for some reason, and what the
> >> error code is.
> >
> > Marek,
>
> Tim,
>
> > Thanks! I did 'not' have CONFIG_RTC_DRV_SNVS enabled in this test case
> > and as soon as I enable that it does bump the count and enable the
> > clock. We actually have a separate watchdog on our boards so I
> > typically disable the SNVS one to avoid the confusion of having two
> > watchdogs for our users. I tried adding 'clocks = <&pmic>' to the wdog
> > node and disalbing the RTC_DRV_SNVS again but it fails to enable that
> > clock.
>
> I believe the 32 kHz fed into the SNVS RTC are mandatory, they must be
> supplied to the MX8M otherwise the SoC hangs. So whatever supplies the
> RTC_XTALI on your board should be connected to the SNVS RTC node clock
> and the SNVS RTC should be enabled.

hmmm... I'm not sure if I agree that the SNVS RTC must be enabled. The
effect of the BD718XX CLK not being enabled is for sure that the SNVS
RTC does not tick 'and' the WDOG timer does not work (simulating a
kernel crash with sysreq_trigger for example hangs indefinitely). If I
leave the SNVS RTC disabled but force the CLK to be enabled (by
disabling CONFIG_COMMON_CLK_BD718XX) the WDOG behaves properly (as
well as the RTC if I have that enabled, but enabling it is not
required).

Maybe there is some board stability issue that I have failed to see as
I have not done much testing with the BD718XX CLK disabled but I
certainly want to make it enabled by default.

>
> > Also I wonder if your patch deserves a 'Fixes: acb01032e11a5 ("arm64:
> > defconfig: Enable clock driver for ROHM BD718x7 PMIC")' tag?
>
> No, since the current board DT without this patch is not really broken.
> Without the clock parts in the PMIC node, the PMIC just supplies 32 kHz
> clock and does not disable those clock, because Linux is not even aware
> of them, so everything works just fine even if the PMIC driver is
> enabled. This could potentially by a Fixes for this specific board DT,
> but I am don't think it's worth it either.

For any board with a BD718x7 PMIC providing the CLK to IMX8M RTC
without your dt change enabling CONFIG_COMMON_CLK_BD718XX will cause
the CLK to get diabled thus the SNVS RTC 'and' WDOG will not function.
So one could argue the dt change fixes the config enabling the clock
driver. It won't cause the board to crash, but it will cause it to not
wdog reset from a crash ;)

Tim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Marek Vasut]

On 9/27/22 22:43, Tim Harvey wrote:
> On Tue, Sep 27, 2022 at 1:31 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 9/27/22 22:23, Tim Harvey wrote:
>>> On Tue, Sep 27, 2022 at 1:10 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 9/27/22 21:43, Tim Harvey wrote:
>>>>
>>>> Tim,
>>>>
>>>>> Marek,
>>>>>
>>>>> The modeling here makes sense, but I tried this on the boards I have
>>>>> with the rohm,bd71847 and it did not bump the clk_enable_count for
>>>>> clk-32k-out and thus drivers/clk/clk-bd718x7.c still disables the
>>>>> clock. Is something else required to make that happen?
>>>>
>>>> The only thing I can think of is, do you have SNVS_RTC driver enabled
>>>> and compiled in, just like the PMIC, or are they maybe modules ?
>>>>
>>>> You can always try and add a printk() into the snvs rtc driver and see
>>>> whether the clk_get there doesn't fail for some reason, and what the
>>>> error code is.
>>>
>>> Marek,
>>
>> Tim,
>>
>>> Thanks! I did 'not' have CONFIG_RTC_DRV_SNVS enabled in this test case
>>> and as soon as I enable that it does bump the count and enable the
>>> clock. We actually have a separate watchdog on our boards so I
>>> typically disable the SNVS one to avoid the confusion of having two
>>> watchdogs for our users. I tried adding 'clocks = <&pmic>' to the wdog
>>> node and disalbing the RTC_DRV_SNVS again but it fails to enable that
>>> clock.
>>
>> I believe the 32 kHz fed into the SNVS RTC are mandatory, they must be
>> supplied to the MX8M otherwise the SoC hangs. So whatever supplies the
>> RTC_XTALI on your board should be connected to the SNVS RTC node clock
>> and the SNVS RTC should be enabled.
> 
> hmmm... I'm not sure if I agree that the SNVS RTC must be enabled. The
> effect of the BD718XX CLK not being enabled is for sure that the SNVS
> RTC does not tick 'and' the WDOG timer does not work (simulating a
> kernel crash with sysreq_trigger for example hangs indefinitely). If I
> leave the SNVS RTC disabled but force the CLK to be enabled (by
> disabling CONFIG_COMMON_CLK_BD718XX) the WDOG behaves properly (as
> well as the RTC if I have that enabled, but enabling it is not
> required).

"i.MX 8M Mini Applications Processor Reference Manual, Rev. 3, 11/2020" 
page 759 says the RTC_XTALI is 32 kHz supply to the SVNS RTC . Without 
the 32 kHz clock the SoC hangs. So I would say, yes, the SNVS RTC should 
be enabled. I have seen the i.MX8M (different ones) hang with 32 kHz 
RTC_XTALI disabled consistently, so the SoC clearly does require those 
clock, it is not a board property.

My personal hypothesis is that the 32k clock are also used for something 
else, something which is not listed in the datasheet, sigh.

> Maybe there is some board stability issue that I have failed to see as
> I have not done much testing with the BD718XX CLK disabled but I
> certainly want to make it enabled by default.
> 
>>
>>> Also I wonder if your patch deserves a 'Fixes: acb01032e11a5 ("arm64:
>>> defconfig: Enable clock driver for ROHM BD718x7 PMIC")' tag?
>>
>> No, since the current board DT without this patch is not really broken.
>> Without the clock parts in the PMIC node, the PMIC just supplies 32 kHz
>> clock and does not disable those clock, because Linux is not even aware
>> of them, so everything works just fine even if the PMIC driver is
>> enabled. This could potentially by a Fixes for this specific board DT,
>> but I am don't think it's worth it either.
> 
> For any board with a BD718x7 PMIC providing the CLK to IMX8M RTC
> without your dt change enabling CONFIG_COMMON_CLK_BD718XX will cause
> the CLK to get diabled thus the SNVS RTC 'and' WDOG will not function.

No, that is not the case. The bd7xxxx-clk is only probed if there are 
valid upstream clock specified in the PMIC DT node (the 'clocks = 
<&clk_xtal32k 0>;' part of this patch). If the clocks property is not 
present in the PMIC node, the bd7xxxx-clk won't probe and kernel won't 
disable the 32k PMIC clock. That's the case here, hence this board was 
not failing to start before, and won't fail to start with this patch, no 
matter whether or not the bd7xxxx-clk driver is compiled in or not, and 
so no need for the Fixes tag really.

> So one could argue the dt change fixes the config enabling the clock
> driver. It won't cause the board to crash, but it will cause it to not
> wdog reset from a crash ;)

I really disagree with that, you cannot really argue that DT change 
fixes defconfig, that is plain backwards ;)

I would rather suggest something else -- if you want to advertise this 
as a fix for the 32k clock hang, fix board you use which currently 
suffers from hang if you enable the bd7xxxx-clk driver, add Fixes tag 
for commit which added the board and reference the defconfig commit too. 
That would be the idea way to document that and tie all the pieces of 
information together.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC

[Shawn Guo]

On Sat, Sep 24, 2022 at 07:46:03PM +0200, Marek Vasut wrote:
> The PMIC is the 32 kHz clock source for the RTC_XTALI input of the SoC
> on this system. The RTC_XTALI input is used to supply 32 kHz clock to
> the SVNS RTC per "i.MX 8M Mini Applications Processor Reference Manual,
> Rev. 3, 11/2020" page 759 "The 32KHz XTAL module uses a different IP and
> it is used as the clock source for the RTC, located in the SNVS." The
> PMIC has its own dedicated 32 kHz XTAL on input.
> 
> Model the connection in DT.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Applied, thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel