[U-Boot] [PATCH v2 01/10] ARM: dove: add support for Marvell Dove SoC

[Sebastian Hesselbarth]

Prafulla,

thanks for the review. I added some notes below at your comments.

On 02/11/2013 04:39 AM, Prafulla Wadaskar wrote:
>>
>> [...]
>>
>> diff --git a/arch/arm/cpu/armv7/dove/mpp.c
>> b/arch/arm/cpu/armv7/dove/mpp.c
>> new file mode 100644
>> index 0000000..ed24b38
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/dove/mpp.c
>> @@ -0,0 +1,318 @@
>>
>> [...]
>>
>> +/*
>> + * MPP0-23 have standard MPP register layout
>> + */
>> +static void dove_mpp_std_set(u16 config)
>> +{
>> +     u8 num = MPP_NUM(config);
>> +     u32 off = (num / MPPS_PER_REG) * MPP_BITS;
>> +     u32 shift = (num % MPPS_PER_REG) * MPP_BITS;
>> +     u32 reg;
>> +
>> +     /* configure standard MPP pin */
>> +     reg  = readl(MPP_CTRL(off));
>> +     reg&= ~(MPP_MASK<<  shift);
>> +     reg |= MPP_SEL(config)<<  shift;
>> +     writel(reg, MPP_CTRL(off));
>> +
>> +     /* configure gpio capabilities */
>> +     if (MPP_GPIO(config))
>> +             orion_gpio_set_valid(num, GPIO_INPUT_OK | GPIO_OUTPUT_OK);
>> +     else
>> +             orion_gpio_set_valid(num, 0);
>
> Why it is orion_gpio*? it should be generic API call or SoC specific.

Dove is reusing orion gpio, orion refers to the SoC family not orion5x.

>> +}
>> +
>> +/*
>> + * MPP0-15 also allow to mux PMU functions
>> + */
>> +static void dove_mpp_pmu_set(u16 config)
>> +{
>> +     u8 num = MPP_NUM(config);
>> +
>> +     if (MPP_SEL(config) == PMU) {
>> +             /* enable PMU on MPP */
>> +             writel(readl(MPP_PMU_GENERAL_CTRL) | (1<<  num),
>> +                    MPP_PMU_GENERAL_CTRL);
>> +             /* disable gpio capabilities */
>> +             orion_gpio_set_valid(num, 0);
>
> I think you are trying to reuse the framework implemented by orion,
> You may move generic part from orion to common area so that you can use it. Using other SoC direct calls doesn't sound good to me.

Kirkwood and others using orion_gpio have very regular layout of mpp pins and
gpio functionality, i.e. you have one pin with one mpp layout and if you want
it to be gpio you choose "gpio" as it's mpp function.

Well, Dove is different here. You first have mpp0-15 which can be either
"normal" mpp pins _or_ assigned to power management unit (pmu). Then there
are mpp groups where more than one pin is controlled by a single mpp register
value. As there are some groups that have some of the pins configured as gpios
while others carry a special function, I chose to have gpio capabilities
configured by mpp code not by gpio function.

>> [...]
>>
>> diff --git a/arch/arm/include/asm/arch-dove/config.h
>> b/arch/arm/include/asm/arch-dove/config.h
>> new file mode 100644
>> index 0000000..2d94a48
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-dove/config.h
>> @@ -0,0 +1,153 @@
>>
>> [...]
>>
>> +/*
>> + * By default kwbimage.cfg from board specific folder is used
>
> I think you should use dvbimage.cfg naming convention, since kwb stands for Kirkwood boot image, same way it will be dove boot image

With recent discussion about renaming kwboot to mvboot, shouldn't this
become mvbimage.cfg?

>> + * If for some board, different configuration file need to be used,
>> + * CONFIG_SYS_KWD_CONFIG should be defined in board specific header
>> file
>> + */
>> +#ifndef CONFIG_SYS_KWD_CONFIG
>> +#define      CONFIG_SYS_KWD_CONFIG
>>        $(SRCTREE)/$(CONFIG_BOARDDIR)/kwbimage.cfg
>> +#endif /* CONFIG_SYS_KWD_CONFIG */
>
> Same: change all references to DOVE, secondly do you think DV is better than DOVE to shorten then name everywhere like KW for Kirkwood?

Ditto, rename KWD to MV or MVB?

>> [...]
>> +/*
>> + * SPI Flash configuration
>> + */
>> +#ifdef CONFIG_CMD_SF
>> +#define CONFIG_HARD_SPI                      1
>> +#define CONFIG_ORION_SPI             1
>> +#define ORION_SPI_BASE                       DOVE_SPI_BASE
>
> ???

Again, Orion refers to SoC family and Dove is reusing the driver.

Sebastian