diff mbox series

[06/12] tty: serial: samsung: Add Exynos850 SoC data

Message ID 20210730144922.29111-7-semen.protsenko@linaro.org
State Superseded
Headers show
Series Add minimal support for Exynos850 SoC | expand

Commit Message

Sam Protsenko July 30, 2021, 2:49 p.m. UTC
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

Comments

Andy Shevchenko July 30, 2021, 3:05 p.m. UTC | #1
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.

> +};
Krzysztof Kozlowski July 30, 2021, 4:05 p.m. UTC | #2
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
Sam Protsenko July 30, 2021, 11:10 p.m. UTC | #3
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
Krzysztof Kozlowski July 31, 2021, 7:12 a.m. UTC | #4
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 mbox series

Patch

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);