-----Original Message----- From: Sebastian Hesselbarth [mailto:sebastian.hesselbarth@gmail.com] Sent: 03 March 2013 17:01 To: Prafulla Wadaskar Cc: u-boot@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);
elseorion_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