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]

Message-ID: <570B8F51.6040108@ti.com>
Date:	Mon, 11 Apr 2016 17:19:37 +0530
From:	Sekhar Nori <nsekhar@...com>
To:	Paul Walmsley <paul@...an.com>,
	"Franklin S Cooper Jr." <fcooper@...com>
CC:	"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)

On Monday 11 April 2016 02:21 AM, Paul Walmsley wrote:
> 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.

Thanks Paul! Although not sure if adding a new compatible for the IP is
the best way (since that would denote a different version of the IP).
How about checking for parent clock iff clk_get() on own device fails
and of_machine_is_compatible() matches the platforms where backward
compatibility needs to be maintained?

Thanks,
Sekhar