| Message ID | 1382716658-6964-6-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 25, 2013 at 10:56 AM, Tero Kristo <t-kristo@ti.com> wrote: > Some devices require their clocks to be available with a specific > dev-id con-id mapping. With DT, the clocks can be found by default > only with their name, or alternatively through the device node of > the consumer. With drivers, that don't support DT fully yet, add > mechanism to register specific clock names. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > Tested-by: Nishanth Menon <nm@ti.com> > Acked-by: Tony Lindgren <tony@atomide.com> You guys are wonderful, by the way ;) > +void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) > +{ > + struct ti_dt_clk *c; > + struct device_node *node; > + struct clk *clk; > + struct of_phandle_args clkspec; > + > + for (c = oclks; c->node_name != NULL; c++) { > + node = of_find_node_by_name(NULL, c->node_name); > + clkspec.np = node; > + clk = of_clk_get_from_provider(&clkspec); > + > + if (!IS_ERR(clk)) { > + c->lk.clk = clk; > + clkdev_add(&c->lk); > + } else { > + pr_warn("%s: failed to lookup clock node %s\n", > + __func__, c->node_name); > + } > + } > +} I have one question here, what makes this part of the patch TI-specific at all except the definition of the structure ti_dt_clk? Mapping DT clocks to generic clkdev legacy namings seems like it would be a necessary evil across *all* SoC platforms. I would say there is a good case for this being generic and used by all platforms waiting for con->id/dev->id to be moved to DT-awareness, the structure itself is very generic within clkdev as long as the drivers conformed anyway (and if they didn't, none of this helps) and I don't think it really needs to be a TI-only thing. The next thing that's going to happen when another platform arrives that wants to do the same thing is duplication or consolidation - so it would probably be a good idea to make this generic to start. While there the names could be a bit more descriptive - dt_clk_name_lookup_map_register(), struct dt_clk_name_lookup_map, DT_CLK_LOOKUP_MAP() ..? The current namespace makes it look like this is the absolute correct thing to do under all circumstances, but in fact this is purely for mapping the old way to the new, better way.. in the event a brand new platform arrives, with all new drivers (or only compliant drivers), none of this code should be implemented for it and it should be presented this way. Thanks, Matt Sealey <neko@bakuhatsu.net>
On 10/29/2013 07:50 PM, Matt Sealey wrote: > On Fri, Oct 25, 2013 at 10:56 AM, Tero Kristo <t-kristo@ti.com> wrote: >> Some devices require their clocks to be available with a specific >> dev-id con-id mapping. With DT, the clocks can be found by default >> only with their name, or alternatively through the device node of >> the consumer. With drivers, that don't support DT fully yet, add >> mechanism to register specific clock names. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> Tested-by: Nishanth Menon <nm@ti.com> >> Acked-by: Tony Lindgren <tony@atomide.com> > > You guys are wonderful, by the way ;) > >> +void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) >> +{ >> + struct ti_dt_clk *c; >> + struct device_node *node; >> + struct clk *clk; >> + struct of_phandle_args clkspec; >> + >> + for (c = oclks; c->node_name != NULL; c++) { >> + node = of_find_node_by_name(NULL, c->node_name); >> + clkspec.np = node; >> + clk = of_clk_get_from_provider(&clkspec); >> + >> + if (!IS_ERR(clk)) { >> + c->lk.clk = clk; >> + clkdev_add(&c->lk); >> + } else { >> + pr_warn("%s: failed to lookup clock node %s\n", >> + __func__, c->node_name); >> + } >> + } >> +} > > I have one question here, what makes this part of the patch > TI-specific at all except the definition of the structure ti_dt_clk? > Mapping DT clocks to generic clkdev legacy namings seems like it would > be a necessary evil across *all* SoC platforms. > > I would say there is a good case for this being generic and used by > all platforms waiting for con->id/dev->id to be moved to DT-awareness, > the structure itself is very generic within clkdev as long as the > drivers conformed anyway (and if they didn't, none of this helps) and > I don't think it really needs to be a TI-only thing. The next thing > that's going to happen when another platform arrives that wants to do > the same thing is duplication or consolidation - so it would probably > be a good idea to make this generic to start. > > While there the names could be a bit more descriptive - > dt_clk_name_lookup_map_register(), struct dt_clk_name_lookup_map, > DT_CLK_LOOKUP_MAP() ..? The current namespace makes it look like this > is the absolute correct thing to do under all circumstances, but in > fact this is purely for mapping the old way to the new, better way.. > in the event a brand new platform arrives, with all new drivers (or > only compliant drivers), none of this code should be implemented for > it and it should be presented this way. An earlier implementation of this patch (or at least a patch that tries to accomplish the same as this does, add support for legacy devices) did try to add generic solution. There was some discussion on it and I decided to drop it due to bad feedback. See: http://www.spinics.net/lists/linux-omap/msg95147.html -Tero
On Wed, Oct 30, 2013 at 3:29 AM, Tero Kristo <t-kristo@ti.com> wrote: > On 10/29/2013 07:50 PM, Matt Sealey wrote: >> >> I have one question here, what makes this part of the patch >> TI-specific at all except the definition of the structure ti_dt_clk? >> Mapping DT clocks to generic clkdev legacy namings seems like it would >> be a necessary evil across *all* SoC platforms. >> >> I would say there is a good case for this being generic > > An earlier implementation of this patch (or at least a patch that tries to > accomplish the same as this does, add support for legacy devices) did try to > add generic solution. There was some discussion on it and I decided to drop > it due to bad feedback. > > See: http://www.spinics.net/lists/linux-omap/msg95147.html I don't see anything that says this should not be generic, just that it was implemented relatively poorly at the time... I agree with Tomasz, find_by_name in device trees is basically evil (it's only supposed to be used if you already know the name because something else passed it to you, and just want it's phandle, but names CAN be ambiguous. The real fix is always pass phandles and not strings of the name property), but that's not to say this thing needs to be TI-specific.. Ah well. If it ends up getting used elsewhere, it can be quickly patched into a generic version anyway, so.. yay! I'm really interested in this all going in - but I would love to see where this is just all patched into a tree already. Pulling things out against a bunch of trees gave me more merge failures and weird happenings than I care to deal with. Is there a TI clock development tree with all this in somewhere publically accessible? I'd like to get a feel for how the DT looks and what's going on, I can't "see" it from the patches. Once it's there.. I'll spend a week or 5 doing all of the i.MX chips and you'll have a second test case for any problems :) Ta, Matt Sealey <neko@bakuhatsu.net>
On 10/30/2013 07:38 PM, Matt Sealey wrote: > On Wed, Oct 30, 2013 at 3:29 AM, Tero Kristo <t-kristo@ti.com> wrote: >> On 10/29/2013 07:50 PM, Matt Sealey wrote: >>> >>> I have one question here, what makes this part of the patch >>> TI-specific at all except the definition of the structure ti_dt_clk? >>> Mapping DT clocks to generic clkdev legacy namings seems like it would >>> be a necessary evil across *all* SoC platforms. >>> >>> I would say there is a good case for this being generic >> >> An earlier implementation of this patch (or at least a patch that tries to >> accomplish the same as this does, add support for legacy devices) did try to >> add generic solution. There was some discussion on it and I decided to drop >> it due to bad feedback. >> >> See: http://www.spinics.net/lists/linux-omap/msg95147.html > > I don't see anything that says this should not be generic, just that > it was implemented relatively poorly at the time... > > I agree with Tomasz, find_by_name in device trees is basically evil > (it's only supposed to be used if you already know the name because > something else passed it to you, and just want it's phandle, but names > CAN be ambiguous. The real fix is always pass phandles and not strings > of the name property), but that's not to say this thing needs to be > TI-specific.. > > Ah well. If it ends up getting used elsewhere, it can be quickly > patched into a generic version anyway, so.. yay! Yeah, this can be converted to generic version easily if need be. The thing with this patch is also that it is kind of temporary solution, it should not be needed anymore once all the drivers are converted to DT. > > I'm really interested in this all going in - but I would love to see > where this is just all patched into a tree already. Pulling things out > against a bunch of trees gave me more merge failures and weird > happenings than I care to deal with. Is there a TI clock development > tree with all this in somewhere publically accessible? I'd like to get > a feel for how the DT looks and what's going on, I can't "see" it from > the patches. Once it's there.. I'll spend a week or 5 doing all of the > i.MX chips and you'll have a second test case for any problems :) Did you try the fully integrated test tree I mentioned in the cover letter? tree: https://github.com/t-kristo/linux-pm.git branch: 3.12-rc6-dt-clks-v9 This has everything in it, and compiles cleanly. -Tero
Hi Matt, On Tuesday 29 of October 2013 12:50:43 Matt Sealey wrote: > On Fri, Oct 25, 2013 at 10:56 AM, Tero Kristo <t-kristo@ti.com> wrote: > > Some devices require their clocks to be available with a specific > > dev-id con-id mapping. With DT, the clocks can be found by default > > only with their name, or alternatively through the device node of > > the consumer. With drivers, that don't support DT fully yet, add > > mechanism to register specific clock names. > > > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > > Tested-by: Nishanth Menon <nm@ti.com> > > Acked-by: Tony Lindgren <tony@atomide.com> > > You guys are wonderful, by the way ;) > > > +void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) > > +{ > > + struct ti_dt_clk *c; > > + struct device_node *node; > > + struct clk *clk; > > + struct of_phandle_args clkspec; > > + > > + for (c = oclks; c->node_name != NULL; c++) { > > + node = of_find_node_by_name(NULL, c->node_name); > > + clkspec.np = node; > > + clk = of_clk_get_from_provider(&clkspec); > > + > > + if (!IS_ERR(clk)) { > > + c->lk.clk = clk; > > + clkdev_add(&c->lk); > > + } else { > > + pr_warn("%s: failed to lookup clock node > > %s\n", > > + __func__, c->node_name); > > + } > > + } > > +} > > I have one question here, what makes this part of the patch > TI-specific at all except the definition of the structure ti_dt_clk? > Mapping DT clocks to generic clkdev legacy namings seems like it would > be a necessary evil across *all* SoC platforms. SoC platforms that are fully converted to Device Tree do not need any legacy clkdev namings, because they have all clkdev look-ups performed using data from DT (see generic clock bindings). So this would be not "*all* SoC platforms", but instead "SoC platforms, which are currently undergoing conversion to DT or conversion of which is yet to be started". Still this might be a good enough justification for having a generic solution for this problem, but I would wait with this until the real need on some of such platforms shows up. I don't see anything wrong in implementing this first as TI-specific quirk and then possibly making it generic if such need shows up. By the way, we already have something like this in clk/samsung/clk.c . This is called aliases there and allows registering clkdev look-ups for particular clocks. However this is specific to our clock drivers that register all clocks inside a single clock privder, based on static ID assignment and all clocks statically listed in clock driver - see clk/samsung/clk-s3c64xx.c for an example. Best regards, Tomasz > I would say there is a good case for this being generic and used by > all platforms waiting for con->id/dev->id to be moved to DT-awareness, > the structure itself is very generic within clkdev as long as the > drivers conformed anyway (and if they didn't, none of this helps) and > I don't think it really needs to be a TI-only thing. The next thing > that's going to happen when another platform arrives that wants to do > the same thing is duplication or consolidation - so it would probably > be a good idea to make this generic to start. > > While there the names could be a bit more descriptive - > dt_clk_name_lookup_map_register(), struct dt_clk_name_lookup_map, > DT_CLK_LOOKUP_MAP() ..? The current namespace makes it look like this > is the absolute correct thing to do under all circumstances, but in > fact this is purely for mapping the old way to the new, better way.. > in the event a brand new platform arrives, with all new drivers (or > only compliant drivers), none of this code should be implemented for > it and it should be presented this way. > > Thanks, > Matt Sealey <neko@bakuhatsu.net> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sat, Nov 2, 2013 at 8:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Hi Matt, > >> I have one question here, what makes this part of the patch >> TI-specific at all except the definition of the structure ti_dt_clk? >> Mapping DT clocks to generic clkdev legacy namings seems like it would >> be a necessary evil across *all* SoC platforms. > > SoC platforms that are fully converted to Device Tree do not need any > legacy clkdev namings, because they have all clkdev look-ups performed > using data from DT (see generic clock bindings). > > So this would be not "*all* SoC platforms", but instead "SoC platforms, > which are currently undergoing conversion to DT or conversion of which is > yet to be started". That is well understood here, I have a long-running hobby project to do this for i.MX5 and i.MX6 - I have a tree from months ago where I restructured the way clocks are probed for both these (moving from mach-imx to drivers/clk and putting in the right places) and successfully moved 8 clocks (on i.MX51, 3 core PLLs, some fixed clocks which got done in the meantime upstream, and one in the CCM - the CPU freq divider) with the correct references and it all operated really well. In the meantime I had to add a bunch of weird hacks to the drivers to deal with the missing connection names, so this being generic would allow the core clock work to be finished while the drivers are brought up to the new convention. > Still this might be a good enough justification for having a generic > solution for this problem, but I would wait with this until the real need > on some of such platforms shows up. I don't see anything wrong in > implementing this first as TI-specific quirk and then possibly making it > generic if such need shows up. Indeed.. and I would like to make use of it but I'm not coding on a TI platform or a Samsung platform:) > By the way, we already have something like this in clk/samsung/clk.c . > This is called aliases there and allows registering clkdev look-ups for > particular clocks. However this is specific to our clock drivers that > register all clocks inside a single clock privder, based on static ID > assignment and all clocks statically listed in clock driver - see > clk/samsung/clk-s3c64xx.c for an example. Right. I'll keep it in mind as I do this. Thanks, this does make my life a lot easier, means I might have it working and submitted before the end of the month instead of sometime next year, I was dreading reworking all these drivers up front ;) Matt Sealey <neko@bakuhatsu.net>
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile index 93177987..05af5d8 100644 --- a/drivers/clk/ti/Makefile +++ b/drivers/clk/ti/Makefile @@ -1,3 +1,3 @@ ifneq ($(CONFIG_OF),) -obj-y += dpll.o +obj-y += clk.o dpll.o endif diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c new file mode 100644 index 0000000..ad58b01 --- /dev/null +++ b/drivers/clk/ti/clk.c @@ -0,0 +1,52 @@ +/* + * TI clock support + * + * Copyright (C) 2013 Texas Instruments, Inc. + * + * Tero Kristo <t-kristo@ti.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/clk/ti.h> +#include <linux/of.h> + +/** + * ti_dt_clocks_register - register DT duplicate clocks during boot + * @oclks: list of clocks to register + * + * Register duplicate or non-standard DT clock entries during boot. By + * default, DT clocks are found based on their node name. If any + * additional con-id / dev-id -> clock mapping is required, use this + * function to list these. + */ +void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) +{ + struct ti_dt_clk *c; + struct device_node *node; + struct clk *clk; + struct of_phandle_args clkspec; + + for (c = oclks; c->node_name != NULL; c++) { + node = of_find_node_by_name(NULL, c->node_name); + clkspec.np = node; + clk = of_clk_get_from_provider(&clkspec); + + if (!IS_ERR(clk)) { + c->lk.clk = clk; + clkdev_add(&c->lk); + } else { + pr_warn("%s: failed to lookup clock node %s\n", + __func__, c->node_name); + } + } +} diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h index c187023..9786752 100644 --- a/include/linux/clk/ti.h +++ b/include/linux/clk/ti.h @@ -15,6 +15,8 @@ #ifndef __LINUX_CLK_TI_H__ #define __LINUX_CLK_TI_H__ +#include <linux/clkdev.h> + /** * struct dpll_data - DPLL registers and integration data * @mult_div1_reg: register containing the DPLL M and N bitfields @@ -135,6 +137,25 @@ struct clk_hw_omap { /* DPLL Type and DCO Selection Flags */ #define DPLL_J_TYPE 0x1 +/** + * struct ti_dt_clk - OMAP DT clock alias declarations + * @lk: clock lookup definition + * @node_name: clock DT node to map to + */ +struct ti_dt_clk { + struct clk_lookup lk; + char *node_name; +}; + +#define DT_CLK(dev, con, name) \ + { \ + .lk = { \ + .dev_id = dev, \ + .con_id = con, \ + }, \ + .node_name = name, \ + } + void omap2_init_clk_hw_omap_clocks(struct clk *clk); int omap3_noncore_dpll_enable(struct clk_hw *hw); void omap3_noncore_dpll_disable(struct clk_hw *hw); @@ -155,6 +176,8 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw, int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate, unsigned long parent_rate); +void ti_dt_clocks_register(struct ti_dt_clk *oclks); + extern const struct clk_hw_omap_ops clkhwops_omap3_dpll; extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;