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

[Prafulla Wadaskar]

> -----Original Message-----
> From: Sebastian Hesselbarth
> [mailto:sebastian.hesselbarth at gmail.com]
> Sent: 03 March 2013 17:01
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Rabeeh Khoury; Albert
> Aribaud; Andy Fleming; Joe Hershberger; Daniel Stodden;
> Luka Perkov
> Subject: Re: [PATCH v2 01/10] ARM: dove: add support
> for Marvell Dove SoC
> 
> 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.

So let's rename them as "mv" as per strategy.
I don't have any issue to refer "mv_gpio" in dove code.

> 
> >> +}
> >> +
> >> +/*
> >> + * 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.

So we can have generic mv specific APIs and the SoC specific layer can be abstracted if possible.
If the integration is not possible with generic code then you can always have dove_gpio driver.

> 
> >> [...]
> >>
> >> 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?

Tool could be mvboot, configuration image can be mv/kw/dvbimage.cfg. But I don't have any issue with name 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.

Let's use mv if the code is being shared.

Regards...
Prafulla . . .

> 
> Sebastian