| Message ID | 20210805065429.27485-1-clin@suse.com |
|---|---|
| Headers | show |
| Series | arm64: dts: initial NXP S32G2 support | expand |
On Thu, Aug 05, 2021 at 02:54:21PM +0800, Chester Lin wrote: > Hello, > > Here I'd like to propose a patchset, which is initial upstream support for NXP > S32G2. S32G is a processor family developed by NXP for automotive solutions, > such as vehicle networking and automotive high-performance processing. This > series focuses on S32G2, which is the latest generation we can find at the > moment. As the first round to support S32G2, this patchset only enables basic > components and interfaces the SoC must have while kernel booting, which aims > to have minimum hardware enablement for these two boards, S32G-VNP-EVB and > S32G-VNP-RDB2. The concepts of how these boards work are originated from the > downstream kernel tree[1] developed by NXP, which provides lots of details > about the SoC S32G274A and its integrated boards. This series has been > verified with downstream ATF[2] & U-Boot[3] based on the ATF boot flow. > > Thanks, > Chester > > [1] https://source.codeaurora.org/external/autobsps32/linux/ > [2] https://source.codeaurora.org/external/autobsps32/arm-trusted-firmware/ > [3] https://source.codeaurora.org/external/autobsps32/u-boot/ > > Chester Lin (8): > dt-bindings: arm: fsl: add NXP S32G2 boards > dt-bindings: serial: fsl-linflexuart: convert to json-schema format > dt-bindings: serial: fsl-linflexuart: Add compatible for S32G2 > arm64: dts: add NXP S32G2 support > arm64: dts: s32g2: add serial/uart support > arm64: dts: s32g2: add VNP-EVB and VNP-RDB2 support > arm64: dts: s32g2: add memory nodes for evb and rdb2 The dts changes look good to me. I will pick up the series once bindings gets acked by Rob. Shawn > MAINTAINERS: Add an entry for NXP S32G2 boards > > .../devicetree/bindings/arm/fsl.yaml | 7 + > .../bindings/serial/fsl,s32-linflexuart.txt | 22 --- > .../bindings/serial/fsl,s32-linflexuart.yaml | 66 +++++++++ > MAINTAINERS | 6 + > arch/arm64/boot/dts/freescale/Makefile | 2 + > arch/arm64/boot/dts/freescale/s32g2.dtsi | 129 ++++++++++++++++++ > .../arm64/boot/dts/freescale/s32g274a-evb.dts | 29 ++++ > .../boot/dts/freescale/s32g274a-rdb2.dts | 33 +++++ > 8 files changed, 272 insertions(+), 22 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.txt > create mode 100644 Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > create mode 100644 arch/arm64/boot/dts/freescale/s32g2.dtsi > create mode 100644 arch/arm64/boot/dts/freescale/s32g274a-evb.dts > create mode 100644 arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts > > -- > 2.30.0 >
Hello Rob and NXP, On 05.08.21 08:54, Chester Lin wrote: > Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference > design 2 board ( S32G-VNP-RDB2). > > Signed-off-by: Chester Lin <clin@suse.com> > --- > Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml > index e2097011c4b0..3914aa09e503 100644 > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > @@ -983,6 +983,13 @@ properties: > - const: solidrun,lx2160a-cex7 > - const: fsl,lx2160a > > + - description: S32G2 based Boards > + items: > + - enum: > + - fsl,s32g274a-evb > + - fsl,s32g274a-rdb2 @Rob: Should for new boards the description: syntax be used also for enums? Or just at SoC level, and for board enums still traditional # comments? > + - const: fsl,s32g2 @NXP: Is it sufficient here to have s32g2, or should we call this s32g274a and adjust the description above to S32G274A? Related, is the trailing A for Arm, like for the Layerscape chips? I.e., not for Alpha or rev.A or something that will change for non-eval chips? > + > - description: S32V234 based Boards > items: > - enum: Otherwise, Reviewed-by: Andreas Färber <afaerber@suse.de> Thanks, Andreas -- SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer HRB 36809 (AG Nürnberg)
On 05.08.21 08:54, Chester Lin wrote: > Add a compatible string for the uart binding of NXP S32G2 platforms. Here > we use "s32v234-linflexuart" as fallback since the current linflexuart > driver still works on S32G2. > > Signed-off-by: Chester Lin <clin@suse.com> > --- > .../bindings/serial/fsl,s32-linflexuart.yaml | 26 ++++++++++++++++--- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > index acfe34706ccb..e731f3f6cea4 100644 > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > @@ -21,10 +21,20 @@ allOf: > > properties: > compatible: > - description: The LINFlexD controller on S32V234 SoC, which can be > - configured in UART mode. > - items: > - - const: fsl,s32v234-linflexuart > + minItems: 1 > + maxItems: 2 Are these necessary for oneOf? > + oneOf: > + - description: The LINFlexD controller on S32G2 SoC, which can be > + configured in UART mode. > + items: > + - enum: > + - fsl,s32g2-linflexuart > + - const: fsl,s32v234-linflexuart This reads inconsistent to me: Either this oneOf is for S32G2 only, then please turn the enum into a const. Or change the description to "on SoCs compatible with S32V234" if we expect the enum list to grow. I believe the idea here was to avoid unnecessary driver compatible and earlycon compatible additions, while preparing for eventual quirks specific to S32G2. @NXP: Should this be s32g2- like above or s32g274a- specifically? Do you agree this is a useful thing to prepare here, as opposed to using only s32v234- in the s32g2* DT? I assume the ordering is done alphabetically as S32G < S32V; alternatively we might order S32V234 first and then the compatible ones. > + > + - description: The LINFlexD controller on S32V234 SoC, which can be > + configured in UART mode. > + items: > + - const: fsl,s32v234-linflexuart To minimize this S32G2 patch, would it be valid to do oneOf for the single S32V in the preceding patch already? Then we would avoid the text movement and re-indentation above and more easily see the lines newly getting added for S32G2. > > reg: > maxItems: 1 > @@ -41,8 +51,16 @@ unevaluatedProperties: false > > examples: > - | > + /* S32V234 */ Could this be: - description: S32V234 | ? > serial@40053000 { > compatible = "fsl,s32v234-linflexuart"; > reg = <0x40053000 0x1000>; > interrupts = <0 59 4>; > }; > + > + /* S32G2 */ This should not be part of the S32V example, but a new one: - | (or with description, as discussed above) > + serial@401c8000 { > + compatible = "fsl,s32g2-linflexuart", "fsl,s32v234-linflexuart"; Potentially affected by naming discussions above. > + reg = <0x401c8000 0x3000>; > + interrupts = <0 82 1>; > + }; Regards, Andreas -- SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer HRB 36809 (AG Nürnberg)
Hi Chester et al., On 05.08.21 08:54, Chester Lin wrote: > Add memory nodes for S32G-VNP-EVB and S32G-VNP-RDB2 since they have fixed > RAM size. You can drop "since they have fixed RAM size" - if they didn't, you would simply choose the lowest size and rely on the bootloader (U-Boot) to overwrite it with the actually detected size. Please expand why this patch is separate - BSP based, I assume? > > Signed-off-by: Chester Lin <clin@suse.com> > --- > arch/arm64/boot/dts/freescale/s32g274a-evb.dts | 8 ++++++++ > arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts | 8 ++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts > index a1ae5031730a..cd41f0af5dd8 100644 > --- a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts > +++ b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-or-later OR MIT > /* > * Copyright (c) 2021 SUSE LLC > + * Copyright 2019-2020 NXP @NXP: Please review year, alignment. Do any Signed-off-bys apply? > */ > > /dts-v1/; > @@ -14,6 +15,13 @@ / { > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + memory@80000000 { > + device_type = "memory"; > + /* 4GB RAM */ This looks strange to me - either put /* 4 GiB RAM */ before the node, three lines above, and/or append comment /* 2 GiB */ on each line below. Note the space, and suggest to be precise about factor 1024 vs. 1000. > + reg = <0 0x80000000 0 0x80000000>, Note that this gives you the range to use for the .dtsi /soc node: Address 0x0 with size 0x80000000 gets mapped to 0x0 0x0, excluding the upper 0x80000000 for the RAM here. Or address 0x0 0x0 for two /soc cells if there are high-memory peripherals. > + <8 0x80000000 0 0x80000000>; Maybe use 0x8 here and 0x0 above? (second 0 stays same, so don't mind) > + }; > }; > > &uart0 { > diff --git a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts > index b2faae306b70..8fbbf3b45eb8 100644 > --- a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts > +++ b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-or-later OR MIT > /* > * Copyright (c) 2021 SUSE LLC > + * Copyright 2019-2020 NXP @NXP: 2021? > */ > > /dts-v1/; > @@ -14,6 +15,13 @@ / { > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + memory@80000000 { > + device_type = "memory"; > + /* 4GB RAM */ > + reg = <0 0x80000000 0 0x80000000>, > + <8 0x80000000 0 0x80000000>; > + }; Same comments as for EVB. > }; > > &uart0 { Regards, Andreas -- SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer HRB 36809 (AG Nürnberg)
Hi Andreas On Thu, 2021-08-12 at 19:42 +0200, Andreas Färber wrote: > Hi Chester et al., > > On 05.08.21 08:54, Chester Lin wrote: > > Add serial/uart support for NXP S32G2. > > You might mention here that (following our initial stub) this commit > is > now apparently based on the CodeAurora BSP branch foo (and therefore > adding its last-year copyright below and separate from 4/8). > > > > > @NXP: If there are downstream Signed-off-bys that you would like to > see > included for this portion here, please speak up. Larisa signed-off should be added. Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com> > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > arch/arm64/boot/dts/freescale/s32g2.dtsi | 31 > > ++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi > > b/arch/arm64/boot/dts/freescale/s32g2.dtsi > > index 3321819c1a2d..0076eacad8a6 100644 > > --- a/arch/arm64/boot/dts/freescale/s32g2.dtsi > > +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0-or-later OR MIT > > /* > > * Copyright (c) 2021 SUSE LLC > > + * Copyright 2017-2020 NXP > > @NXP: Should this be updated to include 2021 from your latest BSP > releases? Do you want it visually aligned by adding the ASCII-art? Yes for both questions. The copyright year sould be updated to 2021 and should be visually aligned. > > > */ > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > @@ -11,6 +12,12 @@ / { > > #address-cells = <2>; > > #size-cells = <2>; > > > > + aliases { > > + serial0 = &uart0; > > + serial1 = &uart1; > > + serial2 = &uart2; > > + }; > > Note: In the past there had been controversies as to whether to > define > aliases globally for a SoC or in a .dts specific to a board's usage. > In this case it does not seem to matter much, as uart0 is being used > as > console on the reference boards. > > > + > > cpus { > > #address-cells = <1>; > > #size-cells = <0>; > > @@ -82,6 +89,30 @@ soc { > > > > ranges; > > > > + uart0: serial@401c8000 { > > + compatible = "fsl,s32g2-linflexuart", > > + "fsl,s32v234-linflexuart"; > > + reg = <0 0x401c8000 0 0x3000>; > > + interrupts = <GIC_SPI 82 > > IRQ_TYPE_EDGE_RISING>; > > + status = "disabled"; > > + }; > > + > > + uart1: serial@401cc000 { > > + compatible = "fsl,s32g2-linflexuart", > > + "fsl,s32v234-linflexuart"; > > + reg = <0 0x401cc000 0 0x3000>; > > + interrupts = <GIC_SPI 83 > > IRQ_TYPE_EDGE_RISING>; > > + status = "disabled"; > > + }; > > + > > + uart2: serial@402bc000 { > > + compatible = "fsl,s32g2-linflexuart", > > + "fsl,s32v234-linflexuart"; > > + reg = <0 0x402bc000 0 0x3000>; > > + interrupts = <GIC_SPI 84 > > IRQ_TYPE_EDGE_RISING>; > > + status = "disabled"; > > + }; > > + > > gic: interrupt-controller@50800000 { > > compatible = "arm,gic-v3"; > > #interrupt-cells = <3>; > > Regards, > Andreas >
On Thu, 2021-08-12 at 18:27 +0200, Andreas Färber wrote: > On 05.08.21 08:54, Chester Lin wrote: > > Add a compatible string for the uart binding of NXP S32G2 > > platforms. Here > > we use "s32v234-linflexuart" as fallback since the current > > linflexuart > > driver still works on S32G2. > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > .../bindings/serial/fsl,s32-linflexuart.yaml | 26 > > ++++++++++++++++--- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32- > > linflexuart.yaml > > b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > index acfe34706ccb..e731f3f6cea4 100644 > > --- a/Documentation/devicetree/bindings/serial/fsl,s32- > > linflexuart.yaml > > +++ b/Documentation/devicetree/bindings/serial/fsl,s32- > > linflexuart.yaml > > @@ -21,10 +21,20 @@ allOf: > > > > properties: > > compatible: > > - description: The LINFlexD controller on S32V234 SoC, which can > > be > > - configured in UART mode. > > - items: > > - - const: fsl,s32v234-linflexuart > > + minItems: 1 > > + maxItems: 2 > > Are these necessary for oneOf? > > > + oneOf: > > + - description: The LINFlexD controller on S32G2 SoC, which > > can be > > + configured in UART mode. > > + items: > > + - enum: > > + - fsl,s32g2-linflexuart > > + - const: fsl,s32v234-linflexuart > > This reads inconsistent to me: Either this oneOf is for S32G2 only, > then > please turn the enum into a const. Or change the description to "on > SoCs > compatible with S32V234" if we expect the enum list to grow. > > I believe the idea here was to avoid unnecessary driver compatible > and > earlycon compatible additions, while preparing for eventual quirks > specific to S32G2. > > @NXP: Should this be s32g2- like above or s32g274a- specifically? Do > you > agree this is a useful thing to prepare here, as opposed to using > only > s32v234- in the s32g2* DT? s32g2- is fine, but the vendor should be nxp, not fsl. nxp,s32g2-linflexuart > > I assume the ordering is done alphabetically as S32G < S32V; > alternatively we might order S32V234 first and then the compatible > ones. > > > + > > + - description: The LINFlexD controller on S32V234 SoC, which > > can be > > + configured in UART mode. > > + items: > > + - const: fsl,s32v234-linflexuart > > To minimize this S32G2 patch, would it be valid to do oneOf for the > single S32V in the preceding patch already? Then we would avoid the > text > movement and re-indentation above and more easily see the lines newly > getting added for S32G2. > > > > > reg: > > maxItems: 1 > > @@ -41,8 +51,16 @@ unevaluatedProperties: false > > > > examples: > > - | > > + /* S32V234 */ > > Could this be: > - description: S32V234 > | > ? > > > serial@40053000 { > > compatible = "fsl,s32v234-linflexuart"; > > reg = <0x40053000 0x1000>; > > interrupts = <0 59 4>; > > }; > > + > > + /* S32G2 */ > > This should not be part of the S32V example, but a new one: > > - | > > (or with description, as discussed above) > > > + serial@401c8000 { > > + compatible = "fsl,s32g2-linflexuart", "fsl,s32v234- > > linflexuart"; > > Potentially affected by naming discussions above. > > > + reg = <0x401c8000 0x3000>; > > + interrupts = <0 82 1>; > > + }; > > Regards, > Andreas >
On Thu, Aug 12, 2021 at 08:25:00PM +0200, Andreas Färber wrote: > Hi Chester et al., > > On 05.08.21 08:54, Chester Lin wrote: > > Add memory nodes for S32G-VNP-EVB and S32G-VNP-RDB2 since they have fixed > > RAM size. > > You can drop "since they have fixed RAM size" - if they didn't, you > would simply choose the lowest size and rely on the bootloader (U-Boot) > to overwrite it with the actually detected size. > > Please expand why this patch is separate - BSP based, I assume? > Yes, the information of memory banks is from s32 downstream kernel, which is listed in the board DTs of older releases [bsp27] although newer releases [bsp28 & bsp29] have moved it into s32 downstream u-boot as runtime fdt-fixup. I think we should still reveal this information in kernel DTs in order to have better understanding of system memory ranges that each board can have. @NXP: Please don't hesitate to let me know if any better idea, thanks! > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > arch/arm64/boot/dts/freescale/s32g274a-evb.dts | 8 ++++++++ > > arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts | 8 ++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts > > index a1ae5031730a..cd41f0af5dd8 100644 > > --- a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts > > +++ b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0-or-later OR MIT > > /* > > * Copyright (c) 2021 SUSE LLC > > + * Copyright 2019-2020 NXP > > @NXP: Please review year, alignment. Do any Signed-off-bys apply? > > > */ > > > > /dts-v1/; > > @@ -14,6 +15,13 @@ / { > > chosen { > > stdout-path = "serial0:115200n8"; > > }; > > + > > + memory@80000000 { > > + device_type = "memory"; > > + /* 4GB RAM */ > > This looks strange to me - either put /* 4 GiB RAM */ before the node, > three lines above, and/or append comment /* 2 GiB */ on each line below. > Note the space, and suggest to be precise about factor 1024 vs. 1000. > Thank you for the suggestion. > > + reg = <0 0x80000000 0 0x80000000>, > > Note that this gives you the range to use for the .dtsi /soc node: > Address 0x0 with size 0x80000000 gets mapped to 0x0 0x0, excluding the > upper 0x80000000 for the RAM here. Or address 0x0 0x0 for two /soc cells > if there are high-memory peripherals. > I don't know if memory ranges might be changed for new boards or CPU revisions in the future, which means the first memory range might be expanded as well [e.g. 0~4GB]. Based this assumption, I think the size should also be changed accordingly. Not sure if overlays can still work with this case but overwriting all reg properties under /soc could be awful. However if we only have to think of current hardware spec, it's good to declare "range = <0 0 0 0x80000000>". Please feel free to let me know if any suggestions, thanks. > > + <8 0x80000000 0 0x80000000>; > > Maybe use 0x8 here and 0x0 above? (second 0 stays same, so don't mind) > Will fix it. > > + }; > > }; > > > > &uart0 { > > diff --git a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts > > index b2faae306b70..8fbbf3b45eb8 100644 > > --- a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts > > +++ b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0-or-later OR MIT > > /* > > * Copyright (c) 2021 SUSE LLC > > + * Copyright 2019-2020 NXP > > @NXP: 2021? > > > */ > > > > /dts-v1/; > > @@ -14,6 +15,13 @@ / { > > chosen { > > stdout-path = "serial0:115200n8"; > > }; > > + > > + memory@80000000 { > > + device_type = "memory"; > > + /* 4GB RAM */ > > + reg = <0 0x80000000 0 0x80000000>, > > + <8 0x80000000 0 0x80000000>; > > + }; > > Same comments as for EVB. > > > }; > > > > &uart0 { > > Regards, > Andreas > > -- > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer > HRB 36809 (AG Nürnberg) >
On Thu, Aug 12, 2021 at 05:46:16PM +0200, Andreas Färber wrote: > Hello Rob and NXP, > > On 05.08.21 08:54, Chester Lin wrote: > > Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference > > design 2 board ( S32G-VNP-RDB2). > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml > > index e2097011c4b0..3914aa09e503 100644 > > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > > @@ -983,6 +983,13 @@ properties: > > - const: solidrun,lx2160a-cex7 > > - const: fsl,lx2160a > > > > + - description: S32G2 based Boards > > + items: > > + - enum: > > + - fsl,s32g274a-evb > > + - fsl,s32g274a-rdb2 > > @Rob: Should for new boards the description: syntax be used also for > enums? Or just at SoC level, and for board enums still traditional # > comments? It's up to how the platform wants to do it. Rob
On Thu, Aug 05, 2021 at 02:54:22PM +0800, Chester Lin wrote: > Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference > design 2 board ( S32G-VNP-RDB2). > > Signed-off-by: Chester Lin <clin@suse.com> > --- > Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml > index e2097011c4b0..3914aa09e503 100644 > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > @@ -983,6 +983,13 @@ properties: > - const: solidrun,lx2160a-cex7 > - const: fsl,lx2160a > > + - description: S32G2 based Boards > + items: > + - enum: > + - fsl,s32g274a-evb > + - fsl,s32g274a-rdb2 > + - const: fsl,s32g2 Given this is an entirely different family from i.MX and new?, shouldn't it use 'nxp' instead of 'fsl'? Either way, Acked-by: Rob Herring <robh@kernel.org> Rob
On Thu, Aug 05, 2021 at 02:54:24PM +0800, Chester Lin wrote: > Add a compatible string for the uart binding of NXP S32G2 platforms. Here > we use "s32v234-linflexuart" as fallback since the current linflexuart > driver still works on S32G2. > > Signed-off-by: Chester Lin <clin@suse.com> > --- > .../bindings/serial/fsl,s32-linflexuart.yaml | 26 ++++++++++++++++--- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > index acfe34706ccb..e731f3f6cea4 100644 > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > @@ -21,10 +21,20 @@ allOf: > > properties: > compatible: > - description: The LINFlexD controller on S32V234 SoC, which can be > - configured in UART mode. > - items: > - - const: fsl,s32v234-linflexuart > + minItems: 1 > + maxItems: 2 minItems/maxItems not needed here. > + oneOf: > + - description: The LINFlexD controller on S32G2 SoC, which can be > + configured in UART mode. > + items: > + - enum: > + - fsl,s32g2-linflexuart > + - const: fsl,s32v234-linflexuart > + > + - description: The LINFlexD controller on S32V234 SoC, which can be > + configured in UART mode. > + items: > + - const: fsl,s32v234-linflexuart > > reg: > maxItems: 1 > @@ -41,8 +51,16 @@ unevaluatedProperties: false > > examples: > - | > + /* S32V234 */ > serial@40053000 { > compatible = "fsl,s32v234-linflexuart"; > reg = <0x40053000 0x1000>; > interrupts = <0 59 4>; > }; > + > + /* S32G2 */ > + serial@401c8000 { > + compatible = "fsl,s32g2-linflexuart", "fsl,s32v234-linflexuart"; > + reg = <0x401c8000 0x3000>; > + interrupts = <0 82 1>; > + }; This doesn't really warrant another example just for a new compatible string.
On Thu, Aug 12, 2021 at 06:27:57PM +0200, Andreas Färber wrote: > On 05.08.21 08:54, Chester Lin wrote: > > Add a compatible string for the uart binding of NXP S32G2 platforms. Here > > we use "s32v234-linflexuart" as fallback since the current linflexuart > > driver still works on S32G2. > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > .../bindings/serial/fsl,s32-linflexuart.yaml | 26 ++++++++++++++++--- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > index acfe34706ccb..e731f3f6cea4 100644 > > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml > > @@ -21,10 +21,20 @@ allOf: > > > > properties: > > compatible: > > - description: The LINFlexD controller on S32V234 SoC, which can be > > - configured in UART mode. > > - items: > > - - const: fsl,s32v234-linflexuart > > + minItems: 1 > > + maxItems: 2 > > Are these necessary for oneOf? > > > + oneOf: > > + - description: The LINFlexD controller on S32G2 SoC, which can be > > + configured in UART mode. > > + items: > > + - enum: > > + - fsl,s32g2-linflexuart > > + - const: fsl,s32v234-linflexuart > > This reads inconsistent to me: Either this oneOf is for S32G2 only, then > please turn the enum into a const. Or change the description to "on SoCs > compatible with S32V234" if we expect the enum list to grow. > > I believe the idea here was to avoid unnecessary driver compatible and > earlycon compatible additions, while preparing for eventual quirks > specific to S32G2. > > @NXP: Should this be s32g2- like above or s32g274a- specifically? Do you > agree this is a useful thing to prepare here, as opposed to using only > s32v234- in the s32g2* DT? > > I assume the ordering is done alphabetically as S32G < S32V; > alternatively we might order S32V234 first and then the compatible ones. > > > + > > + - description: The LINFlexD controller on S32V234 SoC, which can be > > + configured in UART mode. > > + items: > > + - const: fsl,s32v234-linflexuart > > To minimize this S32G2 patch, would it be valid to do oneOf for the > single S32V in the preceding patch already? Then we would avoid the text > movement and re-indentation above and more easily see the lines newly > getting added for S32G2. > > > > > reg: > > maxItems: 1 > > @@ -41,8 +51,16 @@ unevaluatedProperties: false > > > > examples: > > - | > > + /* S32V234 */ > > Could this be: > - description: S32V234 > | > ? No, that would not be valid json-schema. Rob
On Fri, Aug 13, 2021 at 12:53:59PM -0500, Rob Herring wrote: > On Thu, Aug 05, 2021 at 02:54:22PM +0800, Chester Lin wrote: > > Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference > > design 2 board ( S32G-VNP-RDB2). > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml > > index e2097011c4b0..3914aa09e503 100644 > > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > > @@ -983,6 +983,13 @@ properties: > > - const: solidrun,lx2160a-cex7 > > - const: fsl,lx2160a > > > > + - description: S32G2 based Boards > > + items: > > + - enum: > > + - fsl,s32g274a-evb > > + - fsl,s32g274a-rdb2 > > + - const: fsl,s32g2 > > Given this is an entirely different family from i.MX and new?, shouldn't > it use 'nxp' instead of 'fsl'? Either way, It sounds good and Radu from NXP has mentioned a similar idea for the compatible string of linflexuart. To keep the naming consistency, should we change all 'fsl' to 'nxp' as well? For example, we could rename the fsl.yaml to nxp.yaml. However, changing all of them would cause some impacts, which will need more verifications on new strings. Otherwise we would have to tolerate the naming differences only used by s32g2. Thanks, Chester > > Acked-by: Rob Herring <robh@kernel.org> > > Rob >
On 13.08.21 19:53, Rob Herring wrote: > On Thu, Aug 05, 2021 at 02:54:22PM +0800, Chester Lin wrote: >> Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference >> design 2 board ( S32G-VNP-RDB2). >> >> Signed-off-by: Chester Lin <clin@suse.com> >> --- >> Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml >> index e2097011c4b0..3914aa09e503 100644 >> --- a/Documentation/devicetree/bindings/arm/fsl.yaml >> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml >> @@ -983,6 +983,13 @@ properties: >> - const: solidrun,lx2160a-cex7 >> - const: fsl,lx2160a >> >> + - description: S32G2 based Boards >> + items: >> + - enum: >> + - fsl,s32g274a-evb >> + - fsl,s32g274a-rdb2 >> + - const: fsl,s32g2 > > Given this is an entirely different family from i.MX and new?, shouldn't > it use 'nxp' instead of 'fsl'? S32V also still used fsl prefix, despite the company name long being NXP (same for several Layerscape and i.MX models). If, as Radu indicated on 3/8, NXP wants to make that switch now for S32G then I see no reason against nxp. I verified that it's already defined: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/vendor-prefixes.yaml However, should the matching .dts[i] files using nxp prefix (4-6/8) then still go under dts/freescale/, or should they go to a new dts/nxp/ then? That would separate it from S32V. Intel did do a switch from dts/altera/ to dts/intel/ at some point, so there's precedence for either, I guess. No idea whether anything might break if we moved S32V alongside S32G. Similarly, the easiest and most merge-friendly would be to leave arm/fsl.yaml and add the nxp-prefixed S32G2 there, as done here. If NXP want to rename fsl.yaml to nxp.yaml in a general housekeeping effort, that could be done independently, outside Chester's patchset. > Either way, > > Acked-by: Rob Herring <robh@kernel.org> Thanks, Andreas
Hi Chester, On 18.08.21 16:34, Chester Lin wrote: > On Fri, Aug 13, 2021 at 12:53:59PM -0500, Rob Herring wrote: >> On Thu, Aug 05, 2021 at 02:54:22PM +0800, Chester Lin wrote: >>> Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference >>> design 2 board ( S32G-VNP-RDB2). >>> >>> Signed-off-by: Chester Lin <clin@suse.com> >>> --- >>> Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml >>> index e2097011c4b0..3914aa09e503 100644 >>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml >>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml >>> @@ -983,6 +983,13 @@ properties: >>> - const: solidrun,lx2160a-cex7 >>> - const: fsl,lx2160a >>> >>> + - description: S32G2 based Boards >>> + items: >>> + - enum: >>> + - fsl,s32g274a-evb >>> + - fsl,s32g274a-rdb2 >>> + - const: fsl,s32g2 >> >> Given this is an entirely different family from i.MX and new?, shouldn't >> it use 'nxp' instead of 'fsl'? Either way, > > It sounds good and Radu from NXP has mentioned a similar idea for the > compatible string of linflexuart. To keep the naming consistency, should we > change all 'fsl' to 'nxp' as well? I assume that question was just unclearly phrased, so for the record: ABI stability rules forbid us from changing "all 'fsl'" in compatible strings or property names. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ABI.rst Deployed firmware providing mainline-merged platforms with DTBs using fsl prefix (e.g., the quoted LX2160A) needs to continue working with newer drivers, and deployed mainline Linux should continue working after firmware updates that modify the DTB provided to Linux. So, if NXP wants to use nxp prefix for new S32G bindings, you can do that for your additions only, but for LINFlexD UART (3/8) you will still need to use fsl for the "historical" S32V binding used as fallback. Please keep S32G consistent with itself - so if we decide on nxp here, we should apply it to SoC, boards, LINFlexD and any future peripherals. > For example, we could rename the fsl.yaml > to nxp.yaml. Since other people might be contributing i.MX boards etc. to that file, better not make your patch series conflict with other people's patches, so that it can get merged and we can move on to the next patchsets. The schema filename is not ABI, so it can be renamed later. The .dtb path may become ABI (e.g., U-Boot $fdtfile), thus my comment about consciously deciding between freescale/ vs. nxp/ subdirectory. > However, changing all of them would cause some impacts, which will > need more verifications on new strings. Otherwise we would have to tolerate the > naming differences only used by s32g2. I fear tolerating the mess one way or another is the only viable way. Otherwise both bindings and drivers would need duplication for backwards compatibility, for no good reason - Freescale was acquired back in 2015. Cheers, Andreas
On Mon, 6 Sept 2021 at 22:38, Andreas Färber <afaerber@suse.de> wrote: > > Hi Chester, > > On 18.08.21 16:34, Chester Lin wrote: > > On Fri, Aug 13, 2021 at 12:53:59PM -0500, Rob Herring wrote: > >> On Thu, Aug 05, 2021 at 02:54:22PM +0800, Chester Lin wrote: > >>> Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference > >>> design 2 board ( S32G-VNP-RDB2). > >>> > >>> Signed-off-by: Chester Lin <clin@suse.com> > >>> --- > >>> Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml > >>> index e2097011c4b0..3914aa09e503 100644 > >>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml > >>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > >>> @@ -983,6 +983,13 @@ properties: > >>> - const: solidrun,lx2160a-cex7 > >>> - const: fsl,lx2160a > >>> > >>> + - description: S32G2 based Boards > >>> + items: > >>> + - enum: > >>> + - fsl,s32g274a-evb > >>> + - fsl,s32g274a-rdb2 > >>> + - const: fsl,s32g2 > >> > >> Given this is an entirely different family from i.MX and new?, shouldn't > >> it use 'nxp' instead of 'fsl'? Either way, > > > > It sounds good and Radu from NXP has mentioned a similar idea for the > > compatible string of linflexuart. To keep the naming consistency, should we > > change all 'fsl' to 'nxp' as well? > > I assume that question was just unclearly phrased, so for the record: > > ABI stability rules forbid us from changing "all 'fsl'" in compatible > strings or property names. > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ABI.rst > > Deployed firmware providing mainline-merged platforms with DTBs using > fsl prefix (e.g., the quoted LX2160A) needs to continue working with > newer drivers, and deployed mainline Linux should continue working after > firmware updates that modify the DTB provided to Linux. This is a new platform/SoC therefore there is no ABI. There is no requirement in the kernel that a new ABI (which you define in this patchset in the bindings) should be compatible with something somewhere. It's some misunderstanding of stable ABI. Therefore all new compatibles are allowed to be nxp, not fsl. No one here proposed renaming existing compatibles from fsl tro nxp. We talk about new ones. Different question of course whether you want to be nice to some existing out-of-tree users... but then have in mind that we don't care about out of tree. :) Anyway being nice to out-of-tree is not part of ABI. It's just being nice and useful. Best regards, Krzysztof
Hi Krzysztof, On 07.09.21 08:59, Krzysztof Kozlowski wrote: > On Mon, 6 Sept 2021 at 22:38, Andreas Färber <afaerber@suse.de> wrote: >> On 18.08.21 16:34, Chester Lin wrote: >>> On Fri, Aug 13, 2021 at 12:53:59PM -0500, Rob Herring wrote: >>>> On Thu, Aug 05, 2021 at 02:54:22PM +0800, Chester Lin wrote: >>>>> Add bindings for S32G2's evaluation board (S32G-VNP-EVB) and reference >>>>> design 2 board ( S32G-VNP-RDB2). >>>>> >>>>> Signed-off-by: Chester Lin <clin@suse.com> >>>>> --- >>>>> Documentation/devicetree/bindings/arm/fsl.yaml | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml >>>>> index e2097011c4b0..3914aa09e503 100644 >>>>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml >>>>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml >>>>> @@ -983,6 +983,13 @@ properties: >>>>> - const: solidrun,lx2160a-cex7 >>>>> - const: fsl,lx2160a >>>>> >>>>> + - description: S32G2 based Boards >>>>> + items: >>>>> + - enum: >>>>> + - fsl,s32g274a-evb >>>>> + - fsl,s32g274a-rdb2 >>>>> + - const: fsl,s32g2 >>>> >>>> Given this is an entirely different family from i.MX and new?, shouldn't >>>> it use 'nxp' instead of 'fsl'? Either way, >>> >>> It sounds good and Radu from NXP has mentioned a similar idea for the >>> compatible string of linflexuart. To keep the naming consistency, should we >>> change all 'fsl' to 'nxp' as well? >> >> I assume that question was just unclearly phrased, so for the record: >> >> ABI stability rules forbid us from changing "all 'fsl'" in compatible >> strings or property names. >> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ABI.rst >> >> Deployed firmware providing mainline-merged platforms with DTBs using >> fsl prefix (e.g., the quoted LX2160A) needs to continue working with >> newer drivers, and deployed mainline Linux should continue working after >> firmware updates that modify the DTB provided to Linux. > > This is a new platform/SoC therefore there is no ABI. There is no > requirement in the kernel that a new ABI (which you define in this > patchset in the bindings) should be compatible with something > somewhere. It's some misunderstanding of stable ABI. Therefore all new > compatibles are allowed to be nxp, not fsl. > > No one here proposed renaming existing compatibles from fsl tro nxp. > We talk about new ones. Chester seemingly did: "all 'fsl' ... as well", not "all new 'fsl'" ones, in the patch context of existing fsl.yaml. Like I said, it may just have been unluckily worded. Therefore my saying that it does contain tons of non-new SoC/platform bindings that he's not allowed to break by changing them. > Different question of course whether you want to be nice to some > existing out-of-tree users... but then have in mind that we don't care > about out of tree. :) Anyway being nice to out-of-tree is not part of > ABI. It's just being nice and useful. Nobody is suggesting new S32G ABI be compatible with downstream BSPs. These patches and changes we're discussing already differ from the BSP. My point was that as soon as we merge S32G into mainline, it will become ABI and shouldn't be changed incompatibly anymore once in a release. These automotive platforms don't run off-the-shelf distros yet and will need to get their bootloaders upstreamed, too. In particular we'll need mainline TF-A to merge the SCMI implementation before we can rely on it here in the kernel for a clk driver; that's holding up MMC and Ethernet. Best regards, Andreas