Re: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Aug 14, 2013 at 02:54:57PM +0100, Rajendra Nayak wrote:
> 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@ti.com>
> >> ---
> >>  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.

Ok, I'd misunderstood there. Apologies for the noise.

Thanks,
Mark.