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

Kadiyala, Kishore
2011-02-24T14:02:47Z

On Thu, Feb 24, 2011 at 11:58 AM, Varadarajan, Charulatha <charu@ti.com> wrote:
> Kishore,
>
> On Wed, Feb 23, 2011 at 23:17, Kishore Kadiyala <kishore.kadiyala@ti.com> 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.
>
> why?

Will mention the details in the log

>
>> 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(-)
>>
>
> <<snip>>
>
>> 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];
>
> Why do you have a dependency on OMAP44XX_NR_MMC? You should get
> this kind of information using number of iterations in *_hwmod_each_by_class.

Ok , will remove

>
>>
>>  #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;
>
> Ditto
>
>> +
>> +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.
>> +        */
>
> Check multi-line comment style

Ok

>
>> +       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";
>> +       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;
>
> It would be good rename controller_dev_attr to controller_flag

Ok

>
>> +       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;
>> +}
>>
>>  void __init omap2_hsmmc_init(struct omap2_hsmmc_info *controllers)
>>  {
>> @@ -358,16 +496,7 @@ void __init omap2_hsmmc_init(struct omap2_hsmmc_info *controllers)
>>                hsmmc_data[c->mmc - 1] = mmc;
>>        }
>>
>> -       omap2_init_mmc(hsmmc_data, OMAP34XX_NR_MMC);
>> -
>> -       /* pass the device nodes back to board setup code */
>> -       for (c = controllers; c->mmc; c++) {
>> -               struct omap_mmc_platform_data *mmc = hsmmc_data[c->mmc - 1];
>> -
>> -               if (!c->mmc || c->mmc > nr_hsmmc)
>> -                       continue;
>> -               c->dev = mmc->dev;
>> -       }
>> +       omap_hwmod_for_each_by_class("mmc", omap2_mmc_init, controllers);
>>
>>  done:
>>        for (i = 0; i < nr_hsmmc; i++)
>> diff --git a/arch/arm/plat-omap/include/plat/mmc.h b/arch/arm/plat-omap/include/plat/mmc.h
>> index 7853130..6205984 100644
>> --- a/arch/arm/plat-omap/include/plat/mmc.h
>> +++ b/arch/arm/plat-omap/include/plat/mmc.h
>> @@ -24,20 +24,12 @@
>>  #define OMAP1_MMC2_BASE                0xfffb7c00      /* omap16xx only */
>>
>>  #define OMAP24XX_NR_MMC                2
>> -#define OMAP34XX_NR_MMC                3
>>  #define OMAP44XX_NR_MMC                5
>
> As mentioned above, OMAP44XX_NR_MMC & OMAP24XX_NR_MMC shall be
> removed and instead get these info from hwmod.

Agree

>
> I had provided the same comment in the previous patch version too. You may
> reject the comment but mention the reasons for the same.
>
>>  #define OMAP2420_MMC_SIZE      OMAP1_MMC_SIZE
>> -#define OMAP3_HSMMC_SIZE       0x200
>>  #define OMAP4_HSMMC_SIZE       0x1000
>
> Can't this be removed?

will check

>
>>  #define OMAP2_MMC1_BASE                0x4809c000
>>  #define OMAP2_MMC2_BASE                0x480b4000
>> -#define OMAP3_MMC3_BASE                0x480ad000
>> -#define OMAP4_MMC4_BASE                0x480d1000
>> -#define OMAP4_MMC5_BASE                0x480d5000
>>  #define OMAP4_MMC_REG_OFFSET   0x100
>
> Remove OMAP4_MMC_REG_OFFSET too.

This is needed for the OMAP4 since the registers which
were accessed has offset deviation of 0x100 w.r.t to OMAP3

>
>> -#define HSMMC5                 (1 << 4)
>> -#define HSMMC4                 (1 << 3)
>> -#define HSMMC3                 (1 << 2)
>>  #define HSMMC2                 (1 << 1)
>>  #define HSMMC1                 (1 << 0)
>
> <<snip>>
>