powerpc/p5040ds: Add support for P5040DS board

Add support for the Freescale P5040DS Reference Board ("Superhydra"), which
is similar to the P5020DS.  Features of the P5040 are listed below, but
not all of these features (e.g. DPAA networking) are currently supported.

Four P5040 single-threaded e5500 cores built
    Up to 2.4 GHz with 64-bit ISA support
    Three levels of instruction: user, supervisor, hypervisor
CoreNet platform cache (CPC)
    2.0 MB configures as dual 1 MB blocks hierarchical interconnect fabric
Two 64-bit DDR3/3L SDRAM memory controllers with ECC and interleaving support
    Up to 1600MT/s
    Memory pre-fetch engine
DPAA incorporating acceleration for the following functions
    Packet parsing, classification, and distribution (FMAN)
    Queue management for scheduling, packet sequencing and
	congestion management (QMAN)
    Hardware buffer management for buffer allocation and
	de-allocation (BMAN)
    Cryptography acceleration (SEC 5.0) at up to 40 Gbps
SerDes
    20 lanes at up to 5 Gbps
    Supports SGMII, XAUI, PCIe rev1.1/2.0, SATA
Ethernet interfaces
    Two 10 Gbps Ethernet MACs
    Ten 1 Gbps Ethernet MACs
High-speed peripheral interfaces
    Two PCI Express 2.0/3.0 controllers
Additional peripheral interfaces
    Two serial ATA (SATA 2.0) controllers
    Two high-speed USB 2.0 controllers with integrated PHY
    Enhanced secure digital host controller (SD/MMC/eMMC)
    Enhanced serial peripheral interface (eSPI)
    Two I²C controllers
    Four UARTs
    Integrated flash controller supporting NAND and NOR flash
DMA
    Dual four channel
Support for hardware virtualization and partitioning enforcement
    Extra privileged level for hypervisor support
QorIQ Trust Architecture 1.1
    Secure boot, secure debug, tamper detection, volatile key storage

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/boot/dts/fsl/p5040si-post.dtsi   |  320 +++++++++++++++++++++++++
 arch/powerpc/boot/dts/fsl/p5040si-pre.dtsi    |  111 +++++++++
 arch/powerpc/boot/dts/fsl/qoriq-sec5.2-0.dtsi |  118 +++++++++
 arch/powerpc/boot/dts/p5040ds.dts             |  207 ++++++++++++++++
 arch/powerpc/configs/corenet32_smp_defconfig  |    1 +
 arch/powerpc/configs/corenet64_smp_defconfig  |    1 +
 arch/powerpc/platforms/85xx/Kconfig           |   14 +
 arch/powerpc/platforms/85xx/Makefile          |    1 +
 arch/powerpc/platforms/85xx/p5040_ds.c        |   96 ++++++++
 9 files changed, 869 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/boot/dts/fsl/p5040si-post.dtsi
 create mode 100644 arch/powerpc/boot/dts/fsl/p5040si-pre.dtsi
 create mode 100644 arch/powerpc/boot/dts/fsl/qoriq-sec5.2-0.dtsi
 create mode 100644 arch/powerpc/boot/dts/p5040ds.dts
 create mode 100644 arch/powerpc/platforms/85xx/p5040_ds.c

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.

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.

> +	iommu@20000 {
> +		compatible = "fsl,pamu-v1.0", "fsl,pamu";
> +		reg = <0x20000 0x5000>;
> +		interrupts = <
> +			24 2 0 0
> +			16 2 1 30>;
> +	};

It's PAMU v1.1, and there's a version register.

> +/include/ "qoriq-mpic.dtsi"
> +
> +	guts: global-utilities@e0000 {
> +		compatible = "fsl,qoriq-device-config-1.0";
> +		reg = <0xe0000 0xe00>;
> +		fsl,has-rstcr;
> +		#sleep-cells = <1>;
> +		fsl,liodn-bits = <12>;
> +	};
> +
> +	pins: global-utilities@e0e00 {
> +		compatible = "fsl,qoriq-pin-control-1.0";
> +		reg = <0xe0e00 0x200>;
> +		#sleep-cells = <2>;
> +	};

Please add fsl,p5040-device-config and fsl,p5040-pin-control.  If you
want to leave the "1.0" thing in (which was a mistake since this stuff
doesn't seem to be versioned in any public way), double check that it's
100% backwards compatible with p4080.

> +	rcpm: global-utilities@e2000 {
> +		compatible = "fsl,qoriq-rcpm-1.0";
> +		reg = <0xe2000 0x1000>;
> +		#sleep-cells = <1>;
> +	};

Likewise.

> +/dts-v1/;
> +/ {
> +	compatible = "fsl,P5040";

When would we not override this?

> +		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?

> +		flash@0,0 {
> +			compatible = "cfi-flash";
> +			reg = <0 0 0x08000000>;
> +			bank-width = <2>;
> +			device-width = <2>;
> +		};

No partitions on NOR flash?

> +			partition@2000000 {
> +				label = "NAND Root File System";
> +				reg = <0x02000000 0x10000000>;
> +			};
> +
> +			partition@12000000 {
> +				label = "NAND Compressed RFS Image";
> +				reg = <0x12000000 0x08000000>;
> +			};

Why do we need both of these?  Why not one big partition for whichever
type of RFS you have?

> diff --git a/arch/powerpc/platforms/85xx/p5040_ds.c b/arch/powerpc/platforms/85xx/p5040_ds.c
> new file mode 100644
> index 0000000..ca3358f
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/p5040_ds.c
> @@ -0,0 +1,96 @@
> +/*
> + * P5040 DS Setup
> + *
> + * Copyright 2009-2010 Freescale Semiconductor Inc.
> + *
> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/kdev_t.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/phy.h>
> +
> +#include <asm/time.h>
> +#include <asm/machdep.h>
> +#include <asm/pci-bridge.h>
> +#include <mm/mmu_decl.h>
> +#include <asm/prom.h>
> +#include <asm/udbg.h>
> +#include <asm/mpic.h>
> +
> +#include <linux/of_platform.h>
> +#include <sysdev/fsl_soc.h>
> +#include <sysdev/fsl_pci.h>
> +#include <asm/ehv_pic.h>
> +
> +#include "corenet_ds.h"

Do you really need all these?  kdev_t?  phy?

> +
> +/*
> + * Called very early, device-tree isn't unflattened
> + */
> +static int __init p5040_ds_probe(void)
> +{
> +	unsigned long root = of_get_flat_dt_root();
> +#ifdef CONFIG_SMP
> +	extern struct smp_ops_t smp_85xx_ops;
> +#endif
> +
> +	if (of_flat_dt_is_compatible(root, "fsl,P5040DS"))
> +		return 1;
> +
> +	/* Check if we're running under the Freescale hypervisor */
> +	if (of_flat_dt_is_compatible(root, "fsl,P5040DS-hv")) {
> +		ppc_md.init_IRQ = ehv_pic_init;
> +		ppc_md.get_irq = ehv_pic_get_irq;
> +		ppc_md.restart = fsl_hv_restart;
> +		ppc_md.power_off = fsl_hv_halt;
> +		ppc_md.halt = fsl_hv_halt;
> +#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?

While you're at it, you might want to look into converting corenet_ds to
the new PCI init code.

-Scott

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.

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

>> +	iommu@20000 {
>> +		compatible = "fsl,pamu-v1.0", "fsl,pamu";
>> +		reg = <0x20000 0x5000>;
>> +		interrupts = <
>> +			24 2 0 0
>> +			16 2 1 30>;
>> +	};
> 
> It's PAMU v1.1, and there's a version register.

Also broken in the SDK. :-(

>> +/include/ "qoriq-mpic.dtsi"
>> +
>> +	guts: global-utilities@e0000 {
>> +		compatible = "fsl,qoriq-device-config-1.0";
>> +		reg = <0xe0000 0xe00>;
>> +		fsl,has-rstcr;
>> +		#sleep-cells = <1>;
>> +		fsl,liodn-bits = <12>;
>> +	};
>> +
>> +	pins: global-utilities@e0e00 {
>> +		compatible = "fsl,qoriq-pin-control-1.0";
>> +		reg = <0xe0e00 0x200>;
>> +		#sleep-cells = <2>;
>> +	};
> 
> Please add fsl,p5040-device-config and fsl,p5040-pin-control.  If you
> want to leave the "1.0" thing in (which was a mistake since this stuff
> doesn't seem to be versioned in any public way), double check that it's
> 100% backwards compatible with p4080.

Ok.

>> +	rcpm: global-utilities@e2000 {
>> +		compatible = "fsl,qoriq-rcpm-1.0";
>> +		reg = <0xe2000 0x1000>;
>> +		#sleep-cells = <1>;
>> +	};
> 
> Likewise.
> 
>> +/dts-v1/;
>> +/ {
>> +	compatible = "fsl,P5040";
> 
> When would we not override this?

I don't understand.

>> +		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!

I'm surprised the P5020 DTS files are so broken.

>> +		flash@0,0 {
>> +			compatible = "cfi-flash";
>> +			reg = <0 0 0x08000000>;
>> +			bank-width = <2>;
>> +			device-width = <2>;
>> +		};
> 
> No partitions on NOR flash?

I'll check.

>> +			partition@2000000 {
>> +				label = "NAND Root File System";
>> +				reg = <0x02000000 0x10000000>;
>> +			};
>> +
>> +			partition@12000000 {
>> +				label = "NAND Compressed RFS Image";
>> +				reg = <0x12000000 0x08000000>;
>> +			};
> 
> Why do we need both of these?  Why not one big partition for whichever
> type of RFS you have?

Beats me.  Like I said, I just copied them over.  I know that's bad, but
the source files have been around for quite some time, so I'm surprised
they're still all broken.

>> diff --git a/arch/powerpc/platforms/85xx/p5040_ds.c b/arch/powerpc/platforms/85xx/p5040_ds.c
>> new file mode 100644
>> index 0000000..ca3358f
>> --- /dev/null
>> +++ b/arch/powerpc/platforms/85xx/p5040_ds.c
>> @@ -0,0 +1,96 @@
>> +/*
>> + * P5040 DS Setup
>> + *
>> + * Copyright 2009-2010 Freescale Semiconductor Inc.
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/pci.h>
>> +#include <linux/kdev_t.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/phy.h>
>> +
>> +#include <asm/time.h>
>> +#include <asm/machdep.h>
>> +#include <asm/pci-bridge.h>
>> +#include <mm/mmu_decl.h>
>> +#include <asm/prom.h>
>> +#include <asm/udbg.h>
>> +#include <asm/mpic.h>
>> +
>> +#include <linux/of_platform.h>
>> +#include <sysdev/fsl_soc.h>
>> +#include <sysdev/fsl_pci.h>
>> +#include <asm/ehv_pic.h>
>> +
>> +#include "corenet_ds.h"
> 
> Do you really need all these?  kdev_t?  phy?

Probably not.

>> +
>> +/*
>> + * Called very early, device-tree isn't unflattened
>> + */
>> +static int __init p5040_ds_probe(void)
>> +{
>> +	unsigned long root = of_get_flat_dt_root();
>> +#ifdef CONFIG_SMP
>> +	extern struct smp_ops_t smp_85xx_ops;
>> +#endif
>> +
>> +	if (of_flat_dt_is_compatible(root, "fsl,P5040DS"))
>> +		return 1;
>> +
>> +	/* Check if we're running under the Freescale hypervisor */
>> +	if (of_flat_dt_is_compatible(root, "fsl,P5040DS-hv")) {
>> +		ppc_md.init_IRQ = ehv_pic_init;
>> +		ppc_md.get_irq = ehv_pic_get_irq;
>> +		ppc_md.restart = fsl_hv_restart;
>> +		ppc_md.power_off = fsl_hv_halt;
>> +		ppc_md.halt = fsl_hv_halt;
>> +#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.  I'm not sure what you think is wrong
with it.

> While you're at it, you might want to look into converting corenet_ds to
> the new PCI init code.

Well, I just want to get this out the door.

> 
> -Scott
>

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

Scott Wood wrote:

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

That's a good point, but I'm loathe to break the current convention.  I'd
rather post a patch that removes them from all boards, but I'd like an ACK
from Kumar first.

>>> 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. :-)

I know, I know, but you would think problems like this would already be
fixed upstream.  I didn't think I would need to review every single
property in the P5020 device trees.

>>>> +#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.

Can you give me a clue which patch this is, so I can find it on the
mailing list?

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

Ok, I'll match the new paradigm when I find it.

Scott Wood wrote:

>> +/include/ "qoriq-mpic.dtsi"
>> +
>> +	guts: global-utilities@e0000 {
>> +		compatible = "fsl,qoriq-device-config-1.0";
>> +		reg = <0xe0000 0xe00>;
>> +		fsl,has-rstcr;
>> +		#sleep-cells = <1>;
>> +		fsl,liodn-bits = <12>;
>> +	};
>> +
>> +	pins: global-utilities@e0e00 {
>> +		compatible = "fsl,qoriq-pin-control-1.0";
>> +		reg = <0xe0e00 0x200>;
>> +		#sleep-cells = <2>;
>> +	};
> 
> Please add fsl,p5040-device-config and fsl,p5040-pin-control.  If you
> want to leave the "1.0" thing in (which was a mistake since this stuff
> doesn't seem to be versioned in any public way), double check that it's
> 100% backwards compatible with p4080.

For "fsl,qoriq-device-config-1.0", the only difference is in the LIODN
registers, since the P4080 and the P5040 have different devices.  For
those devices that are the same, the LIODN registers are the same.

Is that compatible enough?

The same can be said for "fsl,qoriq-pin-control-1.0".  The registers are
generally the same, except when they reference devices that are different
on the SOCs.  For example, the P4080 does  not have 000E_0E84 USB Polarity
Configuration Register (DCFG_USBPCR).

>> +	rcpm: global-utilities@e2000 {
>> +		compatible = "fsl,qoriq-rcpm-1.0";
>> +		reg = <0xe2000 0x1000>;
>> +		#sleep-cells = <1>;
>> +	};

Same thing here.  Except for a few bits for devices that don't exist on
the other SOC, they're the same.

Scott Wood wrote:
>> > +		flash@0,0 {
>> > +			compatible = "cfi-flash";
>> > +			reg = <0 0 0x08000000>;
>> > +			bank-width = <2>;
>> > +			device-width = <2>;
>> > +		};
> No partitions on NOR flash?

None of the CoreNet device trees include NOR flash partitions.

On 07/24/2012 01:55 PM, Timur Tabi wrote:
> Scott Wood wrote:
> 
>>>>> +	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.
> 
> That's a good point, but I'm loathe to break the current convention.  I'd
> rather post a patch that removes them from all boards, but I'd like an ACK
> from Kumar first.

Yeah, that was more a question for Kumar and the list than a "remove
this" request.

>>>> 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. :-)
> 
> I know, I know, but you would think problems like this would already be
> fixed upstream.  I didn't think I would need to review every single
> property in the P5020 device trees.

In this particular case I should probably go fix the existing trees, but
in general blind copy and paste is a bad thing.

Maybe we should have include files for common partition schemes.

>>> 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.
> 
> Can you give me a clue which patch this is, so I can find it on the
> mailing list?

http://patchwork.ozlabs.org/patch/172243/

...but it only deals with e500v2 so far, so I was wrong when I said that
patch fixes it.  Once we do the equivalent thing for e500mc we can
remove all mpc85xx references to the generic tbsync code.

-Scott

Scott Wood wrote:
>> > +	bus-range = <0x0 0xff>;

> Do we really need this?

I can't find any documentation for this property, but it does appear to be
initialized by U-Boot:

	/* We assume a cfg_addr not being set means we didn't setup the controller */
	if ((hose == NULL) || (hose->cfg_addr == NULL)) {
		fdt_del_node(blob, off);
	} else {
		bus_range[0] = 0;
		bus_range[1] = hose->last_busno - hose->first_busno;
		fdt_setprop(blob, off, "bus-range", &bus_range[0], 2*4);
		fdt_pci_dma_ranges(blob, off, hose);
	}

So I have no idea if it's required in the DTS.

>> > +	clock-frequency = <33333333>;

> I doubt this is accurate.

Every other DTS sets this property to 33333333, and I don't see any code
in U-boot that fixes it up.

Scott Wood wrote:
>> > +	compatible = "fsl,p5040-pcie", "fsl,qoriq-pcie-v2.2";
> p5040 has PCIe v2.4.
> 
> 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.

It appears that this compatible string is used by U-Boot to find the nodes
for fixups:

#elif defined(CONFIG_PPC_P5040)
#define CONFIG_MAX_CPUS			4
...
#define CONFIG_SYS_FSL_PCIE_COMPAT	"fsl,qoriq-pcie-v2.2"

Which means that I can't just change it from v2.2 to v2.4 without breaking
U-Boot.  Ugh.

How do you know that the P5040 has v2.4?

On 07/24/2012 04:19 PM, Timur Tabi wrote:
> Scott Wood wrote:
>>>> +	compatible = "fsl,p5040-pcie", "fsl,qoriq-pcie-v2.2";
>> p5040 has PCIe v2.4.
>>
>> 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.
> 
> It appears that this compatible string is used by U-Boot to find the nodes
> for fixups:

SDK U-Boot, I assume you mean.

> #elif defined(CONFIG_PPC_P5040)
> #define CONFIG_MAX_CPUS			4
> ...
> #define CONFIG_SYS_FSL_PCIE_COMPAT	"fsl,qoriq-pcie-v2.2"
> 
> Which means that I can't just change it from v2.2 to v2.4 without breaking
> U-Boot.  Ugh.

Another reason for dropping the version from this particular compatible.
 Wouldn't stop this breakage, but would make future ones less likely.

And of course why we should carefully verify the information in the
device trees before we go copying and pasting in the first place.

Though, it depends on whether we determine that v2.4 is 100% backwards
compatible with v2.2, in which case we could list both.  It looks like
the v2.3 chips list both, and U-Boot refers to v2.2.  One possible point
of contention on compatibility is the change in where LIODN is specified
(moved from guts to the PCIe registers), especially if we're claiming
backwards comaptibility on the guts side as well.

> How do you know that the P5040 has v2.4?

See PEX_IP_BLK_REV1.

One new thing in v2.4 is the LIODN permission table, rather than one
LIODN for the entire root complex.

-Scott

Scott Wood wrote:

>> It appears that this compatible string is used by U-Boot to find the nodes
>> for fixups:
> 
> SDK U-Boot, I assume you mean.

No, upstream as well.  It's pretty much the same code:

#ifdef CONFIG_SYS_FSL_PCIE_COMPAT
#define FSL_PCIE_COMPAT	CONFIG_SYS_FSL_PCIE_COMPAT
#else
#define FSL_PCIE_COMPAT	"fsl,mpc8548-pcie"
#endif

#define FT_FSL_PCIE1_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 1)
#define FT_FSL_PCIE2_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 2)
#define FT_FSL_PCIE3_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 3)
#define FT_FSL_PCIE4_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 4)

#define __FT_FSL_PCIE_SETUP(blob, compat, num) \
	ft_fsl_pci_setup(blob, compat, CONFIG_SYS_PCIE##num##_ADDR)

On 07/24/2012 04:36 PM, Timur Tabi wrote:
> Scott Wood wrote:
> 
>>> It appears that this compatible string is used by U-Boot to find the nodes
>>> for fixups:
>>
>> SDK U-Boot, I assume you mean.
> 
> No, upstream as well.  It's pretty much the same code:

I don't see P5040 support in upstream U-Boot yet.

> #ifdef CONFIG_SYS_FSL_PCIE_COMPAT
> #define FSL_PCIE_COMPAT	CONFIG_SYS_FSL_PCIE_COMPAT
> #else
> #define FSL_PCIE_COMPAT	"fsl,mpc8548-pcie"
> #endif
> 
> #define FT_FSL_PCIE1_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 1)
> #define FT_FSL_PCIE2_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 2)
> #define FT_FSL_PCIE3_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 3)
> #define FT_FSL_PCIE4_SETUP __FT_FSL_PCIE_SETUP(blob, FSL_PCIE_COMPAT, 4)
> 
> #define __FT_FSL_PCIE_SETUP(blob, compat, num) \
> 	ft_fsl_pci_setup(blob, compat, CONFIG_SYS_PCIE##num##_ADDR)
> 

Yeah, the code's there, but not the wrong compatible string for P5040.

-Scott

Scott Wood wrote:

> Yeah, the code's there, but not the wrong compatible string for P5040.

I wasn't talking about the P5040DS specifically.

My point was that we cannot get rid of the compatible string outright
because U-Boot uses it.

On 07/24/2012 04:43 PM, Timur Tabi wrote:
> Scott Wood wrote:
> 
>> Yeah, the code's there, but not the wrong compatible string for P5040.
> 
> I wasn't talking about the P5040DS specifically.
> 
> My point was that we cannot get rid of the compatible string outright
> because U-Boot uses it.

I thought we were specifically talking about what to set the P5040 PCIe
compatible stringlist to.

-Scott

Scott Wood wrote:
> I thought we were specifically talking about what to set the P5040 PCIe
> compatible stringlist to.

What, I can't have two different conversations going on simultaneously in
one thread?!!?!?

:-)

Yes, we are talking about the P5040 compatible string, but earlier in this
thread, you said:

"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)."

and I was responding to your statement about whether we should drop the
version number.

On 07/24/2012 04:47 PM, Timur Tabi wrote:
> Scott Wood wrote:
>> I thought we were specifically talking about what to set the P5040 PCIe
>> compatible stringlist to.
> 
> What, I can't have two different conversations going on simultaneously in
> one thread?!!?!?
> 
> :-)
> 
> Yes, we are talking about the P5040 compatible string, but earlier in this
> thread, you said:
> 
> "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)."
> 
> and I was responding to your statement about whether we should drop the
> version number.

I meant for new device trees.

-Scott

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-bounces+tie-
> fei.zang=freescale.com@lists.ozlabs.org] On Behalf Of Scott Wood
> Sent: Wednesday, July 25, 2012 2:33 AM
> To: Tabi Timur-B04825
> Cc: linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH] powerpc/p5040ds: Add support for P5040DS board
> 
> 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.
In SDK, we use consist v2.2, so there is no obvious problem, but we need to fix this to v2.4. P5020 PCIe adds Liodn registers.
Roy