Hi Krzysztof, On Wed, 22 Nov 2023 at 07:49, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > >>> --- > >>> .../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
