| Message ID | 20240303121410.240761-1-andy.shevchenko@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v1,1/1] clk: fractional-divider: Move mask calculations out of lock | expand |
On Sun, Mar 3, 2024 at 8:14 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > There is no need to calculate masks under the lock taken. > Move them out of it. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
Am Sonntag, 3. März 2024, 13:14:10 CET schrieb Andy Shevchenko: > There is no need to calculate masks under the lock taken. > Move them out of it. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/clk/clk-fractional-divider.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c > index a0178182fc72..da057172cc90 100644 > --- a/drivers/clk/clk-fractional-divider.c > +++ b/drivers/clk/clk-fractional-divider.c > @@ -195,14 +195,14 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate, > n--; > } > > + mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > + nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > + > if (fd->lock) > spin_lock_irqsave(fd->lock, flags); > else > __acquire(fd->lock); > > - mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > - nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > - > val = clk_fd_readl(fd); > val &= ~(mmask | nmask); > val |= (m << fd->mshift) | (n << fd->nshift); >
Quoting Andy Shevchenko (2024-03-03 04:14:10) > There is no need to calculate masks under the lock taken. > Move them out of it. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- Applied to clk-next > diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c > index a0178182fc72..da057172cc90 100644 > --- a/drivers/clk/clk-fractional-divider.c > +++ b/drivers/clk/clk-fractional-divider.c > @@ -195,14 +195,14 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate, > n--; > } > > + mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > + nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > + > if (fd->lock) > spin_lock_irqsave(fd->lock, flags); > else > __acquire(fd->lock); > > - mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > - nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > - > val = clk_fd_readl(fd); > val &= ~(mmask | nmask); > val |= (m << fd->mshift) | (n << fd->nshift); Should we pre-calculate the mask and shift values too!?
Le 03/03/2024 à 13:14, Andy Shevchenko a écrit : > There is no need to calculate masks under the lock taken. > Move them out of it. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/clk/clk-fractional-divider.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c > index a0178182fc72..da057172cc90 100644 > --- a/drivers/clk/clk-fractional-divider.c > +++ b/drivers/clk/clk-fractional-divider.c > @@ -195,14 +195,14 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate, > n--; > } > > + mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > + nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > + Hi, if this is a hot path, you could maybe even compute: mask = ~(GENMASK(fd->mwidth - 1, 0) << fd->mshift | GENMASK(fd->nwidth - 1, 0) << fd->nshift) unless gcc is smart enough to do it by itself. > if (fd->lock) > spin_lock_irqsave(fd->lock, flags); > else > __acquire(fd->lock); > > - mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > - nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > - > val = clk_fd_readl(fd); > val &= ~(mmask | nmask); val &= mask; > val |= (m << fd->mshift) | (n << fd->nshift); and pre-compute "(m << fd->mshift) | (n << fd->nshift)" outside of the lock too. CJ
On Sat, Mar 9, 2024 at 9:19 AM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > Le 03/03/2024 à 13:14, Andy Shevchenko a écrit : ... > > @@ -195,14 +195,14 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate, > > n--; > > } > > > > + mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > > + nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > if this is a hot path, you could maybe even compute: It's not. set_rate() may be called only on disabled (and unprepared?) clocks, which makes it already a too slow operation. > mask = ~(GENMASK(fd->mwidth - 1, 0) << fd->mshift | > GENMASK(fd->nwidth - 1, 0) << fd->nshift) > > unless gcc is smart enough to do it by itself. > > > if (fd->lock) > > spin_lock_irqsave(fd->lock, flags); > > else > > __acquire(fd->lock); > > > > - mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; > > - nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; > > - > > val = clk_fd_readl(fd); > > val &= ~(mmask | nmask); > > val &= mask; > > > val |= (m << fd->mshift) | (n << fd->nshift); > > and pre-compute "(m << fd->mshift) | (n << fd->nshift)" outside of the > lock too. All of these sound to me as premature optimisations. I only wanted to get back to the status quo.
On Sat, Mar 9, 2024 at 3:06 AM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Andy Shevchenko (2024-03-03 04:14:10) ... > Applied to clk-next Thank you! ... > Should we pre-calculate the mask and shift values too!? Not that it's required, but we _can_ do that, of course.
diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c index a0178182fc72..da057172cc90 100644 --- a/drivers/clk/clk-fractional-divider.c +++ b/drivers/clk/clk-fractional-divider.c @@ -195,14 +195,14 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate, n--; } + mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; + nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; + if (fd->lock) spin_lock_irqsave(fd->lock, flags); else __acquire(fd->lock); - mmask = GENMASK(fd->mwidth - 1, 0) << fd->mshift; - nmask = GENMASK(fd->nwidth - 1, 0) << fd->nshift; - val = clk_fd_readl(fd); val &= ~(mmask | nmask); val |= (m << fd->mshift) | (n << fd->nshift);
There is no need to calculate masks under the lock taken. Move them out of it. Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> --- drivers/clk/clk-fractional-divider.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)