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

[Tudor Ambarus]

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