Message ID | 0acdf3a78f670a2678e03b0bbbb01aa58a11ce9a.1608407584.git.simon@simonsouth.net |
---|---|
State | Superseded |
Headers | show |
Series | pwm: rockchip: Eliminate potential race condition when probing | expand |
On Sat, Dec 19, 2020 at 03:44:10PM -0500, Simon South wrote: > Currently the Rockchip PWM driver enables the signal ("bus") clock for > every PWM device it finds during probing, then disables it for any device > that was not already enabled (such as by a bootloader) when the kernel > started. > > Instead of starting PWMs unnecessarily, check first to see whether a device "starting PWM" here means enabling their clocks, right? I wouldn't expect that this has any effect on the output, am I right? > has already been enabled and if not, do not enable its signal clock. > > Signed-off-by: Simon South <simon@simonsouth.net> > --- > drivers/pwm/pwm-rockchip.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index f286a498b82c..b9faef3e9954 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > return ret; > } > > - ret = clk_prepare_enable(pc->clk); > - if (ret) { > - dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret); > - return ret; > - } > - > - ret = clk_prepare_enable(pc->pclk); > - if (ret) { > - dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); > - clk_disable_unprepare(pc->clk); > - return ret; > - } Just for my understanding: That you moved clk_prepare_enable(pc->pclk) further down is not strictly necessary for your change, right? > - > platform_set_drvdata(pdev, pc); > > pc->data = id->data; > @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > pc->chip.of_pwm_n_cells = 3; > } > > + ret = clk_prepare_enable(pc->pclk); > + if (ret) { > + dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); > + return ret; > + } > + > /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > enable_conf = pc->data->enable_conf; > ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl); > enabled = ((ctrl & enable_conf) == enable_conf); > - if (!enabled) > - clk_disable(pc->clk); > + > + ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk); > + if (ret) { > + dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret); > + clk_disable_unprepare(pc->pclk); > + return ret; > + } I'm not a big fan of this ?: construct. I'd prefer ret = clk_prepare(pc->clk); if (ret) ... /* Keep the PWM clk enabled ... */ enabled = ... if (enabled) { ret = clk_enable(pc->clk); if (ret) ... } even though it is less compact. A small benefit is that the error message can be more accurate. (You wrote "Can't prepare bus clk" while the problem might well be in the enable part, but mentioning "enable" might also be misleading for the enabled = false case.) Best regards Uwe
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > "starting PWM" here means enabling their clocks, right? I wouldn't > expect that this has any effect on the output, am I right? Right, it does not; another misunderstanding on my part. I'll fix the commit message. > Just for my understanding: That you moved clk_prepare_enable(pc->pclk) > further down is not strictly necessary for your change, right? That's right. My thought was to keep the code that enables this clock close to the code that relies on it, and to minimize the duration the clock is enabled. Would it be better to leave the code where it is? > I'm not a big fan of this ?: construct... Neither was I, actually, but I guessed the shorter version would be preferred. I'm happy to change it back to match what you described.
On 2020-12-19 20:44, Simon South wrote: > Currently the Rockchip PWM driver enables the signal ("bus") clock for > every PWM device it finds during probing, then disables it for any device > that was not already enabled (such as by a bootloader) when the kernel > started. > > Instead of starting PWMs unnecessarily, check first to see whether a device > has already been enabled and if not, do not enable its signal clock. > > Signed-off-by: Simon South <simon@simonsouth.net> > --- > drivers/pwm/pwm-rockchip.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index f286a498b82c..b9faef3e9954 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > return ret; > } > > - ret = clk_prepare_enable(pc->clk); > - if (ret) { > - dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret); > - return ret; > - } > - > - ret = clk_prepare_enable(pc->pclk); > - if (ret) { > - dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); > - clk_disable_unprepare(pc->clk); > - return ret; > - } > - > platform_set_drvdata(pdev, pc); > > pc->data = id->data; > @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > pc->chip.of_pwm_n_cells = 3; > } > > + ret = clk_prepare_enable(pc->pclk); > + if (ret) { > + dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); > + return ret; > + } > + > /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > enable_conf = pc->data->enable_conf; > ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl); > enabled = ((ctrl & enable_conf) == enable_conf); > - if (!enabled) > - clk_disable(pc->clk); > + > + ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk); > + if (ret) { > + dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret); Since you're touching it, I guess it might be a good idea to update this message to say "PWM clk" for clarity. I suspect there might also have been some historical confounding in the fact that when there is only one clock (pclk_pwm), it's merely a gate whose parent is pclk_bus, which serves all the peripherals in the usefully-named PD_BUS domain... Robin. > + clk_disable_unprepare(pc->pclk); > + return ret; > + } > > clk_disable(pc->pclk); > >
Robin Murphy <robin.murphy@arm.com> writes: > Since you're touching it, I guess it might be a good idea to update > this message to say "PWM clk" for clarity. Sure, I'll be happy to remove another source of confusion.
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index f286a498b82c..b9faef3e9954 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -327,19 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) return ret; } - ret = clk_prepare_enable(pc->clk); - if (ret) { - dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret); - return ret; - } - - ret = clk_prepare_enable(pc->pclk); - if (ret) { - dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); - clk_disable_unprepare(pc->clk); - return ret; - } - platform_set_drvdata(pdev, pc); pc->data = id->data; @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev) pc->chip.of_pwm_n_cells = 3; } + ret = clk_prepare_enable(pc->pclk); + if (ret) { + dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); + return ret; + } + /* Keep the PWM clk enabled if the PWM appears to be up and running. */ enable_conf = pc->data->enable_conf; ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl); enabled = ((ctrl & enable_conf) == enable_conf); - if (!enabled) - clk_disable(pc->clk); + + ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk); + if (ret) { + dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret); + clk_disable_unprepare(pc->pclk); + return ret; + } clk_disable(pc->pclk);
Currently the Rockchip PWM driver enables the signal ("bus") clock for every PWM device it finds during probing, then disables it for any device that was not already enabled (such as by a bootloader) when the kernel started. Instead of starting PWMs unnecessarily, check first to see whether a device has already been enabled and if not, do not enable its signal clock. Signed-off-by: Simon South <simon@simonsouth.net> --- drivers/pwm/pwm-rockchip.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)