| Message ID | 20231011184823.443959-17-peter.griffin@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/20] dt-bindings: soc: samsung: exynos-pmu: Add gs101 compatible | expand |
On Wed, Oct 11, 2023, at 20:48, Peter Griffin wrote: > Add serial driver data for Google Tensor gs101 SoC. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> Reviewed-by: Arnd Bergmann <arnd@arndb.de> While the patch is now correct, I would point out a few improvements we could make on top: > +static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = { > + EXYNOS_COMMON_SERIAL_DRV_DATA(), > + /* rely on samsung,uart-fifosize DT property for fifosize */ > + .fifosize = { 0 }, > +}; > + > #define EXYNOS4210_SERIAL_DRV_DATA (&exynos4210_serial_drv_data) > #define EXYNOS5433_SERIAL_DRV_DATA (&exynos5433_serial_drv_data) > #define EXYNOS850_SERIAL_DRV_DATA (&exynos850_serial_drv_data) > +#define GS101_SERIAL_DRV_DATA (&gs101_serial_drv_data) Since this is now actually correct for any Exynos variant that has the FIFO size listed in the DT, we could use a variable/macro name that leads itself to being used by future chips. There is also the question of whether we want to address the ordering bug for the other SoC types. The way I understand it, the .fifosize array logic is wrong because it relies on having a particular alias for each of the ports to match the entry in the array. For the exynosautov9, this would be trivially fixed by using the same data as gs101 (since it already lists the correct size in DT), but for the other ones we'd need a different logic. > @@ -2688,6 +2696,9 @@ static const struct platform_device_id > s3c24xx_serial_driver_ids[] = { > }, { > .name = "artpec8-uart", > .driver_data = (kernel_ulong_t)ARTPEC8_SERIAL_DRV_DATA, > + }, { > + .name = "gs101-uart", > + .driver_data = (kernel_ulong_t)GS101_SERIAL_DRV_DATA, > }, > { }, > }; I just noticed that the platform_device_id array is currently only used for mach-crag6410, since everything else uses DT based probing. s3c64xx is scheduled for removal in early 2024 (though no patch has been sent), and we can probably just remove all the atags/platform_device based code when that happens. Arnd
On 11/10/2023 20:48, Peter Griffin wrote: > Add serial driver data for Google Tensor gs101 SoC. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > drivers/tty/serial/samsung_tty.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index 07fb8a9dac63..26bc52e681a4 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -2597,14 +2597,22 @@ static const struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { > .fifosize = { 256, 64, 64, 64 }, > }; > > +static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = { > + EXYNOS_COMMON_SERIAL_DRV_DATA(), > + /* rely on samsung,uart-fifosize DT property for fifosize */ It's an optional property, so you cannot rely on it. > + .fifosize = { 0 }, Looks like it is compatible with exynos850_serial_drv_data, so most likely it should be expressed as compatible in the bindings. Best regards, Krzysztof
Hi Krzysztof, On Thu, 12 Oct 2023 at 07:26, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 11/10/2023 20:48, Peter Griffin wrote: > > Add serial driver data for Google Tensor gs101 SoC. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > drivers/tty/serial/samsung_tty.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > > index 07fb8a9dac63..26bc52e681a4 100644 > > --- a/drivers/tty/serial/samsung_tty.c > > +++ b/drivers/tty/serial/samsung_tty.c > > @@ -2597,14 +2597,22 @@ static const struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { > > .fifosize = { 256, 64, 64, 64 }, > > }; > > > > +static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = { > > + EXYNOS_COMMON_SERIAL_DRV_DATA(), > > + /* rely on samsung,uart-fifosize DT property for fifosize */ > > It's an optional property, so you cannot rely on it. Is it possible to make it a mandatory DT property for certain SoCs? > > > + .fifosize = { 0 }, I can update this to 256, and we can add more sizes as we enable other UARTs I suppose. What's the purpose of having fifosize specified in DT and in *_serial_drv_data? Thanks, Peter
On 12/10/2023 16:03, Peter Griffin wrote: > Hi Krzysztof, > > On Thu, 12 Oct 2023 at 07:26, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 11/10/2023 20:48, Peter Griffin wrote: >>> Add serial driver data for Google Tensor gs101 SoC. >>> >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> >>> --- >>> drivers/tty/serial/samsung_tty.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >>> index 07fb8a9dac63..26bc52e681a4 100644 >>> --- a/drivers/tty/serial/samsung_tty.c >>> +++ b/drivers/tty/serial/samsung_tty.c >>> @@ -2597,14 +2597,22 @@ static const struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { >>> .fifosize = { 256, 64, 64, 64 }, >>> }; >>> >>> +static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = { >>> + EXYNOS_COMMON_SERIAL_DRV_DATA(), >>> + /* rely on samsung,uart-fifosize DT property for fifosize */ >> >> It's an optional property, so you cannot rely on it. > > Is it possible to make it a mandatory DT property for certain SoCs? Yes. > >> >>> + .fifosize = { 0 }, > > I can update this to 256, and we can add more sizes as we enable other > UARTs I suppose. You might also want to fix Arnd's comment, but anyway drivers must be in match with bindings. > > What's the purpose of having fifosize specified in DT and in *_serial_drv_data? I guess safe defaults? Best regards, Krzysztof
Hi Arnd, On Thu, 12 Oct 2023 at 07:07, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Oct 11, 2023, at 20:48, Peter Griffin wrote: > > Add serial driver data for Google Tensor gs101 SoC. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> Thanks! > > While the patch is now correct, I would point out a few > improvements we could make on top: > > > +static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = { > > + EXYNOS_COMMON_SERIAL_DRV_DATA(), > > + /* rely on samsung,uart-fifosize DT property for fifosize */ > > + .fifosize = { 0 }, > > +}; > > + > > #define EXYNOS4210_SERIAL_DRV_DATA (&exynos4210_serial_drv_data) > > #define EXYNOS5433_SERIAL_DRV_DATA (&exynos5433_serial_drv_data) > > #define EXYNOS850_SERIAL_DRV_DATA (&exynos850_serial_drv_data) > > +#define GS101_SERIAL_DRV_DATA (&gs101_serial_drv_data) > > Since this is now actually correct for any Exynos variant that > has the FIFO size listed in the DT, we could use a variable/macro > name that leads itself to being used by future chips. I've updated this to exynos_fifoszdt_serial_drv_data and EXYNOS_FIFOSZDT_SERIAL_DRV_DATA in v4 and added a comment that it is common struct for platforms that specify uart,fifosize in DT. I've also updated the YAML to make this a required property for google,gs101-uart. > > There is also the question of whether we want to address the > ordering bug for the other SoC types. The way I understand it, > the .fifosize array logic is wrong because it relies on having > a particular alias for each of the ports to match the entry in > the array. > For the exynosautov9, this would be trivially fixed > by using the same data as gs101 (since it already lists the > correct size in DT), but for the other ones we'd need a different > logic. > It seems samsung,exynosautov9-uart is in the yaml bindings and exynosautov9.dtsi but never actually made it into the driver. But it could be added to the driver and made to use the common exynos_fifoszdt_serial_drv_data mentioned above. I think any new platform should specify this in DT as many of these UARTs on newer Exynos are actually universal serial IPs which can be UART, I2C or SPI which is board dependent. So having the fifosize in the driver, based on a SoC compatible and relying on probe order and DT aliases seems very prone to error. regards, Peter. > > @@ -2688,6 +2696,9 @@ static const struct platform_device_id > > s3c24xx_serial_driver_ids[] = { > > }, { > > .name = "artpec8-uart", > > .driver_data = (kernel_ulong_t)ARTPEC8_SERIAL_DRV_DATA, > > + }, { > > + .name = "gs101-uart", > > + .driver_data = (kernel_ulong_t)GS101_SERIAL_DRV_DATA, > > }, > > { }, > > }; > > I just noticed that the platform_device_id array is currently > only used for mach-crag6410, since everything else uses DT > based probing. s3c64xx is scheduled for removal in early 2024 > (though no patch has been sent), and we can probably just > remove all the atags/platform_device based code when that happens. > > Arnd
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 07fb8a9dac63..26bc52e681a4 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -2597,14 +2597,22 @@ static const struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { .fifosize = { 256, 64, 64, 64 }, }; +static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = { + EXYNOS_COMMON_SERIAL_DRV_DATA(), + /* rely on samsung,uart-fifosize DT property for fifosize */ + .fifosize = { 0 }, +}; + #define EXYNOS4210_SERIAL_DRV_DATA (&exynos4210_serial_drv_data) #define EXYNOS5433_SERIAL_DRV_DATA (&exynos5433_serial_drv_data) #define EXYNOS850_SERIAL_DRV_DATA (&exynos850_serial_drv_data) +#define GS101_SERIAL_DRV_DATA (&gs101_serial_drv_data) #else #define EXYNOS4210_SERIAL_DRV_DATA NULL #define EXYNOS5433_SERIAL_DRV_DATA NULL #define EXYNOS850_SERIAL_DRV_DATA NULL +#define GS101_SERIAL_DRV_DATA NULL #endif #ifdef CONFIG_ARCH_APPLE @@ -2688,6 +2696,9 @@ static const struct platform_device_id s3c24xx_serial_driver_ids[] = { }, { .name = "artpec8-uart", .driver_data = (kernel_ulong_t)ARTPEC8_SERIAL_DRV_DATA, + }, { + .name = "gs101-uart", + .driver_data = (kernel_ulong_t)GS101_SERIAL_DRV_DATA, }, { }, }; @@ -2709,6 +2720,8 @@ static const struct of_device_id s3c24xx_uart_dt_match[] = { .data = EXYNOS850_SERIAL_DRV_DATA }, { .compatible = "axis,artpec8-uart", .data = ARTPEC8_SERIAL_DRV_DATA }, + { .compatible = "google,gs101-uart", + .data = GS101_SERIAL_DRV_DATA }, {}, }; MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
Add serial driver data for Google Tensor gs101 SoC. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- drivers/tty/serial/samsung_tty.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)