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

[ClÃment PÃron]

Hi,

On Mon, 4 Nov 2019 at 21:19, Jernej Åkrabec <jernej.skrabec@xxxxxxxx> wrote:
>
> 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 first devm_clk_get is indeed wrong, I will remove it!

>
> >       ...
> >
> > 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
>
>
>
>