[U-Boot-Users] cpu/mpc85xx/fdt.c forcing incorrect UART clock into fdt.
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.
diff --git a/cpu/mpc85xx/fdt.c b/cpu/mpc85xx/fdt.c index 92952e6..bd43073 100644 --- a/cpu/mpc85xx/fdt.c +++ b/cpu/mpc85xx/fdt.c @@ -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
Paul Gortmaker wrote:
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
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).
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.
diff --git a/cpu/mpc85xx/fdt.c b/cpu/mpc85xx/fdt.c index 92952e6..bd43073 100644 --- a/cpu/mpc85xx/fdt.c +++ b/cpu/mpc85xx/fdt.c @@ -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
On Jul 8, 2008, at 4:42 PM, Paul Gortmaker wrote:
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...
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
In message: Re: [U-Boot-Users] cpu/mpc85xx/fdt.c forcing incorrect UART clock into fdt. on 09/07/2008 Kumar Gala wrote:
On Jul 8, 2008, at 4:42 PM, Paul Gortmaker wrote:
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.
- k
Paul Gortmaker wrote:
In message: Re: [U-Boot-Users] cpu/mpc85xx/fdt.c forcing incorrect UART clock into fdt. on 09/07/2008 Kumar Gala wrote:
On Jul 8, 2008, at 4:42 PM, Paul Gortmaker wrote:
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.
FWIIW, this works for me (with my disclaimer of not being a domain expert).
gvb
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(-)
diff --git a/cpu/mpc83xx/fdt.c b/cpu/mpc83xx/fdt.c index 02c4d05..8dfe8ba 100644 --- a/cpu/mpc83xx/fdt.c +++ b/cpu/mpc83xx/fdt.c @@ -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); diff --git a/cpu/mpc85xx/fdt.c b/cpu/mpc85xx/fdt.c index 92952e6..bd43073 100644 --- a/cpu/mpc85xx/fdt.c +++ b/cpu/mpc85xx/fdt.c @@ -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 diff --git a/cpu/mpc86xx/fdt.c b/cpu/mpc86xx/fdt.c index 379306e..80a5c78 100644 --- a/cpu/mpc86xx/fdt.c +++ b/cpu/mpc86xx/fdt.c @@ -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 }
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 paul.gortmaker@windriver.com ---
(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(-)
diff --git a/cpu/mpc83xx/fdt.c b/cpu/mpc83xx/fdt.c index 02c4d05..8dfe8ba 100644 --- a/cpu/mpc83xx/fdt.c +++ b/cpu/mpc83xx/fdt.c @@ -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); diff --git a/cpu/mpc85xx/fdt.c b/cpu/mpc85xx/fdt.c index 92952e6..bd43073 100644 --- a/cpu/mpc85xx/fdt.c +++ b/cpu/mpc85xx/fdt.c @@ -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 diff --git a/cpu/mpc86xx/fdt.c b/cpu/mpc86xx/fdt.c index 379306e..80a5c78 100644 --- a/cpu/mpc86xx/fdt.c +++ b/cpu/mpc86xx/fdt.c @@ -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 }
On Wed, 9 Jul 2008 14:43:46 -0400 Paul Gortmaker paul.gortmaker@windriver.com wrote:
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 paul.gortmaker@windriver.com
Acked-by: Kim Phillips kim.phillips@freescale.com
Kim
On Wed, Jul 9, 2008 at 4:53 PM, Kim Phillips kim.phillips@freescale.com wrote:
On Wed, 9 Jul 2008 14:43:46 -0400 Paul Gortmaker paul.gortmaker@windriver.com wrote:
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 paul.gortmaker@windriver.com
Acked-by: Kim Phillips kim.phillips@freescale.com
Applied, thanks
Andy
participants (5)
-
Andy Fleming -
Jerry Van Baren -
Kim Phillips -
Kumar Gala -
Paul Gortmaker