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@xxxxxxxxxx>
>>>> ---
>>>> 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
