| Message ID | 20240110102102.61587-17-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6e1e48b6ef2613ff4c28a34f7a57c29a4367ad87 |
| Headers | show |
| Series | serial: samsung: gs101 updates and winter cleanup | expand |
On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> 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 <tudor.ambarus@linaro.org> > --- > 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 <tudor.ambarus@linaro.org> 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 <tudor.ambarus@linaro.org> >> --- >> 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 <tudor.ambarus@linaro.org> wrote: > > > > On 1/16/24 19:09, Sam Protsenko wrote: > > On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> 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 <semen.protsenko@linaro.org> > > >> 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 <tudor.ambarus@linaro.org> > >> --- > >> 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; > >>
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);
<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 <tudor.ambarus@linaro.org> --- drivers/tty/serial/samsung_tty.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)