Skip to contentSkip to conversation

cpu/mpc85xx/fdt.c forcing incorrect UART clock into fdt.

U-Boot-Users New Paul Gortmaker 5 participants 9 messages Jul 8, 2008 – Jul 15, 2008

lore
Share & tools
lore

Paul Gortmaker

author
#0 lore raw

I was updating a sbc8560 from u-boot v1.2.0 to git-current, and found that I'd loose the kernel serial console when the 8250 driver took over from udbg0 when using u-boot 1.3.x (booting via tftp'ing the dtb and the uImage separately)

I eventually tracked it down to mpc85xx/fdt.c stomping on the contents of the UART clockrate in the dtb. Since the sbc8560 is sort of different -- in that it has external UARTs (instead of SOC/CPM), I'm guessing few other boards see this problem, because bi_busfreq is really their correct UART clk.

I fixed it with this simple patch, but I was wondering whether it would be better to alternatively just check if the dtb value is zero, and then only insert a value; otherwise leave it alone...

Paul.

cpu/mpc85xx/fdt.c +1 -1
index 92952e6..bd43073 100644
@@ -224,7 +224,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-		"clock-frequency", bd->bi_busfreq, 1);
+		"clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 
 #ifdef CONFIG_CPM2
#1 lore raw
Quoted 5 lines · Paul Gortmaker

I was updating a sbc8560 from u-boot v1.2.0 to git-current, and found that I'd loose the kernel serial console when the 8250 driver took over from udbg0 when using u-boot 1.3.x (booting via tftp'ing the dtb and the uImage separately)

Quoted 8 lines

I eventually tracked it down to mpc85xx/fdt.c stomping on the contents of the UART clockrate in the dtb. Since the sbc8560 is sort of different -- in that it has external UARTs (instead of SOC/CPM), I'm guessing few other boards see this problem, because bi_busfreq is really their correct UART clk.

I fixed it with this simple patch, but I was wondering whether it would be better to alternatively just check if the dtb value is zero, and

Not defined: I would have serious heartburn if the .dts/dtb value were defined to be zero. Define or define not, there should be no zero (with apologies to Yoda).

Quoted 3 lines

then only insert a value; otherwise leave it alone...

Paul.

Changing the fixup last parameter (create) from "1" to "0" will prevent overriding an existing value. Note that this will *not* override a silly value (e.g. 0), so, if you do this, the dtb must either not have the clock-frequency defined or have a correct value in that property. This would have the good effect of allowing the board specific clock to be defined in the dtb where it belongs, but still support the older style approach of compiling it in.

This seems like a good improvement (your patch, but changing the "1" to "0") to me, but I'm not a 85xx expert.

cpu/mpc85xx/fdt.c void ft_cpu_setup(void *blob, bd_t *bd) · line 230

Quoted 13 lines
cpu/mpc85xx/fdt.c +1 -1
index 92952e6..bd43073 100644
@@ -224,7 +224,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-		"clock-frequency", bd->bi_busfreq, 1);
+		"clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 
 #ifdef CONFIG_CPM2

Best regards,
gvb

#2 lore raw
In reply to Paul Gortmaker
Quoted 13 lines · Paul Gortmaker

I was updating a sbc8560 from u-boot v1.2.0 to git-current, and found that I'd loose the kernel serial console when the 8250 driver took over
from udbg0 when using u-boot 1.3.x (booting via tftp'ing the dtb and the uImage separately)

I eventually tracked it down to mpc85xx/fdt.c stomping on the contents of the UART clockrate in the dtb. Since the sbc8560 is sort of different -- in that it has external UARTs (instead of SOC/CPM), I'm guessing few other boards see this problem, because bi_busfreq is really their correct UART clk.

Quoted 4 lines

I fixed it with this simple patch, but I was wondering whether it would
be better to alternatively just check if the dtb value is zero, and then only insert a value; otherwise leave it alone...

I think that should be left up to a board to decide, but in general it seems like doing the setting unconditionally is ok, we just didn't handle the case you had and the patch address it.

- k

author
#3 lore raw

In message: Re: [U-Boot-Users] cpu/mpc85xx/fdt.c forcing incorrect UART clock into fdt.

Quoted 4 lines · on 09/07/2008 Kumar Gala wrote:
Quoted 1 line · Paul Gortmaker
Quoted 8 lines
Quoted 4 lines

I fixed it with this simple patch, but I was wondering whether it would
be better to alternatively just check if the dtb value is zero, and then only insert a value; otherwise leave it alone...

I think that should be left up to a board to decide, but in general it seems like doing the setting unconditionally is ok, we just didn't handle the case you had and the patch address it.

OK, so that we only end up changing just one thing at a time, I'll spin a proper patch which just makes the NS16550_CLK change, and we can change the create flag as per Jerry's comment at a later date if required.

In the interest of consistency, I've make the same change for 83xx and 86xx as well, since the code is pretty much identical, and a MAKEALL shows no boards failing due to a non-defined NS16550_CLK. Patch to follow shortly...

Thanks,
Paul.

Quoted 2 lines

- k

#4 lore raw
Quoted 18 lines · Paul Gortmaker

In message: Re: [U-Boot-Users] cpu/mpc85xx/fdt.c forcing incorrect UART clock into fdt.

Quoted 9 lines · on 09/07/2008 Kumar Gala wrote:
Quoted 4 lines · Paul Gortmaker

I fixed it with this simple patch, but I was wondering whether it would
be better to alternatively just check if the dtb value is zero, and then only insert a value; otherwise leave it alone...

I think that should be left up to a board to decide, but in general it seems like doing the setting unconditionally is ok, we just didn't handle the case you had and the patch address it.

OK, so that we only end up changing just one thing at a time, I'll spin a proper patch which just makes the NS16550_CLK change, and we can change the create flag as per Jerry's comment at a later date if required.

Quoted 7 lines

In the interest of consistency, I've make the same change for 83xx and 86xx as well, since the code is pretty much identical, and a MAKEALL shows no boards failing due to a non-defined NS16550_CLK. Patch to follow shortly...

Thanks,
Paul.

FWIIW, this works for me (with my disclaimer of not being a domain expert).

gvb

author
#5 lore raw

PATCH 1/1 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

In reply to Paul Gortmaker

Some boards that have external 16550 UARTs don't have a direct tie between bi_busfreq and the clock used for the UARTs. Boards that do have such a tie should set CFG_NS16550_CLK to be get_bus_freq(0) -- which most of them do already.

---
 cpu/mpc83xx/fdt.c |    2 +-
 cpu/mpc85xx/fdt.c |    2 +-
 cpu/mpc86xx/fdt.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
cpu/mpc83xx/fdt.c +1 -1
index 02c4d05..8dfe8ba 100644
@@ -60,7 +60,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-		"clock-frequency", bd->bi_busfreq, 1);
+		"clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 
 	fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);
cpu/mpc85xx/fdt.c +1 -1
index 92952e6..bd43073 100644
@@ -224,7 +224,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-		"clock-frequency", bd->bi_busfreq, 1);
+		"clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 
 #ifdef CONFIG_CPM2
cpu/mpc86xx/fdt.c +1 -1
index 379306e..80a5c78 100644
@@ -30,6 +30,6 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-			       "clock-frequency", bd->bi_busfreq, 1);
+			       "clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 }
Signature · 1 line
1.5.6.6.gd3e97
author
#6 lore raw

PATCH 1/1 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

Some boards that have external 16550 UARTs don't have a direct tie between bi_busfreq and the clock used for the UARTs. Boards that do have such a tie should set CFG_NS16550_CLK to be get_bus_freq(0) -- which most of them do already.

Signed-off-by Paul Gortmaker <[email protected]>

---

(Just resending same content with signed off this time...)

 cpu/mpc83xx/fdt.c |    2 +-
 cpu/mpc85xx/fdt.c |    2 +-
 cpu/mpc86xx/fdt.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
cpu/mpc83xx/fdt.c +1 -1
index 02c4d05..8dfe8ba 100644
@@ -60,7 +60,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-		"clock-frequency", bd->bi_busfreq, 1);
+		"clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 
 	fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);
cpu/mpc85xx/fdt.c +1 -1
index 92952e6..bd43073 100644
@@ -224,7 +224,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-		"clock-frequency", bd->bi_busfreq, 1);
+		"clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 
 #ifdef CONFIG_CPM2
cpu/mpc86xx/fdt.c +1 -1
index 379306e..80a5c78 100644
@@ -30,6 +30,6 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 #ifdef CFG_NS16550
 	do_fixup_by_compat_u32(blob, "ns16550",
-			       "clock-frequency", bd->bi_busfreq, 1);
+			       "clock-frequency", CFG_NS16550_CLK, 1);
 #endif
 }
Signature · 1 line
1.5.6.6.gd3e97
#7 lore raw

PATCH 1/1 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

Quoted 6 lines · On Wed, 9 Jul 2008 14:43:46 -0400 Paul Gortmaker…

Some boards that have external 16550 UARTs don't have a direct tie between bi_busfreq and the clock used for the UARTs. Boards that do have such a tie should set CFG_NS16550_CLK to be get_bus_freq(0) -- which most of them do already.

Signed-off-by Paul Gortmaker <[email protected]>
Acked-by Kim Phillips <[email protected]>

Kim

Applied
#8 lore raw

PATCH 1/1 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

Quoted 11 lines · Kim Phillips
Quoted 6 lines · On Wed, 9 Jul 2008 14:43:46 -0400 Paul Gortmaker…

Some boards that have external 16550 UARTs don't have a direct tie between bi_busfreq and the clock used for the UARTs. Boards that do have such a tie should set CFG_NS16550_CLK to be get_bus_freq(0) -- which most of them do already.

Signed-off-by Paul Gortmaker <[email protected]>
Acked-by Kim Phillips <[email protected]>

Applied, thanks

Andy