[PATCH RFC] powerpc/mpc85xx: add support for the kmp204x reference board
Valentin Longchamp
valentin.longchamp at keymile.com
Fri Jan 17 23:51:10 EST 2014
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 <valentin.longchamp at keymile.com>
>> ---
>> 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, valentin.longchamp at keymile.com
>> + *
>> + * 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 at ffe000000 {
>> + ranges = <0x00000000 0xf 0xfe000000 0x1000000>;
>> + reg = <0xf 0xfe000000 0 0x00001000>;
>> + spi at 110000 {
>> + flash at 0 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "spansion,s25fl256s1";
>> + reg = <0>;
>> + spi-max-frequency = <20000000>; /* input clock */
>> + partition at u-boot {
>> + label = "u-boot";
>> + reg = <0x00000000 0x00100000>;
>> + read-only;
>> + };
>> + partition at env {
>> + label = "env";
>> + reg = <0x00100000 0x00010000>;
>> + };
>> + partition at envred {
>> + label = "envred";
>> + reg = <0x00110000 0x00010000>;
>> + };
>> + partition at 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 at 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 at 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 at 0,0 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "fsl,elbc-fcm-nand";
>> + reg = <0 0 0x40000>;
>> +
>> + partition at 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
More information about the Linuxppc-dev
mailing list