| Message ID | 20210730144922.29111-7-semen.protsenko@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add minimal support for Exynos850 SoC | expand |
On Fri, Jul 30, 2021 at 5:50 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Add serial driver data for Exynos850 SoC. This driver data is basically > reusing EXYNOS_COMMON_SERIAL_DRV_DATA, which is common for all Exynos > chips, but also enables USI init, which was added in previous commit: > "tty: serial: samsung: Init USI to keep clocks running". ... > +static struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { > + EXYNOS_COMMON_SERIAL_DRV_DATA_USI(1), > + .fifosize = { 0, }, 0 is the default for static globally defined variables. > +};
On 30/07/2021 16:49, Sam Protsenko wrote: > Add serial driver data for Exynos850 SoC. This driver data is basically > reusing EXYNOS_COMMON_SERIAL_DRV_DATA, which is common for all Exynos > chips, but also enables USI init, which was added in previous commit: > "tty: serial: samsung: Init USI to keep clocks running". > > Signed-off-by: Sam Protsenko <semen.protsenko@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 75ccbb08df4a..d059b516a0f4 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -2814,11 +2814,19 @@ static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = { > .fifosize = { 64, 256, 16, 256 }, > }; > > +static struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { > + EXYNOS_COMMON_SERIAL_DRV_DATA_USI(1), > + .fifosize = { 0, }, This does not look correct. You rely on samsung,uart-fifosize property but it is optional. Best regards, Krzysztof
On Fri, 30 Jul 2021 at 19:05, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 30/07/2021 16:49, Sam Protsenko wrote: > > Add serial driver data for Exynos850 SoC. This driver data is basically > > reusing EXYNOS_COMMON_SERIAL_DRV_DATA, which is common for all Exynos > > chips, but also enables USI init, which was added in previous commit: > > "tty: serial: samsung: Init USI to keep clocks running". > > > > Signed-off-by: Sam Protsenko <semen.protsenko@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 75ccbb08df4a..d059b516a0f4 100644 > > --- a/drivers/tty/serial/samsung_tty.c > > +++ b/drivers/tty/serial/samsung_tty.c > > @@ -2814,11 +2814,19 @@ static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = { > > .fifosize = { 64, 256, 16, 256 }, > > }; > > > > +static struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { > > + EXYNOS_COMMON_SERIAL_DRV_DATA_USI(1), > > + .fifosize = { 0, }, > > This does not look correct. You rely on samsung,uart-fifosize property > but it is optional. > Good point. I will replace fifosize elements (in patch series v2) with this code (the reasoning is below): .fifosize = { 256, 64, 64, 64 } TRM mentions that USI block has configurable FIFO of 16/32/64/128/256 byte. In vendor kernel they are setting default values in dtsi instead of driver, that's where fifosize = { 0 } appeared from. And in vendor dtsi they set 256 for serial_0 (USI UART instance), 64 for serial_1 (CMGP0 UART instance) and 64 for serial_2 (CMGP1 UART instance). I tested 256 and 64 for serial_0 (which is used for serial console) As for fifosize array elements count: though it's possible to configure up to 7 UARTs in Exynos850 (it has 5 USI blocks and 2 CMGP blocks, which can be configured as USIs), in a regular case it's only 3 UARTs (1 in USI and 2 in CMGP). This is how it's done in vendor's device tree, and I doubt someone is going to need more than 3 serials anyway, looks like very specific case for a mobile SoC. But CONFIG_SERIAL_SAMSUNG_UARTS_4=y is set by default when using arm64 defconfig, and I'd like to keep minimal delta for this defconfig for now. Hope you are ok with this? Thanks! > > Best regards, > Krzysztof
On 31/07/2021 01:10, Sam Protsenko wrote: > On Fri, 30 Jul 2021 at 19:05, Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> >> On 30/07/2021 16:49, Sam Protsenko wrote: >>> Add serial driver data for Exynos850 SoC. This driver data is basically >>> reusing EXYNOS_COMMON_SERIAL_DRV_DATA, which is common for all Exynos >>> chips, but also enables USI init, which was added in previous commit: >>> "tty: serial: samsung: Init USI to keep clocks running". >>> >>> Signed-off-by: Sam Protsenko <semen.protsenko@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 75ccbb08df4a..d059b516a0f4 100644 >>> --- a/drivers/tty/serial/samsung_tty.c >>> +++ b/drivers/tty/serial/samsung_tty.c >>> @@ -2814,11 +2814,19 @@ static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = { >>> .fifosize = { 64, 256, 16, 256 }, >>> }; >>> >>> +static struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { >>> + EXYNOS_COMMON_SERIAL_DRV_DATA_USI(1), >>> + .fifosize = { 0, }, >> >> This does not look correct. You rely on samsung,uart-fifosize property >> but it is optional. >> > > Good point. I will replace fifosize elements (in patch series v2) with > this code (the reasoning is below): > > .fifosize = { 256, 64, 64, 64 } > > TRM mentions that USI block has configurable FIFO of 16/32/64/128/256 > byte. In vendor kernel they are setting default values in dtsi instead > of driver, that's where fifosize = { 0 } appeared from. And in vendor > dtsi they set 256 for serial_0 (USI UART instance), 64 for serial_1 > (CMGP0 UART instance) and 64 for serial_2 (CMGP1 UART instance). I > tested 256 and 64 for serial_0 (which is used for serial console) > > As for fifosize array elements count: though it's possible to > configure up to 7 UARTs in Exynos850 (it has 5 USI blocks and 2 CMGP > blocks, which can be configured as USIs), in a regular case it's only > 3 UARTs (1 in USI and 2 in CMGP). This is how it's done in vendor's > device tree, and I doubt someone is going to need more than 3 serials > anyway, looks like very specific case for a mobile SoC. But > CONFIG_SERIAL_SAMSUNG_UARTS_4=y is set by default when using arm64 > defconfig, and I'd like to keep minimal delta for this defconfig for > now. > > Hope you are ok with this? > Yes, sounds good. Best regards, Krzysztof
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 75ccbb08df4a..d059b516a0f4 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -2814,11 +2814,19 @@ static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = { .fifosize = { 64, 256, 16, 256 }, }; +static struct s3c24xx_serial_drv_data exynos850_serial_drv_data = { + EXYNOS_COMMON_SERIAL_DRV_DATA_USI(1), + .fifosize = { 0, }, +}; + #define EXYNOS4210_SERIAL_DRV_DATA ((kernel_ulong_t)&exynos4210_serial_drv_data) #define EXYNOS5433_SERIAL_DRV_DATA ((kernel_ulong_t)&exynos5433_serial_drv_data) +#define EXYNOS850_SERIAL_DRV_DATA ((kernel_ulong_t)&exynos850_serial_drv_data) + #else #define EXYNOS4210_SERIAL_DRV_DATA ((kernel_ulong_t)NULL) #define EXYNOS5433_SERIAL_DRV_DATA ((kernel_ulong_t)NULL) +#define EXYNOS850_SERIAL_DRV_DATA ((kernel_ulong_t)NULL) #endif #ifdef CONFIG_ARCH_APPLE @@ -2874,6 +2882,9 @@ static const struct platform_device_id s3c24xx_serial_driver_ids[] = { }, { .name = "s5l-uart", .driver_data = S5L_SERIAL_DRV_DATA, + }, { + .name = "exynos850-uart", + .driver_data = EXYNOS850_SERIAL_DRV_DATA, }, { }, }; @@ -2897,6 +2908,8 @@ static const struct of_device_id s3c24xx_uart_dt_match[] = { .data = (void *)EXYNOS5433_SERIAL_DRV_DATA }, { .compatible = "apple,s5l-uart", .data = (void *)S5L_SERIAL_DRV_DATA }, + { .compatible = "samsung,exynos850-uart", + .data = (void *)EXYNOS850_SERIAL_DRV_DATA }, {}, }; MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
Add serial driver data for Exynos850 SoC. This driver data is basically reusing EXYNOS_COMMON_SERIAL_DRV_DATA, which is common for all Exynos chips, but also enables USI init, which was added in previous commit: "tty: serial: samsung: Init USI to keep clocks running". Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/tty/serial/samsung_tty.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) -- 2.30.2