If we set the uart compatible in the dts file like this:
------------------------------------------------------
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
------------------------------------------------------
and we set the uart compatible in the uart driver like this:
------------------------------------------------------
{ .compatible = "fsl,imx1-uart", ... },
{ .compatible = "fsl,imx21-uart", ... },
{ .compatible = "fsl,imx6q-uart", ... },
{ /* sentinel */ }
------------------------------------------------------
the current code will match the "fsl,imx21-uart" in the end.
Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".
This patch rewrites the match code, and make it to check the compatible
in the order set by the DTS file.
Signed-off-by: Huang Shijie <[email protected]>
---
drivers/of/base.c | 29 +++++++++++++++++++++++++----
1 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0d61fc5..b13846b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -622,10 +622,14 @@ static
const struct of_device_id *__of_match_node(const struct of_device_id *matches,
const struct device_node *node)
{
+ struct of_device_id *old = (struct of_device_id *)matches;
+ const char *cp;
+ int cplen, l;
+
if (!matches)
return NULL;
- while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
+ while (matches->name[0] || matches->type[0]) {
int match = 1;
if (matches->name[0])
match &= node->name
@@ -633,13 +637,30 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
if (matches->type[0])
match &= node->type
&& !strcmp(matches->type, node->type);
- if (matches->compatible[0])
- match &= __of_device_is_compatible(node,
- matches->compatible);
if (match)
return matches;
matches++;
}
+
+ /* Check the compatible in the order set by the DTS file. */
+ cp = __of_get_property(node, "compatible", &cplen);
+ if (!cp)
+ return NULL;
+
+ while (cplen > 0) {
+ matches = old;
+
+ while (matches->compatible[0]) {
+ if (!of_compat_cmp(cp, matches->compatible,
+ strlen(matches->compatible)))
+ return matches;
+ matches++;
+ }
+
+ l = strlen(cp) + 1;
+ cp += l;
+ cplen -= l;
+ }
return NULL;
}
--
1.7.1
On Fri, Jul 05, 2013 at 04:43:38PM +0800, Huang Shijie wrote:
> If we set the uart compatible in the dts file like this:
> ------------------------------------------------------
> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> ------------------------------------------------------
>
> and we set the uart compatible in the uart driver like this:
> ------------------------------------------------------
> { .compatible = "fsl,imx1-uart", ... },
> { .compatible = "fsl,imx21-uart", ... },
> { .compatible = "fsl,imx6q-uart", ... },
> { /* sentinel */ }
> ------------------------------------------------------
>
> the current code will match the "fsl,imx21-uart" in the end.
>
> Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".
>
> This patch rewrites the match code, and make it to check the compatible
> in the order set by the DTS file.
Why don't you set the matching order in the driver the way you want it
to be, i.e.:
{ .compatible = "fsl,imx6q-uart", ... },
{ .compatible = "fsl,imx21-uart", ... },
{ .compatible = "fsl,imx1-uart", ... },
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
于 2013年07月09日 15:05, Sascha Hauer 写道:
> Why don't you set the matching order in the driver the way you want it
> to be, i.e.:
>
> { .compatible = "fsl,imx6q-uart", ... },
> { .compatible = "fsl,imx21-uart", ... },
> { .compatible = "fsl,imx1-uart", ... },
>
yes. i can set it like this.
but this method looks like a ugly workaround.
thanks
Huang Shijie
On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> >Why don't you set the matching order in the driver the way you want it
> >to be, i.e.:
> >
> > { .compatible = "fsl,imx6q-uart", ... },
> > { .compatible = "fsl,imx21-uart", ... },
> > { .compatible = "fsl,imx1-uart", ... },
> >
> yes. i can set it like this.
>
> but this method looks like a ugly workaround.
If a driver has different ways of supporting a single device, then
putting the preferred or most feature rich on top doesn't look very ugly
to me.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
于 2013年07月09日 15:51, Sascha Hauer 写道:
> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>>> Why don't you set the matching order in the driver the way you want it
>>> to be, i.e.:
>>>
>>> { .compatible = "fsl,imx6q-uart", ... },
>>> { .compatible = "fsl,imx21-uart", ... },
>>> { .compatible = "fsl,imx1-uart", ... },
>>>
>> yes. i can set it like this.
>>
>> but this method looks like a ugly workaround.
> If a driver has different ways of supporting a single device, then
> putting the preferred or most feature rich on top doesn't look very ugly
> to me.
this method makes it much _coupled_ between the driver and the dts file.
IMHO, it's an unnecessary _burden_ to the driver programmer:
he should puts the most feature compatible on the top.
it's much graceful if we let the driver programmer be transparent about
this.
thanks
Huang Shijie
On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <[email protected]> wrote:
> ?? 2013??07??09?? 15:51, Sascha Hauer д??:
>
>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>
>>> ?? 2013??07??09?? 15:05, Sascha Hauer д??:
>>>>
>>>> Why don't you set the matching order in the driver the way you want it
>>>> to be, i.e.:
>>>>
>>>> { .compatible = "fsl,imx6q-uart", ... },
>>>> { .compatible = "fsl,imx21-uart", ... },
>>>> { .compatible = "fsl,imx1-uart", ... },
>>>>
>>> yes. i can set it like this.
>>>
>>> but this method looks like a ugly workaround.
>>
>> If a driver has different ways of supporting a single device, then
>> putting the preferred or most feature rich on top doesn't look very ugly
>> to me.
>
> this method makes it much _coupled_ between the driver and the dts file.
>
> IMHO, it's an unnecessary _burden_ to the driver programmer:
> he should puts the most feature compatible on the top.
>
> it's much graceful if we let the driver programmer be transparent about
> this.
The dts requires compatible strings to be most specific to least
specific. There is no reason that driver match tables should not be
the same and that is the assumption. Matching is not just based on
compatible properties and your patch does not handle the other cases.
Rob
>
>
> thanks
> Huang Shijie
>
>
>
>
>
>
>
> _______________________________________________
> devicetree-discuss mailing list
> [email protected]
> https://lists.ozlabs.org/listinfo/devicetree-discuss
On 07/09/2013 01:05 AM, Sascha Hauer wrote:
> On Fri, Jul 05, 2013 at 04:43:38PM +0800, Huang Shijie wrote:
>> If we set the uart compatible in the dts file like this:
>> ------------------------------------------------------
>> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>> ------------------------------------------------------
>>
>> and we set the uart compatible in the uart driver like this:
>> ------------------------------------------------------
>> { .compatible = "fsl,imx1-uart", ... },
>> { .compatible = "fsl,imx21-uart", ... },
>> { .compatible = "fsl,imx6q-uart", ... },
>> { /* sentinel */ }
>> ------------------------------------------------------
>>
>> the current code will match the "fsl,imx21-uart" in the end.
>>
>> Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".
>>
>> This patch rewrites the match code, and make it to check the compatible
>> in the order set by the DTS file.
>
> Why don't you set the matching order in the driver the way you want it
> to be, i.e.:
>
> { .compatible = "fsl,imx6q-uart", ... },
> { .compatible = "fsl,imx21-uart", ... },
> { .compatible = "fsl,imx1-uart", ... },
DT semantics are that earlier entries in the compatible property are
supposed to be matched first, irrespective of how the driver is constructed.
On 07/09/2013 06:03 AM, Rob Herring wrote:
> On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <[email protected]> wrote:
>> ?? 2013??07??09?? 15:51, Sascha Hauer д??:
>>
>>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>>
>>>> ?? 2013??07??09?? 15:05, Sascha Hauer д??:
>>>>>
>>>>> Why don't you set the matching order in the driver the way you want it
>>>>> to be, i.e.:
>>>>>
>>>>> { .compatible = "fsl,imx6q-uart", ... },
>>>>> { .compatible = "fsl,imx21-uart", ... },
>>>>> { .compatible = "fsl,imx1-uart", ... },
>>>>>
>>>> yes. i can set it like this.
>>>>
>>>> but this method looks like a ugly workaround.
>>>
>>> If a driver has different ways of supporting a single device, then
>>> putting the preferred or most feature rich on top doesn't look very ugly
>>> to me.
>>
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>> he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
>
> The dts requires compatible strings to be most specific to least
> specific. There is no reason that driver match tables should not be
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.
Well, that may be true, but the only way to guarantee that the DT
compatible property is matched correctly is to match it in the order
it's written. Forcing driver writers to write the of_match table in a
particular order is quite a hack, and doesn't guarantee the correct
match order in all cases, only typical cases.
?? 2013??07??09?? 20:03, Rob Herring д??:
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.
Could you show a example of "the other cases"?
After this patch,
[1] the matching will first check the @match->type and @match->name,
if we can match the @match->type or @match->name, return the match
immediately.
[2] If [1] fails, we will check the compatible properties.
thanks
Huang Shijie
On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <[email protected]> wrote:
> 于 2013年07月09日 15:51, Sascha Hauer 写道:
> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> >>> Why don't you set the matching order in the driver the way you want it
> >>> to be, i.e.:
> >>>
> >>> { .compatible = "fsl,imx6q-uart", ... },
> >>> { .compatible = "fsl,imx21-uart", ... },
> >>> { .compatible = "fsl,imx1-uart", ... },
> >>>
> >> yes. i can set it like this.
> >>
> >> but this method looks like a ugly workaround.
> > If a driver has different ways of supporting a single device, then
> > putting the preferred or most feature rich on top doesn't look very ugly
> > to me.
> this method makes it much _coupled_ between the driver and the dts file.
>
> IMHO, it's an unnecessary _burden_ to the driver programmer:
> he should puts the most feature compatible on the top.
>
> it's much graceful if we let the driver programmer be transparent about
> this.
Absolutely true. Applied, thanks.
g.
On Sat, Jul 20, 2013 at 06:44:39AM +0100, Grant Likely wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <[email protected]> wrote:
> > 于 2013年07月09日 15:51, Sascha Hauer 写道:
> > > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> > >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> > >>> Why don't you set the matching order in the driver the way you want it
> > >>> to be, i.e.:
> > >>>
> > >>> { .compatible = "fsl,imx6q-uart", ... },
> > >>> { .compatible = "fsl,imx21-uart", ... },
> > >>> { .compatible = "fsl,imx1-uart", ... },
> > >>>
> > >> yes. i can set it like this.
> > >>
> > >> but this method looks like a ugly workaround.
> > > If a driver has different ways of supporting a single device, then
> > > putting the preferred or most feature rich on top doesn't look very ugly
> > > to me.
> > this method makes it much _coupled_ between the driver and the dts file.
> >
> > IMHO, it's an unnecessary _burden_ to the driver programmer:
> > he should puts the most feature compatible on the top.
> >
> > it's much graceful if we let the driver programmer be transparent about
> > this.
>
> Absolutely true. Applied, thanks.
I don't think this will work. The patch looks very similar to one that I
posted about a year ago (commit 107a84e "of: match by compatible
property first"). The problem it was trying to address was exactly the
same. However the patch caused regressions on SPARC and had to be
reverted in commit bc51b0c "Revert "of: match by compatible property
first"".
The revert commit quotes Rob having a fix for this, and I remember that
I pinged him about it occasionally, but I suspect that those must have
fallen through the cracks.
Grant, before applying this patch we should make sure that it doesn't
cause a regression again, so perhaps asking Meelis Roos (mentioned in
the revert commit) to test would be good. I have a strong feeling that
this patch will break in the same way than mine did a year ago, because
it looks too similar and doesn't handle the compatible & name
combination that Rob mentioned.
Thierry
On Sat, Jul 20, 2013 at 12:44 AM, Grant Likely <[email protected]>
wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <[email protected]> wrote:
>> 于 2013年07月09日 15:51, Sascha Hauer 写道:
>> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>> >>> Why don't you set the matching order in the driver the way you want it
>> >>> to be, i.e.:
>> >>>
>> >>> { .compatible = "fsl,imx6q-uart", ... },
>> >>> { .compatible = "fsl,imx21-uart", ... },
>> >>> { .compatible = "fsl,imx1-uart", ... },
>> >>>
>> >> yes. i can set it like this.
>> >>
>> >> but this method looks like a ugly workaround.
>> > If a driver has different ways of supporting a single device, then
>> > putting the preferred or most feature rich on top doesn't look very ugly
>> > to me.
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>> he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
>
> Absolutely true. Applied, thanks.
We can debate whether the driver order matters or not, but either way
I'm not sure this patch does the right thing. It doesn't really look
correct to me, but I haven't dug into it.
We've already tried to fix matching and reverted the fix once before
(commit below). So this patch needs careful review and thought about
cases where the name and/or type is used to match.
commit bc51b0c22cebf5c311a6f1895fcca9f78efd0478
Author: Linus Torvalds <[email protected]>
Date: Tue Jul 10 12:49:32 2012 -0700
Revert "of: match by compatible property first"
This reverts commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6.
Meelis Roos reports a regression since 3.5-rc5 that stops Sun Fire V100
and Sun Netra X1 sparc64 machines from booting, hanging after enabling
serial console. He bisected it to commit 107a84e61cdd.
Rob Herring explains:
"The problem is match combinations of compatible plus name and/or type
fail to match correctly. I have a fix for this, but given how late it
is for 3.5 I think it is best to revert this for now. There could be
other cases that rely on the current although wrong behavior. I will
post an updated version for 3.6."
Bisected-and-reported-by: Meelis Roos <[email protected]>
Requested-by: Rob Herring <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Grant Likely <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
Rob
On Sun, Jul 21, 2013 at 9:45 PM, Rob Herring <[email protected]> wrote:
> We can debate whether the driver order matters or not, but either way
> I'm not sure this patch does the right thing. It doesn't really look
> correct to me, but I haven't dug into it.
>
> We've already tried to fix matching and reverted the fix once before
> (commit below). So this patch needs careful review and thought about
> cases where the name and/or type is used to match.
The rules have always been well established. This patch /shouldn't/
cause any regression that cannot be narrowed down to a fixable driver
bug.
A harder problem to solve however is dealing with the case when
multiple drivers will potentially bind against the same device. Making
that work requires getting all of the match tables into the kernel
early so that the kernel can select the correct driver of many. Can't
be that big a problem though since we've never actually tried to solve
it. :-)
[...]
> Rob Herring explains:
> "The problem is match combinations of compatible plus name and/or type
> fail to match correctly. I have a fix for this, but given how late it
> is for 3.5 I think it is best to revert this for now. There could be
> other cases that rely on the current although wrong behavior. I will
> post an updated version for 3.6."
I don't believe the fix ever got posted. Do you still have it? Or can
you describe what needs to be done?
g.