[RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[Curt Brune]

Currently for PowerPC systems using device tree, the legacy serial
ports are allocated in the order by which they are found in the device
tree structure.

This can cause problems in cases where the device tree nodes are
sysnthesized by an external program (kexec for example), which may not
preserve the original order of the device tree nodes.  This in turn
can result in the serial console changing from ttyS0 to ttyS1
unexpectedly.

This patch uses the 'cell-index' property (if present) to map UART
device tree nodes to legacy serial port number.  The cell-index
property is widely used in all the Freescale .dts files for PowerPC.

Signed-off-by: Curt Brune <curt@cumulusnetworks.com>
---
 arch/powerpc/kernel/legacy_serial.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index 7b750c4..b717133 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -164,6 +164,8 @@ static int __init add_legacy_soc_port(struct device_node *np,
 	u64 addr;
 	const __be32 *addrp;
 	struct device_node *tsi = of_get_parent(np);
+	int index = -1, len;
+	u32 *indexp;
 
 	/* We only support ports that have a clock frequency properly
 	 * encoded in the device-tree.
@@ -188,14 +190,19 @@ static int __init add_legacy_soc_port(struct device_node *np,
 	if (addr == OF_BAD_ADDR)
 		return -1;
 
+	/* Check if the ports have an ordering, defined by 'cell-index' */
+	if (((indexp = (u32*)of_get_property(np, "cell-index", &len)) != NULL) &&
+	    (len == sizeof(u32)))
+		index = *indexp;
+
 	/* Add port, irq will be dealt with later. We passed a translated
 	 * IO port value. It will be fixed up later along with the irq
 	 */
 	if (tsi && !strcmp(tsi->type, "tsi-bridge"))
-		return add_legacy_port(np, -1, UPIO_TSI, addr, addr,
+		return add_legacy_port(np, index, UPIO_TSI, addr, addr,
 				       NO_IRQ, legacy_port_flags, 0);
 	else
-		return add_legacy_port(np, -1, UPIO_MEM, addr, addr,
+		return add_legacy_port(np, index, UPIO_MEM, addr, addr,
 				       NO_IRQ, legacy_port_flags, 0);
 }
 
-- 
1.9.1

Re: [RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[Segher Boessenkool]

On Wed, Nov 25, 2015 at 10:46:49AM -0800, Curt Brune wrote:
> +	/* Check if the ports have an ordering, defined by 'cell-index' */
> +	if (((indexp = (u32*)of_get_property(np, "cell-index", &len)) != NULL) &&
> +	    (len == sizeof(u32)))
> +		index = *indexp;
> +
>  	/* Add port, irq will be dealt with later. We passed a translated
>  	 * IO port value. It will be fixed up later along with the irq
>  	 */
>  	if (tsi && !strcmp(tsi->type, "tsi-bridge"))
> -		return add_legacy_port(np, -1, UPIO_TSI, addr, addr,
> +		return add_legacy_port(np, index, UPIO_TSI, addr, addr,
>  				       NO_IRQ, legacy_port_flags, 0);
>  	else
> -		return add_legacy_port(np, -1, UPIO_MEM, addr, addr,
> +		return add_legacy_port(np, index, UPIO_MEM, addr, addr,
>  				       NO_IRQ, legacy_port_flags, 0);

What does this do if the value in cell-index is a duplicate. or the port
with that number is already created some other way?  Are the numbers in
cell-index global anyway, or relative to some parent device (I couldn't
find the documentation for this).


Segher

Re: [RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[Curt Brune]

On Sun Dec 20 09:02, Segher Boessenkool wrote:
> On Wed, Nov 25, 2015 at 10:46:49AM -0800, Curt Brune wrote:
> > +	/* Check if the ports have an ordering, defined by 'cell-index' */
> > +	if (((indexp = (u32*)of_get_property(np, "cell-index", &len)) != NULL) &&
> > +	    (len == sizeof(u32)))
> > +		index = *indexp;
> > +
> >  	/* Add port, irq will be dealt with later. We passed a translated
> >  	 * IO port value. It will be fixed up later along with the irq
> >  	 */
> >  	if (tsi && !strcmp(tsi->type, "tsi-bridge"))
> > -		return add_legacy_port(np, -1, UPIO_TSI, addr, addr,
> > +		return add_legacy_port(np, index, UPIO_TSI, addr, addr,
> >  				       NO_IRQ, legacy_port_flags, 0);
> >  	else
> > -		return add_legacy_port(np, -1, UPIO_MEM, addr, addr,
> > +		return add_legacy_port(np, index, UPIO_MEM, addr, addr,
> >  				       NO_IRQ, legacy_port_flags, 0);
> 
> What does this do if the value in cell-index is a duplicate. or the port
> with that number is already created some other way?  Are the numbers in
> cell-index global anyway, or relative to some parent device (I couldn't
> find the documentation for this).
> 
> 
> Segher

add_legacy_port() has support for handling duplicate index entries.
Reading that code it looks like the recent entry wins and the older
entry is moved to the next available index.

The numbers in cell-index are not global nor explicitly relative to a
parent device.  I also could not find documentation on this.

In common usage, however, the cell-index property is used in two
different ways:

1.  For devices that have "channels" or "regions" (DMA for example),
the parent device node has child nodes enumerated by cell-index.  For
example see:  arch/powerpc/boot/dts/fsl/pq3-dma-0.dtsi

2.  For multiple instances of a device, for example UARTS,
each device node is enumerated using cell-index.  See:
arch/powerpc/boot/dts/fsl/qoriq-duart-0.dtsi

Cheers,
Curt

Re: [RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[Segher Boessenkool]

On Wed, Dec 30, 2015 at 08:36:01AM -0800, Curt Brune wrote:
> > What does this do if the value in cell-index is a duplicate. or the port
> > with that number is already created some other way?  Are the numbers in
> > cell-index global anyway, or relative to some parent device (I couldn't
> > find the documentation for this).
> 
> add_legacy_port() has support for handling duplicate index entries.
> Reading that code it looks like the recent entry wins and the older
> entry is moved to the next available index.
> 
> The numbers in cell-index are not global nor explicitly relative to a
> parent device.  I also could not find documentation on this.

That sounds like it should work then.  It does change behaviour for
existing configs.

Thanks,


Segher

Re: [RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[Scott Wood]

On Wed, 2015-11-25 at 10:46 -0800, Curt Brune wrote:
> Currently for PowerPC systems using device tree, the legacy serial
> ports are allocated in the order by which they are found in the device
> tree structure.
> 
> This can cause problems in cases where the device tree nodes are
> sysnthesized by an external program (kexec for example), which may not
> preserve the original order of the device tree nodes.  This in turn
> can result in the serial console changing from ttyS0 to ttyS1
> unexpectedly.
> 
> This patch uses the 'cell-index' property (if present) to map UART
> device tree nodes to legacy serial port number.  The cell-index
> property is widely used in all the Freescale .dts files for PowerPC.
> 
> Signed-off-by: Curt Brune <curt@cumulusnetworks.com>
> ---
>  arch/powerpc/kernel/legacy_serial.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/legacy_serial.c
> b/arch/powerpc/kernel/legacy_serial.c
> index 7b750c4..b717133 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -164,6 +164,8 @@ static int __init add_legacy_soc_port(struct device_node
> *np,
>  	u64 addr;
>  	const __be32 *addrp;
>  	struct device_node *tsi = of_get_parent(np);
> +	int index = -1, len;
> +	u32 *indexp;
>  
>  	/* We only support ports that have a clock frequency properly
>  	 * encoded in the device-tree.
> @@ -188,14 +190,19 @@ static int __init add_legacy_soc_port(struct
> device_node *np,
>  	if (addr == OF_BAD_ADDR)
>  		return -1;
>  
> +	/* Check if the ports have an ordering, defined by 'cell-index' */
> +	if (((indexp = (u32*)of_get_property(np, "cell-index", &len)) !=
> NULL) &&
> +	    (len == sizeof(u32)))
> +		index = *indexp;

const u32 *

Or better, use of_read_number() or similar.

-Scott

Re: [RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[Curt Brune]

On Mon Dec 21 21:42, Scott Wood wrote:
> On Wed, 2015-11-25 at 10:46 -0800, Curt Brune wrote:
> > Currently for PowerPC systems using device tree, the legacy serial
> > ports are allocated in the order by which they are found in the device
> > tree structure.
> > 
> > This can cause problems in cases where the device tree nodes are
> > sysnthesized by an external program (kexec for example), which may not
> > preserve the original order of the device tree nodes.  This in turn
> > can result in the serial console changing from ttyS0 to ttyS1
> > unexpectedly.
> > 
> > This patch uses the 'cell-index' property (if present) to map UART
> > device tree nodes to legacy serial port number.  The cell-index
> > property is widely used in all the Freescale .dts files for PowerPC.
> > 
> > Signed-off-by: Curt Brune <curt@cumulusnetworks.com>
> > ---
> >  arch/powerpc/kernel/legacy_serial.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/legacy_serial.c
> > b/arch/powerpc/kernel/legacy_serial.c
> > index 7b750c4..b717133 100644
> > --- a/arch/powerpc/kernel/legacy_serial.c
> > +++ b/arch/powerpc/kernel/legacy_serial.c
> > @@ -164,6 +164,8 @@ static int __init add_legacy_soc_port(struct device_node
> > *np,
> >  	u64 addr;
> >  	const __be32 *addrp;
> >  	struct device_node *tsi = of_get_parent(np);
> > +	int index = -1, len;
> > +	u32 *indexp;
> >  
> >  	/* We only support ports that have a clock frequency properly
> >  	 * encoded in the device-tree.
> > @@ -188,14 +190,19 @@ static int __init add_legacy_soc_port(struct
> > device_node *np,
> >  	if (addr == OF_BAD_ADDR)
> >  		return -1;
> >  
> > +	/* Check if the ports have an ordering, defined by 'cell-index' */
> > +	if (((indexp = (u32*)of_get_property(np, "cell-index", &len)) !=
> > NULL) &&
> > +	    (len == sizeof(u32)))
> > +		index = *indexp;
> 
> const u32 *
> 
> Or better, use of_read_number() or similar.
> 
> -Scott
> 

Ack.  Thanks for the pointer.

-Curt

Re: [RFC 1/1] powerpc: legacy serial port use device tree cell-index property

[Rob Herring]

On Wed, Nov 25, 2015 at 12:46 PM, Curt Brune <curt@cumulusnetworks.com> wrote:
> Currently for PowerPC systems using device tree, the legacy serial
> ports are allocated in the order by which they are found in the device
> tree structure.
>
> This can cause problems in cases where the device tree nodes are
> sysnthesized by an external program (kexec for example), which may not
> preserve the original order of the device tree nodes.  This in turn
> can result in the serial console changing from ttyS0 to ttyS1
> unexpectedly.
>
> This patch uses the 'cell-index' property (if present) to map UART
> device tree nodes to legacy serial port number.  The cell-index
> property is widely used in all the Freescale .dts files for PowerPC.

If cell-index is already present in the dtbs, then this is okay.
Otherwise use aliases. cell-index is pretty much deprecated.

Rob