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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161102080645.vuqebeaoisu5t4sl@pengutronix.de>
Date:   Wed, 2 Nov 2016 09:06:45 +0100
From:   Sascha Hauer <s.hauer@...gutronix.de>
To:     Lothar Waßmann <LW@...O-electronics.de>
Cc:     Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Lukasz Majewski <l.majewski@...ess.pl>,
        linux-pwm@...r.kernel.org,
        Bhuvanchandra DV <bhuvanchandra.dv@...adex.com>,
        linux-kernel@...r.kernel.org, Stefan Agner <stefan@...er.ch>,
        Thierry Reding <thierry.reding@...il.com>,
        kernel@...gutronix.de, Fabio Estevam <fabio.estevam@....com>,
        Fabio Estevam <festevam@...il.com>,
        Philipp Zabel <pza@...gutronix.de>
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 <s.hauer@...gutronix.de>
> > > > 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 <s.hauer@...gutronix.de>
> > > > Cc: Philipp Zabel <p.zabel@...gutronix.de>
> > > > ---
> > > >  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 |