Re: [PATCH] ARM: dts: qcom: ipq4019: fix sleep clock

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Sat 20 Nov 14:11 PST 2021, Christian Lamparter wrote:

> Hi,
> 
> On 18/11/2021 02:27, Bjorn Andersson wrote:
> > On Sun 31 Oct 10:56 CDT 2021, Christian Lamparter wrote:
> > 
> > > From: Pavel Kubelun <be.dissent@gmail.com>
> > > 
> > > It seems like sleep_clk was copied from ipq806x.
> > > Fix ipq40xx sleep_clk to the value QSDK defines.
> > > 
> > > Also rename the sleep clock node like the GCC driver names it.
> > > 
> > > Link: https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/commit/?id=d92ec59973484acc86dd24b67f10f8911b4b4b7d
> > > Link: https://patchwork.kernel.org/comment/22721613/
> > > Signed-off-by: Pavel Kubelun <be.dissent@gmail.com>
> > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> (resend)
> > > ---
> > >   arch/arm/boot/dts/qcom-ipq4019.dtsi | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/qcom-ipq4019.dtsi b/arch/arm/boot/dts/qcom-ipq4019.dtsi
> > > index 7a181352b306..ed40bef91e45 100644
> > > --- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
> > > +++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
> > > @@ -144,9 +144,9 @@ pmu {
> > >   	};
> > >   	clocks {
> > > -		sleep_clk: sleep_clk {
> > > +		sleep_clk: gcc_sleep_clk_src {
> > 
> > Underscore is not a valid character in node names, so it should named
> > something like sleep-clk.
> 
> 
> Ok, this is a bit awkward. The current DeviceTree Specification 0.3v list
> the "underscore" in "Table 2.1: Valid characters for node names". (Page 7)
> <https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf>
> 
> But...
> > It would be nice if the clock driver was converted to using parent_data
> > and fw_name and a clocks reference in the gcc node to &sleep_clk, but if
> > you need something that works with the current implementation then
> > 
> >    clock-output-names = "gcc_sleep_clk_src"; >
> > is your friend.
> > 
> > 
> > But please do consider converting the gcc-ipq4019.c to acquire "xo" and
> > "sleep_clk" using parent_data instead.
> > 
> 
> 
> Yes, I think this could be done. Next month around the seasonal holidays,
> I do think there will be some free time to run this on the IPQ40xx devices
> that would otherwise be blinking away lonely at the office. (Pavel's patch
> has been here so long that I knew it is "working".)
> 
> So the next series will be:
> - change this patch to just fix the clock-frequency value.

But if gcc-ipq4019 is looking for gcc_sleep_clk_src, what changes based
on this change alone?

> - another new patch that updates gcc-ipq4019.c code
>   replace all the parent_name with "xo" and "gcc_sleep_clk_src"
>   with parent_data + fw_name (in the same way in which
>   gcc-ipq6018.c seems to be doing it).
> 

Given that you're planning to move away from using a significant name I
don't think it's worth fixing up the driver to find "sleep_clk" and then
drop that again.

So if you just want to give the sleep clock its correct value, let's
just fix the rate. If you need that clock, with the new rate, to be
present among the parents of the gcc clocks then add the
clock-output-names = "gcc_sleep_clk_src" (and skip changing the driver
at this point).

If you do the latter, and then move to parent_data that new kernel will
fail to find the clock in a dtb generated between now and that point in
time, so I suggest that you anticipate that said input clock will be
given the identifier "sleep_clk".
So in the gcc node you add clocks = <&sleep_clk>; clock-names =
"sleep_clk".

That way, it will find the right clock using the global name from now
on and when you move the driver to parent_data it will find the new
reference.

Regards,
Bjorn

> Is there anything I missed?
> 
> (I could throw in the sleep_clk -> sleep(-clk) change as well.
> But I'll need find a justification for replacing "_" vs "-" there,
> since I can't really lean on the Device-Tree spec since it
> proclamates that the underscore is a "valid character"?!)
> 
> Thanks,
> Christian