| Message ID | 20231120212037.911774-10-peter.griffin@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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.
Hi Peter, kernel test robot noticed the following build warnings: [auto build test WARNING on pinctrl-samsung/for-next] [also build test WARNING on next-20231120] [cannot apply to krzk/for-next robh/for-next linus/master v6.7-rc2] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Peter-Griffin/dt-bindings-soc-samsung-exynos-pmu-Add-gs101-compatible/20231121-052449 base: https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/samsung.git for-next patch link: https://lore.kernel.org/r/20231120212037.911774-10-peter.griffin%40linaro.org patch subject: [PATCH v4 09/19] dt-bindings: serial: samsung: Make samsung,uart-fifosize required property compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20231121/202311211435.CJVOACBE-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311211435.CJVOACBE-lkp@intel.com/ dtcheck warnings: (new ones prefixed by >>) >> Documentation/devicetree/bindings/serial/samsung_uart.yaml:141:8: [warning] wrong indentation: expected 8 but found 7 (indentation) vim +141 Documentation/devicetree/bindings/serial/samsung_uart.yaml 8 9 maintainers: 10 - Krzysztof Kozlowski <krzk@kernel.org> 11 - Greg Kroah-Hartman <gregkh@linuxfoundation.org> 12 13 description: |+ 14 Each Samsung UART should have an alias correctly numbered in the "aliases" 15 node, according to serialN format, where N is the port number (non-negative 16 decimal integer) as specified by User's Manual of respective SoC. 17 18 properties: 19 compatible: 20 oneOf: 21 - items: 22 - const: samsung,exynosautov9-uart 23 - const: samsung,exynos850-uart 24 - enum: 25 - apple,s5l-uart 26 - axis,artpec8-uart 27 - google,gs101-uart 28 - samsung,s3c6400-uart 29 - samsung,s5pv210-uart 30 - samsung,exynos4210-uart 31 - samsung,exynos5433-uart 32 - samsung,exynos850-uart 33 34 reg: 35 maxItems: 1 36 37 reg-io-width: 38 description: | 39 The size (in bytes) of the IO accesses that should be performed 40 on the device. 41 enum: [ 1, 4 ] 42 43 clocks: 44 minItems: 2 45 maxItems: 5 46 47 clock-names: 48 description: N = 0 is allowed for SoCs without internal baud clock mux. 49 minItems: 2 50 items: 51 - const: uart 52 - pattern: '^clk_uart_baud[0-3]$' 53 - pattern: '^clk_uart_baud[0-3]$' 54 - pattern: '^clk_uart_baud[0-3]$' 55 - pattern: '^clk_uart_baud[0-3]$' 56 57 dmas: 58 items: 59 - description: DMA controller phandle and request line for RX 60 - description: DMA controller phandle and request line for TX 61 62 dma-names: 63 items: 64 - const: rx 65 - const: tx 66 67 interrupts: 68 description: RX interrupt and optionally TX interrupt. 69 minItems: 1 70 maxItems: 2 71 72 power-domains: 73 maxItems: 1 74 75 samsung,uart-fifosize: 76 description: The fifo size supported by the UART channel. 77 $ref: /schemas/types.yaml#/definitions/uint32 78 enum: [16, 64, 256] 79 80 required: 81 - compatible 82 - clocks 83 - clock-names 84 - interrupts 85 - reg 86 87 allOf: 88 - $ref: serial.yaml# 89 90 - if: 91 properties: 92 compatible: 93 contains: 94 enum: 95 - samsung,s5pv210-uart 96 then: 97 properties: 98 clocks: 99 minItems: 2 100 maxItems: 3 101 clock-names: 102 minItems: 2 103 items: 104 - const: uart 105 - pattern: '^clk_uart_baud[0-1]$' 106 - pattern: '^clk_uart_baud[0-1]$' 107 108 - if: 109 properties: 110 compatible: 111 contains: 112 enum: 113 - apple,s5l-uart 114 - axis,artpec8-uart 115 - samsung,exynos4210-uart 116 - samsung,exynos5433-uart 117 then: 118 properties: 119 clocks: 120 maxItems: 2 121 clock-names: 122 items: 123 - const: uart 124 - const: clk_uart_baud0 125 126 - if: 127 properties: 128 compatible: 129 contains: 130 enum: 131 - google,gs101-uart 132 - samsung,exynosautov9-uart 133 then: 134 properties: 135 samsung,uart-fifosize: 136 description: The fifo size supported by the UART channel. 137 $ref: /schemas/types.yaml#/definitions/uint32 138 enum: [16, 64, 256] 139 140 required: > 141 - samsung,uart-fifosize 142
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(+)