| Message ID | 20220329061254.59797-1-t.scherer@eckelmann.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: ci4x10: Reflect upstream change to fec clocks | expand |
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
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
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
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 {
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(-)