On Wednesday 14 August 2013 07:15 PM, Mark Rutland wrote: > [Adding Mike Turquette and dt maintainers to Cc] > > On Tue, Jul 23, 2013 at 07:24:38AM +0100, Rajendra Nayak wrote: >> With clocks for OMAP moving to DT, its now possible to pass all optional >> clock >> data for each device from DT instead of having it in hwmod. >> >> Signed-off-by: Rajendra Nayak <[email protected]> >> --- >> arch/arm/mach-omap2/omap_hwmod.c | 66 >> ++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 64 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/omap_hwmod.c >> b/arch/arm/mach-omap2/omap_hwmod.c >> index 12fa589..e5c804b 100644 >> --- a/arch/arm/mach-omap2/omap_hwmod.c >> +++ b/arch/arm/mach-omap2/omap_hwmod.c >> @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh) >> return ret; >> } >> >> +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh, >> + struct device_node *np, >> + int *opt_clks_cnt) >> +{ >> + int i, clks_cnt; >> + const char *clk_name; >> + const char **opt_clk_names; >> + >> + clks_cnt = of_property_count_strings(np, "clock-names"); >> + if (!clks_cnt) >> + return NULL; >> + >> + opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL); >> + if (!opt_clk_names) >> + return NULL; >> + >> + for (i = 0; i < clks_cnt; i++) { >> + of_property_read_string_index(np, "clock-names", i, &clk_name); >> + if (!strcmp(clk_name, "fck")) >> + continue; >> + opt_clks_cnt++; >> + opt_clk_names[i] = clk_name; >> + } >> + return opt_clk_names; >> +} >> + >> +static int _init_opt_clks_dt(struct omap_hwmod *oh, struct device_node *np) >> +{ >> + struct clk *c; >> + int i, opt_clks_cnt = 0; >> + int ret = 0; >> + const char **opt_clk_names; >> + >> + opt_clk_names = _parse_opt_clks_dt(oh, np, &opt_clks_cnt); >> + if (!opt_clk_names) >> + return -EINVAL; >> + >> + oh->opt_clks = kzalloc(sizeof(struct omap_hwmod_opt_clk *) >> + * opt_clks_cnt, GFP_KERNEL); >> + if (!oh->opt_clks) >> + return -ENOMEM; >> + >> + oh->opt_clks_cnt = opt_clks_cnt; >> + >> + for (i = 0; i < oh->opt_clks_cnt; i++) { >> + c = of_clk_get_by_name(np, opt_clk_names[i]); >> + if (IS_ERR(c)) { >> + pr_warn("omap_hwmod: %s: cannot clk_get opt_clk %s\n", >> + oh->name, opt_clk_names[i]); >> + ret = -EINVAL; >> + } >> + oh->opt_clks[i]._clk = c; >> + oh->opt_clks[i].role = opt_clk_names[i]; >> + oh->opt_clks_cnt++; >> + clk_prepare(oh->opt_clks[i]._clk); >> + } >> + return ret; >> +} > > I don't like this. > > clock-names is used to represent the names of clocks as inputs to the > device. The driver must know the names of each and every one of the > clock inputs it intends to use -- there's a finite number, and if it > doesn't know about it it clearly has no idea how that clock's meant to > be used. > > Consider a future revision of the hardware that has an additional clock > input. Some new feature may require that clock, but your driver won't > support that new feature, so you don't need it. Preparing that clock is > a waste of power, and could cause issues if for some reason the clock > was mutually exlcusive with another clock (so preparing it would make > the hardware unusable). If the new revision *requires* that clock to > provide the same interface otherwise, it's not backwards compatible and > needs a new binding, and the driver needs to be extended to support it. > > Given that, preparing all the clocks you've been handed is a hack.
Mark, this is a piece of platform code (hwmod framework for omap) which does a enable/reset/idle of all devices on the SoC early at boot to get rid of bootloader dependencies. This isn;t something used by the drivers when they enable the devices. I don't see any issue with 'waste of power'. The framework (unlike the driver) has no knowledge of what clocks are needed and hence does a enable all momentarily to reset and put the device in a known state. > Simply request by name the ones you know you need, and attempt to > request the optional ones as necessary. Don't blindly go and prepare > everything. > > Thanks, > Mark. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
