PATCH[1/1] 8xx: Add clock-frequency to .dts brg entries

PATCH[1/1] 8xx: Add clock-frequency to .dts brg entries

[Bryan O'Donoghue]

cpm_uart_core has a dependency on fsl,cpm-brg/clock-frequency, this
means that a .dts that uses the cpm uart driver needs to supply a
clock-frequency entry for get_brgfreq to return a meaningful number.

Included is a patchset which adds the correct brgclk to the adder port -
@ 50Mhz and also adds an entry for mpc885ads - which I've noticed is
missing a clock-frequency entry.

Without an entry for clock-frequency in the .dts you'd get -1 as the
return value brgclk !

Signed-off-by: Bryan O'Donoghue <bodonoghue@codehermit.ie>
---

diff --git a/arch/powerpc/boot/dts/adder875-redboot.dts
b/arch/powerpc/boot/dts/adder875-redboot.dts
index 7c25d96..975f3d4 100644
--- a/arch/powerpc/boot/dts/adder875-redboot.dts
+++ b/arch/powerpc/boot/dts/adder875-redboot.dts
@@ -149,6 +149,7 @@
                                compatible = "fsl,mpc875-brg",
                                             "fsl,cpm1-brg",
                                             "fsl,cpm-brg";
+                               clock-frequency = <0x2faf080>;
                                reg = <0x9f0 0x10>;
                        };
 
diff --git a/arch/powerpc/boot/dts/adder875-uboot.dts
b/arch/powerpc/boot/dts/adder875-uboot.dts
index 605202f..18d6dd6 100644
--- a/arch/powerpc/boot/dts/adder875-uboot.dts
+++ b/arch/powerpc/boot/dts/adder875-uboot.dts
@@ -148,6 +148,7 @@
                                compatible = "fsl,mpc875-brg",
                                             "fsl,cpm1-brg",
                                             "fsl,cpm-brg";
+                               clock-frequency = <0x2faf080>;
                               reg = <0x9f0 0x10>;
                        };
 
diff --git a/arch/powerpc/boot/dts/mpc885ads.dts
b/arch/powerpc/boot/dts/mpc885ads.dts
index 8848e63..f2a437b 100644
--- a/arch/powerpc/boot/dts/mpc885ads.dts
+++ b/arch/powerpc/boot/dts/mpc885ads.dts
@@ -166,6 +166,7 @@
                                compatible = "fsl,mpc885-brg",
                                             "fsl,cpm1-brg",
                                             "fsl,cpm-brg";
+                               clock-frequency = <0>;
                                reg = <9f0 10>;
                        };

Re: PATCH[1/1] 8xx: Add clock-frequency to .dts brg entries

[Scott Wood]

On Mon, Jan 28, 2008 at 01:53:11AM +0000, Bryan O'Donoghue wrote:
> cpm_uart_core has a dependency on fsl,cpm-brg/clock-frequency, this
> means that a .dts that uses the cpm uart driver needs to supply a
> clock-frequency entry for get_brgfreq to return a meaningful number.
> 
> Included is a patchset which adds the correct brgclk to the adder port -
> @ 50Mhz and also adds an entry for mpc885ads - which I've noticed is
> missing a clock-frequency entry.

It's not missing -- it's added by the bootwrapper.

-Scott

Re: PATCH[1/1] 8xx: Add clock-frequency to .dts brg entries

[Bryan O'Donoghue]

On Mon, 2008-01-28 at 09:50 -0600, Scott Wood wrote:
> On Mon, Jan 28, 2008 at 01:53:11AM +0000, Bryan O'Donoghue wrote:
> > cpm_uart_core has a dependency on fsl,cpm-brg/clock-frequency, this
> > means that a .dts that uses the cpm uart driver needs to supply a
> > clock-frequency entry for get_brgfreq to return a meaningful number.
> > 
> > Included is a patchset which adds the correct brgclk to the adder port -
> > @ 50Mhz and also adds an entry for mpc885ads - which I've noticed is
> > missing a clock-frequency entry.
> 
> It's not missing -- it's added by the bootwrapper.
> 
> -Scott

Hmm..

You mean that arch/powerpc/boot/mpc8xx.c mpc8xx_set_clocks is supposed
to be adding this field ? 

I see arch/powerpc/boot/wrapper.a has a reference to the function but -
and this time I've checked all documentation - there's no mention of how
to use this library at all... it _looks_ to me like this isn't being
linked in any way.

It for sure is nowhere in the uImage - and I've taken the preferred
route of making a uImage with .dtb - genreated from adder875-uboot.dts

dtc -O -o adder875-uboot.dtb arch/powerpc/boot/dts/adder875-uboot.dtb


cpm_uart depends on "fsl,cpm-brg" and a field called "clock-frequency"

as I understand it that's

fsl,cpm-brg
	|_clock-frequency

whereas mpc8xx_set_clocks seems to add

/soc/cpm/brg
	|_clock-frequency

So unless I'm not understanding the structure of the tree - possible - I
don't see how /soc/cpm/brg => clock-frequency could /possibly/ satisfy
get_brgfreq in fsl_soc.c

If there's something other then making a uImage and dtb and booting
these from u-boot that I'm supposed to be doing here ... it'd help if
you could say..

Otherwise in order to get the UART working using a uImage + dbt I've
found it necessary to add this field to the .dts....

mpc866ads.dts - also has a "fsl,cpm-brg" => clock-frequency entry in 

linux/arch/powerpc/boot/dts/mpc866ads.dts - and to me this looks like
the correct approach for get_brgfreq to function properly...

What do you think ?

Re: PATCH[1/1] 8xx: Add clock-frequency to .dts brg entries

[Scott Wood]

On Mon, Jan 28, 2008 at 11:55:20PM +0000, Bryan O'Donoghue wrote:
> You mean that arch/powerpc/boot/mpc8xx.c mpc8xx_set_clocks is supposed
> to be adding this field ? 

Yes.  Or u-boot, if you're not using the bootwrapper/cuImage.

> I see arch/powerpc/boot/wrapper.a has a reference to the function but -
> and this time I've checked all documentation - there's no mention of how
> to use this library at all... it _looks_ to me like this isn't being
> linked in any way.
> 
> It for sure is nowhere in the uImage - and I've taken the preferred
> route of making a uImage with .dtb - genreated from adder875-uboot.dts

In that case, u-boot needs to add that property.

> dtc -O -o adder875-uboot.dtb arch/powerpc/boot/dts/adder875-uboot.dtb

You'll want to use the -p option to add some extra space for u-boot to
use.

> cpm_uart depends on "fsl,cpm-brg" and a field called "clock-frequency"
> 
> as I understand it that's
> 
> fsl,cpm-brg
> 	|_clock-frequency
> 
> whereas mpc8xx_set_clocks seems to add
> 
> /soc/cpm/brg
> 	|_clock-frequency

The fsl,cpm-brg refers to the compatible property, not the node name.

> mpc866ads.dts - also has a "fsl,cpm-brg" => clock-frequency entry in 
> 
> linux/arch/powerpc/boot/dts/mpc866ads.dts - and to me this looks like
> the correct approach for get_brgfreq to function properly...

And it's zero, meaning that you still need u-boot or the bootwrapper to
fill in the correct value.  The only difference is whether there's a
placeholder property -- they used to be required by older u-boots, but
are no longer necessary.

-Scott

Re: PATCH[1/1] 8xx: Add clock-frequency to .dts brg entries

[Bryan O'Donoghue]

On Tue, 2008-01-29 at 10:26 -0600, Scott Wood wrote:
> On Mon, Jan 28, 2008 at 11:55:20PM +0000, Bryan O'Donoghue wrote:
> > You mean that arch/powerpc/boot/mpc8xx.c mpc8xx_set_clocks is supposed
> > to be adding this field ? 
> 
> Yes.  Or u-boot, if you're not using the bootwrapper/cuImage.

Indeed - I have some patches to send to the u-boot mailing list to cover
manipulation of the "fsl,cpm-brg" "clock-frequency" entry.

The previously sent patch will allow Linux to print stuff out the UART
will the default brgclk - from a uImage and is the field that a future
u-boot patchset would want to manipulate in order to tell Linux about
the brgclk it should have.

I should be a bit clearer about that...

> 
> > I see arch/powerpc/boot/wrapper.a has a reference to the function but -
> > and this time I've checked all documentation - there's no mention of how
> > to use this library at all... it _looks_ to me like this isn't being
> > linked in any way.
> > 
> > It for sure is nowhere in the uImage - and I've taken the preferred
> > route of making a uImage with .dtb - genreated from adder875-uboot.dts
> 
> In that case, u-boot needs to add that property.

See above.

> > dtc -O -o adder875-uboot.dtb arch/powerpc/boot/dts/adder875-uboot.dtb
> 
> You'll want to use the -p option to add some extra space for u-boot to
> use.

Interesting - the u-boot end of this patch is something I'm still
messing about with ... but, the addition of the default brgclk to the
Linux dts is a useful addition - and AFAICT will be needed to allow a
bootloader to manipulate the dts so that Linux will get the right
brgclk.

> > cpm_uart depends on "fsl,cpm-brg" and a field called "clock-frequency"
> > 
> > as I understand it that's
> > 
> > fsl,cpm-brg
> > 	|_clock-frequency
> > 
> > whereas mpc8xx_set_clocks seems to add
> > 
> > /soc/cpm/brg
> > 	|_clock-frequency
> 
> The fsl,cpm-brg refers to the compatible property, not the node name.

Indeed and the get_brgclk does a find_compatible() lookup of some
sort... so I think that means this is the right field to add to allow a
bootloader to manipulate the brgclk ... it's what the other stuff in
u-boot for the mpc8xxx stuff does, so it follows to do the same thing
for 8xx too.

> > mpc866ads.dts - also has a "fsl,cpm-brg" => clock-frequency entry in 
> > 
> > linux/arch/powerpc/boot/dts/mpc866ads.dts - and to me this looks like
> > the correct approach for get_brgfreq to function properly...
> 
> And it's zero, meaning that you still need u-boot or the bootwrapper to
> fill in the correct value.

Indeed - I'm hoping I won't have too much trouble getting those patches
applied...