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

[Timur Tabi <timur@freescale.com>]

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
> 


-- 
Timur Tabi
Linux kernel developer at Freescale