Re: [PATCH v3 12/25] clk: bcm: rpi: Use CCF boundaries instead of rolling our own

[Nicolas Saenz Julienne]

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@xxxxxxxxxx>
> > > ---
> > >  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@xxxxxxx>

Regards,
Nicolas

Attachment: signature.asc
Description: This is a digitally signed message part