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 > > > > > > 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 > > > > > > Cc: Philipp Zabel > > > > > > --- > > > > > > 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 you
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 > > > > > 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 > > > > > Cc: Philipp Zabel > > > > > --- > > > > > 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 i
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 > > > > 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 > > > > Cc: Philipp Zabel > > > > --- > > > > 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- |
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 > > > 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 > > > Cc: Philipp Zabel > > > --- > > > 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
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 > > 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 > > Cc: Philipp Zabel > > --- > > 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- |
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
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 > 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 > Cc: Philipp Zabel > --- > 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. Lothar Waßmann
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
On Mon, Oct 31, 2016 at 10:29:37AM +0100, Sascha Hauer wrote: > > 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. > > For reference: > > I verified on i.MX53 and i.MX25 that the ipg clock provided to the pwm > driver is not needed when accessing registers. I would have to verify > that on i.MX27 aswell, but I do not have a board handy at the moment. Also on i.MX27 disabling the PWMs ipg_clk does not disable register accesses. Sascha > > The current assumption as discussed by Philipp and me is that the ipg > clk is only needed when the pwm output is driven by the ipg clk > (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG) > > 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- | > > -- 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- |
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
Hi Sascha > On Tue, Nov 01, 2016 at 06:57:23AM +0100, Lukasz Majewski wrote: > > Hi Sascha, > > > > > The current assumption as discussed by Philipp and me is that the > > > ipg clk is only needed when the pwm output is driven by the ipg > > > clk (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG) > > > > At least on my setup (i.MX6q) the ipg clock (ipg_clk) don't need to > > be explicitly enabled in the ->apply() callback (in the pwm-imx.c) > > when MX3_PWMCR_CLKSRC_IPG (0x01 - ipg_clk) is selected as the PWM > > source. > > No. If you look in the device tree you'll see that there is no special > gateable ipg clock for the PWM. Instead the SoC ipg clock is > registered for the PWM which is not gateable. I do understand that the goal is to enable ipg clock only on demand (when we access registers) and just wanted to say that the approach with ipg enabled in dts works on my setup (and for now is sufficient). I suppose that ipg gating support for PWM will be provided in a separate patch. > > Sascha > Best regards, Łukasz Majewski pgpqQbDhKjZpq.pgp Description: OpenPGP digital signature
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
On Tue, Nov 01, 2016 at 06:57:23AM +0100, Lukasz Majewski wrote: > Hi Sascha, > > > The current assumption as discussed by Philipp and me is that the ipg > > clk is only needed when the pwm output is driven by the ipg clk > > (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG) > > At least on my setup (i.MX6q) the ipg clock (ipg_clk) don't need to be > explicitly enabled in the ->apply() callback (in the pwm-imx.c) when > MX3_PWMCR_CLKSRC_IPG (0x01 - ipg_clk) is selected as the PWM source. No. If you look in the device tree you'll see that there is no special gateable ipg clock for the PWM. Instead the SoC ipg clock is registered for the PWM which is not gateable. 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- |
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
Hi Sascha, > The current assumption as discussed by Philipp and me is that the ipg > clk is only needed when the pwm output is driven by the ipg clk > (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG) At least on my setup (i.MX6q) the ipg clock (ipg_clk) don't need to be explicitly enabled in the ->apply() callback (in the pwm-imx.c) when MX3_PWMCR_CLKSRC_IPG (0x01 - ipg_clk) is selected as the PWM source. Best regards, Łukasz Majewski pgp5t_QIQmyKK.pgp Description: OpenPGP digital signature
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
Hi Sascha, > On Mon, Oct 31, 2016 at 06:59:04AM +0100, Sascha Hauer wrote: > > On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote: > > > On Thu, 27 Oct 2016 08:29:39 +0200 > > > Lukasz Majewski wrote: > > > > > > > The code has been rewritten to remove "generic" calls to > > > > imx_pwm_{enable|disable|config}. > > > > > > > > Such approach would facilitate switch to atomic PWM (a.k.a > > > > ->apply()) implementation. > > > > > > > > Suggested-by: Stefan Agner > > > > Suggested-by: Boris Brezillon > > > > Signed-off-by: Lukasz > > > > Majewski --- > > > > Changes for v2: > > > > - Add missing clock unprepare for clk_ipg > > > > - Enable peripheral PWM clock (clk_per) > > > > --- > > > > drivers/pwm/pwm-imx.c | 50 > > > > ++ 1 file > > > > changed, 38 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > > index ea3ce79..822eb5a 100644 > > > > --- a/drivers/pwm/pwm-imx.c > > > > +++ b/drivers/pwm/pwm-imx.c > > > > @@ -65,8 +65,6 @@ struct imx_chip { > > > > static int imx_pwm_config_v1(struct pwm_chip *chip, > > > > struct pwm_device *pwm, int duty_ns, int > > > > period_ns) { > > > > - struct imx_chip *imx = to_imx_chip(chip); > > > > - > > > > /* > > > > * The PWM subsystem allows for exact frequencies. > > > > However, > > > > * I cannot connect a scope on my device to the PWM > > > > line and @@ -84,26 +82,56 @@ static int > > > > imx_pwm_config_v1(struct pwm_chip *chip, > > > > * both the prescaler (/1 .. /128) and then by CLKSEL > > > > * (/2 .. /16). > > > > */ > > > > + struct imx_chip *imx = to_imx_chip(chip); > > > > u32 max = readl(imx->mmio_base + MX1_PWMP); > > > > u32 p = max * duty_ns / period_ns; > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > > + if (ret) > > > > + return ret; > > > > + > > > > writel(max - p, imx->mmio_base + MX1_PWMS); > > > > > > > > + clk_disable_unprepare(imx->clk_ipg); > > > > + > > > > return 0; > > > > } > > > > > > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool > > > > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, > > > > struct pwm_device *pwm) { > > > > struct imx_chip *imx = to_imx_chip(chip); > > > > + int ret; > > > > u32 val; > > > > > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = clk_prepare_enable(imx->clk_per); > > > > + if (ret) > > > > + return ret; > > > > + > > > > val = readl(imx->mmio_base + MX1_PWMC); > > > > + val |= MX1_PWMC_EN; > > > > + writel(val, imx->mmio_base + MX1_PWMC); > > > > > > > > - if (enable) > > > > - val |= MX1_PWMC_EN; > > > > - else > > > > - val &= ~MX1_PWMC_EN; > > > > + clk_disable_unprepare(imx->clk_ipg); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct > > > > pwm_device *pwm) +{ > > > > + struct imx_chip *imx = to_imx_chip(chip); > > > > + u32 val; > > > > + > > > > + val = readl(imx->mmio_base + MX1_PWMC); > > > > + val &= ~MX1_PWMC_EN; > > > > > > > > writel(val, imx->mmio_base + MX1_PWMC); > > > > > > Are you sure you don't need to enable the ipg clk when > > > manipulating the PWMC register? > > > If it's not needed here, then it's probably not needed in > > > imx_pwm_enable_v1() either. > > > > 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 > > 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. > > For reference: > > I verified on i.MX53 and i.MX25 that the ipg clock provided to the pwm > driver is not needed when accessing registers. In the v3 of the
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
On Mon, Oct 31, 2016 at 06:59:04AM +0100, Sascha Hauer wrote: > On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote: > > On Thu, 27 Oct 2016 08:29:39 +0200 > > Lukasz Majewski wrote: > > > > > The code has been rewritten to remove "generic" calls to > > > imx_pwm_{enable|disable|config}. > > > > > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply()) > > > implementation. > > > > > > Suggested-by: Stefan Agner > > > Suggested-by: Boris Brezillon > > > Signed-off-by: Lukasz Majewski > > > --- > > > Changes for v2: > > > - Add missing clock unprepare for clk_ipg > > > - Enable peripheral PWM clock (clk_per) > > > --- > > > drivers/pwm/pwm-imx.c | 50 > > > ++ > > > 1 file changed, 38 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > index ea3ce79..822eb5a 100644 > > > --- a/drivers/pwm/pwm-imx.c > > > +++ b/drivers/pwm/pwm-imx.c > > > @@ -65,8 +65,6 @@ struct imx_chip { > > > static int imx_pwm_config_v1(struct pwm_chip *chip, > > > struct pwm_device *pwm, int duty_ns, int period_ns) > > > { > > > - struct imx_chip *imx = to_imx_chip(chip); > > > - > > > /* > > >* The PWM subsystem allows for exact frequencies. However, > > >* I cannot connect a scope on my device to the PWM line and > > > @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip, > > >* both the prescaler (/1 .. /128) and then by CLKSEL > > >* (/2 .. /16). > > >*/ > > > + struct imx_chip *imx = to_imx_chip(chip); > > > u32 max = readl(imx->mmio_base + MX1_PWMP); > > > u32 p = max * duty_ns / period_ns; > > > + int ret; > > > + > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > + if (ret) > > > + return ret; > > > + > > > writel(max - p, imx->mmio_base + MX1_PWMS); > > > > > > + clk_disable_unprepare(imx->clk_ipg); > > > + > > > return 0; > > > } > > > > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) > > > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device > > > *pwm) > > > { > > > struct imx_chip *imx = to_imx_chip(chip); > > > + int ret; > > > u32 val; > > > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > + if (ret) > > > + return ret; > > > + > > > + ret = clk_prepare_enable(imx->clk_per); > > > + if (ret) > > > + return ret; > > > + > > > val = readl(imx->mmio_base + MX1_PWMC); > > > + val |= MX1_PWMC_EN; > > > + writel(val, imx->mmio_base + MX1_PWMC); > > > > > > - if (enable) > > > - val |= MX1_PWMC_EN; > > > - else > > > - val &= ~MX1_PWMC_EN; > > > + clk_disable_unprepare(imx->clk_ipg); > > > + > > > + return 0; > > > +} > > > + > > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device > > > *pwm) > > > +{ > > > + struct imx_chip *imx = to_imx_chip(chip); > > > + u32 val; > > > + > > > + val = readl(imx->mmio_base + MX1_PWMC); > > > + val &= ~MX1_PWMC_EN; > > > > > > writel(val, imx->mmio_base + MX1_PWMC); > > > > Are you sure you don't need to enable the ipg clk when manipulating the > > PWMC register? > > If it's not needed here, then it's probably not needed in > > imx_pwm_enable_v1() either. > > 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 > 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. For reference: I verified on i.MX53 and i.MX25 that the ipg clock provided to the pwm driver is not needed when accessing registers. I would have to verify that on i.MX27 aswell, but I do not have a board handy at the moment. The current assumption as discussed by Philipp and me is that the ipg clk is only needed when the pwm output is driven by the ipg clk (MX3_PWMCR[16:17] = MX3_PWMCR_CLKSRC_IPG) Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildeshei
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
On Mon, Oct 31, 2016 at 09:06:00AM +0100, Lukasz Majewski wrote: > Hi Sascha, > > > On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote: > > > On Thu, 27 Oct 2016 08:29:39 +0200 > > > Lukasz Majewski wrote: > > > > > > > The code has been rewritten to remove "generic" calls to > > > > imx_pwm_{enable|disable|config}. > > > > > > > > Such approach would facilitate switch to atomic PWM (a.k.a > > > > ->apply()) implementation. > > > > > > > > Suggested-by: Stefan Agner > > > > Suggested-by: Boris Brezillon > > > > Signed-off-by: Lukasz Majewski > > > > --- > > > > Changes for v2: > > > > - Add missing clock unprepare for clk_ipg > > > > - Enable peripheral PWM clock (clk_per) > > > > --- > > > > drivers/pwm/pwm-imx.c | 50 > > > > ++ 1 file > > > > changed, 38 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > > index ea3ce79..822eb5a 100644 > > > > --- a/drivers/pwm/pwm-imx.c > > > > +++ b/drivers/pwm/pwm-imx.c > > > > @@ -65,8 +65,6 @@ struct imx_chip { > > > > static int imx_pwm_config_v1(struct pwm_chip *chip, > > > > struct pwm_device *pwm, int duty_ns, int > > > > period_ns) { > > > > - struct imx_chip *imx = to_imx_chip(chip); > > > > - > > > > /* > > > > * The PWM subsystem allows for exact frequencies. > > > > However, > > > > * I cannot connect a scope on my device to the PWM line > > > > and @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct > > > > pwm_chip *chip, > > > > * both the prescaler (/1 .. /128) and then by CLKSEL > > > > * (/2 .. /16). > > > > */ > > > > + struct imx_chip *imx = to_imx_chip(chip); > > > > u32 max = readl(imx->mmio_base + MX1_PWMP); > > > > u32 p = max * duty_ns / period_ns; > > > > + int ret; > > > > + > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > > + if (ret) > > > > + return ret; > > > > + > > > > writel(max - p, imx->mmio_base + MX1_PWMS); > > > > > > > > + clk_disable_unprepare(imx->clk_ipg); > > > > + > > > > return 0; > > > > } > > > > > > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool > > > > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, > > > > struct pwm_device *pwm) { > > > > struct imx_chip *imx = to_imx_chip(chip); > > > > + int ret; > > > > u32 val; > > > > > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = clk_prepare_enable(imx->clk_per); > > > > + if (ret) > > > > + return ret; > > > > + > > > > val = readl(imx->mmio_base + MX1_PWMC); > > > > + val |= MX1_PWMC_EN; > > > > + writel(val, imx->mmio_base + MX1_PWMC); > > > > > > > > - if (enable) > > > > - val |= MX1_PWMC_EN; > > > > - else > > > > - val &= ~MX1_PWMC_EN; > > > > + clk_disable_unprepare(imx->clk_ipg); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct > > > > pwm_device *pwm) +{ > > > > + struct imx_chip *imx = to_imx_chip(chip); > > > > + u32 val; > > > > + > > > > + val = readl(imx->mmio_base + MX1_PWMC); > > > > + val &= ~MX1_PWMC_EN; > > > > > > > > writel(val, imx->mmio_base + MX1_PWMC); > > > > > > Are you sure you don't need to enable the ipg clk when manipulating > > > the PWMC register? > > > If it's not needed here, then it's probably not needed in > > > imx_pwm_enable_v1() either. > > > > As said, even the commit 7b27c160c68 introducing the register clk did > > not enable the clock consistently for all register accesses. > > If I might ask - do you have i.MX hardware with PWMv1? If yes, I would > be grateful for testing (and provide proper patch), since I don't posses > one. PWMv1 is only found on i.MX1. While I indeed have hardware for this I don't want to spend the time to blow the dust from it and search for a PWM output pin on that. BTW. i.MX1 does not have a real ipg clock, the dts file registers the dummy clock for it. So on PWMv1 hardware the ipg clock is not needed for sure. > > > Maybe > > it's best to include the following patch so that we can find a clear > > culprit > > If we don't have HW to test the solution - why should we apply this > patch and introduce regression? The current state does not handle the ipg clock properly, it's broken already. So it's probably better to remove the inconsistent code rather than to keep it and to introduce regressions step by step and in the end leaving the question "How could this ever have worked"? > > > > If you can provide (and test) fix for v1 - please prepare patch, so it > could be added on top of this patch series (as done with pwm polarity > inversion in this patch
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
Hi Sascha, > On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote: > > On Thu, 27 Oct 2016 08:29:39 +0200 > > Lukasz Majewski wrote: > > > > > The code has been rewritten to remove "generic" calls to > > > imx_pwm_{enable|disable|config}. > > > > > > Such approach would facilitate switch to atomic PWM (a.k.a > > > ->apply()) implementation. > > > > > > Suggested-by: Stefan Agner > > > Suggested-by: Boris Brezillon > > > Signed-off-by: Lukasz Majewski > > > --- > > > Changes for v2: > > > - Add missing clock unprepare for clk_ipg > > > - Enable peripheral PWM clock (clk_per) > > > --- > > > drivers/pwm/pwm-imx.c | 50 > > > ++ 1 file > > > changed, 38 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > index ea3ce79..822eb5a 100644 > > > --- a/drivers/pwm/pwm-imx.c > > > +++ b/drivers/pwm/pwm-imx.c > > > @@ -65,8 +65,6 @@ struct imx_chip { > > > static int imx_pwm_config_v1(struct pwm_chip *chip, > > > struct pwm_device *pwm, int duty_ns, int > > > period_ns) { > > > - struct imx_chip *imx = to_imx_chip(chip); > > > - > > > /* > > >* The PWM subsystem allows for exact frequencies. > > > However, > > >* I cannot connect a scope on my device to the PWM line > > > and @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct > > > pwm_chip *chip, > > >* both the prescaler (/1 .. /128) and then by CLKSEL > > >* (/2 .. /16). > > >*/ > > > + struct imx_chip *imx = to_imx_chip(chip); > > > u32 max = readl(imx->mmio_base + MX1_PWMP); > > > u32 p = max * duty_ns / period_ns; > > > + int ret; > > > + > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > + if (ret) > > > + return ret; > > > + > > > writel(max - p, imx->mmio_base + MX1_PWMS); > > > > > > + clk_disable_unprepare(imx->clk_ipg); > > > + > > > return 0; > > > } > > > > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool > > > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, > > > struct pwm_device *pwm) { > > > struct imx_chip *imx = to_imx_chip(chip); > > > + int ret; > > > u32 val; > > > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > > + if (ret) > > > + return ret; > > > + > > > + ret = clk_prepare_enable(imx->clk_per); > > > + if (ret) > > > + return ret; > > > + > > > val = readl(imx->mmio_base + MX1_PWMC); > > > + val |= MX1_PWMC_EN; > > > + writel(val, imx->mmio_base + MX1_PWMC); > > > > > > - if (enable) > > > - val |= MX1_PWMC_EN; > > > - else > > > - val &= ~MX1_PWMC_EN; > > > + clk_disable_unprepare(imx->clk_ipg); > > > + > > > + return 0; > > > +} > > > + > > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct > > > pwm_device *pwm) +{ > > > + struct imx_chip *imx = to_imx_chip(chip); > > > + u32 val; > > > + > > > + val = readl(imx->mmio_base + MX1_PWMC); > > > + val &= ~MX1_PWMC_EN; > > > > > > writel(val, imx->mmio_base + MX1_PWMC); > > > > Are you sure you don't need to enable the ipg clk when manipulating > > the PWMC register? > > If it's not needed here, then it's probably not needed in > > imx_pwm_enable_v1() either. > > As said, even the commit 7b27c160c68 introducing the register clk did > not enable the clock consistently for all register accesses. If I might ask - do you have i.MX hardware with PWMv1? If yes, I would be grateful for testing (and provide proper patch), since I don't posses one. > Maybe > it's best to include the following patch so that we can find a clear > culprit If we don't have HW to test the solution - why should we apply this patch and introduce regression? If you can provide (and test) fix for v1 - please prepare patch, so it could be added on top of this patch series (as done with pwm polarity inversion in this patch series). > 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 > 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 > Cc: Philipp Zabel > --- > 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-
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
On Thu, Oct 27, 2016 at 09:40:05AM +0200, Boris Brezillon wrote: > On Thu, 27 Oct 2016 08:29:39 +0200 > Lukasz Majewski wrote: > > > The code has been rewritten to remove "generic" calls to > > imx_pwm_{enable|disable|config}. > > > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply()) > > implementation. > > > > Suggested-by: Stefan Agner > > Suggested-by: Boris Brezillon > > Signed-off-by: Lukasz Majewski > > --- > > Changes for v2: > > - Add missing clock unprepare for clk_ipg > > - Enable peripheral PWM clock (clk_per) > > --- > > drivers/pwm/pwm-imx.c | 50 > > ++ > > 1 file changed, 38 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > index ea3ce79..822eb5a 100644 > > --- a/drivers/pwm/pwm-imx.c > > +++ b/drivers/pwm/pwm-imx.c > > @@ -65,8 +65,6 @@ struct imx_chip { > > static int imx_pwm_config_v1(struct pwm_chip *chip, > > struct pwm_device *pwm, int duty_ns, int period_ns) > > { > > - struct imx_chip *imx = to_imx_chip(chip); > > - > > /* > > * The PWM subsystem allows for exact frequencies. However, > > * I cannot connect a scope on my device to the PWM line and > > @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip, > > * both the prescaler (/1 .. /128) and then by CLKSEL > > * (/2 .. /16). > > */ > > + struct imx_chip *imx = to_imx_chip(chip); > > u32 max = readl(imx->mmio_base + MX1_PWMP); > > u32 p = max * duty_ns / period_ns; > > + int ret; > > + > > + ret = clk_prepare_enable(imx->clk_ipg); > > + if (ret) > > + return ret; > > + > > writel(max - p, imx->mmio_base + MX1_PWMS); > > > > + clk_disable_unprepare(imx->clk_ipg); > > + > > return 0; > > } > > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) > > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm) > > { > > struct imx_chip *imx = to_imx_chip(chip); > > + int ret; > > u32 val; > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > + if (ret) > > + return ret; > > + > > + ret = clk_prepare_enable(imx->clk_per); > > + if (ret) > > + return ret; > > + > > val = readl(imx->mmio_base + MX1_PWMC); > > + val |= MX1_PWMC_EN; > > + writel(val, imx->mmio_base + MX1_PWMC); > > > > - if (enable) > > - val |= MX1_PWMC_EN; > > - else > > - val &= ~MX1_PWMC_EN; > > + clk_disable_unprepare(imx->clk_ipg); > > + > > + return 0; > > +} > > + > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device > > *pwm) > > +{ > > + struct imx_chip *imx = to_imx_chip(chip); > > + u32 val; > > + > > + val = readl(imx->mmio_base + MX1_PWMC); > > + val &= ~MX1_PWMC_EN; > > > > writel(val, imx->mmio_base + MX1_PWMC); > > Are you sure you don't need to enable the ipg clk when manipulating the > PWMC register? > If it's not needed here, then it's probably not needed in > imx_pwm_enable_v1() either. 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 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 Cc: Philipp Zabel --- 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 i
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
Hi Boris, > On Thu, 27 Oct 2016 08:29:39 +0200 > Lukasz Majewski wrote: > > > The code has been rewritten to remove "generic" calls to > > imx_pwm_{enable|disable|config}. > > > > Such approach would facilitate switch to atomic PWM (a.k.a > > ->apply()) implementation. > > > > Suggested-by: Stefan Agner > > Suggested-by: Boris Brezillon > > Signed-off-by: Lukasz Majewski > > --- > > Changes for v2: > > - Add missing clock unprepare for clk_ipg > > - Enable peripheral PWM clock (clk_per) > > --- > > drivers/pwm/pwm-imx.c | 50 > > ++ 1 file changed, > > 38 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > index ea3ce79..822eb5a 100644 > > --- a/drivers/pwm/pwm-imx.c > > +++ b/drivers/pwm/pwm-imx.c > > @@ -65,8 +65,6 @@ struct imx_chip { > > static int imx_pwm_config_v1(struct pwm_chip *chip, > > struct pwm_device *pwm, int duty_ns, int period_ns) > > { > > - struct imx_chip *imx = to_imx_chip(chip); > > - > > /* > > * The PWM subsystem allows for exact frequencies. However, > > * I cannot connect a scope on my device to the PWM line > > and @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct > > pwm_chip *chip, > > * both the prescaler (/1 .. /128) and then by CLKSEL > > * (/2 .. /16). > > */ > > + struct imx_chip *imx = to_imx_chip(chip); > > u32 max = readl(imx->mmio_base + MX1_PWMP); > > u32 p = max * duty_ns / period_ns; > > + int ret; > > + > > + ret = clk_prepare_enable(imx->clk_ipg); > > + if (ret) > > + return ret; > > + > > writel(max - p, imx->mmio_base + MX1_PWMS); > > > > + clk_disable_unprepare(imx->clk_ipg); > > + > > return 0; > > } > > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool > > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct > > pwm_device *pwm) { > > struct imx_chip *imx = to_imx_chip(chip); > > + int ret; > > u32 val; > > > > + ret = clk_prepare_enable(imx->clk_ipg); > > + if (ret) > > + return ret; > > + > > + ret = clk_prepare_enable(imx->clk_per); > > + if (ret) > > + return ret; > > + > > val = readl(imx->mmio_base + MX1_PWMC); > > + val |= MX1_PWMC_EN; > > + writel(val, imx->mmio_base + MX1_PWMC); > > > > - if (enable) > > - val |= MX1_PWMC_EN; > > - else > > - val &= ~MX1_PWMC_EN; > > + clk_disable_unprepare(imx->clk_ipg); > > + > > + return 0; > > +} > > + > > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct > > pwm_device *pwm) +{ > > + struct imx_chip *imx = to_imx_chip(chip); > > + u32 val; > > + > > + val = readl(imx->mmio_base + MX1_PWMC); > > + val &= ~MX1_PWMC_EN; > > > > writel(val, imx->mmio_base + MX1_PWMC); > > Are you sure you don't need to enable the ipg clk when manipulating > the PWMC register? > If it's not needed here, then it's probably not needed in > imx_pwm_enable_v1() either. Yes, probably it is needed. As I've mentioned in the cover letter - I do not have PWMv1 HW so I can only compile test the code. (And here support from the community is very welcome). Best regards, Łukasz Majewski > > > + > > + clk_disable_unprepare(imx->clk_per); > > } > > > > static int imx_pwm_config_v2(struct pwm_chip *chip, > > @@ -241,9 +269,9 @@ static void imx_pwm_disable(struct pwm_chip > > *chip, struct pwm_device *pwm) } > > > > static struct pwm_ops imx_pwm_ops_v1 = { > > - .enable = imx_pwm_enable, > > - .disable = imx_pwm_disable, > > - .config = imx_pwm_config, > > + .enable = imx_pwm_enable_v1, > > + .disable = imx_pwm_disable_v1, > > + .config = imx_pwm_config_v1, > > .owner = THIS_MODULE, > > }; > > > > @@ -262,8 +290,6 @@ struct imx_pwm_data { > > }; > > > > static struct imx_pwm_data imx_pwm_data_v1 = { > > - .config = imx_pwm_config_v1, > > - .set_enable = imx_pwm_set_enable_v1, > > .pwm_ops = &imx_pwm_ops_v1, > > }; > > > pgp_tkm6XTVYB.pgp Description: OpenPGP digital signature
Re: [PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
On Thu, 27 Oct 2016 08:29:39 +0200 Lukasz Majewski wrote: > The code has been rewritten to remove "generic" calls to > imx_pwm_{enable|disable|config}. > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply()) > implementation. > > Suggested-by: Stefan Agner > Suggested-by: Boris Brezillon > Signed-off-by: Lukasz Majewski > --- > Changes for v2: > - Add missing clock unprepare for clk_ipg > - Enable peripheral PWM clock (clk_per) > --- > drivers/pwm/pwm-imx.c | 50 ++ > 1 file changed, 38 insertions(+), 12 deletions(-) > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > index ea3ce79..822eb5a 100644 > --- a/drivers/pwm/pwm-imx.c > +++ b/drivers/pwm/pwm-imx.c > @@ -65,8 +65,6 @@ struct imx_chip { > static int imx_pwm_config_v1(struct pwm_chip *chip, > struct pwm_device *pwm, int duty_ns, int period_ns) > { > - struct imx_chip *imx = to_imx_chip(chip); > - > /* >* The PWM subsystem allows for exact frequencies. However, >* I cannot connect a scope on my device to the PWM line and > @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip, >* both the prescaler (/1 .. /128) and then by CLKSEL >* (/2 .. /16). >*/ > + struct imx_chip *imx = to_imx_chip(chip); > u32 max = readl(imx->mmio_base + MX1_PWMP); > u32 p = max * duty_ns / period_ns; > + int ret; > + > + ret = clk_prepare_enable(imx->clk_ipg); > + if (ret) > + return ret; > + > writel(max - p, imx->mmio_base + MX1_PWMS); > > + clk_disable_unprepare(imx->clk_ipg); > + > return 0; > } > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct imx_chip *imx = to_imx_chip(chip); > + int ret; > u32 val; > > + ret = clk_prepare_enable(imx->clk_ipg); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(imx->clk_per); > + if (ret) > + return ret; > + > val = readl(imx->mmio_base + MX1_PWMC); > + val |= MX1_PWMC_EN; > + writel(val, imx->mmio_base + MX1_PWMC); > > - if (enable) > - val |= MX1_PWMC_EN; > - else > - val &= ~MX1_PWMC_EN; > + clk_disable_unprepare(imx->clk_ipg); > + > + return 0; > +} > + > +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct imx_chip *imx = to_imx_chip(chip); > + u32 val; > + > + val = readl(imx->mmio_base + MX1_PWMC); > + val &= ~MX1_PWMC_EN; > > writel(val, imx->mmio_base + MX1_PWMC); Are you sure you don't need to enable the ipg clk when manipulating the PWMC register? If it's not needed here, then it's probably not needed in imx_pwm_enable_v1() either. > + > + clk_disable_unprepare(imx->clk_per); > } > > static int imx_pwm_config_v2(struct pwm_chip *chip, > @@ -241,9 +269,9 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct > pwm_device *pwm) > } > > static struct pwm_ops imx_pwm_ops_v1 = { > - .enable = imx_pwm_enable, > - .disable = imx_pwm_disable, > - .config = imx_pwm_config, > + .enable = imx_pwm_enable_v1, > + .disable = imx_pwm_disable_v1, > + .config = imx_pwm_config_v1, > .owner = THIS_MODULE, > }; > > @@ -262,8 +290,6 @@ struct imx_pwm_data { > }; > > static struct imx_pwm_data imx_pwm_data_v1 = { > - .config = imx_pwm_config_v1, > - .set_enable = imx_pwm_set_enable_v1, > .pwm_ops = &imx_pwm_ops_v1, > }; >
[PATCH v2 03/10] pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm operation
The code has been rewritten to remove "generic" calls to imx_pwm_{enable|disable|config}. Such approach would facilitate switch to atomic PWM (a.k.a ->apply()) implementation. Suggested-by: Stefan Agner Suggested-by: Boris Brezillon Signed-off-by: Lukasz Majewski --- Changes for v2: - Add missing clock unprepare for clk_ipg - Enable peripheral PWM clock (clk_per) --- drivers/pwm/pwm-imx.c | 50 ++ 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c index ea3ce79..822eb5a 100644 --- a/drivers/pwm/pwm-imx.c +++ b/drivers/pwm/pwm-imx.c @@ -65,8 +65,6 @@ struct imx_chip { static int imx_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) { - struct imx_chip *imx = to_imx_chip(chip); - /* * The PWM subsystem allows for exact frequencies. However, * I cannot connect a scope on my device to the PWM line and @@ -84,26 +82,56 @@ static int imx_pwm_config_v1(struct pwm_chip *chip, * both the prescaler (/1 .. /128) and then by CLKSEL * (/2 .. /16). */ + struct imx_chip *imx = to_imx_chip(chip); u32 max = readl(imx->mmio_base + MX1_PWMP); u32 p = max * duty_ns / period_ns; + int ret; + + ret = clk_prepare_enable(imx->clk_ipg); + if (ret) + return ret; + writel(max - p, imx->mmio_base + MX1_PWMS); + clk_disable_unprepare(imx->clk_ipg); + return 0; } -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm) { struct imx_chip *imx = to_imx_chip(chip); + int ret; u32 val; + ret = clk_prepare_enable(imx->clk_ipg); + if (ret) + return ret; + + ret = clk_prepare_enable(imx->clk_per); + if (ret) + return ret; + val = readl(imx->mmio_base + MX1_PWMC); + val |= MX1_PWMC_EN; + writel(val, imx->mmio_base + MX1_PWMC); - if (enable) - val |= MX1_PWMC_EN; - else - val &= ~MX1_PWMC_EN; + clk_disable_unprepare(imx->clk_ipg); + + return 0; +} + +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct imx_chip *imx = to_imx_chip(chip); + u32 val; + + val = readl(imx->mmio_base + MX1_PWMC); + val &= ~MX1_PWMC_EN; writel(val, imx->mmio_base + MX1_PWMC); + + clk_disable_unprepare(imx->clk_per); } static int imx_pwm_config_v2(struct pwm_chip *chip, @@ -241,9 +269,9 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) } static struct pwm_ops imx_pwm_ops_v1 = { - .enable = imx_pwm_enable, - .disable = imx_pwm_disable, - .config = imx_pwm_config, + .enable = imx_pwm_enable_v1, + .disable = imx_pwm_disable_v1, + .config = imx_pwm_config_v1, .owner = THIS_MODULE, }; @@ -262,8 +290,6 @@ struct imx_pwm_data { }; static struct imx_pwm_data imx_pwm_data_v1 = { - .config = imx_pwm_config_v1, - .set_enable = imx_pwm_set_enable_v1, .pwm_ops = &imx_pwm_ops_v1, }; -- 2.1.4