Subject: [PATCH] ARM: OMAP2+: Fix populating the hwmod data from device tree

From: [email-masked] (Tony Lindgren)
Date: Thu, 5 Dec 2013 09:29:10 -0800

* Tony Lindgren <[email-masked]> [131121 12:49]:
> * Tony Lindgren <[email-masked]> [131120 17:46]:
> > * Tony Lindgren <[email-masked]> [131120 16:06]:
> > > 
> > > They at least had interrupts listed looking at commit 3b9b10. Probably
> > > the thing to do for now is to revert those changes, and see if we can
> > > just remove the L3 entries from the .dtsi files.
> > 
> > Actually the patch I posted should be able to handle also the
> > ti,hwmods = "l3_main_1", "l3_main_2", "l3_main_3" property in omap4.dtsi,
> > but we're not currently parsing that as we only look at the children
> > and not the properties of the OCP bus. Should be fixable, will take a look
> > tomorrow if this approach makes sense to you.
> 
> OK this one seems to do the right thing for me.

No comments? I'll queue the patch below to the fixes, please yell
if you see any issues with that.
 
> Tony
> 
> 
> From: Tony Lindgren <[email-masked]>
> Date: Wed, 20 Nov 2013 15:46:51 -0800
> Subject: [PATCH] ARM: OMAP2+: Fix overwriting hwmod data with data from device tree
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> We have some device tree properties where the ti,hwmod have multiple
> values:
> 
> am33xx.dtsi:	ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
> am4372.dtsi:	ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
> dra7.dtsi:	ti,hwmods = "l3_main_1", "l3_main_2";
> omap3.dtsi:	ti,hwmods = "mcbsp2", "mcbsp2_sidetone";
> omap3.dtsi:	ti,hwmods = "mcbsp3", "mcbsp3_sidetone";
> omap4.dtsi:	ti,hwmods = "l3_main_1", "l3_main_2", "l3_main_3";
> omap5.dtsi:	ti,hwmods = "l3_main_1", "l3_main_2", "l3_main_3";
> 
> That's not correct way of doing things in this case because these are
> separate devices with their own address space, interrupts, SYSCONFIG
> registers and can set their PM states independently.
> 
> So they should all be fixed up to be separate devices in the .dts files.
> 
> We also have the related data removed for at least omap4 in commit
> 3b9b10151c68 (ARM: OMAP4: hwmod data: Clean up the data file), so
> that data is wrongly initialized as null data.
> 
> So we need to fix two bugs:
> 
> 1. We are only checking the first entry of the ti,hwmods property
> 
>    This means that we're only initializing the first hwmods entry
>    instead of the ones listed in the ti,hwmods property.
> 
> 2. We are only checking the child nodes, not the nodes themselves
> 
>    This means that anything listed at OCP level is currently just
>    ignored and unitialized and at least the omap4 case, with the
>    legacy data missing from the hwmod.
> 
> Fix both of the issues by using an index to the ti,hwmods property
> and changing the hwmod lookup function to also check the current node
> for ti,hwmods property instead of just the children.
> 
> While at it, let's also add some warnings for the bad data so it's
> easier to fix.
> 
> Cc: "Beno?t Cousson" <[email-masked]>
> Cc: Paul Walmsley <[email-masked]>
> Signed-off-by: Tony Lindgren <[email-masked]>
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index e3f0eca..ee655da 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2326,38 +2326,80 @@ static int _shutdown(struct omap_hwmod *oh)
>  	return 0;
>  }
>  
> +static int of_dev_find_hwmod(struct device_node *np,
> +			     struct omap_hwmod *oh)
> +{
> +	int count, i, res;
> +	const char *p;
> +
> +	count = of_property_count_strings(np, "ti,hwmods");
> +	if (count < 1)
> +		return -ENODEV;
> +
> +	for (i = 0; i < count; i++) {
> +		res = of_property_read_string_index(np, "ti,hwmods",
> +						    i, &p);
> +		if (res)
> +			continue;
> +		if (!strcmp(p, oh->name)) {
> +			pr_debug("omap_hwmod: dt %s[%i] uses hwmod %s\n",
> +				 np->name, i, oh->name);
> +			return i;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  /**
>   * of_dev_hwmod_lookup - look up needed hwmod from dt blob
>   * @np: struct device_node *
>   * @oh: struct omap_hwmod *
> + * @index: index of the entry found
> + * @found: struct device_node * found or NULL
>   *
>   * Parse the dt blob and find out needed hwmod. Recursive function is
>   * implemented to take care hierarchical dt blob parsing.
> - * Return: The device node on success or NULL on failure.
> + * Return: Returns 0 on success, -ENODEV when not found.
>   */
> -static struct device_node *of_dev_hwmod_lookup(struct device_node *np,
> -						struct omap_hwmod *oh)
> +static int of_dev_hwmod_lookup(struct device_node *np,
> +			       struct omap_hwmod *oh,
> +			       int *index,
> +			       struct device_node **found)
>  {
> -	struct device_node *np0 = NULL, *np1 = NULL;
> -	const char *p;
> +	struct device_node *np0 = NULL;
> +	int res;
> +
> +	res = of_dev_find_hwmod(np, oh);
> +	if (res >= 0) {
> +		*found = np;
> +		*index = res;
> +		return 0;
> +	}
>  
>  	for_each_child_of_node(np, np0) {
> -		if (of_find_property(np0, "ti,hwmods", NULL)) {
> -			p = of_get_property(np0, "ti,hwmods", NULL);
> -			if (!strcmp(p, oh->name))
> -				return np0;
> -			np1 = of_dev_hwmod_lookup(np0, oh);
> -			if (np1)
> -				return np1;
> +		struct device_node *fc;
> +		int i;
> +
> +		res = of_dev_hwmod_lookup(np0, oh, &i, &fc);
> +		if (res == 0) {
> +			*found = fc;
> +			*index = i;
> +			return 0;
>  		}
>  	}
> -	return NULL;
> +
> +	*found = NULL;
> +	*index = 0;
> +
> +	return -ENODEV;
>  }
>  
>  /**
>   * _init_mpu_rt_base - populate the virtual address for a hwmod
>   * @oh: struct omap_hwmod * to locate the virtual address
>   * @data: (unused, caller should pass NULL)
> + * @index: index of the reg entry iospace in device tree
>   * @np: struct device_node * of the IP block's device node in the DT data
>   *
>   * Cache the virtual address used by the MPU to access this IP block's
> @@ -2368,7 +2410,7 @@ static struct device_node *of_dev_hwmod_lookup(struct device_node *np,
>   * -ENXIO on absent or invalid register target address space.
>   */
>  static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data,
> -				    struct device_node *np)
> +				    int index, struct device_node *np)
>  {
>  	struct omap_hwmod_addr_space *mem;
>  	void __iomem *va_start = NULL;
> @@ -2390,13 +2432,17 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data,
>  		if (!np)
>  			return -ENXIO;
>  
> -		va_start = of_iomap(np, oh->mpu_rt_idx);
> +		va_start = of_iomap(np, index + oh->mpu_rt_idx);
>  	} else {
>  		va_start = ioremap(mem->pa_start, mem->pa_end - mem->pa_start);
>  	}
>  
>  	if (!va_start) {
> -		pr_err("omap_hwmod: %s: Could not ioremap\n", oh->name);
> +		if (mem)
> +			pr_err("omap_hwmod: %s: Could not ioremap\n", oh->name);
> +		else
> +			pr_err("omap_hwmod: %s: Missing dt reg%i for %s\n",
> +			       oh->name, index, np->full_name);
>  		return -ENXIO;
>  	}
>  
> @@ -2422,17 +2468,29 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data,
>   */
>  static int __init _init(struct omap_hwmod *oh, void *data)
>  {
> -	int r;
> +	int r, index;
>  	struct device_node *np = NULL;
>  
>  	if (oh->_state != _HWMOD_STATE_REGISTERED)
>  		return 0;
>  
> -	if (of_have_populated_dt())
> -		np = of_dev_hwmod_lookup(of_find_node_by_name(NULL, "ocp"), oh);
> +	if (of_have_populated_dt()) {
> +		struct device_node *bus;
> +
> +		bus = of_find_node_by_name(NULL, "ocp");
> +		if (!bus)
> +			return -ENODEV;
> +
> +		r = of_dev_hwmod_lookup(bus, oh, &index, &np);
> +		if (r)
> +			pr_debug("omap_hwmod: %s missing dt data\n", oh->name);
> +		else if (np && index)
> +			pr_warn("omap_hwmod: %s using broken dt data from %s\n",
> +				oh->name, np->name);
> +	}
>  
>  	if (oh->class->sysc) {
> -		r = _init_mpu_rt_base(oh, NULL, np);
> +		r = _init_mpu_rt_base(oh, NULL, index, np);
>  		if (r < 0) {
>  			WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n",
>  			     oh->name);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html