diff mbox

[6/8] --- arch/powerpc/boot/dts/socrates.dts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

Message ID 20090331124036.057237866@denx.de (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Wolfgang Grandegger March 31, 2009, 12:37 p.m. UTC

Comments

David Gibson March 31, 2009, 11:04 p.m. UTC | #1 
On Tue, Mar 31, 2009 at 02:37:33PM +0200, Wolfgang Grandegger wrote:
> Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
> +++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
> @@ -52,6 +52,7 @@
>  	soc8544@e0000000 {
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> +		device_type = "soc";

You should not be adding device_type to a new board's dts.  You
*especially* shouldn't be doing that with no patch comment explaining
why.
Wolfgang Grandegger April 1, 2009, 7:31 a.m. UTC | #2 
Hi David,

this patch went out by mistake, sorry.

David Gibson wrote:
> On Tue, Mar 31, 2009 at 02:37:33PM +0200, Wolfgang Grandegger wrote:
>> Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
>> ===================================================================
>> --- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
>> +++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
>> @@ -52,6 +52,7 @@
>>  	soc8544@e0000000 {
>>  		#address-cells = <1>;
>>  		#size-cells = <1>;
>> +		device_type = "soc";
> 
> You should not be adding device_type to a new board's dts.  You
> *especially* shouldn't be doing that with no patch comment explaining
> why.

In the final patch I have removed the device_type = "soc", but it makes
trouble as some mpc85xx rely on it. Removing it would also break U-Boot
compatibility. Is it worth that? The issue is discussed on the thread
http://ozlabs.org/pipermail/linuxppc-dev/2009-April/070020.html.

Wolfgang.
David Gibson April 1, 2009, 11:09 a.m. UTC | #3 
On Wed, Apr 01, 2009 at 09:31:34AM +0200, Wolfgang Grandegger wrote:
> Hi David,
> 
> this patch went out by mistake, sorry.
> 
> David Gibson wrote:
> > On Tue, Mar 31, 2009 at 02:37:33PM +0200, Wolfgang Grandegger wrote:
> >> Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
> >> ===================================================================
> >> --- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
> >> +++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
> >> @@ -52,6 +52,7 @@
> >>  	soc8544@e0000000 {
> >>  		#address-cells = <1>;
> >>  		#size-cells = <1>;
> >> +		device_type = "soc";
> > 
> > You should not be adding device_type to a new board's dts.  You
> > *especially* shouldn't be doing that with no patch comment explaining
> > why.
> 
> In the final patch I have removed the device_type = "soc", but it makes
> trouble as some mpc85xx rely on it.

Um.. what?  Some mpc85xx?  This is the dts for *one specific board*, I
don't see how something that affects "some mpc85xx" is relevant.
Wolfgang Grandegger April 1, 2009, 11:59 a.m. UTC | #4 
David Gibson wrote:
> On Wed, Apr 01, 2009 at 09:31:34AM +0200, Wolfgang Grandegger wrote:
>> Hi David,
>>
>> this patch went out by mistake, sorry.
>>
>> David Gibson wrote:
>>> On Tue, Mar 31, 2009 at 02:37:33PM +0200, Wolfgang Grandegger wrote:
>>>> Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
>>>> ===================================================================
>>>> --- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
>>>> +++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
>>>> @@ -52,6 +52,7 @@
>>>>  	soc8544@e0000000 {
>>>>  		#address-cells = <1>;
>>>>  		#size-cells = <1>;
>>>> +		device_type = "soc";
>>> You should not be adding device_type to a new board's dts.  You
>>> *especially* shouldn't be doing that with no patch comment explaining
>>> why.
>> In the final patch I have removed the device_type = "soc", but it makes
>> trouble as some mpc85xx rely on it.
> 
> Um.. what?  Some mpc85xx?  This is the dts for *one specific board*, I
> don't see how something that affects "some mpc85xx" is relevant.

Well, to be more precise, it affects all boards using CONFIG_FSL_SOC
because they use functions searching for the device type "soc", e.g.
fsl_get_sys_freq():

http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80

This needs to be fixed before the line 'device_type = "soc"' can be removed.

Wolfgang.
Anton Vorontsov April 1, 2009, 12:30 p.m. UTC | #5 
On Wed, Apr 01, 2009 at 10:09:55PM +1100, David Gibson wrote:
> On Wed, Apr 01, 2009 at 09:31:34AM +0200, Wolfgang Grandegger wrote:
> > Hi David,
> > 
> > this patch went out by mistake, sorry.
> > 
> > David Gibson wrote:
> > > On Tue, Mar 31, 2009 at 02:37:33PM +0200, Wolfgang Grandegger wrote:
> > >> Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
> > >> ===================================================================
> > >> --- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
> > >> +++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
> > >> @@ -52,6 +52,7 @@
> > >>  	soc8544@e0000000 {
> > >>  		#address-cells = <1>;
> > >>  		#size-cells = <1>;
> > >> +		device_type = "soc";
> > > 
> > > You should not be adding device_type to a new board's dts.  You
> > > *especially* shouldn't be doing that with no patch comment explaining
> > > why.
> > 
> > In the final patch I have removed the device_type = "soc", but it makes
> > trouble as some mpc85xx rely on it.
> 
> Um.. what?  Some mpc85xx?  This is the dts for *one specific board*, I
> don't see how something that affects "some mpc85xx" is relevant.

Not some, but *all* mpc85xx. The thing is that U-Boot is searching
for device_type = "soc" to fixup some properties (for example,
bus-frequency) for all mpc85xx parts, so you can't easily remove
the device_type w/o breaking boot for firmwares that customers have
pre-programmed in their products.

Note that upgrading firmware without JTAG is dangerous, so it's not
an option to "force" everybody to upgrade their firmware just
because we want to remove some cruft from the device tree.
David Gibson April 1, 2009, 11:57 p.m. UTC | #6 
On Wed, Apr 01, 2009 at 01:59:36PM +0200, Wolfgang Grandegger wrote:
> David Gibson wrote:
> > On Wed, Apr 01, 2009 at 09:31:34AM +0200, Wolfgang Grandegger wrote:
> >> Hi David,
> >>
> >> this patch went out by mistake, sorry.
> >>
> >> David Gibson wrote:
> >>> On Tue, Mar 31, 2009 at 02:37:33PM +0200, Wolfgang Grandegger wrote:
> >>>> Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
> >>>> ===================================================================
> >>>> --- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
> >>>> +++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
> >>>> @@ -52,6 +52,7 @@
> >>>>  	soc8544@e0000000 {
> >>>>  		#address-cells = <1>;
> >>>>  		#size-cells = <1>;
> >>>> +		device_type = "soc";
> >>> You should not be adding device_type to a new board's dts.  You
> >>> *especially* shouldn't be doing that with no patch comment explaining
> >>> why.
> >> In the final patch I have removed the device_type = "soc", but it makes
> >> trouble as some mpc85xx rely on it.
> > 
> > Um.. what?  Some mpc85xx?  This is the dts for *one specific board*, I
> > don't see how something that affects "some mpc85xx" is relevant.
> 
> Well, to be more precise, it affects all boards using CONFIG_FSL_SOC
> because they use functions searching for the device type "soc", e.g.
> fsl_get_sys_freq():
> 
> http://lxr.linux.no/linux+v2.6.29/arch/powerpc/sysdev/fsl_soc.c#L80
> 
> This needs to be fixed before the line 'device_type = "soc"' can be removed.

Gah.  I had been under the impression that had been fixed already.
Kumar!  Get your house in order!

Ok, I guess it will have to stay for now, just as long as the new
style marker in compatible is there as well.
David Gibson April 1, 2009, 11:58 p.m. UTC | #7 
On Wed, Apr 01, 2009 at 04:30:29PM +0400, Anton Vorontsov wrote:
> On Wed, Apr 01, 2009 at 10:09:55PM +1100, David Gibson wrote:
> > On Wed, Apr 01, 2009 at 09:31:34AM +0200, Wolfgang Grandegger wrote:
> > > Hi David,
> > > 
> > > this patch went out by mistake, sorry.
> > > 
> > > David Gibson wrote:
> > > > On Tue, Mar 31, 2009 at 02:37:33PM +0200, Wolfgang Grandegger wrote:
> > > >> Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
> > > >> ===================================================================
> > > >> --- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
> > > >> +++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
> > > >> @@ -52,6 +52,7 @@
> > > >>  	soc8544@e0000000 {
> > > >>  		#address-cells = <1>;
> > > >>  		#size-cells = <1>;
> > > >> +		device_type = "soc";
> > > > 
> > > > You should not be adding device_type to a new board's dts.  You
> > > > *especially* shouldn't be doing that with no patch comment explaining
> > > > why.
> > > 
> > > In the final patch I have removed the device_type = "soc", but it makes
> > > trouble as some mpc85xx rely on it.
> > 
> > Um.. what?  Some mpc85xx?  This is the dts for *one specific board*, I
> > don't see how something that affects "some mpc85xx" is relevant.
> 
> Not some, but *all* mpc85xx. The thing is that U-Boot is searching
> for device_type = "soc" to fixup some properties (for example,
> bus-frequency) for all mpc85xx parts, so you can't easily remove
> the device_type w/o breaking boot for firmwares that customers have
> pre-programmed in their products.

I thought that was fixed in current u-boot versions.

> Note that upgrading firmware without JTAG is dangerous, so it's not
> an option to "force" everybody to upgrade their firmware just
> because we want to remove some cruft from the device tree.

I realise that, but again this is a *new board*, which presumably can
use an up-to-date u-boot version.
diff mbox

Patch

Index: linux-2.6/arch/powerpc/boot/dts/socrates.dts
===================================================================
--- linux-2.6.orig/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:24:38.078719288 +0200
+++ linux-2.6/arch/powerpc/boot/dts/socrates.dts	2009-03-31 13:47:56.986720719 +0200
@@ -52,6 +52,7 @@ 
 	soc8544@e0000000 {
 		#address-cells = <1>;
 		#size-cells = <1>;
+		device_type = "soc";
 
 		ranges = <0x00000000 0xe0000000 0x00100000>;
 		reg = <0xe0000000 0x00001000>;	// CCSRBAR 1M
@@ -78,10 +79,11 @@ 
 			#address-cells = <1>;
 			#size-cells = <0>;
 			cell-index = <0>;
-			compatible = "fsl-i2c";
+			compatible = "fsl,mpc8544-i2c", "fsl-i2c";
 			reg = <0x3000 0x100>;
 			interrupts = <43 2>;
 			interrupt-parent = <&mpic>;
+			clock-frequency = <103000>;
 			dfsrr;
 
 			dtt@28 {
@@ -110,10 +112,12 @@ 
 			#address-cells = <1>;
 			#size-cells = <0>;
 			cell-index = <1>;
-			compatible = "fsl-i2c";
+			compatible = "fsl,mpc8544-i2c", "fsl-i2c";
 			reg = <0x3100 0x100>;
 			interrupts = <43 2>;
 			interrupt-parent = <&mpic>;
+			fsl,preserve-clocking;
+			clock-frequency = <400000>;
 			dfsrr;
 		};
 
@@ -207,6 +211,7 @@ 
 			compatible = "fsl,mpc8548-guts";
 			reg = <0xe0000 0x1000>;
 			fsl,has-rstcr;
+			fsl,has-sec-cfg;
 		};
 
 		mpic: pic@40000 {