Re: [PATCH] Add MPC5200B base board mvBC-P

["André Schwarz" <Andre.Schwarz@matrix-vision.de>]

[-- Attachment #1: Type: text/plain, Size: 6908 bytes --]

Grant,

thanks for the quick feedback - I'll try to improve.
Hopefully monday morning will be on time ....

Comments inline.

regards,
André

Grant Likely wrote:
> On Fri, Jul 04, 2008 at 06:35:39PM +0200, Andre Schwarz wrote:
>   
>> The mvBlueCOUGAR-P is a MPC5200B based camera system with Intel Gigabit ethernet
>> controller (using e1000). It's just another MPC5200_simple board.
>>
>> Signed-off-by: Andre Schwarz <andre.schwarz@matrix-vision.de>
>> ---
>>
>>
>> Grant,
>>
>> I don't know if there are any merge windows ...
>> If the patch should be modified or re-submitted on a later time please let me know.
>>     
>
> The merge window will be opening any day now.  If you address comments
> quickly then I should be able to merge it into 2.6.27
>
>   
great.
>>  arch/powerpc/boot/dts/mvbc-p.dts             |  206 +++++
>>  arch/powerpc/configs/mvbc-p_defconfig        | 1158 ++++++++++++++++++++++++++
>>     
> Rename this to arch/powerpc/config/52xx/mvbc_p_defconfig (use platform
> specific defconfig dir and don't mix '-' and '_' in filenames).
>
>   
ok - no problem.
>> diff --git a/arch/powerpc/boot/dts/mvbc-p.dts b/arch/powerpc/boot/dts/mvbc-p.dts
>> new file mode 100644
>> index 0000000..90a2808
>> --- /dev/null
>> +++ b/arch/powerpc/boot/dts/mvbc-p.dts
>> @@ -0,0 +1,206 @@
>> +/*
>> + * mvBlueCOUGAR-P device tree source
>> + *
>> + * Copyright (C) 2008 Matrix Vision GmbH
>> + * Andre Schwarz <andre.schwarz@matrix-vision.de>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +/ {
>> +	model = "matrix-vision,mvbc-p";
>> +	compatible = "matrix-vision,mvbc-p";
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		PowerPC,5200@0 {
>> +			device_type = "cpu";
>> +			reg = <0>;
>> +			d-cache-line-size = <32>;
>> +			i-cache-line-size = <32>;
>> +			d-cache-size = <0x4000>;
>> +			i-cache-size = <0x4000>;
>> +			timebase-frequency = <0>;
>> +			bus-frequency = <0>;
>> +			clock-frequency = <0>;
>> +		};
>> +	};
>> +
>> +	memory {
>> +		device_type = "memory";
>> +		reg = <0x00000000 0x00000000>;
>> +	};
>> +
>> +	soc5200@f0000000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "fsl,mpc5200-immr";
>>     
>
> Does this board use the original 5200, or the 5200B?  If it uses the
> 5200B, then you should specify both fsl,mpc5200b-immr and
> fsl,mpc5200-immr.  Same goes for all other compatible properties in the
> tree; see lite5200b.dts for an example.
>
> I am toying with the option of eliminating the need for fsl,mpc5200b-*,
> but until then the conservative and safest thing to do is to claim
> compatibility with both.
>
>   
It's a MPC5200B. I thought that the "b"-Option has already out of use 
after reading about this a few weeks ago ... maybe I misinterpreted.
I'll fix this.
>> +	lpb {
>> +		compatible = "fsl,lpb";
>>     
>
> You should also claim compatibility with "simple-bus" here.
>
> ie:  compatible = "fsl,lpb", "simple-bus";
>
>   
ok.
>> +		#address-cells = <2>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x0 0xff800000 0x00800000>;
>> +		flash@0,0 {
>> +			compatible = "cfi-flash";
>>     
>
> For completeness, it is good practice for the first entry in the compatible
> list to be the actual flash chip, followed by "cfi-flash"
>
>   
ok.
>> +			reg = <0 0 0x800000>;
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			bank-width = <1>;
>> +			device-width = <1>;
>> +			nor_total@0x0 {
>> +				reg = <0x0 0x800000>;
>> +			};
>>     
>
> I don't know if this is legal; to have overlapping flash sections (but
> I'm not a cfi-flash binding expert).
>
>   
Hopefully this is still possible. Nested mtd partitions have always been 
possible, e.g. embedded environment inside u-boot partition.
It's proven very useful to have access to the whole chip - so it's 
possible to make binary updates even with the partition layout changing ....
I'd really like to stick to it if you don't mind.
>> +			u-boot@0x0 {
>> +				reg = <0x0 0x40000>;
>> +			};
>> +			u-boot_autoscript@0x40000 {
>> +				reg = <0x40000 0x10000>;
>> +			};
>> +			u-boot_autoscript_red@0x50000 {
>> +				reg = <0x50000 0x10000>;
>> +			};
>> +			fpga@0x60000 {
>> +				reg = <0x60000 0x40000>;
>> +			};
>> +			user@0xa0000 {
>> +				reg = <0xa00000 0x60000>;
>> +			};
>> +			rfs@0x100000 {
>> +				reg = <0x100000 0x300000>;
>> +			};
>> +			kernel@0x400000 {
>> +				reg = <0x400000 0x3c0000>;
>> +			};
>> +			dtb@0x7c0000 {
>> +				reg = <0x7c0000 0x10000>;
>> +			};
>> +			dtb@0x7d0000 {
>> +				reg = <0x7d0000 0x10000>;
>> +			};
>> +			ppcboot_env@0x7e0000 {
>> +				reg = <0x7e0000 0x10000>;
>> +			};
>> +			ppcboot_env@0x7f0000 {
>> +				reg = <0x7f0000 0x10000>;
>> +			};
>>     
>
> I think it would be better to just leave out the partition information
> and modify U-Boot to fill them in (just like memory and clock speed are
> left out).  Things like flash partitions are less like hardware
> description and more like configuration data.
>
>   
never did this. Is it quick'n'easy ?
Honestly I don't like the bootloader to set up everything.
If you need any change it will require a bootloader update which is a 
very fragile operation out in the field.
There will always be bricked systems afterwards ....
Failure in updating the (redundant) dtb blob or kernel will do almost 
always no harm since the system is still accessible and flashable using 
ethernet or serial.

If it's not against all rule (which I don't think) I'd really like to 
stick to it, too.
Is this ok ?
>> +		};
>> +	};
>> +
>> +	pci: pci@0xf0000d00 {
>> +		#interrupt-cells = <1>;
>> +		#size-cells = <2>;
>> +		#address-cells = <3>;
>> +		device_type = "pci";
>> +		compatible = "fsl,mpc5200-pci";
>> +		reg = <0xf0000d00 0x100>;
>> +		interrupt-map-mask = <0xf800 0 0 7>;
>> +		interrupt-map = <0x5800 0 0 1 &mpc5200_pic 1 2 3
>> +			0x5000 0 0 1 &mpc5200_pic 1 3 3>;
>> +		clock-frequency = <0>;
>> +		interrupts = <2 8 0 2 9 0 2 10 0>;
>> +		interrupt-parent = <&mpc5200_pic>;
>> +		bus-range = <0 0>;
>> +		ranges = <0x42000000 0 0x80000000 0x80000000 0 0x20000000
>> +			0x02000000 0 0xa0000000 0xa0000000 0 0x10000000
>> +			0x01000000 0 0x00000000 0xb0000000 0 0x01000000>;
>> +	};
>> +};
>>     



MATRIX VISION GmbH, Talstraße 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschäftsführer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[-- Attachment #2: Type: text/html, Size: 8586 bytes --]