From: "Cousson, Benoit" <b-cousson@ti.com>
To: "Kadiyala, Kishore" <kishore.kadiyala@ti.com>
Cc: "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
"tony@atomide.com" <tony@atomide.com>,
"cjb@laptop.org" <cjb@laptop.org>,
"Chikkature Rajashekar, Madhusudhan" <madhu.cr@ti.com>,
"khilman@deeprootsystems.com" <khilman@deeprootsystems.com>,
"paul@pwsan.com" <paul@pwsan.com>
Subject: Re: [PATCH v3 5/5] OMAP: adapt hsmmc to hwmod framework
Date: Thu, 24 Feb 2011 12:49:02 +0100 [thread overview]
Message-ID: <4D6645AE.5080506@ti.com> (raw)
In-Reply-To: <1298483231-29622-6-git-send-email-kishore.kadiyala@ti.com>
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.
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?
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.
> +
> +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 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.
> + 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
next prev parent reply other threads:[~2011-02-24 11:49 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-23 17:47 [PATCH v3 0/5] OMAP: HSMMC: hwmod adaptation Kishore Kadiyala
2011-02-23 17:47 ` [PATCH v3 1/5] OMAP2430: hwmod data: Add HSMMC Kishore Kadiyala
2011-02-23 17:47 ` [PATCH v3 2/5] OMAP3: " Kishore Kadiyala
2011-02-23 17:47 ` [PATCH v3 3/5] OMAP4: hwmod data: enable HSMMC Kishore Kadiyala
2011-02-24 10:21 ` Cousson, Benoit
2011-02-23 17:47 ` [PATCH v3 4/5] OMAP: hwmod data: Add dev_attr and use in the host driver Kishore Kadiyala
2011-02-24 6:00 ` Varadarajan, Charulatha
2011-02-24 14:05 ` Kadiyala, Kishore
2011-02-24 10:59 ` Cousson, Benoit
2011-02-24 13:58 ` Kadiyala, Kishore
2011-02-23 17:47 ` [PATCH v3 5/5] OMAP: adapt hsmmc to hwmod framework Kishore Kadiyala
2011-02-24 6:28 ` Varadarajan, Charulatha
2011-02-24 14:02 ` Kadiyala, Kishore
2011-02-24 11:49 ` Cousson, Benoit [this message]
2011-02-24 13:55 ` Kadiyala, Kishore
2011-02-24 14:33 ` Cousson, Benoit
2011-02-24 10:19 ` [PATCH v3 0/5] OMAP: HSMMC: hwmod adaptation Cousson, Benoit
2011-02-24 18:10 ` Kadiyala, Kishore
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D6645AE.5080506@ti.com \
--to=b-cousson@ti.com \
--cc=cjb@laptop.org \
--cc=khilman@deeprootsystems.com \
--cc=kishore.kadiyala@ti.com \
--cc=linux-mmc@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=madhu.cr@ti.com \
--cc=paul@pwsan.com \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.