[PATCH] ARM: dts: ci4x10: Reflect upstream change to fec clocks

[PATCH] ARM: dts: ci4x10: Reflect upstream change to fec clocks

[Thorsten Scherer]

Reflect changes of

    commit f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk support")

and overwrite clocks accordingly.

Signed-off-by: Thorsten Scherer <t.scherer@eckelmann.de>
---
 arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
index b4a9523e325b..98a503d9be21 100644
--- a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
+++ b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
@@ -297,7 +297,10 @@ &fec {
 	phy-mode = "rmii";
 	phy-reset-gpios = <&gpio1 18 GPIO_ACTIVE_LOW>;
 	phy-handle = <&phy>;
-	clocks = <&clks IMX6QDL_CLK_ENET>, <&clks IMX6QDL_CLK_ENET>, <&rmii_clk>;
+	clocks = <&clks IMX6QDL_CLK_ENET>,
+		 <&clks IMX6QDL_CLK_ENET>,
+		 <&rmii_clk>,
+		 <&clks IMX6QDL_CLK_ENET_REF>;
 	status = "okay";
 
 	mdio {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: ci4x10: Reflect upstream change to fec clocks

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1812 bytes --]

Hello Thorsten,

from your POV f3e7dae323ab is an upstream change, but as your patch
targets upstream, too, it doesn't really make sense to write "Reflect
upstream change" in the Subject.

I would have written:

	ARM: dts: ci4x10: Adapt to changes in imx6qdl.dtsi regarding fec clocks

	Commit f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk
	support") added another item to the list of clocks for the fec
	device. As imx6dl-eckelmann-ci4x10.dts only overwrites clocks,
	but not clock-names this resulted in an inconsistency with
	clocks having one item more than clock-names.

Maybe it would make sense to overwrite clock-names, too, to prevent a
similar inconsistency in the future?! If yes:

	Also overwrite clock-names with the same value as in
	imx6qdl.dtsi. This is a no-op today, but prevents similar
	inconsistencies if the soc file will be changed in a similar way
	in the future.

The breakage isn't bad I think (is it?), so I don't think a Fixes: line
is necessary. @Shawn: Do you agree?

On Tue, Mar 29, 2022 at 08:12:54AM +0200, Thorsten Scherer wrote:
> diff --git a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> index b4a9523e325b..98a503d9be21 100644
> --- a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> +++ b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> @@ -297,7 +297,10 @@ &fec {
>  	phy-mode = "rmii";
>  	phy-reset-gpios = <&gpio1 18 GPIO_ACTIVE_LOW>;
>  	phy-handle = <&phy>;
> -	clocks = <&clks IMX6QDL_CLK_ENET>, <&clks IMX6QDL_CLK_ENET>, <&rmii_clk>;
> +	clocks = <&clks IMX6QDL_CLK_ENET>,
> +		 <&clks IMX6QDL_CLK_ENET>,
> +		 <&rmii_clk>,
> +		 <&clks IMX6QDL_CLK_ENET_REF>;

The change is fine, but if you agree to adding clock-names needs
adaption of course.

Best regards
Uwe

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: ci4x10: Reflect upstream change to fec clocks

[Thorsten Scherer]

Hello Uwe,

> Hello Thorsten,
> 
> from your POV f3e7dae323ab is an upstream change, but as your patch
> targets upstream, too, it doesn't really make sense to write "Reflect
> upstream change" in the Subject.
> 

you are right.

> I would have written:
> 
> 	ARM: dts: ci4x10: Adapt to changes in imx6qdl.dtsi regarding fec clocks
> 
> 	Commit f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk
> 	support") added another item to the list of clocks for the fec
> 	device. As imx6dl-eckelmann-ci4x10.dts only overwrites clocks,
> 	but not clock-names this resulted in an inconsistency with
> 	clocks having one item more than clock-names.

Thank you for suggesting a message.  Would you mind, if I'd pick it up
verbatim?

> Maybe it would make sense to overwrite clock-names, too, to prevent a
> similar inconsistency in the future?! If yes:
> 
> 	Also overwrite clock-names with the same value as in
> 	imx6qdl.dtsi. This is a no-op today, but prevents similar
> 	inconsistencies if the soc file will be changed in a similar way
> 	in the future.
> 

Overwriting clock-names sounds sensible.

> The breakage isn't bad I think (is it?), so I don't think a Fixes: line
> is necessary. @Shawn: Do you agree?
> 
> On Tue, Mar 29, 2022 at 08:12:54AM +0200, Thorsten Scherer wrote:
> > diff --git a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> > index b4a9523e325b..98a503d9be21 100644
> > --- a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> > +++ b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> > @@ -297,7 +297,10 @@ &fec {
> >  	phy-mode = "rmii";
> >  	phy-reset-gpios = <&gpio1 18 GPIO_ACTIVE_LOW>;
> >  	phy-handle = <&phy>;
> > -	clocks = <&clks IMX6QDL_CLK_ENET>, <&clks IMX6QDL_CLK_ENET>, <&rmii_clk>;
> > +	clocks = <&clks IMX6QDL_CLK_ENET>,
> > +		 <&clks IMX6QDL_CLK_ENET>,
> > +		 <&rmii_clk>,
> > +		 <&clks IMX6QDL_CLK_ENET_REF>;
> 
> The change is fine, but if you agree to adding clock-names needs
> adaption of course.
> 

After Shawn's feedback concerning the Fixes line I will send a v2.

> Best regards
> Uwe

Kind regards
Thorsten

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: ci4x10: Reflect upstream change to fec clocks

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2508 bytes --]

Hey Thorsten,

On Thu, Mar 31, 2022 at 03:25:34PM +0200, Thorsten Scherer wrote:
> > I would have written:
> > 
> > 	ARM: dts: ci4x10: Adapt to changes in imx6qdl.dtsi regarding fec clocks
> > 
> > 	Commit f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk
> > 	support") added another item to the list of clocks for the fec
> > 	device. As imx6dl-eckelmann-ci4x10.dts only overwrites clocks,
> > 	but not clock-names this resulted in an inconsistency with
> > 	clocks having one item more than clock-names.
> 
> Thank you for suggesting a message.  Would you mind, if I'd pick it up
> verbatim?

I don't mind. If you take it verbatim you can be sure to get a
compliment from me for the elaborate wording :-)

> > Maybe it would make sense to overwrite clock-names, too, to prevent a
> > similar inconsistency in the future?! If yes:
> > 
> > 	Also overwrite clock-names with the same value as in
> > 	imx6qdl.dtsi. This is a no-op today, but prevents similar
> > 	inconsistencies if the soc file will be changed in a similar way
> > 	in the future.
> > 
> 
> Overwriting clock-names sounds sensible.
> 
> > The breakage isn't bad I think (is it?), so I don't think a Fixes: line
> > is necessary. @Shawn: Do you agree?
> > 
> > On Tue, Mar 29, 2022 at 08:12:54AM +0200, Thorsten Scherer wrote:
> > > diff --git a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> > > index b4a9523e325b..98a503d9be21 100644
> > > --- a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> > > +++ b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
> > > @@ -297,7 +297,10 @@ &fec {
> > >  	phy-mode = "rmii";
> > >  	phy-reset-gpios = <&gpio1 18 GPIO_ACTIVE_LOW>;
> > >  	phy-handle = <&phy>;
> > > -	clocks = <&clks IMX6QDL_CLK_ENET>, <&clks IMX6QDL_CLK_ENET>, <&rmii_clk>;
> > > +	clocks = <&clks IMX6QDL_CLK_ENET>,
> > > +		 <&clks IMX6QDL_CLK_ENET>,
> > > +		 <&rmii_clk>,
> > > +		 <&clks IMX6QDL_CLK_ENET_REF>;
> > 
> > The change is fine, but if you agree to adding clock-names needs
> > adaption of course.
> > 
> 
> After Shawn's feedback concerning the Fixes line I will send a v2.

I wouldn't wait. Just send a v2 with or without a Fixes line and mention
below the --- that Shawn should add or drop it if he doesn't agree to
your choice.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] ARM: dts: ci4x10: Adapt to changes in imx6qdl.dtsi regarding fec clocks

[Thorsten Scherer]

Commit f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk
support") added another item to the list of clocks for the fec
device. As imx6dl-eckelmann-ci4x10.dts only overwrites clocks,
but not clock-names this resulted in an inconsistency with
clocks having one item more than clock-names.

Also overwrite clock-names with the same value as in
imx6qdl.dtsi. This is a no-op today, but prevents similar
inconsistencies if the soc file will be changed in a similar way
in the future.

Signed-off-by: Thorsten Scherer <t.scherer@eckelmann.de>
---
@Shawn: Please feel free to add a Fixes: line, if you will.

 arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
index b4a9523e325b..864dc5018451 100644
--- a/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
+++ b/arch/arm/boot/dts/imx6dl-eckelmann-ci4x10.dts
@@ -297,7 +297,11 @@ &fec {
 	phy-mode = "rmii";
 	phy-reset-gpios = <&gpio1 18 GPIO_ACTIVE_LOW>;
 	phy-handle = <&phy>;
-	clocks = <&clks IMX6QDL_CLK_ENET>, <&clks IMX6QDL_CLK_ENET>, <&rmii_clk>;
+	clocks = <&clks IMX6QDL_CLK_ENET>,
+		 <&clks IMX6QDL_CLK_ENET>,
+		 <&rmii_clk>,
+		 <&clks IMX6QDL_CLK_ENET_REF>;
+	clock-names = "ipg", "ahb", "ptp", "enet_out";
 	status = "okay";
 
 	mdio {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: dts: ci4x10: Adapt to changes in imx6qdl.dtsi regarding fec clocks

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1179 bytes --]

Hello Thorsten,

On Wed, Apr 06, 2022 at 06:39:45AM +0200, Thorsten Scherer wrote:
> Commit f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk
> support") added another item to the list of clocks for the fec
> device. As imx6dl-eckelmann-ci4x10.dts only overwrites clocks,
> but not clock-names this resulted in an inconsistency with
> clocks having one item more than clock-names.
> 
> Also overwrite clock-names with the same value as in
> imx6qdl.dtsi. This is a no-op today, but prevents similar
> inconsistencies if the soc file will be changed in a similar way
> in the future.

What an elaborate wording :-)

> Signed-off-by: Thorsten Scherer <t.scherer@eckelmann.de>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

> ---
> @Shawn: Please feel free to add a Fixes: line, if you will.

s/will/want/ (If I had to guess, I'd say Thorsten is a German. :-)

The Fixes line would be:

Fixes: f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk support")

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: dts: ci4x10: Adapt to changes in imx6qdl.dtsi regarding fec clocks

[Shawn Guo]

On Wed, Apr 06, 2022 at 06:39:45AM +0200, Thorsten Scherer wrote:
> Commit f3e7dae323ab ("ARM: dts: imx6qdl: add enet_out clk
> support") added another item to the list of clocks for the fec
> device. As imx6dl-eckelmann-ci4x10.dts only overwrites clocks,
> but not clock-names this resulted in an inconsistency with
> clocks having one item more than clock-names.
> 
> Also overwrite clock-names with the same value as in
> imx6qdl.dtsi. This is a no-op today, but prevents similar
> inconsistencies if the soc file will be changed in a similar way
> in the future.
> 
> Signed-off-by: Thorsten Scherer <t.scherer@eckelmann.de>

Applied, thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel