| Message ID | 20231011184823.443959-17-peter.griffin@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand |
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(+)