Received-SPF: pass (google.com: domain of linux-kernel+bounces-17740-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <19746c85-6eff-4f63-9370-9592ad73f22c@linaro.org>
Date: Fri, 5 Jan 2024 10:22:46 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 04/12] tty: serial: samsung: prepare for different IO
 types
Content-Language: en-US
To: Greg KH <gregkh@linuxfoundation.org>
Cc: peter.griffin@linaro.org, robh+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, mturquette@baylibre.com,
 sboyd@kernel.org, conor+dt@kernel.org, andi.shyti@kernel.org,
 alim.akhtar@samsung.com, jirislaby@kernel.org, s.nawrocki@samsung.com,
 tomasz.figa@gmail.com, cw00.choi@samsung.com, arnd@arndb.de,
 semen.protsenko@linaro.org, andre.draszik@linaro.org, saravanak@google.com,
 willmcvicker@google.com, linux-arm-kernel@lists.infradead.org,
 linux-samsung-soc@vger.kernel.org, linux-clk@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org,
 kernel-team@android.com
References: <20231228125805.661725-1-tudor.ambarus@linaro.org>
 <20231228125805.661725-5-tudor.ambarus@linaro.org>
 <2024010432-taco-moneyless-53e2@gregkh>
 <a3a9df6a-4270-4076-9e9b-ce2fc7284d54@linaro.org>
 <2024010450-heritage-variety-d72d@gregkh>
From: Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <2024010450-heritage-variety-d72d@gregkh>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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