Re: [PATCH v3 5/5] OMAP: adapt hsmmc to hwmod framework

Kadiyala, Kishore
2011-02-24T13:55:07Z

On Thu, Feb 24, 2011 at 5:19 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 2/23/2011 6:47 PM, Kadiyala, Kishore wrote:
>>
>> Changes involves:
>> 1) Remove controller reset in devices.c which is taken care of
>>    by hwmod framework.
>> 2) Removing all base address macro defines except keeping one for
>> OMAP2420.
>> 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.
>>
>> Signed-off-by: Paul Walmsley<paul@pwsan.com>
>> Signed-off-by: Kishore Kadiyala<kishore.kadiyala@ti.com>
>> Cc: Benoit Cousson<b-cousson@ti.com>
>> CC: Kevin Hilman<khilman@deeprootsystems.com>
>> ---
>>  arch/arm/mach-omap2/devices.c         |  251
>> ---------------------------------
>>  arch/arm/mach-omap2/hsmmc.c           |  153 ++++++++++++++++++--
>>  arch/arm/plat-omap/include/plat/mmc.h |   14 --
>>  3 files changed, 141 insertions(+), 277 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
>> index 7b35c87..2d2deb6 100644
>> --- a/arch/arm/mach-omap2/devices.c
>> +++ b/arch/arm/mach-omap2/devices.c
>> @@ -503,112 +503,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
>> -
>>  #if defined(CONFIG_MMC_OMAP) || defined(CONFIG_MMC_OMAP_MODULE)
>>
>>  static inline void omap242x_mmc_mux(struct omap_mmc_platform_data
>> @@ -665,150 +559,6 @@ void __init omap242x_init_mmc(struct
>> omap_mmc_platform_data **mmc_data)
>>
>>  #endif
>>
>> -#if defined(CONFIG_MMC_OMAP_HS) || defined(CONFIG_MMC_OMAP_HS_MODULE)
>> -
>> -static inline void omap2_mmc_mux(struct omap_mmc_platform_data
>> *mmc_controller,
>> -                       int controller_nr)
>> -{
>> -       if ((mmc_controller->slots[0].switch_pin>  0)&&  \
>> -               (mmc_controller->slots[0].switch_pin<
>>  OMAP_MAX_GPIO_LINES))
>> -               omap_mux_init_gpio(mmc_controller->slots[0].switch_pin,
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -       if ((mmc_controller->slots[0].gpio_wp>  0)&&  \
>> -               (mmc_controller->slots[0].gpio_wp<  OMAP_MAX_GPIO_LINES))
>> -               omap_mux_init_gpio(mmc_controller->slots[0].gpio_wp,
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -       if (cpu_is_omap34xx()) {
>> -               if (controller_nr == 0) {
>> -                       omap_mux_init_signal("sdmmc1_clk",
>> -                               OMAP_PIN_INPUT_PULLUP);
>> -                       omap_mux_init_signal("sdmmc1_cmd",
>> -                               OMAP_PIN_INPUT_PULLUP);
>> -                       omap_mux_init_signal("sdmmc1_dat0",
>> -                               OMAP_PIN_INPUT_PULLUP);
>> -                       if (mmc_controller->slots[0].caps&
>> -                               (MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA))
>> {
>> -                               omap_mux_init_signal("sdmmc1_dat1",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                               omap_mux_init_signal("sdmmc1_dat2",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                               omap_mux_init_signal("sdmmc1_dat3",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                       }
>> -                       if (mmc_controller->slots[0].caps&
>> -                                               MMC_CAP_8_BIT_DATA) {
>> -                               omap_mux_init_signal("sdmmc1_dat4",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                               omap_mux_init_signal("sdmmc1_dat5",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                               omap_mux_init_signal("sdmmc1_dat6",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                               omap_mux_init_signal("sdmmc1_dat7",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                       }
>> -               }
>> -               if (controller_nr == 1) {
>> -                       /* MMC2 */
>> -                       omap_mux_init_signal("sdmmc2_clk",
>> -                               OMAP_PIN_INPUT_PULLUP);
>> -                       omap_mux_init_signal("sdmmc2_cmd",
>> -                               OMAP_PIN_INPUT_PULLUP);
>> -                       omap_mux_init_signal("sdmmc2_dat0",
>> -                               OMAP_PIN_INPUT_PULLUP);
>> -
>> -                       /*
>> -                        * For 8 wire configurations, Lines DAT4, 5, 6 and
>> 7 need to be muxed
>> -                        * in the board-*.c files
>> -                        */
>> -                       if (mmc_controller->slots[0].caps&
>> -                               (MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA))
>> {
>> -                               omap_mux_init_signal("sdmmc2_dat1",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                               omap_mux_init_signal("sdmmc2_dat2",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                               omap_mux_init_signal("sdmmc2_dat3",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                       }
>> -                       if (mmc_controller->slots[0].caps&
>> -
>> MMC_CAP_8_BIT_DATA) {
>> -
>> omap_mux_init_signal("sdmmc2_dat4.sdmmc2_dat4",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -
>> omap_mux_init_signal("sdmmc2_dat5.sdmmc2_dat5",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -
>> omap_mux_init_signal("sdmmc2_dat6.sdmmc2_dat6",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -
>> omap_mux_init_signal("sdmmc2_dat7.sdmmc2_dat7",
>> -                                       OMAP_PIN_INPUT_PULLUP);
>> -                       }
>> -               }
>> -
>> -               /*
>> -                * For MMC3 the pins need to be muxed in the board-*.c
>> files
>> -                */
>> -       }
>> -}
>> -
>> -void __init omap2_init_mmc(struct omap_mmc_platform_data **mmc_data,
>> -                       int nr_controllers)
>> -{
>> -       int i;
>> -       char *name;
>> -
>> -       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 (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]);
>> -       };
>> -}
>> -
>> -#endif
>> -
>>
>>  /*-------------------------------------------------------------------------*/
>>
>>  #if defined(CONFIG_HDQ_MASTER_OMAP) ||
>> defined(CONFIG_HDQ_MASTER_OMAP_MODULE)
>> @@ -878,7 +628,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..b6e1eae 100644
>> --- a/arch/arm/mach-omap2/hsmmc.c
>> +++ b/arch/arm/mach-omap2/hsmmc.c
>> @@ -16,7 +16,11 @@
>>  #include<mach/hardware.h>
>>  #include<plat/mmc.h>
>>  #include<plat/omap-pm.h>
>> +#include<plat/mux.h>
>> +#include<plat/omap_hwmod.h>
>> +#include<plat/omap_device.h>
>>
>> +#include "mux.h"
>>  #include "hsmmc.h"
>>  #include "control.h"
>>
>> @@ -30,7 +34,7 @@ static u16 control_mmc1;
>>
>>  static struct hsmmc_controller {
>>         char                            name[HSMMC_NAME_LEN + 1];
>> -} hsmmc[OMAP34XX_NR_MMC];
>> +} hsmmc[OMAP44XX_NR_MMC];
>
> I do not know the details of that driver, so this comment might be
> completely irrelevant, but in theory, such static table should not be
> necessary since the driver core already maintain a list of all instances
> bound to it.

I agree, but this is used in slot data for the controller and is used
in the driver
to create a /sys entry. I will try to avoid the "OMAP44XX_NR_MMC" dependency.

> Because of that, you will have to modify this code for every new OMAP
> generation each time we add a new instance.
>
>>
>>  #if defined(CONFIG_ARCH_OMAP3)&&  defined(CONFIG_PM)
>>
>> @@ -204,7 +208,141 @@ static int nop_mmc_set_power(struct device *dev, int
>> slot, int power_on,
>>         return 0;
>>  }
>>
>> -static struct omap_mmc_platform_data *hsmmc_data[OMAP34XX_NR_MMC]
>> __initdata;
>> +static inline void omap2_mmc_mux(struct omap_mmc_platform_data
>> *mmc_controller,
>> +                       int controller_nr)
>> +{
>> +       if ((mmc_controller->slots[0].switch_pin>  0)&&  \
>> +               (mmc_controller->slots[0].switch_pin<
>>  OMAP_MAX_GPIO_LINES))
>> +               omap_mux_init_gpio(mmc_controller->slots[0].switch_pin,
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +       if ((mmc_controller->slots[0].gpio_wp>  0)&&  \
>> +               (mmc_controller->slots[0].gpio_wp<  OMAP_MAX_GPIO_LINES))
>> +               omap_mux_init_gpio(mmc_controller->slots[0].gpio_wp,
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +       if (cpu_is_omap34xx()) {
>> +               if (controller_nr == 0) {
>> +                       omap_mux_init_signal("sdmmc1_clk",
>> +                               OMAP_PIN_INPUT_PULLUP);
>> +                       omap_mux_init_signal("sdmmc1_cmd",
>> +                               OMAP_PIN_INPUT_PULLUP);
>> +                       omap_mux_init_signal("sdmmc1_dat0",
>> +                               OMAP_PIN_INPUT_PULLUP);
>> +                       if (mmc_controller->slots[0].caps&
>> +                               (MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA))
>> {
>> +                               omap_mux_init_signal("sdmmc1_dat1",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                               omap_mux_init_signal("sdmmc1_dat2",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                               omap_mux_init_signal("sdmmc1_dat3",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                       }
>> +                       if (mmc_controller->slots[0].caps&
>> +                                               MMC_CAP_8_BIT_DATA) {
>> +                               omap_mux_init_signal("sdmmc1_dat4",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                               omap_mux_init_signal("sdmmc1_dat5",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                               omap_mux_init_signal("sdmmc1_dat6",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                               omap_mux_init_signal("sdmmc1_dat7",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                       }
>> +               }
>> +               if (controller_nr == 1) {
>> +                       /* MMC2 */
>> +                       omap_mux_init_signal("sdmmc2_clk",
>> +                               OMAP_PIN_INPUT_PULLUP);
>> +                       omap_mux_init_signal("sdmmc2_cmd",
>> +                               OMAP_PIN_INPUT_PULLUP);
>> +                       omap_mux_init_signal("sdmmc2_dat0",
>> +                               OMAP_PIN_INPUT_PULLUP);
>> +
>> +                       /*
>> +                        * For 8 wire configurations, Lines DAT4, 5, 6 and
>> 7
>> +                        * need to be muxed in the board-*.c files
>> +                        */
>> +                       if (mmc_controller->slots[0].caps&
>> +                               (MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA))
>> {
>> +                               omap_mux_init_signal("sdmmc2_dat1",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                               omap_mux_init_signal("sdmmc2_dat2",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                               omap_mux_init_signal("sdmmc2_dat3",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                       }
>> +                       if (mmc_controller->slots[0].caps&
>> +
>> MMC_CAP_8_BIT_DATA) {
>> +
>> omap_mux_init_signal("sdmmc2_dat4.sdmmc2_dat4",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +
>> omap_mux_init_signal("sdmmc2_dat5.sdmmc2_dat5",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +
>> omap_mux_init_signal("sdmmc2_dat6.sdmmc2_dat6",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +
>> omap_mux_init_signal("sdmmc2_dat7.sdmmc2_dat7",
>> +                                       OMAP_PIN_INPUT_PULLUP);
>> +                       }
>> +               }
>> +
>> +               /*
>> +                * For MMC3 the pins need to be muxed in the board-*.c
>> files
>> +                */
>> +       }
>> +}
>> +
>> +static struct omap_mmc_platform_data *hsmmc_data[OMAP44XX_NR_MMC]
>> __initdata;
>
> Same concern than for hsmmc, why do you need static table here?

Agree, will remove static declaration.

> And why do you need another structure? The omap2_hsmmc_info should already
> be in a pdata kind of structure.
> The board file should just populate a table of pdata that you will use
> during init.

No, omap2_hsmmc_info is intermediate structure used by the boards
files to update
some basic info of the controller, based on which the pdata is
populated in hsmmc.c.

>
>> +
>> +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.
>> +        */
>> +};
>> +
>> +static int omap2_mmc_init(struct omap_hwmod *oh, void *hsmmcinfo)
>> +{
>> +       struct omap_device *od;
>> +       struct omap_device_pm_latency *ohl;
>> +       char *name;
>> +       int ohl_cnt = 0;
>> +       struct mmc_dev_attr *mmc_dattr = oh->dev_attr;
>> +       struct omap2_hsmmc_info *c = (struct omap2_hsmmc_info *)
>> hsmmcinfo;
>> +       int idx;
>> +       static int mmc_num;
>> +
>> +       /* Initialization of controllers which are not updated
>> +        * in board file will be skipped here.
>> +        */
>> +       c += mmc_num;
>> +       if (!c->mmc) {
>> +               pr_err("omap_hsmmc_info is not updated in board file\n");
>> +               return 0;
>> +       }
>> +       idx = c->mmc - 1 ;
>> +       name = "mmci-omap-hs";
>
> Could you please rename the device to have something in the form: omap_XXXX?
> In that case "omap_mmc" should be good. The "hs" is just a indication of one
> of the mmc instance capability and does not have to be in the device name
> since there is no none-hs instance.

I understood your concern but omap1,omap2420 uses mmc driver while
omap2430, omap3 , omap4 has hsmmc driver.

omap1, omap2420 boards have device name as "mmci-omap" currently, but
if they undergo
the similar change as proposed above then it looks like "omap_mmc"

Therefore for hsmmc driver, I will be happy to have something like "omap_hsmmc"

please let me know if this is fine.

>
> I know that this is not in your patch and was already there before, but that
> code is happily mixing MMC or HSMMC everywhere, so having a little bit of
> consistency will be good. I vote to stick to MMC only.
> The "omap2_" prefix does not seems necessary too for my point of view.
> "omap_" is good enough.
>
>> +       ohl = omap_hsmmc_latency;
>> +       ohl_cnt = ARRAY_SIZE(omap_hsmmc_latency);
>> +       omap2_mmc_mux(hsmmc_data[idx], idx);
>> +       hsmmc_data[idx]->controller_dev_attr = mmc_dattr->flags;
>
> You should copy the data and not keep a reference to internal hwmod
> structure. In your case, you should check if dev_attr is not null. It will
> avoid adding some empty dev_attr structure in the hwmod_data files.

Ok, will make changes.

>
>> +       od = omap_device_build(name, idx, oh, hsmmc_data[idx],
>> +               sizeof(struct omap_mmc_platform_data), ohl, ohl_cnt,
>> false);
>> +       if (IS_ERR(od)) {
>> +               WARN(1, "Cant build omap_device for %s:%s.\n",
>> +                                       name, oh->name);
>> +               return PTR_ERR(od);
>> +       }
>> +       /*
>> +        * return device handle to board setup code
>> +        * required to populate for regulator framework structure
>> +        */
>> +       c->dev =&od->pdev.dev;
>> +       mmc_num++;
>> +       return 0;
>> +}
>
> Regards,
> Benoit
>