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.longch...@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.longch...@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@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
