Grant Likely [off-list ref] wrote on 02/20/2014 07:41:34 AM:

From: Grant Likely <redacted>
To: Paul Gortmaker <redacted>, Rob Herring 
[off-list ref]
Cc: Kevin Hao <redacted>, "devicetree@vger.kernel.org" 
[off-list ref], Arnd Bergmann [off-list ref], Chris 
Proctor [off-list ref], Stephen N Chivers 
[off-list ref], Rob Herring [off-list ref], Scott Wood 
[off-list ref], linuxppc-dev <linuxppc-
dev@lists.ozlabs.org>, Sebastian Hesselbarth 

[off-list ref]

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 [off-list ref]

On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker 
[off-list ref] wrote:

quoted

On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring [off-list ref] 

wrote:

quoted

quoted

On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao [off-list ref] 

wrote:

quoted

quoted

quoted

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.

quoted

quoted

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

quoted

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 <redacted>

g.

quoted

-----------------------------------------------
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 [off-list ref]
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

quoted

    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

quoted

    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 [off-list ref]
    [grant.likely: Changed multiplier to 4 which makes more sense]
    Signed-off-by: Grant Likely [off-list ref]

: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.
--

quoted

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

quoted

quoted

best compatible with name and type NULL, then search the table the 

old

quoted

quoted

way. I realize it appears you are doing this, but it is not clear 

this

quoted

quoted

is the intent of the code. I would like to see this written as a 

patch

quoted

quoted

with commit 105353145eafb3ea919 reverted first and you add a new 

match

quoted

quoted

function to call first and then fallback to the existing function.

Rob

quoted

Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Signed-off-by: Kevin Hao <redacted>
---
 drivers/of/base.c | 55 ++++++++++++++++++++++++++++++++++++

+------------------

quoted

quoted

quoted

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

quoted

quoted

quoted

+
+       if (m->type[0])
+               match &= node->type && !strcmp(m->type, 

node->type);

quoted

quoted

quoted

+
+       return match;
+}
+
 static
 const struct of_device_id *__of_match_node(const struct 

of_device_id *matches,

quoted

quoted

quoted

                                           const struct 

device_node *node)

quoted

quoted

quoted

 {
        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 */

quoted

quoted

quoted

                while (m->name[0] || m->type[0] || 

m->compatible[0]) {

quoted

quoted

quoted

-                       int match = 1;
-                       if (m->name[0])
-                               match &= node->name
-                                       && !strcmp(m->name, 

node->name);

quoted

quoted

quoted

-                       if (m->type[0])
-                               match &= node->type
-                                       && !strcmp(m->type, 

node->type);

quoted

quoted

quoted

-                       if (m->compatible[0])
-                               match &= cp
-                                       && !of_compat_cmp

(m->compatible, cp,

quoted

quoted

quoted

+                       if (!m->compatible[0]) {
+                               m++;
+                               continue;
+                       }
+
+                       match = of_match_type_or_name(node, m);
+                       match &= cp && 

!of_compat_cmp(m->compatible, cp,

quoted

quoted

quoted

                                                        strlen

(m->compatible));

quoted

quoted

quoted

                        if (match)
                                return m;

@@ -763,12 +776,18 @@ const struct of_device_id 

*__of_match_node(const struct of_device_id *matches,

quoted

quoted

quoted

                }

                /* 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))

quoted

quoted

quoted

+                       return m;
+               m++;
+       }

        return NULL;
 }
--
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe 

devicetree" in

quoted

quoted

quoted

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