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 <rnayak@xxxxxx>
>> ---
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
