| Message ID | 201303031309.01118.heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/03/2013 01:08 PM, Heiko Stübner wrote: > Am Sonntag, 3. März 2013, 12:45:01 schrieb Tomasz Figa: >> On Sunday 03 of March 2013 12:17:29 Sylwester Nawrocki wrote: >>> On 03/03/2013 02:08 AM, Heiko Stübner wrote: >>>> But is there an easy way to define more than one alias? On the s3c2416 >>>> for example the hsmmc hclk is the "hsmmc" io-clock, as well as the >>>> source for the "mmc_busclk.0". Same for the "uart" pclk, that is also >>>> a baud clock source. >>> >>> This driver currently provides for only one additional clkdev lookup >>> entry per a platform clock. I pointed out this desing issue in the >>> early version of the patch set. It's because a machine clock definition >>> is coupled with a clock consumer definition. And IMO various >>> samsung_clock_register_* functions should not have >>> clk_register_clkdev() inside them. I.e. first step could be registering >>> all machine clocks and in the second one clkdev lookup entries could be >>> created. This is how most (all?) existing SoC clock drivers are >>> working. >>> >>> But those multiple aliases are important only for machines with device >>> tree support, aren't they ? >> >> I suppose you meant _without_ device tree support, right? Yes, my mistake, sorry for the confusion. > The aliases are only needed for the non-dt case. But as I think common clk > support will be a requirement for dt support in the future, similar to > pinctrl, without the correct handling of the aliases it will be hard to > incrementally convert the other platforms (i.e. s3c24xx before s3c2443, etc). Yes, indeed. That's a very valid point to have the handling of the aliases implemented correctly, not assuming it will be needed temporarily only. > For the time being I've added my own register_alias function to Thomas' core > code, hijacking the clk_table for this - attached for reference below. The patch looks good to me. It would make sense to handle all clkdev entries like this. >>> I hope this patch series gets merged early to linux-next in the 3.10 >>> cycle so the multiple accumulated fixup patches for this clock driver >>> can be merged as well and issues like that you pointed out can be >>> resolved with incremental patches. >> >> Yes, I hope so too. > > me too. Following all this still out-of-tree stuff makes me dizzy :-) Yeah, especially that it is not always clear what tag the patch series are based of off. For a long patch series like these, touching the core subsystems, it would be nice to have a corresponding git tree so it is possible to actually use and test the patches without much trouble. > Heiko > > > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c > index d36cdd5..7f1b5bc 100644 > --- a/drivers/clk/samsung/clk.c > +++ b/drivers/clk/samsung/clk.c > @@ -57,14 +57,15 @@ void __init samsung_clk_init(struct device_node *np, void __iomem *base, > unsigned long nr_rdump) > { > reg_base = base; > - if (!np) > - return; > > -#ifdef CONFIG_OF > clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL); > if (!clk_table) > panic("could not allocate clock lookup table\n"); > > + if (!np) > + return; > + > +#ifdef CONFIG_OF > clk_data.clks = clk_table; > clk_data.clk_num = nr_clks; > of_clk_add_provider(np, of_clk_src_onecell_get,&clk_data); > @@ -96,6 +97,46 @@ void samsung_clk_add_lookup(struct clk *clk, unsigned int id) > clk_table[id] = clk; > } > > +/* register a list of aliases */ > +void __init samsung_clk_register_alias(struct samsung_clock_alias *list, > + unsigned int nr_clk) > +{ > + struct clk *clk; > + unsigned int idx, ret; > + > + if (!clk_table) { > + pr_err("%s: clock table missing\n", __func__); > + return; > + } > + > + for (idx = 0; idx< nr_clk; idx++, list++) { > + if (!list->id) { > + pr_err("%s: clock id missing for index %d\n", __func__, > + idx); > + continue; > + } > + > + clk = clk_table[list->id]; > + if (!clk) { > + pr_err("%s: failed to find clock %d\n", __func__, > + list->id); > + continue; > + } > + > + /* register a clock lookup only if a clock alias is specified */ > + if (!list->alias) { > + pr_err("%s: no alias defined for clock %d\n", __func__, > + list->id); I wouldn't print that error in general. It might be a clock with NULL conn_id. It's not an error condition. > + continue; > + } > + > + ret = clk_register_clkdev(clk, list->alias, list->dev_name); > + if (ret) > + pr_err("%s: failed to register lookup %s\n", > + __func__, list->alias); > + } > +} > + > /* register a list of fixed clocks */ > void __init samsung_clk_register_fixed_rate( > struct samsung_fixed_rate_clock *list, unsigned int nr_clk) > diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h > index 175a9d0..8be9248 100644 > --- a/drivers/clk/samsung/clk.h > +++ b/drivers/clk/samsung/clk.h > @@ -23,6 +23,25 @@ > #include<mach/map.h> > > /** > + * struct samsung_clock_alias: information about mux clock > + * @id: platform specific id of the clock. > + * @dev_name: name of the device to which this clock belongs. > + * @alias: optional clock alias name to be assigned to this clock. > + */ > +struct samsung_clock_alias { > + unsigned int id; > + const char *dev_name; > + const char *alias; > +}; > + > +#define ALIAS(_id, dname, a) \ > + { \ > + .id = _id, \ > + .dev_name = dname, \ > + .alias = a, \ > + } > + > +/** > * struct samsung_fixed_rate_clock: information about fixed-rate clock > * @id: platform specific id of the clock. > * @name: name of this fixed-rate clock. > @@ -260,6 +282,8 @@ extern void __init samsung_clk_of_register_fixed_ext( > > extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id); > > +extern void samsung_clk_register_alias(struct samsung_clock_alias *list, > + unsigned int nr_clk); > extern void __init samsung_clk_register_fixed_rate( > struct samsung_fixed_rate_clock *clk_list, unsigned int nr_clk); > extern void __init samsung_clk_register_fixed_factor(
diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c index d36cdd5..7f1b5bc 100644 --- a/drivers/clk/samsung/clk.c +++ b/drivers/clk/samsung/clk.c @@ -57,14 +57,15 @@ void __init samsung_clk_init(struct device_node *np, void __iomem *base, unsigned long nr_rdump) { reg_base = base; - if (!np) - return; -#ifdef CONFIG_OF clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL); if (!clk_table) panic("could not allocate clock lookup table\n"); + if (!np) + return; + +#ifdef CONFIG_OF clk_data.clks = clk_table; clk_data.clk_num = nr_clks; of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data); @@ -96,6 +97,46 @@ void samsung_clk_add_lookup(struct clk *clk, unsigned int id) clk_table[id] = clk; } +/* register a list of aliases */ +void __init samsung_clk_register_alias(struct samsung_clock_alias *list, + unsigned int nr_clk) +{ + struct clk *clk; + unsigned int idx, ret; + + if (!clk_table) { + pr_err("%s: clock table missing\n", __func__); + return; + } + + for (idx = 0; idx < nr_clk; idx++, list++) { + if (!list->id) { + pr_err("%s: clock id missing for index %d\n", __func__, + idx); + continue; + } + + clk = clk_table[list->id]; + if (!clk) { + pr_err("%s: failed to find clock %d\n", __func__, + list->id); + continue; + } + + /* register a clock lookup only if a clock alias is specified */ + if (!list->alias) { + pr_err("%s: no alias defined for clock %d\n", __func__, + list->id); + continue; + } + + ret = clk_register_clkdev(clk, list->alias, list->dev_name); + if (ret) + pr_err("%s: failed to register lookup %s\n", + __func__, list->alias); + } +} + /* register a list of fixed clocks */ void __init samsung_clk_register_fixed_rate( struct samsung_fixed_rate_clock *list, unsigned int nr_clk) diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h index 175a9d0..8be9248 100644 --- a/drivers/clk/samsung/clk.h +++ b/drivers/clk/samsung/clk.h @@ -23,6 +23,25 @@ #include <mach/map.h> /** + * struct samsung_clock_alias: information about mux clock + * @id: platform specific id of the clock. + * @dev_name: name of the device to which this clock belongs. + * @alias: optional clock alias name to be assigned to this clock. + */ +struct samsung_clock_alias { + unsigned int id; + const char *dev_name; + const char *alias; +}; + +#define ALIAS(_id, dname, a) \ + { \ + .id = _id, \ + .dev_name = dname, \ + .alias = a, \ + } + +/** * struct samsung_fixed_rate_clock: information about fixed-rate clock * @id: platform specific id of the clock. * @name: name of this fixed-rate clock. @@ -260,6 +282,8 @@ extern void __init samsung_clk_of_register_fixed_ext( extern void samsung_clk_add_lookup(struct clk *clk, unsigned int id); +extern void samsung_clk_register_alias(struct samsung_clock_alias *list, + unsigned int nr_clk); extern void __init samsung_clk_register_fixed_rate( struct samsung_fixed_rate_clock *clk_list, unsigned int nr_clk); extern void __init samsung_clk_register_fixed_factor(