by Sascha Hauer

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Wa?mann wrote:
> Hi,
>
> On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > As said, even the commit 7b27c160c68 introducing the register clk did not
> > enable the clock consistently for all register accesses. Maybe it's best
> > to include the following patch so that we can find a clear culprit and
> > do not bury the ipg clock changes in larger patches.
> >
> > Sascha
> >
> > -----------------------------8<-----------------------------------
> >
> > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > From: Sascha Hauer <[email protected]>
> > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > Subject: [PATCH] pwm: imx: remove ipg clock
> >
> > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > commit message it was claimed that the ipg clock is enabled for register
> > accesses. This is true for the ->config() callback, but not for the
> > ->set_enable() callback. Given that the ipg clock is not consistently
> > enabled for all register accesses we can assume that either it is not
> > required at all or that the current code does not work.
> > Remove the ipg clock code for now so that it's no longer in the way of
> > refactoring the driver.
> >
> > Signed-off-by: Sascha Hauer <[email protected]>
> > Cc: Philipp Zabel <[email protected]>
> > ---
> > drivers/pwm/pwm-imx.c | 19 +------------------
> > 1 file changed, 1 insertion(+), 18 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..70609ef2 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -49,7 +49,6 @@
> >
> > struct imx_chip {
> > struct clk *clk_per;
> > - struct clk *clk_ipg;
> >
> > void __iomem *mmio_base;
> >
> > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > struct pwm_device *pwm, int duty_ns, int period_ns)
> > {
> > struct imx_chip *imx = to_imx_chip(chip);
> > - int ret;
> > -
> > - ret = clk_prepare_enable(imx->clk_ipg);
> > - if (ret)
> > - return ret;
> >
> > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> > -
> > - clk_disable_unprepare(imx->clk_ipg);
> > -
> > - return ret;
> > + return imx->config(chip, pwm, duty_ns, period_ns);
> > }
> >
> > static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > return PTR_ERR(imx->clk_per);
> > }
> >
> > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > - if (IS_ERR(imx->clk_ipg)) {
> > - dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > - PTR_ERR(imx->clk_ipg));
> > - return PTR_ERR(imx->clk_ipg);
> > - }
> > -
> > imx->chip.ops = &imx_pwm_ops;
> > imx->chip.dev = &pdev->dev;
> > imx->chip.base = -1;
> >
> If the IPG clock is not needed by the driver it should be removed from
> DT as well.

No, it's only the half truth that it's not needed. It would indeed be
needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
The driver currently doesn't do this, so it doesn't need the clock. We
should still leave the clocks in the dts files in case we decide to use
that clock later.

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2016-11-02 07:57:14

by Lothar Waßmann

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

Hi,

On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> > Hi,
> >
> > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > enable the clock consistently for all register accesses. Maybe it's best
> > > to include the following patch so that we can find a clear culprit and
> > > do not bury the ipg clock changes in larger patches.
> > >
> > > Sascha
> > >
> > > -----------------------------8<-----------------------------------
> > >
> > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > From: Sascha Hauer <[email protected]>
> > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > Subject: [PATCH] pwm: imx: remove ipg clock
> > >
> > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > commit message it was claimed that the ipg clock is enabled for register
> > > accesses. This is true for the ->config() callback, but not for the
> > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > enabled for all register accesses we can assume that either it is not
> > > required at all or that the current code does not work.
> > > Remove the ipg clock code for now so that it's no longer in the way of
> > > refactoring the driver.
> > >
> > > Signed-off-by: Sascha Hauer <[email protected]>
> > > Cc: Philipp Zabel <[email protected]>
> > > ---
> > > drivers/pwm/pwm-imx.c | 19 +------------------
> > > 1 file changed, 1 insertion(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index d600fd5..70609ef2 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -49,7 +49,6 @@
> > >
> > > struct imx_chip {
> > > struct clk *clk_per;
> > > - struct clk *clk_ipg;
> > >
> > > void __iomem *mmio_base;
> > >
> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > > struct pwm_device *pwm, int duty_ns, int period_ns)
> > > {
> > > struct imx_chip *imx = to_imx_chip(chip);
> > > - int ret;
> > > -
> > > - ret = clk_prepare_enable(imx->clk_ipg);
> > > - if (ret)
> > > - return ret;
> > >
> > > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > -
> > > - clk_disable_unprepare(imx->clk_ipg);
> > > -
> > > - return ret;
> > > + return imx->config(chip, pwm, duty_ns, period_ns);
> > > }
> > >
> > > static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > > return PTR_ERR(imx->clk_per);
> > > }
> > >
> > > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > - if (IS_ERR(imx->clk_ipg)) {
> > > - dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > - PTR_ERR(imx->clk_ipg));
> > > - return PTR_ERR(imx->clk_ipg);
> > > - }
> > > -
> > > imx->chip.ops = &imx_pwm_ops;
> > > imx->chip.dev = &pdev->dev;
> > > imx->chip.base = -1;
> > >
> > If the IPG clock is not needed by the driver it should be removed from
> > DT as well.
>
> No, it's only the half truth that it's not needed. It would indeed be
> needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
>
That's a different story!
Currently the DT specifies two clocks for the PWM:
1. register access clock (which we now know is unnecessary)
2. PWM source clock
In the case mentioned above, the IPG clock has to be specified as the
SECOND clock entry in DT, because otherwise the clock won't be
enabled/disabled as required!

Lothar Waßmann

2016-11-02 08:07:40

by Sascha Hauer

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

On Wed, Nov 02, 2016 at 08:56:20AM +0100, Lothar Wa?mann wrote:
> Hi,
>
> On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> > On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Wa?mann wrote:
> > > Hi,
> > >
> > > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > > enable the clock consistently for all register accesses. Maybe it's best
> > > > to include the following patch so that we can find a clear culprit and
> > > > do not bury the ipg clock changes in larger patches.
> > > >
> > > > Sascha
> > > >
> > > > -----------------------------8<-----------------------------------
> > > >
> > > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > > From: Sascha Hauer <[email protected]>
> > > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > > Subject: [PATCH] pwm: imx: remove ipg clock
> > > >
> > > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > > commit message it was claimed that the ipg clock is enabled for register
> > > > accesses. This is true for the ->config() callback, but not for the
> > > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > > enabled for all register accesses we can assume that either it is not
> > > > required at all or that the current code does not work.
> > > > Remove the ipg clock code for now so that it's no longer in the way of
> > > > refactoring the driver.
> > > >
> > > > Signed-off-by: Sascha Hauer <[email protected]>
> > > > Cc: Philipp Zabel <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-imx.c | 19 +------------------
> > > > 1 file changed, 1 insertion(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index d600fd5..70609ef2 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -49,7 +49,6 @@
> > > >
> > > > struct imx_chip {
> > > > struct clk *clk_per;
> > > > - struct clk *clk_ipg;
> > > >
> > > > void __iomem *mmio_base;
> > > >
> > > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > > > struct pwm_device *pwm, int duty_ns, int period_ns)
> > > > {
> > > > struct imx_chip *imx = to_imx_chip(chip);
> > > > - int ret;
> > > > -
> > > > - ret = clk_prepare_enable(imx->clk_ipg);
> > > > - if (ret)
> > > > - return ret;
> > > >
> > > > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > > -
> > > > - clk_disable_unprepare(imx->clk_ipg);
> > > > -
> > > > - return ret;
> > > > + return imx->config(chip, pwm, duty_ns, period_ns);
> > > > }
> > > >
> > > > static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > > > return PTR_ERR(imx->clk_per);
> > > > }
> > > >
> > > > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > > - if (IS_ERR(imx->clk_ipg)) {
> > > > - dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > > - PTR_ERR(imx->clk_ipg));
> > > > - return PTR_ERR(imx->clk_ipg);
> > > > - }
> > > > -
> > > > imx->chip.ops = &imx_pwm_ops;
> > > > imx->chip.dev = &pdev->dev;
> > > > imx->chip.base = -1;
> > > >
> > > If the IPG clock is not needed by the driver it should be removed from
> > > DT as well.
> >
> > No, it's only the half truth that it's not needed. It would indeed be
> > needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
> >
> That's a different story!
> Currently the DT specifies two clocks for the PWM:
> 1. register access clock (which we now know is unnecessary)
> 2. PWM source clock
> In the case mentioned above, the IPG clock has to be specified as the
> SECOND clock entry in DT, because otherwise the clock won't be
> enabled/disabled as required!

Since the driver gets its clock by name (clk_get(&pdev->dev, "per"/"ipg"))
the position in the DT doesn't matter at all.

The only thing that isn't accurate is that the "ipg" clock in the device
tree is not for register access, but itself a clock to be used as PWM
source. This is no functional problem though.

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2016-11-02 08:51:38

by Lothar Waßmann

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

Hi,

On Wed, 2 Nov 2016 09:06:45 +0100 Sascha Hauer wrote:
> On Wed, Nov 02, 2016 at 08:56:20AM +0100, Lothar Waßmann wrote:
> > Hi,
> >
> > On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> > > On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> > > > Hi,
> > > >
> > > > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > > > enable the clock consistently for all register accesses. Maybe it's best
> > > > > to include the following patch so that we can find a clear culprit and
> > > > > do not bury the ipg clock changes in larger patches.
> > > > >
> > > > > Sascha
> > > > >
> > > > > -----------------------------8<-----------------------------------
> > > > >
> > > > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > > > From: Sascha Hauer <[email protected]>
> > > > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > > > Subject: [PATCH] pwm: imx: remove ipg clock
> > > > >
> > > > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > > > commit message it was claimed that the ipg clock is enabled for register
> > > > > accesses. This is true for the ->config() callback, but not for the
> > > > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > > > enabled for all register accesses we can assume that either it is not
> > > > > required at all or that the current code does not work.
> > > > > Remove the ipg clock code for now so that it's no longer in the way of
> > > > > refactoring the driver.
> > > > >
> > > > > Signed-off-by: Sascha Hauer <[email protected]>
> > > > > Cc: Philipp Zabel <[email protected]>
> > > > > ---
> > > > > drivers/pwm/pwm-imx.c | 19 +------------------
> > > > > 1 file changed, 1 insertion(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > index d600fd5..70609ef2 100644
> > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > @@ -49,7 +49,6 @@
> > > > >
> > > > > struct imx_chip {
> > > > > struct clk *clk_per;
> > > > > - struct clk *clk_ipg;
> > > > >
> > > > > void __iomem *mmio_base;
> > > > >
> > > > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > > > > struct pwm_device *pwm, int duty_ns, int period_ns)
> > > > > {
> > > > > struct imx_chip *imx = to_imx_chip(chip);
> > > > > - int ret;
> > > > > -
> > > > > - ret = clk_prepare_enable(imx->clk_ipg);
> > > > > - if (ret)
> > > > > - return ret;
> > > > >
> > > > > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > > > -
> > > > > - clk_disable_unprepare(imx->clk_ipg);
> > > > > -
> > > > > - return ret;
> > > > > + return imx->config(chip, pwm, duty_ns, period_ns);
> > > > > }
> > > > >
> > > > > static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > > > > return PTR_ERR(imx->clk_per);
> > > > > }
> > > > >
> > > > > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > > > - if (IS_ERR(imx->clk_ipg)) {
> > > > > - dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > > > - PTR_ERR(imx->clk_ipg));
> > > > > - return PTR_ERR(imx->clk_ipg);
> > > > > - }
> > > > > -
> > > > > imx->chip.ops = &imx_pwm_ops;
> > > > > imx->chip.dev = &pdev->dev;
> > > > > imx->chip.base = -1;
> > > > >
> > > > If the IPG clock is not needed by the driver it should be removed from
> > > > DT as well.
> > >
> > > No, it's only the half truth that it's not needed. It would indeed be
> > > needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
> > >
> > That's a different story!
> > Currently the DT specifies two clocks for the PWM:
> > 1. register access clock (which we now know is unnecessary)
> > 2. PWM source clock
> > In the case mentioned above, the IPG clock has to be specified as the
> > SECOND clock entry in DT, because otherwise the clock won't be
> > enabled/disabled as required!
>
> Since the driver gets its clock by name (clk_get(&pdev->dev, "per"/"ipg"))
> the position in the DT doesn't matter at all.
>
Do you really think so?
The driver does a lookup for a clock named 'ipg' which it doesn't use
at all with your proposed patcht and a lookup for the 'per' clock which
it enables/disables whenever the PWM output is switched inactive/active.
Since the clock named 'per' is the second clock in DTB it is moot to
have the ipg clock in the first position when intending to use it as
PWM source clock!

> The only thing that isn't accurate is that the "ipg" clock in the device
> tree is not for register access, but itself a clock to be used as PWM
> source. This is no functional problem though.
>
That only happens to work accidentally, because the IPG clock will never
be switched off anyway. But it is semantically incorrect and should not
be promoted for others to copy...

Lothar Waßmann

2016-11-02 09:34:57

by Philipp Zabel

[permalink] [raw]

Subject: Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation

Hi Lothar,

Am Mittwoch, den 02.11.2016, 09:51 +0100 schrieb Lothar Waßmann:
> Hi,
>
> On Wed, 2 Nov 2016 09:06:45 +0100 Sascha Hauer wrote:
> > On Wed, Nov 02, 2016 at 08:56:20AM +0100, Lothar Waßmann wrote:
> > > Hi,
> > >
> > > On Wed, 2 Nov 2016 08:36:14 +0100 Sascha Hauer wrote:
> > > > On Wed, Nov 02, 2016 at 08:18:52AM +0100, Lothar Waßmann wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, 31 Oct 2016 06:59:04 +0100 Sascha Hauer wrote:
> > > > > > As said, even the commit 7b27c160c68 introducing the register clk did not
> > > > > > enable the clock consistently for all register accesses. Maybe it's best
> > > > > > to include the following patch so that we can find a clear culprit and
> > > > > > do not bury the ipg clock changes in larger patches.
> > > > > >
> > > > > > Sascha
> > > > > >
> > > > > > -----------------------------8<-----------------------------------
> > > > > >
> > > > > > From 30b77e83269a58c2cb5ce6de8be647e027030d34 Mon Sep 17 00:00:00 2001
> > > > > > From: Sascha Hauer <[email protected]>
> > > > > > Date: Mon, 31 Oct 2016 06:45:33 +0100
> > > > > > Subject: [PATCH] pwm: imx: remove ipg clock
> > > > > >
> > > > > > The use of the ipg clock was introduced with commit 7b27c160c6. In the
> > > > > > commit message it was claimed that the ipg clock is enabled for register
> > > > > > accesses. This is true for the ->config() callback, but not for the
> > > > > > ->set_enable() callback. Given that the ipg clock is not consistently
> > > > > > enabled for all register accesses we can assume that either it is not
> > > > > > required at all or that the current code does not work.
> > > > > > Remove the ipg clock code for now so that it's no longer in the way of
> > > > > > refactoring the driver.
> > > > > >
> > > > > > Signed-off-by: Sascha Hauer <[email protected]>
> > > > > > Cc: Philipp Zabel <[email protected]>
> > > > > > ---
> > > > > > drivers/pwm/pwm-imx.c | 19 +------------------
> > > > > > 1 file changed, 1 insertion(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > > index d600fd5..70609ef2 100644
> > > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > > @@ -49,7 +49,6 @@
> > > > > >
> > > > > > struct imx_chip {
> > > > > > struct clk *clk_per;
> > > > > > - struct clk *clk_ipg;
> > > > > >
> > > > > > void __iomem *mmio_base;
> > > > > >
> > > > > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> > > > > > struct pwm_device *pwm, int duty_ns, int period_ns)
> > > > > > {
> > > > > > struct imx_chip *imx = to_imx_chip(chip);
> > > > > > - int ret;
> > > > > > -
> > > > > > - ret = clk_prepare_enable(imx->clk_ipg);
> > > > > > - if (ret)
> > > > > > - return ret;
> > > > > >
> > > > > > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > > > > -
> > > > > > - clk_disable_unprepare(imx->clk_ipg);
> > > > > > -
> > > > > > - return ret;
> > > > > > + return imx->config(chip, pwm, duty_ns, period_ns);
> > > > > > }
> > > > > >
> > > > > > static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > > > @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > > > > > return PTR_ERR(imx->clk_per);
> > > > > > }
> > > > > >
> > > > > > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > > > > - if (IS_ERR(imx->clk_ipg)) {
> > > > > > - dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > > > > > - PTR_ERR(imx->clk_ipg));
> > > > > > - return PTR_ERR(imx->clk_ipg);
> > > > > > - }
> > > > > > -
> > > > > > imx->chip.ops = &imx_pwm_ops;
> > > > > > imx->chip.dev = &pdev->dev;
> > > > > > imx->chip.base = -1;
> > > > > >
> > > > > If the IPG clock is not needed by the driver it should be removed from
> > > > > DT as well.
> > > >
> > > > No, it's only the half truth that it's not needed. It would indeed be
> > > > needed if the driver used the ipg clock as source for the PWM (PWMCR[17:16] = 0b01).
> > > >
> > > That's a different story!
> > > Currently the DT specifies two clocks for the PWM:
> > > 1. register access clock (which we now know is unnecessary)
> > > 2. PWM source clock
> > > In the case mentioned above, the IPG clock has to be specified as the
> > > SECOND clock entry in DT, because otherwise the clock won't be
> > > enabled/disabled as required!
> >
> > Since the driver gets its clock by name (clk_get(&pdev->dev, "per"/"ipg"))
> > the position in the DT doesn't matter at all.
> >
> Do you really think so?

Could you elaborate why the position of the clock phandles in the clocks
property is an issue at all?

> The driver does a lookup for a clock named 'ipg' which it doesn't use
> at all with your proposed patcht

With the proposed patch the lookup is removed, too.

It could be added back to the driver if somebody ever has the need to
clock pwm output from the ipg_clk instead of the ipg_clk_highfreq input
to the pwm module.

> and a lookup for the 'per' clock which
> it enables/disables whenever the PWM output is switched inactive/active.
> Since the clock named 'per' is the second clock in DTB it is moot to
> have the ipg clock in the first position when intending to use it as
> PWM source clock!

Since the clock lookup is by name, the order of the clocks could indeed
be changed, but what is gained from that?

> > The only thing that isn't accurate is that the "ipg" clock in the device
> > tree is not for register access, but itself a clock to be used as PWM
> > source. This is no functional problem though.
> >
> That only happens to work accidentally, because the IPG clock will never
> be switched off anyway.

The "ipg" clock name here does not describe the global ipg root clock,
and as we just realized doesn't supply the registers. It just happens to
be the name of the "ipg_clk" input to the pwm module, which is used to
generate the pwm output if the CLKSRC field in PWMx_PWMCR[17:16] is set
to 0x1 (ipg_clk).

> But it is semantically incorrect and should not
> be promoted for others to copy...

I don't agree. If at all, we are missing documented inputs (the 32k
clock).

regards
Philipp

2016-11-03 09:34:42

by Philipp Zabel

[permalink] [raw]

Subject: Re: [PATCH v2 04/10] pwm: imx: Move PWMv2 software reset code to a separate function

Hi Lukasz,

Am Donnerstag, den 27.10.2016, 08:29 +0200 schrieb Lukasz Majewski:
> The software reset code has been extracted from imx_pwm_config_v2 function
> and moved to new one - imx_pwm_sw_reset().
>
> This change reduces the overall size of imx_pwm_config_v2() and prepares
> it for atomic PWM operation.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>

Acked-by: Philipp Zabel <[email protected]>

> ---
> Changes for v2:
> - Add missing parenthesis
> ---
> drivers/pwm/pwm-imx.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 822eb5a..8174ae6 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -134,6 +134,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> clk_disable_unprepare(imx->clk_per);
> }
>
> +static void imx_pwm_sw_reset(struct pwm_chip *chip)
> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + struct device *dev = chip->dev;
> + int wait_count = 0;
> + u32 cr;
> +
> + writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> + do {
> + usleep_range(200, 1000);
> + cr = readl(imx->mmio_base + MX3_PWMCR);
> + } while ((cr & MX3_PWMCR_SWR) &&
> + (wait_count++ < MX3_PWM_SWR_LOOP));

Not an issue for this patch, but this could be converted to use
readl_poll_timeout.

> +
> + if (cr & MX3_PWMCR_SWR)
> + dev_warn(dev, "software reset timeout\n");
> +}
> +
> +
> static int imx_pwm_config_v2(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -143,7 +162,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> unsigned long period_cycles, duty_cycles, prescale;
> unsigned int period_ms;
> bool enable = pwm_is_enabled(pwm);
> - int wait_count = 0, fifoav;
> + int fifoav;
> u32 cr, sr;
>
> /*
> @@ -166,15 +185,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> dev_warn(dev, "there is no free FIFO slot\n");
> }
> } else {
> - writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> - do {
> - usleep_range(200, 1000);
> - cr = readl(imx->mmio_base + MX3_PWMCR);
> - } while ((cr & MX3_PWMCR_SWR) &&
> - (wait_count++ < MX3_PWM_SWR_LOOP));
> -
> - if (cr & MX3_PWMCR_SWR)
> - dev_warn(dev, "software reset timeout\n");
> + imx_pwm_sw_reset(chip);
> }
>
> c = clk_get_rate(imx->clk_per);

regards
Philipp