| Message ID | 1457400224-24797-2-git-send-email-fcooper@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Gentle ping on this On 03/07/2016 07:23 PM, 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@ti.com> > --- > 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"); > if (IS_ERR(clk)) { > dev_err(&pdev->dev, "failed to get clock\n"); > return PTR_ERR(clk); > diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c > index 6a41e66..09dc1bc 100644 > --- a/drivers/pwm/pwm-tiehrpwm.c > +++ b/drivers/pwm/pwm-tiehrpwm.c > @@ -443,7 +443,7 @@ static int ehrpwm_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"); > if (IS_ERR(clk)) { > dev_err(&pdev->dev, "failed to get clock\n"); > return PTR_ERR(clk);
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@ti.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? Thanks, Sekhar
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@ti.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. > > Thanks, > Sekhar >
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@ti.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
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@ti.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
On Mon, Apr 11, 2016 at 6:49 AM, Sekhar Nori <nsekhar@ti.com> 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 <fcooper@ti.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? New compatible strings are acceptable. Rob
On Monday 11 April 2016 06:15 PM, Rob Herring wrote: > On Mon, Apr 11, 2016 at 6:49 AM, Sekhar Nori <nsekhar@ti.com> wrote: >> On Monday 11 April 2016 02:21 AM, Paul Walmsley wrote: >>> 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@ti.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? > > New compatible strings are acceptable. Alright, thanks for clarifying that. Regards, Sekhar
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"); if (IS_ERR(clk)) { dev_err(&pdev->dev, "failed to get clock\n"); return PTR_ERR(clk); diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c index 6a41e66..09dc1bc 100644 --- a/drivers/pwm/pwm-tiehrpwm.c +++ b/drivers/pwm/pwm-tiehrpwm.c @@ -443,7 +443,7 @@ static int ehrpwm_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"); if (IS_ERR(clk)) { dev_err(&pdev->dev, "failed to get clock\n"); return PTR_ERR(clk);
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@ti.com> --- drivers/pwm/pwm-tiecap.c | 2 +- drivers/pwm/pwm-tiehrpwm.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)