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@xxxxxxxxxxx> 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@xxxxxxx> wrote:
> >>>>>>
> >>>>>> On 9/8/22 18:00, Tim Harvey wrote:
> >>>>>>> On Thu, Sep 1, 2022 at 9:14 PM Matti Vaittinen
> >>>> <mazziesaccount@xxxxxxxxx> 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