BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Tim Harvey]

Greetings,

I've found that the bd71847 clk driver (CONFIG_COMMON_CLK_BD718XX
drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847 C32K_OUT
pin) which is connected IMX8MM RTC_XTALI which ends up disabling the
IMX RTC as well as the IMX WDOG functionality.

You can see this with:
# cat /sys/kernel/debug/clk/clk-32k-out/clk_rate
32768
# cat /sys/kernel/debug/clk/clk-32k-out/clk_enable_count
0
# cat /sys/class/rtc/rtc0/name
snvs_rtc 30370000.snvs:snvs-rtc-lp
# cat /sys/class/rtc/rtc0/time
00:00:03
^^^ time never changes

This happens via clk_unprepare_unused() as nothing is flagging the
clk-32k-out as being used. What should be added to the device-tree to
signify that this clk is indeed necessary and should not be disabled?

Best Regards,

Tim

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Matti Vaittinen]

Hi Tim,

On 9/2/22 01:23, Tim Harvey wrote:
> Greetings,
> 
> I've found that the bd71847 clk driver (CONFIG_COMMON_CLK_BD718XX
> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847 C32K_OUT
> pin) which is connected IMX8MM RTC_XTALI which ends up disabling the
> IMX RTC as well as the IMX WDOG functionality.

//snip

> This happens via clk_unprepare_unused() as nothing is flagging the
> clk-32k-out as being used. What should be added to the device-tree to
> signify that this clk is indeed necessary and should not be disabled?

I have seen following proposal from Marek Vasut:

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

I am not sure if the discussion is completed though. I guess it was 
agreed this was needed/usefull and maybe the remaining thing to decide 
was just the property naming.

Best Regards
	-- Matti

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

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

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Tim Harvey]

On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> Hi Tim,
>
> On 9/2/22 01:23, Tim Harvey wrote:
> > Greetings,
> >
> > I've found that the bd71847 clk driver (CONFIG_COMMON_CLK_BD718XX
> > drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847 C32K_OUT
> > pin) which is connected IMX8MM RTC_XTALI which ends up disabling the
> > IMX RTC as well as the IMX WDOG functionality.
>
> //snip
>
> > This happens via clk_unprepare_unused() as nothing is flagging the
> > clk-32k-out as being used. What should be added to the device-tree to
> > signify that this clk is indeed necessary and should not be disabled?
>
> I have seen following proposal from Marek Vasut:
>
> https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
>
> I am not sure if the discussion is completed though. I guess it was
> agreed this was needed/usefull and maybe the remaining thing to decide
> was just the property naming.
>
> Best Regards
>         -- Matti
>

Thanks Matti,

Marek - has there been any progress on determining how best to keep
certain clocks from being disabled?

Best Regards,

Tim

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Marek Vasut]

On 9/8/22 18:00, Tim Harvey wrote:
> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>> Hi Tim,
>>
>> On 9/2/22 01:23, Tim Harvey wrote:
>>> Greetings,
>>>
>>> I've found that the bd71847 clk driver (CONFIG_COMMON_CLK_BD718XX
>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847 C32K_OUT
>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling the
>>> IMX RTC as well as the IMX WDOG functionality.
>>
>> //snip
>>
>>> This happens via clk_unprepare_unused() as nothing is flagging the
>>> clk-32k-out as being used. What should be added to the device-tree to
>>> signify that this clk is indeed necessary and should not be disabled?
>>
>> I have seen following proposal from Marek Vasut:
>>
>> https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
>>
>> I am not sure if the discussion is completed though. I guess it was
>> agreed this was needed/usefull and maybe the remaining thing to decide
>> was just the property naming.
>>
>> Best Regards
>>          -- Matti
>>
> 
> Thanks Matti,
> 
> Marek - has there been any progress on determining how best to keep
> certain clocks from being disabled?

No. You can read the discussion above.

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Tim Harvey]

On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
>
> On 9/8/22 18:00, Tim Harvey wrote:
> > On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >>
> >> Hi Tim,
> >>
> >> On 9/2/22 01:23, Tim Harvey wrote:
> >>> Greetings,
> >>>
> >>> I've found that the bd71847 clk driver (CONFIG_COMMON_CLK_BD718XX
> >>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847 C32K_OUT
> >>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling the
> >>> IMX RTC as well as the IMX WDOG functionality.
> >>
> >> //snip
> >>
> >>> This happens via clk_unprepare_unused() as nothing is flagging the
> >>> clk-32k-out as being used. What should be added to the device-tree to
> >>> signify that this clk is indeed necessary and should not be disabled?
> >>
> >> I have seen following proposal from Marek Vasut:
> >>
> >> https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
> >>
> >> I am not sure if the discussion is completed though. I guess it was
> >> agreed this was needed/usefull and maybe the remaining thing to decide
> >> was just the property naming.
> >>
> >> Best Regards
> >>          -- Matti
> >>
> >
> > Thanks Matti,
> >
> > Marek - has there been any progress on determining how best to keep
> > certain clocks from being disabled?
>
> No. You can read the discussion above.

Marek,

I wasn't on the linux-clk list at that time so can't respond to the
thread but the discussion seems to have died out a couple of months
ago with no agreement between you or Stephen on how to deal with it.

So where do we take this from here? It looks like there are about 18
boards with dt's using "rohm,bd718*" which would all have non working
RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
arch/arm64/configs/defconfig) right?

$ git grep "rohm,bd718" arch/ | cut
-d: -f1
arch/arm64/boot/dts/freescale/imx8mm-beacon-som.dtsi
arch/arm64/boot/dts/freescale/imx8mm-data-modul-edm-sbc.dts
arch/arm64/boot/dts/freescale/imx8mm-emcon.dtsi
arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
arch/arm64/boot/dts/freescale/imx8mm-var-som.dtsi
arch/arm64/boot/dts/freescale/imx8mm-venice-gw7901.dts
arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts
arch/arm64/boot/dts/freescale/imx8mm-venice-gw7903.dts
arch/arm64/boot/dts/freescale/imx8mm-venice-gw7904.dts
arch/arm64/boot/dts/freescale/imx8mn-beacon-som.dtsi
arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2-common.dtsi
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
arch/arm64/boot/dts/freescale/imx8mn-var-som.dtsi
arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
arch/arm64/boot/dts/freescale/imx8mq-phanbell.dts
arch/arm64/boot/dts/freescale/imx8mq-pico-pi.dts

Best Regards,

Tim

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Marek Vasut]

On 9/8/22 21:25, Tim Harvey wrote:
> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 9/8/22 18:00, Tim Harvey wrote:
>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>>
>>>> Hi Tim,
>>>>
>>>> On 9/2/22 01:23, Tim Harvey wrote:
>>>>> Greetings,
>>>>>
>>>>> I've found that the bd71847 clk driver (CONFIG_COMMON_CLK_BD718XX
>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847 C32K_OUT
>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling the
>>>>> IMX RTC as well as the IMX WDOG functionality.
>>>>
>>>> //snip
>>>>
>>>>> This happens via clk_unprepare_unused() as nothing is flagging the
>>>>> clk-32k-out as being used. What should be added to the device-tree to
>>>>> signify that this clk is indeed necessary and should not be disabled?
>>>>
>>>> I have seen following proposal from Marek Vasut:
>>>>
>>>> https://lore.kernel.org/all/20220517235919.200375-1-marex@denx.de/T/#m52d6d0831bf43d5f293e35cb27f3021f278d0564
>>>>
>>>> I am not sure if the discussion is completed though. I guess it was
>>>> agreed this was needed/usefull and maybe the remaining thing to decide
>>>> was just the property naming.
>>>>
>>>> Best Regards
>>>>           -- Matti
>>>>
>>>
>>> Thanks Matti,
>>>
>>> Marek - has there been any progress on determining how best to keep
>>> certain clocks from being disabled?
>>
>> No. You can read the discussion above.
> 
> Marek,
> 
> I wasn't on the linux-clk list at that time so can't respond to the
> thread but the discussion seems to have died out a couple of months
> ago with no agreement between you or Stephen on how to deal with it.
> 
> So where do we take this from here? It looks like there are about 18
> boards with dt's using "rohm,bd718*" which would all have non working
> RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
> arch/arm64/configs/defconfig) right?

Feel free to continue the effort.

RE: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Peng Fan]

> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure
> 
> On 9/8/22 21:25, Tim Harvey wrote:
> > On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 9/8/22 18:00, Tim Harvey wrote:
> >>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
> >>>>
> >>>> Hi Tim,
> >>>>
> >>>> On 9/2/22 01:23, Tim Harvey wrote:
> >>>>> Greetings,
> >>>>>
> >>>>> I've found that the bd71847 clk driver
> (CONFIG_COMMON_CLK_BD718XX
> >>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
> >>>>> C32K_OUT
> >>>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling
> >>>>> the IMX RTC as well as the IMX WDOG functionality.
> >>>>
> >>>> //snip
> >>>>
> >>>>> This happens via clk_unprepare_unused() as nothing is flagging the
> >>>>> clk-32k-out as being used. What should be added to the device-tree
> >>>>> to signify that this clk is indeed necessary and should not be disabled?
> >>>>
> >>>> I have seen following proposal from Marek Vasut:
> >>>>
> >>>>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
> >>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
> marex%40denx.de%2FT%
> >>>>
> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
> 1%7Cp
> >>>>
> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
> 3bc2b
> >>>>
> 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
> 7CTWFpb
> >>>>
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6
> >>>>
> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
> 2FLByaEhh5
> >>>> Dtah9K8CcAOAM%3D&amp;reserved=0
> >>>>
> >>>> I am not sure if the discussion is completed though. I guess it was
> >>>> agreed this was needed/usefull and maybe the remaining thing to
> >>>> decide was just the property naming.
> >>>>
> >>>> Best Regards
> >>>>           -- Matti
> >>>>
> >>>
> >>> Thanks Matti,
> >>>
> >>> Marek - has there been any progress on determining how best to keep
> >>> certain clocks from being disabled?
> >>
> >> No. You can read the discussion above.
> >
> > Marek,
> >
> > I wasn't on the linux-clk list at that time so can't respond to the
> > thread but the discussion seems to have died out a couple of months
> > ago with no agreement between you or Stephen on how to deal with it.
> >
> > So where do we take this from here? It looks like there are about 18
> > boards with dt's using "rohm,bd718*" which would all have non working
> > RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
> > arch/arm64/configs/defconfig) right?

Is there any requirement that the bd718xx clk needs to be runtime on/off?
I suppose the clk should always be never be off, if yes, why not have something:

diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
index ac40b669d60b..109cef35418b 100644
--- a/drivers/clk/clk-bd718x7.c
+++ b/drivers/clk/clk-bd718x7.c
@@ -104,6 +104,8 @@ static int bd71837_clk_probe(struct platform_device *pdev)
                dev_err(&pdev->dev, "No parent clk found\n");
                return -EINVAL;
        }
+       init.flags = CLK_IGNORE_UNUSED;
+
        switch (chip) {
        case ROHM_CHIP_TYPE_BD71837:
        case ROHM_CHIP_TYPE_BD71847:

> 
> Feel free to continue the effort.

Tim, would you continue the effort?

Regards,
Peng.

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Marek Vasut]

On 9/9/22 04:06, Peng Fan wrote:
>> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure
>>
>> On 9/8/22 21:25, Tim Harvey wrote:
>>> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>> <mazziesaccount@gmail.com> wrote:
>>>>>>
>>>>>> Hi Tim,
>>>>>>
>>>>>> On 9/2/22 01:23, Tim Harvey wrote:
>>>>>>> Greetings,
>>>>>>>
>>>>>>> I've found that the bd71847 clk driver
>> (CONFIG_COMMON_CLK_BD718XX
>>>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
>>>>>>> C32K_OUT
>>>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling
>>>>>>> the IMX RTC as well as the IMX WDOG functionality.
>>>>>>
>>>>>> //snip
>>>>>>
>>>>>>> This happens via clk_unprepare_unused() as nothing is flagging the
>>>>>>> clk-32k-out as being used. What should be added to the device-tree
>>>>>>> to signify that this clk is indeed necessary and should not be disabled?
>>>>>>
>>>>>> I have seen following proposal from Marek Vasut:
>>>>>>
>>>>>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
>>>>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
>> marex%40denx.de%2FT%
>>>>>>
>> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
>> 1%7Cp
>>>>>>
>> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
>> 3bc2b
>>>>>>
>> 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
>> 7CTWFpb
>>>>>>
>> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
>> 6
>>>>>>
>> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
>> 2FLByaEhh5
>>>>>> Dtah9K8CcAOAM%3D&amp;reserved=0
>>>>>>
>>>>>> I am not sure if the discussion is completed though. I guess it was
>>>>>> agreed this was needed/usefull and maybe the remaining thing to
>>>>>> decide was just the property naming.
>>>>>>
>>>>>> Best Regards
>>>>>>            -- Matti
>>>>>>
>>>>>
>>>>> Thanks Matti,
>>>>>
>>>>> Marek - has there been any progress on determining how best to keep
>>>>> certain clocks from being disabled?
>>>>
>>>> No. You can read the discussion above.
>>>
>>> Marek,
>>>
>>> I wasn't on the linux-clk list at that time so can't respond to the
>>> thread but the discussion seems to have died out a couple of months
>>> ago with no agreement between you or Stephen on how to deal with it.
>>>
>>> So where do we take this from here? It looks like there are about 18
>>> boards with dt's using "rohm,bd718*" which would all have non working
>>> RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
>>> arch/arm64/configs/defconfig) right?
> 
> Is there any requirement that the bd718xx clk needs to be runtime on/off?

Yes, the 32kHz clock on BD71xxx should behave like any other clock, 
unless specified otherwise, see below.

> I suppose the clk should always be never be off, if yes, why not have something:

What is needed in this specific case of BD718xx is I think clock 
consumer on the MX8M clock driver side which would claim the 32kHz input 
from the PMIC and up the clock enable count to keep the 32 kHz clock 
always on. The PMIC is most likely supplying 32 kHz clock to the MX8M, 
which if the 32 kHz clock are turned off would hang (I observed that 
before too).

What I tried to address in this thread is a generic problem which 
commonly appears on various embedded systems, except every time anyone 
tried to solve it in a generic manner, it was rejected or they gave up.

The problem is this -- you have an arbitrary clock, and you need to keep 
it running always otherwise the system fails, and you do not have a 
clock consumer in the DT for whatever reason e.g. because the SoC is 
only used as a clock source for some unrelated clock net. There must be 
a way to mark the clock as "never disable these", i.e. critical-clock. 
(I feel like I keep repeating this over and over in this thread, so 
please read the whole thread backlog)

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Matti Vaittinen]

Hi dee Ho peeps,

On 9/9/22 05:35, Marek Vasut wrote:
> On 9/9/22 04:06, Peng Fan wrote:
>>> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT 
>>> failure
>>>
>>> On 9/8/22 21:25, Tim Harvey wrote:
>>>> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
>>>>>>>
>>>>>>> Hi Tim,
>>>>>>>
>>>>>>> On 9/2/22 01:23, Tim Harvey wrote:
>>>>>>>> Greetings,
>>>>>>>>
>>>>>>>> I've found that the bd71847 clk driver
>>> (CONFIG_COMMON_CLK_BD718XX
>>>>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
>>>>>>>> C32K_OUT
>>>>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling
>>>>>>>> the IMX RTC as well as the IMX WDOG functionality.
>>>>>>>
>>>>>>> //snip
>>>>>>>
>>>>>>>> This happens via clk_unprepare_unused() as nothing is flagging the
>>>>>>>> clk-32k-out as being used. What should be added to the device-tree
>>>>>>>> to signify that this clk is indeed necessary and should not be 
>>>>>>>> disabled?
>>>>>>>
>>>>>>> I have seen following proposal from Marek Vasut:
>>>>>>>
>>>>>>>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
>>>>>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
>>> marex%40denx.de%2FT%
>>>>>>>
>>> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
>>> 1%7Cp
>>>>>>>
>>> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
>>> 3bc2b
>>>>>>>
>>> 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
>>> 7CTWFpb
>>>>>>>
>>> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
>>> 6
>>>>>>>
>>> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
>>> 2FLByaEhh5
>>>>>>> Dtah9K8CcAOAM%3D&amp;reserved=0
>>>>>>>
>>>>>>> I am not sure if the discussion is completed though. I guess it was
>>>>>>> agreed this was needed/usefull and maybe the remaining thing to
>>>>>>> decide was just the property naming.
>>>>>>>
>>>>>>> Best Regards
>>>>>>>            -- Matti
>>>>>>>
>>>>>>
>>>>>> Thanks Matti,
>>>>>>
>>>>>> Marek - has there been any progress on determining how best to keep
>>>>>> certain clocks from being disabled?
>>>>>
>>>>> No. You can read the discussion above.
>>>>
>>>> Marek,
>>>>
>>>> I wasn't on the linux-clk list at that time so can't respond to the
>>>> thread but the discussion seems to have died out a couple of months
>>>> ago with no agreement between you or Stephen on how to deal with it.
>>>>
>>>> So where do we take this from here? It looks like there are about 18
>>>> boards with dt's using "rohm,bd718*" which would all have non working
>>>> RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
>>>> arch/arm64/configs/defconfig) right?

Yeah. The ROHM BD71837 and BD71847 (and BD71850 - which is one of the 
variants) are used quite a lot. I am pretty sure not fixing this in 
upstream is increasing downstream solutions. I don't think that should 
be preferred.

>>
>> Is there any requirement that the bd718xx clk needs to be runtime on/off?
> 
> Yes, the 32kHz clock on BD71xxx should behave like any other clock, 
> unless specified otherwise, see below.
> 
>> I suppose the clk should always be never be off, if yes, why not have 
>> something:
> 
> What is needed in this specific case of BD718xx is I think clock 
> consumer on the MX8M clock driver side which would claim the 32kHz input 
> from the PMIC and up the clock enable count to keep the 32 kHz clock 
> always on.

This sounds like a solution that would describe the actual HW setup. I 
don't know the CCF of the i.MX8 well enough to tell whether this can 
ensure the clk is not disabled before the consumer is found or when the 
consumer is going down though. Simplest thing to me would really be to 
just mark the clk as "do-not-touch" one on the boards where it must not 
be touched.

  The PMIC is most likely supplying 32 kHz clock to the MX8M,
> which if the 32 kHz clock are turned off would hang (I observed that 
> before too).
> 
> What I tried to address in this thread is a generic problem which 
> commonly appears on various embedded systems, except every time anyone 
> tried to solve it in a generic manner, it was rejected or they gave up.

I agree with Marek - generic solution would be nice. I don't think this 
is something specific to this PMIC.

> The problem is this -- you have an arbitrary clock, and you need to keep 
> it running always otherwise the system fails, and you do not have a 
> clock consumer in the DT for whatever reason e.g. because the SoC is 
> only used as a clock source for some unrelated clock net. There must be 
> a way to mark the clock as "never disable these", i.e. critical-clock. 
> (I feel like I keep repeating this over and over in this thread, so 
> please read the whole thread backlog)

Thanks for the explanation and effor you did Marek.

My take on this is that from a (generic) component vendor perspective it 
is a bad idea to hard-code the clock status (enable/disable) in the PMIC 
driver. A vendor wants to provide a driver which allows use of the 
component in wide variety of systems/boards. When the PMIC contains a 
clock gate, the PMIC driver should provide the means of controlling it. 
Some setups may want it enabled, other disabled and some want runtime 
control. This "use-policy" must not be hard coded in the driver - it 
needs to come from HW description which explains how the clk line is 
wired and potentially also from the consumer drivers. This enables the 
same PMIC driver to support all different setups with their own needs, 
right?

I am not sure if some non email discussions have been ongoing around 
this topic but just by reading the emails it seemed to me that Marek's 
suggestion was acked by the DT folks - and I don't think that Stephen 
was (at the end of the day) against that either(?). Maybe I missed 
something.

Yours
	-- Matti

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

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

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Peng Fan]

On 9/9/2022 1:06 PM, Matti Vaittinen wrote:
> Hi dee Ho peeps,
> 
> On 9/9/22 05:35, Marek Vasut wrote:
>> On 9/9/22 04:06, Peng Fan wrote:
>>>> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT 
>>>> failure
>>>>
>>>> On 9/8/22 21:25, Tim Harvey wrote:
>>>>> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>>>> <mazziesaccount@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Hi Tim,
>>>>>>>>
>>>>>>>> On 9/2/22 01:23, Tim Harvey wrote:
>>>>>>>>> Greetings,
>>>>>>>>>
>>>>>>>>> I've found that the bd71847 clk driver
>>>> (CONFIG_COMMON_CLK_BD718XX
>>>>>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
>>>>>>>>> C32K_OUT
>>>>>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling
>>>>>>>>> the IMX RTC as well as the IMX WDOG functionality.
>>>>>>>>
>>>>>>>> //snip
>>>>>>>>
>>>>>>>>> This happens via clk_unprepare_unused() as nothing is flagging the
>>>>>>>>> clk-32k-out as being used. What should be added to the device-tree
>>>>>>>>> to signify that this clk is indeed necessary and should not be 
>>>>>>>>> disabled?
>>>>>>>>
>>>>>>>> I have seen following proposal from Marek Vasut:
>>>>>>>>
>>>>>>>>
>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
>>>>>>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
>>>> marex%40denx.de%2FT%
>>>>>>>>
>>>> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
>>>> 1%7Cp
>>>>>>>>
>>>> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
>>>> 3bc2b
>>>>>>>>
>>>> 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
>>>> 7CTWFpb
>>>>>>>>
>>>> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
>>>> 6
>>>>>>>>
>>>> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
>>>> 2FLByaEhh5
>>>>>>>> Dtah9K8CcAOAM%3D&amp;reserved=0
>>>>>>>>
>>>>>>>> I am not sure if the discussion is completed though. I guess it was
>>>>>>>> agreed this was needed/usefull and maybe the remaining thing to
>>>>>>>> decide was just the property naming.
>>>>>>>>
>>>>>>>> Best Regards
>>>>>>>>            -- Matti
>>>>>>>>
>>>>>>>
>>>>>>> Thanks Matti,
>>>>>>>
>>>>>>> Marek - has there been any progress on determining how best to keep
>>>>>>> certain clocks from being disabled?
>>>>>>
>>>>>> No. You can read the discussion above.
>>>>>
>>>>> Marek,
>>>>>
>>>>> I wasn't on the linux-clk list at that time so can't respond to the
>>>>> thread but the discussion seems to have died out a couple of months
>>>>> ago with no agreement between you or Stephen on how to deal with it.
>>>>>
>>>>> So where do we take this from here? It looks like there are about 18
>>>>> boards with dt's using "rohm,bd718*" which would all have non working
>>>>> RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
>>>>> arch/arm64/configs/defconfig) right?
> 
> Yeah. The ROHM BD71837 and BD71847 (and BD71850 - which is one of the 
> variants) are used quite a lot. I am pretty sure not fixing this in 
> upstream is increasing downstream solutions. I don't think that should 
> be preferred.
> 
>>>
>>> Is there any requirement that the bd718xx clk needs to be runtime 
>>> on/off?
>>
>> Yes, the 32kHz clock on BD71xxx should behave like any other clock, 
>> unless specified otherwise, see below.
>>
>>> I suppose the clk should always be never be off, if yes, why not have 
>>> something:
>>
>> What is needed in this specific case of BD718xx is I think clock 
>> consumer on the MX8M clock driver side which would claim the 32kHz 
>> input from the PMIC and up the clock enable count to keep the 32 kHz 
>> clock always on.
> 
> This sounds like a solution that would describe the actual HW setup. I 
> don't know the CCF of the i.MX8 well enough to tell whether this can 
> ensure the clk is not disabled before the consumer is found or when the 
> consumer is going down though. Simplest thing to me would really be to 
> just mark the clk as "do-not-touch" one on the boards where it must not 
> be touched.
> 
>   The PMIC is most likely supplying 32 kHz clock to the MX8M,
>> which if the 32 kHz clock are turned off would hang (I observed that 
>> before too).
>>
>> What I tried to address in this thread is a generic problem which 
>> commonly appears on various embedded systems, except every time anyone 
>> tried to solve it in a generic manner, it was rejected or they gave up.
> 
> I agree with Marek - generic solution would be nice. I don't think this 
> is something specific to this PMIC.
> 
>> The problem is this -- you have an arbitrary clock, and you need to 
>> keep it running always otherwise the system fails, and you do not have 
>> a clock consumer in the DT for whatever reason e.g. because the SoC is 
>> only used as a clock source for some unrelated clock net. There must 
>> be a way to mark the clock as "never disable these", i.e. 
>> critical-clock. (I feel like I keep repeating this over and over in 
>> this thread, so please read the whole thread backlog)
> 
> Thanks for the explanation and effor you did Marek.
> 
> My take on this is that from a (generic) component vendor perspective it 
> is a bad idea to hard-code the clock status (enable/disable) in the PMIC 
> driver. A vendor wants to provide a driver which allows use of the 
> component in wide variety of systems/boards. When the PMIC contains a 
> clock gate, the PMIC driver should provide the means of controlling it. 
> Some setups may want it enabled, other disabled and some want runtime 
> control. This "use-policy" must not be hard coded in the driver - it 
> needs to come from HW description which explains how the clk line is 
> wired and potentially also from the consumer drivers. This enables the 
> same PMIC driver to support all different setups with their own needs, 
> right?
> 
> I am not sure if some non email discussions have been ongoing around 
> this topic but just by reading the emails it seemed to me that Marek's 
> suggestion was acked by the DT folks - and I don't think that Stephen 
> was (at the end of the day) against that either(?). Maybe I missed 
> something.

After a thought, maybe an easier way is to add a optional property
xxx,32k-always-on to the pmic node/driver.

Regards,
Peng.

> 
> Yours
>      -- Matti
> 

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Tim Harvey]

On Mon, Sep 12, 2022 at 12:40 AM Peng Fan <peng.fan@oss.nxp.com> wrote:
>
>
>
> On 9/9/2022 1:06 PM, Matti Vaittinen wrote:
> > Hi dee Ho peeps,
> >
> > On 9/9/22 05:35, Marek Vasut wrote:
> >> On 9/9/22 04:06, Peng Fan wrote:
> >>>> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT
> >>>> failure
> >>>>
> >>>> On 9/8/22 21:25, Tim Harvey wrote:
> >>>>> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 9/8/22 18:00, Tim Harvey wrote:
> >>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> >>>> <mazziesaccount@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> Hi Tim,
> >>>>>>>>
> >>>>>>>> On 9/2/22 01:23, Tim Harvey wrote:
> >>>>>>>>> Greetings,
> >>>>>>>>>
> >>>>>>>>> I've found that the bd71847 clk driver
> >>>> (CONFIG_COMMON_CLK_BD718XX
> >>>>>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
> >>>>>>>>> C32K_OUT
> >>>>>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling
> >>>>>>>>> the IMX RTC as well as the IMX WDOG functionality.
> >>>>>>>>
> >>>>>>>> //snip
> >>>>>>>>
> >>>>>>>>> This happens via clk_unprepare_unused() as nothing is flagging the
> >>>>>>>>> clk-32k-out as being used. What should be added to the device-tree
> >>>>>>>>> to signify that this clk is indeed necessary and should not be
> >>>>>>>>> disabled?
> >>>>>>>>
> >>>>>>>> I have seen following proposal from Marek Vasut:
> >>>>>>>>
> >>>>>>>>
> >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
> >>>>>>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
> >>>> marex%40denx.de%2FT%
> >>>>>>>>
> >>>> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
> >>>> 1%7Cp
> >>>>>>>>
> >>>> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
> >>>> 3bc2b
> >>>>>>>>
> >>>> 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
> >>>> 7CTWFpb
> >>>>>>>>
> >>>> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> >>>> 6
> >>>>>>>>
> >>>> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
> >>>> 2FLByaEhh5
> >>>>>>>> Dtah9K8CcAOAM%3D&amp;reserved=0
> >>>>>>>>
> >>>>>>>> I am not sure if the discussion is completed though. I guess it was
> >>>>>>>> agreed this was needed/usefull and maybe the remaining thing to
> >>>>>>>> decide was just the property naming.
> >>>>>>>>
> >>>>>>>> Best Regards
> >>>>>>>>            -- Matti
> >>>>>>>>
> >>>>>>>
> >>>>>>> Thanks Matti,
> >>>>>>>
> >>>>>>> Marek - has there been any progress on determining how best to keep
> >>>>>>> certain clocks from being disabled?
> >>>>>>
> >>>>>> No. You can read the discussion above.
> >>>>>
> >>>>> Marek,
> >>>>>
> >>>>> I wasn't on the linux-clk list at that time so can't respond to the
> >>>>> thread but the discussion seems to have died out a couple of months
> >>>>> ago with no agreement between you or Stephen on how to deal with it.
> >>>>>
> >>>>> So where do we take this from here? It looks like there are about 18
> >>>>> boards with dt's using "rohm,bd718*" which would all have non working
> >>>>> RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
> >>>>> arch/arm64/configs/defconfig) right?
> >
> > Yeah. The ROHM BD71837 and BD71847 (and BD71850 - which is one of the
> > variants) are used quite a lot. I am pretty sure not fixing this in
> > upstream is increasing downstream solutions. I don't think that should
> > be preferred.
> >
> >>>
> >>> Is there any requirement that the bd718xx clk needs to be runtime
> >>> on/off?
> >>
> >> Yes, the 32kHz clock on BD71xxx should behave like any other clock,
> >> unless specified otherwise, see below.
> >>
> >>> I suppose the clk should always be never be off, if yes, why not have
> >>> something:
> >>
> >> What is needed in this specific case of BD718xx is I think clock
> >> consumer on the MX8M clock driver side which would claim the 32kHz
> >> input from the PMIC and up the clock enable count to keep the 32 kHz
> >> clock always on.
> >
> > This sounds like a solution that would describe the actual HW setup. I
> > don't know the CCF of the i.MX8 well enough to tell whether this can
> > ensure the clk is not disabled before the consumer is found or when the
> > consumer is going down though. Simplest thing to me would really be to
> > just mark the clk as "do-not-touch" one on the boards where it must not
> > be touched.
> >
> >   The PMIC is most likely supplying 32 kHz clock to the MX8M,
> >> which if the 32 kHz clock are turned off would hang (I observed that
> >> before too).
> >>
> >> What I tried to address in this thread is a generic problem which
> >> commonly appears on various embedded systems, except every time anyone
> >> tried to solve it in a generic manner, it was rejected or they gave up.
> >
> > I agree with Marek - generic solution would be nice. I don't think this
> > is something specific to this PMIC.
> >
> >> The problem is this -- you have an arbitrary clock, and you need to
> >> keep it running always otherwise the system fails, and you do not have
> >> a clock consumer in the DT for whatever reason e.g. because the SoC is
> >> only used as a clock source for some unrelated clock net. There must
> >> be a way to mark the clock as "never disable these", i.e.
> >> critical-clock. (I feel like I keep repeating this over and over in
> >> this thread, so please read the whole thread backlog)
> >
> > Thanks for the explanation and effor you did Marek.
> >
> > My take on this is that from a (generic) component vendor perspective it
> > is a bad idea to hard-code the clock status (enable/disable) in the PMIC
> > driver. A vendor wants to provide a driver which allows use of the
> > component in wide variety of systems/boards. When the PMIC contains a
> > clock gate, the PMIC driver should provide the means of controlling it.
> > Some setups may want it enabled, other disabled and some want runtime
> > control. This "use-policy" must not be hard coded in the driver - it
> > needs to come from HW description which explains how the clk line is
> > wired and potentially also from the consumer drivers. This enables the
> > same PMIC driver to support all different setups with their own needs,
> > right?
> >
> > I am not sure if some non email discussions have been ongoing around
> > this topic but just by reading the emails it seemed to me that Marek's
> > suggestion was acked by the DT folks - and I don't think that Stephen
> > was (at the end of the day) against that either(?). Maybe I missed
> > something.
>
> After a thought, maybe an easier way is to add a optional property
> xxx,32k-always-on to the pmic node/driver.
>
> Regards,
> Peng.

Is there simply a way to add the clk to the snvs_rtc and the wdog dt
nodes so they have a use count and don't get disabled?

Best Regards,

Tim

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Matti Vaittinen]

On 9/12/22 20:15, Tim Harvey wrote:
> On Mon, Sep 12, 2022 at 12:40 AM Peng Fan <peng.fan@oss.nxp.com> wrote:

//snip

>>
>> After a thought, maybe an easier way is to add a optional property
>> xxx,32k-always-on to the pmic node/driver.
>>

Yes, that would be easy. Yet, creating a driver specific DT-property 
feels a tad wrong. I don't think the BD718xx is in any way special so it 
should not need such a vendor specific property. It might be better to 
find more generic solution.

> Is there simply a way to add the clk to the snvs_rtc and the wdog dt
> nodes so they have a use count and don't get disabled?

To me that does sound like the right thing to do. If we have a consumer 
which requires the clock, then describing this dependency in DT sounds 
like a correct approach - assuming this keeps the clock enabled without 
a race between instantiating the PMIC and finding the clock consumers.

Finally, if adding the consumers does not help, and if there will be no 
consensus regarding the generic property - then I think it's better to 
have a vendor specific property (as Peng suggested) than it is having 
the boards broken. Eg, if all else fails and if there is a buy-in for 
the vendor specific propety from Rob and Stephen - then I can also live 
with it (even if it sure significantly decreases my happiness level :p)

Yours,
	-- Matti

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

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

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Peng Fan]

On 9/13/2022 4:40 AM, Matti Vaittinen wrote:
> On 9/12/22 20:15, Tim Harvey wrote:
>> On Mon, Sep 12, 2022 at 12:40 AM Peng Fan <peng.fan@oss.nxp.com> wrote:
> 
> //snip
> 
>>>
>>> After a thought, maybe an easier way is to add a optional property
>>> xxx,32k-always-on to the pmic node/driver.
>>>
> 
> Yes, that would be easy. Yet, creating a driver specific DT-property 
> feels a tad wrong. I don't think the BD718xx is in any way special so it 
> should not need such a vendor specific property. It might be better to 
> find more generic solution.

I am not sure, even if saying always-on-clocks are accepted, the 
property still needs to wrote into the BD718xx node, because BD718xx
itself serves as a clock provider.

Regards,
Peng.

> 
>> Is there simply a way to add the clk to the snvs_rtc and the wdog dt
>> nodes so they have a use count and don't get disabled?
> 
> To me that does sound like the right thing to do. If we have a consumer 
> which requires the clock, then describing this dependency in DT sounds 
> like a correct approach - assuming this keeps the clock enabled without 
> a race between instantiating the PMIC and finding the clock consumers.
> 
> Finally, if adding the consumers does not help, and if there will be no 
> consensus regarding the generic property - then I think it's better to 
> have a vendor specific property (as Peng suggested) than it is having 
> the boards broken. Eg, if all else fails and if there is a buy-in for 
> the vendor specific propety from Rob and Stephen - then I can also live 
> with it (even if it sure significantly decreases my happiness level :p)



> 
> Yours,
>      -- Matti
> 

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

Hi,

I had the same trouble before for QMX6 system on module, which feeds
the i.MX6 32k clock via I2C RTC's 32k output. Here is how it has
been solved upstream:

https://lore.kernel.org/all/20210428222953.235280-1-sebastian.reichel@collabora.com/

Patch 1 and patch 5 (look for "rtc_sqw") are relevant for this
specific issue. Discussion history is linked from the cover letter.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Matti Vaittinen]

Thanks for the input Sebastian!

On 9/13/22 18:21, Sebastian Reichel wrote:
> Hi,
> 
> I had the same trouble before for QMX6 system on module, which feeds
> the i.MX6 32k clock via I2C RTC's 32k output. Here is how it has
> been solved upstream:
> 
> https://lore.kernel.org/all/20210428222953.235280-1-sebastian.reichel@collabora.com/
> 

So, if my poor brains (that have been conferencing the whole day) can 
still read this correctly - upstream solution is that drivers 
controllong clock gate need to have this "fixed-clock" propery check && 
not register the gate if fixed-clock is present, right?

I think the fixed clock is better than the vendor specific property as 
it still describes the real HW. I am not really thrilled by the fact 
that each clk (provider) driver may potentially need to implement this 
as no one knows when the clocks are used in such an environment. This is 
why I feel the support would better fit the core. (Yep - I didn't yet 
read the linked discussion - I know people who are smarter than me have 
probably thought this through already).

So, basically this would require adding fixed-clock node in PMIC node 
when the 32K clock must not be touched. I hope this suits the people 
looking after the board dts files. In the clk driver it requires the 
check for "fixed-clock" node + return w/o registering the clk if node is 
there.

I guess we could at least have a registration API (something like 
clk_register_if_not_fixed(), but "naming is hard" said Rob once to me) - 
it would not only slightly simplify the drivers but it would also help 
avoiding this same discussion with the next board where similar problem 
is surfacing. This of course needs buy-in from Stephen (as does any 
change to bd718x7-clk which goes through his tree).

Finally this probably requires the binding docs changes to all PMICs 
which use the bd718x7-clk driver - and I guess that is Rob's territory.

I am happy if someone patches the bd718x7-clk + all the binding docs. 
Especially the binding docs - I never get the right at first shot. I can 
also try giving a hand with the clk-bd718x7 if no one else will, but 
that will take some time (I'm currently travelling) :( Tim, others, 
please let me know if you wish me to try looking at this.

Yours
	-- Matti

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

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

Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Peng Fan]

On 9/13/2022 1:15 AM, Tim Harvey wrote:
> On Mon, Sep 12, 2022 at 12:40 AM Peng Fan <peng.fan@oss.nxp.com> wrote:
>>
>>
>>
>> On 9/9/2022 1:06 PM, Matti Vaittinen wrote:
>>> Hi dee Ho peeps,
>>>
>>> On 9/9/22 05:35, Marek Vasut wrote:
>>>> On 9/9/22 04:06, Peng Fan wrote:
>>>>>> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT
>>>>>> failure
>>>>>>
>>>>>> On 9/8/22 21:25, Tim Harvey wrote:
>>>>>>> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>>>>>> <mazziesaccount@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Tim,
>>>>>>>>>>
>>>>>>>>>> On 9/2/22 01:23, Tim Harvey wrote:
>>>>>>>>>>> Greetings,
>>>>>>>>>>>
>>>>>>>>>>> I've found that the bd71847 clk driver
>>>>>> (CONFIG_COMMON_CLK_BD718XX
>>>>>>>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
>>>>>>>>>>> C32K_OUT
>>>>>>>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up disabling
>>>>>>>>>>> the IMX RTC as well as the IMX WDOG functionality.
>>>>>>>>>>
>>>>>>>>>> //snip
>>>>>>>>>>
>>>>>>>>>>> This happens via clk_unprepare_unused() as nothing is flagging the
>>>>>>>>>>> clk-32k-out as being used. What should be added to the device-tree
>>>>>>>>>>> to signify that this clk is indeed necessary and should not be
>>>>>>>>>>> disabled?
>>>>>>>>>>
>>>>>>>>>> I have seen following proposal from Marek Vasut:
>>>>>>>>>>
>>>>>>>>>>
>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
>>>>>>>>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
>>>>>> marex%40denx.de%2FT%
>>>>>>>>>>
>>>>>> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
>>>>>> 1%7Cp
>>>>>>>>>>
>>>>>> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
>>>>>> 3bc2b
>>>>>>>>>>
>>>>>> 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
>>>>>> 7CTWFpb
>>>>>>>>>>
>>>>>> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
>>>>>> 6
>>>>>>>>>>
>>>>>> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
>>>>>> 2FLByaEhh5
>>>>>>>>>> Dtah9K8CcAOAM%3D&amp;reserved=0
>>>>>>>>>>
>>>>>>>>>> I am not sure if the discussion is completed though. I guess it was
>>>>>>>>>> agreed this was needed/usefull and maybe the remaining thing to
>>>>>>>>>> decide was just the property naming.
>>>>>>>>>>
>>>>>>>>>> Best Regards
>>>>>>>>>>             -- Matti
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks Matti,
>>>>>>>>>
>>>>>>>>> Marek - has there been any progress on determining how best to keep
>>>>>>>>> certain clocks from being disabled?
>>>>>>>>
>>>>>>>> No. You can read the discussion above.
>>>>>>>
>>>>>>> Marek,
>>>>>>>
>>>>>>> I wasn't on the linux-clk list at that time so can't respond to the
>>>>>>> thread but the discussion seems to have died out a couple of months
>>>>>>> ago with no agreement between you or Stephen on how to deal with it.
>>>>>>>
>>>>>>> So where do we take this from here? It looks like there are about 18
>>>>>>> boards with dt's using "rohm,bd718*" which would all have non working
>>>>>>> RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled (which it is in
>>>>>>> arch/arm64/configs/defconfig) right?
>>>
>>> Yeah. The ROHM BD71837 and BD71847 (and BD71850 - which is one of the
>>> variants) are used quite a lot. I am pretty sure not fixing this in
>>> upstream is increasing downstream solutions. I don't think that should
>>> be preferred.
>>>
>>>>>
>>>>> Is there any requirement that the bd718xx clk needs to be runtime
>>>>> on/off?
>>>>
>>>> Yes, the 32kHz clock on BD71xxx should behave like any other clock,
>>>> unless specified otherwise, see below.
>>>>
>>>>> I suppose the clk should always be never be off, if yes, why not have
>>>>> something:
>>>>
>>>> What is needed in this specific case of BD718xx is I think clock
>>>> consumer on the MX8M clock driver side which would claim the 32kHz
>>>> input from the PMIC and up the clock enable count to keep the 32 kHz
>>>> clock always on.
>>>
>>> This sounds like a solution that would describe the actual HW setup. I
>>> don't know the CCF of the i.MX8 well enough to tell whether this can
>>> ensure the clk is not disabled before the consumer is found or when the
>>> consumer is going down though. Simplest thing to me would really be to
>>> just mark the clk as "do-not-touch" one on the boards where it must not
>>> be touched.
>>>
>>>    The PMIC is most likely supplying 32 kHz clock to the MX8M,
>>>> which if the 32 kHz clock are turned off would hang (I observed that
>>>> before too).
>>>>
>>>> What I tried to address in this thread is a generic problem which
>>>> commonly appears on various embedded systems, except every time anyone
>>>> tried to solve it in a generic manner, it was rejected or they gave up.
>>>
>>> I agree with Marek - generic solution would be nice. I don't think this
>>> is something specific to this PMIC.
>>>
>>>> The problem is this -- you have an arbitrary clock, and you need to
>>>> keep it running always otherwise the system fails, and you do not have
>>>> a clock consumer in the DT for whatever reason e.g. because the SoC is
>>>> only used as a clock source for some unrelated clock net. There must
>>>> be a way to mark the clock as "never disable these", i.e.
>>>> critical-clock. (I feel like I keep repeating this over and over in
>>>> this thread, so please read the whole thread backlog)
>>>
>>> Thanks for the explanation and effor you did Marek.
>>>
>>> My take on this is that from a (generic) component vendor perspective it
>>> is a bad idea to hard-code the clock status (enable/disable) in the PMIC
>>> driver. A vendor wants to provide a driver which allows use of the
>>> component in wide variety of systems/boards. When the PMIC contains a
>>> clock gate, the PMIC driver should provide the means of controlling it.
>>> Some setups may want it enabled, other disabled and some want runtime
>>> control. This "use-policy" must not be hard coded in the driver - it
>>> needs to come from HW description which explains how the clk line is
>>> wired and potentially also from the consumer drivers. This enables the
>>> same PMIC driver to support all different setups with their own needs,
>>> right?
>>>
>>> I am not sure if some non email discussions have been ongoing around
>>> this topic but just by reading the emails it seemed to me that Marek's
>>> suggestion was acked by the DT folks - and I don't think that Stephen
>>> was (at the end of the day) against that either(?). Maybe I missed
>>> something.
>>
>> After a thought, maybe an easier way is to add a optional property
>> xxx,32k-always-on to the pmic node/driver.
>>
>> Regards,
>> Peng.
> 
> Is there simply a way to add the clk to the snvs_rtc and the wdog dt
> nodes so they have a use count and don't get disabled?

I just see snvs_rtc requires osc 32k, but I not see wdog requires 32KHz.

"
The 32KHz XTAL module uses a different IP and it is used as the clock 
source for the
RTC, located in the SNVS.
"

Regards,
Peng.

> 
> Best Regards,
> 
> Tim

RE: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Peng Fan]

> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure
> 
> On 9/9/22 04:06, Peng Fan wrote:
> >> Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT
> >> failure
> >>
> >> On 9/8/22 21:25, Tim Harvey wrote:
> >>> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 9/8/22 18:00, Tim Harvey wrote:
> >>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> >> <mazziesaccount@gmail.com> wrote:
> >>>>>>
> >>>>>> Hi Tim,
> >>>>>>
> >>>>>> On 9/2/22 01:23, Tim Harvey wrote:
> >>>>>>> Greetings,
> >>>>>>>
> >>>>>>> I've found that the bd71847 clk driver
> >> (CONFIG_COMMON_CLK_BD718XX
> >>>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
> >>>>>>> C32K_OUT
> >>>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up
> disabling
> >>>>>>> the IMX RTC as well as the IMX WDOG functionality.
> >>>>>>
> >>>>>> //snip
> >>>>>>
> >>>>>>> This happens via clk_unprepare_unused() as nothing is flagging
> >>>>>>> the clk-32k-out as being used. What should be added to the
> >>>>>>> device-tree to signify that this clk is indeed necessary and should
> not be disabled?
> >>>>>>
> >>>>>> I have seen following proposal from Marek Vasut:
> >>>>>>
> >>>>>>
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
> >>>>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
> >> marex%40denx.de%2FT%
> >>>>>>
> >>
> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
> >> 1%7Cp
> >>>>>>
> >>
> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
> >> 3bc2b
> >>>>>>
> >>
> 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
> >> 7CTWFpb
> >>>>>>
> >>
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> >> 6
> >>>>>>
> >>
> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
> >> 2FLByaEhh5
> >>>>>> Dtah9K8CcAOAM%3D&amp;reserved=0
> >>>>>>
> >>>>>> I am not sure if the discussion is completed though. I guess it
> >>>>>> was agreed this was needed/usefull and maybe the remaining thing
> >>>>>> to decide was just the property naming.
> >>>>>>
> >>>>>> Best Regards
> >>>>>>            -- Matti
> >>>>>>
> >>>>>
> >>>>> Thanks Matti,
> >>>>>
> >>>>> Marek - has there been any progress on determining how best to
> >>>>> keep certain clocks from being disabled?
> >>>>
> >>>> No. You can read the discussion above.
> >>>
> >>> Marek,
> >>>
> >>> I wasn't on the linux-clk list at that time so can't respond to the
> >>> thread but the discussion seems to have died out a couple of months
> >>> ago with no agreement between you or Stephen on how to deal with it.
> >>>
> >>> So where do we take this from here? It looks like there are about 18
> >>> boards with dt's using "rohm,bd718*" which would all have non
> >>> working RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled
> (which it is
> >>> in
> >>> arch/arm64/configs/defconfig) right?
> >
> > Is there any requirement that the bd718xx clk needs to be runtime on/off?
> 
> Yes, the 32kHz clock on BD71xxx should behave like any other clock, unless
> specified otherwise, see below.
> 
> > I suppose the clk should always be never be off, if yes, why not have
> something:
> 
> What is needed in this specific case of BD718xx is I think clock consumer on
> the MX8M clock driver side which would claim the 32kHz input from the
> PMIC and up the clock enable count to keep the 32 kHz clock always on. The
> PMIC is most likely supplying 32 kHz clock to the MX8M, which if the 32 kHz
> clock are turned off would hang (I observed that before too).

i.MX8M has internal 32 KHz XTAL module, why need external pmic 32KHz feed
in?

Thanks,
Peng.
> 
> What I tried to address in this thread is a generic problem which commonly
> appears on various embedded systems, except every time anyone tried to
> solve it in a generic manner, it was rejected or they gave up.
> 
> The problem is this -- you have an arbitrary clock, and you need to keep it
> running always otherwise the system fails, and you do not have a clock
> consumer in the DT for whatever reason e.g. because the SoC is only used
> as a clock source for some unrelated clock net. There must be a way to mark
> the clock as "never disable these", i.e. critical-clock.
> (I feel like I keep repeating this over and over in this thread, so please read
> the whole thread backlog)

RE: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure

[Peng Fan]

> -----Original Message-----
> From: Peng Fan <peng.fan@nxp.com>
> Sent: 2022年9月9日 14:57
> To: Marek Vasut <marex@denx.de>; tharvey@gateworks.com; Stephen
> Boyd <sboyd@kernel.org>
> Cc: Matti Vaittinen <mazziesaccount@gmail.com>; linux-clk <linux-
> clk@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; Fabio
> Estevam <festevam@gmail.com>; Shawn Guo <shawnguo@kernel.org>; dl-
> linux-imx <linux-imx@nxp.com>; Michael Turquette
> <mturquette@baylibre.com>
> Subject: RE: BD71847 clk driver disables clk-32k-out causing RTC/WDT failure
> 
> > Subject: Re: BD71847 clk driver disables clk-32k-out causing RTC/WDT
> > failure
> >
> > On 9/9/22 04:06, Peng Fan wrote:
> > >> Subject: Re: BD71847 clk driver disables clk-32k-out causing
> > >> RTC/WDT failure
> > >>
> > >> On 9/8/22 21:25, Tim Harvey wrote:
> > >>> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <marex@denx.de>
> wrote:
> > >>>>
> > >>>> On 9/8/22 18:00, Tim Harvey wrote:
> > >>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> > >> <mazziesaccount@gmail.com> wrote:
> > >>>>>>
> > >>>>>> Hi Tim,
> > >>>>>>
> > >>>>>> On 9/2/22 01:23, Tim Harvey wrote:
> > >>>>>>> Greetings,
> > >>>>>>>
> > >>>>>>> I've found that the bd71847 clk driver
> > >> (CONFIG_COMMON_CLK_BD718XX
> > >>>>>>> drivers/clk/clk-bd718x7.c) disables clk-32k-out (the BD71847
> > >>>>>>> C32K_OUT
> > >>>>>>> pin) which is connected IMX8MM RTC_XTALI which ends up
> > disabling
> > >>>>>>> the IMX RTC as well as the IMX WDOG functionality.
> > >>>>>>
> > >>>>>> //snip
> > >>>>>>
> > >>>>>>> This happens via clk_unprepare_unused() as nothing is flagging
> > >>>>>>> the clk-32k-out as being used. What should be added to the
> > >>>>>>> device-tree to signify that this clk is indeed necessary and
> > >>>>>>> should
> > not be disabled?
> > >>>>>>
> > >>>>>> I have seen following proposal from Marek Vasut:
> > >>>>>>
> > >>>>>>
> > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fl
> > >>>>>> ore.kernel.org%2Fall%2F20220517235919.200375-1-
> > >> marex%40denx.de%2FT%
> > >>>>>>
> > >>
> >
> 2F%23m52d6d0831bf43d5f293e35cb27f3021f278d0564&amp;data=05%7C0
> > >> 1%7Cp
> > >>>>>>
> > >>
> >
> eng.fan%40nxp.com%7C07d48edcc47c4694e08208da91da2bf4%7C686ea1d
> > >> 3bc2b
> > >>>>>>
> > >>
> > 4c6fa92cd99c5c301635%7C0%7C0%7C637982664162868785%7CUnknown%
> > >> 7CTWFpb
> > >>>>>>
> > >>
> >
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> > >> 6
> > >>>>>>
> > >>
> > Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uF26u9g4onuqCWzPRAvD%2F%
> > >> 2FLByaEhh5
> > >>>>>> Dtah9K8CcAOAM%3D&amp;reserved=0
> > >>>>>>
> > >>>>>> I am not sure if the discussion is completed though. I guess it
> > >>>>>> was agreed this was needed/usefull and maybe the remaining
> > >>>>>> thing to decide was just the property naming.
> > >>>>>>
> > >>>>>> Best Regards
> > >>>>>>            -- Matti
> > >>>>>>
> > >>>>>
> > >>>>> Thanks Matti,
> > >>>>>
> > >>>>> Marek - has there been any progress on determining how best to
> > >>>>> keep certain clocks from being disabled?
> > >>>>
> > >>>> No. You can read the discussion above.
> > >>>
> > >>> Marek,
> > >>>
> > >>> I wasn't on the linux-clk list at that time so can't respond to
> > >>> the thread but the discussion seems to have died out a couple of
> > >>> months ago with no agreement between you or Stephen on how to
> deal with it.
> > >>>
> > >>> So where do we take this from here? It looks like there are about
> > >>> 18 boards with dt's using "rohm,bd718*" which would all have non
> > >>> working RTC/WDOG with CONFIG_COMMON_CLK_BD718XX enabled
> > (which it is
> > >>> in
> > >>> arch/arm64/configs/defconfig) right?
> > >
> > > Is there any requirement that the bd718xx clk needs to be runtime
> on/off?
> >
> > Yes, the 32kHz clock on BD71xxx should behave like any other clock,
> > unless specified otherwise, see below.
> >
> > > I suppose the clk should always be never be off, if yes, why not
> > > have
> > something:
> >
> > What is needed in this specific case of BD718xx is I think clock
> > consumer on the MX8M clock driver side which would claim the 32kHz
> > input from the PMIC and up the clock enable count to keep the 32 kHz
> > clock always on. The PMIC is most likely supplying 32 kHz clock to the
> > MX8M, which if the 32 kHz clock are turned off would hang (I observed
> that before too).
> 
> i.MX8M has internal 32 KHz XTAL module, why need external pmic 32KHz
> feed in?

Ignore this comment, it still need external osc.

Regards,
Peng.

> 
> Thanks,
> Peng.
> >
> > What I tried to address in this thread is a generic problem which
> > commonly appears on various embedded systems, except every time
> anyone
> > tried to solve it in a generic manner, it was rejected or they gave up.
> >
> > The problem is this -- you have an arbitrary clock, and you need to
> > keep it running always otherwise the system fails, and you do not have
> > a clock consumer in the DT for whatever reason e.g. because the SoC is
> > only used as a clock source for some unrelated clock net. There must
> > be a way to mark the clock as "never disable these", i.e. critical-clock.
> > (I feel like I keep repeating this over and over in this thread, so
> > please read the whole thread backlog)