[U-Boot] [PATCH] ppc4xx: Fix UART baudrate setup by FDT

[U-Boot] [PATCH] ppc4xx: Fix UART baudrate setup by FDT

[Matthias Fuchs]

On ppc4xx platforms __ft_board_setup updates clock-frequency
properties of all ns16550 compatible UARTs. This is not a good
idea when those UARTs are external discrete UARTs that are
not clocked by some internal clocks. So any external clock value
in the DTB is overwritten and those UARTs will not be setup correctly
by the Linux kernel.

This patch uses the approach from fdt_fixup_ethernet(). Only UART nodes
that have a serial* alias are updated.
---
 common/fdt_support.c  |   20 ++++++++++++++++++++
 cpu/ppc4xx/fdt.c      |    3 ++-
 include/fdt_support.h |    1 +
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 5a83bca..99d4c3f 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -454,6 +454,26 @@ void fdt_fixup_ethernet(void *fdt)
 	}
 }
 
+void fdt_fixup_serial(void *fdt)
+{
+	int node, i;
+	char serial[10];
+	const char *path;
+
+	node = fdt_path_offset(fdt, "/aliases");
+	if (node < 0)
+		return;
+
+	i = 0;
+	while (1) {
+		sprintf(serial, "serial%d", i++);
+		path = fdt_getprop(fdt, node, serial, NULL);
+		if (path == NULL)
+			break;
+		do_fixup_by_path_u32(fdt, path, "clock-frequency", gd->uart_clk, 1);
+	}
+}
+
 #ifdef CONFIG_HAS_FSL_DR_USB
 void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 {
diff --git a/cpu/ppc4xx/fdt.c b/cpu/ppc4xx/fdt.c
index c55e1cf..5b267d1 100644
--- a/cpu/ppc4xx/fdt.c
+++ b/cpu/ppc4xx/fdt.c
@@ -134,8 +134,9 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 	/*
 	 * Setup all baudrates for the UARTs
+	 * Note: aliases in the dts are required for this
 	 */
-	do_fixup_by_compat_u32(blob, "ns16550", "clock-frequency", gd->uart_clk, 1);
+	fdt_fixup_serial(blob);
 
 	/*
 	 * Fixup all ethernet nodes
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 6062df9..bf3df60 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -49,6 +49,7 @@ void do_fixup_by_compat_u32(void *fdt, const char *compat,
 			    const char *prop, u32 val, int create);
 int fdt_fixup_memory(void *blob, u64 start, u64 size);
 void fdt_fixup_ethernet(void *fdt);
+void fdt_fixup_serial(void *fdt);
 int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
 			 const void *val, int len, int create);
 void fdt_fixup_qe_firmware(void *fdt);
-- 
1.5.6.3

[U-Boot] [PATCH V2] ppc4xx: Fix UART baudrate setup by FDT

[Matthias Fuchs]

On ppc4xx platforms __ft_board_setup updates clock-frequency
properties of all ns16550 compatible UARTs. This is not a good
idea when those UARTs are external discrete UARTs that are
not clocked by some internal clocks. So any external clock value
in the DTB is overwritten and those UARTs will not be setup correctly
by the Linux kernel.

This patch uses the approach from fdt_fixup_ethernet(). Only UART nodes
that have a serial* alias are updated.

Signed-off-by: Matthias Fuchs <matthias.fuchs@esd-electronics.com>
---
 common/fdt_support.c  |   20 ++++++++++++++++++++
 cpu/ppc4xx/fdt.c      |    3 ++-
 include/fdt_support.h |    1 +
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 5a83bca..99d4c3f 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -454,6 +454,26 @@ void fdt_fixup_ethernet(void *fdt)
 	}
 }
 
+void fdt_fixup_serial(void *fdt)
+{
+	int node, i;
+	char serial[10];
+	const char *path;
+
+	node = fdt_path_offset(fdt, "/aliases");
+	if (node < 0)
+		return;
+
+	i = 0;
+	while (1) {
+		sprintf(serial, "serial%d", i++);
+		path = fdt_getprop(fdt, node, serial, NULL);
+		if (path == NULL)
+			break;
+		do_fixup_by_path_u32(fdt, path, "clock-frequency", gd->uart_clk, 1);
+	}
+}
+
 #ifdef CONFIG_HAS_FSL_DR_USB
 void fdt_fixup_dr_usb(void *blob, bd_t *bd)
 {
diff --git a/cpu/ppc4xx/fdt.c b/cpu/ppc4xx/fdt.c
index c55e1cf..5b267d1 100644
--- a/cpu/ppc4xx/fdt.c
+++ b/cpu/ppc4xx/fdt.c
@@ -134,8 +134,9 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 	/*
 	 * Setup all baudrates for the UARTs
+	 * Note: aliases in the dts are required for this
 	 */
-	do_fixup_by_compat_u32(blob, "ns16550", "clock-frequency", gd->uart_clk, 1);
+	fdt_fixup_serial(blob);
 
 	/*
 	 * Fixup all ethernet nodes
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 6062df9..bf3df60 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -49,6 +49,7 @@ void do_fixup_by_compat_u32(void *fdt, const char *compat,
 			    const char *prop, u32 val, int create);
 int fdt_fixup_memory(void *blob, u64 start, u64 size);
 void fdt_fixup_ethernet(void *fdt);
+void fdt_fixup_serial(void *fdt);
 int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
 			 const void *val, int len, int create);
 void fdt_fixup_qe_firmware(void *fdt);
-- 
1.5.6.3

[U-Boot] [PATCH V2] ppc4xx: Fix UART baudrate setup by FDT

[Stefan Roese]

Hi Matthias,

On Friday 02 January 2009, Matthias Fuchs wrote:
> On ppc4xx platforms __ft_board_setup updates clock-frequency
> properties of all ns16550 compatible UARTs. This is not a good
> idea when those UARTs are external discrete UARTs that are
> not clocked by some internal clocks. So any external clock value
> in the DTB is overwritten and those UARTs will not be setup correctly
> by the Linux kernel.
>
> This patch uses the approach from fdt_fixup_ethernet(). Only UART nodes
> that have a serial* alias are updated.

Wouldn't it be "better" to check if an external UART clock is configured via 
CONFIG_SYS_EXT_SERIAL_CLOCK and just use it instead of the calculated 
internal clock value in this case?

BTW: This patch version is not ppc4xx specific as it touches the common 
fdt_support.c. So you would need to split this patch if we decide to go your 
way.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] ppc4xx: Fix UART baudrate setup by FDT

[Matthias Fuchs]

> Hi Matthias,
> 
> On Friday 02 January 2009, Matthias Fuchs wrote:
> > On ppc4xx platforms __ft_board_setup updates clock-frequency
> > properties of all ns16550 compatible UARTs. This is not a good
> > idea when those UARTs are external discrete UARTs that are
> > not clocked by some internal clocks. So any external clock value
> > in the DTB is overwritten and those UARTs will not be setup correctly
> > by the Linux kernel.
> >
> > This patch uses the approach from fdt_fixup_ethernet(). Only UART nodes
> > that have a serial* alias are updated.
> 
> Wouldn't it be "better" to check if an external UART clock is configured via 
> CONFIG_SYS_EXT_SERIAL_CLOCK and just use it instead of the calculated 
> internal clock value in this case?
CONFIG_SYS_EXT_SERIAL_CLOCK is already used to tell U-Boot, that the internal
UARTs are clocked by some externally provides clock. In my special case, I have external
UART chips connected to a 405EP. Even defining CONFIG_SYS_EXT_SERIAL_CLOCK will
bring up an error - the 405EP does not support external UART clock. 
So this special macro should not be used. 
On our board (PLU405) there is no reason to touch the external UARTs from
U-Boot. So I think the best way would be to leave their fdt description untouched. 
And the correct clock comes from the device tree.

So which UART's clock would you prefer to be setup to CONFIG_SYS_EXT_SERIAL_CLOCK?
All non-alias'd?

(In reality my UARTs are ns16850 compatible. When I put that into the DT,
U-Boot does not touch them, but the Linux kernel's drivers/serial/of_serial.c
does not handle them. Independant from this I posted a patch to extend of_serial.c
for ns16850 to the linux-serial mailing list.)

So the very best way would be to let U-Boot detect internal UARTs. We could do that
by adding an additional compatible value to the internal UART nodes:

                  UART0: serial at ef600300 {
                                device_type = "serial";
                                compatible = "ns16550", "ibm,uart";
                                ...

Any other ideas?
 
> BTW: This patch version is not ppc4xx specific as it touches the common 
> fdt_support.c. So you would need to split this patch if we decide to go your 
> way.
I will do so when we decided which way to go.

Matthias

> 
> Best regards,
> Stefan

[U-Boot] [PATCH V2] ppc4xx: Fix UART baudrate setup by FDT

[Stefan Roese]

On Monday 05 January 2009, Matthias Fuchs wrote:
> > > On ppc4xx platforms __ft_board_setup updates clock-frequency
> > > properties of all ns16550 compatible UARTs. This is not a good
> > > idea when those UARTs are external discrete UARTs that are
> > > not clocked by some internal clocks. So any external clock value
> > > in the DTB is overwritten and those UARTs will not be setup correctly
> > > by the Linux kernel.
> > >
> > > This patch uses the approach from fdt_fixup_ethernet(). Only UART nodes
> > > that have a serial* alias are updated.
> >
> > Wouldn't it be "better" to check if an external UART clock is configured
> > via CONFIG_SYS_EXT_SERIAL_CLOCK and just use it instead of the calculated
> > internal clock value in this case?
>
> CONFIG_SYS_EXT_SERIAL_CLOCK is already used to tell U-Boot, that the
> internal UARTs are clocked by some externally provides clock. In my special
> case, I have external UART chips connected to a 405EP.

Ups. I misread your original mail. I thought you were talking about external 
clocks provided to the internal UARTs instad of using external UARTs. Sorry.

> Even defining 
> CONFIG_SYS_EXT_SERIAL_CLOCK will bring up an error - the 405EP does not
> support external UART clock. So this special macro should not be used.
> On our board (PLU405) there is no reason to touch the external UARTs from
> U-Boot. So I think the best way would be to leave their fdt description
> untouched. And the correct clock comes from the device tree.

OK, makes sense to me.

> So which UART's clock would you prefer to be setup to
> CONFIG_SYS_EXT_SERIAL_CLOCK? All non-alias'd?
>
> (In reality my UARTs are ns16850 compatible. When I put that into the DT,
> U-Boot does not touch them, but the Linux kernel's
> drivers/serial/of_serial.c does not handle them. Independant from this I
> posted a patch to extend of_serial.c for ns16850 to the linux-serial
> mailing list.)
>
> So the very best way would be to let U-Boot detect internal UARTs. We could
> do that by adding an additional compatible value to the internal UART
> nodes:
>
>                   UART0: serial at ef600300 {
>                                 device_type = "serial";
>                                 compatible = "ns16550", "ibm,uart";
>                                 ...

That could be done as well. Perhaps it's the "better" solution. You might want 
to ask on the linuxppc-dev list if such a patch is welcome. If yes, then we 
should go this way.

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] ppc4xx: Fix UART baudrate setup by FDT

[Matthias Fuchs]

Hi Stefan,

> > Even defining 
> > CONFIG_SYS_EXT_SERIAL_CLOCK will bring up an error - the 405EP does not
> > support external UART clock. So this special macro should not be used.
> > On our board (PLU405) there is no reason to touch the external UARTs from
> > U-Boot. So I think the best way would be to leave their fdt description
> > untouched. And the correct clock comes from the device tree.
> 
> OK, makes sense to me.

> > So the very best way would be to let U-Boot detect internal UARTs. We could
> > do that by adding an additional compatible value to the internal UART
> > nodes:
> >
> >                   UART0: serial at ef600300 {
> >                                 device_type = "serial";
> >                                 compatible = "ns16550", "ibm,uart";
> >                                 ...
> 
> That could be done as well. Perhaps it's the "better" solution. You might want 
> to ask on the linuxppc-dev list if such a patch is welcome. If yes, then we 
> should go this way.
Changing U-Boot to check for something different than ns16550 will break all
board that have ns16550 in their dt. 

The Linux kernel only checks for ns16550 compatible nodes under specific parents
(see arch/powerpc/kernel/legacy_serial.c). For 4xx this is only "opb" and not even opb/ebc.

So I think we should do it also this way. In this case we could keep the modification 
in U-Boots' cpu/ppc4xx/fdt.c.

When you are with me I will try to prepare a patch.

Matthias

> 
> Thanks.
> 
> Best regards,
> Stefan

[U-Boot] [PATCH V2] ppc4xx: Fix UART baudrate setup by FDT

[Stefan Roese]

Hi Matthias,

On Tuesday 06 January 2009, Matthias Fuchs wrote:
> > >                   UART0: serial at ef600300 {
> > >                                 device_type = "serial";
> > >                                 compatible = "ns16550", "ibm,uart";
> > >                                 ...
> >
> > That could be done as well. Perhaps it's the "better" solution. You might
> > want to ask on the linuxppc-dev list if such a patch is welcome. If yes,
> > then we should go this way.
>
> Changing U-Boot to check for something different than ns16550 will break
> all board that have ns16550 in their dt.

Ah, your talking about compatibility problems with older dts files. 
Understood.

> The Linux kernel only checks for ns16550 compatible nodes under specific
> parents (see arch/powerpc/kernel/legacy_serial.c). For 4xx this is only
> "opb" and not even opb/ebc.
>
> So I think we should do it also this way. In this case we could keep the
> modification in U-Boots' cpu/ppc4xx/fdt.c.
>
> When you are with me I will try to prepare a patch.

Yes, this sounds like a good idea. Please go ahead.

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================