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
