diff mbox series

[v2,04/12] tty: serial: samsung: prepare for different IO types

Message ID 20231228125805.661725-5-tudor.ambarus@linaro.org
State Superseded
Headers show
Series GS101 Oriole: CMU_PERIC0 support and USI updates | expand

Commit Message

Tudor Ambarus Dec. 28, 2023, 12:57 p.m. UTC
GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
include the I3C and USI (I2C, SPI, UART) only allow 32-bit
register accesses. If using 8-bit register accesses, a SError
Interrupt is raised causing the system unusable.

Instead of specifying the reg-io-width = 4 everywhere, for each node,
the requirement should be deduced from the compatible.

Prepare the samsung tty driver to allow IO types different than
UPIO_MEM. ``struct uart_port::iotype`` is an unsigned char where all
its 8 bits are exposed to uapi. We can't make NULL checks on it to
verify if it's set, thus always set it from the driver's data.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
v2: new patch

 drivers/tty/serial/samsung_tty.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Greg KH Jan. 4, 2024, 3:32 p.m. UTC | #1
On Thu, Dec 28, 2023 at 12:57:57PM +0000, Tudor Ambarus wrote:
> GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
> include the I3C and USI (I2C, SPI, UART) only allow 32-bit
> register accesses. If using 8-bit register accesses, a SError
> Interrupt is raised causing the system unusable.
> 
> Instead of specifying the reg-io-width = 4 everywhere, for each node,
> the requirement should be deduced from the compatible.
> 
> Prepare the samsung tty driver to allow IO types different than
> UPIO_MEM. ``struct uart_port::iotype`` is an unsigned char where all
> its 8 bits are exposed to uapi. We can't make NULL checks on it to
> verify if it's set, thus always set it from the driver's data.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
> v2: new patch
> 
>  drivers/tty/serial/samsung_tty.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 66bd6c090ace..97ce4b2424af 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -72,6 +72,7 @@ struct s3c24xx_uart_info {
>  	const char		*name;
>  	enum s3c24xx_port_type	type;
>  	unsigned int		port_type;
> +	unsigned char		iotype;
>  	unsigned int		fifosize;
>  	unsigned long		rx_fifomask;
>  	unsigned long		rx_fifoshift;

Is there a reason you are trying to add unused memory spaces to this
structure for no valid reason?  I don't think you could have picked a
more incorrect place in there to add this :)

Please fix.

thanks,

greg k-h
Tudor Ambarus Jan. 4, 2024, 3:41 p.m. UTC | #2
On 1/4/24 15:32, Greg KH wrote:
> On Thu, Dec 28, 2023 at 12:57:57PM +0000, Tudor Ambarus wrote:
>> GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
>> include the I3C and USI (I2C, SPI, UART) only allow 32-bit
>> register accesses. If using 8-bit register accesses, a SError
>> Interrupt is raised causing the system unusable.
>>
>> Instead of specifying the reg-io-width = 4 everywhere, for each node,
>> the requirement should be deduced from the compatible.
>>
>> Prepare the samsung tty driver to allow IO types different than
>> UPIO_MEM. ``struct uart_port::iotype`` is an unsigned char where all
>> its 8 bits are exposed to uapi. We can't make NULL checks on it to
>> verify if it's set, thus always set it from the driver's data.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>> v2: new patch
>>
>>  drivers/tty/serial/samsung_tty.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index 66bd6c090ace..97ce4b2424af 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -72,6 +72,7 @@ struct s3c24xx_uart_info {
>>  	const char		*name;
>>  	enum s3c24xx_port_type	type;
>>  	unsigned int		port_type;
>> +	unsigned char		iotype;
>>  	unsigned int		fifosize;
>>  	unsigned long		rx_fifomask;
>>  	unsigned long		rx_fifoshift;
> 
> Is there a reason you are trying to add unused memory spaces to this
> structure for no valid reason?  I don't think you could have picked a
> more incorrect place in there to add this :)
> 
> Please fix.
> 

Will put it after "const char *name".
Thanks,
ta
Greg KH Jan. 4, 2024, 3:56 p.m. UTC | #3
On Thu, Jan 04, 2024 at 03:41:28PM +0000, Tudor Ambarus wrote:
> 
> 
> On 1/4/24 15:32, Greg KH wrote:
> > On Thu, Dec 28, 2023 at 12:57:57PM +0000, Tudor Ambarus wrote:
> >> GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
> >> include the I3C and USI (I2C, SPI, UART) only allow 32-bit
> >> register accesses. If using 8-bit register accesses, a SError
> >> Interrupt is raised causing the system unusable.
> >>
> >> Instead of specifying the reg-io-width = 4 everywhere, for each node,
> >> the requirement should be deduced from the compatible.
> >>
> >> Prepare the samsung tty driver to allow IO types different than
> >> UPIO_MEM. ``struct uart_port::iotype`` is an unsigned char where all
> >> its 8 bits are exposed to uapi. We can't make NULL checks on it to
> >> verify if it's set, thus always set it from the driver's data.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >> v2: new patch
> >>
> >>  drivers/tty/serial/samsung_tty.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> >> index 66bd6c090ace..97ce4b2424af 100644
> >> --- a/drivers/tty/serial/samsung_tty.c
> >> +++ b/drivers/tty/serial/samsung_tty.c
> >> @@ -72,6 +72,7 @@ struct s3c24xx_uart_info {
> >>  	const char		*name;
> >>  	enum s3c24xx_port_type	type;
> >>  	unsigned int		port_type;
> >> +	unsigned char		iotype;
> >>  	unsigned int		fifosize;
> >>  	unsigned long		rx_fifomask;
> >>  	unsigned long		rx_fifoshift;
> > 
> > Is there a reason you are trying to add unused memory spaces to this
> > structure for no valid reason?  I don't think you could have picked a
> > more incorrect place in there to add this :)
> > 
> > Please fix.
> > 
> 
> Will put it after "const char *name".

If you do, spend some time with the tool, pahole, and see if that's
really the best place for it or not.  Might be, might not be, but you
should verify it please.

thanks,

greg k-h
Tudor Ambarus Jan. 5, 2024, 10:22 a.m. UTC | #4
On 1/4/24 15:56, Greg KH wrote:
> On Thu, Jan 04, 2024 at 03:41:28PM +0000, Tudor Ambarus wrote:
>>
>>
>> On 1/4/24 15:32, Greg KH wrote:
>>> On Thu, Dec 28, 2023 at 12:57:57PM +0000, Tudor Ambarus wrote:
>>>> GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
>>>> include the I3C and USI (I2C, SPI, UART) only allow 32-bit
>>>> register accesses. If using 8-bit register accesses, a SError
>>>> Interrupt is raised causing the system unusable.
>>>>
>>>> Instead of specifying the reg-io-width = 4 everywhere, for each node,
>>>> the requirement should be deduced from the compatible.
>>>>
>>>> Prepare the samsung tty driver to allow IO types different than
>>>> UPIO_MEM. ``struct uart_port::iotype`` is an unsigned char where all
>>>> its 8 bits are exposed to uapi. We can't make NULL checks on it to
>>>> verify if it's set, thus always set it from the driver's data.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>> ---
>>>> v2: new patch
>>>>
>>>>  drivers/tty/serial/samsung_tty.c | 9 ++++++++-
>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>>>> index 66bd6c090ace..97ce4b2424af 100644
>>>> --- a/drivers/tty/serial/samsung_tty.c
>>>> +++ b/drivers/tty/serial/samsung_tty.c
>>>> @@ -72,6 +72,7 @@ struct s3c24xx_uart_info {
>>>>  	const char		*name;
>>>>  	enum s3c24xx_port_type	type;
>>>>  	unsigned int		port_type;
>>>> +	unsigned char		iotype;
>>>>  	unsigned int		fifosize;
>>>>  	unsigned long		rx_fifomask;
>>>>  	unsigned long		rx_fifoshift;
>>>
>>> Is there a reason you are trying to add unused memory spaces to this
>>> structure for no valid reason?  I don't think you could have picked a
>>> more incorrect place in there to add this :)
>>>
>>> Please fix.
>>>
>>
>> Will put it after "const char *name".
> 
> If you do, spend some time with the tool, pahole, and see if that's
> really the best place for it or not.  Might be, might not be, but you
> should verify it please.
> 

Thanks!

I played with pahole a bit. For arm32 this struct is not as bad defined
as for arm64, all members fit in the same cacheline. There are some
holes though and 2 cachelines for arm64 where this struct needs some
love. The best and minimum invasive change for my iotype member would be
to put it before the "has_divslot" member, as the has_divslot bitfield
will be combined with the previous field.

But I think the entire struct has to be reworked and the driver
butchered a bit so that we get to a better memory footprint and a single
cacheline. I volunteer to do this in a separate patch set so that we
don't block this series. I think the final struct can look as following:

struct s3c24xx_uart_info {
	const char  *              name;                 /*     0     8 */
	enum s3c24xx_port_type     type;                 /*     8     4 */
	unsigned int               port_type;            /*    12     4 */
	unsigned int               fifosize;             /*    16     4 */
	u32                        rx_fifomask;          /*    20     4 */
	u32                        rx_fifoshift;         /*    24     4 */
	u32                        rx_fifofull;          /*    28     4 */
	u32                        tx_fifomask;          /*    32     4 */
	u32                        tx_fifoshift;         /*    36     4 */
	u32                        tx_fifofull;          /*    40     4 */
	u32                        clksel_mask;          /*    44     4 */
	u32                        clksel_shift;         /*    48     4 */
	u32                        ucon_mask;            /*    52     4 */
	u8                         def_clk_sel;          /*    56     1 */
	u8                         num_clks;             /*    57     1 */
	u8                         iotype;               /*    58     1 */
	u8                         has_divslot:1;        /*    59: 0  1 */

	/* size: 64, cachelines: 1, members: 17 */
	/* padding: 4 */
	/* bit_padding: 7 bits */
};


This looks a lot better than what we have now:
	/* size: 120, cachelines: 2, members: 17 */
	/* sum members: 105, holes: 2, sum holes: 8 */
	/* sum bitfield members: 1 bits (0 bytes) */
	/* padding: 4 */
	/* bit_padding: 23 bits */
	/* last cacheline: 56 bytes */

I'll put iotype before has_divslot and then follow up with a patch set
to clean the driver. Cheers,
ta
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 66bd6c090ace..97ce4b2424af 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -72,6 +72,7 @@  struct s3c24xx_uart_info {
 	const char		*name;
 	enum s3c24xx_port_type	type;
 	unsigned int		port_type;
+	unsigned char		iotype;
 	unsigned int		fifosize;
 	unsigned long		rx_fifomask;
 	unsigned long		rx_fifoshift;
@@ -1742,7 +1743,6 @@  static void s3c24xx_serial_init_port_default(int index) {
 
 	spin_lock_init(&port->lock);
 
-	port->iotype = UPIO_MEM;
 	port->uartclk = 0;
 	port->fifosize = 16;
 	port->flags = UPF_BOOT_AUTOCONF;
@@ -1989,6 +1989,8 @@  static int s3c24xx_serial_probe(struct platform_device *pdev)
 		break;
 	}
 
+	ourport->port.iotype = ourport->info->iotype;
+
 	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
@@ -2401,6 +2403,7 @@  static const struct s3c24xx_serial_drv_data s3c6400_serial_drv_data = {
 		.name		= "Samsung S3C6400 UART",
 		.type		= TYPE_S3C6400,
 		.port_type	= PORT_S3C6400,
+		.iotype		= UPIO_MEM,
 		.fifosize	= 64,
 		.has_divslot	= 1,
 		.rx_fifomask	= S3C2440_UFSTAT_RXMASK,
@@ -2430,6 +2433,7 @@  static const struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
 		.name		= "Samsung S5PV210 UART",
 		.type		= TYPE_S3C6400,
 		.port_type	= PORT_S3C6400,
+		.iotype		= UPIO_MEM,
 		.has_divslot	= 1,
 		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,
 		.rx_fifoshift	= S5PV210_UFSTAT_RXSHIFT,
@@ -2459,6 +2463,7 @@  static const struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
 		.name		= "Samsung Exynos UART",	\
 		.type		= TYPE_S3C6400,			\
 		.port_type	= PORT_S3C6400,			\
+		.iotype		= UPIO_MEM,			\
 		.has_divslot	= 1,				\
 		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,	\
 		.rx_fifoshift	= S5PV210_UFSTAT_RXSHIFT,	\
@@ -2519,6 +2524,7 @@  static const struct s3c24xx_serial_drv_data s5l_serial_drv_data = {
 		.name		= "Apple S5L UART",
 		.type		= TYPE_APPLE_S5L,
 		.port_type	= PORT_8250,
+		.iotype		= UPIO_MEM,
 		.fifosize	= 16,
 		.rx_fifomask	= S3C2410_UFSTAT_RXMASK,
 		.rx_fifoshift	= S3C2410_UFSTAT_RXSHIFT,
@@ -2548,6 +2554,7 @@  static const struct s3c24xx_serial_drv_data artpec8_serial_drv_data = {
 		.name		= "Axis ARTPEC-8 UART",
 		.type		= TYPE_S3C6400,
 		.port_type	= PORT_S3C6400,
+		.iotype		= UPIO_MEM,
 		.fifosize	= 64,
 		.has_divslot	= 1,
 		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,