[PATCH 2/3 v4] arm: kirkwood: add dreamplug (fdt) support.

[Grant Likely]

On Thu, Feb 23, 2012 at 9:12 AM, Jason <jason at lakedaemon.net> wrote:
> On Thu, Feb 23, 2012 at 07:34:33AM +0000, Arnd Bergmann wrote:
>> On Thursday 23 February 2012, Rob Herring wrote:
>> > On 02/22/2012 01:18 PM, Jason Cooper wrote:
>>
>> > > diff --git a/arch/arm/mach-kirkwood/Kconfig b/arch/arm/mach-kirkwood/Kconfig
>> > > index 7fc603b..6095884 100644
>> > > --- a/arch/arm/mach-kirkwood/Kconfig
>> > > +++ b/arch/arm/mach-kirkwood/Kconfig
>> > > @@ -44,6 +44,20 @@ config MACH_GURUPLUG
>> > >     Say 'Y' here if you want your kernel to support the
>> > >     Marvell GuruPlug Reference Board.
>> > >
>> > > +config ARCH_KIRKWOOD_DT
>> > > + bool "Marvell Kirkwood Flattened Device Tree"
>> > > + select USE_OF
>> > > + help
>> > > +   Say 'Y' here if you want your kernel to support the
>> > > +   Marvell Kirkwood using flattened device tree.
>> > > +
>> > > +config MACH_DREAMPLUG_DT
>> > > + bool "Marvell DreamPlug (Flattened Device Tree)"
>> > > + depends on ARCH_KIRKWOOD_DT
>> > > + help
>> > > +   Say 'Y' here if you want your kernel to support the
>> > > +   Marvell DreamPlug (Flattened Device Tree).
>> >
>> > Why do you need 2 entries?
>>
>> The first one is used to build the board file for all DT based machines,
>> the second one is used to select the dts file. I would be enough to have just
>> the first one, but that makes it harder to find for people looking for
>> dreamplug.
>>
>> Maybe change the text to "Generic Marvell Kirkwood DT based (e.g. DreamPlug)"?
>
> I was thinking about this more, and had, I hope, a better idea:  change
> the 'depends on ARCH_KIRKWOOD_DT' to 'selects ARCH_KIRKWOOD_DT' and
> remove the first entry.  This way, the logic remains the same and the
> word 'Dreamplug' is always visible.  Any objections?
>
>> > > +
>> > > +static unsigned int dreamplug_mpp_config[] __initdata = {
>> > > + MPP0_SPI_SCn,
>> > > + MPP1_SPI_MOSI,
>> > > + MPP2_SPI_SCK,
>> > > + MPP3_SPI_MISO,
>> > > + MPP47_GPIO,     /* Bluetooth LED */
>> > > + MPP48_GPIO,     /* Wifi LED */
>> > > + MPP49_GPIO,     /* Wifi AP LED */
>> > > + 0
>> > > +};
>> >
>> > Do you need this to boot? All this data should come from the dtb.
>>
>> Putting this into the dtb would require converting kirkwood to use the
>> pinctrl subsystem first, if we want to have proper bindings. When I
>> did the hands-on review of the code with Jason during ELC, we decided
>> to leave this being done the legacy way for now, which also matches
>> how Tegra does it until we have the pinctrl bindings.
>>
>> Also note how the patch description says "Driver porting will start
>> with the uart (see next patch), and progress from there.  Possibly,
>> spi/flash/partitions will be next." I think this is the best approach
>> indeed and I'd probably leave the pinctrl stuff until the end.
>
> Agreed.
>
>> > > +
>> > > +static void __init kirkwood_dt_init(void)
>> > > +{
>> > > + kirkwood_init();
>> > > +
>> > > + if (!of_machine_is_compatible("kirkwood,dreamplug"))
>> >
>> > Huh? Your string doesn't match your dts file.
>> >
>> > You've already matched against "marvell,dreamplug", so is this check
>> > necessary?
>>
>> It's wrong, and the condition is negated. It should be
>>
>>       if (of_machine_is_compatible("marvell,dreamplug"))
>>               dreamplug_init();
>>
>> The idea is that there is one init function for all dt based
>> kirkwood machines, which contains special setup functions for
>> those boards that don't (yet) describe all the hardware in the
>> dt.
>
> Good catch, will fix in v5.
>
>> > > +
>> > > +static const char *kirkwood_dt_board_compat[] = {
>> > > + "marvell,dreamplug",
>> > > + NULL
>> > > +};
>>
>> Since you mention the name, it should probably be "globalscale,dreamplug"
>> because the device is made by GlobalScale instead of Marvell.
>
> hmm, Globalscale Tech is, as I understand it, a turn-key manufacturer.
> They are simply building Marvell's development platforms for them,
> handling sales, etc.  My impression was that Marvell designed the board
> and contracted Globalscale to build and distribute it.
>
> I don't care which we use, but is the convention to use the SoC designer
> (marvell,dreamplug), the SoC (kirkwood,dreamplug), or the brand
> (globalscale,dreamplug)?
>
> If there is no set standard, I think the SoC is most accurate, as
> nothing prevents a manufacturer from swapping out ICs/SoCs between
> manufacturing runs of the same make/model.  Look at certain wifi USB
> devices for examples.

convention is to use the vendor of the device.  If it is marketed as a
Marvell product, then use "marvell,...".  If it is Globalscale, then
"globalscale,...".  The actual SoC on the device is irrelevant for the
top level compatible property name.

g.