| Message ID | 1480932549-30811-2-git-send-email-bgolaszewski@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 05 December 2016 03:39 PM, Bartosz Golaszewski wrote: > The aemif clock is added twice to the lookup table in da850.c. This > breaks the children list of pll0_sysclk3 as we're using the same list > links in struct clk. When calling clk_set_rate(), we get stuck in > propagate_rate(). > > Create a separate clock for nand, inheriting the rate of the aemif > clock and retrieve it in the davinci_nand module. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > arch/arm/mach-davinci/da850.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c > index e770c97..c008e5e 100644 > --- a/arch/arm/mach-davinci/da850.c > +++ b/arch/arm/mach-davinci/da850.c > @@ -367,6 +367,11 @@ static struct clk aemif_clk = { > .flags = ALWAYS_ENABLED, > }; > > +static struct clk aemif_nand_clk = { > + .name = "nand", > + .parent = &aemif_clk, > +}; > + > static struct clk usb11_clk = { > .name = "usb11", > .parent = &pll0_sysclk4, > @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = { > CLK("da830-mmc.0", NULL, &mmcsd0_clk), > CLK("da830-mmc.1", NULL, &mmcsd1_clk), > CLK("ti-aemif", NULL, &aemif_clk), > - CLK(NULL, "aemif", &aemif_clk), > + CLK(NULL, "aemif", &aemif_nand_clk), Why use a NULL device name here? Same question was asked on v2 submission. Also, can you please make sure you are testing this in both DT mode (da850-lcdk) and non-DT boot (da850-evm). Thanks, Sekhar
2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>: > On Monday 05 December 2016 03:39 PM, Bartosz Golaszewski wrote: >> The aemif clock is added twice to the lookup table in da850.c. This >> breaks the children list of pll0_sysclk3 as we're using the same list >> links in struct clk. When calling clk_set_rate(), we get stuck in >> propagate_rate(). >> >> Create a separate clock for nand, inheriting the rate of the aemif >> clock and retrieve it in the davinci_nand module. >> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> --- >> arch/arm/mach-davinci/da850.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c >> index e770c97..c008e5e 100644 >> --- a/arch/arm/mach-davinci/da850.c >> +++ b/arch/arm/mach-davinci/da850.c >> @@ -367,6 +367,11 @@ static struct clk aemif_clk = { >> .flags = ALWAYS_ENABLED, >> }; >> >> +static struct clk aemif_nand_clk = { >> + .name = "nand", >> + .parent = &aemif_clk, >> +}; >> + >> static struct clk usb11_clk = { >> .name = "usb11", >> .parent = &pll0_sysclk4, >> @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = { >> CLK("da830-mmc.0", NULL, &mmcsd0_clk), >> CLK("da830-mmc.1", NULL, &mmcsd1_clk), >> CLK("ti-aemif", NULL, &aemif_clk), >> - CLK(NULL, "aemif", &aemif_clk), >> + CLK(NULL, "aemif", &aemif_nand_clk), > > Why use a NULL device name here? Same question was asked on v2 Eek, sorry, I missed that. > submission. Also, can you please make sure you are testing this in both > DT mode (da850-lcdk) and non-DT boot (da850-evm). Will do. Thanks, Bartosz
On Monday 05 December 2016 04:02 PM, Bartosz Golaszewski wrote: > 2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>: >> On Monday 05 December 2016 03:39 PM, Bartosz Golaszewski wrote: >>> The aemif clock is added twice to the lookup table in da850.c. This >>> breaks the children list of pll0_sysclk3 as we're using the same list >>> links in struct clk. When calling clk_set_rate(), we get stuck in >>> propagate_rate(). >>> >>> Create a separate clock for nand, inheriting the rate of the aemif >>> clock and retrieve it in the davinci_nand module. >>> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> --- >>> arch/arm/mach-davinci/da850.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c >>> index e770c97..c008e5e 100644 >>> --- a/arch/arm/mach-davinci/da850.c >>> +++ b/arch/arm/mach-davinci/da850.c >>> @@ -367,6 +367,11 @@ static struct clk aemif_clk = { >>> .flags = ALWAYS_ENABLED, >>> }; >>> >>> +static struct clk aemif_nand_clk = { >>> + .name = "nand", >>> + .parent = &aemif_clk, >>> +}; >>> + >>> static struct clk usb11_clk = { >>> .name = "usb11", >>> .parent = &pll0_sysclk4, >>> @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = { >>> CLK("da830-mmc.0", NULL, &mmcsd0_clk), >>> CLK("da830-mmc.1", NULL, &mmcsd1_clk), >>> CLK("ti-aemif", NULL, &aemif_clk), >>> - CLK(NULL, "aemif", &aemif_clk), >>> + CLK(NULL, "aemif", &aemif_nand_clk), >> >> Why use a NULL device name here? Same question was asked on v2 > > Eek, sorry, I missed that. For next version, can you also add a comment on top of 'struct clk aemif_nand_clk' explaining why its needed? Thanks, Sekhar
2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>: > On Monday 05 December 2016 03:39 PM, Bartosz Golaszewski wrote: >> The aemif clock is added twice to the lookup table in da850.c. This >> breaks the children list of pll0_sysclk3 as we're using the same list >> links in struct clk. When calling clk_set_rate(), we get stuck in >> propagate_rate(). >> >> Create a separate clock for nand, inheriting the rate of the aemif >> clock and retrieve it in the davinci_nand module. >> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> --- >> arch/arm/mach-davinci/da850.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c >> index e770c97..c008e5e 100644 >> --- a/arch/arm/mach-davinci/da850.c >> +++ b/arch/arm/mach-davinci/da850.c >> @@ -367,6 +367,11 @@ static struct clk aemif_clk = { >> .flags = ALWAYS_ENABLED, >> }; >> >> +static struct clk aemif_nand_clk = { >> + .name = "nand", >> + .parent = &aemif_clk, >> +}; >> + >> static struct clk usb11_clk = { >> .name = "usb11", >> .parent = &pll0_sysclk4, >> @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = { >> CLK("da830-mmc.0", NULL, &mmcsd0_clk), >> CLK("da830-mmc.1", NULL, &mmcsd1_clk), >> CLK("ti-aemif", NULL, &aemif_clk), >> - CLK(NULL, "aemif", &aemif_clk), >> + CLK(NULL, "aemif", &aemif_nand_clk), > > Why use a NULL device name here? Hi Sekhar, there's an issue with this bit. I added an of_dev_auxdata entry to da8xx-dt.c for the nand node, but it didn't work (the nand driver could not get the clock). When I dug deeper, it turned out, the nand node is created from aemif_probe() instead of from da850_init_machine() and the lookup table is not passed as argument to of_platform_populate(). There are two solutions: one is using "620000000.nand" as dev_id in the clock lookup table, but that's ugly. The second is leaving dev_id as NULL - I verified that the nand driver works correctly having only the connector id. Please let me know which one you prefer or if you have other ideas. Best regards, Bartosz Golaszewski
On Tuesday 06 December 2016 05:28 PM, Bartosz Golaszewski wrote: > 2016-12-05 11:15 GMT+01:00 Sekhar Nori <nsekhar@ti.com>: >> On Monday 05 December 2016 03:39 PM, Bartosz Golaszewski wrote: >>> The aemif clock is added twice to the lookup table in da850.c. This >>> breaks the children list of pll0_sysclk3 as we're using the same list >>> links in struct clk. When calling clk_set_rate(), we get stuck in >>> propagate_rate(). >>> >>> Create a separate clock for nand, inheriting the rate of the aemif >>> clock and retrieve it in the davinci_nand module. >>> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> --- >>> arch/arm/mach-davinci/da850.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c >>> index e770c97..c008e5e 100644 >>> --- a/arch/arm/mach-davinci/da850.c >>> +++ b/arch/arm/mach-davinci/da850.c >>> @@ -367,6 +367,11 @@ static struct clk aemif_clk = { >>> .flags = ALWAYS_ENABLED, >>> }; >>> >>> +static struct clk aemif_nand_clk = { >>> + .name = "nand", >>> + .parent = &aemif_clk, >>> +}; >>> + >>> static struct clk usb11_clk = { >>> .name = "usb11", >>> .parent = &pll0_sysclk4, >>> @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = { >>> CLK("da830-mmc.0", NULL, &mmcsd0_clk), >>> CLK("da830-mmc.1", NULL, &mmcsd1_clk), >>> CLK("ti-aemif", NULL, &aemif_clk), >>> - CLK(NULL, "aemif", &aemif_clk), >>> + CLK(NULL, "aemif", &aemif_nand_clk), >> >> Why use a NULL device name here? > > Hi Sekhar, > > there's an issue with this bit. I added an of_dev_auxdata entry to > da8xx-dt.c for the nand node, but it didn't work (the nand driver > could not get the clock). When I dug deeper, it turned out, the nand > node is created from aemif_probe() instead of from > da850_init_machine() and the lookup table is not passed as argument to > of_platform_populate(). > > There are two solutions: one is using "620000000.nand" as dev_id in > the clock lookup table, but that's ugly. The second is leaving dev_id > as NULL - I verified that the nand driver works correctly having only > the connector id. Please let me know which one you prefer or if you > have other ideas. Alright, I will take a look at whats going on here. This series will have to wait for v4.11 anyway. Thanks, Sekhar
On 12/05/2016 04:09 AM, Bartosz Golaszewski wrote: > The aemif clock is added twice to the lookup table in da850.c. This > breaks the children list of pll0_sysclk3 as we're using the same list > links in struct clk. When calling clk_set_rate(), we get stuck in > propagate_rate(). &emac_clk is used twice in this list as well. Shouldn't we fix it too? I would expect that it causes the same problem. > > Create a separate clock for nand, inheriting the rate of the aemif > clock and retrieve it in the davinci_nand module. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > arch/arm/mach-davinci/da850.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c > index e770c97..c008e5e 100644 > --- a/arch/arm/mach-davinci/da850.c > +++ b/arch/arm/mach-davinci/da850.c > @@ -367,6 +367,11 @@ static struct clk aemif_clk = { > .flags = ALWAYS_ENABLED, > }; > > +static struct clk aemif_nand_clk = { > + .name = "nand", > + .parent = &aemif_clk, > +}; > + > static struct clk usb11_clk = { > .name = "usb11", > .parent = &pll0_sysclk4, > @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = { > CLK("da830-mmc.0", NULL, &mmcsd0_clk), > CLK("da830-mmc.1", NULL, &mmcsd1_clk), > CLK("ti-aemif", NULL, &aemif_clk), > - CLK(NULL, "aemif", &aemif_clk), > + CLK(NULL, "aemif", &aemif_nand_clk), > CLK("ohci-da8xx", "usb11", &usb11_clk), > CLK("musb-da8xx", "usb20", &usb20_clk), > CLK("spi_davinci.0", NULL, &spi0_clk), >
On Wednesday 07 December 2016 07:24 AM, David Lechner wrote: > On 12/05/2016 04:09 AM, Bartosz Golaszewski wrote: >> The aemif clock is added twice to the lookup table in da850.c. This >> breaks the children list of pll0_sysclk3 as we're using the same list >> links in struct clk. When calling clk_set_rate(), we get stuck in >> propagate_rate(). > > &emac_clk is used twice in this list as well. Shouldn't we fix it too? I > would expect that it causes the same problem. Yes, indeed. Thanks, Sekhar
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c index e770c97..c008e5e 100644 --- a/arch/arm/mach-davinci/da850.c +++ b/arch/arm/mach-davinci/da850.c @@ -367,6 +367,11 @@ static struct clk aemif_clk = { .flags = ALWAYS_ENABLED, }; +static struct clk aemif_nand_clk = { + .name = "nand", + .parent = &aemif_clk, +}; + static struct clk usb11_clk = { .name = "usb11", .parent = &pll0_sysclk4, @@ -537,7 +542,7 @@ static struct clk_lookup da850_clks[] = { CLK("da830-mmc.0", NULL, &mmcsd0_clk), CLK("da830-mmc.1", NULL, &mmcsd1_clk), CLK("ti-aemif", NULL, &aemif_clk), - CLK(NULL, "aemif", &aemif_clk), + CLK(NULL, "aemif", &aemif_nand_clk), CLK("ohci-da8xx", "usb11", &usb11_clk), CLK("musb-da8xx", "usb20", &usb20_clk), CLK("spi_davinci.0", NULL, &spi0_clk),
The aemif clock is added twice to the lookup table in da850.c. This breaks the children list of pll0_sysclk3 as we're using the same list links in struct clk. When calling clk_set_rate(), we get stuck in propagate_rate(). Create a separate clock for nand, inheriting the rate of the aemif clock and retrieve it in the davinci_nand module. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- arch/arm/mach-davinci/da850.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)