| Message ID | 20231228125805.661725-5-tudor.ambarus@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GS101 Oriole: CMU_PERIC0 support and USI updates | expand |
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
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
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
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 --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,
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(-)