[PATCH] of: give priority to the compatible match in __of_match_node()

Stephen N Chivers schivers at csc.com.au
Thu Feb 20 13:05:56 EST 2014


Grant Likely <glikely at secretlab.ca> wrote on 02/20/2014 07:41:34 AM:

> From: Grant Likely <grant.likely at linaro.org>
> To: Paul Gortmaker <paul.gortmaker at windriver.com>, Rob Herring 
> <robherring2 at gmail.com>
> Cc: Kevin Hao <haokexin at gmail.com>, "devicetree at vger.kernel.org" 
> <devicetree at vger.kernel.org>, Arnd Bergmann <arnd at arndb.de>, Chris 
> Proctor <cproctor at csc.com.au>, Stephen N Chivers 
> <schivers at csc.com.au>, Rob Herring <robh+dt at kernel.org>, Scott Wood 
> <scottwood at freescale.com>, linuxppc-dev <linuxppc-
> dev at lists.ozlabs.org>, Sebastian Hesselbarth 
<sebastian.hesselbarth at 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 at secretlab.ca>
> 
> On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker 
> <paul.gortmaker at windriver.com> wrote:
> > On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2 at gmail.com> 
wrote:
> > > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin at 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 at csc.com>
> 
> g.
> 
> > -----------------------------------------------
> > libphy: Freescale PowerQUICC MII Bus: probed
> > mdio_bus ethernet at e002400: /soc8548 at e0000000/ethernet at 24000/mdio at 520
> > PHY address 1312 is too large
> > libphy: Freescale PowerQUICC MII Bus: probed
> > libphy: Freescale PowerQUICC MII Bus: probed
> > mdio_bus ethernet at e002500: /soc8548 at e0000000/ethernet at 25000/mdio at 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 at 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 at gmail.com>
> >     [grant.likely: Changed multiplier to 4 which makes more sense]
> >     Signed-off-by: Grant Likely <grant.likely at 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 at gmail.com>
> > >> Signed-off-by: Kevin Hao <haokexin at 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 at vger.kernel.org
> > >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > _______________________________________________
> > > Linuxppc-dev mailing list
> > > Linuxppc-dev at lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> 



More information about the Linuxppc-dev mailing list