Re: [PATCH v2] OF: base: match each node compatible against all givenmatches first

[Sebastian Hesselbarth]

On 12/03/2013 09:14 PM, Meelis Roos wrote:
> > Currently, of_match_node compares each given match against all node's
> > compatible strings with of_device_is_compatible.
> >
> > To achieve multiple compatible strings per node with ordering from
> > specific to generic, this requires given matches to be ordered from
> > specific to generic. For most of the drivers this is not true and also
> > an alphabetical ordering is more sane there.
> >
> > Therefore, this patch modifies of_match_node to match each of the node's
> > compatible strings against all given matches first, before checking the
> > next compatible string. This implies that node's compatibles are ordered
> > from specific to generic while given matches can be in any order.
>
> I think I am on the CC: list because of a CPU detection problem report
> on sparc64 (183912d352a242a276a7877852f107459a13aff9 (of: move
> of_get_cpu_node implementation to DT core library) caused trouble and

The reason you are on Cc is that Thierry added you on last patch
version. I cannot see how above commit should be related with this
one, but maybe Thierry can comment on it.

> was reverted). So while your V2 patch does not cause any visible harm on
> the same Sun E3500, my gut feeling is that an additional patch would be
> needed to actually test it (a patch like
> 183912d352a242a276a7877852f107459a13aff9).

This patch deals with matching a node with more than one compatible
string on a (unordered) list of matches. Although not related to your
issue, it is good to hear that it causes no harm on DT-mature archs :)

I tested it with ARM and l2x0 cache controllers, where the specific
of_device_id (marvell,tauros3-cache) is sorted after the generic
one (arm,pl310-cache). The corresponding node's property is
compatible = "marvell,tauros3-cache", "arm,pl310-cache".

Without this patch, of_match_node always hits the first match that
equals _any_ of the above compatible strings. With this patch, it
hits the matches _in order_ of the compatible strings.

> Is this correct or am I missing something?

Thierry?

> > Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@xxxxxxxxx>
> > ---
> > Changelog:
> > v1->v2:
> > - Allow checks against nodes with no compatible (Reported by Rob Herring)
> > - Add some comments
> >
> > Cc: Grant Likely <grant.likely@xxxxxxxxxx>
> > Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
> > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> > Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> > Cc: Meelis Roos <mroos@xxxxxxxx>
> > Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > Cc: Scott Wood <scottwood@xxxxxxxxxxxxx>
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >   drivers/of/base.c |   53 +++++++++++++++++++++++++++++++++++++----------------
> >   1 files changed, 37 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index f807d0e..8d007d8 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -731,24 +731,42 @@ static
> >   const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> >   					   const struct device_node *node)
> >   {
> > +	const char *cp;
> > +	int cplen, l;
> > +
> >   	if (!matches)
> >   		return NULL;
> >
> > -	while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
> > -		int match = 1;
> > -		if (matches->name[0])
> > -			match &= node->name
> > -				&& !strcmp(matches->name, node->name);
> > -		if (matches->type[0])
> > -			match &= node->type
> > -				&& !strcmp(matches->type, node->type);
> > -		if (matches->compatible[0])
> > -			match &= __of_device_is_compatible(node,
> > -							   matches->compatible);
> > -		if (match)
> > -			return matches;
> > -		matches++;
> > -	}
> > +	cp = __of_get_property(node, "compatible", &cplen);
> > +	do {
> > +		const struct of_device_id *m = matches;
> > +
> > +		/* Check against matches with current compatible string */
> > +		while (m->name[0] || m->type[0] || m->compatible[0]) {
> > +			int match = 1;
> > +			if (m->name[0])
> > +				match &= node->name
> > +					&& !strcmp(m->name, node->name);
> > +			if (m->type[0])
> > +				match &= node->type
> > +					&& !strcmp(m->type, node->type);
> > +			if (m->compatible[0])
> > +				match &= cp
> > +					&& !of_compat_cmp(m->compatible, cp,
> > +							strlen(m->compatible));
> > +			if (match)
> > +				return m;
> > +			m++;
> > +		}
> > +
> > +		/* Get node's next compatible string */
> > +		if (cp) {
> > +			l = strlen(cp) + 1;
> > +			cp += l;
> > +			cplen -= l;
> > +		}
> > +	} while (cp && (cplen > 0));
> > +
> >   	return NULL;
> >   }
> >
> > @@ -757,7 +775,10 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> >    *	@matches:	array of of device match structures to search in
> >    *	@node:		the of device structure to match against
> >    *
> > - *	Low level utility function used by device matching.
> > + *	Low level utility function used by device matching. Matching order
> > + *	is to compare each of the node's compatibles with all given matches
> > + *	first. This implies node's compatible is sorted from specific to
> > + *	generic while matches can be in any order.
> >    */
> >   const struct of_device_id *of_match_node(const struct of_device_id *matches,
> >   					 const struct device_node *node)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/