| Message ID | 1477418044-24603-1-git-send-email-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 25 October 2016 11:24 PM, David Lechner wrote: > This fixes pwm name matching for DA850 familiy devices. When using device > tree, the da850_auxdata_lookup[] table caused pwm devices to have the exact > same name, which caused errors when trying to register the devices. > > The names for clock matching in da850_clks[] also have to be updated to > to exactly match in order for the clock lookup to work correctly. > > Signed-off-by: David Lechner <david@lechnology.com> Applied to "fixes" branch. Will send upstream after testing a bit and also waiting to see if anyone else has any comments. Thanks, Sekhar
Hi Sekhar, Sekhar Nori <nsekhar@ti.com> writes: > On Tuesday 25 October 2016 11:24 PM, David Lechner wrote: >> This fixes pwm name matching for DA850 familiy devices. When using device >> tree, the da850_auxdata_lookup[] table caused pwm devices to have the exact >> same name, which caused errors when trying to register the devices. >> >> The names for clock matching in da850_clks[] also have to be updated to >> to exactly match in order for the clock lookup to work correctly. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > Applied to "fixes" branch. Will send upstream after testing a bit and > also waiting to see if anyone else has any comments. I'm not seeing a fixes branch in your tree. Did you push this out? Similarily, I didn't see the updates in v4.10/defconfig either. Kevin
On Wednesday 26 October 2016 09:32 PM, Kevin Hilman wrote: > Hi Sekhar, > > Sekhar Nori <nsekhar@ti.com> writes: > >> On Tuesday 25 October 2016 11:24 PM, David Lechner wrote: >>> This fixes pwm name matching for DA850 familiy devices. When using device >>> tree, the da850_auxdata_lookup[] table caused pwm devices to have the exact >>> same name, which caused errors when trying to register the devices. >>> >>> The names for clock matching in da850_clks[] also have to be updated to >>> to exactly match in order for the clock lookup to work correctly. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >> >> Applied to "fixes" branch. Will send upstream after testing a bit and >> also waiting to see if anyone else has any comments. > > I'm not seeing a fixes branch in your tree. Did you push this out? > Similarily, I didn't see the updates in v4.10/defconfig either. Pushed now. Will push a merged master branch tomorrow after some testing. Thanks, Sekhar
On Tuesday 25 October 2016 11:24 PM, David Lechner wrote: > This fixes pwm name matching for DA850 familiy devices. When using device > tree, the da850_auxdata_lookup[] table caused pwm devices to have the exact > same name, which caused errors when trying to register the devices. > > The names for clock matching in da850_clks[] also have to be updated to > to exactly match in order for the clock lookup to work correctly. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > Tested working on LEGO MINDSTORMS EV3. > > > arch/arm/mach-davinci/da850.c | 10 +++++++--- > arch/arm/mach-davinci/da8xx-dt.c | 10 +++++----- > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c > index ed3d0e9..6b78a8f 100644 > --- a/arch/arm/mach-davinci/da850.c > +++ b/arch/arm/mach-davinci/da850.c > @@ -510,9 +510,13 @@ static struct clk_lookup da850_clks[] = { > CLK("vpif", NULL, &vpif_clk), > CLK("ahci_da850", NULL, &sata_clk), > CLK("davinci-rproc.0", NULL, &dsp_clk), > - CLK("ehrpwm", "fck", &ehrpwm_clk), > - CLK("ehrpwm", "tbclk", &ehrpwm_tbclk), > - CLK("ecap", "fck", &ecap_clk), > + CLK("ehrpwm.0", "fck", &ehrpwm_clk), > + CLK("ehrpwm.0", "tbclk", &ehrpwm_tbclk), > + CLK("ehrpwm.1", "fck", &ehrpwm_clk), > + CLK("ehrpwm.1", "tbclk", &ehrpwm_tbclk), > + CLK("ecap.0", "fck", &ecap_clk), > + CLK("ecap.1", "fck", &ecap_clk), > + CLK("ecap.2", "fck", &ecap_clk), This has exposed a limitation of DaVinci clock framework. The struct clk are stored as a linked list themselves. So a node repeating in the table above will create a loop in the linked list. This is easily seen on the LCDK board. davinci_clk_disable_unused() never returns. PWMs are unused on that board. There is no "simple" solution to this AFAICS. One solution is to separate the iterator from the clock hardware structure and use struct clk_hw available in struct clk_lookup. Or move DaVinci to common clock framework. This is of course preferred but much more involved as all 6 supported SoCs have to be moved together. Thanks, Sekhar
On 10/31/2016 05:18 AM, Sekhar Nori wrote: > On Tuesday 25 October 2016 11:24 PM, David Lechner wrote: >> This fixes pwm name matching for DA850 familiy devices. When using device >> tree, the da850_auxdata_lookup[] table caused pwm devices to have the exact >> same name, which caused errors when trying to register the devices. >> >> The names for clock matching in da850_clks[] also have to be updated to >> to exactly match in order for the clock lookup to work correctly. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> >> Tested working on LEGO MINDSTORMS EV3. >> >> >> arch/arm/mach-davinci/da850.c | 10 +++++++--- >> arch/arm/mach-davinci/da8xx-dt.c | 10 +++++----- >> 2 files changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c >> index ed3d0e9..6b78a8f 100644 >> --- a/arch/arm/mach-davinci/da850.c >> +++ b/arch/arm/mach-davinci/da850.c >> @@ -510,9 +510,13 @@ static struct clk_lookup da850_clks[] = { >> CLK("vpif", NULL, &vpif_clk), >> CLK("ahci_da850", NULL, &sata_clk), >> CLK("davinci-rproc.0", NULL, &dsp_clk), >> - CLK("ehrpwm", "fck", &ehrpwm_clk), >> - CLK("ehrpwm", "tbclk", &ehrpwm_tbclk), >> - CLK("ecap", "fck", &ecap_clk), >> + CLK("ehrpwm.0", "fck", &ehrpwm_clk), >> + CLK("ehrpwm.0", "tbclk", &ehrpwm_tbclk), >> + CLK("ehrpwm.1", "fck", &ehrpwm_clk), >> + CLK("ehrpwm.1", "tbclk", &ehrpwm_tbclk), >> + CLK("ecap.0", "fck", &ecap_clk), >> + CLK("ecap.1", "fck", &ecap_clk), >> + CLK("ecap.2", "fck", &ecap_clk), > > This has exposed a limitation of DaVinci clock framework. The struct clk > are stored as a linked list themselves. So a node repeating in the table > above will create a loop in the linked list. This is easily seen on the > LCDK board. davinci_clk_disable_unused() never returns. PWMs are unused > on that board. > > There is no "simple" solution to this AFAICS. One solution is to > separate the iterator from the clock hardware structure and use struct > clk_hw available in struct clk_lookup. > > Or move DaVinci to common clock framework. This is of course preferred > but much more involved as all 6 supported SoCs have to be moved together. > > Thanks, > Sekhar > > The simple solution for now could be to make child clocks for each of these that simply enable the parent clock. e.g. ehrpwm0_clk and ehpwm1_clk are children of ehrpwm_clk, etc. Looking at da830.c, it looks like the solution was to make multiple clocks that use the same LPSC, but this does not seem right to me.
On Monday 31 October 2016 08:18 PM, David Lechner wrote: > On 10/31/2016 05:18 AM, Sekhar Nori wrote: >> On Tuesday 25 October 2016 11:24 PM, David Lechner wrote: >>> This fixes pwm name matching for DA850 familiy devices. When using >>> device >>> tree, the da850_auxdata_lookup[] table caused pwm devices to have the >>> exact >>> same name, which caused errors when trying to register the devices. >>> >>> The names for clock matching in da850_clks[] also have to be updated to >>> to exactly match in order for the clock lookup to work correctly. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >>> --- >>> >>> Tested working on LEGO MINDSTORMS EV3. >>> >>> >>> arch/arm/mach-davinci/da850.c | 10 +++++++--- >>> arch/arm/mach-davinci/da8xx-dt.c | 10 +++++----- >>> 2 files changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/arch/arm/mach-davinci/da850.c >>> b/arch/arm/mach-davinci/da850.c >>> index ed3d0e9..6b78a8f 100644 >>> --- a/arch/arm/mach-davinci/da850.c >>> +++ b/arch/arm/mach-davinci/da850.c >>> @@ -510,9 +510,13 @@ static struct clk_lookup da850_clks[] = { >>> CLK("vpif", NULL, &vpif_clk), >>> CLK("ahci_da850", NULL, &sata_clk), >>> CLK("davinci-rproc.0", NULL, &dsp_clk), >>> - CLK("ehrpwm", "fck", &ehrpwm_clk), >>> - CLK("ehrpwm", "tbclk", &ehrpwm_tbclk), >>> - CLK("ecap", "fck", &ecap_clk), >>> + CLK("ehrpwm.0", "fck", &ehrpwm_clk), >>> + CLK("ehrpwm.0", "tbclk", &ehrpwm_tbclk), >>> + CLK("ehrpwm.1", "fck", &ehrpwm_clk), >>> + CLK("ehrpwm.1", "tbclk", &ehrpwm_tbclk), >>> + CLK("ecap.0", "fck", &ecap_clk), >>> + CLK("ecap.1", "fck", &ecap_clk), >>> + CLK("ecap.2", "fck", &ecap_clk), >> >> This has exposed a limitation of DaVinci clock framework. The struct clk >> are stored as a linked list themselves. So a node repeating in the table >> above will create a loop in the linked list. This is easily seen on the >> LCDK board. davinci_clk_disable_unused() never returns. PWMs are unused >> on that board. >> >> There is no "simple" solution to this AFAICS. One solution is to >> separate the iterator from the clock hardware structure and use struct >> clk_hw available in struct clk_lookup. >> >> Or move DaVinci to common clock framework. This is of course preferred >> but much more involved as all 6 supported SoCs have to be moved together. >> >> Thanks, >> Sekhar >> >> > > The simple solution for now could be to make child clocks for each of > these that simply enable the parent clock. e.g. ehrpwm0_clk and > ehpwm1_clk are children of ehrpwm_clk, etc. That sounds good. I saw the v2 patch you sent and it looks good. I noticed that aemif clock is repeated in da850.c too. That needs fixing. > Looking at da830.c, it looks like the solution was to make multiple > clocks that use the same LPSC, but this does not seem right to me. The PWM clock definitions there are definitely wrong. It should be fixed. Looks like its been like that forever. Thanks, Sekhar
On 11/01/2016 05:31 AM, Sekhar Nori wrote: > On Monday 31 October 2016 08:18 PM, David Lechner wrote: >> On 10/31/2016 05:18 AM, Sekhar Nori wrote: >>> On Tuesday 25 October 2016 11:24 PM, David Lechner wrote: >>>> This fixes pwm name matching for DA850 familiy devices. When using >>>> device >>>> tree, the da850_auxdata_lookup[] table caused pwm devices to have the >>>> exact >>>> same name, which caused errors when trying to register the devices. >>>> >>>> The names for clock matching in da850_clks[] also have to be updated to >>>> to exactly match in order for the clock lookup to work correctly. >>>> >>>> Signed-off-by: David Lechner <david@lechnology.com> >>>> --- >>>> >>>> Tested working on LEGO MINDSTORMS EV3. >>>> >>>> >>>> arch/arm/mach-davinci/da850.c | 10 +++++++--- >>>> arch/arm/mach-davinci/da8xx-dt.c | 10 +++++----- >>>> 2 files changed, 12 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-davinci/da850.c >>>> b/arch/arm/mach-davinci/da850.c >>>> index ed3d0e9..6b78a8f 100644 >>>> --- a/arch/arm/mach-davinci/da850.c >>>> +++ b/arch/arm/mach-davinci/da850.c >>>> @@ -510,9 +510,13 @@ static struct clk_lookup da850_clks[] = { >>>> CLK("vpif", NULL, &vpif_clk), >>>> CLK("ahci_da850", NULL, &sata_clk), >>>> CLK("davinci-rproc.0", NULL, &dsp_clk), >>>> - CLK("ehrpwm", "fck", &ehrpwm_clk), >>>> - CLK("ehrpwm", "tbclk", &ehrpwm_tbclk), >>>> - CLK("ecap", "fck", &ecap_clk), >>>> + CLK("ehrpwm.0", "fck", &ehrpwm_clk), >>>> + CLK("ehrpwm.0", "tbclk", &ehrpwm_tbclk), >>>> + CLK("ehrpwm.1", "fck", &ehrpwm_clk), >>>> + CLK("ehrpwm.1", "tbclk", &ehrpwm_tbclk), >>>> + CLK("ecap.0", "fck", &ecap_clk), >>>> + CLK("ecap.1", "fck", &ecap_clk), >>>> + CLK("ecap.2", "fck", &ecap_clk), >>> >>> This has exposed a limitation of DaVinci clock framework. The struct clk >>> are stored as a linked list themselves. So a node repeating in the table >>> above will create a loop in the linked list. This is easily seen on the >>> LCDK board. davinci_clk_disable_unused() never returns. PWMs are unused >>> on that board. >>> >>> There is no "simple" solution to this AFAICS. One solution is to >>> separate the iterator from the clock hardware structure and use struct >>> clk_hw available in struct clk_lookup. >>> >>> Or move DaVinci to common clock framework. This is of course preferred >>> but much more involved as all 6 supported SoCs have to be moved together. >>> >>> Thanks, >>> Sekhar >>> >>> >> >> The simple solution for now could be to make child clocks for each of >> these that simply enable the parent clock. e.g. ehrpwm0_clk and >> ehpwm1_clk are children of ehrpwm_clk, etc. > > That sounds good. I saw the v2 patch you sent and it looks good. I > noticed that aemif clock is repeated in da850.c too. That needs fixing. Same with emac clock. >> Looking at da830.c, it looks like the solution was to make multiple >> clocks that use the same LPSC, but this does not seem right to me. > > The PWM clock definitions there are definitely wrong. It should be > fixed. Looks like its been like that forever. > > Thanks, > Sekhar >
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c index ed3d0e9..6b78a8f 100644 --- a/arch/arm/mach-davinci/da850.c +++ b/arch/arm/mach-davinci/da850.c @@ -510,9 +510,13 @@ static struct clk_lookup da850_clks[] = { CLK("vpif", NULL, &vpif_clk), CLK("ahci_da850", NULL, &sata_clk), CLK("davinci-rproc.0", NULL, &dsp_clk), - CLK("ehrpwm", "fck", &ehrpwm_clk), - CLK("ehrpwm", "tbclk", &ehrpwm_tbclk), - CLK("ecap", "fck", &ecap_clk), + CLK("ehrpwm.0", "fck", &ehrpwm_clk), + CLK("ehrpwm.0", "tbclk", &ehrpwm_tbclk), + CLK("ehrpwm.1", "fck", &ehrpwm_clk), + CLK("ehrpwm.1", "tbclk", &ehrpwm_tbclk), + CLK("ecap.0", "fck", &ecap_clk), + CLK("ecap.1", "fck", &ecap_clk), + CLK("ecap.2", "fck", &ecap_clk), CLK(NULL, NULL, NULL), }; diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c index 7947267..6ad8ddb 100644 --- a/arch/arm/mach-davinci/da8xx-dt.c +++ b/arch/arm/mach-davinci/da8xx-dt.c @@ -23,11 +23,11 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { OF_DEV_AUXDATA("ti,davinci-i2c", 0x01e28000, "i2c_davinci.2", NULL), OF_DEV_AUXDATA("ti,davinci-wdt", 0x01c21000, "davinci-wdt", NULL), OF_DEV_AUXDATA("ti,da830-mmc", 0x01c40000, "da830-mmc.0", NULL), - OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f00000, "ehrpwm", NULL), - OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f02000, "ehrpwm", NULL), - OF_DEV_AUXDATA("ti,da850-ecap", 0x01f06000, "ecap", NULL), - OF_DEV_AUXDATA("ti,da850-ecap", 0x01f07000, "ecap", NULL), - OF_DEV_AUXDATA("ti,da850-ecap", 0x01f08000, "ecap", NULL), + OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f00000, "ehrpwm.0", NULL), + OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f02000, "ehrpwm.1", NULL), + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f06000, "ecap.0", NULL), + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f07000, "ecap.1", NULL), + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f08000, "ecap.2", NULL), OF_DEV_AUXDATA("ti,da830-spi", 0x01c41000, "spi_davinci.0", NULL), OF_DEV_AUXDATA("ti,da830-spi", 0x01f0e000, "spi_davinci.1", NULL), OF_DEV_AUXDATA("ns16550a", 0x01c42000, "serial8250.0", NULL),
This fixes pwm name matching for DA850 familiy devices. When using device tree, the da850_auxdata_lookup[] table caused pwm devices to have the exact same name, which caused errors when trying to register the devices. The names for clock matching in da850_clks[] also have to be updated to to exactly match in order for the clock lookup to work correctly. Signed-off-by: David Lechner <david@lechnology.com> --- Tested working on LEGO MINDSTORMS EV3. arch/arm/mach-davinci/da850.c | 10 +++++++--- arch/arm/mach-davinci/da8xx-dt.c | 10 +++++----- 2 files changed, 12 insertions(+), 8 deletions(-)