Subject: Re: [PATCH RFC] powerpc/mpc85xx: add support for the kmp204x reference board

From: Valentin Longchamp <[email-masked]>
Date: Fri, 17 Jan 2014 13:51:10 +0100

Hi Scott,

Thanks for you feedback.

On 01/17/2014 12:35 AM, Scott Wood wrote:
> On Thu, 2014-01-16 at 14:38 +0100, Valentin Longchamp wrote:
>> This patch introduces the support for Keymile's kmp204x reference
>> design. This design is based on Freescale's P2040/P2041 SoC.
>>
>> The peripherals used by this design are:
>> - SPI NOR Flash as bootloader medium
>> - NAND Flash with a ubi partition
>> - 2 PCIe busses (hosts 1 and 3)
>> - 3 FMAN Ethernet devices (FMAN1 DTSEC1/2/5)
>> - 4 Local Bus windows, with one dedicated to the QRIO reset/power mgmt
>>   FPGA
>> - 2 HW I2C busses
>> - last but not least, the mandatory serial port
>>
>> The patch also adds a defconfig file for this reference design and a DTS
>> file for the kmcoge4 board which is the first one based on this
>> reference design.
>>
>> To try to avoid code duplication, the support was added directly to the
>> corenet_generic.c file.
>>
>> Signed-off-by: Valentin Longchamp <[email-masked]>
>> ---
>>  arch/powerpc/boot/dts/kmcoge4.dts             | 165 ++++++++++++++++++
>>  arch/powerpc/configs/85xx/kmp204x_defconfig   | 231 ++++++++++++++++++++++++++
>>  arch/powerpc/platforms/85xx/Kconfig           |  14 ++
>>  arch/powerpc/platforms/85xx/Makefile          |   1 +
>>  arch/powerpc/platforms/85xx/corenet_generic.c |  52 ++++++
>>  5 files changed, 463 insertions(+)
>>  create mode 100644 arch/powerpc/boot/dts/kmcoge4.dts
>>  create mode 100644 arch/powerpc/configs/85xx/kmp204x_defconfig
>>
>> diff --git a/arch/powerpc/boot/dts/kmcoge4.dts b/arch/powerpc/boot/dts/kmcoge4.dts
>> new file mode 100644
>> index 0000000..c10df6d
>> --- /dev/null
>> +++ b/arch/powerpc/boot/dts/kmcoge4.dts
>> @@ -0,0 +1,165 @@
>> +/*
>> + * Keymile kmcoge4 Device Tree Source, based on the P2041RDB DTS
>> + *
>> + * (C) Copyright 2014
>> + * Valentin Longchamp, Keymile AG, [email-masked]
>> + *
>> + * Copyright 2011 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/ "fsl/p2041si-pre.dtsi"
>> +
>> +/ {
>> +	model = "keymile,kmcoge4";
>> +	compatible = "keymile,kmp204x";
> 
> Don't put wildcards in compatible.

Well it's a wildcard in the sense that we support both the p2040 and the p2041,
but it's also the name of the plaftorm, similarly to names like '85xx' or 'tqm85xx'.

> 
>> +	soc: soc@ffe000000 {
>> +		ranges = <0x00000000 0xf 0xfe000000 0x1000000>;
>> +		reg = <0xf 0xfe000000 0 0x00001000>;
>> +		spi@110000 {
>> +			flash@0 {
>> +				#address-cells = <1>;
>> +				#size-cells = <1>;
>> +				compatible = "spansion,s25fl256s1";
>> +				reg = <0>;
>> +				spi-max-frequency = <20000000>; /* input clock */
>> +				partition@u-boot {
>> +					label = "u-boot";
>> +					reg = <0x00000000 0x00100000>;
>> +					read-only;
>> +				};
>> +				partition@env {
>> +					label = "env";
>> +					reg = <0x00100000 0x00010000>;
>> +				};
>> +				partition@envred {
>> +					label = "envred";
>> +					reg = <0x00110000 0x00010000>;
>> +				};
>> +				partition@fman {
>> +					label = "fman-ucode";
>> +					reg = <0x00120000 0x00010000>;
>> +					read-only;
>> +				};
>> +			};
> 
> I realize it's common practice, but it would be good to get away from
> putting partition layouts in the dts file.  Alternatives include using
> mtdparts on the command line, or having U-Boot put the partition info
> into the dtb based on the mtdparts environment variable (there is
> existing code for this).

I agree that u-boot also has to know about the addresses because it also
accesses these partitions.

But I think it is clearer to have this in the device tree: I try to keep the
kernel command line small and I don't like having u-boot "fixing" the dtb at
runtime.

> 
>> +			zl30343@1 {
>> +				compatible = "gen,spidev";
> 
> Node names are supposed to be generic.  Compatibles are supposed to be
> specific.

That's a very specific device for which we only have a userspace driver and for
which we must use the generic kernel spidev driver. That's why the node name is
so specific and the compatible field very generic.

> 
>> +	lbc: localbus@ffe124000 {
>> +		reg = <0xf 0xfe124000 0 0x1000>;
>> +		ranges = <0 0 0xf 0xffa00000 0x00040000		/* LB 0 */
>> +			  1 0 0xf 0xfb000000 0x00010000		/* LB 1 */
>> +			  2 0 0xf 0xd0000000 0x10000000		/* LB 2 */
>> +			  3 0 0xf 0xe0000000 0x10000000>;	/* LB 3 */
>> +
>> +		nand@0,0 {
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			compatible = "fsl,elbc-fcm-nand";
>> +			reg = <0 0 0x40000>;
>> +
>> +			partition@0 {
>> +				label = "ubi0";
>> +				reg = <0x0 0x8000000>;
>> +			};
>> +		};
>> +	};
> 
> No nodes for those other chipselects?

Well, there are nodes, but they are internally developed FPGAs and the drivers
are not mainlined that's why I removed the nodes.

> 
>> diff --git a/arch/powerpc/configs/85xx/kmp204x_defconfig b/arch/powerpc/configs/85xx/kmp204x_defconfig
>> new file mode 100644
>> index 0000000..3bbf4fa
>> --- /dev/null
>> +++ b/arch/powerpc/configs/85xx/kmp204x_defconfig
> 
> Why does this board need its own defconfig?

Apart from the different drivers and FS that we use (or don't use) on the
system, the most notable differences are:
- lowmem must be set to a bigger size so that we can ioremap the the total
memory requested for all of our PCIe devices
- CGROUPS is enabled because that's a mandatory feature for our systems
- NAND_ECC_BCH is enabled because it is used on all of our NAND devices

> 
>> diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
>> index fbd871e..8e84e1c 100644
>> --- a/arch/powerpc/platforms/85xx/corenet_generic.c
>> +++ b/arch/powerpc/platforms/85xx/corenet_generic.c
>> @@ -122,6 +122,7 @@ static const char * const hv_boards[] __initconst = {
>>  	NULL
>>  };
>>  
>> +#ifdef CONFIG_CORENET_GENERIC
> 
> corenet_generic.c without CONFIG_CORENET_GENERIC?

It would be possible with the CONFIG_KMP204X introduced with the patch. But this
is linked with the below discussion.

> 
>>  /*
>>   * Called very early, device-tree isn't unflattened
>>   */
>> @@ -180,3 +181,54 @@ machine_arch_initcall(corenet_generic, corenet_gen_publish_devices);
>>  #ifdef CONFIG_SWIOTLB
>>  machine_arch_initcall(corenet_generic, swiotlb_setup_bus_notifier);
>>  #endif
>> +#endif
>> +
>> +#ifdef CONFIG_KMP204X
>> +/*
>> + * Called very early, device-tree isn't unflattened
>> + */
>> +static int __init kmp204x_generic_probe(void)
>> +{
>> +	unsigned long root = of_get_flat_dt_root();
>> +
>> +	return of_flat_dt_is_compatible(root, "keymile,kmp204x");
>> +}
>> +
>> +
>> +/*
>> + * Setup the architecture
>> + */
>> +void __init kmp204x_gen_setup_arch(void)
>> +{
>> +	mpc85xx_smp_init();
>> +
>> +	swiotlb_detect_4g();
>> +
>> +	pr_info("%s platform from Keymile\n", ppc_md.name);
>> +}
>> +
>> +define_machine(kmp204x) {
>> +	.name			= "kmp204x",
>> +	.probe			= kmp204x_generic_probe,
>> +	.setup_arch		= kmp204x_gen_setup_arch,
>> +	.init_IRQ		= corenet_gen_pic_init,
>> +#ifdef CONFIG_PCI
>> +	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
>> +#endif
>> +	.get_irq		= mpic_get_coreint_irq,
>> +	.restart		= fsl_rstcr_restart,
>> +	.calibrate_decr		= generic_calibrate_decr,
>> +	.progress		= udbg_progress,
>> +#ifdef CONFIG_PPC64
>> +	.power_save		= book3e_idle,
>> +#else
>> +	.power_save		= e500_idle,
>> +#endif
>> +};
>> +
>> +machine_arch_initcall(kmp204x, corenet_gen_publish_devices);
>> +
>> +#ifdef CONFIG_SWIOTLB
>> +machine_arch_initcall(kmp204x, swiotlb_setup_bus_notifier);
>> +#endif
>> +#endif
> 
> The whole point of corenet_generic.c is to avoid duplicating all of this
> stuff.
> 
> Can't you just use corenet_generic as-is other than adding the
> compatible to boards[]?  If not, explain why and put it in a different
> file.
> 

That's a valid point and I have to admit I have hesitated about that. I have
mostly based my work on the FSL SDK where every single board has a "dedicated" file.

I agree that I do nothing different than the corenet_generic does and adding my
platform to the boards[] would be the same and you are right, I should use that
and avoid code duplication.

The only thing that would "bother" me is thus the pr_info print from
*_gen_setup_arch(), it would be nice if somehow we could differentiate it or at
least make it more generic since the kmp204x boards are not strictly boards from
Freescale.

Best regards

Valentin