Re: [PATCH v2 08/20] dt-bindings: serial: samsung: Add google-gs101-uart compatible

Peter Griffin
2023-10-11T11:55:38Z

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.