| Message ID | 20221211205124.23823-2-marex@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3,1/2] dt-bindings: rtc: m41t80: Convert text schema to YAML one | expand |
Hi, On Sun, Dec 11, 2022 at 09:51:24PM +0100, Marek Vasut wrote: > The clock {} subnode seems like it is describing an always-on clock > generated by the PMIC. This should rather be modeled by consumer of > the clock taking phandle to the RTC node itself, since it already > does have clock-cells and all. Since there are no users of the clock > subnode in tree anyway, mark it as deprecated to avoid proliferation > of this approach. > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alessandro Zummo <a.zummo@towertech.it> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > Cc: linux-rtc@vger.kernel.org > To: devicetree@vger.kernel.org > --- > V2: - Add AB from Krzysztof > V3: - No change > --- I just noticed this by accident. Basically everything in the patch description is wrong: 1. There is a in-tree user: arch/arm/boot/dts/imx6dl-qmx6.dtsi 2. The PMIC has nothing to do with this 3. Directly referencing the RTC does not work, since that introduces an unsolvable dependency loop on QMX6. This was the solution accepted by Rob and Saravana: [v1] https://lore.kernel.org/lkml/20210222171247.97609-1-sebastian.reichel@collabora.com/ [v2] https://lore.kernel.org/all/20210428222953.235280-1-sebastian.reichel@collabora.com/ -- Sebastian > Documentation/devicetree/bindings/rtc/st,m41t80.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml > index fc9c6da6483f5..03ff833f5fe9d 100644 > --- a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml > +++ b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml > @@ -40,6 +40,7 @@ properties: > clock: > type: object > $ref: /schemas/clock/fixed-clock.yaml# > + deprecated: true > properties: > clock-frequency: > const: 32768 > -- > 2.35.1 >
On 12/15/22 19:06, Sebastian Reichel wrote: > Hi, Hi, > On Sun, Dec 11, 2022 at 09:51:24PM +0100, Marek Vasut wrote: >> The clock {} subnode seems like it is describing an always-on clock >> generated by the PMIC. This should rather be modeled by consumer of >> the clock taking phandle to the RTC node itself, since it already >> does have clock-cells and all. Since there are no users of the clock >> subnode in tree anyway, mark it as deprecated to avoid proliferation >> of this approach. >> >> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Alessandro Zummo <a.zummo@towertech.it> >> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> >> Cc: linux-rtc@vger.kernel.org >> To: devicetree@vger.kernel.org >> --- >> V2: - Add AB from Krzysztof >> V3: - No change >> --- > > I just noticed this by accident. Basically everything in the patch > description is wrong: > > 1. There is a in-tree user: arch/arm/boot/dts/imx6dl-qmx6.dtsi Sorry, I missed this one. > 2. The PMIC has nothing to do with this In [3] the commit message claims the PMIC supplies 32kHz clock to i.MX6 CKIL , which per IMX6DQRM rev.6 Table 18-3 row SNVS indirectly supplies SNVS RTC . This reminded me of commit: 9509593f327ac ("arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on Data Modul i.MX8M Mini eDM SBC") which solves exactly the same problem, system hangs when 32 kHz clock are stopped, except this time on i.MX8MM, clock are generated by PMIC on I2C (notice how the PMIC is referenced directly) and the clock are supplied to the SVNS RTC XTal terminals. I wonder if this could be reused on the QMX6 board too ? > 3. Directly referencing the RTC does not work, since that introduces > an unsolvable dependency loop on QMX6. This was the solution accepted > by Rob and Saravana: > > [v1] https://lore.kernel.org/lkml/20210222171247.97609-1-sebastian.reichel@collabora.com/ > [v2] https://lore.kernel.org/all/20210428222953.235280-1-sebastian.reichel@collabora.com/ [3] https://lore.kernel.org/linux-clk/20191108170135.9053-1-sebastian.reichel@collabora.com/
Hi, On Thu, Dec 15, 2022 at 08:39:47PM +0100, Marek Vasut wrote: > On 12/15/22 19:06, Sebastian Reichel wrote: > > On Sun, Dec 11, 2022 at 09:51:24PM +0100, Marek Vasut wrote: > > > The clock {} subnode seems like it is describing an always-on clock > > > generated by the PMIC. This should rather be modeled by consumer of > > > the clock taking phandle to the RTC node itself, since it already > > > does have clock-cells and all. Since there are no users of the clock > > > subnode in tree anyway, mark it as deprecated to avoid proliferation > > > of this approach. > > > > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > --- > > > Cc: Alessandro Zummo <a.zummo@towertech.it> > > > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > > > Cc: linux-rtc@vger.kernel.org > > > To: devicetree@vger.kernel.org > > > --- > > > V2: - Add AB from Krzysztof > > > V3: - No change > > > --- > > > > I just noticed this by accident. Basically everything in the patch > > description is wrong: > > > > 1. There is a in-tree user: arch/arm/boot/dts/imx6dl-qmx6.dtsi > > Sorry, I missed this one. > > > 2. The PMIC has nothing to do with this > > In [3] the commit message claims the PMIC supplies 32kHz clock to i.MX6 CKIL, > which per IMX6DQRM rev.6 Table 18-3 row SNVS indirectly supplies SNVS RTC. > This reminded me of commit: The word PMIC is not mentioned once in [3]. PMIC is not involved. The QMX6 32khz chain is like this: 32kHz crystal -> m41t62 crystal input m41t62 clock output -> i.MX6 CKIL > 9509593f327ac ("arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on > Data Modul i.MX8M Mini eDM SBC") > > which solves exactly the same problem, system hangs when 32 kHz clock are > stopped, except this time on i.MX8MM, clock are generated by PMIC on I2C > (notice how the PMIC is referenced directly) and the clock are supplied to > the SVNS RTC XTal terminals. > > I wonder if this could be reused on the QMX6 board too? IIRC On i.MX6 referencing the I2C connected RTC results in boot hanging forever when trying to get the ckil clock in imx6q_clocks_init. At least it used to be the case when I was working on this - I no longer have access to the boards. Of course properly referencing the RTC clock was the first route I tried. -- Sebastian > > 3. Directly referencing the RTC does not work, since that introduces > > an unsolvable dependency loop on QMX6. This was the solution accepted > > by Rob and Saravana: > > > > [v1] https://lore.kernel.org/lkml/20210222171247.97609-1-sebastian.reichel@collabora.com/ > > [v2] https://lore.kernel.org/all/20210428222953.235280-1-sebastian.reichel@collabora.com/ > > [3] https://lore.kernel.org/linux-clk/20191108170135.9053-1-sebastian.reichel@collabora.com/
On 12/16/22 15:24, Sebastian Reichel wrote: > Hi, Hi, > On Thu, Dec 15, 2022 at 08:39:47PM +0100, Marek Vasut wrote: >> On 12/15/22 19:06, Sebastian Reichel wrote: >>> On Sun, Dec 11, 2022 at 09:51:24PM +0100, Marek Vasut wrote: >>>> The clock {} subnode seems like it is describing an always-on clock >>>> generated by the PMIC. This should rather be modeled by consumer of >>>> the clock taking phandle to the RTC node itself, since it already >>>> does have clock-cells and all. Since there are no users of the clock >>>> subnode in tree anyway, mark it as deprecated to avoid proliferation >>>> of this approach. >>>> >>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>> --- >>>> Cc: Alessandro Zummo <a.zummo@towertech.it> >>>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> >>>> Cc: Rob Herring <robh+dt@kernel.org> >>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> >>>> Cc: linux-rtc@vger.kernel.org >>>> To: devicetree@vger.kernel.org >>>> --- >>>> V2: - Add AB from Krzysztof >>>> V3: - No change >>>> --- >>> >>> I just noticed this by accident. Basically everything in the patch >>> description is wrong: >>> >>> 1. There is a in-tree user: arch/arm/boot/dts/imx6dl-qmx6.dtsi >> >> Sorry, I missed this one. >> >>> 2. The PMIC has nothing to do with this >> >> In [3] the commit message claims the PMIC supplies 32kHz clock to i.MX6 CKIL, >> which per IMX6DQRM rev.6 Table 18-3 row SNVS indirectly supplies SNVS RTC. >> This reminded me of commit: > > The word PMIC is not mentioned once in [3]. s@PMIC@m41t62 RTC@, sorry. > PMIC is not involved. > The QMX6 32khz chain is like this: > > 32kHz crystal -> m41t62 crystal input > m41t62 clock output -> i.MX6 CKIL > >> 9509593f327ac ("arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on >> Data Modul i.MX8M Mini eDM SBC") >> >> which solves exactly the same problem, system hangs when 32 kHz clock are >> stopped, except this time on i.MX8MM, clock are generated by PMIC on I2C >> (notice how the PMIC is referenced directly) and the clock are supplied to >> the SVNS RTC XTal terminals. >> >> I wonder if this could be reused on the QMX6 board too? > > IIRC On i.MX6 referencing the I2C connected RTC results in boot > hanging forever when trying to get the ckil clock in > imx6q_clocks_init. At least it used to be the case when I was > working on this - I no longer have access to the boards. Of course > properly referencing the RTC clock was the first route I tried. Hmmmmm, what shall we do, un-deprecate the clock sub-node ?
Hi, On Fri, Dec 16, 2022 at 03:36:17PM +0100, Marek Vasut wrote: > On 12/16/22 15:24, Sebastian Reichel wrote: > > On Thu, Dec 15, 2022 at 08:39:47PM +0100, Marek Vasut wrote: > > > On 12/15/22 19:06, Sebastian Reichel wrote: > > > > On Sun, Dec 11, 2022 at 09:51:24PM +0100, Marek Vasut wrote: > > > > > The clock {} subnode seems like it is describing an always-on clock > > > > > generated by the PMIC. This should rather be modeled by consumer of > > > > > the clock taking phandle to the RTC node itself, since it already > > > > > does have clock-cells and all. Since there are no users of the clock > > > > > subnode in tree anyway, mark it as deprecated to avoid proliferation > > > > > of this approach. > > > > > > > > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > > > --- > > > > > Cc: Alessandro Zummo <a.zummo@towertech.it> > > > > > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > > > > > Cc: linux-rtc@vger.kernel.org > > > > > To: devicetree@vger.kernel.org > > > > > --- > > > > > V2: - Add AB from Krzysztof > > > > > V3: - No change > > > > > --- > > > > > > > > I just noticed this by accident. Basically everything in the patch > > > > description is wrong: > > > > > > > > 1. There is a in-tree user: arch/arm/boot/dts/imx6dl-qmx6.dtsi > > > > > > Sorry, I missed this one. > > > > > > > 2. The PMIC has nothing to do with this > > > > > > In [3] the commit message claims the PMIC supplies 32kHz clock to i.MX6 CKIL, > > > which per IMX6DQRM rev.6 Table 18-3 row SNVS indirectly supplies SNVS RTC. > > > This reminded me of commit: > > > > The word PMIC is not mentioned once in [3]. > > s@PMIC@m41t62 RTC@, sorry. > > > PMIC is not involved. > > The QMX6 32khz chain is like this: > > > > 32kHz crystal -> m41t62 crystal input > > m41t62 clock output -> i.MX6 CKIL > > > > > 9509593f327ac ("arm64: dts: imx8mm: Model PMIC to SNVS RTC clock path on > > > Data Modul i.MX8M Mini eDM SBC") > > > > > > which solves exactly the same problem, system hangs when 32 kHz clock are > > > stopped, except this time on i.MX8MM, clock are generated by PMIC on I2C > > > (notice how the PMIC is referenced directly) and the clock are supplied to > > > the SVNS RTC XTal terminals. > > > > > > I wonder if this could be reused on the QMX6 board too? > > > > IIRC On i.MX6 referencing the I2C connected RTC results in boot > > hanging forever when trying to get the ckil clock in > > imx6q_clocks_init. At least it used to be the case when I was > > working on this - I no longer have access to the boards. Of course > > properly referencing the RTC clock was the first route I tried. > > Hmmmmm, what shall we do, un-deprecate the clock sub-node? Depends on the exact meaning of "deprecate: true". I think we all agree, that it's better to avoid the sub-node and only use it when it's really required. But having a deprecation warning for an in-tree user without a clear path forward also seems to be annoying. I think it makes sense for the DT binding maintainers (Rob/Krzysztof) to comment on this. -- Sebastian
On 16/12/2022 16:05:48+0100, Sebastian Reichel wrote: > > > IIRC On i.MX6 referencing the I2C connected RTC results in boot > > > hanging forever when trying to get the ckil clock in > > > imx6q_clocks_init. At least it used to be the case when I was > > > working on this - I no longer have access to the boards. Of course > > > properly referencing the RTC clock was the first route I tried. > > > > Hmmmmm, what shall we do, un-deprecate the clock sub-node? > > Depends on the exact meaning of "deprecate: true". I think we all > agree, that it's better to avoid the sub-node and only use it when > it's really required. But having a deprecation warning for an > in-tree user without a clear path forward also seems to be annoying. > I think it makes sense for the DT binding maintainers (Rob/Krzysztof) > to comment on this. > I dropped the commit from the PR I'm going to send Linus later today
On 12/16/22 17:24, Alexandre Belloni wrote: > On 16/12/2022 16:05:48+0100, Sebastian Reichel wrote: >>>> IIRC On i.MX6 referencing the I2C connected RTC results in boot >>>> hanging forever when trying to get the ckil clock in >>>> imx6q_clocks_init. At least it used to be the case when I was >>>> working on this - I no longer have access to the boards. Of course >>>> properly referencing the RTC clock was the first route I tried. >>> >>> Hmmmmm, what shall we do, un-deprecate the clock sub-node? >> >> Depends on the exact meaning of "deprecate: true". I think we all >> agree, that it's better to avoid the sub-node and only use it when >> it's really required. But having a deprecation warning for an >> in-tree user without a clear path forward also seems to be annoying. >> I think it makes sense for the DT binding maintainers (Rob/Krzysztof) >> to comment on this. >> > > I dropped the commit from the PR I'm going to send Linus later today That works, thank you !
diff --git a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml index fc9c6da6483f5..03ff833f5fe9d 100644 --- a/Documentation/devicetree/bindings/rtc/st,m41t80.yaml +++ b/Documentation/devicetree/bindings/rtc/st,m41t80.yaml @@ -40,6 +40,7 @@ properties: clock: type: object $ref: /schemas/clock/fixed-clock.yaml# + deprecated: true properties: clock-frequency: const: 32768
The clock {} subnode seems like it is describing an always-on clock generated by the PMIC. This should rather be modeled by consumer of the clock taking phandle to the RTC node itself, since it already does have clock-cells and all. Since there are no users of the clock subnode in tree anyway, mark it as deprecated to avoid proliferation of this approach. Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Alessandro Zummo <a.zummo@towertech.it> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> Cc: linux-rtc@vger.kernel.org To: devicetree@vger.kernel.org --- V2: - Add AB from Krzysztof V3: - No change --- Documentation/devicetree/bindings/rtc/st,m41t80.yaml | 1 + 1 file changed, 1 insertion(+)