Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Kumar Gala]

On Feb 11, 2014, at 2:57 PM, Stephen N Chivers <schivers-znpAAEhiOVUQrrorzV6ljw@public.gmane.org> wrote:

> I have been trial booting a 3.14-rc2 kernel for a 85xx platform 
> (dtbImage).
> 
> After mounting the root filesystem there are no messages from the init 
> scripts
> and the serial console is not available for login.
> 
> In the kernel log messages there is:
> 
> of_serial f1004500.serial: Unknown serial port found, ignored.
> 
> The serial nodes in boards dts file are specified as:
> 
>        serial0: serial@4500 {
>                        cell-index = <0>;
>                        device_type = "serial";
>                        compatible = "fsl,ns16550", "ns16550";
>                        reg = <0x4500 0x100>;
>                        clock-frequency = <0>;
>                        interrupts = <0x2a 0x2>;
>                        interrupt-parent = <&mpic>;
>                };
> 
> Reversing the order of the compatible:
> 
>        compatible = "ns16550", "fsl,ns16550";
> 
> restores the serial console.
> 
> Linux-3.13 does not have this behaviour.
> 
> There are 49 dts files in Linux-3.14-rc2 that have the fsl,ns16550 
> compatible first.

Hmm,

Wondering if this caused the issue:

commit 105353145eafb3ea919f5cdeb652a9d8f270228e
Author: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date:   Tue Dec 3 14:52:00 2013 +0100

    OF: base: match each node compatible against all given matches first


- k
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Sebastian Hesselbarth]

On 02/11/2014 11:33 PM, Kumar Gala wrote:
>
> On Feb 11, 2014, at 2:57 PM, Stephen N Chivers <schivers-znpAAEhiOVUQrrorzV6ljw@public.gmane.org> wrote:
>
>> I have been trial booting a 3.14-rc2 kernel for a 85xx platform
>> (dtbImage).
>>
>> After mounting the root filesystem there are no messages from the init
>> scripts
>> and the serial console is not available for login.
>>
>> In the kernel log messages there is:
>>
>> of_serial f1004500.serial: Unknown serial port found, ignored.
>>
>> The serial nodes in boards dts file are specified as:
>>
>>         serial0: serial@4500 {
>>                         cell-index = <0>;
>>                         device_type = "serial";
>>                         compatible = "fsl,ns16550", "ns16550";
>>                         reg = <0x4500 0x100>;
>>                         clock-frequency = <0>;
>>                         interrupts = <0x2a 0x2>;
>>                         interrupt-parent = <&mpic>;
>>                 };
>>
>> Reversing the order of the compatible:
>>
>>         compatible = "ns16550", "fsl,ns16550";
>>
>> restores the serial console.
>>
>> Linux-3.13 does not have this behaviour.
>>
>> There are 49 dts files in Linux-3.14-rc2 that have the fsl,ns16550
>> compatible first.
>
> Hmm,
>
> Wondering if this caused the issue:
>
> commit 105353145eafb3ea919f5cdeb652a9d8f270228e
> Author: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date:   Tue Dec 3 14:52:00 2013 +0100
>
>      OF: base: match each node compatible against all given matches first

[adding Arnd on Cc]

Could be. I checked tty/serial/of_serial.c and it does not provide a
compatible for "fsl,ns16550". Does reverting the patch fix the issue
observed?

I don't think the missing compatible is causing it, but of_serial
provides a DT match for .type = "serial" just to fail later on
with the error seen above.

The commit in question reorders of_match_device in a way that match
table order is not relevant anymore. This can cause it to match
.type = "serial" first here.

Rather than touching the commit, I suggest to remove the problematic
.type = "serial" from the match table. It is of no use anyway.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Stephen N Chivers]

Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote on 
02/12/2014 09:51:43 AM:

> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> To: Kumar Gala <galak@kernel.crashing.org>, Stephen N Chivers 
> <schivers@csc.com.au>
> Cc: linuxppc-dev@lists.ozlabs.org, Chris Proctor 
> <cproctor@csc.com.au>, devicetree <devicetree@vger.kernel.org>, Arnd
> Bergmann <arnd@arndb.de>
> Date: 02/12/2014 09:51 AM
> Subject: Re: Linux-3.14-rc2: Order of serial node compatibles in DTS 
files.
> 
> On 02/11/2014 11:33 PM, Kumar Gala wrote:
> >
> > On Feb 11, 2014, at 2:57 PM, Stephen N Chivers <schivers@csc.com.au> 
wrote:
> >
> >> I have been trial booting a 3.14-rc2 kernel for a 85xx platform
> >> (dtbImage).
> >>
> >> After mounting the root filesystem there are no messages from the 
init
> >> scripts
> >> and the serial console is not available for login.
> >>
> >> In the kernel log messages there is:
> >>
> >> of_serial f1004500.serial: Unknown serial port found, ignored.
> >>
> >> The serial nodes in boards dts file are specified as:
> >>
> >>         serial0: serial@4500 {
> >>                         cell-index = <0>;
> >>                         device_type = "serial";
> >>                         compatible = "fsl,ns16550", "ns16550";
> >>                         reg = <0x4500 0x100>;
> >>                         clock-frequency = <0>;
> >>                         interrupts = <0x2a 0x2>;
> >>                         interrupt-parent = <&mpic>;
> >>                 };
> >>
> >> Reversing the order of the compatible:
> >>
> >>         compatible = "ns16550", "fsl,ns16550";
> >>
> >> restores the serial console.
> >>
> >> Linux-3.13 does not have this behaviour.
> >>
> >> There are 49 dts files in Linux-3.14-rc2 that have the fsl,ns16550
> >> compatible first.
> >
> > Hmm,
> >
> > Wondering if this caused the issue:
> >
> > commit 105353145eafb3ea919f5cdeb652a9d8f270228e
> > Author: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > Date:   Tue Dec 3 14:52:00 2013 +0100
> >
> >      OF: base: match each node compatible against all given matches 
first
> 
> [adding Arnd on Cc]
> 
> Could be. I checked tty/serial/of_serial.c and it does not provide a
> compatible for "fsl,ns16550". Does reverting the patch fix the issue
> observed?
> 
> I don't think the missing compatible is causing it, but of_serial
> provides a DT match for .type = "serial" just to fail later on
> with the error seen above.
> 
> The commit in question reorders of_match_device in a way that match
> table order is not relevant anymore. This can cause it to match
> .type = "serial" first here.
> 
> Rather than touching the commit, I suggest to remove the problematic
> .type = "serial" from the match table. It is of no use anyway.
Deleting the "serial" line from the match table fixes the problem.
I tested it for both orderings of compatible.
> 
> Sebastian

Thanks,
Stephen Chivers,
CSC Australia Pty. Ltd.

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Scott Wood]

On Tue, 2014-02-11 at 23:51 +0100, Sebastian Hesselbarth wrote:
> On 02/11/2014 11:33 PM, Kumar Gala wrote:
> > Hmm,
> >
> > Wondering if this caused the issue:
> >
> > commit 105353145eafb3ea919f5cdeb652a9d8f270228e
> > Author: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Date:   Tue Dec 3 14:52:00 2013 +0100
> >
> >      OF: base: match each node compatible against all given matches first
> 
> [adding Arnd on Cc]
> 
> Could be. I checked tty/serial/of_serial.c and it does not provide a
> compatible for "fsl,ns16550". Does reverting the patch fix the issue
> observed?
> 
> I don't think the missing compatible is causing it, but of_serial
> provides a DT match for .type = "serial" just to fail later on
> with the error seen above.
> 
> The commit in question reorders of_match_device in a way that match
> table order is not relevant anymore. This can cause it to match
> .type = "serial" first here.
> 
> Rather than touching the commit, I suggest to remove the problematic
> .type = "serial" from the match table. It is of no use anyway.

Regardless of whether .type = "serial" gets removed, it seems wrong for
of_match_node() to accept a .type-only match (or .name, or anything else
that doesn't involve .compatible) before it accepts a compatible match
other than the first in the compatible property.

-Scott


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Sebastian Hesselbarth]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

On 02/12/2014 12:38 AM, Stephen N Chivers wrote:
> Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on
>> On 02/11/2014 11:33 PM, Kumar Gala wrote:
>>> On Feb 11, 2014, at 2:57 PM, Stephen N Chivers <schivers-znpAAEhiOVUQrrorzV6ljw@public.gmane.org> wrote:
>>>> I have been trial booting a 3.14-rc2 kernel for a 85xx platform
>>>> (dtbImage).
[...]
>>>>
>>>> of_serial f1004500.serial: Unknown serial port found, ignored.
>>>>
>>>> The serial nodes in boards dts file are specified as:
>>>>
>>>>          serial0: serial@4500 {
>>>>                          cell-index = <0>;
>>>>                          device_type = "serial";
>>>>                          compatible = "fsl,ns16550", "ns16550";
>>>>                          reg = <0x4500 0x100>;
>>>>                          clock-frequency = <0>;
>>>>                          interrupts = <0x2a 0x2>;
>>>>                          interrupt-parent = <&mpic>;
>>>>                  };
>>>
>>> Wondering if this caused the issue:
>>>
>>> commit 105353145eafb3ea919f5cdeb652a9d8f270228e
>>> Author: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Date:   Tue Dec 3 14:52:00 2013 +0100
>>>
>>>       OF: base: match each node compatible against all given matches first
>>
[...]
>>
>> I don't think the missing compatible is causing it, but of_serial
>> provides a DT match for .type = "serial" just to fail later on
>> with the error seen above.
>>
>> The commit in question reorders of_match_device in a way that match
>> table order is not relevant anymore. This can cause it to match
>> .type = "serial" first here.
>>
>> Rather than touching the commit, I suggest to remove the problematic
>> .type = "serial" from the match table. It is of no use anyway.
> Deleting the "serial" line from the match table fixes the problem.
> I tested it for both orderings of compatible.

I revert my statement about removing anything from of_serial.c. Instead
we should try to prefer matches with compatibles over type/name without
compatibles. Something like the patch below (compile tested only)






[-- Attachment #2: of_base_match.patch --]
[-- Type: text/x-patch, Size: 1484 bytes --]

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ff85450d5683..60da53b385ff 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -734,6 +734,7 @@ static
 const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 					   const struct device_node *node)
 {
+	const struct of_device_id *m;
 	const char *cp;
 	int cplen, l;
 
@@ -742,15 +743,15 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 
 	cp = __of_get_property(node, "compatible", &cplen);
 	do {
-		const struct of_device_id *m = matches;
+		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])
+			if (m->name[0] && m->compatible[0])
 				match &= node->name
 					&& !strcmp(m->name, node->name);
-			if (m->type[0])
+			if (m->type[0] && m->compatible[0])
 				match &= node->type
 					&& !strcmp(m->type, node->type);
 			if (m->compatible[0])
@@ -770,6 +771,21 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 		}
 	} while (cp && (cplen > 0));
 
+	/* Check against matches without compatible string */
+	m = matches;
+	while (m->name[0] || m->type[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 (match)
+			return m;
+		m++;
+	}
+
 	return NULL;
 }
 

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Sebastian Hesselbarth]

On 02/12/2014 12:41 AM, Scott Wood wrote:
> On Tue, 2014-02-11 at 23:51 +0100, Sebastian Hesselbarth wrote:
>> On 02/11/2014 11:33 PM, Kumar Gala wrote:
>>> Hmm,
>>>
>>> Wondering if this caused the issue:
>>>
>>> commit 105353145eafb3ea919f5cdeb652a9d8f270228e
>>> Author: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Date:   Tue Dec 3 14:52:00 2013 +0100
>>>
>>>       OF: base: match each node compatible against all given matches first
>>
>> [adding Arnd on Cc]
>>
>> Could be. I checked tty/serial/of_serial.c and it does not provide a
>> compatible for "fsl,ns16550". Does reverting the patch fix the issue
>> observed?
>>
>> I don't think the missing compatible is causing it, but of_serial
>> provides a DT match for .type = "serial" just to fail later on
>> with the error seen above.
>>
>> The commit in question reorders of_match_device in a way that match
>> table order is not relevant anymore. This can cause it to match
>> .type = "serial" first here.
>>
>> Rather than touching the commit, I suggest to remove the problematic
>> .type = "serial" from the match table. It is of no use anyway.
>
> Regardless of whether .type = "serial" gets removed, it seems wrong for
> of_match_node() to accept a .type-only match (or .name, or anything else
> that doesn't involve .compatible) before it accepts a compatible match
> other than the first in the compatible property.

Right, I thought about it and came to the same conclusion. I sent a
patch a second ago to prefer .compatible != NULL matches over those
with .compatible == NULL.

Would be great if Stephen can re-test that. If it solves the issue, I
can send a patch tomorrow.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Stephen N Chivers]

Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 
02/12/2014 10:46:36 AM:

> From: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> To: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>, Stephen N Chivers 
> <schivers-znpAAEhiOVUQrrorzV6ljw@public.gmane.org>, Chris Proctor <cproctor-znpAAEhiOVUQrrorzV6ljw@public.gmane.org>, 
> linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>, 
> devicetree <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Date: 02/12/2014 11:04 AM
> Subject: Re: Linux-3.14-rc2: Order of serial node compatibles in DTS 
files.
> 
> On 02/12/2014 12:41 AM, Scott Wood wrote:
> > On Tue, 2014-02-11 at 23:51 +0100, Sebastian Hesselbarth wrote:
> >> On 02/11/2014 11:33 PM, Kumar Gala wrote:
> >>> Hmm,
> >>>
> >>> Wondering if this caused the issue:
> >>>
> >>> commit 105353145eafb3ea919f5cdeb652a9d8f270228e
> >>> Author: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>> Date:   Tue Dec 3 14:52:00 2013 +0100
> >>>
> >>>       OF: base: match each node compatible against all given matches 
first
> >>
> >> [adding Arnd on Cc]
> >>
> >> Could be. I checked tty/serial/of_serial.c and it does not provide a
> >> compatible for "fsl,ns16550". Does reverting the patch fix the issue
> >> observed?
> >>
> >> I don't think the missing compatible is causing it, but of_serial
> >> provides a DT match for .type = "serial" just to fail later on
> >> with the error seen above.
> >>
> >> The commit in question reorders of_match_device in a way that match
> >> table order is not relevant anymore. This can cause it to match
> >> .type = "serial" first here.
> >>
> >> Rather than touching the commit, I suggest to remove the problematic
> >> .type = "serial" from the match table. It is of no use anyway.
> >
> > Regardless of whether .type = "serial" gets removed, it seems wrong 
for
> > of_match_node() to accept a .type-only match (or .name, or anything 
else
> > that doesn't involve .compatible) before it accepts a compatible match
> > other than the first in the compatible property.
> 
> Right, I thought about it and came to the same conclusion. I sent a
> patch a second ago to prefer .compatible != NULL matches over those
> with .compatible == NULL.
> 
> Would be great if Stephen can re-test that. If it solves the issue, I
> can send a patch tomorrow.
Done.

But, the Interrupt Controller (MPIC)
goes AWOL and it is down hill from there.

The MPIC is specified in the DTS as:

        mpic: pic@40000 {
                        interrupt-controller;
                        #address-cells = <0>;
                        #interrupt-cells = <2>;
                        reg = <0x40000 0x40000>;
                        compatible = "chrp,open-pic";
                        device_type = "open-pic";
                        big-endian;
                };

The board support file has the standard mechanism for allocating
the PIC:

        struct mpic *mpic;

        mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC  ");
        BUG_ON(mpic == NULL);

        mpic_init(mpic);

I checked for damage in applying the patch and it has applied
correctly.

Stephen Chivers,
CSC Australia Pty. Ltd.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 3128 bytes --]

On Wed, Feb 12, 2014 at 10:21:58AM +1000, Stephen N Chivers wrote:
> But, the Interrupt Controller (MPIC)
> goes AWOL and it is down hill from there.
> 
> The MPIC is specified in the DTS as:
> 
>         mpic: pic@40000 {
>                         interrupt-controller;
>                         #address-cells = <0>;
>                         #interrupt-cells = <2>;
>                         reg = <0x40000 0x40000>;
>                         compatible = "chrp,open-pic";
>                         device_type = "open-pic";
>                         big-endian;
>                 };
> 
> The board support file has the standard mechanism for allocating
> the PIC:
> 
>         struct mpic *mpic;
> 
>         mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC  ");
>         BUG_ON(mpic == NULL);
> 
>         mpic_init(mpic);
> 
> I checked for damage in applying the patch and it has applied
> correctly.

How about the following fix?

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ff85450d5683..ca91984d3c4b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -730,32 +730,40 @@ out:
 }
 EXPORT_SYMBOL(of_find_node_with_property);
 
+static int of_match_type_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;
+		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,
+		while (m->compatible[0]) {
+			match = of_match_type_name(node, m);
+			match &= cp && !of_compat_cmp(m->compatible, cp,
 							strlen(m->compatible));
 			if (match)
 				return m;
@@ -770,6 +778,15 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 		}
 	} while (cp && (cplen > 0));
 
+	/* Check against matches without compatible string */
+	m = matches;
+	while (!m->compatible[0] && (m->name[0] || m->type[0])) {
+		match = of_match_type_name(node, m);
+		if (match)
+			return m;
+		m++;
+	}
+
 	return NULL;
 }


Thanks,
Kevin
> 
> Stephen Chivers,
> CSC Australia Pty. Ltd.
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Sebastian Hesselbarth]

On 02/12/2014 01:21 AM, Stephen N Chivers wrote:
> Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on
> 02/12/2014 10:46:36 AM:
>
>> From: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> To: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>, Stephen N Chivers
>> <schivers-znpAAEhiOVUQrrorzV6ljw@public.gmane.org>, Chris Proctor <cproctor-znpAAEhiOVUQrrorzV6ljw@public.gmane.org>,
>> linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>,
>> devicetree <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>> Date: 02/12/2014 11:04 AM
>> Subject: Re: Linux-3.14-rc2: Order of serial node compatibles in DTS
> files.
>>
>> On 02/12/2014 12:41 AM, Scott Wood wrote:
>>>
>>> Regardless of whether .type = "serial" gets removed, it seems wrong for
>>> of_match_node() to accept a .type-only match (or .name, or anything else
>>> that doesn't involve .compatible) before it accepts a compatible match
>>> other than the first in the compatible property.
>>
>> Right, I thought about it and came to the same conclusion. I sent a
>> patch a second ago to prefer .compatible != NULL matches over those
>> with .compatible == NULL.
>>
>> Would be great if Stephen can re-test that. If it solves the issue, I
>> can send a patch tomorrow.
> Done.
>
> But, the Interrupt Controller (MPIC)
> goes AWOL and it is down hill from there.
>
> The MPIC is specified in the DTS as:
>
>          mpic: pic@40000 {
>                          interrupt-controller;
>                          #address-cells = <0>;
>                          #interrupt-cells = <2>;
>                          reg = <0x40000 0x40000>;
>                          compatible = "chrp,open-pic";
>                          device_type = "open-pic";
>                          big-endian;
>                  };
>
> The board support file has the standard mechanism for allocating
> the PIC:
>
>          struct mpic *mpic;
>
>          mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC  ");
>          BUG_ON(mpic == NULL);
>
>          mpic_init(mpic);
>
> I checked for damage in applying the patch and it has applied
> correctly.

Hmm, I did a mistake in the patch:

-	while (m->name[0] || m->type[0]) {
+	while (m->compatible[0] || m->name[0] || m->type[0]) {

for the second added match. Otherwise, the matches are not
evaluated down to the sentinel but the loop quits on the first
match table entry without .name and .type set.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Sebastian Hesselbarth]

On 02/12/2014 06:28 AM, Kevin Hao wrote:
> On Wed, Feb 12, 2014 at 10:21:58AM +1000, Stephen N Chivers wrote:
>> But, the Interrupt Controller (MPIC)
>> goes AWOL and it is down hill from there.
>>
>> The MPIC is specified in the DTS as:
>>
>>          mpic: pic@40000 {
>>                          interrupt-controller;
>>                          #address-cells = <0>;
>>                          #interrupt-cells = <2>;
>>                          reg = <0x40000 0x40000>;
>>                          compatible = "chrp,open-pic";
>>                          device_type = "open-pic";
>>                          big-endian;
>>                  };
>>
>> The board support file has the standard mechanism for allocating
>> the PIC:
>>
>>          struct mpic *mpic;
>>
>>          mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC  ");
>>          BUG_ON(mpic == NULL);
>>
>>          mpic_init(mpic);
>>
>> I checked for damage in applying the patch and it has applied
>> correctly.
>
> How about the following fix?
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ff85450d5683..ca91984d3c4b 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -730,32 +730,40 @@ out:
>   }
>   EXPORT_SYMBOL(of_find_node_with_property);
>
> +static int of_match_type_name(const struct device_node *node,
> +				const struct of_device_id *m)

I am fine with having a sub-function here, but it should rather be
named of_match_type_or_name.

> +{
> +	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;
> +}
[...]
> +	/* Check against matches without compatible string */
> +	m = matches;
> +	while (!m->compatible[0] && (m->name[0] || m->type[0])) {

We shouldn't check for anything else than the sentinel here.
Although I guess yours will not quit early as mine did but that
way we don't have to worry about it.

Sebastian

> +		match = of_match_type_name(node, m);
> +		if (match)
> +			return m;
> +		m++;
> +	}
> +
>   	return NULL;
>   }
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

On Wed, Feb 12, 2014 at 09:30:00AM +0100, Sebastian Hesselbarth wrote:
> On 02/12/2014 06:28 AM, Kevin Hao wrote:
> >On Wed, Feb 12, 2014 at 10:21:58AM +1000, Stephen N Chivers wrote:
> >>But, the Interrupt Controller (MPIC)
> >>goes AWOL and it is down hill from there.
> >>
> >>The MPIC is specified in the DTS as:
> >>
> >>         mpic: pic@40000 {
> >>                         interrupt-controller;
> >>                         #address-cells = <0>;
> >>                         #interrupt-cells = <2>;
> >>                         reg = <0x40000 0x40000>;
> >>                         compatible = "chrp,open-pic";
> >>                         device_type = "open-pic";
> >>                         big-endian;
> >>                 };
> >>
> >>The board support file has the standard mechanism for allocating
> >>the PIC:
> >>
> >>         struct mpic *mpic;
> >>
> >>         mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC  ");
> >>         BUG_ON(mpic == NULL);
> >>
> >>         mpic_init(mpic);
> >>
> >>I checked for damage in applying the patch and it has applied
> >>correctly.
> >
> >How about the following fix?
> >
> >diff --git a/drivers/of/base.c b/drivers/of/base.c
> >index ff85450d5683..ca91984d3c4b 100644
> >--- a/drivers/of/base.c
> >+++ b/drivers/of/base.c
> >@@ -730,32 +730,40 @@ out:
> >  }
> >  EXPORT_SYMBOL(of_find_node_with_property);
> >
> >+static int of_match_type_name(const struct device_node *node,
> >+				const struct of_device_id *m)
> 
> I am fine with having a sub-function here, but it should rather be
> named of_match_type_or_name.

OK.

> 
> >+{
> >+	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;
> >+}
> [...]
> >+	/* Check against matches without compatible string */
> >+	m = matches;
> >+	while (!m->compatible[0] && (m->name[0] || m->type[0])) {
> 
> We shouldn't check for anything else than the sentinel here.
> Although I guess yours will not quit early as mine did but that
> way we don't have to worry about it.

Yes, this is still buggy. I will change something like this:

	m = matches;
	/* Check against matches without compatible string */
	while (m->name[0] || m->type[0] || m->compatible[0]) {
		if (m->compatible[0]) {
			m++;
			continue;
		}

		match = of_match_type_name(node, m);
		if (match)
			return m;
		m++;
	}

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Wed, Feb 12, 2014 at 09:25:24AM +0100, Sebastian Hesselbarth wrote:
> 
> Hmm, I did a mistake in the patch:
> 
> -	while (m->name[0] || m->type[0]) {
> +	while (m->compatible[0] || m->name[0] || m->type[0]) {
> 
> for the second added match. Otherwise, the matches are not
> evaluated down to the sentinel but the loop quits on the first
> match table entry without .name and .type set.

But this is still not right. We also need to skip the matches with
.compatible here.

Thanks,
Kevin
> 
> Sebastian
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Arnd Bergmann]

On Wednesday 12 February 2014, Sebastian Hesselbarth wrote:
> On 02/12/2014 12:38 AM, Stephen N Chivers wrote:
> > Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on

> >> I don't think the missing compatible is causing it, but of_serial
> >> provides a DT match for .type = "serial" just to fail later on
> >> with the error seen above.
> >>
> >> The commit in question reorders of_match_device in a way that match
> >> table order is not relevant anymore. This can cause it to match
> >> .type = "serial" first here.
> >>
> >> Rather than touching the commit, I suggest to remove the problematic
> >> .type = "serial" from the match table. It is of no use anyway.
> > Deleting the "serial" line from the match table fixes the problem.
> > I tested it for both orderings of compatible.
> 
> I revert my statement about removing anything from of_serial.c. Instead
> we should try to prefer matches with compatibles over type/name without
> compatibles. Something like the patch below (compile tested only)

That would probably be a good idea. However, I think in this
case we also want to remove the line from the driver, as it clearly
never works on any hardware and the driver just errors out for the
device_type match.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Sebastian Hesselbarth]

On 02/12/14 11:31, Kevin Hao wrote:
> On Wed, Feb 12, 2014 at 09:30:00AM +0100, Sebastian Hesselbarth wrote:
>> On 02/12/2014 06:28 AM, Kevin Hao wrote:
>>> On Wed, Feb 12, 2014 at 10:21:58AM +1000, Stephen N Chivers wrote:
>>>> But, the Interrupt Controller (MPIC)
>>>> goes AWOL and it is down hill from there.
>>>>
>>>> The MPIC is specified in the DTS as:
>>>>
>>>>          mpic: pic@40000 {
>>>>                          interrupt-controller;
>>>>                          #address-cells = <0>;
>>>>                          #interrupt-cells = <2>;
>>>>                          reg = <0x40000 0x40000>;
>>>>                          compatible = "chrp,open-pic";
>>>>                          device_type = "open-pic";
>>>>                          big-endian;
>>>>                  };
>>>>
>>>> The board support file has the standard mechanism for allocating
>>>> the PIC:
>>>>
>>>>          struct mpic *mpic;
>>>>
>>>>          mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC  ");
>>>>          BUG_ON(mpic == NULL);
>>>>
>>>>          mpic_init(mpic);
>>>>
>>>> I checked for damage in applying the patch and it has applied
>>>> correctly.
>>>
>>> How about the following fix?
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index ff85450d5683..ca91984d3c4b 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -730,32 +730,40 @@ out:
>>>   }
>>>   EXPORT_SYMBOL(of_find_node_with_property);
>>>
>>> +static int of_match_type_name(const struct device_node *node,
>>> +				const struct of_device_id *m)
>>
>> I am fine with having a sub-function here, but it should rather be
>> named of_match_type_or_name.
>
> OK.
>
>>
>>> +{
>>> +	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;
>>> +}
>> [...]
>>> +	/* Check against matches without compatible string */
>>> +	m = matches;
>>> +	while (!m->compatible[0] && (m->name[0] || m->type[0])) {
>>
>> We shouldn't check for anything else than the sentinel here.
>> Although I guess yours will not quit early as mine did but that
>> way we don't have to worry about it.
>
> Yes, this is still buggy. I will change something like this:
>
> 	m = matches;
> 	/* Check against matches without compatible string */
> 	while (m->name[0] || m->type[0] || m->compatible[0]) {
> 		if (m->compatible[0]) {
> 			m++;
> 			continue;
> 		}
>
> 		match = of_match_type_name(node, m);
> 		if (match)
> 			return m;
> 		m++;
> 	}

You can cook it down to:

	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++;
	}





--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files.

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Wed, Feb 12, 2014 at 12:26:14PM +0100, Sebastian Hesselbarth wrote:
> You can cook it down to:
> 
> 	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++;
> 	}

Will do.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]