* [PATCH] of: give priority to the compatible match in __of_match_node()
@ 2014-02-12 11:38 Kevin Hao
[not found] ` < CAL_JsqKMi2H=vwoxrOt8QRA2xJeiLqBKKfLtt4QRCRoFk6JUHg@mail.gmail.com>
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Kevin Hao @ 2014-02-12 11:38 UTC (permalink / raw)
To: devicetree, linuxppc-dev
Cc: Chris Proctor, Arnd Bergmann, Stephen N Chivers, Grant Likely,
Rob Herring, Scott Wood, Sebastian Hesselbarth
When the device node do have a compatible property, we definitely
prefer the compatible match besides the type and name. Only if
there is no such a match, we then consider the candidate which
doesn't have compatible entry but do match the type or name with
the device node.
This is based on a patch from Sebastian Hesselbarth.
http://patchwork.ozlabs.org/patch/319434/
I did some code refactoring and also fixed a bug in the original patch.
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 18 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index ff85450d5683..9d655df458bd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -730,32 +730,45 @@ out:
}
EXPORT_SYMBOL(of_find_node_with_property);
+static int of_match_type_or_name(const struct device_node *node,
+ const struct of_device_id *m)
+{
+ 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);
+
+ return match;
+}
+
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;
+ const struct of_device_id *m;
+ int match;
if (!matches)
return NULL;
cp = __of_get_property(node, "compatible", &cplen);
- do {
- const struct of_device_id *m = matches;
+ while (cp && (cplen > 0)) {
+ 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,
+ if (!m->compatible[0]) {
+ m++;
+ continue;
+ }
+
+ match = of_match_type_or_name(node, m);
+ match &= cp && !of_compat_cmp(m->compatible, cp,
strlen(m->compatible));
if (match)
return m;
@@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
}
/* Get node's next compatible string */
- if (cp) {
- l = strlen(cp) + 1;
- cp += l;
- cplen -= l;
- }
- } while (cp && (cplen > 0));
+ l = strlen(cp) + 1;
+ cp += l;
+ cplen -= l;
+ }
+
+ m = matches;
+ /* Check against matches without compatible string */
+ while (m->name[0] || m->type[0] || m->compatible[0]) {
+ if (!m->compatible[0] && of_match_type_or_name(node, m))
+ return m;
+ m++;
+ }
return NULL;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 12+ messages in thread[parent not found: < CAL_JsqKMi2H=vwoxrOt8QRA2xJeiLqBKKfLtt4QRCRoFk6JUHg@mail.gmail.com>]
[parent not found: < CAP=VYLr7hqnzW2HLS4GCMFeghCgPmrQZHYst+AUfZc_WNT-Wog@mail.gmail.com>]
[parent not found: < 20140219204134.E4A2DC4088D@trevor.secretlab.ca>]
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-12 11:38 [PATCH] of: give priority to the compatible match in __of_match_node() Kevin Hao [not found] ` < CAL_JsqKMi2H=vwoxrOt8QRA2xJeiLqBKKfLtt4QRCRoFk6JUHg@mail.gmail.com> @ 2014-02-12 20:42 ` Stephen N Chivers 2014-02-13 19:01 ` Rob Herring 2 siblings, 0 replies; 12+ messages in thread From: Stephen N Chivers @ 2014-02-12 20:42 UTC (permalink / raw) To: Kevin Hao Cc: Chris Proctor, Arnd Bergmann, devicetree, Stephen N Chivers, Scott Wood, Rob Herring, Grant Likely, linuxppc-dev, Sebastian Hesselbarth Kevin Hao <haokexin@gmail.com> wrote on 02/12/2014 10:38:04 PM: > From: Kevin Hao <haokexin@gmail.com> > To: devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>, Stephen > N Chivers <schivers@csc.com.au>, Chris Proctor > <cproctor@csc.com.au>, Arnd Bergmann <arnd@arndb.de>, Scott Wood > <scottwood@freescale.com>, Grant Likely <grant.likely@linaro.org>, > Rob Herring <robh+dt@kernel.org> > Date: 02/12/2014 10:38 PM > Subject: [PATCH] of: give priority to the compatible match in > __of_match_node() > > When the device node do have a compatible property, we definitely > prefer the compatible match besides the type and name. Only if > there is no such a match, we then consider the candidate which > doesn't have compatible entry but do match the type or name with > the device node. > > This is based on a patch from Sebastian Hesselbarth. > http://patchwork.ozlabs.org/patch/319434/ > > I did some code refactoring and also fixed a bug in the original patch. > > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Signed-off-by: Kevin Hao <haokexin@gmail.com> Tested-by: Stephen Chivers <schivers@csc.com> Patch works for both orderings. Platform boots without problems and I get the normal serial console. > --- > drivers/of/base.c | 55 ++++++++++++++++++++++++++++++++++++ > +------------------ > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index ff85450d5683..9d655df458bd 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -730,32 +730,45 @@ out: > } > EXPORT_SYMBOL(of_find_node_with_property); > > +static int of_match_type_or_name(const struct device_node *node, > + const struct of_device_id *m) > +{ > + 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); > + > + return match; > +} > + > 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; > + const struct of_device_id *m; > + int match; > > if (!matches) > return NULL; > > cp = __of_get_property(node, "compatible", &cplen); > - do { > - const struct of_device_id *m = matches; > + while (cp && (cplen > 0)) { > + 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, > + if (!m->compatible[0]) { > + m++; > + continue; > + } > + > + match = of_match_type_or_name(node, m); > + match &= cp && !of_compat_cmp(m->compatible, cp, > strlen(m->compatible)); > if (match) > return m; > @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node > (const struct of_device_id *matches, > } > > /* Get node's next compatible string */ > - if (cp) { > - l = strlen(cp) + 1; > - cp += l; > - cplen -= l; > - } > - } while (cp && (cplen > 0)); > + l = strlen(cp) + 1; > + cp += l; > + cplen -= l; > + } > + > + m = matches; > + /* Check against matches without compatible string */ > + while (m->name[0] || m->type[0] || m->compatible[0]) { > + if (!m->compatible[0] && of_match_type_or_name(node, m)) > + return m; > + m++; > + } > > return NULL; > } > -- > 1.8.5.3 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-12 11:38 [PATCH] of: give priority to the compatible match in __of_match_node() Kevin Hao [not found] ` < CAL_JsqKMi2H=vwoxrOt8QRA2xJeiLqBKKfLtt4QRCRoFk6JUHg@mail.gmail.com> 2014-02-12 20:42 ` Stephen N Chivers @ 2014-02-13 19:01 ` Rob Herring 2014-02-14 1:20 ` Kevin Hao ` (2 more replies) 2 siblings, 3 replies; 12+ messages in thread From: Rob Herring @ 2014-02-13 19:01 UTC (permalink / raw) To: Kevin Hao Cc: devicetree@vger.kernel.org, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Grant Likely, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: > When the device node do have a compatible property, we definitely > prefer the compatible match besides the type and name. Only if > there is no such a match, we then consider the candidate which > doesn't have compatible entry but do match the type or name with > the device node. > > This is based on a patch from Sebastian Hesselbarth. > http://patchwork.ozlabs.org/patch/319434/ > > I did some code refactoring and also fixed a bug in the original patch. I'm inclined to just revert this once again and avoid possibly breaking yet another platform. However, I think I would like to see this structured differently. We basically have 2 ways of matching: the existing pre-3.14 way and the desired match on best compatible only. All new bindings should match with the new way and the old way needs to be kept for compatibility. So lets structure the code that way. Search the match table first for best compatible with name and type NULL, then search the table the old way. I realize it appears you are doing this, but it is not clear this is the intent of the code. I would like to see this written as a patch with commit 105353145eafb3ea919 reverted first and you add a new match function to call first and then fallback to the existing function. Rob > > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Signed-off-by: Kevin Hao <haokexin@gmail.com> > --- > drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index ff85450d5683..9d655df458bd 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -730,32 +730,45 @@ out: > } > EXPORT_SYMBOL(of_find_node_with_property); > > +static int of_match_type_or_name(const struct device_node *node, > + const struct of_device_id *m) > +{ > + 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); > + > + return match; > +} > + > 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; > + const struct of_device_id *m; > + int match; > > if (!matches) > return NULL; > > cp = __of_get_property(node, "compatible", &cplen); > - do { > - const struct of_device_id *m = matches; > + while (cp && (cplen > 0)) { > + 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, > + if (!m->compatible[0]) { > + m++; > + continue; > + } > + > + match = of_match_type_or_name(node, m); > + match &= cp && !of_compat_cmp(m->compatible, cp, > strlen(m->compatible)); > if (match) > return m; > @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches, > } > > /* Get node's next compatible string */ > - if (cp) { > - l = strlen(cp) + 1; > - cp += l; > - cplen -= l; > - } > - } while (cp && (cplen > 0)); > + l = strlen(cp) + 1; > + cp += l; > + cplen -= l; > + } > + > + m = matches; > + /* Check against matches without compatible string */ > + while (m->name[0] || m->type[0] || m->compatible[0]) { > + if (!m->compatible[0] && of_match_type_or_name(node, m)) > + return m; > + m++; > + } > > return NULL; > } > -- > 1.8.5.3 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-13 19:01 ` Rob Herring @ 2014-02-14 1:20 ` Kevin Hao 2014-02-17 18:06 ` Grant Likely 2014-02-19 18:25 ` Paul Gortmaker 2 siblings, 0 replies; 12+ messages in thread From: Kevin Hao @ 2014-02-14 1:20 UTC (permalink / raw) To: Rob Herring Cc: devicetree@vger.kernel.org, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Grant Likely, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth [-- Attachment #1: Type: text/plain, Size: 1510 bytes --] On Thu, Feb 13, 2014 at 01:01:42PM -0600, Rob Herring wrote: > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: > > When the device node do have a compatible property, we definitely > > prefer the compatible match besides the type and name. Only if > > there is no such a match, we then consider the candidate which > > doesn't have compatible entry but do match the type or name with > > the device node. > > > > This is based on a patch from Sebastian Hesselbarth. > > http://patchwork.ozlabs.org/patch/319434/ > > > > I did some code refactoring and also fixed a bug in the original patch. > > I'm inclined to just revert this once again and avoid possibly > breaking yet another platform. > > However, I think I would like to see this structured differently. We > basically have 2 ways of matching: the existing pre-3.14 way and the > desired match on best compatible only. All new bindings should match > with the new way and the old way needs to be kept for compatibility. > So lets structure the code that way. Search the match table first for > best compatible with name and type NULL, then search the table the old > way. I realize it appears you are doing this, but it is not clear this > is the intent of the code. I would like to see this written as a patch > with commit 105353145eafb3ea919 reverted first and you add a new match > function to call first and then fallback to the existing function. OK, I will git it a try. Thanks, Kevin [-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-13 19:01 ` Rob Herring 2014-02-14 1:20 ` Kevin Hao @ 2014-02-17 18:06 ` Grant Likely 2014-02-19 18:25 ` Paul Gortmaker 2 siblings, 0 replies; 12+ messages in thread From: Grant Likely @ 2014-02-17 18:06 UTC (permalink / raw) To: Rob Herring, Kevin Hao Cc: devicetree@vger.kernel.org, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth On Thu, 13 Feb 2014 13:01:42 -0600, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: > > When the device node do have a compatible property, we definitely > > prefer the compatible match besides the type and name. Only if > > there is no such a match, we then consider the candidate which > > doesn't have compatible entry but do match the type or name with > > the device node. > > > > This is based on a patch from Sebastian Hesselbarth. > > http://patchwork.ozlabs.org/patch/319434/ > > > > I did some code refactoring and also fixed a bug in the original patch. > > I'm inclined to just revert this once again and avoid possibly > breaking yet another platform. > > However, I think I would like to see this structured differently. We > basically have 2 ways of matching: the existing pre-3.14 way and the > desired match on best compatible only. All new bindings should match > with the new way and the old way needs to be kept for compatibility. > So lets structure the code that way. Search the match table first for > best compatible with name and type NULL, then search the table the old > way. I realize it appears you are doing this, but it is not clear this > is the intent of the code. I would like to see this written as a patch > with commit 105353145eafb3ea919 reverted first and you add a new match > function to call first and then fallback to the existing function. I disagree here, partially because it complicates the code and partially because it leaves an incorrect corner case. Compatible is always the preferred matching, even on old drivers, but there are bindings that take both compatible and type or name into account. Even those cases should rank the compatible order. g. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-13 19:01 ` Rob Herring 2014-02-14 1:20 ` Kevin Hao 2014-02-17 18:06 ` Grant Likely @ 2014-02-19 18:25 ` Paul Gortmaker 2014-02-19 18:57 ` Scott Wood 2014-02-19 20:41 ` Grant Likely 2 siblings, 2 replies; 12+ messages in thread From: Paul Gortmaker @ 2014-02-19 18:25 UTC (permalink / raw) To: Rob Herring Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Scott Wood, Rob Herring, Grant Likely, linuxppc-dev, Sebastian Hesselbarth On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: >> When the device node do have a compatible property, we definitely >> prefer the compatible match besides the type and name. Only if >> there is no such a match, we then consider the candidate which >> doesn't have compatible entry but do match the type or name with >> the device node. >> >> This is based on a patch from Sebastian Hesselbarth. >> http://patchwork.ozlabs.org/patch/319434/ >> >> I did some code refactoring and also fixed a bug in the original patch. > > I'm inclined to just revert this once again and avoid possibly > breaking yet another platform. Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot on my sbc8548. It fails with: ----------------------------------------------- libphy: Freescale PowerQUICC MII Bus: probed mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520 PHY address 1312 is too large libphy: Freescale PowerQUICC MII Bus: probed libphy: Freescale PowerQUICC MII Bus: probed mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520 PHY address 1312 is too large libphy: Freescale PowerQUICC MII Bus: probed TCP: cubic registered Initializing XFRM netlink socket NET: Registered protocol family 17 <fail nfs mount> ----------------------------------------------- On a normal boot, we should see this: ----------------------------------------------- libphy: Freescale PowerQUICC MII Bus: probed libphy: Freescale PowerQUICC MII Bus: probed fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 TCP: cubic registered Initializing XFRM netlink socket NET: Registered protocol family 17 ----------------------------------------------- Git bisect says: ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 Author: Kevin Hao <haokexin@gmail.com> Date: Tue Feb 18 15:57:30 2014 +0800 of: reimplement the matching method for __of_match_node() In the current implementation of __of_match_node(), it will compare each given match entry against all the 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, we define a following priority order for the match, and then scan all the entries to find the best match. 1. specific compatible && type && name 2. specific compatible && type 3. specific compatible && name 4. specific compatible 5. general compatible && type && name 6. general compatible && type 7. general compatible && name 8. general compatible 9. type && name 10. type 11. name This is based on some pseudo-codes provided by Grant Likely. Signed-off-by: Kevin Hao <haokexin@gmail.com> [grant.likely: Changed multiplier to 4 which makes more sense] Signed-off-by: Grant Likely <grant.likely@linaro.org> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers As a double check, I checked out the head of linux-next, and did a revert of the above commit, and my sbc8548 can then boot properly. Not doing anything fancy ; using the defconfig exactly as-is, and ensuring the dtb is fresh from linux-next HEAD of today. Thanks, Paul. -- > > However, I think I would like to see this structured differently. We > basically have 2 ways of matching: the existing pre-3.14 way and the > desired match on best compatible only. All new bindings should match > with the new way and the old way needs to be kept for compatibility. > So lets structure the code that way. Search the match table first for > best compatible with name and type NULL, then search the table the old > way. I realize it appears you are doing this, but it is not clear this > is the intent of the code. I would like to see this written as a patch > with commit 105353145eafb3ea919 reverted first and you add a new match > function to call first and then fallback to the existing function. > > Rob > >> >> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> >> Signed-off-by: Kevin Hao <haokexin@gmail.com> >> --- >> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 37 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/of/base.c b/drivers/of/base.c >> index ff85450d5683..9d655df458bd 100644 >> --- a/drivers/of/base.c >> +++ b/drivers/of/base.c >> @@ -730,32 +730,45 @@ out: >> } >> EXPORT_SYMBOL(of_find_node_with_property); >> >> +static int of_match_type_or_name(const struct device_node *node, >> + const struct of_device_id *m) >> +{ >> + 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); >> + >> + return match; >> +} >> + >> 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; >> + const struct of_device_id *m; >> + int match; >> >> if (!matches) >> return NULL; >> >> cp = __of_get_property(node, "compatible", &cplen); >> - do { >> - const struct of_device_id *m = matches; >> + while (cp && (cplen > 0)) { >> + 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, >> + if (!m->compatible[0]) { >> + m++; >> + continue; >> + } >> + >> + match = of_match_type_or_name(node, m); >> + match &= cp && !of_compat_cmp(m->compatible, cp, >> strlen(m->compatible)); >> if (match) >> return m; >> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches, >> } >> >> /* Get node's next compatible string */ >> - if (cp) { >> - l = strlen(cp) + 1; >> - cp += l; >> - cplen -= l; >> - } >> - } while (cp && (cplen > 0)); >> + l = strlen(cp) + 1; >> + cp += l; >> + cplen -= l; >> + } >> + >> + m = matches; >> + /* Check against matches without compatible string */ >> + while (m->name[0] || m->type[0] || m->compatible[0]) { >> + if (!m->compatible[0] && of_match_type_or_name(node, m)) >> + return m; >> + m++; >> + } >> >> return NULL; >> } >> -- >> 1.8.5.3 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-19 18:25 ` Paul Gortmaker @ 2014-02-19 18:57 ` Scott Wood 2014-02-19 20:41 ` Grant Likely 1 sibling, 0 replies; 12+ messages in thread From: Scott Wood @ 2014-02-19 18:57 UTC (permalink / raw) To: Paul Gortmaker Cc: Chris Proctor, Kevin Hao, Arnd Bergmann, devicetree@vger.kernel.org, Stephen N Chivers, Rob Herring, Rob Herring, Grant Likely, linuxppc-dev, Sebastian Hesselbarth On Wed, 2014-02-19 at 13:25 -0500, Paul Gortmaker wrote: > On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote: > > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: > >> When the device node do have a compatible property, we definitely > >> prefer the compatible match besides the type and name. Only if > >> there is no such a match, we then consider the candidate which > >> doesn't have compatible entry but do match the type or name with > >> the device node. > >> > >> This is based on a patch from Sebastian Hesselbarth. > >> http://patchwork.ozlabs.org/patch/319434/ > >> > >> I did some code refactoring and also fixed a bug in the original patch. > > > > I'm inclined to just revert this once again and avoid possibly > > breaking yet another platform. > > Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot > on my sbc8548. It fails with: > ----------------------------------------------- > libphy: Freescale PowerQUICC MII Bus: probed > mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520 > PHY address 1312 is too large > libphy: Freescale PowerQUICC MII Bus: probed > libphy: Freescale PowerQUICC MII Bus: probed > mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520 > PHY address 1312 is too large > libphy: Freescale PowerQUICC MII Bus: probed > TCP: cubic registered > Initializing XFRM netlink socket > NET: Registered protocol family 17 > <fail nfs mount> > ----------------------------------------------- > > On a normal boot, we should see this: > ----------------------------------------------- > libphy: Freescale PowerQUICC MII Bus: probed > libphy: Freescale PowerQUICC MII Bus: probed > fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 > fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd > fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled > fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 > fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 > fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 > fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd > fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled > fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 > fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 > TCP: cubic registered > Initializing XFRM netlink socket > NET: Registered protocol family 17 > ----------------------------------------------- > > > Git bisect says: > > ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit > commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 > Author: Kevin Hao <haokexin@gmail.com> > Date: Tue Feb 18 15:57:30 2014 +0800 > > of: reimplement the matching method for __of_match_node() > > In the current implementation of __of_match_node(), it will compare > each given match entry against all the 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, we define a following priority order for the match, and > then scan all the entries to find the best match. > 1. specific compatible && type && name > 2. specific compatible && type > 3. specific compatible && name > 4. specific compatible > 5. general compatible && type && name > 6. general compatible && type > 7. general compatible && name > 8. general compatible > 9. type && name > 10. type > 11. name > > This is based on some pseudo-codes provided by Grant Likely. > > Signed-off-by: Kevin Hao <haokexin@gmail.com> > [grant.likely: Changed multiplier to 4 which makes more sense] > Signed-off-by: Grant Likely <grant.likely@linaro.org> > > :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0 > 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers > > As a double check, I checked out the head of linux-next, and did a > revert of the above commit, and my sbc8548 can then boot properly. > > Not doing anything fancy ; using the defconfig exactly as-is, and > ensuring the dtb is fresh from linux-next HEAD of today. It looks like the new matching function doesn't check the validity of the entire match (i.e. everything specified must match the node) before scoring it. The ethernet driver has this match (among others): { .type = "network", .compatible = "gianfar", }, ...while the mdio driver has this match (among others): { .type = "mdio", .compatible = "gianfar", .data = &(struct fsl_pq_mdio_data) { .mii_offset = offsetof(struct fsl_pq_mdio, mii), .get_tbipa = get_gfar_tbipa, }, }, (Yes, it's an awful device tree binding.) It seems that the ethernet device is being bound to the mdio driver due to the compatible match, even though type differs. You could also end up with a .type and/or .name based match, despite .compatible being present an a mismatch. -Scott ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-19 18:25 ` Paul Gortmaker 2014-02-19 18:57 ` Scott Wood @ 2014-02-19 20:41 ` Grant Likely 2014-02-19 21:23 ` Paul Gortmaker 2014-02-20 2:05 ` Stephen N Chivers 1 sibling, 2 replies; 12+ messages in thread From: Grant Likely @ 2014-02-19 20:41 UTC (permalink / raw) To: Paul Gortmaker, Rob Herring Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote: > > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: > >> When the device node do have a compatible property, we definitely > >> prefer the compatible match besides the type and name. Only if > >> there is no such a match, we then consider the candidate which > >> doesn't have compatible entry but do match the type or name with > >> the device node. > >> > >> This is based on a patch from Sebastian Hesselbarth. > >> http://patchwork.ozlabs.org/patch/319434/ > >> > >> I did some code refactoring and also fixed a bug in the original patch. > > > > I'm inclined to just revert this once again and avoid possibly > > breaking yet another platform. > > Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot > on my sbc8548. It fails with: I think I've got it fixed now with the latest series. Please try the devicetree/merge branch on git://git.secretlab.ca/git/linux g. > ----------------------------------------------- > libphy: Freescale PowerQUICC MII Bus: probed > mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520 > PHY address 1312 is too large > libphy: Freescale PowerQUICC MII Bus: probed > libphy: Freescale PowerQUICC MII Bus: probed > mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520 > PHY address 1312 is too large > libphy: Freescale PowerQUICC MII Bus: probed > TCP: cubic registered > Initializing XFRM netlink socket > NET: Registered protocol family 17 > <fail nfs mount> > ----------------------------------------------- > > On a normal boot, we should see this: > ----------------------------------------------- > libphy: Freescale PowerQUICC MII Bus: probed > libphy: Freescale PowerQUICC MII Bus: probed > fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 > fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd > fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled > fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 > fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 > fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 > fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd > fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled > fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 > fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 > TCP: cubic registered > Initializing XFRM netlink socket > NET: Registered protocol family 17 > ----------------------------------------------- > > > Git bisect says: > > ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit > commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 > Author: Kevin Hao <haokexin@gmail.com> > Date: Tue Feb 18 15:57:30 2014 +0800 > > of: reimplement the matching method for __of_match_node() > > In the current implementation of __of_match_node(), it will compare > each given match entry against all the 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, we define a following priority order for the match, and > then scan all the entries to find the best match. > 1. specific compatible && type && name > 2. specific compatible && type > 3. specific compatible && name > 4. specific compatible > 5. general compatible && type && name > 6. general compatible && type > 7. general compatible && name > 8. general compatible > 9. type && name > 10. type > 11. name > > This is based on some pseudo-codes provided by Grant Likely. > > Signed-off-by: Kevin Hao <haokexin@gmail.com> > [grant.likely: Changed multiplier to 4 which makes more sense] > Signed-off-by: Grant Likely <grant.likely@linaro.org> > > :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0 > 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers > > As a double check, I checked out the head of linux-next, and did a > revert of the above commit, and my sbc8548 can then boot properly. > > Not doing anything fancy ; using the defconfig exactly as-is, and > ensuring the dtb is fresh from linux-next HEAD of today. > > Thanks, > Paul. > -- > > > > > However, I think I would like to see this structured differently. We > > basically have 2 ways of matching: the existing pre-3.14 way and the > > desired match on best compatible only. All new bindings should match > > with the new way and the old way needs to be kept for compatibility. > > So lets structure the code that way. Search the match table first for > > best compatible with name and type NULL, then search the table the old > > way. I realize it appears you are doing this, but it is not clear this > > is the intent of the code. I would like to see this written as a patch > > with commit 105353145eafb3ea919 reverted first and you add a new match > > function to call first and then fallback to the existing function. > > > > Rob > > > >> > >> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > >> Signed-off-by: Kevin Hao <haokexin@gmail.com> > >> --- > >> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------ > >> 1 file changed, 37 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/of/base.c b/drivers/of/base.c > >> index ff85450d5683..9d655df458bd 100644 > >> --- a/drivers/of/base.c > >> +++ b/drivers/of/base.c > >> @@ -730,32 +730,45 @@ out: > >> } > >> EXPORT_SYMBOL(of_find_node_with_property); > >> > >> +static int of_match_type_or_name(const struct device_node *node, > >> + const struct of_device_id *m) > >> +{ > >> + 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); > >> + > >> + return match; > >> +} > >> + > >> 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; > >> + const struct of_device_id *m; > >> + int match; > >> > >> if (!matches) > >> return NULL; > >> > >> cp = __of_get_property(node, "compatible", &cplen); > >> - do { > >> - const struct of_device_id *m = matches; > >> + while (cp && (cplen > 0)) { > >> + 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, > >> + if (!m->compatible[0]) { > >> + m++; > >> + continue; > >> + } > >> + > >> + match = of_match_type_or_name(node, m); > >> + match &= cp && !of_compat_cmp(m->compatible, cp, > >> strlen(m->compatible)); > >> if (match) > >> return m; > >> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches, > >> } > >> > >> /* Get node's next compatible string */ > >> - if (cp) { > >> - l = strlen(cp) + 1; > >> - cp += l; > >> - cplen -= l; > >> - } > >> - } while (cp && (cplen > 0)); > >> + l = strlen(cp) + 1; > >> + cp += l; > >> + cplen -= l; > >> + } > >> + > >> + m = matches; > >> + /* Check against matches without compatible string */ > >> + while (m->name[0] || m->type[0] || m->compatible[0]) { > >> + if (!m->compatible[0] && of_match_type_or_name(node, m)) > >> + return m; > >> + m++; > >> + } > >> > >> return NULL; > >> } > >> -- > >> 1.8.5.3 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe devicetree" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > _______________________________________________ > > Linuxppc-dev mailing list > > Linuxppc-dev@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/linuxppc-dev ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-19 20:41 ` Grant Likely @ 2014-02-19 21:23 ` Paul Gortmaker 2014-02-19 22:40 ` Grant Likely 2014-02-20 2:05 ` Stephen N Chivers 1 sibling, 1 reply; 12+ messages in thread From: Paul Gortmaker @ 2014-02-19 21:23 UTC (permalink / raw) To: Grant Likely, Rob Herring Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth On 14-02-19 03:41 PM, Grant Likely wrote: > On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote: >> On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote: >>> On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: >>>> When the device node do have a compatible property, we definitely >>>> prefer the compatible match besides the type and name. Only if >>>> there is no such a match, we then consider the candidate which >>>> doesn't have compatible entry but do match the type or name with >>>> the device node. >>>> >>>> This is based on a patch from Sebastian Hesselbarth. >>>> http://patchwork.ozlabs.org/patch/319434/ >>>> >>>> I did some code refactoring and also fixed a bug in the original patch. >>> >>> I'm inclined to just revert this once again and avoid possibly >>> breaking yet another platform. >> >> Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot >> on my sbc8548. It fails with: > > I think I've got it fixed now with the latest series. Please try the > devicetree/merge branch on git://git.secretlab.ca/git/linux That seems to work. libphy: Freescale PowerQUICC MII Bus: probed libphy: Freescale PowerQUICC MII Bus: probed fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 TCP: cubic registered Initializing XFRM netlink socket NET: Registered protocol family 17 ... root@sbc8548:~# cat /proc/version Linux version 3.14.0-rc3-00024-g7119f42057c6 (paul@yow-lpgnfs-02) (gcc version 4.5.2 (GCC) ) #1 Wed Feb 19 16:08:17 EST 2014 root@sbc8548:~# Paul. -- > > g. > >> ----------------------------------------------- >> libphy: Freescale PowerQUICC MII Bus: probed >> mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520 >> PHY address 1312 is too large >> libphy: Freescale PowerQUICC MII Bus: probed >> libphy: Freescale PowerQUICC MII Bus: probed >> mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520 >> PHY address 1312 is too large >> libphy: Freescale PowerQUICC MII Bus: probed >> TCP: cubic registered >> Initializing XFRM netlink socket >> NET: Registered protocol family 17 >> <fail nfs mount> >> ----------------------------------------------- >> >> On a normal boot, we should see this: >> ----------------------------------------------- >> libphy: Freescale PowerQUICC MII Bus: probed >> libphy: Freescale PowerQUICC MII Bus: probed >> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 >> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd >> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled >> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 >> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 >> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 >> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd >> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled >> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 >> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 >> TCP: cubic registered >> Initializing XFRM netlink socket >> NET: Registered protocol family 17 >> ----------------------------------------------- >> >> >> Git bisect says: >> >> ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit >> commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 >> Author: Kevin Hao <haokexin@gmail.com> >> Date: Tue Feb 18 15:57:30 2014 +0800 >> >> of: reimplement the matching method for __of_match_node() >> >> In the current implementation of __of_match_node(), it will compare >> each given match entry against all the 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, we define a following priority order for the match, and >> then scan all the entries to find the best match. >> 1. specific compatible && type && name >> 2. specific compatible && type >> 3. specific compatible && name >> 4. specific compatible >> 5. general compatible && type && name >> 6. general compatible && type >> 7. general compatible && name >> 8. general compatible >> 9. type && name >> 10. type >> 11. name >> >> This is based on some pseudo-codes provided by Grant Likely. >> >> Signed-off-by: Kevin Hao <haokexin@gmail.com> >> [grant.likely: Changed multiplier to 4 which makes more sense] >> Signed-off-by: Grant Likely <grant.likely@linaro.org> >> >> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0 >> 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers >> >> As a double check, I checked out the head of linux-next, and did a >> revert of the above commit, and my sbc8548 can then boot properly. >> >> Not doing anything fancy ; using the defconfig exactly as-is, and >> ensuring the dtb is fresh from linux-next HEAD of today. >> >> Thanks, >> Paul. >> -- >> >>> >>> However, I think I would like to see this structured differently. We >>> basically have 2 ways of matching: the existing pre-3.14 way and the >>> desired match on best compatible only. All new bindings should match >>> with the new way and the old way needs to be kept for compatibility. >>> So lets structure the code that way. Search the match table first for >>> best compatible with name and type NULL, then search the table the old >>> way. I realize it appears you are doing this, but it is not clear this >>> is the intent of the code. I would like to see this written as a patch >>> with commit 105353145eafb3ea919 reverted first and you add a new match >>> function to call first and then fallback to the existing function. >>> >>> Rob >>> >>>> >>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> >>>> Signed-off-by: Kevin Hao <haokexin@gmail.com> >>>> --- >>>> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------ >>>> 1 file changed, 37 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/of/base.c b/drivers/of/base.c >>>> index ff85450d5683..9d655df458bd 100644 >>>> --- a/drivers/of/base.c >>>> +++ b/drivers/of/base.c >>>> @@ -730,32 +730,45 @@ out: >>>> } >>>> EXPORT_SYMBOL(of_find_node_with_property); >>>> >>>> +static int of_match_type_or_name(const struct device_node *node, >>>> + const struct of_device_id *m) >>>> +{ >>>> + 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); >>>> + >>>> + return match; >>>> +} >>>> + >>>> 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; >>>> + const struct of_device_id *m; >>>> + int match; >>>> >>>> if (!matches) >>>> return NULL; >>>> >>>> cp = __of_get_property(node, "compatible", &cplen); >>>> - do { >>>> - const struct of_device_id *m = matches; >>>> + while (cp && (cplen > 0)) { >>>> + 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, >>>> + if (!m->compatible[0]) { >>>> + m++; >>>> + continue; >>>> + } >>>> + >>>> + match = of_match_type_or_name(node, m); >>>> + match &= cp && !of_compat_cmp(m->compatible, cp, >>>> strlen(m->compatible)); >>>> if (match) >>>> return m; >>>> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches, >>>> } >>>> >>>> /* Get node's next compatible string */ >>>> - if (cp) { >>>> - l = strlen(cp) + 1; >>>> - cp += l; >>>> - cplen -= l; >>>> - } >>>> - } while (cp && (cplen > 0)); >>>> + l = strlen(cp) + 1; >>>> + cp += l; >>>> + cplen -= l; >>>> + } >>>> + >>>> + m = matches; >>>> + /* Check against matches without compatible string */ >>>> + while (m->name[0] || m->type[0] || m->compatible[0]) { >>>> + if (!m->compatible[0] && of_match_type_or_name(node, m)) >>>> + return m; >>>> + m++; >>>> + } >>>> >>>> return NULL; >>>> } >>>> -- >>>> 1.8.5.3 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> _______________________________________________ >>> Linuxppc-dev mailing list >>> Linuxppc-dev@lists.ozlabs.org >>> https://lists.ozlabs.org/listinfo/linuxppc-dev > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-19 21:23 ` Paul Gortmaker @ 2014-02-19 22:40 ` Grant Likely 2014-02-19 22:44 ` Paul Gortmaker 0 siblings, 1 reply; 12+ messages in thread From: Grant Likely @ 2014-02-19 22:40 UTC (permalink / raw) To: Paul Gortmaker, Rob Herring Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth On Wed, 19 Feb 2014 16:23:02 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > On 14-02-19 03:41 PM, Grant Likely wrote: > > On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote: > >> On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote: > >>> On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: > >>>> When the device node do have a compatible property, we definitely > >>>> prefer the compatible match besides the type and name. Only if > >>>> there is no such a match, we then consider the candidate which > >>>> doesn't have compatible entry but do match the type or name with > >>>> the device node. > >>>> > >>>> This is based on a patch from Sebastian Hesselbarth. > >>>> http://patchwork.ozlabs.org/patch/319434/ > >>>> > >>>> I did some code refactoring and also fixed a bug in the original patch. > >>> > >>> I'm inclined to just revert this once again and avoid possibly > >>> breaking yet another platform. > >> > >> Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot > >> on my sbc8548. It fails with: > > > > I think I've got it fixed now with the latest series. Please try the > > devicetree/merge branch on git://git.secretlab.ca/git/linux > > That seems to work. Great, thanks for the testing. Can I add a Tested-by line for you? g. > > libphy: Freescale PowerQUICC MII Bus: probed > libphy: Freescale PowerQUICC MII Bus: probed > fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 > fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd > fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled > fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 > fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 > fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 > fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd > fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled > fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 > fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 > TCP: cubic registered > Initializing XFRM netlink socket > NET: Registered protocol family 17 > > ... > > root@sbc8548:~# cat /proc/version > Linux version 3.14.0-rc3-00024-g7119f42057c6 (paul@yow-lpgnfs-02) (gcc version 4.5.2 (GCC) ) #1 Wed Feb 19 16:08:17 EST 2014 > root@sbc8548:~# > > Paul. > -- > > > > > g. > > > >> ----------------------------------------------- > >> libphy: Freescale PowerQUICC MII Bus: probed > >> mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520 > >> PHY address 1312 is too large > >> libphy: Freescale PowerQUICC MII Bus: probed > >> libphy: Freescale PowerQUICC MII Bus: probed > >> mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520 > >> PHY address 1312 is too large > >> libphy: Freescale PowerQUICC MII Bus: probed > >> TCP: cubic registered > >> Initializing XFRM netlink socket > >> NET: Registered protocol family 17 > >> <fail nfs mount> > >> ----------------------------------------------- > >> > >> On a normal boot, we should see this: > >> ----------------------------------------------- > >> libphy: Freescale PowerQUICC MII Bus: probed > >> libphy: Freescale PowerQUICC MII Bus: probed > >> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 > >> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd > >> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled > >> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 > >> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 > >> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 > >> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd > >> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled > >> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 > >> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 > >> TCP: cubic registered > >> Initializing XFRM netlink socket > >> NET: Registered protocol family 17 > >> ----------------------------------------------- > >> > >> > >> Git bisect says: > >> > >> ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit > >> commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 > >> Author: Kevin Hao <haokexin@gmail.com> > >> Date: Tue Feb 18 15:57:30 2014 +0800 > >> > >> of: reimplement the matching method for __of_match_node() > >> > >> In the current implementation of __of_match_node(), it will compare > >> each given match entry against all the 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, we define a following priority order for the match, and > >> then scan all the entries to find the best match. > >> 1. specific compatible && type && name > >> 2. specific compatible && type > >> 3. specific compatible && name > >> 4. specific compatible > >> 5. general compatible && type && name > >> 6. general compatible && type > >> 7. general compatible && name > >> 8. general compatible > >> 9. type && name > >> 10. type > >> 11. name > >> > >> This is based on some pseudo-codes provided by Grant Likely. > >> > >> Signed-off-by: Kevin Hao <haokexin@gmail.com> > >> [grant.likely: Changed multiplier to 4 which makes more sense] > >> Signed-off-by: Grant Likely <grant.likely@linaro.org> > >> > >> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0 > >> 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers > >> > >> As a double check, I checked out the head of linux-next, and did a > >> revert of the above commit, and my sbc8548 can then boot properly. > >> > >> Not doing anything fancy ; using the defconfig exactly as-is, and > >> ensuring the dtb is fresh from linux-next HEAD of today. > >> > >> Thanks, > >> Paul. > >> -- > >> > >>> > >>> However, I think I would like to see this structured differently. We > >>> basically have 2 ways of matching: the existing pre-3.14 way and the > >>> desired match on best compatible only. All new bindings should match > >>> with the new way and the old way needs to be kept for compatibility. > >>> So lets structure the code that way. Search the match table first for > >>> best compatible with name and type NULL, then search the table the old > >>> way. I realize it appears you are doing this, but it is not clear this > >>> is the intent of the code. I would like to see this written as a patch > >>> with commit 105353145eafb3ea919 reverted first and you add a new match > >>> function to call first and then fallback to the existing function. > >>> > >>> Rob > >>> > >>>> > >>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > >>>> Signed-off-by: Kevin Hao <haokexin@gmail.com> > >>>> --- > >>>> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------ > >>>> 1 file changed, 37 insertions(+), 18 deletions(-) > >>>> > >>>> diff --git a/drivers/of/base.c b/drivers/of/base.c > >>>> index ff85450d5683..9d655df458bd 100644 > >>>> --- a/drivers/of/base.c > >>>> +++ b/drivers/of/base.c > >>>> @@ -730,32 +730,45 @@ out: > >>>> } > >>>> EXPORT_SYMBOL(of_find_node_with_property); > >>>> > >>>> +static int of_match_type_or_name(const struct device_node *node, > >>>> + const struct of_device_id *m) > >>>> +{ > >>>> + 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); > >>>> + > >>>> + return match; > >>>> +} > >>>> + > >>>> 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; > >>>> + const struct of_device_id *m; > >>>> + int match; > >>>> > >>>> if (!matches) > >>>> return NULL; > >>>> > >>>> cp = __of_get_property(node, "compatible", &cplen); > >>>> - do { > >>>> - const struct of_device_id *m = matches; > >>>> + while (cp && (cplen > 0)) { > >>>> + 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, > >>>> + if (!m->compatible[0]) { > >>>> + m++; > >>>> + continue; > >>>> + } > >>>> + > >>>> + match = of_match_type_or_name(node, m); > >>>> + match &= cp && !of_compat_cmp(m->compatible, cp, > >>>> strlen(m->compatible)); > >>>> if (match) > >>>> return m; > >>>> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches, > >>>> } > >>>> > >>>> /* Get node's next compatible string */ > >>>> - if (cp) { > >>>> - l = strlen(cp) + 1; > >>>> - cp += l; > >>>> - cplen -= l; > >>>> - } > >>>> - } while (cp && (cplen > 0)); > >>>> + l = strlen(cp) + 1; > >>>> + cp += l; > >>>> + cplen -= l; > >>>> + } > >>>> + > >>>> + m = matches; > >>>> + /* Check against matches without compatible string */ > >>>> + while (m->name[0] || m->type[0] || m->compatible[0]) { > >>>> + if (!m->compatible[0] && of_match_type_or_name(node, m)) > >>>> + return m; > >>>> + m++; > >>>> + } > >>>> > >>>> return NULL; > >>>> } > >>>> -- > >>>> 1.8.5.3 > >>>> > >>>> -- > >>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in > >>>> the body of a message to majordomo@vger.kernel.org > >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > >>> _______________________________________________ > >>> Linuxppc-dev mailing list > >>> Linuxppc-dev@lists.ozlabs.org > >>> https://lists.ozlabs.org/listinfo/linuxppc-dev > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-19 22:40 ` Grant Likely @ 2014-02-19 22:44 ` Paul Gortmaker 0 siblings, 0 replies; 12+ messages in thread From: Paul Gortmaker @ 2014-02-19 22:44 UTC (permalink / raw) To: Grant Likely, Rob Herring Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann, Chris Proctor, Stephen N Chivers, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth On 14-02-19 05:40 PM, Grant Likely wrote: > On Wed, 19 Feb 2014 16:23:02 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote: >> On 14-02-19 03:41 PM, Grant Likely wrote: >>> On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote: >>>> On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>> On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: >>>>>> When the device node do have a compatible property, we definitely >>>>>> prefer the compatible match besides the type and name. Only if >>>>>> there is no such a match, we then consider the candidate which >>>>>> doesn't have compatible entry but do match the type or name with >>>>>> the device node. >>>>>> >>>>>> This is based on a patch from Sebastian Hesselbarth. >>>>>> http://patchwork.ozlabs.org/patch/319434/ >>>>>> >>>>>> I did some code refactoring and also fixed a bug in the original patch. >>>>> >>>>> I'm inclined to just revert this once again and avoid possibly >>>>> breaking yet another platform. >>>> >>>> Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot >>>> on my sbc8548. It fails with: >>> >>> I think I've got it fixed now with the latest series. Please try the >>> devicetree/merge branch on git://git.secretlab.ca/git/linux >> >> That seems to work. > > Great, thanks for the testing. Can I add a Tested-by line for you? Absolutely; sorry for not thinking to explicitly indicate that. P. -- > > g. > >> >> libphy: Freescale PowerQUICC MII Bus: probed >> libphy: Freescale PowerQUICC MII Bus: probed >> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 >> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd >> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled >> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 >> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 >> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 >> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd >> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled >> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 >> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 >> TCP: cubic registered >> Initializing XFRM netlink socket >> NET: Registered protocol family 17 >> >> ... >> >> root@sbc8548:~# cat /proc/version >> Linux version 3.14.0-rc3-00024-g7119f42057c6 (paul@yow-lpgnfs-02) (gcc version 4.5.2 (GCC) ) #1 Wed Feb 19 16:08:17 EST 2014 >> root@sbc8548:~# >> >> Paul. >> -- >> >>> >>> g. >>> >>>> ----------------------------------------------- >>>> libphy: Freescale PowerQUICC MII Bus: probed >>>> mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520 >>>> PHY address 1312 is too large >>>> libphy: Freescale PowerQUICC MII Bus: probed >>>> libphy: Freescale PowerQUICC MII Bus: probed >>>> mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520 >>>> PHY address 1312 is too large >>>> libphy: Freescale PowerQUICC MII Bus: probed >>>> TCP: cubic registered >>>> Initializing XFRM netlink socket >>>> NET: Registered protocol family 17 >>>> <fail nfs mount> >>>> ----------------------------------------------- >>>> >>>> On a normal boot, we should see this: >>>> ----------------------------------------------- >>>> libphy: Freescale PowerQUICC MII Bus: probed >>>> libphy: Freescale PowerQUICC MII Bus: probed >>>> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 >>>> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd >>>> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled >>>> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 >>>> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 >>>> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 >>>> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd >>>> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled >>>> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 >>>> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 >>>> TCP: cubic registered >>>> Initializing XFRM netlink socket >>>> NET: Registered protocol family 17 >>>> ----------------------------------------------- >>>> >>>> >>>> Git bisect says: >>>> >>>> ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit >>>> commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 >>>> Author: Kevin Hao <haokexin@gmail.com> >>>> Date: Tue Feb 18 15:57:30 2014 +0800 >>>> >>>> of: reimplement the matching method for __of_match_node() >>>> >>>> In the current implementation of __of_match_node(), it will compare >>>> each given match entry against all the 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, we define a following priority order for the match, and >>>> then scan all the entries to find the best match. >>>> 1. specific compatible && type && name >>>> 2. specific compatible && type >>>> 3. specific compatible && name >>>> 4. specific compatible >>>> 5. general compatible && type && name >>>> 6. general compatible && type >>>> 7. general compatible && name >>>> 8. general compatible >>>> 9. type && name >>>> 10. type >>>> 11. name >>>> >>>> This is based on some pseudo-codes provided by Grant Likely. >>>> >>>> Signed-off-by: Kevin Hao <haokexin@gmail.com> >>>> [grant.likely: Changed multiplier to 4 which makes more sense] >>>> Signed-off-by: Grant Likely <grant.likely@linaro.org> >>>> >>>> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0 >>>> 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers >>>> >>>> As a double check, I checked out the head of linux-next, and did a >>>> revert of the above commit, and my sbc8548 can then boot properly. >>>> >>>> Not doing anything fancy ; using the defconfig exactly as-is, and >>>> ensuring the dtb is fresh from linux-next HEAD of today. >>>> >>>> Thanks, >>>> Paul. >>>> -- >>>> >>>>> >>>>> However, I think I would like to see this structured differently. We >>>>> basically have 2 ways of matching: the existing pre-3.14 way and the >>>>> desired match on best compatible only. All new bindings should match >>>>> with the new way and the old way needs to be kept for compatibility. >>>>> So lets structure the code that way. Search the match table first for >>>>> best compatible with name and type NULL, then search the table the old >>>>> way. I realize it appears you are doing this, but it is not clear this >>>>> is the intent of the code. I would like to see this written as a patch >>>>> with commit 105353145eafb3ea919 reverted first and you add a new match >>>>> function to call first and then fallback to the existing function. >>>>> >>>>> Rob >>>>> >>>>>> >>>>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> >>>>>> Signed-off-by: Kevin Hao <haokexin@gmail.com> >>>>>> --- >>>>>> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------ >>>>>> 1 file changed, 37 insertions(+), 18 deletions(-) >>>>>> >>>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c >>>>>> index ff85450d5683..9d655df458bd 100644 >>>>>> --- a/drivers/of/base.c >>>>>> +++ b/drivers/of/base.c >>>>>> @@ -730,32 +730,45 @@ out: >>>>>> } >>>>>> EXPORT_SYMBOL(of_find_node_with_property); >>>>>> >>>>>> +static int of_match_type_or_name(const struct device_node *node, >>>>>> + const struct of_device_id *m) >>>>>> +{ >>>>>> + 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); >>>>>> + >>>>>> + return match; >>>>>> +} >>>>>> + >>>>>> 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; >>>>>> + const struct of_device_id *m; >>>>>> + int match; >>>>>> >>>>>> if (!matches) >>>>>> return NULL; >>>>>> >>>>>> cp = __of_get_property(node, "compatible", &cplen); >>>>>> - do { >>>>>> - const struct of_device_id *m = matches; >>>>>> + while (cp && (cplen > 0)) { >>>>>> + 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, >>>>>> + if (!m->compatible[0]) { >>>>>> + m++; >>>>>> + continue; >>>>>> + } >>>>>> + >>>>>> + match = of_match_type_or_name(node, m); >>>>>> + match &= cp && !of_compat_cmp(m->compatible, cp, >>>>>> strlen(m->compatible)); >>>>>> if (match) >>>>>> return m; >>>>>> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches, >>>>>> } >>>>>> >>>>>> /* Get node's next compatible string */ >>>>>> - if (cp) { >>>>>> - l = strlen(cp) + 1; >>>>>> - cp += l; >>>>>> - cplen -= l; >>>>>> - } >>>>>> - } while (cp && (cplen > 0)); >>>>>> + l = strlen(cp) + 1; >>>>>> + cp += l; >>>>>> + cplen -= l; >>>>>> + } >>>>>> + >>>>>> + m = matches; >>>>>> + /* Check against matches without compatible string */ >>>>>> + while (m->name[0] || m->type[0] || m->compatible[0]) { >>>>>> + if (!m->compatible[0] && of_match_type_or_name(node, m)) >>>>>> + return m; >>>>>> + m++; >>>>>> + } >>>>>> >>>>>> return NULL; >>>>>> } >>>>>> -- >>>>>> 1.8.5.3 >>>>>> >>>>>> -- >>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in >>>>>> the body of a message to majordomo@vger.kernel.org >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> _______________________________________________ >>>>> Linuxppc-dev mailing list >>>>> Linuxppc-dev@lists.ozlabs.org >>>>> https://lists.ozlabs.org/listinfo/linuxppc-dev >>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] of: give priority to the compatible match in __of_match_node() 2014-02-19 20:41 ` Grant Likely 2014-02-19 21:23 ` Paul Gortmaker @ 2014-02-20 2:05 ` Stephen N Chivers 1 sibling, 0 replies; 12+ messages in thread From: Stephen N Chivers @ 2014-02-20 2:05 UTC (permalink / raw) To: Grant Likely Cc: Chris Proctor, Kevin Hao, Arnd Bergmann, devicetree@vger.kernel.org, Stephen N Chivers, Paul Gortmaker, Rob Herring, Rob Herring, Scott Wood, linuxppc-dev, Sebastian Hesselbarth Grant Likely <glikely@secretlab.ca> wrote on 02/20/2014 07:41:34 AM: > From: Grant Likely <grant.likely@linaro.org> > To: Paul Gortmaker <paul.gortmaker@windriver.com>, Rob Herring > <robherring2@gmail.com> > Cc: Kevin Hao <haokexin@gmail.com>, "devicetree@vger.kernel.org" > <devicetree@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>, Chris > Proctor <cproctor@csc.com.au>, Stephen N Chivers > <schivers@csc.com.au>, Rob Herring <robh+dt@kernel.org>, Scott Wood > <scottwood@freescale.com>, linuxppc-dev <linuxppc- > dev@lists.ozlabs.org>, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Date: 02/20/2014 07:41 AM > Subject: Re: [PATCH] of: give priority to the compatible match in > __of_match_node() > Sent by: Grant Likely <glikely@secretlab.ca> > > On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker > <paul.gortmaker@windriver.com> wrote: > > On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote: > > > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote: > > >> When the device node do have a compatible property, we definitely > > >> prefer the compatible match besides the type and name. Only if > > >> there is no such a match, we then consider the candidate which > > >> doesn't have compatible entry but do match the type or name with > > >> the device node. > > >> > > >> This is based on a patch from Sebastian Hesselbarth. > > >> http://patchwork.ozlabs.org/patch/319434/ > > >> > > >> I did some code refactoring and also fixed a bug in the original patch. > > > > > > I'm inclined to just revert this once again and avoid possibly > > > breaking yet another platform. > > > > Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot > > on my sbc8548. It fails with: > > I think I've got it fixed now with the latest series. Please try the > devicetree/merge branch on git://git.secretlab.ca/git/linux I have tested this with the following platforms: MVME5100, MVME4100, SAM440EP and MPC8349MITXGP. All boot and reach the login state on the serial console. The MVME4100 is a MPC8548 platform like the SBC8548 and suffered from the same PHY address is too large problem when used with todays linux-next. Tested-by: Stephen Chivers <schivers@csc.com> > > g. > > > ----------------------------------------------- > > libphy: Freescale PowerQUICC MII Bus: probed > > mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520 > > PHY address 1312 is too large > > libphy: Freescale PowerQUICC MII Bus: probed > > libphy: Freescale PowerQUICC MII Bus: probed > > mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520 > > PHY address 1312 is too large > > libphy: Freescale PowerQUICC MII Bus: probed > > TCP: cubic registered > > Initializing XFRM netlink socket > > NET: Registered protocol family 17 > > <fail nfs mount> > > ----------------------------------------------- > > > > On a normal boot, we should see this: > > ----------------------------------------------- > > libphy: Freescale PowerQUICC MII Bus: probed > > libphy: Freescale PowerQUICC MII Bus: probed > > fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4 > > fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd > > fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled > > fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256 > > fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256 > > fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4 > > fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd > > fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled > > fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256 > > fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256 > > TCP: cubic registered > > Initializing XFRM netlink socket > > NET: Registered protocol family 17 > > ----------------------------------------------- > > > > > > Git bisect says: > > > > ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit > > commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 > > Author: Kevin Hao <haokexin@gmail.com> > > Date: Tue Feb 18 15:57:30 2014 +0800 > > > > of: reimplement the matching method for __of_match_node() > > > > In the current implementation of __of_match_node(), it will compare > > each given match entry against all the 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, we define a following priority order for the match, and > > then scan all the entries to find the best match. > > 1. specific compatible && type && name > > 2. specific compatible && type > > 3. specific compatible && name > > 4. specific compatible > > 5. general compatible && type && name > > 6. general compatible && type > > 7. general compatible && name > > 8. general compatible > > 9. type && name > > 10. type > > 11. name > > > > This is based on some pseudo-codes provided by Grant Likely. > > > > Signed-off-by: Kevin Hao <haokexin@gmail.com> > > [grant.likely: Changed multiplier to 4 which makes more sense] > > Signed-off-by: Grant Likely <grant.likely@linaro.org> > > > > :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0 > > 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers > > > > As a double check, I checked out the head of linux-next, and did a > > revert of the above commit, and my sbc8548 can then boot properly. > > > > Not doing anything fancy ; using the defconfig exactly as-is, and > > ensuring the dtb is fresh from linux-next HEAD of today. > > > > Thanks, > > Paul. > > -- > > > > > > > > However, I think I would like to see this structured differently. We > > > basically have 2 ways of matching: the existing pre-3.14 way and the > > > desired match on best compatible only. All new bindings should match > > > with the new way and the old way needs to be kept for compatibility. > > > So lets structure the code that way. Search the match table first for > > > best compatible with name and type NULL, then search the table the old > > > way. I realize it appears you are doing this, but it is not clear this > > > is the intent of the code. I would like to see this written as a patch > > > with commit 105353145eafb3ea919 reverted first and you add a new match > > > function to call first and then fallback to the existing function. > > > > > > Rob > > > > > >> > > >> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > > >> Signed-off-by: Kevin Hao <haokexin@gmail.com> > > >> --- > > >> drivers/of/base.c | 55 ++++++++++++++++++++++++++++++++++++ > +------------------ > > >> 1 file changed, 37 insertions(+), 18 deletions(-) > > >> > > >> diff --git a/drivers/of/base.c b/drivers/of/base.c > > >> index ff85450d5683..9d655df458bd 100644 > > >> --- a/drivers/of/base.c > > >> +++ b/drivers/of/base.c > > >> @@ -730,32 +730,45 @@ out: > > >> } > > >> EXPORT_SYMBOL(of_find_node_with_property); > > >> > > >> +static int of_match_type_or_name(const struct device_node *node, > > >> + const struct of_device_id *m) > > >> +{ > > >> + 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); > > >> + > > >> + return match; > > >> +} > > >> + > > >> 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; > > >> + const struct of_device_id *m; > > >> + int match; > > >> > > >> if (!matches) > > >> return NULL; > > >> > > >> cp = __of_get_property(node, "compatible", &cplen); > > >> - do { > > >> - const struct of_device_id *m = matches; > > >> + while (cp && (cplen > 0)) { > > >> + 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, > > >> + if (!m->compatible[0]) { > > >> + m++; > > >> + continue; > > >> + } > > >> + > > >> + match = of_match_type_or_name(node, m); > > >> + match &= cp && !of_compat_cmp(m->compatible, cp, > > >> strlen > (m->compatible)); > > >> if (match) > > >> return m; > > >> @@ -763,12 +776,18 @@ const struct of_device_id > *__of_match_node(const struct of_device_id *matches, > > >> } > > >> > > >> /* Get node's next compatible string */ > > >> - if (cp) { > > >> - l = strlen(cp) + 1; > > >> - cp += l; > > >> - cplen -= l; > > >> - } > > >> - } while (cp && (cplen > 0)); > > >> + l = strlen(cp) + 1; > > >> + cp += l; > > >> + cplen -= l; > > >> + } > > >> + > > >> + m = matches; > > >> + /* Check against matches without compatible string */ > > >> + while (m->name[0] || m->type[0] || m->compatible[0]) { > > >> + if (!m->compatible[0] && of_match_type_or_name(node, m)) > > >> + return m; > > >> + m++; > > >> + } > > >> > > >> return NULL; > > >> } > > >> -- > > >> 1.8.5.3 > > >> > > >> -- > > >> To unsubscribe from this list: send the line "unsubscribe devicetree" in > > >> the body of a message to majordomo@vger.kernel.org > > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > _______________________________________________ > > > Linuxppc-dev mailing list > > > Linuxppc-dev@lists.ozlabs.org > > > https://lists.ozlabs.org/listinfo/linuxppc-dev > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-02-20 2:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-12 11:38 [PATCH] of: give priority to the compatible match in __of_match_node() Kevin Hao
[not found] ` < CAL_JsqKMi2H=vwoxrOt8QRA2xJeiLqBKKfLtt4QRCRoFk6JUHg@mail.gmail.com>
[not found] ` < CAP=VYLr7hqnzW2HLS4GCMFeghCgPmrQZHYst+AUfZc_WNT-Wog@mail.gmail.com>
[not found] ` < 20140219204134.E4A2DC4088D@trevor.secretlab.ca>
2014-02-12 20:42 ` Stephen N Chivers
2014-02-13 19:01 ` Rob Herring
2014-02-14 1:20 ` Kevin Hao
2014-02-17 18:06 ` Grant Likely
2014-02-19 18:25 ` Paul Gortmaker
2014-02-19 18:57 ` Scott Wood
2014-02-19 20:41 ` Grant Likely
2014-02-19 21:23 ` Paul Gortmaker
2014-02-19 22:40 ` Grant Likely
2014-02-19 22:44 ` Paul Gortmaker
2014-02-20 2:05 ` Stephen N Chivers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).