Re: [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock

[Jernej Åkrabec]

Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König 
napisal(a):
> Hello Clément,
> 
> On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote:
> > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
> > 
> > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > From: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > 
> > > > H6 PWM core needs bus clock to be enabled in order to work.
> > > > 
> > > > Add an optional probe for it and a fallback for previous
> > > > bindings without name on module clock.
> > > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > Signed-off-by: Clément Péron <peron.clem@xxxxxxxxx>
> > > > ---
> > > > 
> > > >  drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 36 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index d194b8ebdb00..b5e7ac364f59 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
> > > > 
> > > >  struct sun4i_pwm_chip {
> > > >  
> > > >       struct pwm_chip chip;
> > > > 
> > > > +     struct clk *bus_clk;
> > > > 
> > > >       struct clk *clk;
> > > >       struct reset_control *rst;
> > > >       void __iomem *base;
> > > > 
> > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device
> > > > *pdev)> > 
> > > Adding more context here:
> > > |       pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > |       
> > > >       if (IS_ERR(pwm->clk))
> > > >       
> > > >               return PTR_ERR(pwm->clk);
> > > > 
> > > > +     /* Get all clocks and reset line */
> > > > +     pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> > > > +     if (IS_ERR(pwm->clk)) {
> > > > +             dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > +                     PTR_ERR(pwm->clk));
> > > > +             return PTR_ERR(pwm->clk);
> > > > +     }
> > > 
> > > I guess you want to drop the first assignment to pwm->clk.
> > 
> > devm_clk_get_optional will return NULL if there is no entry, I don't
> > get where I need to drop it assignment.
> 
> With your patch the code looks as follows:
> 
> 	pwm->clk = devm_clk_get(&pdev->dev, NULL);
> 	if (IS_ERR(pwm->clk))
> 		return PTR_ERR(pwm->clk);
> 
> 	/* Get all clocks and reset line */
> 	pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");

Actually, it's the other way around, e.g. "mod" clock is checked first.

> 	...
> 
> The assignment to pwm->clk above the comment is the one I suggested to
> drop.

Neither can be dropped. DT files for other SoCs don't have clock-names 
property, so search for "mod" clock will fail and then fallback option without 
name is used.

Best regards,
Jernej

> 
> > > > +     /* Fallback for old dtbs with a single clock and no name */
> > > > +     if (!pwm->clk) {
> > > > +             pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +             if (IS_ERR(pwm->clk)) {
> > > > +                     dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > +                             PTR_ERR(pwm->clk));
> > > > +                     return PTR_ERR(pwm->clk);
> > > > +             }
> > > > +     }
> > > 
> > > There is a slight change of behaviour if I'm not mistaken. If you have
> > > 
> > > this:
> > >         clocks = <&clk1>;
> > >         clock-names = "mod";
> > >         
> > >         pwm {
> > >         
> > >                 compatible = "allwinner,sun4i-a10-pwm"
> > >                 clocks = <&clk2>;
> > >         
> > >         }
> > > 
> > > you now use clk1 instead of clk2 before.
> > > 
> > > Assuming this is only a theoretical problem, at least pointing this out
> > > in the commit log would be good I think.
> > 
> > Yes it's correct and as you said the driver don't check for a correct
> > device tree, that why it's now optional probe.
> > Let's assume that's the device-tree is correct, I will add a comment
> > in the commit log.
> 
> If the mod clock was shared by all peripherals on the bus this would be
> IMHO quite elegant. Probably it depends on what you mean by saying
> "incorrect" if this snippet is incorrect. (It can be part of a valid dtb
> that even complies to the binding documentation. However that's not how
> any existing allwinner hardware looks like.) But let's stop arguing as
> we agree it's a corner case and if you mention it in the commit log
> we're both happy.
> 
> > > What is that clock used for? Is it required to access the hardware
> > > registers? Or is it only required while the PWM is enabled? If so you
> > > could enable the clock more finegrainded.
> > 
> > Regarding the datasheet it's required to access the hardware.
> > page 261 :
> > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf
> So enabling the bus clock is called "open APB1 Bus gating" in that
> manual? If I understand that correctly the bus clock then only need to
> be on while accessing the registers and could be disabled once the
> hardware is programmed and running.
> 
> Can you please describe that in a comment. Something like:
> 
> 	/*
> 	 * We're keeping the bus clock on for the sake of simplicity.
> 	 * Actually it only needs to be on for hardware register
> 	 * accesses.
> 	 */
> 
> should be fine. This way it's at least obvious that the handling could
> be improved.
> 
> Best regards
> Uwe