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

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

[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.

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

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

[Jerry Van Baren]

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

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

[Kumar Gala]

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

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

[Paul Gortmaker]

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

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

[Jerry Van Baren]

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

[U-Boot-Users] [PATCH] 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

[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(-)

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
 }
-- 
1.5.6.6.gd3e97

[U-Boot-Users] [PATCH] 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

[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 <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
 }
-- 
1.5.6.6.gd3e97

[U-Boot-Users] [PATCH] 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

[Kim Phillips]

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

[U-Boot-Users] [PATCH] 8xxx-fdt: set ns16550 clock from CFG_NS16550_CLK, not bi_busfreq

[Andy Fleming]

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