| Message ID | 20231010224928.2296997-9-peter.griffin@linaro.org |
|---|---|
| State | Superseded |
| 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/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hi, Greg, On 10/11/23 08:48, Greg KH wrote: > On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote: >> Add dedicated google-gs101-uart compatible to the dt-schema for >> representing uart of the Google Tensor gs101 SoC. >> >> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >> --- >> Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >> index 8bd88d5cbb11..72471ebe5734 100644 >> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml >> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >> @@ -19,11 +19,13 @@ properties: >> compatible: >> oneOf: >> - items: >> + - const: google,gs101-uart >> - const: samsung,exynosautov9-uart >> - const: samsung,exynos850-uart >> - enum: >> - apple,s5l-uart >> - axis,artpec8-uart >> + - google,gs101-uart > > These shouldn't be needed, just declare the device as the same as what We should have SoC specific compatibles so that any further quirks or incompatibilities can be easily addressed. It's not only the IP itself that can differ, it's also the integration of the IP into the final product that could have an influence on the behavior. Cheers, ta > the chip really is (i.e. a samsung uart), that way no .yaml or kernel > driver changes are needed at all. >
Hi Greg, Thanks for your review feedback! On Wed, 11 Oct 2023 at 08:48, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote: > > Add dedicated google-gs101-uart compatible to the dt-schema for > > representing uart of the Google Tensor gs101 SoC. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > index 8bd88d5cbb11..72471ebe5734 100644 > > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > @@ -19,11 +19,13 @@ properties: > > compatible: > > oneOf: > > - items: > > + - const: google,gs101-uart > > - const: samsung,exynosautov9-uart > > - const: samsung,exynos850-uart > > - enum: > > - apple,s5l-uart > > - axis,artpec8-uart > > + - google,gs101-uart > > These shouldn't be needed, just declare the device as the same as what > the chip really is (i.e. a samsung uart), that way no .yaml or kernel > driver changes are needed at all. What you describe is actually how I had it in the v1 submission, which is also similar to what exynosautov9.dtsi is doing by re-using the "samsung,exynos850-uart" compatible, and associated data in the driver. However the review feedback in v1 from Krzysztof and Tudor was to add a dedicated compatible for it. I guess I could have re-used the existing EXYNOS850_SERIAL_DRV_DATA structure though rather than duplicating that as well. I'll let Krzysztof comment on why a dedicated compatible is required. regards, Peter
On Wed, Oct 11, 2023, at 10:57, Greg KH wrote: > On Wed, Oct 11, 2023 at 09:49:07AM +0100, Tudor Ambarus wrote: >> On 10/11/23 08:48, Greg KH wrote: >> > On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote: >> >> Add dedicated google-gs101-uart compatible to the dt-schema for >> >> representing uart of the Google Tensor gs101 SoC. >> >> >> >> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >> >> --- >> >> Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> oneOf: >> >> - items: >> >> + - const: google,gs101-uart >> >> - const: samsung,exynosautov9-uart >> >> - const: samsung,exynos850-uart >> >> - enum: >> >> - apple,s5l-uart >> >> - axis,artpec8-uart >> >> + - google,gs101-uart >> > >> > These shouldn't be needed, just declare the device as the same as what >> >> We should have SoC specific compatibles so that any further quirks or >> incompatibilities can be easily addressed. > > "further" work on quirks or incompatibilities can be added when they are > found and needed. We don't add stuff for no good reason to the kernel. > >> It's not only the IP itself >> that can differ, it's also the integration of the IP into the final >> product that could have an influence on the behavior. > > This is for the Pixel 6, a device that is no longer even shipping. The > "final product" is long stable, so this should not be an issue. The driver does have soc specific settings for each compatible string, in this case it looks like it overrides the FIFO size based on driver specific data and the order in which the ports are probed [1]. I don't understand why the driver does this, but my impression is that if we wanted to change it to no longer rely on that data, we'd also need a new compatible string. Ideally, the actual compatible list in the DTB lists both the specific implementation (google,gs101-uart) in order to allow such hacks if needed, and a more generic string (e.g. "samsung,exynos850-uart" for an older device that is entirely compatible) in order to not actually need driver changes. Arnd [1] https://lore.kernel.org/linux-arm-kernel/20231010224928.2296997-17-peter.griffin@linaro.org/
On Wed, Oct 11, 2023, at 11:42, Greg Kroah-Hartman wrote: > On Wed, Oct 11, 2023 at 11:30:25AM +0200, Arnd Bergmann wrote: >> On Wed, Oct 11, 2023, at 10:57, Greg KH wrote: >> > >> >> It's not only the IP itself >> >> that can differ, it's also the integration of the IP into the final >> >> product that could have an influence on the behavior. >> > >> > This is for the Pixel 6, a device that is no longer even shipping. The >> > "final product" is long stable, so this should not be an issue. >> >> The driver does have soc specific settings for each compatible >> string, in this case it looks like it overrides the FIFO size >> based on driver specific data and the order in which the >> ports are probed [1]. I don't understand why the driver does >> this, but my impression is that if we wanted to change it to no >> longer rely on that data, we'd also need a new compatible >> string. > > As I reviewed that patch already, it is just duplicating an existing > quirk/device that the driver already supports, so there is no need for > any "new device type" to be added to that driver, just use the existing > hardware description in the dt and all should be fine. The thing is, I suspect that the FIFO size override is actually wrong for the exynos850 as well, and is almost certainly wrong for both exynosautov9 and google-gs101: - The driver overrides an exynos850 compatible uart to use a 256 byte FIFO on whichever port is probed first, 64 byte on the next three ports, and the setting from DT on any later ones, falling back to 16 bytes if the DT does not set anything. - exynos850 only actually has three of these ports, not four. It does not lists FIFO size in the dts at all. - exynosautov9 has a total of 11 ports, each of these compatible with both "samsung,exynosautov9-uart" as the specific value and "samsung,exynos850-uart" as the generic fallback. The DT lists a FIFO size of 256 bytes for ports 0, 1, and 6, but lists FIFO size 64 for each of the other ones. - google-gs101 only lists a single uart in the dts, and sets it to a 256 byte FIFO. - testla-fsd claims to be compatible with exynos4210, which also overrides the first two ports in probe order to 256 and 64 bytes respectively (like exynos850), but it only has two ports. - artpec8 has a separate compatible string so it overrides all ports to 64 bytes. I don't know why probe order would have anything to do with this, so most likely these are all the same thing and should just put a fixed FIFO size into the DT for each port instance. Arnd
Hi Arnd, Thanks for your feedback. On Wed, 11 Oct 2023 at 11:19, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Oct 11, 2023, at 11:42, Greg Kroah-Hartman wrote: > > On Wed, Oct 11, 2023 at 11:30:25AM +0200, Arnd Bergmann wrote: > >> On Wed, Oct 11, 2023, at 10:57, Greg KH wrote: > >> > > >> >> It's not only the IP itself > >> >> that can differ, it's also the integration of the IP into the final > >> >> product that could have an influence on the behavior. > >> > > >> > This is for the Pixel 6, a device that is no longer even shipping. The > >> > "final product" is long stable, so this should not be an issue. > >> > >> The driver does have soc specific settings for each compatible > >> string, in this case it looks like it overrides the FIFO size > >> based on driver specific data and the order in which the > >> ports are probed [1]. I don't understand why the driver does > >> this, but my impression is that if we wanted to change it to no > >> longer rely on that data, we'd also need a new compatible > >> string. > > > > As I reviewed that patch already, it is just duplicating an existing > > quirk/device that the driver already supports, so there is no need for > > any "new device type" to be added to that driver, just use the existing > > hardware description in the dt and all should be fine. > > The thing is, I suspect that the FIFO size override is actually > wrong for the exynos850 as well, and is almost certainly wrong > for both exynosautov9 and google-gs101: > > - The driver overrides an exynos850 compatible uart to use a > 256 byte FIFO on whichever port is probed first, 64 byte > on the next three ports, and the setting from DT on any > later ones, falling back to 16 bytes if the DT does not set > anything. > > - exynos850 only actually has three of these ports, not > four. It does not lists FIFO size in the dts at all. > > - exynosautov9 has a total of 11 ports, each of these > compatible with both "samsung,exynosautov9-uart" as > the specific value and "samsung,exynos850-uart" as > the generic fallback. The DT lists a FIFO size of 256 > bytes for ports 0, 1, and 6, but lists FIFO size 64 > for each of the other ones. > > - google-gs101 only lists a single uart in the dts, > and sets it to a 256 byte FIFO. > > - testla-fsd claims to be compatible with exynos4210, > which also overrides the first two ports in probe > order to 256 and 64 bytes respectively (like exynos850), > but it only has two ports. > > - artpec8 has a separate compatible string so it overrides > all ports to 64 bytes. > > I don't know why probe order would have anything to do > with this, so most likely these are all the same thing > and should just put a fixed FIFO size into the DT for > each port instance. I agree, I just looked again at gs101 and in total we can have 19 uarts on this SoC. 3 of them are 256byte rx/tx fifo and the other 16 have 64byte tx/rx fifo size, but this is a SoC design choice and has nothing to do with probe order. I will update in v3 to get fifo size from DT. regards, Peter.
On Wed, Oct 11, 2023 at 9:48 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > - enum: > > - apple,s5l-uart > > - axis,artpec8-uart > > + - google,gs101-uart > > These shouldn't be needed, just declare the device as the same as what > the chip really is (i.e. a samsung uart), that way no .yaml or kernel > driver changes are needed at all. We strive to have these as unique as possible, as it is a hardware description. It is fine to write drivers in Linux or any other OS just being aware of a "courser" idea of what UART this is, in this case would have looked something like this: compatible = "google,gs101-uart", "samsung-uart"; And the driver would be able to match to just the latter string (these are listed in "particularity order"). BUT! The binding authors chose not to go that path, instead they have one unique compatible string per hardware/integration version, essentially per-SoC. So in this case it is just: compatible = "google,gs101-uart"; It is kind of impossible to fix now as well, because these bindings are already deployed. So they are like a BIOS: written in stone. It is possible to add dual compatibles for this *and following* variants, but I don't know how Krzysztof feels about that, and as others point out, probably knowledge of the exact SoC is necessary. Yours, Linus Walleij
On 11/10/2023 10:57, Greg KH wrote: > On Wed, Oct 11, 2023 at 09:49:07AM +0100, Tudor Ambarus wrote: >> Hi, Greg, >> >> On 10/11/23 08:48, Greg KH wrote: >>> On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote: >>>> Add dedicated google-gs101-uart compatible to the dt-schema for >>>> representing uart of the Google Tensor gs101 SoC. >>>> >>>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >>>> --- >>>> Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>>> index 8bd88d5cbb11..72471ebe5734 100644 >>>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>>> @@ -19,11 +19,13 @@ properties: >>>> compatible: >>>> oneOf: >>>> - items: >>>> + - const: google,gs101-uart >>>> - const: samsung,exynosautov9-uart >>>> - const: samsung,exynos850-uart >>>> - enum: >>>> - apple,s5l-uart >>>> - axis,artpec8-uart >>>> + - google,gs101-uart >>> >>> These shouldn't be needed, just declare the device as the same as what >> >> We should have SoC specific compatibles so that any further quirks or >> incompatibilities can be easily addressed. > > "further" work on quirks or incompatibilities can be added when they are > found and needed. We don't add stuff for no good reason to the kernel. With a Devicetree bindings maintainer hat: We expect the device-specific compatible in all bindings, followed by fallback. The fallback is used by the driver, the device-specific for any future needs. This is the practice we follow everywhere and recommend everywhere since some time. It is also documented here: https://elixir.bootlin.com/linux/v6.6-rc5/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 Best regards, Krzysztof
On 11/10/2023 00:49, Peter Griffin wrote: > Add dedicated google-gs101-uart compatible to the dt-schema for > representing uart of the Google Tensor gs101 SoC. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > index 8bd88d5cbb11..72471ebe5734 100644 > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > @@ -19,11 +19,13 @@ properties: > compatible: > oneOf: > - items: > + - const: google,gs101-uart You just broke existing users. It does not look like you tested the DTS against bindings. Please run `make dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). Best regards, Krzysztof
Hi Krzysztof, Thanks for your review. On Wed, 11 Oct 2023 at 13:09, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 11/10/2023 00:49, Peter Griffin wrote: > > Add dedicated google-gs101-uart compatible to the dt-schema for > > representing uart of the Google Tensor gs101 SoC. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > index 8bd88d5cbb11..72471ebe5734 100644 > > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > @@ -19,11 +19,13 @@ properties: > > compatible: > > oneOf: > > - items: > > + - const: google,gs101-uart > > You just broke existing users. > > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check W=1` (see > Documentation/devicetree/bindings/writing-schema.rst or > https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > for instructions). > Will fix in v3 fyi I've been running with make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- dt_binding_check DT_SCHEMA_FILES=google make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- dt_binding_check DT_SCHEMA_FILES=samsung make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y W=1 google/gs101-oriole.dtb But clearly that wasn't enough to catch this. `make dtbs_check W=1` takes a long time and gives so much output. I suppose adding a few of the other exynos based boards should still be fairly quick and hopefully catch things like this. For example adding make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y W=1 exynos/exynos850-e850-96.dtb make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y W=1 exynos/exynos5433-tm2.dtb make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y W=1 exynos/exynosautov9-sadk.dtb regards, Peter.
On 11/10/2023 15:27, Peter Griffin wrote: > Hi Krzysztof, > > Thanks for your review. > > On Wed, 11 Oct 2023 at 13:09, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 11/10/2023 00:49, Peter Griffin wrote: >>> Add dedicated google-gs101-uart compatible to the dt-schema for >>> representing uart of the Google Tensor gs101 SoC. >>> >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >>> --- >>> Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> index 8bd88d5cbb11..72471ebe5734 100644 >>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml >>> @@ -19,11 +19,13 @@ properties: >>> compatible: >>> oneOf: >>> - items: >>> + - const: google,gs101-uart >> >> You just broke existing users. >> >> It does not look like you tested the DTS against bindings. Please run >> `make dtbs_check W=1` (see >> Documentation/devicetree/bindings/writing-schema.rst or >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ >> for instructions). >> > > Will fix in v3 > > fyi I've been running with > > make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- > dt_binding_check DT_SCHEMA_FILES=google > make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- > dt_binding_check DT_SCHEMA_FILES=samsung > make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y > W=1 google/gs101-oriole.dtb > > But clearly that wasn't enough to catch this. None of the commands above test existing DTS... >`make dtbs_check W=1` > takes a long time > and gives so much output. I suppose adding a few of the other exynos > based boards should > still be fairly quick and hopefully catch things like this. For example adding > > make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y > W=1 exynos/exynos850-e850-96.dtb > make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y > W=1 exynos/exynos5433-tm2.dtb > make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y > W=1 exynos/exynosautov9-sadk.dtb > > regards, > > Peter. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml index 8bd88d5cbb11..72471ebe5734 100644 --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml @@ -19,11 +19,13 @@ properties: compatible: oneOf: - items: + - const: google,gs101-uart - const: samsung,exynosautov9-uart - const: samsung,exynos850-uart - enum: - apple,s5l-uart - axis,artpec8-uart + - google,gs101-uart - samsung,s3c2410-uart - samsung,s3c2412-uart - samsung,s3c2440-uart
Add dedicated google-gs101-uart compatible to the dt-schema for representing uart of the Google Tensor gs101 SoC. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++ 1 file changed, 2 insertions(+)