| Message ID | 1375460751-23676-2-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tero, On Friday 02 of August 2013 19:25:20 Tero Kristo wrote: > clk_get_sys / clk_get can now find clocks from device-tree. If a DT > clock is found, an entry is added to the clk_lookup list also for > subsequent searches. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > Cc: Russell King <linux@arm.linux.org.uk> > --- > drivers/clk/clkdev.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 442a313..cbeb252 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c > @@ -93,6 +93,18 @@ struct clk *of_clk_get_by_name(struct device_node > *np, const char *name) EXPORT_SYMBOL(of_clk_get_by_name); > #endif > > +/** > + * clkdev_add_nolock - add lookup entry for a clock > + * @cl: pointer to new clock lookup entry > + * > + * Non-locking version, used internally by clk_find() to add DT based > + * clock lookup entries. > + */ > +static void clkdev_add_nolock(struct clk_lookup *cl) > +{ > + list_add_tail(&cl->node, &clocks); > +} > + > /* > * Find the correct struct clk for the device and connection ID. > * We do slightly fuzzy matching here: > @@ -106,6 +118,9 @@ static struct clk_lookup *clk_find(const char > *dev_id, const char *con_id) { > struct clk_lookup *p, *cl = NULL; > int match, best_found = 0, best_possible = 0; > + struct device_node *node; > + struct clk *clk; > + struct of_phandle_args clkspec; > > if (dev_id) > best_possible += 2; > @@ -133,6 +148,24 @@ static struct clk_lookup *clk_find(const char > *dev_id, const char *con_id) break; > } > } > + > + if (cl) > + return cl; > + > + /* If clock was not found, attempt to look-up from DT */ > + node = of_find_node_by_name(NULL, con_id); Why are we introducing the "lookup by name" brokenness to the yet (mostly) sane DT world? We already have a good way of binding things together in DT, which is using phandles. Not even saying that this (or something this patch relies on) breaks the ePAPR recommendation about node naming, which states that node names should not be used to convey platform-specific data, but instead should be as generic as possible to show what kind of hardware is represented by the node. Best regards, Tomasz P.S. Added missing DT maintainers to CC. > + clkspec.np = node; > + > + clk = of_clk_get_from_provider(&clkspec); > + > + if (!IS_ERR(clk)) { > + /* We found a clock, add node to clkdev */ > + cl = clkdev_alloc(clk, con_id, dev_id); > + if (cl) > + clkdev_add_nolock(cl); > + } > + > return cl; > }
On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote: > + > + if (cl) > + return cl; > + > + /* If clock was not found, attempt to look-up from DT */ > + node = of_find_node_by_name(NULL, con_id); This is utterly broken if you're looking up purely by a connection ID. Please, go back and read clk_get()'s documentation in linux/clk.h. It's spelt out very plainly there. NAK.
On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote: > > + if (cl) > > + return cl; > > + > > + /* If clock was not found, attempt to look-up from DT */ > > + node = of_find_node_by_name(NULL, con_id); > > Why are we introducing the "lookup by name" brokenness to the yet (mostly) > sane DT world? > > We already have a good way of binding things together in DT, which is > using phandles. > > Not even saying that this (or something this patch relies on) breaks the > ePAPR recommendation about node naming, which states that node names > should not be used to convey platform-specific data, but instead should be > as generic as possible to show what kind of hardware is represented by the > node. Tell me this: many devices name their clock inputs. You can find these input names in data sheets and the like. These are the names defined by the hardware. What is wrong about using those names in DT? Remember that the second argument to clk_get() is the _clock_ _input_ _name_ on the _device_ passed in the first argument. It is not *ever* some random name of the clock producer unless someone's fscked up their use of this API (in which case they're the ones with the problem.)
Hi Russell, On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote: > On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote: > > > + if (cl) > > > + return cl; > > > + > > > + /* If clock was not found, attempt to look-up from DT */ > > > + node = of_find_node_by_name(NULL, con_id); > > > > Why are we introducing the "lookup by name" brokenness to the yet > > (mostly) sane DT world? > > > > We already have a good way of binding things together in DT, which is > > using phandles. > > > > Not even saying that this (or something this patch relies on) breaks > > the ePAPR recommendation about node naming, which states that node > > names should not be used to convey platform-specific data, but > > instead should be as generic as possible to show what kind of > > hardware is represented by the node. > > Tell me this: many devices name their clock inputs. You can find these > input names in data sheets and the like. These are the names defined > by the hardware. What is wrong about using those names in DT? Yes, clock inputs. But this is about lookup by clock name, not clock input name, as far as I can tell from the patch, based on looking for a node with given name over the whole DT. > Remember that the second argument to clk_get() is the _clock_ _input_ > _name_ on the _device_ passed in the first argument. It is not *ever* > some random name of the clock producer unless someone's fscked up their > use of this API (in which case they're the ones with the problem.) This is perfectly fine and this is how the generic common clock framework DT bindings work - clock-names property is a list of clock input names and clocks property contain specifiers of respective clocks. Best regards, Tomasz
On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote: > Hi Russell, > > On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote: > > On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote: > > > > + if (cl) > > > > + return cl; > > > > + > > > > + /* If clock was not found, attempt to look-up from DT */ > > > > + node = of_find_node_by_name(NULL, con_id); > > > > > > Why are we introducing the "lookup by name" brokenness to the yet > > > (mostly) sane DT world? > > > > > > We already have a good way of binding things together in DT, which is > > > using phandles. > > > > > > Not even saying that this (or something this patch relies on) breaks > > > the ePAPR recommendation about node naming, which states that node > > > names should not be used to convey platform-specific data, but > > > instead should be as generic as possible to show what kind of > > > hardware is represented by the node. > > > > Tell me this: many devices name their clock inputs. You can find these > > input names in data sheets and the like. These are the names defined > > by the hardware. What is wrong about using those names in DT? > > Yes, clock inputs. But this is about lookup by clock name, not clock input > name, as far as I can tell from the patch, based on looking for a node > with given name over the whole DT. "con_id" is supposed to be the clock input name when the clock API is used correctly. > > Remember that the second argument to clk_get() is the _clock_ _input_ > > _name_ on the _device_ passed in the first argument. It is not *ever* > > some random name of the clock producer unless someone's fscked up their > > use of this API (in which case they're the ones with the problem.) > > This is perfectly fine and this is how the generic common clock framework > DT bindings work - clock-names property is a list of clock input names and > clocks property contain specifiers of respective clocks. There seems to be a some pressure to do clk_get()->of_clk_get() conversions, and also to find ways to lookup clocks. It seems to me that no one understands how DT handles clocks. Maybe that's where the problem is - lack of documentation about how DT clocks are handled.
On Saturday 03 of August 2013 19:48:05 Russell King - ARM Linux wrote: > On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote: > > Hi Russell, > > > > On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote: > > > On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote: > > > > > + if (cl) > > > > > + return cl; > > > > > + > > > > > + /* If clock was not found, attempt to look-up from DT */ > > > > > + node = of_find_node_by_name(NULL, con_id); > > > > > > > > Why are we introducing the "lookup by name" brokenness to the yet > > > > (mostly) sane DT world? > > > > > > > > We already have a good way of binding things together in DT, which > > > > is > > > > using phandles. > > > > > > > > Not even saying that this (or something this patch relies on) > > > > breaks > > > > the ePAPR recommendation about node naming, which states that node > > > > names should not be used to convey platform-specific data, but > > > > instead should be as generic as possible to show what kind of > > > > hardware is represented by the node. > > > > > > Tell me this: many devices name their clock inputs. You can find > > > these > > > input names in data sheets and the like. These are the names > > > defined > > > by the hardware. What is wrong about using those names in DT? > > > > Yes, clock inputs. But this is about lookup by clock name, not clock > > input name, as far as I can tell from the patch, based on looking for > > a node with given name over the whole DT. > > "con_id" is supposed to be the clock input name when the clock API is > used correctly. Right. Still, the code is looking for a node with the same name as the supposed input name, which seems strange to me, because clock input names are supposed to be defined in consumer's node, inside clock-names property. > > > Remember that the second argument to clk_get() is the _clock_ > > > _input_ > > > _name_ on the _device_ passed in the first argument. It is not > > > *ever* > > > some random name of the clock producer unless someone's fscked up > > > their > > > use of this API (in which case they're the ones with the problem.) > > > > This is perfectly fine and this is how the generic common clock > > framework DT bindings work - clock-names property is a list of clock > > input names and clocks property contain specifiers of respective > > clocks. > > There seems to be a some pressure to do clk_get()->of_clk_get() > conversions, and also to find ways to lookup clocks. I don't really see any need for this kind of conversion. clk_get() already handles lookup using DT correctly and from drivers perspective nothing changes. > It seems to me > that no one understands how DT handles clocks. Maybe that's where the > problem is - lack of documentation about how DT clocks are handled. Hmm, there is pretty much of description and examples in Documentation/devicetree/bindings/clock/clock-bindings.txt For me it was enough to learn how DT based clock lookup works, but I'm quite used to DT, so I'm not a good test sample. Best regards, Tomasz
Hey, (Sorry was out for a short vacation, thus late reply.) On 08/03/2013 10:04 PM, Tomasz Figa wrote: > On Saturday 03 of August 2013 19:48:05 Russell King - ARM Linux wrote: >> On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote: >>> Hi Russell, >>> >>> On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote: >>>> On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote: >>>>>> + if (cl) >>>>>> + return cl; >>>>>> + >>>>>> + /* If clock was not found, attempt to look-up from DT */ >>>>>> + node = of_find_node_by_name(NULL, con_id); >>>>> >>>>> Why are we introducing the "lookup by name" brokenness to the yet >>>>> (mostly) sane DT world? >>>>> >>>>> We already have a good way of binding things together in DT, which >>>>> is >>>>> using phandles. >>>>> >>>>> Not even saying that this (or something this patch relies on) >>>>> breaks >>>>> the ePAPR recommendation about node naming, which states that node >>>>> names should not be used to convey platform-specific data, but >>>>> instead should be as generic as possible to show what kind of >>>>> hardware is represented by the node. >>>> >>>> Tell me this: many devices name their clock inputs. You can find >>>> these >>>> input names in data sheets and the like. These are the names >>>> defined >>>> by the hardware. What is wrong about using those names in DT? >>> >>> Yes, clock inputs. But this is about lookup by clock name, not clock >>> input name, as far as I can tell from the patch, based on looking for >>> a node with given name over the whole DT. >> >> "con_id" is supposed to be the clock input name when the clock API is >> used correctly. > > Right. Still, the code is looking for a node with the same name as the > supposed input name, which seems strange to me, because clock input names > are supposed to be defined in consumer's node, inside clock-names > property. > >>>> Remember that the second argument to clk_get() is the _clock_ >>>> _input_ >>>> _name_ on the _device_ passed in the first argument. It is not >>>> *ever* >>>> some random name of the clock producer unless someone's fscked up >>>> their >>>> use of this API (in which case they're the ones with the problem.) >>> >>> This is perfectly fine and this is how the generic common clock >>> framework DT bindings work - clock-names property is a list of clock >>> input names and clocks property contain specifiers of respective >>> clocks. >> >> There seems to be a some pressure to do clk_get()->of_clk_get() >> conversions, and also to find ways to lookup clocks. > > I don't really see any need for this kind of conversion. clk_get() already > handles lookup using DT correctly and from drivers perspective nothing > changes. > >> It seems to me >> that no one understands how DT handles clocks. Maybe that's where the >> problem is - lack of documentation about how DT clocks are handled. > > Hmm, there is pretty much of description and examples in > > Documentation/devicetree/bindings/clock/clock-bindings.txt > > For me it was enough to learn how DT based clock lookup works, but I'm > quite used to DT, so I'm not a good test sample. > > Best regards, > Tomasz > I think based on these comments this patch needs to go away, I will have to find another way to map the devices in and to avoid supporting the legacy mappings (hwmod depends on this heavily atm, but Rajendra posted some patches to address this.) I'll see what I can do for next rev. I might implement some tweak inside the OMAP codebase for this. -Tero
On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote: > On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote: >> + >> + if (cl) >> + return cl; >> + >> + /* If clock was not found, attempt to look-up from DT */ >> + node = of_find_node_by_name(NULL, con_id); > > This is utterly broken if you're looking up purely by a connection ID. > Please, go back and read clk_get()'s documentation in linux/clk.h. > It's spelt out very plainly there. > > NAK. > Hi Russell, After looking at this a bit more, it seems difficult to get rid of the clk_get -> of_clk_get conversion, however based on this comment I am somewhat stuck. Do you think it would be acceptable if I change the implementation of this patch to work like this: 1) both dev_id + con_id defined: a) search for device node named dev_id b) check clock-names for matching con_id c) return matching clock Example in kernel: clocksource-nomadik-mtu.c : clk_get_sys("mtu0", "apb_pclk"); ste-nomadik-snt8815.dts: mtu0: mtu@101e2000 { /* Nomadik system timer */ compatible = "st,nomadik-mtu"; reg = <0x101e2000 0x1000>; interrupt-parent = <&vica>; interrupts = <4>; clocks = <&timclk>, <&pclk>; clock-names = "timclk", "apb_pclk"; }; 2) dev_id = NULL, con_id defined (current patch implementation, most of the OMAP clocks use this approach): a) search for a device node named con_id b) return corresponding clock from the node Alternatively I must implement a regression and break some of the OMAP drivers with this set, and also implement omap internal clk_get functionality (basically adding a similar wrapper that I have implemented in this patch) under mach-omap2 to get some basic OMAP infra to work properly (namely, hwmod.) I can probably avoid part of this by adding more beef to the *.dts files for the critical parts to get boot working at least. -Tero
On Mon, Aug 26, 2013 at 05:36:15PM +0300, Tero Kristo wrote: > On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote: >> On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote: >>> + >>> + if (cl) >>> + return cl; >>> + >>> + /* If clock was not found, attempt to look-up from DT */ >>> + node = of_find_node_by_name(NULL, con_id); >> >> This is utterly broken if you're looking up purely by a connection ID. >> Please, go back and read clk_get()'s documentation in linux/clk.h. >> It's spelt out very plainly there. >> >> NAK. >> > > Hi Russell, > > After looking at this a bit more, it seems difficult to get rid of the > clk_get -> of_clk_get conversion, however based on this comment I am > somewhat stuck. Do you think it would be acceptable if I change the > implementation of this patch to work like this: Firstly, for clk_get(), you don't need to do anything - clk_get() already deals with DT, and if your DT is correct, it should already be returning the appropriate clock(s). I guess the problem here is clk_get_sys(), because that doesn't take a struct device (and it doesn't take a struct device because it's used from code where there is no struct device to use with it.) If you have an OF node, what's wrong with using of_clk_get_by_name()? If you don't have an OF node, how do you expect to find the clock in the device tree, remembering that clocks are specific to devices, and the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier.
On 08/26/2013 08:03 PM, Russell King - ARM Linux wrote: > On Mon, Aug 26, 2013 at 05:36:15PM +0300, Tero Kristo wrote: >> On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote: >>> On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote: >>>> + >>>> + if (cl) >>>> + return cl; >>>> + >>>> + /* If clock was not found, attempt to look-up from DT */ >>>> + node = of_find_node_by_name(NULL, con_id); >>> >>> This is utterly broken if you're looking up purely by a connection ID. >>> Please, go back and read clk_get()'s documentation in linux/clk.h. >>> It's spelt out very plainly there. >>> >>> NAK. >>> >> >> Hi Russell, >> >> After looking at this a bit more, it seems difficult to get rid of the >> clk_get -> of_clk_get conversion, however based on this comment I am >> somewhat stuck. Do you think it would be acceptable if I change the >> implementation of this patch to work like this: > > Firstly, for clk_get(), you don't need to do anything - clk_get() already > deals with DT, and if your DT is correct, it should already be returning > the appropriate clock(s). > > I guess the problem here is clk_get_sys(), because that doesn't take a > struct device (and it doesn't take a struct device because it's used > from code where there is no struct device to use with it.) Yeah, most of the OMAP clocks have been traditionally referenced in this way, they are using clk_get_sys and with dev=NULL. conn-id is mostly unique. > > If you have an OF node, what's wrong with using of_clk_get_by_name()? > If you don't have an OF node, how do you expect to find the clock in > the device tree, remembering that clocks are specific to devices, and > the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier. In the legacy code, pretty much all the clocks are mapped through clk_get_sys(NULL, conn), and large amount of the drivers don't even have DT conversion in place yet. I am attempting to handle the cases where we don't have OF nodes.... yet at least, without converting everything to DT, and without causing regressions (also, I don't even pretend to be capable of testing all the potential drivers for this.) Some parts of the basic infra which are using clk_get_sys are pretty silly also, like the ocp_if parts of the hwmod, I haven't figured out yet how to convert that to DT. Personally I don't mind causing a regressions here though, if I get approval for it from Tony. -Tero
* Tero Kristo <t-kristo@ti.com> [130826 11:19]: > On 08/26/2013 08:03 PM, Russell King - ARM Linux wrote: > >On Mon, Aug 26, 2013 at 05:36:15PM +0300, Tero Kristo wrote: > >>On 08/03/2013 09:31 PM, Russell King - ARM Linux wrote: > >>>On Fri, Aug 02, 2013 at 07:25:20PM +0300, Tero Kristo wrote: > >>>>+ > >>>>+ if (cl) > >>>>+ return cl; > >>>>+ > >>>>+ /* If clock was not found, attempt to look-up from DT */ > >>>>+ node = of_find_node_by_name(NULL, con_id); > >>> > >>>This is utterly broken if you're looking up purely by a connection ID. > >>>Please, go back and read clk_get()'s documentation in linux/clk.h. > >>>It's spelt out very plainly there. > >>> > >>>NAK. > >>> > >> > >>Hi Russell, > >> > >>After looking at this a bit more, it seems difficult to get rid of the > >>clk_get -> of_clk_get conversion, however based on this comment I am > >>somewhat stuck. Do you think it would be acceptable if I change the > >>implementation of this patch to work like this: > > > >Firstly, for clk_get(), you don't need to do anything - clk_get() already > >deals with DT, and if your DT is correct, it should already be returning > >the appropriate clock(s). > > > >I guess the problem here is clk_get_sys(), because that doesn't take a > >struct device (and it doesn't take a struct device because it's used > >from code where there is no struct device to use with it.) > > Yeah, most of the OMAP clocks have been traditionally referenced in > this way, they are using clk_get_sys and with dev=NULL. conn-id is > mostly unique. > > > > >If you have an OF node, what's wrong with using of_clk_get_by_name()? > >If you don't have an OF node, how do you expect to find the clock in > >the device tree, remembering that clocks are specific to devices, and > >the 2nd argument to clk_get()/clk_get_sys() is not a unique identifier. > > In the legacy code, pretty much all the clocks are mapped through > clk_get_sys(NULL, conn), and large amount of the drivers don't even > have DT conversion in place yet. I am attempting to handle the cases > where we don't have OF nodes.... yet at least, without converting > everything to DT, and without causing regressions (also, I don't > even pretend to be capable of testing all the potential drivers for > this.) Some parts of the basic infra which are using clk_get_sys are > pretty silly also, like the ocp_if parts of the hwmod, I haven't > figured out yet how to convert that to DT. > > Personally I don't mind causing a regressions here though, if I get > approval for it from Tony. No, let's not do that. We don't want to break existing drivers. How about initialize the platform driver clocks in the clock driver and set up the clock alias? Then when the driver is fixed for DT, those can be dropped one at a time. Regards, Tony
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 442a313..cbeb252 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -93,6 +93,18 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name) EXPORT_SYMBOL(of_clk_get_by_name); #endif +/** + * clkdev_add_nolock - add lookup entry for a clock + * @cl: pointer to new clock lookup entry + * + * Non-locking version, used internally by clk_find() to add DT based + * clock lookup entries. + */ +static void clkdev_add_nolock(struct clk_lookup *cl) +{ + list_add_tail(&cl->node, &clocks); +} + /* * Find the correct struct clk for the device and connection ID. * We do slightly fuzzy matching here: @@ -106,6 +118,9 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) { struct clk_lookup *p, *cl = NULL; int match, best_found = 0, best_possible = 0; + struct device_node *node; + struct clk *clk; + struct of_phandle_args clkspec; if (dev_id) best_possible += 2; @@ -133,6 +148,24 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) break; } } + + if (cl) + return cl; + + /* If clock was not found, attempt to look-up from DT */ + node = of_find_node_by_name(NULL, con_id); + + clkspec.np = node; + + clk = of_clk_get_from_provider(&clkspec); + + if (!IS_ERR(clk)) { + /* We found a clock, add node to clkdev */ + cl = clkdev_alloc(clk, con_id, dev_id); + if (cl) + clkdev_add_nolock(cl); + } + return cl; }
clk_get_sys / clk_get can now find clocks from device-tree. If a DT clock is found, an entry is added to the clk_lookup list also for subsequent searches. Signed-off-by: Tero Kristo <t-kristo@ti.com> Cc: Russell King <linux@arm.linux.org.uk> --- drivers/clk/clkdev.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)