| Message ID | 20221022214233.82467-3-marcel@ziswiler.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | arm: dts: imx: sync device trees with upstream linux kernel part 1 | expand |
Hi Marcel, thanks for contributing, Il 22/10/2022 23:42, Marcel Ziswiler ha scritto: > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > Fix lpuart issue in common U-Boot device tree. There's no need to repeat in commit log the subject. > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > --- > > (no changes since v1) > > arch/arm/dts/imxrt1020-evk-u-boot.dtsi | 7 ++++--- > arch/arm/dts/imxrt1020-evk.dts | 1 - > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi > index 9e1b074d2e..7cab486f5f 100644 > --- a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi > +++ b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi > @@ -67,9 +67,6 @@ > > imxrt1020-evk { > u-boot,dm-spl; > - pinctrl_lpuart1: lpuart1grp { > - u-boot,dm-spl; > - }; > > pinctrl_semc: semcgrp { > u-boot,dm-spl; > @@ -81,6 +78,10 @@ > }; > }; > > +&pinctrl_lpuart1 { > + u-boot,dm-spl; > +}; > + I don't understand the goal of this change, can you elaborate? As I remember pinctrl_lpuart1 already works correctly. The same goes for: pinctrl_semc pinctrl_usdhc0 So you're not fixing something. > &usdhc1 { > u-boot,dm-spl; > }; > diff --git a/arch/arm/dts/imxrt1020-evk.dts b/arch/arm/dts/imxrt1020-evk.dts > index 22ae5ed735..d4d1de4ea8 100644 > --- a/arch/arm/dts/imxrt1020-evk.dts > +++ b/arch/arm/dts/imxrt1020-evk.dts > @@ -6,7 +6,6 @@ > > /dts-v1/; > #include "imxrt1020.dtsi" > -#include "imxrt1020-evk-u-boot.dtsi" This ^^^ is needed, please revert it. > #include "imxrt1020-pinfunc.h" > > / { Have you also tested the change on a board? Best regards
Hi Giulio On Sun, 2022-10-23 at 01:32 +0200, Giulio Benetti wrote: > Hi Marcel, > > thanks for contributing, You are very welcome. > Il 22/10/2022 23:42, Marcel Ziswiler ha scritto: > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > > Fix lpuart issue in common U-Boot device tree. > > There's no need to repeat in commit log the subject. Well, most maintainers do want an actual commit message and won't accept it being empty. For trivial commits like this one it is quite common to therefore just repeat the subject. > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > --- > > > > (no changes since v1) > > > > arch/arm/dts/imxrt1020-evk-u-boot.dtsi | 7 ++++--- > > arch/arm/dts/imxrt1020-evk.dts | 1 - > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi > > index 9e1b074d2e..7cab486f5f 100644 > > --- a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi > > +++ b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi > > @@ -67,9 +67,6 @@ > > > > imxrt1020-evk { > > u-boot,dm-spl; > > - pinctrl_lpuart1: lpuart1grp { > > - u-boot,dm-spl; > > - }; > > > > pinctrl_semc: semcgrp { > > u-boot,dm-spl; > > @@ -81,6 +78,10 @@ > > }; > > }; > > > > +&pinctrl_lpuart1 { > > + u-boot,dm-spl; > > +}; > > + > > I don't understand the goal of this change, can you elaborate? Well, the goal is to use them Linux kernel device trees and have any required changes in such -u-boot.dtsi device tree include files. As such we may just reference resp. node by its handle and subsequently add that U- Boot specific property. > As I remember pinctrl_lpuart1 already works correctly. The same goes > for: > pinctrl_semc > pinctrl_usdhc0 > > So you're not fixing something. Well, I am basically fixing it for when them device trees get synchronised from the Linux kernel. One may basically consider out-of-synch device trees being an issue in need of fixing. > > &usdhc1 { > > u-boot,dm-spl; > > }; > > diff --git a/arch/arm/dts/imxrt1020-evk.dts b/arch/arm/dts/imxrt1020-evk.dts > > index 22ae5ed735..d4d1de4ea8 100644 > > --- a/arch/arm/dts/imxrt1020-evk.dts > > +++ b/arch/arm/dts/imxrt1020-evk.dts > > @@ -6,7 +6,6 @@ > > > > /dts-v1/; > > #include "imxrt1020.dtsi" > > -#include "imxrt1020-evk-u-boot.dtsi" > > This ^^^ is needed, please revert it. No, you do not understand. We just synchronise them device trees from the Linux kernel. Nothing should ever be changed here. Any and all U-Boot specific changes need to go into the -u-boot.dtsi device tree include files which BTW get included automatically by the U-Boot build system. > > #include "imxrt1020-pinfunc.h" > > > > / { > > Have you also tested the change on a board? As outlined in the cover letter I did not test each and every board as I do not have them all available. If you do have some of those boards available I would appreciate you giving it a try. Thanks! > Best regards > -- > Giulio Benetti > CEO/CTO@Benetti Engineering sas Cheers Marcel
Hi Marcel, > Il giorno 24 ott 2022, alle ore 00:01, Marcel Ziswiler <marcel.ziswiler@toradex.com> ha scritto: > > Hi Giulio > >> On Sun, 2022-10-23 at 01:32 +0200, Giulio Benetti wrote: >> Hi Marcel, >> >> thanks for contributing, > > You are very welcome. > >> Il 22/10/2022 23:42, Marcel Ziswiler ha scritto: >>> From: Marcel Ziswiler <marcel.ziswiler@toradex.com> >>> >>> Fix lpuart issue in common U-Boot device tree. >> >> There's no need to repeat in commit log the subject. > > Well, most maintainers do want an actual commit message and won't accept it being empty. For trivial commits > like this one it is quite common to therefore just repeat the subject. You’re right, this is project dependent, here in uboot they want it like that. > >>> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> >>> --- >>> >>> (no changes since v1) >>> >>> arch/arm/dts/imxrt1020-evk-u-boot.dtsi | 7 ++++--- >>> arch/arm/dts/imxrt1020-evk.dts | 1 - >>> 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi >>> index 9e1b074d2e..7cab486f5f 100644 >>> --- a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi >>> +++ b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi >>> @@ -67,9 +67,6 @@ >>> >>> imxrt1020-evk { >>> u-boot,dm-spl; >>> - pinctrl_lpuart1: lpuart1grp { >>> - u-boot,dm-spl; >>> - }; >>> >>> pinctrl_semc: semcgrp { >>> u-boot,dm-spl; >>> @@ -81,6 +78,10 @@ >>> }; >>> }; >>> >>> +&pinctrl_lpuart1 { >>> + u-boot,dm-spl; >>> +}; >>> + >> >> I don't understand the goal of this change, can you elaborate? > > Well, the goal is to use them Linux kernel device trees and have any required changes in such -u-boot.dtsi > device tree include files. As such we may just reference resp. node by its handle and subsequently add that U- > Boot specific property. Ah yes and > >> As I remember pinctrl_lpuart1 already works correctly. The same goes >> for: >> pinctrl_semc >> pinctrl_usdhc0 >> >> So you're not fixing something. > > Well, I am basically fixing it for when them device trees get synchronised from the Linux kernel. One may > basically consider out-of-synch device trees being an issue in need of fixing. Yes. Can you please add this explanation in commit log? It’s easier to keep track and to review too. > >>> &usdhc1 { >>> u-boot,dm-spl; >>> }; >>> diff --git a/arch/arm/dts/imxrt1020-evk.dts b/arch/arm/dts/imxrt1020-evk.dts >>> index 22ae5ed735..d4d1de4ea8 100644 >>> --- a/arch/arm/dts/imxrt1020-evk.dts >>> +++ b/arch/arm/dts/imxrt1020-evk.dts >>> @@ -6,7 +6,6 @@ >>> >>> /dts-v1/; >>> #include "imxrt1020.dtsi" >>> -#include "imxrt1020-evk-u-boot.dtsi" >> >> This ^^^ is needed, please revert it. > > No, you do not understand. We just synchronise them device trees from the Linux kernel. Nothing should ever be > changed here. Any and all U-Boot specific changes need to go into the -u-boot.dtsi device tree include files > which BTW get included automatically by the U-Boot build system. This has been changed during time. Can you please add this note too in commit log? > >>> #include "imxrt1020-pinfunc.h" >>> >>> / { >> >> Have you also tested the change on a board? > > As outlined in the cover letter I did not test each and every board as I do not have them all available. If you > do have some of those boards available I would appreciate you giving it a try. Yes I do have 1020 so I can give a try of V4. Best regards Giulio > > Thanks! > >> Best regards >> -- >> Giulio Benetti >> CEO/CTO@Benetti Engineering sas > > Cheers > > Marcel
diff --git a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi index 9e1b074d2e..7cab486f5f 100644 --- a/arch/arm/dts/imxrt1020-evk-u-boot.dtsi +++ b/arch/arm/dts/imxrt1020-evk-u-boot.dtsi @@ -67,9 +67,6 @@ imxrt1020-evk { u-boot,dm-spl; - pinctrl_lpuart1: lpuart1grp { - u-boot,dm-spl; - }; pinctrl_semc: semcgrp { u-boot,dm-spl; @@ -81,6 +78,10 @@ }; }; +&pinctrl_lpuart1 { + u-boot,dm-spl; +}; + &usdhc1 { u-boot,dm-spl; }; diff --git a/arch/arm/dts/imxrt1020-evk.dts b/arch/arm/dts/imxrt1020-evk.dts index 22ae5ed735..d4d1de4ea8 100644 --- a/arch/arm/dts/imxrt1020-evk.dts +++ b/arch/arm/dts/imxrt1020-evk.dts @@ -6,7 +6,6 @@ /dts-v1/; #include "imxrt1020.dtsi" -#include "imxrt1020-evk-u-boot.dtsi" #include "imxrt1020-pinfunc.h" / {