[v2,3/7] pwm: sun4i: Add an optional probe for bus clock
diff mbox series

Message ID 20191103203334.10539-4-peron.clem@gmail.com
State New
Headers show
Series
  • Add support for H6 PWM
Related show

Commit Message

Clément Péron Nov. 3, 2019, 8:33 p.m. UTC
From: Jernej Skrabec <jernej.skrabec@siol.net>

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@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Uwe Kleine-König Nov. 4, 2019, 8:24 a.m. UTC | #1
Hello,

On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> 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@siol.net>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  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.

> +	/* 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.

> +	pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus");
> +	if (IS_ERR(pwm->bus_clk)) {
> +		dev_err(&pdev->dev, "get bus_clock failed %ld\n",
> +			PTR_ERR(pwm->bus_clk));
> +		return PTR_ERR(pwm->bus_clk);
> +	}
> +
>  	pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
>  	if (IS_ERR(pwm->rst)) {
>  		if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> @@ -381,6 +407,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/* Enable bus clock */
> +	ret = clk_prepare_enable(pwm->bus_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n");

I'd do s/prepare_enable/prepare and enable/ here.

> +		goto err_bus;
> +	}
> +
>  	pwm->chip.dev = &pdev->dev;
>  	pwm->chip.ops = &sun4i_pwm_ops;
>  	pwm->chip.base = -1;
> @@ -401,6 +434,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_pwm_add:
> +	clk_disable_unprepare(pwm->bus_clk);
> +err_bus:
>  	reset_control_assert(pwm->rst);
>  
>  	return ret;

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.

Best regards
Uwe
Clément Péron Nov. 4, 2019, 6:07 p.m. UTC | #2
Hi,

On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > 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@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  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.

>
> > +     /* 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.

>
> > +     pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus");
> > +     if (IS_ERR(pwm->bus_clk)) {
> > +             dev_err(&pdev->dev, "get bus_clock failed %ld\n",
> > +                     PTR_ERR(pwm->bus_clk));
> > +             return PTR_ERR(pwm->bus_clk);
> > +     }
> > +
> >       pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> >       if (IS_ERR(pwm->rst)) {
> >               if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> > @@ -381,6 +407,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > +     /* Enable bus clock */
> > +     ret = clk_prepare_enable(pwm->bus_clk);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n");
>
> I'd do s/prepare_enable/prepare and enable/ here.
Ok

>
> > +             goto err_bus;
> > +     }
> > +
> >       pwm->chip.dev = &pdev->dev;
> >       pwm->chip.ops = &sun4i_pwm_ops;
> >       pwm->chip.base = -1;
> > @@ -401,6 +434,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> >       return 0;
> >
> >  err_pwm_add:
> > +     clk_disable_unprepare(pwm->bus_clk);
> > +err_bus:
> >       reset_control_assert(pwm->rst);
> >
> >       return ret;
>
> 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

Regards,
Clément

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Uwe Kleine-König Nov. 4, 2019, 8:10 p.m. UTC | #3
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@pengutronix.de> wrote:
> > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > >
> > > 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@siol.net>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > >  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");
	...

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

> > > +     /* 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
Jernej Skrabec Nov. 4, 2019, 8:19 p.m. UTC | #4
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@pengutronix.de> wrote:
> > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > 
> > > > 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@siol.net>
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > ---
> > > > 
> > > >  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
Clément Péron Nov. 4, 2019, 8:27 p.m. UTC | #5
Hi,

On Mon, 4 Nov 2019 at 21:19, Jernej Škrabec <jernej.skrabec@siol.net> 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@pengutronix.de> wrote:
> > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > >
> > > > > 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@siol.net>
> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > > ---
> > > > >
> > > > >  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
>
>
>
>
Jernej Skrabec Nov. 4, 2019, 8:38 p.m. UTC | #6
Dne ponedeljek, 04. november 2019 ob 21:27:04 CET je Clément Péron napisal(a):
> Hi,
> 
> On Mon, 4 Nov 2019 at 21:19, Jernej Škrabec <jernej.skrabec@siol.net> 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@pengutronix.de> wrote:
> > > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > > 
> > > > > > 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@siol.net>
> > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  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!

Sorry, I missed that too. Yeah, it should be removed.

Best regards,
Jernej

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

Patch
diff mbox series

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)
 	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);
+	}
+
+	/* 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);
+		}
+	}
+
+	pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus");
+	if (IS_ERR(pwm->bus_clk)) {
+		dev_err(&pdev->dev, "get bus_clock failed %ld\n",
+			PTR_ERR(pwm->bus_clk));
+		return PTR_ERR(pwm->bus_clk);
+	}
+
 	pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
 	if (IS_ERR(pwm->rst)) {
 		if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
@@ -381,6 +407,13 @@  static int sun4i_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Enable bus clock */
+	ret = clk_prepare_enable(pwm->bus_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n");
+		goto err_bus;
+	}
+
 	pwm->chip.dev = &pdev->dev;
 	pwm->chip.ops = &sun4i_pwm_ops;
 	pwm->chip.base = -1;
@@ -401,6 +434,8 @@  static int sun4i_pwm_probe(struct platform_device *pdev)
 	return 0;
 
 err_pwm_add:
+	clk_disable_unprepare(pwm->bus_clk);
+err_bus:
 	reset_control_assert(pwm->rst);
 
 	return ret;
@@ -415,6 +450,7 @@  static int sun4i_pwm_remove(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	clk_disable_unprepare(pwm->bus_clk);
 	reset_control_assert(pwm->rst);
 
 	return 0;