<linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.
Update the driver to consider a pool selection of maximum 8 clocks. The
final scope is to reduce the memory footprint of
``struct s3c24xx_uart_info``.
Signed-off-by: Tudor Ambarus <[email protected]>
---
drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 436739cf9225..5df2bcebf9fb 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
unsigned long tx_fifomask;
unsigned long tx_fifoshift;
unsigned long tx_fifofull;
- unsigned int def_clk_sel;
- unsigned long num_clks;
unsigned long clksel_mask;
unsigned long clksel_shift;
unsigned long ucon_mask;
+ u8 def_clk_sel;
+ u8 num_clks;
u8 iotype;
/* uart port features */
@@ -1339,7 +1339,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
#define MAX_CLK_NAME_LENGTH 15
-static inline int s3c24xx_serial_getsource(struct uart_port *port)
+static inline u8 s3c24xx_serial_getsource(struct uart_port *port)
{
const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
u32 ucon;
@@ -1352,8 +1352,7 @@ static inline int s3c24xx_serial_getsource(struct uart_port *port)
return ucon >> info->clksel_shift;
}
-static void s3c24xx_serial_setsource(struct uart_port *port,
- unsigned int clk_sel)
+static void s3c24xx_serial_setsource(struct uart_port *port, u8 clk_sel)
{
const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
u32 ucon;
@@ -1372,14 +1371,15 @@ static void s3c24xx_serial_setsource(struct uart_port *port,
static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport,
unsigned int req_baud, struct clk **best_clk,
- unsigned int *clk_num)
+ u8 *clk_num)
{
const struct s3c24xx_uart_info *info = ourport->info;
struct clk *clk;
unsigned long rate;
- unsigned int cnt, baud, quot, best_quot = 0;
+ unsigned int baud, quot, best_quot = 0;
char clkname[MAX_CLK_NAME_LENGTH];
int calc_deviation, deviation = (1 << 30) - 1;
+ u8 cnt;
for (cnt = 0; cnt < info->num_clks; cnt++) {
/* Keep selected clock if provided */
@@ -1472,9 +1472,10 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
struct s3c24xx_uart_port *ourport = to_ourport(port);
struct clk *clk = ERR_PTR(-EINVAL);
unsigned long flags;
- unsigned int baud, quot, clk_sel = 0;
+ unsigned int baud, quot;
unsigned int udivslot = 0;
u32 ulcon, umcon;
+ u8 clk_sel = 0;
/*
* We don't support modem control lines.
@@ -1775,10 +1776,9 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
struct device *dev = ourport->port.dev;
const struct s3c24xx_uart_info *info = ourport->info;
char clk_name[MAX_CLK_NAME_LENGTH];
- unsigned int clk_sel;
struct clk *clk;
- int clk_num;
int ret;
+ u8 clk_sel, clk_num;
clk_sel = ourport->cfg->clk_sel ? : info->def_clk_sel;
for (clk_num = 0; clk_num < info->num_clks; clk_num++) {
@@ -2286,9 +2286,9 @@ s3c24xx_serial_get_options(struct uart_port *port, int *baud,
{
struct clk *clk;
unsigned long rate;
- unsigned int clk_sel;
u32 ulcon, ucon, ubrdiv;
char clk_name[MAX_CLK_NAME_LENGTH];
+ u8 clk_sel;
ulcon = rd_regl(port, S3C2410_ULCON);
ucon = rd_regl(port, S3C2410_UCON);
--
2.43.0.472.g3155946c3a-goog
On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaroorg> wrote:
>
> <linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.
Then maybe it makes sense to turn those two field into 4-bit bit
fields? More importantly, what particular problem does this patch
solve, is this optimization really needed, and why? I'm not saying
it's not needed, just that commit message might've been more verbose
about this.
> Update the driver to consider a pool selection of maximum 8 clocks. The
> final scope is to reduce the memory footprint of
> ``struct s3c24xx_uart_info``.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 436739cf9225..5df2bcebf9fb 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
> unsigned long tx_fifomask;
> unsigned long tx_fifoshift;
> unsigned long tx_fifofull;
> - unsigned int def_clk_sel;
> - unsigned long num_clks;
> unsigned long clksel_mask;
> unsigned long clksel_shift;
> unsigned long ucon_mask;
> + u8 def_clk_sel;
> + u8 num_clks;
> u8 iotype;
>
> /* uart port features */
> @@ -1339,7 +1339,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
>
> #define MAX_CLK_NAME_LENGTH 15
>
> -static inline int s3c24xx_serial_getsource(struct uart_port *port)
> +static inline u8 s3c24xx_serial_getsource(struct uart_port *port)
> {
> const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> u32 ucon;
> @@ -1352,8 +1352,7 @@ static inline int s3c24xx_serial_getsource(struct uart_port *port)
> return ucon >> info->clksel_shift;
> }
>
> -static void s3c24xx_serial_setsource(struct uart_port *port,
> - unsigned int clk_sel)
> +static void s3c24xx_serial_setsource(struct uart_port *port, u8 clk_sel)
> {
> const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> u32 ucon;
> @@ -1372,14 +1371,15 @@ static void s3c24xx_serial_setsource(struct uart_port *port,
>
> static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport,
> unsigned int req_baud, struct clk **best_clk,
> - unsigned int *clk_num)
> + u8 *clk_num)
> {
> const struct s3c24xx_uart_info *info = ourport->info;
> struct clk *clk;
> unsigned long rate;
> - unsigned int cnt, baud, quot, best_quot = 0;
> + unsigned int baud, quot, best_quot = 0;
> char clkname[MAX_CLK_NAME_LENGTH];
> int calc_deviation, deviation = (1 << 30) - 1;
> + u8 cnt;
>
> for (cnt = 0; cnt < info->num_clks; cnt++) {
> /* Keep selected clock if provided */
> @@ -1472,9 +1472,10 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
> struct s3c24xx_uart_port *ourport = to_ourport(port);
> struct clk *clk = ERR_PTR(-EINVAL);
> unsigned long flags;
> - unsigned int baud, quot, clk_sel = 0;
> + unsigned int baud, quot;
> unsigned int udivslot = 0;
> u32 ulcon, umcon;
> + u8 clk_sel = 0;
>
> /*
> * We don't support modem control lines.
> @@ -1775,10 +1776,9 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
> struct device *dev = ourport->port.dev;
> const struct s3c24xx_uart_info *info = ourport->info;
> char clk_name[MAX_CLK_NAME_LENGTH];
> - unsigned int clk_sel;
> struct clk *clk;
> - int clk_num;
> int ret;
> + u8 clk_sel, clk_num;
>
> clk_sel = ourport->cfg->clk_sel ? : info->def_clk_sel;
> for (clk_num = 0; clk_num < info->num_clks; clk_num++) {
> @@ -2286,9 +2286,9 @@ s3c24xx_serial_get_options(struct uart_port *port, int *baud,
> {
> struct clk *clk;
> unsigned long rate;
> - unsigned int clk_sel;
> u32 ulcon, ucon, ubrdiv;
> char clk_name[MAX_CLK_NAME_LENGTH];
> + u8 clk_sel;
>
> ulcon = rd_regl(port, S3C2410_ULCON);
> ucon = rd_regl(port, S3C2410_UCON);
> --
> 2.43.0.472.g3155946c3a-goog
>
>
On 1/16/24 19:09, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <[email protected]> wrote:
>>
>> <linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.
>
> Then maybe it makes sense to turn those two field into 4-bit bit
> fields? More importantly, what particular problem does this patch
> solve, is this optimization really needed, and why? I'm not saying
> it's not needed, just that commit message might've been more verbose
> about this.
>
I guess I could have been more verbose in the phrase from below and said
that for arm64 ``struct s3c24xx_uart_info`` spans through 2 cachelines
and contains 2 holes, and with a bit of love it can fit a single
cacheline with no holes. The end goal is to reduce the memory footprint
of that struct.
I chose u8 and allowed a max of 8 clocks simple because it's large
enough to allow more clocks than are supported by the driver now, and
not too big to cause spanning of the structure through 2 cachelines.
>> Update the driver to consider a pool selection of maximum 8 clocks. The
>> final scope is to reduce the memory footprint of
>> ``struct s3c24xx_uart_info``.
>>
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index 436739cf9225..5df2bcebf9fb 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
>> unsigned long tx_fifomask;
>> unsigned long tx_fifoshift;
>> unsigned long tx_fifofull;
>> - unsigned int def_clk_sel;
>> - unsigned long num_clks;
>> unsigned long clksel_mask;
>> unsigned long clksel_shift;
>> unsigned long ucon_mask;
>> + u8 def_clk_sel;
>> + u8 num_clks;
>> u8 iotype;
>>
On Wed, Jan 17, 2024 at 10:26 AM Tudor Ambarus <[email protected]> wrote:
>
>
>
> On 1/16/24 19:09, Sam Protsenko wrote:
> > On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <[email protected]> wrote:
> >>
> >> <linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.
> >
> > Then maybe it makes sense to turn those two field into 4-bit bit
> > fields? More importantly, what particular problem does this patch
> > solve, is this optimization really needed, and why? I'm not saying
> > it's not needed, just that commit message might've been more verbose
> > about this.
> >
>
> I guess I could have been more verbose in the phrase from below and said
> that for arm64 ``struct s3c24xx_uart_info`` spans through 2 cachelines
> and contains 2 holes, and with a bit of love it can fit a single
> cacheline with no holes. The end goal is to reduce the memory footprint
> of that struct.
>
Oh yeah, I actually like the cachelines part. Please add that bit to
the commit message if you don't mind.
> I chose u8 and allowed a max of 8 clocks simple because it's large
> enough to allow more clocks than are supported by the driver now, and
> not too big to cause spanning of the structure through 2 cachelines.
>
Gotcha. Maybe also add that reasoning to the commit message. Just a
thought. With above comments addressed, feel free to add:
Reviewed-by: Sam Protsenko <[email protected]>
>
> >> Update the driver to consider a pool selection of maximum 8 clocks. The
> >> final scope is to reduce the memory footprint of
> >> ``struct s3c24xx_uart_info``.
> >>
> >> Signed-off-by: Tudor Ambarus <[email protected]>
> >> ---
> >> drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
> >> 1 file changed, 11 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> >> index 436739cf9225..5df2bcebf9fb 100644
> >> --- a/drivers/tty/serial/samsung_tty.c
> >> +++ b/drivers/tty/serial/samsung_tty.c
> >> @@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
> >> unsigned long tx_fifomask;
> >> unsigned long tx_fifoshift;
> >> unsigned long tx_fifofull;
> >> - unsigned int def_clk_sel;
> >> - unsigned long num_clks;
> >> unsigned long clksel_mask;
> >> unsigned long clksel_shift;
> >> unsigned long ucon_mask;
> >> + u8 def_clk_sel;
> >> + u8 num_clks;
> >> u8 iotype;
> >>