Re: [PATCH] powerpc/p5040ds: Add support for P5040DS board

[Scott Wood <scottwood@freescale.com>]

On 07/24/2012 01:09 PM, Timur Tabi wrote:
> Scott Wood wrote:
>> On 07/24/2012 11:42 AM, Timur Tabi wrote:
>>> +/* controller at 0x200000 */
>>> +&pci0 {
>>> +	compatible = "fsl,p5040-pcie", "fsl,qoriq-pcie-v2.2";
>>
>> p5040 has PCIe v2.4.
> 
> Then it's broken on the SDK as well.

Yes.  There was internal discussion about this over the last few days.

>> Note that there is a version register, so perhaps we should drop the
>> version number from the compatible (and mention the version register in
>> the binding).
>>
>> Might want to double check the other version numbers in this file too.
>>
>>> +	bus-range = <0x0 0xff>;
>>
>> Do we really need this?
>>
>>> +	clock-frequency = <33333333>;
>>
>> I doubt this is accurate.
> 
> Almost all of this is copy-paste from the P5020, so if it's broken here,
> it's either broken on the P5020 or also broken on the SDK.

Now's as good a time as any to fix it.

>>> +/dts-v1/;
>>> +/ {
>>> +	compatible = "fsl,P5040";
>>
>> When would we not override this?
> 
> I don't understand.

I was wondering why we put these chip-based toplevel compatibles in the
dtsi, when we'll always overwrite it with a board-based toplevel compatible.

>>> +		spi@110000 {
>>> +			flash@0 {
>>> +				#address-cells = <1>;
>>> +				#size-cells = <1>;
>>> +				compatible = "spansion,s25sl12801";
>>> +				reg = <0>;
>>> +				spi-max-frequency = <40000000>; /* input clock */
>>> +				partition@u-boot {
>>> +					label = "u-boot";
>>> +					reg = <0x00000000 0x00100000>;
>>> +					read-only;
>>> +				};
>>> +				partition@kernel {
>>> +					label = "kernel";
>>> +					reg = <0x00100000 0x00500000>;
>>> +					read-only;
>>> +				};
>>> +				partition@dtb {
>>> +					label = "dtb";
>>> +					reg = <0x00600000 0x00100000>;
>>> +					read-only;
>>> +				};
>>> +				partition@fs {
>>> +					label = "file system";
>>> +					reg = <0x00700000 0x00900000>;
>>> +				};
>>
>> Why are kernel/dtb read only?
> 
> Because that's how it is on the P5020!

This is a copy-and-paste meme that I've probably complained about a few
dozen times by now. :-)

>>> +#ifdef CONFIG_SMP
>>> +		/*
>>> +		 * Disable the timebase sync operations because we can't write
>>> +		 * to the timebase registers under the hypervisor.
>>> +		  */
>>> +		smp_85xx_ops.give_timebase = NULL;
>>> +		smp_85xx_ops.take_timebase = NULL;
>>> +#endif
>>
>> Why are they getting set in the first place?
> 
> This is how the structure is defined in smp.c:
> 
> struct smp_ops_t smp_85xx_ops = {
> 	.kick_cpu = smp_85xx_kick_cpu,
> #ifdef CONFIG_KEXEC
> 	.give_timebase	= smp_generic_give_timebase,
> 	.take_timebase	= smp_generic_take_timebase,
> #endif
> };
> 
> This code has not changed in years.

There was a patch to fix this, but I guess it hasn't been merged yet.

> I'm not sure what you think is wrong
> with it.

We should never be using smp_generic_take/give_timebase.  We have a
better way of synchronizing for the few cases where we need to.

-Scott