Hi Stefan, > On 2016-12-28 23:01, Lukasz Majewski wrote: > > Hi Stefan, > > > >> Hi Stefan, > >> > >> > On 2016-12-26 23:55, Lukasz Majewski wrote: > >> > > From: Sascha Hauer > >> > > > >> > > The use of the ipg clock was introduced with commit > >> > > 7b27c160c681 ("pwm: i.MX: fix clock lookup"). > >> > > 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. > >> > > >> > Hi Lukasz, > >> > > >> > Has my concern addressed in any way with this resend? > >> > https://lkml.org/lkml/2016/11/22/729 > >> > >> Unfortunately not, since I don't have iMX7 for testing. > >> > >> > > >> > Breaking hardware is usually not an option :-) > >> > >> Yes, I know, but > >> > >> Please look on the patch set from my perspective: > >> > >> I originally wanted to add polarity inversion to PWM. Then, there > >> was the request from you and Boris to go with "atomicity" support, > >> so I converted the driver to support it. > >> > >> This patch set has been resent on purpose at the end of merge > >> window, so we do have some time to fix it if it would be accepted > >> to -next tree (or any other PWM related one). Moreover, the burden > >> for preparing patches would be smaller - since we all have agreed > >> that "atomicity" is a more than welcome feature. > >> > >> > >> > > >> > I checked the i.MX 7 reference manual again, and in this case the > >> > peripheral access clock is a clock line named "ipg_clk_s" (Table > >> > 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7 > >> > all clocks are behind a single gate, so in fact it does not > >> > matter which clock we take. Given that others have peripheral > >> > access behind the "pwm" gate, I guess we should take the "pwm" > >> > gate... > >> > >> > >> If possible please prepare a patch. It would be the best solution. > > > > If I might ask - are you willing to prepare patch to fix iMX7 or > > shall I roll back to the ipg code already present in main line ? > > I doubt that just rolling back the existing code works for the new > atomic change... I guess we have to really introduce a clk enable on > each register access, also for the new atomic code. Maybe this is a way to go. > > Not sure if we should fold this change in a existing commit or create > a new patch ontop of your changes. The latter is probably easier, I would opt for creating (adding) changes on top of my changes. > but > creates a "window of brokenness" for i.MX 7 in git history. But then, > I don't really mind since it currently works more or less by chance... Hmm... it seems like Sascha patch (included to the patch series): http://patchwork.ozlabs.org/patch/708847/ is a good starting point (at least we removed ipg clock) for fixing things. > > I can prepare a patch. Thank you :-) Best regards, Łukasz Majewski > > -- > Stefan > > > > > > > Best regards, > > Łukasz Majewski > > > >> > >> Thanks in advance, > >> Łukasz Majewski > >> > >> > > >> > -- > >> > Stefan > >> > > >> > > > >> > > Signed-off-by: Sascha Hauer > >> > > Cc: Philipp Zabel > >> > > --- > >> > > [commit message text refactored by Lukasz Majewski > >> > > ] --- > >> > > Changes for v3: > >> > > - New patch > >> > > --- > >> > > 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; > >>