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
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/[email protected]/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! ~~
On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <[email protected]> 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/[email protected]/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
On 9/8/22 18:00, Tim Harvey wrote:
> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <[email protected]> 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/[email protected]/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.
On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <[email protected]> wrote:
>
> On 9/8/22 18:00, Tim Harvey wrote:
> > On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <[email protected]> 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/[email protected]/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
On 9/8/22 21:25, Tim Harvey wrote:
> On Thu, Sep 8, 2022 at 9:55 AM Marek Vasut <[email protected]> wrote:
>>
>> On 9/8/22 18:00, Tim Harvey wrote:
>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen <[email protected]> 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/[email protected]/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.
> 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 <[email protected]> wrote:
> >>
> >> On 9/8/22 18:00, Tim Harvey wrote:
> >>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
> 2FLByaEhh5
> >>>> Dtah9K8CcAOAM%3D&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.
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 <[email protected]> wrote:
>>>>
>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
>> 2FLByaEhh5
>>>>>> Dtah9K8CcAOAM%3D&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)
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 <[email protected]> wrote:
>>>>>
>>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>>> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
>>> 2FLByaEhh5
>>>>>>> Dtah9K8CcAOAM%3D&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! ~~
> 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 <[email protected]> wrote:
> >>>>
> >>>> On 9/8/22 18:00, Tim Harvey wrote:
> >>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> >> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
> >> 2FLByaEhh5
> >>>>>> Dtah9K8CcAOAM%3D&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)
> -----Original Message-----
> From: Peng Fan <[email protected]>
> Sent: 2022??9??9?? 14:57
> To: Marek Vasut <[email protected]>; [email protected]; Stephen
> Boyd <[email protected]>
> Cc: Matti Vaittinen <[email protected]>; linux-clk <linux-
> [email protected]>; open list <[email protected]>; Fabio
> Estevam <[email protected]>; Shawn Guo <[email protected]>; dl-
> linux-imx <[email protected]>; Michael Turquette
> <[email protected]>
> 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 <[email protected]>
> wrote:
> > >>>>
> > >>>> On 9/8/22 18:00, Tim Harvey wrote:
> > >>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> > >> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
> > >> 2FLByaEhh5
> > >>>>>> Dtah9K8CcAOAM%3D&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)
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 <[email protected]> wrote:
>>>>>>
>>>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>>>> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
>>>> 2FLByaEhh5
>>>>>>>> Dtah9K8CcAOAM%3D&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
>
On Mon, Sep 12, 2022 at 12:40 AM Peng Fan <[email protected]> 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 <[email protected]> wrote:
> >>>>>>
> >>>>>> On 9/8/22 18:00, Tim Harvey wrote:
> >>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> >>>> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
> >>>> 2FLByaEhh5
> >>>>>>>> Dtah9K8CcAOAM%3D&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
On 9/12/22 20:15, Tim Harvey wrote:
> On Mon, Sep 12, 2022 at 12:40 AM Peng Fan <[email protected]> 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! ~~
On 9/13/2022 1:15 AM, Tim Harvey wrote:
> On Mon, Sep 12, 2022 at 12:40 AM Peng Fan <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On 9/8/22 18:00, Tim Harvey wrote:
>>>>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
>>>>>> <[email protected]> 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&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&sdata=uF26u9g4onuqCWzPRAvD%2F%
>>>>>> 2FLByaEhh5
>>>>>>>>>> Dtah9K8CcAOAM%3D&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
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 <[email protected]> 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
>
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/[email protected]/
Patch 1 and patch 5 (look for "rtc_sqw") are relevant for this
specific issue. Discussion history is linked from the cover letter.
-- Sebastian
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/[email protected]/
>
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! ~~