Re: [PATCH v3 02/11] pwm: imx: remove ipg clock

[Lukasz Majewski]

Hi Boris, Stefan,

> On Tue, 22 Nov 2016 13:04:11 -0800
> Stefan Agner <stefan@xxxxxxxx> wrote:
> 
> > On 2016-11-01 00:10, Lukasz Majewski wrote:
> > > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > > Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>  
> > 
> > I have to NACK here, sorry guys.
> > 
> > Just tested this on a i.MX 7, the kernel freezes in imx_pwm_config,
> > I guess that is where the code accesses a register first.
> > 
> > The i.MX 7 DT (imx7s.dtsi) specifies the same clock for ipg and
> > per, but it seems that this clock is crucial for register access on
> > i.MX 7:
> > 
> > clocks = <&clks IMX7D_PWM1_ROOT_CLK>,
> >          <&clks IMX7D_PWM1_ROOT_CLK>;
> > clock-names = "ipg", "per";          
> > 
> > So since the "per" clock is the same in the i.MX 7 case,
> > imx_pwm_enable worked...
> > 
> > I agree that the old code is a bit weird, especially that we get the
> > clock in imx_pwm_enable. It seems that all device trees specify a
> > "ipg" clock, so I guess we can get the clock at probe time for all
> > variants of this IP and just enable it on peripheral access...
> 
> Or, we patch the code to take the per clk before accessing PWM regs,
> and release it once we're done.
> AFAIU, the IPG clock is only supposed to be enabled when the PWM takes
> its sources from the IPG channel, 

+1

That is what TRM says about this clock.

> it has nothing to do we register
> accesses. If this is correct, then doing what you suggest implies
> abusing the IPG clk meaning.

+1

Best regards,
Åukasz Majewski

> 
> > 
> > --
> > Stefan
> > 
> > 
> > > ---
> > > [commit message text refactored by Lukasz Majewski
> > > <l.majewski@xxxxxxxxx>] ---
> > > 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;  
>

Attachment: pgpnqtStnbHEf.pgp
Description: OpenPGP digital signature