linux-kernel - Re: [PATCH v5 1/6] pwms: pwm-ti*: Get the clock from the PWMSS (parent)

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]

Date:	Sun, 10 Apr 2016 20:51:33 +0000 (UTC)
From:	Paul Walmsley <paul@...an.com>
To:	"Franklin S Cooper Jr." <fcooper@...com>
cc:	Sekhar Nori <nsekhar@...com>, "Kristo, Tero" <t-kristo@...com>,
	thierry.reding@...il.com, robh+dt@...nel.org, pawel.moll@....com,
	mark.rutland@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org, bcousson@...libre.com, tony@...mide.com,
	linux@....linux.org.uk, linux-pwm@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-omap@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	vigneshr@...com
Subject: Re: [PATCH v5 1/6] pwms: pwm-ti*: Get the clock from the PWMSS
 (parent)

Hi guys

On Tue, 5 Apr 2016, Franklin S Cooper Jr. wrote:

> On 04/05/2016 01:08 AM, Sekhar Nori wrote:
> > On Tuesday 08 March 2016 06:53 AM, Franklin S Cooper Jr wrote:
> > > The eCAP and ePWM doesn't have their own separate clocks. They simply
> > > utilize the clock provided directly by the PWMSS. Therefore, they simply
> > > need to grab a reference to their parent's clock.
> > >
> > > Signed-off-by: Franklin S Cooper Jr <fcooper@...com>
> >
> > So this assumes that eCAP and eHRPWM are always under the PWMSS
> > umbrella. But on TI AM18x, thats not true. These IPs exist independently
> > and receive functional clock from PLL sysclk outputs.
> >
> > > ---
> > >  drivers/pwm/pwm-tiecap.c   | 2 +-
> > >  drivers/pwm/pwm-tiehrpwm.c | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
> > > index 616af76..9418159 100644
> > > --- a/drivers/pwm/pwm-tiecap.c
> > > +++ b/drivers/pwm/pwm-tiecap.c
> > > @@ -212,7 +212,7 @@ static int ecap_pwm_probe(struct platform_device *pdev)
> > >      if (!pc)
> > >          return -ENOMEM;
> > >  
> > > -    clk = devm_clk_get(&pdev->dev, "fck");
> > > +    clk = devm_clk_get(pdev->dev.parent, "fck");
> >
> > Even keeping the AM18x usecase aside, this seems to be pushing too much
> > platform information into the driver. The "fck" is a valid connection id
> > for the eCAP IP. Whether its valid for the parent device too is not
> > something this driver should need to know.
> >
> > So it looks like what you need is for the clock hierarchy for the
> > platform to have clocks for eHRPWM and eCAP derived out of PWMSS clock?
> 
> So I believe this is a question on if we want to hide the minor
> delta between AM18 vs AM335x, AM437x and AM57x/DRA7 in the driver
> or within the DT.
> 
> Note that handling this by defining new clocks in DT will then
> result in older DTBs not working. I don't think its worth breaking
> backwards compatibility for AM335x and AM437x DTBs for fixing support
> for AM18 based SOCs. Especially since those SOCs haven't worked with
> this driver for several years. By handling things within the driver rather
> than DT we can atleast insure that we can get everything working while
> avoiding breaking backwards compatibility.

I agree with Sekhar that we shouldn't embed this parent clock quirk 
into the driver.  

Can you just define a new compatibility string such that the driver can be 
written with no embedded integration quirks?  Then add a workaround in the 
driver that will use pdev->dev.parent for the old (deprecated) 
compatibility string and log a warning to the kernel console that the DT 
needs to be updated.


- Paul