Patchwork ARM: OMAP2+: Fix populating the hwmod data from device tree

login
register
mail settings
Submitter Tony Lindgren
Date Nov. 20, 2013, 2:56 a.m.
Message ID <20131120025620.GF10317@atomide.com>
Download mbox | patch
Permalink /patch/292640/
State New
Headers show

Comments

Tony Lindgren - Nov. 20, 2013, 2:56 a.m.
We have some device tree properties where the ti,hwmod has 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";

So we need to handle the whole string array instead of just the
first string to find the related hwmod entry.

Cc: "Benoît Cousson" <bcousson@baylibre.com>
Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
Tony Lindgren - Nov. 20, 2013, 6:12 p.m.
* Tony Lindgren <tony@atomide.com> [131119 18:57]:
> We have some device tree properties where the ti,hwmod has 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";
> 
> So we need to handle the whole string array instead of just the
> first string to find the related hwmod entry.
> 
> Cc: "Benoît Cousson" <bcousson@baylibre.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2228,11 +2228,23 @@ static struct device_node *of_dev_hwmod_lookup(struct device_node *np,
>  						struct omap_hwmod *oh)
>  {
>  	struct device_node *np0 = NULL, *np1 = NULL;
> -	const char *p;
>  
>  	for_each_child_of_node(np, np0) {
> -		if (of_find_property(np0, "ti,hwmods", NULL)) {
> -			p = of_get_property(np0, "ti,hwmods", NULL);
> +		int count, i;
> +
> +		count = of_property_count_strings(np0, "ti,hwmods");
> +		if (count < 1)
> +			continue;
> +
> +		for (i = 0; i < count; i++) {
> +			const char *p;
> +			int res;
> +
> +			res = of_property_read_string_index(np0, "ti,hwmods",
> +							    i, &p);
> +			if (res)
> +				continue;
> +
>  			if (!strcmp(p, oh->name))
>  				return np0;
>  			np1 = of_dev_hwmod_lookup(np0, oh);

Hmm I think this also needs part two to it to populate the right IO space
based on the index, this probably now wrongly populates the first IO space
for all the hwmod instances in the group.

Regards,

Tony
Tony Lindgren - Nov. 20, 2013, 7:22 p.m.
* Tony Lindgren <tony@atomide.com> [131120 10:13]:
> * Tony Lindgren <tony@atomide.com> [131119 18:57]:
> > We have some device tree properties where the ti,hwmod has multiple
> > values:
> > 
> > am33xx.dtsi:	ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
> > am4372.dtsi:	ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";

I think these should be defined in the .dtsi files as separate devices
and removed from the related omap_hwmod_*_data.c files.

> > dra7.dtsi:	ti,hwmods = "l3_main_1", "l3_main_2";

These too should be defined in the .dtsi file. Looks like we're missing
the related data in the omap_hwmod_7xx_data.c file anyways?

> > omap3.dtsi:	ti,hwmods = "mcbsp2", "mcbsp2_sidetone";
> > omap3.dtsi:	ti,hwmods = "mcbsp3", "mcbsp3_sidetone";

OK the sidetone data we can probably parse using the ti,hwmods as
an index.

> > 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";

Benoit, I guess these mean that those devices should be created
dynamically, right?

Yet we've removed the related data in commit 3b9b10 and are not
passing it in the .dtsi file?

So these should be created as separate devices in the .dtsi file
instead AFAIK.

> Hmm I think this also needs part two to it to populate the right IO space
> based on the index, this probably now wrongly populates the first IO space
> for all the hwmod instances in the group.

Yeah this patch clearly is not the right fix to the problem of
missing data.

Regards,

Tony
Paul Walmsley - Nov. 20, 2013, 9:58 p.m.
On Wed, 20 Nov 2013, Tony Lindgren wrote:

> * Tony Lindgren <tony@atomide.com> [131120 10:13]:
> > * Tony Lindgren <tony@atomide.com> [131119 18:57]:
> > > We have some device tree properties where the ti,hwmod has multiple
> > > values:
> > > 
> > > am33xx.dtsi:	ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
> > > am4372.dtsi:	ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
> 
> I think these should be defined in the .dtsi files as separate devices
> and removed from the related omap_hwmod_*_data.c files.
> 
> > > dra7.dtsi:	ti,hwmods = "l3_main_1", "l3_main_2";
> 
> These too should be defined in the .dtsi file. Looks like we're missing
> the related data in the omap_hwmod_7xx_data.c file anyways?
> 
> > > omap3.dtsi:	ti,hwmods = "mcbsp2", "mcbsp2_sidetone";
> > > omap3.dtsi:	ti,hwmods = "mcbsp3", "mcbsp3_sidetone";
> 
> OK the sidetone data we can probably parse using the ti,hwmods as
> an index.
> 
> > > 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";
> 
> Benoit, I guess these mean that those devices should be created
> dynamically, right?
> 
> Yet we've removed the related data in commit 3b9b10 and are not
> passing it in the .dtsi file?
> 
> So these should be created as separate devices in the .dtsi file
> instead AFAIK.
> 
> > Hmm I think this also needs part two to it to populate the right IO space
> > based on the index, this probably now wrongly populates the first IO space
> > for all the hwmod instances in the group.
> 
> Yeah this patch clearly is not the right fix to the problem of
> missing data.

In most of the cases with multiple hwmods per DT device, there's probably 
something wrong with the data.  So we should try to remove those when 
possible.

For example if you look at the SIDETONE blocks, those should definitely be 
separate DT devices - they have separate address spaces, SYSCONFIG 
registers, etc. (section 21.6.2 "SIDETONE register mapping summary", 
OMAP36xx TRM)

Would strongly suspect that's the case also for the EDMA blocks you 
mention.

Am not 100% sure about the L3 cases; would have to dig deeper.


- Paul

Patch

--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2228,11 +2228,23 @@  static struct device_node *of_dev_hwmod_lookup(struct device_node *np,
 						struct omap_hwmod *oh)
 {
 	struct device_node *np0 = NULL, *np1 = NULL;
-	const char *p;
 
 	for_each_child_of_node(np, np0) {
-		if (of_find_property(np0, "ti,hwmods", NULL)) {
-			p = of_get_property(np0, "ti,hwmods", NULL);
+		int count, i;
+
+		count = of_property_count_strings(np0, "ti,hwmods");
+		if (count < 1)
+			continue;
+
+		for (i = 0; i < count; i++) {
+			const char *p;
+			int res;
+
+			res = of_property_read_string_index(np0, "ti,hwmods",
+							    i, &p);
+			if (res)
+				continue;
+
 			if (!strcmp(p, oh->name))
 				return np0;
 			np1 = of_dev_hwmod_lookup(np0, oh);