diff mbox series

[17/18] tty: serial: samsung: shrink port feature flags to u8

Message ID 20240110102102.61587-18-tudor.ambarus@linaro.org
State New
Headers show
Series serial: samsung: gs101 updates and winter cleanup | expand

Commit Message

Tudor Ambarus Jan. 10, 2024, 10:21 a.m. UTC
There's a single flag defined as of now. Shrink the feature flags to u8
and aim for a better memory footprint for ``struct s3c24xx_uart_info``.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tudor Ambarus Jan. 19, 2024, 8:56 a.m. UTC | #1
On 1/16/24 19:03, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> There's a single flag defined as of now. Shrink the feature flags to u8
>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/tty/serial/samsung_tty.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index 5df2bcebf9fb..598d9fe7a492 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
>>
>>         /* uart port features */
>>
>> -       unsigned int            has_divslot:1;
>> +       u8                      has_divslot:1;
> 
> But that's already a bit field. Why does it matter which type it is?
> 

If using unsigned int the bitfied is combined with the previous u8
fields, whereas if using u8 the bitfield will be independently defined.
So no benefit in terms of memory footprint, it's just a cosmetic change
to align the bitfield with the previous u8 fields. Allowing u32 for just
a bit can be misleading as one would ask itself where are the other
bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
compromise.

I'll update the commit message with this explanation in v2 because I'd
keep the patch, it makes the struct look cleaner. At the same time I
don't have a strong opinion, so if you'd like to see the patch dropped,
tell there, I'll be fine with it.
Jiri Slaby Jan. 19, 2024, 9:07 a.m. UTC | #2
On 19. 01. 24, 9:56, Tudor Ambarus wrote:
> 
> 
> On 1/16/24 19:03, Sam Protsenko wrote:
>> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>> There's a single flag defined as of now. Shrink the feature flags to u8
>>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>> ---
>>>   drivers/tty/serial/samsung_tty.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>>> index 5df2bcebf9fb..598d9fe7a492 100644
>>> --- a/drivers/tty/serial/samsung_tty.c
>>> +++ b/drivers/tty/serial/samsung_tty.c
>>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
>>>
>>>          /* uart port features */
>>>
>>> -       unsigned int            has_divslot:1;
>>> +       u8                      has_divslot:1;
>>
>> But that's already a bit field. Why does it matter which type it is?
>>
> 
> If using unsigned int the bitfied is combined with the previous u8
> fields, whereas if using u8 the bitfield will be independently defined.
> So no benefit in terms of memory footprint, it's just a cosmetic change
> to align the bitfield with the previous u8 fields. Allowing u32 for just
> a bit can be misleading as one would ask itself where are the other
> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
> compromise.

Why? What's wrong with bool? bitfields have terrible semantics wrt 
atomic writes for example.
Tudor Ambarus Jan. 19, 2024, 9:43 a.m. UTC | #3
On 1/19/24 09:07, Jiri Slaby wrote:

Hi, Jiri!

> On 19. 01. 24, 9:56, Tudor Ambarus wrote:
>>
>>
>> On 1/16/24 19:03, Sam Protsenko wrote:
>>> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus
>>> <tudor.ambarus@linaro.org> wrote:
>>>>
>>>> There's a single flag defined as of now. Shrink the feature flags to u8
>>>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>> ---
>>>>   drivers/tty/serial/samsung_tty.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/samsung_tty.c
>>>> b/drivers/tty/serial/samsung_tty.c
>>>> index 5df2bcebf9fb..598d9fe7a492 100644
>>>> --- a/drivers/tty/serial/samsung_tty.c
>>>> +++ b/drivers/tty/serial/samsung_tty.c
>>>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
>>>>
>>>>          /* uart port features */
>>>>
>>>> -       unsigned int            has_divslot:1;
>>>> +       u8                      has_divslot:1;
>>>
>>> But that's already a bit field. Why does it matter which type it is?
>>>
>>
>> If using unsigned int the bitfied is combined with the previous u8
>> fields, whereas if using u8 the bitfield will be independently defined.
>> So no benefit in terms of memory footprint, it's just a cosmetic change
>> to align the bitfield with the previous u8 fields. Allowing u32 for just
>> a bit can be misleading as one would ask itself where are the other
>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
>> compromise.
> 
> Why? What's wrong with bool? bitfields have terrible semantics wrt
> atomic writes for example.
> 

Bool occupies a byte and if more port features will ever be added we'll
occupy more bytes. Here's how the structure will look like with a bool:

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 */
	bool                       has_divslot;          /*    59     1 */

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

What's your preference?
Thanks,
ta
Jiri Slaby Jan. 19, 2024, 9:54 a.m. UTC | #4
Hi,

On 19. 01. 24, 10:43, Tudor Ambarus wrote:
>>> If using unsigned int the bitfied is combined with the previous u8
>>> fields, whereas if using u8 the bitfield will be independently defined.
>>> So no benefit in terms of memory footprint, it's just a cosmetic change
>>> to align the bitfield with the previous u8 fields. Allowing u32 for just
>>> a bit can be misleading as one would ask itself where are the other
>>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
>>> compromise.
>>
>> Why? What's wrong with bool? bitfields have terrible semantics wrt
>> atomic writes for example.
>>
> 
> Bool occupies a byte and if more port features will ever be added we'll
> occupy more bytes. Here's how the structure will look like with a bool:
> 
> 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 */
> 	bool                       has_divslot;          /*    59     1 */
> 
> 	/* size: 64, cachelines: 1, members: 17 */
> 	/* padding: 4 */
> };
> 
> What's your preference?

bool :).
Tudor Ambarus Jan. 19, 2024, 10:02 a.m. UTC | #5
On 1/19/24 09:54, Jiri Slaby wrote:
> Hi,
> 
> On 19. 01. 24, 10:43, Tudor Ambarus wrote:
>>>> If using unsigned int the bitfied is combined with the previous u8
>>>> fields, whereas if using u8 the bitfield will be independently defined.
>>>> So no benefit in terms of memory footprint, it's just a cosmetic change
>>>> to align the bitfield with the previous u8 fields. Allowing u32 for
>>>> just
>>>> a bit can be misleading as one would ask itself where are the other
>>>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
>>>> compromise.
>>>
>>> Why? What's wrong with bool? bitfields have terrible semantics wrt
>>> atomic writes for example.
>>>
>>
>> Bool occupies a byte and if more port features will ever be added we'll
>> occupy more bytes. Here's how the structure will look like with a bool:
>>
>> 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 */
>>     bool                       has_divslot;          /*    59     1 */
>>
>>     /* size: 64, cachelines: 1, members: 17 */
>>     /* padding: 4 */
>> };
>>
>> What's your preference?
> 
> bool :).
> 
I'm fine with a bool too as since the introduction of this driver we
have just this flag, it's unlikey to have 4 more soon to bypass the
first cacheline. Will change to bool.

Cheers,
ta
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 5df2bcebf9fb..598d9fe7a492 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -90,7 +90,7 @@  struct s3c24xx_uart_info {
 
 	/* uart port features */
 
-	unsigned int		has_divslot:1;
+	u8			has_divslot:1;
 };
 
 struct s3c24xx_serial_drv_data {