| Message ID | 20231120212037.911774-10-peter.griffin@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand |
On Mon, 20 Nov 2023 21:20:27 +0000, Peter Griffin wrote: > Specifying samsung,uart-fifosize in both DT and driver static data is error > prone and relies on driver probe order and dt aliases to be correct. > > Additionally on many Exynos platforms these are (USI) universal serial > interfaces which can be uart, spi or i2c, so it can change per board. > > For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a > required property. For these platforms fifosize now *only* comes from DT. > > It is hoped other Exynos platforms will also switch over time. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/serial/samsung_uart.yaml:141:8: [warning] wrong indentation: expected 8 but found 7 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231120212037.911774-10-peter.griffin@linaro.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: > Specifying samsung,uart-fifosize in both DT and driver static data is error > prone and relies on driver probe order and dt aliases to be correct. > > Additionally on many Exynos platforms these are (USI) universal serial > interfaces which can be uart, spi or i2c, so it can change per board. > > For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a > required property. For these platforms fifosize now *only* comes from DT. > > It is hoped other Exynos platforms will also switch over time. Then allow the property on them. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > index ccc3626779d9..22a1edadc4fe 100644 > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > @@ -133,6 +133,23 @@ allOf: > - const: uart > - const: clk_uart_baud0 > > + - if: > + properties: > + compatible: > + contains: > + enum: > + - google,gs101-uart > + - samsung,exynosautov9-uart > + then: > + properties: > + samsung,uart-fifosize: > + description: The fifo size supported by the UART channel. > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [16, 64, 256] We already have 'fifo-size' in several drivers. Use that. Please move its type/description definitions to serial.yaml and make drivers just do 'fifo-size: true' if they use it. > + > + required: > + - samsung,uart-fifosize A new required property is an ABI break. Please explain why that is okay in the commit message. Rob
Hi Rob, Thanks for your review. On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote: > > On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: > > Specifying samsung,uart-fifosize in both DT and driver static data is error > > prone and relies on driver probe order and dt aliases to be correct. > > > > Additionally on many Exynos platforms these are (USI) universal serial > > interfaces which can be uart, spi or i2c, so it can change per board. > > > > For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a > > required property. For these platforms fifosize now *only* comes from DT. > > > > It is hoped other Exynos platforms will also switch over time. > > Then allow the property on them. Not sure I fully understand your comment. Can you elaborate? Do you mean leave the 'samsung,uart-fifosize' as an optional property like it is currently even for the platforms that now require it to be present to function correctly? I deliberately restricted the yaml change to only require this property for the SoCs that already set the 'samsung,uart-fifosize' dt property. As setting the property and having the driver use what is specified in DT also requires a corresponding driver update (otherwise fifosize gets overwritten by the driver static data, and then becomes dependent on probe order, dt aliases etc). The rationale was drivers 'opt in' and add themselves to the compatibles in this patch as they migrate away from obtaining fifo size from driver static data to obtaining it from DT. > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > index ccc3626779d9..22a1edadc4fe 100644 > > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > @@ -133,6 +133,23 @@ allOf: > > - const: uart > > - const: clk_uart_baud0 > > > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - google,gs101-uart > > + - samsung,exynosautov9-uart > > + then: > > + properties: > > + samsung,uart-fifosize: > > + description: The fifo size supported by the UART channel. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [16, 64, 256] > > We already have 'fifo-size' in several drivers. Use that. Please move > its type/description definitions to serial.yaml and make drivers just do > 'fifo-size: true' if they use it. What do you suggest we do for the samsung,uart-fifosize property that is being used upstream? > > > + > > + required: > > + - samsung,uart-fifosize > > A new required property is an ABI break. Please explain why that is okay > in the commit message. > I can update the commit message to make clear there is an ABI break. As mentioned above the platforms where this is now required are either already setting the property or are new in this series. Is that sufficient justification? regards, Peter
On 21/11/2023 18:15, Peter Griffin wrote: > Hi Rob, > > Thanks for your review. > > On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote: >> >> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: >>> Specifying samsung,uart-fifosize in both DT and driver static data is error >>> prone and relies on driver probe order and dt aliases to be correct. >>> >>> Additionally on many Exynos platforms these are (USI) universal serial >>> interfaces which can be uart, spi or i2c, so it can change per board. >>> >>> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a >>> required property. For these platforms fifosize now *only* comes from DT. >>> >>> It is hoped other Exynos platforms will also switch over time. >> >> Then allow the property on them. > > Not sure I fully understand your comment. Can you elaborate? Do you > mean leave the 'samsung,uart-fifosize' as an optional property like it > is currently even for the platforms that now require it to be present > to function correctly? > > I deliberately restricted the yaml change to only require this > property for the SoCs that already set the 'samsung,uart-fifosize' dt > property. As setting the property and having the driver use what is > specified in DT also requires a corresponding driver update (otherwise > fifosize gets overwritten by the driver static data, and then becomes > dependent on probe order, dt aliases etc). The rationale was drivers > 'opt in' and add themselves to the compatibles in this patch as they > migrate away from obtaining fifo size from driver static data to > obtaining it from DT. Your code diff looks like you are adding the property only to these models. > >> >>> >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >>> --- >>> .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> index ccc3626779d9..22a1edadc4fe 100644 >>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> @@ -133,6 +133,23 @@ allOf: >>> - const: uart >>> - const: clk_uart_baud0 >>> >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + enum: >>> + - google,gs101-uart >>> + - samsung,exynosautov9-uart >>> + then: >>> + properties: >>> + samsung,uart-fifosize: >>> + description: The fifo size supported by the UART channel. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [16, 64, 256] >> >> We already have 'fifo-size' in several drivers. Use that. Please move >> its type/description definitions to serial.yaml and make drivers just do >> 'fifo-size: true' if they use it. > > What do you suggest we do for the samsung,uart-fifosize property that > is being used upstream? Nothing, your diff is just wrong. Or at least nothing needed. Just drop all this properties: here and only make it required for Google GS101. > >> >>> + >>> + required: >>> + - samsung,uart-fifosize >> >> A new required property is an ABI break. Please explain why that is okay >> in the commit message. >> > > I can update the commit message to make clear there is an ABI break. > As mentioned above the platforms where this is now required are either > already setting the property or are new in this series. Is that > sufficient justification? Yes, but only first case. You need to order your patches correctly - first is ABI break expecting ExynopsAutov9 to provide FIFO size in DTS with its explanation. Second commit is adding GS101 where there is no ABI break. Best regards, Krzysztof
Hi Krzysztof, On Wed, 22 Nov 2023 at 07:49, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 21/11/2023 18:15, Peter Griffin wrote: > > Hi Rob, > > > > Thanks for your review. > > > > On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote: > >> > >> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote: > >>> Specifying samsung,uart-fifosize in both DT and driver static data is error > >>> prone and relies on driver probe order and dt aliases to be correct. > >>> > >>> Additionally on many Exynos platforms these are (USI) universal serial > >>> interfaces which can be uart, spi or i2c, so it can change per board. > >>> > >>> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a > >>> required property. For these platforms fifosize now *only* comes from DT. > >>> > >>> It is hoped other Exynos platforms will also switch over time. > >> > >> Then allow the property on them. > > > > Not sure I fully understand your comment. Can you elaborate? Do you > > mean leave the 'samsung,uart-fifosize' as an optional property like it > > is currently even for the platforms that now require it to be present > > to function correctly? > > > > I deliberately restricted the yaml change to only require this > > property for the SoCs that already set the 'samsung,uart-fifosize' dt > > property. As setting the property and having the driver use what is > > specified in DT also requires a corresponding driver update (otherwise > > fifosize gets overwritten by the driver static data, and then becomes > > dependent on probe order, dt aliases etc). The rationale was drivers > > 'opt in' and add themselves to the compatibles in this patch as they > > migrate away from obtaining fifo size from driver static data to > > obtaining it from DT. > > Your code diff looks like you are adding the property only to these models. OK, I intended to only make it a required DT property for these models. Presumably there is a better way to achieve that. > > > > >> > >>> > >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > >>> --- > >>> .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ > >>> 1 file changed, 17 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > >>> index ccc3626779d9..22a1edadc4fe 100644 > >>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > >>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > >>> @@ -133,6 +133,23 @@ allOf: > >>> - const: uart > >>> - const: clk_uart_baud0 > >>> > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + enum: > >>> + - google,gs101-uart > >>> + - samsung,exynosautov9-uart > >>> + then: > >>> + properties: > >>> + samsung,uart-fifosize: > >>> + description: The fifo size supported by the UART channel. > >>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>> + enum: [16, 64, 256] > >> > >> We already have 'fifo-size' in several drivers. Use that. Please move > >> its type/description definitions to serial.yaml and make drivers just do > >> 'fifo-size: true' if they use it. > > > > What do you suggest we do for the samsung,uart-fifosize property that > > is being used upstream? > > Nothing, your diff is just wrong. Or at least nothing needed. Just drop > all this properties: here and only make it required for Google GS101. OK, so your happy with the approach just not the implementation/patch and you don't want it updated to use fifo-size instead of samsung,uart-fifosize > > > > > >> > >>> + > >>> + required: > >>> + - samsung,uart-fifosize > >> > >> A new required property is an ABI break. Please explain why that is okay > >> in the commit message. > >> > > > > I can update the commit message to make clear there is an ABI break. > > As mentioned above the platforms where this is now required are either > > already setting the property or are new in this series. Is that > > sufficient justification? > Yes, but only first case. You need to order your patches correctly - > first is ABI break expecting ExynopsAutov9 to provide FIFO size in DTS > with its explanation. Second commit is adding GS101 where there is no > ABI break. OK, I'll drop the ExynopsAutov9 part then. I don't want to complicate this series by introducing an ABI breakage on some other unrelated Exynos platform. Peter
diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml index ccc3626779d9..22a1edadc4fe 100644 --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml @@ -133,6 +133,23 @@ allOf: - const: uart - const: clk_uart_baud0 + - if: + properties: + compatible: + contains: + enum: + - google,gs101-uart + - samsung,exynosautov9-uart + then: + properties: + samsung,uart-fifosize: + description: The fifo size supported by the UART channel. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [16, 64, 256] + + required: + - samsung,uart-fifosize + unevaluatedProperties: false examples:
Specifying samsung,uart-fifosize in both DT and driver static data is error prone and relies on driver probe order and dt aliases to be correct. Additionally on many Exynos platforms these are (USI) universal serial interfaces which can be uart, spi or i2c, so it can change per board. For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a required property. For these platforms fifosize now *only* comes from DT. It is hoped other Exynos platforms will also switch over time. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- .../bindings/serial/samsung_uart.yaml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)