On 01/17/2014 10:48 PM, Scott Wood wrote:
> On Fri, 2014-01-17 at 13:51 +0100, Valentin Longchamp wrote:
>> 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'.
>
> Names like 85xx are not allowed in device trees.
>
> With "p204x", what would happen if a p2042 were introduced, that were
> not compatible?
What would you suggest as a generic name for the architecture that supports
both ?
>
> Why isn't the compatible "keymile,kmcoge4", like the model?
Because kmcoge4 is the board that is based on the kmp204x architecture/design.
We expect other boards (kmcoge7 for instance) based on the same kmp204x design.
You would prefer that I have the model and compatible stricly the same and add
any future board into the compatible boards[] from corenet_generic ?
If possible I would like to be able to see the boards that are based on a
similar design, that's what I wanted to achieve with this kmp204x name.
>
>>> 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.
>
> The problem is that the dts source is often far removed from the actual
> programming of flash, and the partitioning can vary based on use case,
> or change for other reasons (e.g. there have been requests to change
> existing partition layouts to accommodate growth in U-Boot size).
>
> Ideally it wouldn't be in the device tree at all, but having U-Boot fix
> it up based on an environment variable is better than statically
> defining it in a file in the Linux tree.
>
>>>> + 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.
>
> The device tree describes the hardware, not what driver you want to use.
>
> Plus, I don't see any driver that matches "gen,spidev" nor any binding
> for it, and "gen" doesn't make sense as a vendor prefix. The only
> instance of that string I can find in the Linux tree is in mgcoge.dts.
Well it comes from mgcoge and that's why I have used this
It's for usage with the spidev driver (driver/spi/spidev.c). I agree that the
gen brings nothing. Would
spidev@1 {
compatible = "spidev";
make more sense ?
>
>>> That's why the node name is
>>> so specific and the compatible field very generic.
>
> Userspace can't search for a node by compatible?
>
>>>> + 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.
>
> The device tree describes the hardware, not what drivers are currently
> mainlined in Linux.
What do you want me to do: add the nodes for which there are no bindings ?
I did this similarly to the situation with the FSL .dtsi that currently in
mainline do not include the DPAA/QMAN/BMAN 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,
>
> That is not generally a good reason for a separate defconfig. Just
> enable the drivers you need in the main defconfig, and don't worry about
> the drivers you don't need. You may want a smaller kernel for actual
> shipping products (though the changelog said this is a reference
> board...), but in mainline we want a small number of defconfigs that
> cover as many boards as possible (or at least, a reasonably small number
> and not one per board).
It's a reference design meaning that then all the further boards based on the
kmp204x design would reuse that defconfig. But I understand that you want to
avoid to multiply the number of defconfigs.
>
>> 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
>
> I don't think there would be a problem adding CGROUPS or NAND_ECC_BCH to
> corenet32_smp_defconfig (though CGROUPS seems more like a use-case
> configuration than something to do with the board itself). The lowmem
> adjustment is probably a good reason, though I wish things like that
> could be specified as a defconfig that #includes corenet32_smp_defconfig
> and then just makes a couple changes.
>
Yes that would be a nice feature to have: even for me, I would love to be able
to rely on corenet32_smp_defconfig, include it and just add my changes.
>>> 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.
>
> Just remove the "from Freescale Semiconductor" part of the string.
>
OK.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
