Re: [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime

2013-06-14 Thread Tony Lindgren 
* Tony Lindgren t...@atomide.com [130612 06:27]:
 * Linus Walleij linus.wall...@linaro.org [130611 01:00]:
  If we can agree on this I will add the active state to the
  state table and add a container in the core for this as well
  as pinctrl_pm_select_active_state() so we can skip all the
  pointless boilerplate also in the OMAP drivers, plus increase
  the readability and portability quite a bit.
 
 Sounds good to me as long as we don't always need to select
 the default pins over and over in PM runtime_resume.

Here's this patch updated for pinctrl next with the active state
added. This patch should be merged separately towards the end
of the merge window once pinctrl code has been merged. The other
three patches can be merged independently.

Regards,

Tony


From: Tony Lindgren t...@atomide.com
Date: Thu, 13 Jun 2013 23:19:09 -0700
Subject: [PATCH] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM 
runtime

On some omaps we need to remux MMC pins for PM, and for some omaps
we need to remux the SDIO IRQ pin.

Based on an earlier patch by Andreas Fenkart afenk...@gmail.com.

Cc: Andreas Fenkart afenk...@gmail.com
Cc: Balaji T K balaj...@ti.com
Cc: Linus Walleij linus.wall...@linaro.org
Signed-off-by: Tony Lindgren t...@atomide.com

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 6377836..7029d34 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -475,6 +475,39 @@ static void omap_hsmmc_gpio_free(struct 
omap_mmc_platform_data *pdata)
gpio_free(pdata-slots[0].gpio_cirq);
 }
 
+static int omap_hsmmc_pinctrl_init(struct omap_hsmmc_host *host)
+{
+   struct device *dev = mmc_dev(host-mmc);
+   int res, found = 0;
+
+   if (!dev-pins)
+   return 0;
+
+   /*
+* The active and idle pins are optional, and used for
+* SDIO interrupt, or eMMC pulls for off-idle.
+*/
+   if (IS_ERR(dev-pins-active_state) ||
+   IS_ERR(dev-pins-idle_state)) {
+   return 0;
+   }
+
+   /* Let's make sure the idle and active states work */
+   res = pinctrl_pm_select_idle_state(dev);
+   if (res  0)
+   return -ENODEV;
+   found++;
+
+   res = pinctrl_pm_select_active_state(dev);
+   if (res  0)
+   return -ENODEV;
+   found++;
+
+   dev_info(mmc_dev(dev), pins configured for dynamic remuxing\n);
+
+   return found;
+}
+
 /*
  * Start clock to the card
  */
@@ -1868,7 +1901,6 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
const struct of_device_id *match;
dma_cap_mask_t mask;
unsigned tx_req, rx_req;
-   struct pinctrl *pinctrl;
 
match = of_match_device(of_match_ptr(omap_mmc_of_match), pdev-dev);
if (match) {
@@ -2101,21 +2133,19 @@ static int omap_hsmmc_probe(struct platform_device 
*pdev)
 
omap_hsmmc_disable_irq(host);
 
-   pinctrl = devm_pinctrl_get_select_default(pdev-dev);
-   if (IS_ERR(pinctrl))
-   dev_warn(pdev-dev,
-   pins are not configured from the driver\n);
-
/*
-* For now, only support SDIO interrupt if we are doing
-* muxing of dat1 when booted with DT. This is because the
+* For now, only support SDIO interrupt if we are doing dynamic
+* remuxing of dat1 when booted with DT. This is because the
 * supposedly the wake-up events for CTPL don't work from deeper
 * idle states. And we don't want to add new legacy mux platform
 * init code callbacks any longer as we are moving to DT based
 * booting anyways.
 */
if (match) {
-   if (!IS_ERR(pinctrl)  mmc_slot(host).sdio_irq)
+   ret = omap_hsmmc_pinctrl_init(host);
+   if (ret  0)
+   goto err_pinctrl_state;
+   else if (ret  1  mmc_slot(host).sdio_irq)
mmc-caps |= MMC_CAP_SDIO_IRQ;
}
 
@@ -2143,6 +2173,7 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 
 err_slot_name:
mmc_remove_host(mmc);
+err_pinctrl_state:
if ((mmc_slot(host).sdio_irq))
free_irq(mmc_slot(host).sdio_irq, host);
 err_irq_sdio:
@@ -2337,6 +2368,10 @@ static int omap_hsmmc_runtime_suspend(struct device *dev)
OMAP_HSMMC_WRITE(host-base, STAT, STAT_CLEAR);
spin_unlock_irqrestore(host-irq_lock, flags);
 
+   ret = pinctrl_pm_select_idle_state(dev);
+   if (ret  0)
+   dev_err(dev, Unable to select idle pinmux\n);
+
if (mmc_slot(host).sdio_irq)
enable_irq(mmc_slot(host).sdio_irq);
}
@@ -2360,6 +2395,10 @@ static int omap_hsmmc_runtime_resume(struct device *dev)
if (mmc_slot(host).sdio_irq)
disable_irq(mmc_slot(host).sdio_irq);
 
+   ret =

Re: [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime

2013-06-12 Thread Tony Lindgren 
* Linus Walleij linus.wall...@linaro.org [130611 01:00]:
 On Mon, Jun 10, 2013 at 6:23 PM, Tony Lindgren t...@atomide.com wrote:
 
  We only should remux the pins that need remuxing as that's done
  every time hitting idle. So I think we should have the following
  default groups:
 
  default Static pins that don't change, no need to remux
  configured in consumer driver probe like we already
  do
 
  active  Optional dynamic pins remuxed for runtime, can be
  configured and selected in consumer driver probe.
  The consumer driver may also want to select this
  state in PM runtime resume.
 
  idleOptional dynamic pins remuxed for idle. The consumer
  driver may also want to select this state in PM
  runtime suspend depending on device_can_wakeup()
  and driver specific needs.
 
 The one thing I don't understand is why a driver would select the
 active state in probe(), unless it's a driver that does not support
 runtime PM. (But maybe that's what you mean.)

Yes you're right, there should not be any need to select active state
in probe, that should be selected by PM runtime.
 
 Compare this to linus/pinctrl/pinctrl-state.h:
 
 /**
  * @PINCTRL_STATE_DEFAULT: the state the pinctrl handle shall be put
  *  into as default, usually this means the pins are up and ready to
  *  be used by the device driver. This state is commonly used by
  *  hogs to configure muxing and pins at boot, and also as a state
  *  to go into when returning from sleep and idle in
  *  .pm_runtime_resume() or ordinary .resume() for example.
  * @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into
  *  when the pins are idle. This is a state where the system is relaxed
  *  but not fully sleeping - some power may be on but clocks gated for
  *  example. Could typically be set from a pm_runtime_suspend() or
  *  pm_runtime_idle() operation.
  * @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into
  *  when the pins are sleeping. This is a state where the system is in
  *  its lowest sleep state. Could typically be set from an
  *  ordinary .suspend() function.
  */
 #define PINCTRL_STATE_DEFAULT default
 #define PINCTRL_STATE_IDLE idle
 #define PINCTRL_STATE_SLEEP sleep
 
 The way I currently use these in e.g.
 drivers/spi/spi-pl022 is:
 
 probe:
  - default
 
 runtime_suspend:
  - idle
 
 runtime_resume:
  - default
 
 suspend:
  - sleep
 
 resume:
   - default
   - idle
 
 Notice that we go to default then idle on probe and
 runtime resume. This is because the idle state is
 optional (as is the sleep state).
 
 So I guess if we should extend this terminology to match
 what you are using for the OMAP it would rather be like
 this:
 
 probe:
  - default
 
 runtime_suspend:
  - idle
 
 runtime_resume:
  - default
  - active

At least for omaps, there's no need to select default in
runtime_resume as the default pins stay that way.
 
 suspend:
  - sleep

For omaps, we would just select idle pins again in the
suspend case.
 
 resume:
   - default
   - idle

And for omaps, there's no need to select default in resume
either. Just selecting active would do the trick for resume.

So for omaps, the sequence would be:

probe:
 - default (typically all device pins except rx pin)

runtime_suspend:
suspend:
 - idle (remux rx pin from device to gpio input for wake)
 
runtime_resume:
resume:
 - active (remux rx pin from gpio input to device)
 
 Just one more optional active state in runtime resume.
 Correct?

Yes the active is needed, but sleep would be unused for
omaps.
 
 If we can agree on this I will add the active state to the
 state table and add a container in the core for this as well
 as pinctrl_pm_select_active_state() so we can skip all the
 pointless boilerplate also in the OMAP drivers, plus increase
 the readability and portability quite a bit.

Sounds good to me as long as we don't always need to select
the default pins over and over in PM runtime_resume.
 
  However in this case I *suspect* that what you really want
  to do it to rename the state called default to sleep
  (it appears the default state is sleepy) and then rename
  the active state to default (as this is the defined semantic
  meaning of default from linux/pinctrl/pinctrl-state.h.
 
  The idle state above could also be called sleep instead of idle
  if you prefer that.
 
 No I think we should reserve that name for the pin state
 associated with suspend(). Let's leave it like this.

OK
 
  I think the confusion is caused by the fact that we need three
  mux groups, not just two :) The toggling between active and idle
  is the hotpath as that can potentially happen for multiple drivers
  every time we enter and exit idle.
 
 Actually we have the same thing, it's just that our default
 and active are the same thing. But it seems we need to

Re: [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime

2013-06-11 Thread Linus Walleij 
On Mon, Jun 10, 2013 at 6:23 PM, Tony Lindgren t...@atomide.com wrote:

 We only should remux the pins that need remuxing as that's done
 every time hitting idle. So I think we should have the following
 default groups:

 default Static pins that don't change, no need to remux
 configured in consumer driver probe like we already
 do

 active  Optional dynamic pins remuxed for runtime, can be
 configured and selected in consumer driver probe.
 The consumer driver may also want to select this
 state in PM runtime resume.

 idleOptional dynamic pins remuxed for idle. The consumer
 driver may also want to select this state in PM
 runtime suspend depending on device_can_wakeup()
 and driver specific needs.

The one thing I don't understand is why a driver would select the
active state in probe(), unless it's a driver that does not support
runtime PM. (But maybe that's what you mean.)

Compare this to linus/pinctrl/pinctrl-state.h:

/**
 * @PINCTRL_STATE_DEFAULT: the state the pinctrl handle shall be put
 *  into as default, usually this means the pins are up and ready to
 *  be used by the device driver. This state is commonly used by
 *  hogs to configure muxing and pins at boot, and also as a state
 *  to go into when returning from sleep and idle in
 *  .pm_runtime_resume() or ordinary .resume() for example.
 * @PINCTRL_STATE_IDLE: the state the pinctrl handle shall be put into
 *  when the pins are idle. This is a state where the system is relaxed
 *  but not fully sleeping - some power may be on but clocks gated for
 *  example. Could typically be set from a pm_runtime_suspend() or
 *  pm_runtime_idle() operation.
 * @PINCTRL_STATE_SLEEP: the state the pinctrl handle shall be put into
 *  when the pins are sleeping. This is a state where the system is in
 *  its lowest sleep state. Could typically be set from an
 *  ordinary .suspend() function.
 */
#define PINCTRL_STATE_DEFAULT default
#define PINCTRL_STATE_IDLE idle
#define PINCTRL_STATE_SLEEP sleep

The way I currently use these in e.g.
drivers/spi/spi-pl022 is:

probe:
 - default

runtime_suspend:
 - idle

runtime_resume:
 - default

suspend:
 - sleep

resume:
  - default
  - idle

Notice that we go to default then idle on probe and
runtime resume. This is because the idle state is
optional (as is the sleep state).

So I guess if we should extend this terminology to match
what you are using for the OMAP it would rather be like
this:

probe:
 - default

runtime_suspend:
 - idle

runtime_resume:
 - default
 - active

suspend:
 - sleep

resume:
  - default
  - idle

Just one more optional active state in runtime resume.
Correct?

If we can agree on this I will add the active state to the
state table and add a container in the core for this as well
as pinctrl_pm_select_active_state() so we can skip all the
pointless boilerplate also in the OMAP drivers, plus increase
the readability and portability quite a bit.

 However in this case I *suspect* that what you really want
 to do it to rename the state called default to sleep
 (it appears the default state is sleepy) and then rename
 the active state to default (as this is the defined semantic
 meaning of default from linux/pinctrl/pinctrl-state.h.

 The idle state above could also be called sleep instead of idle
 if you prefer that.

No I think we should reserve that name for the pin state
associated with suspend(). Let's leave it like this.

 I think the confusion is caused by the fact that we need three
 mux groups, not just two :) The toggling between active and idle
 is the hotpath as that can potentially happen for multiple drivers
 every time we enter and exit idle.

Actually we have the same thing, it's just that our default
and active are the same thing. But it seems we need to
add your granularity to this.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime

2013-06-10 Thread Linus Walleij 
On Fri, Jun 7, 2013 at 11:49 PM, Tony Lindgren t...@atomide.com wrote:

 On some omaps we need to remux MMC pins for PM, and for some omaps
 we need to remux the SDIO IRQ pin.

 Based on an earlier patch by Andreas Fenkart afenk...@gmail.com.
(...)
 +   host-pinctrl = devm_pinctrl_get(host-dev);
 +   if (IS_ERR(host-pinctrl)) {
 +   dev_dbg(host-dev, no pinctrl handle\n);
 +   ret = 0;
 +   goto out;
 +   }
 +
 +   host-fixed = pinctrl_lookup_state(host-pinctrl,
 +  PINCTRL_STATE_DEFAULT);
 +   if (IS_ERR(host-fixed)) {
 +   dev_dbg(host-dev,
 +pins are not configured from the driver\n);
 +   host-fixed = NULL;
 +   ret = 0;
 +   goto out;
 +   }
 +
 +   ret = pinctrl_select_state(host-pinctrl, host-fixed);
 +   if (ret  0)
 +   goto err;
 +
 +   /* For most cases we don't have wake-ups, and exit after this */
 +   host-active = pinctrl_lookup_state(host-pinctrl, active);
 +   if (IS_ERR(host-active)) {
 +   ret = PTR_ERR(host-active);
 +   host-active = NULL;
 +   return 0;
 +   }
 +
 +   host-idle = pinctrl_lookup_state(host-pinctrl,
 + PINCTRL_STATE_IDLE);
 +   if (IS_ERR(host-idle)) {
 +   ret = PTR_ERR(host-idle);
 +   host-idle = NULL;
 +   goto err;
 +   }

You can use the new infrastructure to make the core select:

pinctrl_pm_select_default_state(host-dev);
pinctrl_pm_select_idle_state(host-dev);

What is the semantic difference between default and active?

If this is something very generic that a lot of platforms will want
to have, why not add it to include/linux/pinctrl/pinctrl-state.h
and augment the core to cache and handle this too?

However in this case I *suspect* that what you really want
to do it to rename the state called default to sleep
(it appears the default state is sleepy) and then rename
the active state to default (as this is the defined semantic
meaning of default from linux/pinctrl/pinctrl-state.h.

But maybe I'm not quite getting the subtle difference between
default and active here so enlighten me.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime

2013-06-10 Thread Tony Lindgren 
* Linus Walleij linus.wall...@linaro.org [130610 09:09]:
 
 You can use the new infrastructure to make the core select:
 
 pinctrl_pm_select_default_state(host-dev);
 pinctrl_pm_select_idle_state(host-dev);

OK great.
 
 What is the semantic difference between default and active?

We only should remux the pins that need remuxing as that's done
every time hitting idle. So I think we should have the following
default groups:

default Static pins that don't change, no need to remux
configured in consumer driver probe like we already
do

active  Optional dynamic pins remuxed for runtime, can be
configured and selected in consumer driver probe.
The consumer driver may also want to select this
state in PM runtime resume.

idleOptional dynamic pins remuxed for idle. The consumer
driver may also want to select this state in PM
runtime suspend depending on device_can_wakeup()
and driver specific needs.
 
 If this is something very generic that a lot of platforms will want
 to have, why not add it to include/linux/pinctrl/pinctrl-state.h
 and augment the core to cache and handle this too?

Yes we should do that assuming the above grouping makes sense
to you.
 
 However in this case I *suspect* that what you really want
 to do it to rename the state called default to sleep
 (it appears the default state is sleepy) and then rename
 the active state to default (as this is the defined semantic
 meaning of default from linux/pinctrl/pinctrl-state.h.

The idle state above could also be called sleep instead of idle
if you prefer that.

 But maybe I'm not quite getting the subtle difference between
 default and active here so enlighten me.

I think the confusion is caused by the fact that we need three
mux groups, not just two :) The toggling between active and idle
is the hotpath as that can potentially happen for multiple drivers
every time we enter and exit idle.

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] mmc: omap_hsmmc: Remux pins to support SDIO interrupt and PM runtime

2013-06-07 Thread Tony Lindgren 
On some omaps we need to remux MMC pins for PM, and for some omaps
we need to remux the SDIO IRQ pin.

Based on an earlier patch by Andreas Fenkart afenk...@gmail.com.

Cc: Andreas Fenkart afenk...@gmail.com
Cc: Balaji T K balaj...@ti.com
Cc: Linus Walleij linus.wall...@linaro.org
Signed-off-by: Tony Lindgren t...@atomide.com
---
 drivers/mmc/host/omap_hsmmc.c |   93 +
 1 file changed, 84 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 7e28501..8ca08fb 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -185,6 +185,8 @@ struct omap_hsmmc_host {
int req_in_progress;
struct omap_hsmmc_next  next_data;
boolsdio_irq_en;
+   struct pinctrl  *pinctrl;
+   struct pinctrl_state*fixed, *active, *idle;
boolactive_pinmux;
struct  omap_mmc_platform_data  *pdata;
 };
@@ -473,6 +475,67 @@ static void omap_hsmmc_gpio_free(struct 
omap_mmc_platform_data *pdata)
gpio_free(pdata-slots[0].gpio_cirq);
 }
 
+static int omap_hsmmc_pin_init(struct omap_hsmmc_host *host)
+{
+   int ret;
+
+   host-pinctrl = devm_pinctrl_get(host-dev);
+   if (IS_ERR(host-pinctrl)) {
+   dev_dbg(host-dev, no pinctrl handle\n);
+   ret = 0;
+   goto out;
+   }
+
+   host-fixed = pinctrl_lookup_state(host-pinctrl,
+  PINCTRL_STATE_DEFAULT);
+   if (IS_ERR(host-fixed)) {
+   dev_dbg(host-dev,
+pins are not configured from the driver\n);
+   host-fixed = NULL;
+   ret = 0;
+   goto out;
+   }
+
+   ret = pinctrl_select_state(host-pinctrl, host-fixed);
+   if (ret  0)
+   goto err;
+
+   /* For most cases we don't have wake-ups, and exit after this */
+   host-active = pinctrl_lookup_state(host-pinctrl, active);
+   if (IS_ERR(host-active)) {
+   ret = PTR_ERR(host-active);
+   host-active = NULL;
+   return 0;
+   }
+
+   host-idle = pinctrl_lookup_state(host-pinctrl,
+ PINCTRL_STATE_IDLE);
+   if (IS_ERR(host-idle)) {
+   ret = PTR_ERR(host-idle);
+   host-idle = NULL;
+   goto err;
+   }
+
+   /* Let's make sure the active and idle states work */
+   ret = pinctrl_select_state(host-pinctrl, host-idle);
+   if (ret  0)
+   goto err;
+
+   ret = pinctrl_select_state(host-pinctrl, host-active);
+   if (ret  0)
+   goto err;
+
+   dev_info(mmc_dev(host-mmc), pins configured for wake-up events\n);
+
+   return 0;
+
+err:
+   dev_err(mmc_dev(host-mmc), pins configuration error: %i\n, ret);
+
+out:
+   return ret;
+}
+
 /*
  * Start clock to the card
  */
@@ -1854,7 +1917,6 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
const struct of_device_id *match;
dma_cap_mask_t mask;
unsigned tx_req, rx_req;
-   struct pinctrl *pinctrl;
 
match = of_match_device(of_match_ptr(omap_mmc_of_match), pdev-dev);
if (match) {
@@ -2086,21 +2148,19 @@ static int omap_hsmmc_probe(struct platform_device 
*pdev)
 
omap_hsmmc_disable_irq(host);
 
-   pinctrl = devm_pinctrl_get_select_default(pdev-dev);
-   if (IS_ERR(pinctrl))
-   dev_warn(pdev-dev,
-   pins are not configured from the driver\n);
-
/*
-* For now, only support SDIO interrupt if we are doing
-* muxing of dat1 when booted with DT. This is because the
+* For now, only support SDIO interrupt if we are doing dynamic
+* remuxing of dat1 when booted with DT. This is because the
 * supposedly the wake-up events for CTPL don't work from deeper
 * idle states. And we don't want to add new legacy mux platform
 * init code callbacks any longer as we are moving to DT based
 * booting anyways.
 */
if (match) {
-   if (!IS_ERR(pinctrl)  mmc_slot(host).sdio_irq)
+   ret = omap_hsmmc_pin_init(host);
+   if (ret)
+   goto err_pinctrl_state;
+   else if (host-idle  mmc_slot(host).sdio_irq)
mmc-caps |= MMC_CAP_SDIO_IRQ;
}
 
@@ -2128,6 +2188,8 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 
 err_slot_name:
mmc_remove_host(mmc);
+err_pinctrl_state:
+   devm_pinctrl_put(host-pinctrl);
if ((mmc_slot(host).sdio_irq))
free_irq(mmc_slot(host).sdio_irq, host);
 err_irq_sdio:
@@ -2185,6 +2247,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
dma_release_channel(host-tx_chan);
if (host-rx_chan)