Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Lukasz Majewski]

Hi Boris,

> On Tue, 3 Jan 2017 23:46:58 +0100
> Lukasz Majewski <lukma@xxxxxxx> wrote:
> 
> > > > > > >> > > > Same goes for the regression introduced in patch
> > > > > > >> > > > 2: I think it's better to keep things bisectable
> > > > > > >> > > > on all platforms (even if it appeared to work by
> > > > > > >> > > > chance on imx7, it did work before this
> > > > > > >> > > > change).      
> > > > > > >> > >
> > > > > > >> > > Could you be more specific about your idea to solve
> > > > > > >> > > this problem?      
> > > > > > >> >
> > > > > > >> > Stefan already provided a patch, I just think it
> > > > > > >> > should be fixed before patch 2 to avoid breaking
> > > > > > >> > bisectibility.      
> > > > > > >>
> > > > > > >> My idea is as follows:
> > > > > > >>
> > > > > > >> I will drop patch v2 (prepared by Sasha) and then squash
> > > > > > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable
> > > > > > >> code will be removed with other not needed code during
> > > > > > >> conversion.      
> > > > > > > 
> > > > > > > How about keeping patch 2 but enabling/disabling the
> > > > > > > periph clk in imx_pwm_config() instead of completely
> > > > > > > dropping the enable/disable clk sequence.
> > > > > > > 
> > > > > > > In patch 7 you just add the logic we talked about earlier:
> > > > > > > unconditionally enable the periph clk when entering the
> > > > > > > imx_pwm_apply_v2() function and disable it before leaving
> > > > > > > the function.
> > > > > > > 
> > > > > > > This way you can preserve bisectibility and still get rid
> > > > > > > of the ipg clk.
> > > > > > > 
> > > > > > > Stefan, what's your opinion?      
> > > > > > 
> > > > > > We will get rid of the ipg clocks anyway in patch 8 (which
> > > > > > removes those functions completely).
> > > > > > 
> > > > > > So I think Lukasz approach should be fine, just drop patch
> > > > > > 2 and squash my patch into patch 7.    
> > > > > 
> > > > > Well, the end result will be same (ipg_clk will be gone after
> > > > > patch 8), but then it's hard to track why this clock suddenly
> > > > > disappeared. I still think it's worth adding an extra commit
> > > > > explaining that enabling the per_clk before accessing IP
> > > > > registers is needed on some platforms (imx7), and that IPG
> > > > > clk is actually not required until we start using it as a
> > > > > source for the PWM signal generation.
> > > > > 
> > > > > Maybe I'm the only one to think so. In this case, feel free to
> > > > > drop patch 2.    
> > > > 
> > > > If you feel really bad about this issue, then we can drop patch
> > > > 2 and:
> > > > 
> > > > reorganize patch 7/11 to 
> > > >  - keep code, which adds imx_pwm_apply_v2() function code (just
> > > > moves it as is) 
> > > >  - remove .apply = imx_pwm_apply_v2 entry from pwm_ops
> > > > structure.
> > > > 
> > > > 
> > > > On top of it add patch to enable/disable unconditionally the
> > > > imx->clk_per clock to avoid problems on imx7 (and state them in
> > > > commit message).
> > > > 
> > > > Then we add separate patch with 
> > > > .apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new"
> > > > atomic approach.
> > > > 
> > > > And at last we apply patch 8/11, which removes the code for old
> > > > (non atomic) behaviour.
> > > > 
> > > > All the issues are documented in this way on the cost of having
> > > > "dead" (I mean not used) imx_pwm_apply_v2() for two commits.
> > > >   
> > > 
> > > This looks even more complicated.
> > > Sorry, but I don't see the problem with modifying patch 2 to
> > > enable per_clk instead of ipg_clk. Can you explain what's
> > > bothering you?  
> > 
> > But in patch 2:
> > "pwm: imx: remove ipg clock"
> > 
> >  we _remove_ the clk_ipg from imx_pwm_config() and imx_pwm_probe(),
> > so I'm quite puzzled with your above statement.
> 
> See my reworked version below.
> 
> > 
> > > 
> > > If you really want to do the change after patch 7, fine, but in
> > > this case, keep the existing logic: enable/disable ipg_clk in
> > > imx_pwm_apply_v2() until you drop the ipg_clk and replace the
> > > ipg_clk enable/disable sequence by the equivalent enable/disable
> > > per_clk one. 
> > 
> > Frankly, I do agree with Stefan here - we should drop patch 2,
> > squash all changes (including imx7 clock issues) to patch 7
> > (including verbose commit message) and remove the non-atomic code
> > in patch 8.
> 
> Hm, this is not like I'm asking something impossible here (see the
> following patch).
> 
> --->8---
> From c79bb872a40b8e322fd13f33f374fb1ba085e7a9 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Date: Mon, 26 Dec 2016 23:55:52 +0100
> Subject: [PATCH v4] pwm: imx: remove ipg clock and enable per clock
> when required
> 
> 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.
> 
> In the other hand, the imx7 IP requires the peripheral clock to be
> enabled before accessing its registers. Since ->config() can be called
> when the PWM is disabled (in which case, the peripheral clock is also
> disabled), we need to surround the imx->config() with
> clk_prepare_enable(per_clk)/clk_disable_unprepare(per_clk) calls.
> 
> Note that the imx7 IP was working fine so far because the ipg clock
> was actually pointing to the peripheral clock, which guaranteed
> peripheral clock activation even when ->config() was called when the
> PWM was disabled.
> 
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> ---
> Changes in v4:
> - Enable per clk before calling imx->config()
> 
> Changes in v3:
> - New patch
> 
>  drivers/pwm/pwm-imx.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5cd4ba..b1d1e50d3956 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;
>  
> @@ -206,13 +205,13 @@ static int imx_pwm_config(struct pwm_chip *chip,
>  	struct imx_chip *imx = to_imx_chip(chip);
>  	int ret;
>  
> -	ret = clk_prepare_enable(imx->clk_ipg);
> +	ret = clk_prepare_enable(imx->clk_per);
>  	if (ret)
>  		return ret;
>  
>  	ret = imx->config(chip, pwm, duty_ns, period_ns);
>  
> -	clk_disable_unprepare(imx->clk_ipg);
> +	clk_disable_unprepare(imx->clk_per);
>  
>  	return ret;
>  }
> @@ -293,13 +292,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;

Patch seems OK, so I will test it latter today including rework of
patch 7 (do not recalculate things when enabling PWM and the peripheral
clock availability.

Thanks Boris for preparing the patch.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@xxxxxxx