| Message ID | 20160816223338.20776-2-kbeldan@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 17 August 2016 04:03 AM, Karl Beldan wrote: > Many davinci boards (da830 and da850 families) don't have their clocks > in DT yet and won't be successful in getting an unnamed aemif clock Actually none of DaVinci platforms have clocks in DT yet. > without explicitly registering them via clk_lookups, failing the > ti-aemif memory driver probe. I am happy with the patch itself. But I think the terminology used in the commit message can be made more accurate. clk_get() does not look up a clock by name. It looks up a clock by consumer device and a consumer id (used for multiple clocks used by same consumer device). The AEMIF clock in DaVinci has a name already. Its assigned in da850.c as "aemif". But the clock name itself does not matter in clock lookup. So, IMO, saying "won't be successful in getting an unnamed aemif clock" is inaccurate. > The current aemif lookup entry resolving to the same clock: > 'CLK(NULL, "aemif", &aemif_clk)' > remains, as it is currently used (davinci_nand is getting a named clock > "aemif"). So the existing look-up does not recognize the AEMIF as a device (NULL device name) and is using a "global" consumer id to look-up "device-less" clocks. As you noted, this entry should remain for non-DT mode and for backward compatibility. > This change will allow to switch from the mach-davinci aemif code to > the ti-aemif memory driver. > > Signed-off-by: Karl Beldan <kbeldan@baylibre.com> Thanks, Sekhar
On Wed, Aug 17, 2016 at 08:15:41PM +0530, Sekhar Nori wrote: > On Wednesday 17 August 2016 04:03 AM, Karl Beldan wrote: > > Many davinci boards (da830 and da850 families) don't have their clocks > > in DT yet and won't be successful in getting an unnamed aemif clock > > Actually none of DaVinci platforms have clocks in DT yet. > Indeed, I haven't got used to the TI platforms, I mistook some omap SoCs for davinci ones. > > without explicitly registering them via clk_lookups, failing the > > ti-aemif memory driver probe. > > I am happy with the patch itself. But I think the terminology used in > the commit message can be made more accurate. clk_get() does not look up > a clock by name. It looks up a clock by consumer device and a consumer > id (used for multiple clocks used by same consumer device). The AEMIF > clock in DaVinci has a name already. Its assigned in da850.c as "aemif". > But the clock name itself does not matter in clock lookup. > I will be happy to send a more accurate comment, here is my case for the record and the feedback, although I try to keep my distance from comments of comments ;). Checking clk_get: struct clk *clk_get(struct device *dev, const char *con_id) { [...] if (dev) { struct clk *__of_clk_get_by_name(struct device_node *np, const char *dev_id, const char *name) clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); [...] } return clk_get_sys(dev_id, con_id); } In DT case the con_id _is_ the clock name, so the assertion "clk_get() does not look up a clock by name" would be false ? Also, numerous commits refer to *clk_get(*, NULL) as getting an unnamed clock, although it only really is accurate to the point in DT cases. > So, IMO, saying "won't be successful in getting an unnamed aemif clock" > is inaccurate. > You are right, it is innacurate, because in that case it won't try getting such a clock, ie. by name. Instead it will resort to looking it up by dev_id / con_id, connecting back with your point. I will resend the series with this commit message reworded. Rgds, Karl > > The current aemif lookup entry resolving to the same clock: > > 'CLK(NULL, "aemif", &aemif_clk)' > > remains, as it is currently used (davinci_nand is getting a named clock > > "aemif"). > > So the existing look-up does not recognize the AEMIF as a device (NULL > device name) and is using a "global" consumer id to look-up > "device-less" clocks. As you noted, this entry should remain for non-DT > mode and for backward compatibility. > > > This change will allow to switch from the mach-davinci aemif code to > > the ti-aemif memory driver. > > > > Signed-off-by: Karl Beldan <kbeldan@baylibre.com> > > Thanks, > Sekhar
On Thu, Aug 18, 2016 at 07:33:19AM +0000, Karl Beldan wrote: > Checking clk_get: > > struct clk *clk_get(struct device *dev, const char *con_id) > { > [...] > if (dev) { > struct clk *__of_clk_get_by_name(struct device_node *np, > const char *dev_id, > const char *name) > clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); > [...] > } > return clk_get_sys(dev_id, con_id); > } > > In DT case the con_id _is_ the clock name, so the assertion "clk_get() > does not look up a clock by name" would be false ? Wrong. clocks = <&provider 0>, <&provider 1>; clock-names = "fck", "ick"; fck = clk_get(dev, "fck"); ick = clk_get(dev, "ick"); Works just the same. The whole point of the string is to identify an _individual_ _input_ _to_ _the_ _device_ and not a global clock name. Think what happens if you specify a clock name. Where does that name come from? What if the clock is named differently on another SoC? With that approach, we need to define a new set of APIs to allow a device to determine the global clock name for the clock that it's interested in - which is completely absurd when we've already got that within clk_get(). The whole point of the clk API is to abstract those differences. That's why it takes a _connection_ _id_ and not a clock name. I really can't understand why people keep getting this wrong in their heads. It makes no sense to me for this to take a global clock name. > Also, numerous commits refer to *clk_get(*, NULL) as getting an unnamed > clock, although it only really is accurate to the point in DT cases. Table-driven clkdev copes fine with that, and will try to locate an appropriate table entry with a NULL connection ID, not matching any non-NULL connection ID entry. In the DT case, it's always the first specified clock.
On Thu, Aug 18, 2016 at 10:04:37AM +0100, Russell King - ARM Linux wrote: > On Thu, Aug 18, 2016 at 07:33:19AM +0000, Karl Beldan wrote: > > Checking clk_get: > > > > struct clk *clk_get(struct device *dev, const char *con_id) > > { > > [...] > > if (dev) { > > struct clk *__of_clk_get_by_name(struct device_node *np, > > const char *dev_id, > > const char *name) > > clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); > > [...] > > } > > return clk_get_sys(dev_id, con_id); > > } > > > > In DT case the con_id _is_ the clock name, so the assertion "clk_get() > > does not look up a clock by name" would be false ? > > Wrong. > > clocks = <&provider 0>, <&provider 1>; > clock-names = "fck", "ick"; > > fck = clk_get(dev, "fck"); > > ick = clk_get(dev, "ick"); > > Works just the same. The whole point of the string is to identify an > _individual_ _input_ _to_ _the_ _device_ and not a global clock name. > I hope so. You are matching con_id with _a_ clock name, that's my point. Maybe my writing 'the clock name' instead of 'a clock name' made you deduce I was asserting in some way that the clock tree was addressed by some kind of a unique root namespace via the clock-names ?? It was not the case ('the' was contextual if that means anything). > Think what happens if you specify a clock name. Where does that name > come from? What if the clock is named differently on another SoC? > With that approach, we need to define a new set of APIs to allow a > device to determine the global clock name for the clock that it's > interested in - which is completely absurd when we've already got > that within clk_get(). > > The whole point of the clk API is to abstract those differences. That's > why it takes a _connection_ _id_ and not a clock name. > > I really can't understand why people keep getting this wrong in their > heads. It makes no sense to me for this to take a global clock name. > Nope, I am fine with that, it is a convenient scheme. > > Also, numerous commits refer to *clk_get(*, NULL) as getting an unnamed > > clock, although it only really is accurate to the point in DT cases. > > Table-driven clkdev copes fine with that, and will try to locate an > appropriate table entry with a NULL connection ID, not matching any > non-NULL connection ID entry. > Yes it does, my change relies on that and I was happy to be able to so. It is good to see the maintainers like Sekhar and you seizing the opportunity to throw light on such things. Karl > In the DT case, it's always the first specified clock. > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c index 0d046ac..ed3d0e9 100644 --- a/arch/arm/mach-davinci/da850.c +++ b/arch/arm/mach-davinci/da850.c @@ -501,6 +501,7 @@ static struct clk_lookup da850_clks[] = { CLK("da8xx_lcdc.0", "fck", &lcdc_clk), 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, "usb11", &usb11_clk), CLK(NULL, "usb20", &usb20_clk), diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c index ca99711..c9f7e92 100644 --- a/arch/arm/mach-davinci/da8xx-dt.c +++ b/arch/arm/mach-davinci/da8xx-dt.c @@ -37,6 +37,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1", NULL), OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", NULL), + OF_DEV_AUXDATA("ti,da850-aemif", 0x68000000, "ti-aemif", NULL), {} };
Many davinci boards (da830 and da850 families) don't have their clocks in DT yet and won't be successful in getting an unnamed aemif clock without explicitly registering them via clk_lookups, failing the ti-aemif memory driver probe. The current aemif lookup entry resolving to the same clock: 'CLK(NULL, "aemif", &aemif_clk)' remains, as it is currently used (davinci_nand is getting a named clock "aemif"). This change will allow to switch from the mach-davinci aemif code to the ti-aemif memory driver. Signed-off-by: Karl Beldan <kbeldan@baylibre.com> --- arch/arm/mach-davinci/da850.c | 1 + arch/arm/mach-davinci/da8xx-dt.c | 1 + 2 files changed, 2 insertions(+)