Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754836AbcDKMpt (ORCPT ); Mon, 11 Apr 2016 08:45:49 -0400 Received: from mail.kernel.org ([198.145.29.136]:37291 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754209AbcDKMpr (ORCPT ); Mon, 11 Apr 2016 08:45:47 -0400 MIME-Version: 1.0 In-Reply-To: <570B8F51.6040108@ti.com> References: <1457400224-24797-1-git-send-email-fcooper@ti.com> <1457400224-24797-2-git-send-email-fcooper@ti.com> <5703564C.7090700@ti.com> <5703A0C4.6010406@ti.com> <570B8F51.6040108@ti.com> From: Rob Herring Date: Mon, 11 Apr 2016 07:45:22 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 1/6] pwms: pwm-ti*: Get the clock from the PWMSS (parent) To: Sekhar Nori Cc: Paul Walmsley , "Franklin S Cooper Jr." , "Kristo, Tero" , Thierry Reding , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Benoit Cousson , Tony Lindgren , Russell King - ARM Linux , Linux PWM List , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , linux-omap , "linux-arm-kernel@lists.infradead.org" , Vignesh R Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3264 Lines: 72 On Mon, Apr 11, 2016 at 6:49 AM, Sekhar Nori wrote: > 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 >>>> >>>> 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? New compatible strings are acceptable. Rob