| Message ID | 1374713022-6049-4-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Stephen, On Wednesday 24 of July 2013 17:43:31 Stephen Boyd wrote: > In similar fashion as of_regulator_match() add an of_clk_match() > function that finds an initializes clock init_data structs from > devicetree. Drivers should use this API to find clocks that their > device is providing and then iterate over their match table > registering the clocks with the init data parsed. I think all you need here is declaring all your clock drivers using CLK_OF_DECLARE() macro. Then they will be automatically probed by of_clk_init(). See last lines of drivers/clk/clk-fixed-rate.c for an example. Best regards, Tomasz > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/clk/clk.c | 64 > ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/clk-provider.h | 16 +++++++++++ > 2 files changed, 80 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index ea8e951b..1e3e0db 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2289,4 +2289,68 @@ err: > return -ENOMEM; > } > EXPORT_SYMBOL_GPL(of_init_clk_data); > + > +/** > + * of_clk_match() - Scan and match clock providers from the DT node of > a device + * @dev: device initializing clock providers > + * @matches: match table of clocks > + * @num_matches: number of entries in @matches > + * > + * This function uses a match table specified by the clock driver to > parse + * clock init data from the device tree. @dev is expected to be > a device with + * a 'clocks' child node containing a set of child > nodes, each providing the + * init data for one clock. The data parsed > from a child node will be matched + * to a clock based on the child > node's name. Note that the match table is + * modified in place. > + * > + * Returns the number of matches found or a negative error code on > failure. + */ > +int of_clk_match(struct device *dev, struct of_clk_match *matches, > + int num_matches) > +{ > + int count = 0; > + int err, i; > + struct device_node *np, *clocks; > + struct clk_init_data *init; > + > + clocks = of_find_node_by_name(dev->of_node, "clocks"); > + if (!clocks) > + return -EINVAL; > + > + for (i = 0; i < num_matches; i++) { > + struct of_clk_match *match = &matches[i]; > + match->init_data = NULL; > + match->of_node = NULL; > + } > + > + for_each_available_child_of_node(clocks, np) { > + for (i = 0; i < num_matches; i++) { > + struct of_clk_match *match = &matches[i]; > + if (match->of_node) > + continue; > + > + if (strcmp(match->name, np->name)) > + continue; > + > + init = devm_kzalloc(dev, sizeof(*init), GFP_KERNEL); > + if (!init) > + return -ENOMEM; > + > + err = of_init_clk_data(np, init); > + if (err) { > + dev_err(dev, > + "failed to parse DT for clock %s\n", > + np->name); > + return err; > + } > + match->of_node = np; > + match->init_data = init; > + count++; > + break; > + } > + } > + > + return count; > +} > +EXPORT_SYMBOL_GPL(of_clk_match); > #endif > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 18d6362..484f8ad 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -461,6 +461,13 @@ struct clk_onecell_data { > __used __section(__clk_of_table) \ > = { .compatible = compat, .data = fn }; > > +struct of_clk_match { > + const char *name; > + void *driver_data; > + struct clk_init_data *init_data; > + struct device_node *of_node; > +}; > + > #ifdef CONFIG_OF > int of_clk_add_provider(struct device_node *np, > struct clk *(*clk_src_get)(struct of_phandle_args *args, > @@ -475,6 +482,9 @@ const char *of_clk_get_parent_name(struct > device_node *np, int index); void of_clk_init(const struct of_device_id > *matches); > int of_init_clk_data(struct device_node *np, struct clk_init_data > *init); > > +int of_clk_match(struct device *dev, struct of_clk_match *matches, > + int num_matches); > + > #else /* !CONFIG_OF */ > > static inline int of_clk_add_provider(struct device_node *np, > @@ -508,6 +518,12 @@ of_init_clk_data(struct device_node *np, struct > clk_init_data *init) { > return 0; > } > +static inline int > +of_clk_match(struct device *dev, struct of_clk_match *matches, int > num_matches) +{ > + > + return 0; > +} > #endif /* CONFIG_OF */ > #endif /* CONFIG_COMMON_CLK */ > #endif /* CLK_PROVIDER_H */ -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/25, Tomasz Figa wrote: > Hi Stephen, > > On Wednesday 24 of July 2013 17:43:31 Stephen Boyd wrote: > > In similar fashion as of_regulator_match() add an of_clk_match() > > function that finds an initializes clock init_data structs from > > devicetree. Drivers should use this API to find clocks that their > > device is providing and then iterate over their match table > > registering the clocks with the init data parsed. > > I think all you need here is declaring all your clock drivers using > CLK_OF_DECLARE() macro. Then they will be automatically probed by > of_clk_init(). See last lines of drivers/clk/clk-fixed-rate.c for an > example. > CLK_OF_DECLARE() will not work because it doesn't give the caller a struct device. I want that struct device to use managed allocations and clk registrations.
On 08/12, Mike Turquette wrote: > Quoting Stephen Boyd (2013-07-24 17:43:31) > > In similar fashion as of_regulator_match() add an of_clk_match() > > function that finds an initializes clock init_data structs from > > devicetree. Drivers should use this API to find clocks that their > > device is providing and then iterate over their match table > > registering the clocks with the init data parsed. > > > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > Stephen, > > In general I like this approach. Writing real device drivers for clock > controllers is The Right Way and of_clk_match helps. > > Am I reading this correctly that the base register addresses/offsets for > the clock nodes still come from C files? For example I still see > pll8_desc defining reg stuff in drivers/clk/msm/gcc-8960.c. I think we may be able to put the registers in DT but I don't know why we need to do that if we're already matching up nodes with C structs. It also made me want to introduce devm_of_iomap() which just seemed wrong (if you have a dev struct why can't you use devm_ioremap()). > > What do you think about fetching this data from DT? My thinking here is > that the definition of that PLL structure would be in C, as would all of > the control logic. But any per-clock data whatsoever should live in DTS. > This means the clock data you supply in the DTS files in patches #9 and > #10 would have base addresses or offsets per-clock. I think this echoes > Mark R's concerns as well. > > In the future if new chips have more of that type of PLL it would not > require changes to your clock driver, only new DTS data for the new > chip. > > I could have that wrong though, there is a fair amount of indirection in > this series... Let's take the PLL example and see if I follow what would be in DT and what would be in C. Right now we have pll8: pll8 { #clock-cells = <0>; compatible = "qcom,pll"; clocks = <&pxo>; }; in DT and static struct pll_desc pll8_desc = { .l_reg = 0x3144, .m_reg = 0x3148, .n_reg = 0x314c, .config_reg = 0x3154, .mode_reg = 0x3140, .status_reg = 0x3158, .status_bit = 16, }; in C. Do you want everything to be in DT? Something like: pll8: pll8@3140 { #clock-cells = <0>; compatible = "qcom,pll"; clocks = <&pxo>; reg = <0x3140 0x20>; }; and then assume that all those registers are offset from the base register and that the status bit is 16 (it usually is but not always)? The problem I see is this quickly breaks down with more complicated clocks like the RCGs. We have gsbi5_uart_rcg: gsbi5_uart_rcg { #clock-cells = <0>; compatible = "qcom,p2-mn16-clock"; clocks = <&pxo>, <&vpll8>; }; in DT and static struct freq_tbl clk_tbl_gsbi_uart[] = { { 1843200, PLL8, 2, 6, 625 }, { 3686400, PLL8, 2, 12, 625 }, { 7372800, PLL8, 2, 24, 625 }, { 14745600, PLL8, 2, 48, 625 }, { 16000000, PLL8, 4, 1, 6 }, { 24000000, PLL8, 4, 1, 4 }, { 32000000, PLL8, 4, 1, 3 }, { 40000000, PLL8, 1, 5, 48 }, { 46400000, PLL8, 1, 29, 240 }, { 48000000, PLL8, 4, 1, 2 }, { 51200000, PLL8, 1, 2, 15 }, { 56000000, PLL8, 1, 7, 48 }, { 58982400, PLL8, 1, 96, 625 }, { 64000000, PLL8, 2, 1, 3 }, { } }; static struct rcg_desc gsbi5_uart_rcg = { .ctl_reg = 0x2a54, .ns_reg = 0x2a54, .md_reg = 0x2a50, .ctl_bit = 11, .mnctr_en_bit = 8, .mnctr_reset_bit = 7, .mnctr_mode_shift = 5, .pre_div_shift = 3, .src_sel_shift = 0, .n_val_shift = 16, .m_val_shift = 16, .parent_map = gcc_pxo_pll8_map, .freq_tbl = clk_tbl_gsbi_uart, }; in C. It starts to get pretty unwieldy when you put this all in DT, plus you'll notice that the ns_reg and ctl_reg are the same here because we've generalized the code to work with different types of software interfaces (technically this clock has no ctl register, just an NS and MD register). Our multimedia clock controllers don't follow any standard base/offset pair and so the ctl_reg can be a different offset from the md_reg depending on which clock we're talking about. My initial try at translating this into DT pretty much just made every struct member into a property, including the duplicate register, expect for the frequency table, which could probably also be DT-ified with some work. gsbi5_uart_rcg: gsbi5_uart_rcg@2a54 { #clock-cells = <0>; compatible = "qcom,p2-mn16-clock"; clocks = <&pxo>, <&vpll8>; reg = <0x2a54 0x4>, <0x2a54 0x4>, <0x2a50 0x4>; ctl_bit = <11>; mnctr_en_bit = <8>; mnctr_reset_bit = <7>; mnctr_mode_shift = <5>; pre_div_shift = <3>; src_sel_shift = <0>; n_val_shift = <16>; m_val_shift = <16>; }; This is great for making the kernel DT-data-driven, but I couldn't find any other driver that was describing register level details in DT. The good news is that newer clock controllers follow a standard and so we can specify one or two register properties and the type of clock and we're pretty much done. The software interface hasn't been randomized like on earlier controllers and bits within registers are always the same. We still have some clocks that are just on/off switches though and so we'll have to put register level details like which bit turns that clock on in DT (which I believe is not preferred/allowed?). I don't see any way to avoid that if we want it to be entirely DT driven.
On 08/14, Mike Turquette wrote: > Quoting Stephen Boyd (2013-08-12 22:48:39) > > On 08/12, Mike Turquette wrote: > > > Quoting Stephen Boyd (2013-07-24 17:43:31) > > > > In similar fashion as of_regulator_match() add an of_clk_match() > > > > function that finds an initializes clock init_data structs from > > > > devicetree. Drivers should use this API to find clocks that their > > > > device is providing and then iterate over their match table > > > > registering the clocks with the init data parsed. > > > > > > > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > > > > > Stephen, > > > > > > In general I like this approach. Writing real device drivers for clock > > > controllers is The Right Way and of_clk_match helps. > > > > > > Am I reading this correctly that the base register addresses/offsets for > > > the clock nodes still come from C files? For example I still see > > > pll8_desc defining reg stuff in drivers/clk/msm/gcc-8960.c. > > > > I think we may be able to put the registers in DT but I don't > > know why we need to do that if we're already matching up nodes > > with C structs. > > The reason to do so is to remove the per-clock data from the kernel > sources entirely. Separating logic and data. Ok. > > > It also made me want to introduce devm_of_iomap() > > which just seemed wrong (if you have a dev struct why can't you > > use devm_ioremap()). > > [snip] > > > > This is great for making the kernel DT-data-driven, but I > > couldn't find any other driver that was describing register level > > details in DT. > > Yeah, this sucks. Building a binding from a C struct is a bad idea. How > many permutations are there? Hopefully some clocks use the same bit > shifts and have reliable register offsets, with the only difference > being base address. If this is the case then a compatible string could > be done for each permutation assuming that number is low and manageable. In the multimedia controller almost every clock has a different register layout. Making a compatible string for each clock is pretty much what I've done if you consider that I match up the name of the node to a struct instead of matching a compatible string to a struct. In the global controller it's more sane, following a single register layout per compatible string. Remember though, all the single bit clocks (gates or what we call branches) have a completely random location for their on/off bit and status bit. > > > > > The good news is that newer clock controllers follow a standard > > and so we can specify one or two register properties and the type > > of clock and we're pretty much done. The software interface > > hasn't been randomized like on earlier controllers and bits > > within registers are always the same. > > This does not suck. Just for the sake of argument let's say that you > only had to deal with this new and improved register layout and not the > old (current) stuff. Do you still see a reason to match DT data up with > C struct data objects in the kernel? I would still need to match up frequency tables unless I figure out a way to put that in DT too. > > > We still have some clocks > > that are just on/off switches though and so we'll have to put > > register level details like which bit turns that clock on in DT > > (which I believe is not preferred/allowed?). I don't see any way > > to avoid that if we want it to be entirely DT driven. > > This is what I'm doing for the generic clock bindings. No one has > screamed over that stuff so I guess rules were meant to be broken. >
On Aug 15, 2013, at 12:02 AM, Mike Turquette wrote: >> Right now we have >> >> pll8: pll8 { >> #clock-cells = <0>; >> compatible = "qcom,pll"; >> clocks = <&pxo>; >> }; >> >> in DT and >> >> static struct pll_desc pll8_desc = { >> .l_reg = 0x3144, >> .m_reg = 0x3148, >> .n_reg = 0x314c, >> .config_reg = 0x3154, >> .mode_reg = 0x3140, >> .status_reg = 0x3158, >> .status_bit = 16, >> }; >> >> in C. Do you want everything to be in DT? Something like: >> >> pll8: pll8@3140 { >> #clock-cells = <0>; >> compatible = "qcom,pll"; >> clocks = <&pxo>; >> reg = <0x3140 0x20>; >> }; >> >> and then assume that all those registers are offset from the base >> register and that the status bit is 16 (it usually is but not >> always)? I think its reasonable to put the various regs associated with a clock in the .dts like the example you show, but we should be going down to bit level details. If we think of each clock as its own device its reasonable that the clock would have some set of registers associated with it. -k
On Aug 16, 2013, at 11:43 AM, Kumar Gala wrote: > > On Aug 15, 2013, at 12:02 AM, Mike Turquette wrote: > >>> Right now we have >>> >>> pll8: pll8 { >>> #clock-cells = <0>; >>> compatible = "qcom,pll"; >>> clocks = <&pxo>; >>> }; >>> >>> in DT and >>> >>> static struct pll_desc pll8_desc = { >>> .l_reg = 0x3144, >>> .m_reg = 0x3148, >>> .n_reg = 0x314c, >>> .config_reg = 0x3154, >>> .mode_reg = 0x3140, >>> .status_reg = 0x3158, >>> .status_bit = 16, >>> }; >>> >>> in C. Do you want everything to be in DT? Something like: >>> >>> pll8: pll8@3140 { >>> #clock-cells = <0>; >>> compatible = "qcom,pll"; >>> clocks = <&pxo>; >>> reg = <0x3140 0x20>; >>> }; >>> >>> and then assume that all those registers are offset from the base >>> register and that the status bit is 16 (it usually is but not >>> always)? > > I think its reasonable to put the various regs associated with a clock in the .dts like the example you show, but we should be going down to bit level details. If we think of each clock as its own device its reasonable that the clock would have some set of registers associated with it. oops, we should NOT be going down to bit level. - k
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index ea8e951b..1e3e0db 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2289,4 +2289,68 @@ err: return -ENOMEM; } EXPORT_SYMBOL_GPL(of_init_clk_data); + +/** + * of_clk_match() - Scan and match clock providers from the DT node of a device + * @dev: device initializing clock providers + * @matches: match table of clocks + * @num_matches: number of entries in @matches + * + * This function uses a match table specified by the clock driver to parse + * clock init data from the device tree. @dev is expected to be a device with + * a 'clocks' child node containing a set of child nodes, each providing the + * init data for one clock. The data parsed from a child node will be matched + * to a clock based on the child node's name. Note that the match table is + * modified in place. + * + * Returns the number of matches found or a negative error code on failure. + */ +int of_clk_match(struct device *dev, struct of_clk_match *matches, + int num_matches) +{ + int count = 0; + int err, i; + struct device_node *np, *clocks; + struct clk_init_data *init; + + clocks = of_find_node_by_name(dev->of_node, "clocks"); + if (!clocks) + return -EINVAL; + + for (i = 0; i < num_matches; i++) { + struct of_clk_match *match = &matches[i]; + match->init_data = NULL; + match->of_node = NULL; + } + + for_each_available_child_of_node(clocks, np) { + for (i = 0; i < num_matches; i++) { + struct of_clk_match *match = &matches[i]; + if (match->of_node) + continue; + + if (strcmp(match->name, np->name)) + continue; + + init = devm_kzalloc(dev, sizeof(*init), GFP_KERNEL); + if (!init) + return -ENOMEM; + + err = of_init_clk_data(np, init); + if (err) { + dev_err(dev, + "failed to parse DT for clock %s\n", + np->name); + return err; + } + match->of_node = np; + match->init_data = init; + count++; + break; + } + } + + return count; +} +EXPORT_SYMBOL_GPL(of_clk_match); #endif diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 18d6362..484f8ad 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -461,6 +461,13 @@ struct clk_onecell_data { __used __section(__clk_of_table) \ = { .compatible = compat, .data = fn }; +struct of_clk_match { + const char *name; + void *driver_data; + struct clk_init_data *init_data; + struct device_node *of_node; +}; + #ifdef CONFIG_OF int of_clk_add_provider(struct device_node *np, struct clk *(*clk_src_get)(struct of_phandle_args *args, @@ -475,6 +482,9 @@ const char *of_clk_get_parent_name(struct device_node *np, int index); void of_clk_init(const struct of_device_id *matches); int of_init_clk_data(struct device_node *np, struct clk_init_data *init); +int of_clk_match(struct device *dev, struct of_clk_match *matches, + int num_matches); + #else /* !CONFIG_OF */ static inline int of_clk_add_provider(struct device_node *np, @@ -508,6 +518,12 @@ of_init_clk_data(struct device_node *np, struct clk_init_data *init) { return 0; } +static inline int +of_clk_match(struct device *dev, struct of_clk_match *matches, int num_matches) +{ + + return 0; +} #endif /* CONFIG_OF */ #endif /* CONFIG_COMMON_CLK */ #endif /* CLK_PROVIDER_H */
In similar fashion as of_regulator_match() add an of_clk_match() function that finds an initializes clock init_data structs from devicetree. Drivers should use this API to find clocks that their device is providing and then iterate over their match table registering the clocks with the init data parsed. Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/clk/clk.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk-provider.h | 16 +++++++++++ 2 files changed, 80 insertions(+)