| Message ID | eb1b2838f1c3c006c24bcb9816f75e1351c63b05.1590594293.git-series.maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: bcm: rpi: Add support for BCM2711 firmware clocks | expand |
On Wed, 2020-05-27 at 17:45 +0200, Maxime Ripard wrote: > The raspberrypi firmware clock driver has a min_rate / max_rate clamping by > storing the info it needs in a private structure. > > However, the CCF already provides such a facility, so we can switch to it > to remove the boilerplate. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/clk/bcm/clk-raspberrypi.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk- > raspberrypi.c > index a20492fade6a..e135ad28d38d 100644 > --- a/drivers/clk/bcm/clk-raspberrypi.c > +++ b/drivers/clk/bcm/clk-raspberrypi.c > @@ -36,9 +36,6 @@ struct raspberrypi_clk { > struct rpi_firmware *firmware; > struct platform_device *cpufreq; > > - unsigned long min_rate; > - unsigned long max_rate; > - > struct clk_hw pllb; > }; > > @@ -142,13 +139,11 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw > *hw, unsigned long rate, > static int raspberrypi_pll_determine_rate(struct clk_hw *hw, > struct clk_rate_request *req) > { > - struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk, > - pllb); > u64 div, final_rate; > u32 ndiv, fdiv; > > /* We can't use req->rate directly as it would overflow */ > - final_rate = clamp(req->rate, rpi->min_rate, rpi->max_rate); > + final_rate = clamp(req->rate, req->min_rate, req->max_rate); > > div = (u64)final_rate << A2W_PLL_FRAC_BITS; > do_div(div, req->best_parent_rate); > @@ -215,12 +210,15 @@ static int raspberrypi_register_pllb(struct > raspberrypi_clk *rpi) > dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n", > min_rate, max_rate); > > - rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; > - rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; > - > rpi->pllb.init = &init; > > - return devm_clk_hw_register(rpi->dev, &rpi->pllb); > + ret = devm_clk_hw_register(rpi->dev, &rpi->pllb); > + if (!ret) > + clk_hw_set_rate_range(&rpi->pllb, > + min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE, > + max_rate * > RPI_FIRMWARE_PLLB_ARM_DIV_RATE); Isn't there a potential race here? Albeit unlikely, cpufreq could show up and call clk_round_rate() in between the registration and you setting the ranges. Regards, Nicolas
Hi Nicolas, On Thu, Jun 04, 2020 at 08:02:22PM +0200, Nicolas Saenz Julienne wrote: > On Wed, 2020-05-27 at 17:45 +0200, Maxime Ripard wrote: > > The raspberrypi firmware clock driver has a min_rate / max_rate clamping by > > storing the info it needs in a private structure. > > > > However, the CCF already provides such a facility, so we can switch to it > > to remove the boilerplate. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/clk/bcm/clk-raspberrypi.c | 18 ++++++++---------- > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk- > > raspberrypi.c > > index a20492fade6a..e135ad28d38d 100644 > > --- a/drivers/clk/bcm/clk-raspberrypi.c > > +++ b/drivers/clk/bcm/clk-raspberrypi.c > > @@ -36,9 +36,6 @@ struct raspberrypi_clk { > > struct rpi_firmware *firmware; > > struct platform_device *cpufreq; > > > > - unsigned long min_rate; > > - unsigned long max_rate; > > - > > struct clk_hw pllb; > > }; > > > > @@ -142,13 +139,11 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw > > *hw, unsigned long rate, > > static int raspberrypi_pll_determine_rate(struct clk_hw *hw, > > struct clk_rate_request *req) > > { > > - struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk, > > - pllb); > > u64 div, final_rate; > > u32 ndiv, fdiv; > > > > /* We can't use req->rate directly as it would overflow */ > > - final_rate = clamp(req->rate, rpi->min_rate, rpi->max_rate); > > + final_rate = clamp(req->rate, req->min_rate, req->max_rate); > > > > div = (u64)final_rate << A2W_PLL_FRAC_BITS; > > do_div(div, req->best_parent_rate); > > @@ -215,12 +210,15 @@ static int raspberrypi_register_pllb(struct > > raspberrypi_clk *rpi) > > dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n", > > min_rate, max_rate); > > > > - rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; > > - rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; > > - > > rpi->pllb.init = &init; > > > > - return devm_clk_hw_register(rpi->dev, &rpi->pllb); > > + ret = devm_clk_hw_register(rpi->dev, &rpi->pllb); > > + if (!ret) > > + clk_hw_set_rate_range(&rpi->pllb, > > + min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE, > > + max_rate * > > RPI_FIRMWARE_PLLB_ARM_DIV_RATE); > > Isn't there a potential race here? Albeit unlikely, cpufreq could show > up and call clk_round_rate() in between the registration and you > setting the ranges. IIRC, driver's probe are not called in parallel but in sequence, so we should be covered here. Maxime
On Fri, 2020-06-05 at 11:28 +0200, Maxime Ripard wrote: > Hi Nicolas, > > On Thu, Jun 04, 2020 at 08:02:22PM +0200, Nicolas Saenz Julienne wrote: > > On Wed, 2020-05-27 at 17:45 +0200, Maxime Ripard wrote: > > > The raspberrypi firmware clock driver has a min_rate / max_rate clamping > > > by > > > storing the info it needs in a private structure. > > > > > > However, the CCF already provides such a facility, so we can switch to it > > > to remove the boilerplate. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > --- > > > drivers/clk/bcm/clk-raspberrypi.c | 18 ++++++++---------- > > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk- > > > raspberrypi.c > > > index a20492fade6a..e135ad28d38d 100644 > > > --- a/drivers/clk/bcm/clk-raspberrypi.c > > > +++ b/drivers/clk/bcm/clk-raspberrypi.c > > > @@ -36,9 +36,6 @@ struct raspberrypi_clk { > > > struct rpi_firmware *firmware; > > > struct platform_device *cpufreq; > > > > > > - unsigned long min_rate; > > > - unsigned long max_rate; > > > - > > > struct clk_hw pllb; > > > }; > > > > > > @@ -142,13 +139,11 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw > > > *hw, unsigned long rate, > > > static int raspberrypi_pll_determine_rate(struct clk_hw *hw, > > > struct clk_rate_request *req) > > > { > > > - struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk, > > > - pllb); > > > u64 div, final_rate; > > > u32 ndiv, fdiv; > > > > > > /* We can't use req->rate directly as it would overflow */ > > > - final_rate = clamp(req->rate, rpi->min_rate, rpi->max_rate); > > > + final_rate = clamp(req->rate, req->min_rate, req->max_rate); > > > > > > div = (u64)final_rate << A2W_PLL_FRAC_BITS; > > > do_div(div, req->best_parent_rate); > > > @@ -215,12 +210,15 @@ static int raspberrypi_register_pllb(struct > > > raspberrypi_clk *rpi) > > > dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n", > > > min_rate, max_rate); > > > > > > - rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; > > > - rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; > > > - > > > rpi->pllb.init = &init; > > > > > > - return devm_clk_hw_register(rpi->dev, &rpi->pllb); > > > + ret = devm_clk_hw_register(rpi->dev, &rpi->pllb); > > > + if (!ret) > > > + clk_hw_set_rate_range(&rpi->pllb, > > > + min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE, > > > + max_rate * > > > RPI_FIRMWARE_PLLB_ARM_DIV_RATE); > > > > Isn't there a potential race here? Albeit unlikely, cpufreq could show > > up and call clk_round_rate() in between the registration and you > > setting the ranges. > > IIRC, driver's probe are not called in parallel but in sequence, so we > should be covered here. Right, of course. Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Regards, Nicolas
diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index a20492fade6a..e135ad28d38d 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -36,9 +36,6 @@ struct raspberrypi_clk { struct rpi_firmware *firmware; struct platform_device *cpufreq; - unsigned long min_rate; - unsigned long max_rate; - struct clk_hw pllb; }; @@ -142,13 +139,11 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate, static int raspberrypi_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) { - struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk, - pllb); u64 div, final_rate; u32 ndiv, fdiv; /* We can't use req->rate directly as it would overflow */ - final_rate = clamp(req->rate, rpi->min_rate, rpi->max_rate); + final_rate = clamp(req->rate, req->min_rate, req->max_rate); div = (u64)final_rate << A2W_PLL_FRAC_BITS; do_div(div, req->best_parent_rate); @@ -215,12 +210,15 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi) dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n", min_rate, max_rate); - rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; - rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE; - rpi->pllb.init = &init; - return devm_clk_hw_register(rpi->dev, &rpi->pllb); + ret = devm_clk_hw_register(rpi->dev, &rpi->pllb); + if (!ret) + clk_hw_set_rate_range(&rpi->pllb, + min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE, + max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE); + + return ret; } static struct clk_fixed_factor raspberrypi_clk_pllb_arm = {
The raspberrypi firmware clock driver has a min_rate / max_rate clamping by storing the info it needs in a private structure. However, the CCF already provides such a facility, so we can switch to it to remove the boilerplate. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/bcm/clk-raspberrypi.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)