On Wed, Feb 9, 2011 at 4:35 AM, Kevin Hilman <khilman@xxxxxx> wrote:
> Kishore Kadiyala <kishore.kadiyala@xxxxxx> writes:
>
>> Changes involves:
>> 1) Remove controller reset in devices.c which is taken care
>> by hwmod framework.
>> 2) Removing all base address macro defines.
>> 3) Using omap-device layer to register device and utilizing data from
>> hwmod data file for base address, dma channel number, Irq_number,
>> device attribute.
>> 4) Update the driver to use dev_attr to find whether controller
>> supports dual volt cards.
>> 5) Moving "omap_mmc_add" api from plat-omap/devices.c to mach-omap1/devices.c
>>
>> Signed-off-by: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx>
>
> This seems to still be largely based on work originally done by Paul
> with minor changes from me. You should mention that here and Cc him
> here as well. My contributions were minimal here, so this s-o-b can
> just be a Cc for now until I have given an acked-by.
Sure, I will take care of this in my next revision V3
>
>> Signed-off-by: Kishore Kadiyala <kishore.kadiyala@xxxxxx>
>
>
>> ---
>> arch/arm/mach-omap1/devices.c | 41 +++++++
>> arch/arm/mach-omap2/board-n8x0.c | 6 +-
>> arch/arm/mach-omap2/devices.c | 207 +++++++--------------------------
>> arch/arm/mach-omap2/hsmmc.c | 6 +-
>> arch/arm/plat-omap/devices.c | 50 --------
>> arch/arm/plat-omap/include/plat/mmc.h | 36 ++-----
>> drivers/mmc/host/omap_hsmmc.c | 4 +-
>> 7 files changed, 100 insertions(+), 250 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap1/devices.c b/arch/arm/mach-omap1/devices.c
>> index b0f4c23..eae41b6 100644
>> --- a/arch/arm/mach-omap1/devices.c
>> +++ b/arch/arm/mach-omap1/devices.c
>> @@ -72,6 +72,47 @@ static inline void omap_init_mbox(void) { }
>>
>> #if defined(CONFIG_MMC_OMAP) || defined(CONFIG_MMC_OMAP_MODULE)
>>
>> +#define OMAP_MMC_NR_RES 2
>> +int __init omap_mmc_add(const char *name, int id, unsigned long base,
>> + unsigned long size, unsigned int irq,
>> + struct omap_mmc_platform_data *data)
>> +{
>> + struct platform_device *pdev;
>> + struct resource res[OMAP_MMC_NR_RES];
>> + int ret;
>> +
>> + pdev = platform_device_alloc(name, id);
>> + if (!pdev)
>> + return -ENOMEM;
>> +
>> + memset(res, 0, OMAP_MMC_NR_RES * sizeof(struct resource));
>> + res[0].start = base;
>> + res[0].end = base + size - 1;
>> + res[0].flags = IORESOURCE_MEM;
>> + res[1].start = irq;
>> + res[1].end = irq;
>> + res[1].flags = IORESOURCE_IRQ;
>> +
>> + ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
>> + if (ret == 0)
>> + ret = platform_device_add_data(pdev, data, sizeof(*data));
>> + if (ret)
>> + goto fail;
>> +
>> + ret = platform_device_add(pdev);
>> + if (ret)
>> + goto fail;
>> +
>> + /* return device handle to board setup code */
>> + data->dev = &pdev->dev;
>> + return 0;
>> +
>> +fail:
>> + platform_device_put(pdev);
>> + return ret;
>> +}
>> +
>> static inline void omap1_mmc_mux(struct omap_mmc_platform_data *mmc_controller,
>> int controller_nr)
>> {
>> diff --git a/arch/arm/mach-omap2/board-n8x0.c b/arch/arm/mach-omap2/board-n8x0.c
>> index 1cb53bc..5c9cd31 100644
>> --- a/arch/arm/mach-omap2/board-n8x0.c
>> +++ b/arch/arm/mach-omap2/board-n8x0.c
>> @@ -594,8 +594,6 @@ static struct omap_mmc_platform_data mmc1_data = {
>> },
>> };
>>
>> -static struct omap_mmc_platform_data *mmc_data[OMAP24XX_NR_MMC];
>> -
>> static void __init n8x0_mmc_init(void)
>>
>> {
>> @@ -637,8 +635,8 @@ static void __init n8x0_mmc_init(void)
>> gpio_direction_output(N810_EMMC_VIO_GPIO, 0);
>> }
>>
>> - mmc_data[0] = &mmc1_data;
>> - omap2_init_mmc(mmc_data, OMAP24XX_NR_MMC);
>> + hsmmc_data[0] = &mmc1_data;
>> + omap_hwmod_for_each_by_class("mmc", omap2_init_mmc, NULL);
>
> The interface between board code and common code isn't quite right.
>
> The hwmod_for_each should not be done in board code, this should be in
> common code available to all boards.
OK
>
> The board code should simply call some mmc init routine optionally
> passing int the platform_data, just like is done for hsmmc_init.
>
> With common hwmod data, shouldn't it be possible to unify the init of
> MMC and HS-MMC controllers?
The init of MMC & HS-MMC are different.
HSMMC supported boards, the board file fills an intermediate structure
omap2_hsmmc_info
which is used by the hsmmc.c file to update the omap_mmc_platform_data.
MMC supported boards, the board file fills the omap_mmc_platform_data.
Ok, will try to keep an API common for init of both MMC & HSMMC
>
>
>> }
>> #else
>>
>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
>> index 2c9c912..aa07264 100644
>> --- a/arch/arm/mach-omap2/devices.c
>> +++ b/arch/arm/mach-omap2/devices.c
>> @@ -610,112 +610,6 @@ static inline void omap_init_aes(void) { }
>>
>> /*-------------------------------------------------------------------------*/
>>
>> -#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4)
>> -
>> -#define MMCHS_SYSCONFIG 0x0010
>> -#define MMCHS_SYSCONFIG_SWRESET (1 << 1)
>> -#define MMCHS_SYSSTATUS 0x0014
>> -#define MMCHS_SYSSTATUS_RESETDONE (1 << 0)
>> -
>> -static struct platform_device dummy_pdev = {
>> - .dev = {
>> - .bus = &platform_bus_type,
>> - },
>> -};
>> -
>> -/**
>> - * omap_hsmmc_reset() - Full reset of each HS-MMC controller
>> - *
>> - * Ensure that each MMC controller is fully reset. Controllers
>> - * left in an unknown state (by bootloader) may prevent retention
>> - * or OFF-mode. This is especially important in cases where the
>> - * MMC driver is not enabled, _or_ built as a module.
>> - *
>> - * In order for reset to work, interface, functional and debounce
>> - * clocks must be enabled. The debounce clock comes from func_32k_clk
>> - * and is not under SW control, so we only enable i- and f-clocks.
>> - **/
>> -static void __init omap_hsmmc_reset(void)
>> -{
>> - u32 i, nr_controllers;
>> - struct clk *iclk, *fclk;
>> -
>> - if (cpu_is_omap242x())
>> - return;
>> -
>> - nr_controllers = cpu_is_omap44xx() ? OMAP44XX_NR_MMC :
>> - (cpu_is_omap34xx() ? OMAP34XX_NR_MMC : OMAP24XX_NR_MMC);
>> -
>> - for (i = 0; i < nr_controllers; i++) {
>> - u32 v, base = 0;
>> - struct device *dev = &dummy_pdev.dev;
>> -
>> - switch (i) {
>> - case 0:
>> - base = OMAP2_MMC1_BASE;
>> - break;
>> - case 1:
>> - base = OMAP2_MMC2_BASE;
>> - break;
>> - case 2:
>> - base = OMAP3_MMC3_BASE;
>> - break;
>> - case 3:
>> - if (!cpu_is_omap44xx())
>> - return;
>> - base = OMAP4_MMC4_BASE;
>> - break;
>> - case 4:
>> - if (!cpu_is_omap44xx())
>> - return;
>> - base = OMAP4_MMC5_BASE;
>> - break;
>> - }
>> -
>> - if (cpu_is_omap44xx())
>> - base += OMAP4_MMC_REG_OFFSET;
>> -
>> - dummy_pdev.id = i;
>> - dev_set_name(&dummy_pdev.dev, "mmci-omap-hs.%d", i);
>> - iclk = clk_get(dev, "ick");
>> - if (IS_ERR(iclk))
>> - goto err1;
>> - if (clk_enable(iclk))
>> - goto err2;
>> -
>> - fclk = clk_get(dev, "fck");
>> - if (IS_ERR(fclk))
>> - goto err3;
>> - if (clk_enable(fclk))
>> - goto err4;
>> -
>> - omap_writel(MMCHS_SYSCONFIG_SWRESET, base + MMCHS_SYSCONFIG);
>> - v = omap_readl(base + MMCHS_SYSSTATUS);
>> - while (!(omap_readl(base + MMCHS_SYSSTATUS) &
>> - MMCHS_SYSSTATUS_RESETDONE))
>> - cpu_relax();
>> -
>> - clk_disable(fclk);
>> - clk_put(fclk);
>> - clk_disable(iclk);
>> - clk_put(iclk);
>> - }
>> - return;
>> -
>> -err4:
>> - clk_put(fclk);
>> -err3:
>> - clk_disable(iclk);
>> -err2:
>> - clk_put(iclk);
>> -err1:
>> - printk(KERN_WARNING "%s: Unable to enable clocks for MMC%d, "
>> - "cannot reset.\n", __func__, i);
>> -}
>> -#else
>> -static inline void omap_hsmmc_reset(void) {}
>> -#endif
>> -
>
> Very nice to see this disappear.
>
>> #if defined(CONFIG_MMC_OMAP) || defined(CONFIG_MMC_OMAP_MODULE) || \
>> defined(CONFIG_MMC_OMAP_HS) || defined(CONFIG_MMC_OMAP_HS_MODULE)
>>
>> @@ -828,66 +722,54 @@ static inline void omap2_mmc_mux(struct omap_mmc_platform_data *mmc_controller,
>> }
>> }
>>
>> -void __init omap2_init_mmc(struct omap_mmc_platform_data **mmc_data,
>> - int nr_controllers)
>> +struct omap_mmc_platform_data *hsmmc_data[OMAP44XX_NR_MMC];
>> +EXPORT_SYMBOL(hsmmc_data);
>
> This should not be exported. In fact, it should be static. The need
> to modify this from board files suggest the interface from board files
> is not correct.
OK, Will try keep it static
>
>> +static struct omap_device_pm_latency omap_hsmmc_latency[] = {
>> + [0] = {
>> + .deactivate_func = omap_device_idle_hwmods,
>> + .activate_func = omap_device_enable_hwmods,
>> + .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>> + },
>> + /*
>> + * XXX There should also be an entry here to power off/on the
>> + * MMC regulators/PBIAS cells, etc.
>> + */
>> +};
>> +
>> +int omap2_init_mmc(struct omap_hwmod *oh, void *nop)
>> {
>> - int i;
>> + struct omap_device *od;
>> + struct omap_device_pm_latency *ohl;
>> char *name;
>> + int ohl_cnt = 0;
>> + static int mmc_num;
>
> With the 'static' here, this function now keeps state of how many
> controllers there are? This is fragile and not good for readability.
>
> The common code here should to an omap_hwmod_for_each() and will know
> how many controllers are on the SoC. The board code will then register
> its data and now many controllers it wants to use. I don't see any
> reason to have to keep this static state.
OK , Will try to implement as mentioned.
>
>> - for (i = 0; i < nr_controllers; i++) {
>> - unsigned long base, size;
>> - unsigned int irq = 0;
>> -
>> - if (!mmc_data[i])
>> - continue;
>> -
>> - omap2_mmc_mux(mmc_data[i], i);
>> -
>> - switch (i) {
>> - case 0:
>> - base = OMAP2_MMC1_BASE;
>> - irq = INT_24XX_MMC_IRQ;
>> - break;
>> - case 1:
>> - base = OMAP2_MMC2_BASE;
>> - irq = INT_24XX_MMC2_IRQ;
>> - break;
>> - case 2:
>> - if (!cpu_is_omap44xx() && !cpu_is_omap34xx())
>> - return;
>> - base = OMAP3_MMC3_BASE;
>> - irq = INT_34XX_MMC3_IRQ;
>> - break;
>> - case 3:
>> - if (!cpu_is_omap44xx())
>> - return;
>> - base = OMAP4_MMC4_BASE;
>> - irq = OMAP44XX_IRQ_MMC4;
>> - break;
>> - case 4:
>> - if (!cpu_is_omap44xx())
>> - return;
>> - base = OMAP4_MMC5_BASE;
>> - irq = OMAP44XX_IRQ_MMC5;
>> - break;
>> - default:
>> - continue;
>> - }
>> + if (!hsmmc_data[mmc_num]) {
>> + mmc_num++;
>> + return 0;
>> + }
>> + if (cpu_is_omap2420()) {
>> + name = "mmci-omap";
>> + } else {
>> + name = "mmci-omap-hs";
>> + ohl = omap_hsmmc_latency;
>> + ohl_cnt = ARRAY_SIZE(omap_hsmmc_latency);
>> + }
>
> The naming used here (and in the driver) should match the naming used
> from the hwmod data. We're trying to align theses to the commonly
> defined names.
OK , will correct this in V3
>
>> - if (cpu_is_omap2420()) {
>> - size = OMAP2420_MMC_SIZE;
>> - name = "mmci-omap";
>> - } else if (cpu_is_omap44xx()) {
>> - if (i < 3)
>> - irq += OMAP44XX_IRQ_GIC_START;
>> - size = OMAP4_HSMMC_SIZE;
>> - name = "mmci-omap-hs";
>> - } else {
>> - size = OMAP3_HSMMC_SIZE;
>> - name = "mmci-omap-hs";
>> - }
>> - omap_mmc_add(name, i, base, size, irq, mmc_data[i]);
>> - };
>> + hsmmc_data[mmc_num]->dev_attr = oh->dev_attr;
>> + omap2_mmc_mux(hsmmc_data[mmc_num], mmc_num);
>> + od = omap_device_build(name, mmc_num, oh, hsmmc_data[mmc_num],
>> + sizeof(struct omap_mmc_platform_data), ohl, ohl_cnt, false);
>> + WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
>> + name, oh->name);
>> + /*
>> + * return device handle to board setup code
>> + * required to populate for regulator framework structure
>> + */
>> + hsmmc_data[mmc_num]->dev = &od->pdev.dev;
>> + mmc_num++;
>> + return 0;
>> }
>>
>> #endif
>> @@ -961,7 +843,6 @@ static int __init omap2_init_devices(void)
>> * please keep these calls, and their implementations above,
>> * in alphabetical order so they're easier to sort through.
>> */
>> - omap_hsmmc_reset();
>> omap_init_audio();
>> omap_init_camera();
>> omap_init_mbox();
>> diff --git a/arch/arm/mach-omap2/hsmmc.c b/arch/arm/mach-omap2/hsmmc.c
>> index 34272e4..8f1a484 100644
>> --- a/arch/arm/mach-omap2/hsmmc.c
>> +++ b/arch/arm/mach-omap2/hsmmc.c
>> @@ -30,7 +30,7 @@ static u16 control_mmc1;
>>
>> static struct hsmmc_controller {
>> char name[HSMMC_NAME_LEN + 1];
>> -} hsmmc[OMAP34XX_NR_MMC];
>> +} hsmmc[OMAP44XX_NR_MMC];
>
> This structure seems unused.
It's used to fill the slot name of each controller's platform data .
<snip>
Regards,
Kishore
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
