| Message ID | 1373013818-11365-1-git-send-email-b32955@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
? 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
? 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 <b32955@freescale.com> 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 > devicetree-discuss@lists.ozlabs.org > 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 <b32955@freescale.com> 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 <b32955@freescale.com> 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 <b32955@freescale.com> 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 <grant.likely@linaro.org> wrote: > On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> 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 <torvalds@linux-foundation.org> 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 <mroos@linux.ee> Requested-by: Rob Herring <rob.herring@calxeda.com> Cc: Thierry Reding <thierry.reding@avionic-design.de> Cc: Grant Likely <grant.likely@secretlab.ca> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Rob
On Sun, Jul 21, 2013 at 9:45 PM, Rob Herring <robherring2@gmail.com> 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.
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; }
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 <b32955@freescale.com> --- drivers/of/base.c | 29 +++++++++++++++++++++++++---- 1 files changed, 25 insertions(+), 4 deletions(-)