[PATCH] glamo-mci: Make use of mmc_host_enable/disable instead of custom timer.

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] glamo-mci: Make use of mmc_host_enable/disable instead of custom timer.

Thibaut Girka
A custom timer was used to suspend the mmc host when idle.
MMC core now provides a generic way to do that.
---
 drivers/mfd/glamo/glamo-mci.c |   70 ++++++++++++++++++++--------------------
 1 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/mfd/glamo/glamo-mci.c b/drivers/mfd/glamo/glamo-mci.c
index 1d3f9af..08d0154 100644
--- a/drivers/mfd/glamo/glamo-mci.c
+++ b/drivers/mfd/glamo/glamo-mci.c
@@ -52,8 +52,6 @@ struct glamo_mci_host {
 
  unsigned char request_counter;
 
- struct timer_list disable_timer;
-
  struct work_struct read_work;
 
  unsigned clk_enabled:1;
@@ -156,22 +154,18 @@ static void glamo_mci_reset(struct glamo_mci_host *host)
 
 }
 
-static void glamo_mci_clock_disable(struct glamo_mci_host *host)
-{
-    glamo_engine_suspend(host->core, GLAMO_ENGINE_MMC);
-}
-
-static void glamo_mci_clock_enable(struct glamo_mci_host *host)
+static int glamo_mci_clock_disable(struct mmc_host *mmc, int lazy)
 {
- del_timer_sync(&host->disable_timer);
-
-    glamo_engine_enable(host->core, GLAMO_ENGINE_MMC);
+ struct glamo_mci_host *host = mmc_priv(mmc);
+ glamo_engine_suspend(host->core, GLAMO_ENGINE_MMC);
+ return 0;
 }
 
-static void glamo_mci_disable_timer(unsigned long data)
+static int glamo_mci_clock_enable(struct mmc_host *mmc)
 {
- struct glamo_mci_host *host = (struct glamo_mci_host *)data;
- glamo_mci_clock_disable(host);
+ struct glamo_mci_host *host = mmc_priv(mmc);
+ glamo_engine_enable(host->core, GLAMO_ENGINE_MMC);
+ return 0;
 }
 
 
@@ -220,13 +214,9 @@ static int glamo_mci_set_card_clock(struct glamo_mci_host *host, int freq)
 {
  int real_rate = 0;
 
- if (freq) {
- glamo_mci_clock_enable(host);
+ if (freq)
  real_rate = glamo_engine_reclock(host->core, GLAMO_ENGINE_MMC,
- freq);
- } else {
- glamo_mci_clock_disable(host);
- }
+ freq);
 
  return real_rate;
 }
@@ -251,7 +241,6 @@ static int glamo_mci_wait_idle(struct glamo_mci_host *host,
 static void glamo_mci_request_done(struct glamo_mci_host *host, struct
 mmc_request *mrq)
 {
- mod_timer(&host->disable_timer, jiffies + HZ / 16);
  mmc_request_done(host->mmc, mrq);
 }
 
@@ -584,7 +573,6 @@ static void glamo_mci_send_request(struct mmc_host *mmc,
  struct glamo_mci_host *host = mmc_priv(mmc);
  struct mmc_command *cmd = mrq->cmd;
 
- glamo_mci_clock_enable(host);
  host->request_counter++;
  if (cmd->data) {
  if (glamo_mci_prepare_pio(host, cmd->data)) {
@@ -643,8 +631,6 @@ static void glamo_mci_set_power_mode(struct glamo_mci_host *host,
  break;
  case MMC_POWER_OFF:
  default:
- glamo_engine_disable(host->core, GLAMO_ENGINE_MMC);
-
  ret = regulator_disable(host->regulator);
  if (ret)
  dev_warn(&host->pdev->dev,
@@ -663,6 +649,8 @@ static void glamo_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
  int sd_drive;
  int ret;
 
+ mmc_host_enable(mmc);
+
  /* Set power */
  glamo_mci_set_power_mode(host, ios->power_mode);
 
@@ -674,6 +662,7 @@ static void glamo_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
  else
  host->vdd = ios->vdd;
  }
+
  rate = glamo_mci_set_card_clock(host, ios->clock);
 
  if ((ios->power_mode == MMC_POWER_ON) ||
@@ -698,6 +687,11 @@ static void glamo_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
  glamo_reg_set_bit_mask(host, GLAMO_REG_MMC_BASIC,
  GLAMO_BASIC_MMC_EN_4BIT_DATA | 0xb0,
    bus_width | sd_drive << 6);
+
+ if (host->power_mode == MMC_POWER_OFF)
+ mmc_host_disable(host->mmc);
+ else
+ mmc_host_lazy_disable(host->mmc);
 }
 
 
@@ -710,6 +704,8 @@ static int glamo_mci_get_ro(struct mmc_host *mmc)
 }
 
 static struct mmc_host_ops glamo_mci_ops = {
+ .enable = glamo_mci_clock_enable,
+ .disable = glamo_mci_clock_disable,
  .request = glamo_mci_send_request,
  .set_ios = glamo_mci_set_ios,
  .get_ro = glamo_mci_get_ro,
@@ -839,12 +835,12 @@ static int glamo_mci_probe(struct platform_device *pdev)
 
  platform_set_drvdata(pdev, mmc);
 
- glamo_engine_enable(host->core, GLAMO_ENGINE_MMC);
- glamo_mci_reset(host);
- glamo_engine_disable(host->core, GLAMO_ENGINE_MMC);
+ mmc->caps |= MMC_CAP_DISABLE;
+ mmc_set_disable_delay(mmc, 1000 / 16);
 
- setup_timer(&host->disable_timer, glamo_mci_disable_timer,
- (unsigned long)host);
+ mmc_host_enable(mmc);
+ glamo_mci_reset(host);
+ mmc_host_lazy_disable(mmc);
 
  ret = mmc_add_host(mmc);
  if (ret) {
@@ -881,7 +877,10 @@ static int glamo_mci_remove(struct platform_device *pdev)
 
  free_irq(host->irq, host);
 
+ mmc_host_enable(mmc);
  mmc_remove_host(mmc);
+ mmc_host_disable(mmc);
+
  iounmap(host->mmio_base);
  iounmap(host->data_base);
  release_mem_region(host->mmio_mem->start,
@@ -892,8 +891,6 @@ static int glamo_mci_remove(struct platform_device *pdev)
  regulator_put(host->regulator);
 
  mmc_free_host(mmc);
-
- glamo_engine_disable(host->core, GLAMO_ENGINE_MMC);
  return 0;
 }
 
@@ -908,7 +905,9 @@ static int glamo_mci_suspend(struct device *dev)
 
  disable_irq(host->irq);
 
+ mmc_host_enable(mmc);
  ret = mmc_suspend_host(mmc, PMSG_SUSPEND);
+ mmc_host_disable(mmc);
 
  return ret;
 }
@@ -919,16 +918,17 @@ static int glamo_mci_resume(struct device *dev)
  struct glamo_mci_host *host = mmc_priv(mmc);
  int ret;
 
-
+ mmc_host_enable(mmc);
  glamo_mci_reset(host);
- glamo_engine_enable(host->core, GLAMO_ENGINE_MMC);
-    mdelay(10);
+ mdelay(10);
 
  ret = mmc_resume_host(host->mmc);
 
  enable_irq(host->irq);
 
- return 0;
+ mmc_host_lazy_disable(host->mmc);
+
+ return ret;
 }
 
 static struct dev_pm_ops glamo_mci_pm_ops = {
--
1.7.1


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] glamo-mci: Make use of mmc_host_enable/disable instead of custom timer.

Thibaut Girka
Here is a few comments on the patch, explaining why there is quite a lot
of mmc_host_{enable,disable} calls:

> @@ -839,12 +835,12 @@ static int glamo_mci_probe(struct platform_device *pdev)
>  
>   platform_set_drvdata(pdev, mmc);
>  
> - glamo_engine_enable(host->core, GLAMO_ENGINE_MMC);
> - glamo_mci_reset(host);
> - glamo_engine_disable(host->core, GLAMO_ENGINE_MMC);
> + mmc->caps |= MMC_CAP_DISABLE;
> + mmc_set_disable_delay(mmc, 1000 / 16);
>  
> - setup_timer(&host->disable_timer, glamo_mci_disable_timer,
> - (unsigned long)host);
> + mmc_host_enable(mmc);
> + glamo_mci_reset(host);
> + mmc_host_lazy_disable(mmc);
> [...]
> @@ -919,16 +918,17 @@ static int glamo_mci_resume(struct device *dev)
> struct glamo_mci_host *host = mmc_priv(mmc);
>   int ret;
>  
> -
> + mmc_host_enable(mmc);
>   glamo_mci_reset(host);
> - glamo_engine_enable(host->core, GLAMO_ENGINE_MMC);
> > [...]
- return 0;
> + mmc_host_lazy_disable(host->mmc);

In those two cases, we are calling glamo_mci_reset, so, the engine
should be enabled, which is done with mmc_host_enable.


> @@ -881,7 +877,10 @@ static int glamo_mci_remove(struct platform_device *pdev)
>  
>   free_irq(host->irq, host);
>  
> + mmc_host_enable(mmc);
>   mmc_remove_host(mmc);
> + mmc_host_disable(mmc);
> +
> [...]
> @@ -908,7 +905,9 @@ static int glamo_mci_suspend(struct device *dev)
>  
>   disable_irq(host->irq);
>  
> + mmc_host_enable(mmc);
>   ret = mmc_suspend_host(mmc, PMSG_SUSPEND);
> + mmc_host_disable(mmc);
In these two other cases, we are calling mmc_{suspend,remove}_host,
which are calling mmc_stop_host, eventually calling
mmc_host_lazy_disable from mmc_release_host.
This schedules a delayed work, but we want to disable the engine right
now and dismiss any delayed work, since the device won't be available,
that's why we are using mmc_host_disable here.


signature.asc (205 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/5] glamo-mci: safer probe

Thibaut Girka
In reply to this post by Thibaut Girka
Calls mmc_host_disable instead of mmc_host_lazy_disable if an error occurs,
thus avoiding to schedule a delayed work that would end up using free'd objects.
---
 drivers/mfd/glamo/glamo-mci.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/glamo/glamo-mci.c b/drivers/mfd/glamo/glamo-mci.c
index 08d0154..c0c58f1 100644
--- a/drivers/mfd/glamo/glamo-mci.c
+++ b/drivers/mfd/glamo/glamo-mci.c
@@ -840,16 +840,19 @@ static int glamo_mci_probe(struct platform_device *pdev)
 
  mmc_host_enable(mmc);
  glamo_mci_reset(host);
- mmc_host_lazy_disable(mmc);
 
  ret = mmc_add_host(mmc);
  if (ret) {
  dev_err(&pdev->dev, "failed to add mmc host.\n");
- goto probe_freeirq;
+ goto probe_mmc_host_disable;
  }
 
+ mmc_host_lazy_disable(mmc);
+
  return 0;
 
+probe_mmc_host_disable:
+ mmc_host_disable(mmc);
 probe_freeirq:
  free_irq(host->irq, host);
 probe_iounmap_data:
@@ -891,6 +894,7 @@ static int glamo_mci_remove(struct platform_device *pdev)
  regulator_put(host->regulator);
 
  mmc_free_host(mmc);
+
  return 0;
 }
 
--
1.7.1


Reply | Threaded
Open this post in threaded view
|

[PATCH 2/5] glamo-mci: free irq after removing host, not before

Thibaut Girka
In reply to this post by Thibaut Girka
---
 drivers/mfd/glamo/glamo-mci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/glamo/glamo-mci.c b/drivers/mfd/glamo/glamo-mci.c
index c0c58f1..f44dfc8 100644
--- a/drivers/mfd/glamo/glamo-mci.c
+++ b/drivers/mfd/glamo/glamo-mci.c
@@ -878,12 +878,13 @@ static int glamo_mci_remove(struct platform_device *pdev)
  struct mmc_host *mmc = platform_get_drvdata(pdev);
  struct glamo_mci_host *host = mmc_priv(mmc);
 
- free_irq(host->irq, host);
-
  mmc_host_enable(mmc);
  mmc_remove_host(mmc);
  mmc_host_disable(mmc);
 
+ synchronize_irq(host->irq);
+ free_irq(host->irq, host);
+
  iounmap(host->mmio_base);
  iounmap(host->data_base);
  release_mem_region(host->mmio_mem->start,
--
1.7.1


Reply | Threaded
Open this post in threaded view
|

[PATCH 3/5] glamo: fixed some style/indentation issues

Thibaut Girka
In reply to this post by Thibaut Girka
---
 drivers/mfd/glamo/glamo-core.c |    4 +-
 drivers/mfd/glamo/glamo-fb.c   |    2 +-
 drivers/mfd/glamo/glamo-mci.c  |   61 +++++++++++++++++++--------------------
 3 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/drivers/mfd/glamo/glamo-core.c b/drivers/mfd/glamo/glamo-core.c
index 23073fe..4c8b7ea 100644
--- a/drivers/mfd/glamo/glamo-core.c
+++ b/drivers/mfd/glamo/glamo-core.c
@@ -793,7 +793,7 @@ static int glamo_run_script(struct glamo_core *glamo,
 static const struct glamo_script glamo_init_script[] = {
  { GLAMO_REG_CLOCK_HOST, 0x1000 },
  { GLAMO_SCRIPT_WAIT,     2 },
- { GLAMO_REG_CLOCK_MEMORY, 0x1000 },
+ { GLAMO_REG_CLOCK_MEMORY, 0x1000 },
  { GLAMO_REG_CLOCK_MEMORY, 0x2000 },
  { GLAMO_REG_CLOCK_LCD, 0x1000 },
  { GLAMO_REG_CLOCK_MMC, 0x1000 },
@@ -1247,7 +1247,7 @@ static int glamo_resume(struct device *dev)
  return 0;
 }
 
-static struct dev_pm_ops glamo_pm_ops = {
+static const struct dev_pm_ops glamo_pm_ops = {
  .suspend    = glamo_suspend,
  .resume     = glamo_resume,
  .poweroff   = glamo_suspend,
diff --git a/drivers/mfd/glamo/glamo-fb.c b/drivers/mfd/glamo/glamo-fb.c
index cffb180..29c16f0 100644
--- a/drivers/mfd/glamo/glamo-fb.c
+++ b/drivers/mfd/glamo/glamo-fb.c
@@ -943,7 +943,7 @@ static int glamofb_resume(struct device *dev)
  return 0;
 }
 
-static struct dev_pm_ops glamofb_pm_ops = {
+static const struct dev_pm_ops glamofb_pm_ops = {
  .suspend = glamofb_suspend,
  .resume = glamofb_resume,
 };
diff --git a/drivers/mfd/glamo/glamo-mci.c b/drivers/mfd/glamo/glamo-mci.c
index f44dfc8..94ebec9 100644
--- a/drivers/mfd/glamo/glamo-mci.c
+++ b/drivers/mfd/glamo/glamo-mci.c
@@ -58,9 +58,9 @@ struct glamo_mci_host {
 };
 
 static void glamo_mci_send_request(struct mmc_host *mmc,
-    struct mmc_request *mrq);
+   struct mmc_request *mrq);
 static void glamo_mci_send_command(struct glamo_mci_host *host,
-    struct mmc_command *cmd);
+   struct mmc_command *cmd);
 
 /*
  * Max SD clock rate
@@ -119,14 +119,14 @@ static inline void glamo_reg_write(struct glamo_mci_host *glamo,
 }
 
 static inline uint16_t glamo_reg_read(struct glamo_mci_host *glamo,
-   uint16_t reg)
+      uint16_t reg)
 {
  return readw(glamo->mmio_base + reg);
 }
 
 static void glamo_reg_set_bit_mask(struct glamo_mci_host *glamo,
- uint16_t reg, uint16_t mask,
- uint16_t val)
+   uint16_t reg, uint16_t mask,
+   uint16_t val)
 {
  uint16_t tmp;
 
@@ -222,7 +222,7 @@ static int glamo_mci_set_card_clock(struct glamo_mci_host *host, int freq)
 }
 
 static int glamo_mci_wait_idle(struct glamo_mci_host *host,
- unsigned long timeout)
+       unsigned long timeout)
 {
  uint16_t status;
  do {
@@ -238,8 +238,8 @@ static int glamo_mci_wait_idle(struct glamo_mci_host *host,
  return 0;
 }
 
-static void glamo_mci_request_done(struct glamo_mci_host *host, struct
-mmc_request *mrq)
+static void glamo_mci_request_done(struct glamo_mci_host *host,
+   struct mmc_request *mrq)
 {
  mmc_request_done(host->mmc, mrq);
 }
@@ -359,7 +359,7 @@ done:
 }
 
 static void glamo_mci_send_command(struct glamo_mci_host *host,
-  struct mmc_command *cmd)
+   struct mmc_command *cmd)
 {
  uint8_t u8a[6];
  uint16_t fire = 0;
@@ -509,14 +509,13 @@ static void glamo_mci_send_command(struct glamo_mci_host *host,
  status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
  while (((((status >> 15) & 1) != (host->request_counter & 1)) ||
  (!(status & (GLAMO_STAT1_MMC_RB_RRDY |
- GLAMO_STAT1_MMC_RTOUT |
- GLAMO_STAT1_MMC_DTOUT |
- GLAMO_STAT1_MMC_BWERR |
- GLAMO_STAT1_MMC_BRERR)))) && (timeout--));
-
- if ((status & (GLAMO_STAT1_MMC_RTOUT |
-   GLAMO_STAT1_MMC_DTOUT)) ||
- (timeout == 0)) {
+     GLAMO_STAT1_MMC_RTOUT |
+     GLAMO_STAT1_MMC_DTOUT |
+     GLAMO_STAT1_MMC_BWERR |
+     GLAMO_STAT1_MMC_BRERR)))) && (timeout--));
+
+ if ((status & (GLAMO_STAT1_MMC_RTOUT | GLAMO_STAT1_MMC_DTOUT)) ||
+    (timeout == 0)) {
  cmd->error = -ETIMEDOUT;
  } else if (status & (GLAMO_STAT1_MMC_BWERR | GLAMO_STAT1_MMC_BRERR)) {
  cmd->error = -EILSEQ;
@@ -568,7 +567,7 @@ static int glamo_mci_prepare_pio(struct glamo_mci_host *host,
 }
 
 static void glamo_mci_send_request(struct mmc_host *mmc,
- struct mmc_request *mrq)
+   struct mmc_request *mrq)
 {
  struct glamo_mci_host *host = mmc_priv(mmc);
  struct mmc_command *cmd = mrq->cmd;
@@ -610,7 +609,7 @@ done:
 }
 
 static void glamo_mci_set_power_mode(struct glamo_mci_host *host,
- unsigned char power_mode)
+     unsigned char power_mode)
 {
  int ret;
 
@@ -754,8 +753,8 @@ static int glamo_mci_probe(struct platform_device *pdev)
  }
 
  host->mmio_mem = request_mem_region(host->mmio_mem->start,
- resource_size(host->mmio_mem),
- pdev->name);
+    resource_size(host->mmio_mem),
+    pdev->name);
 
  if (!host->mmio_mem) {
  dev_err(&pdev->dev, "failed to request io memory region.\n");
@@ -764,7 +763,7 @@ static int glamo_mci_probe(struct platform_device *pdev)
  }
 
  host->mmio_base = ioremap(host->mmio_mem->start,
-      resource_size(host->mmio_mem));
+  resource_size(host->mmio_mem));
  if (!host->mmio_base) {
  dev_err(&pdev->dev, "failed to ioremap() io memory region.\n");
  ret = -EINVAL;
@@ -782,8 +781,8 @@ static int glamo_mci_probe(struct platform_device *pdev)
  }
 
  host->data_mem = request_mem_region(host->data_mem->start,
- resource_size(host->data_mem),
- pdev->name);
+    resource_size(host->data_mem),
+    pdev->name);
 
  if (!host->data_mem) {
  dev_err(&pdev->dev, "failed to request io memory region.\n");
@@ -791,7 +790,7 @@ static int glamo_mci_probe(struct platform_device *pdev)
  goto probe_iounmap_mmio;
  }
  host->data_base = ioremap(host->data_mem->start,
-      resource_size(host->data_mem));
+  resource_size(host->data_mem));
 
  if (host->data_base == 0) {
  dev_err(&pdev->dev, "failed to ioremap() io memory region.\n");
@@ -800,7 +799,7 @@ static int glamo_mci_probe(struct platform_device *pdev)
  }
 
  ret = request_threaded_irq(host->irq, NULL, glamo_mci_irq, IRQF_SHARED,
-   pdev->name, host);
+   pdev->name, host);
  if (ret) {
  dev_err(&pdev->dev, "failed to register irq.\n");
  goto probe_iounmap_data;
@@ -859,12 +858,12 @@ probe_iounmap_data:
  iounmap(host->data_base);
 probe_free_mem_region_data:
  release_mem_region(host->data_mem->start,
- resource_size(host->data_mem));
+   resource_size(host->data_mem));
 probe_iounmap_mmio:
  iounmap(host->mmio_base);
 probe_free_mem_region_mmio:
  release_mem_region(host->mmio_mem->start,
- resource_size(host->mmio_mem));
+   resource_size(host->mmio_mem));
 probe_regulator_put:
  regulator_put(host->regulator);
 probe_free_host:
@@ -936,11 +935,11 @@ static int glamo_mci_resume(struct device *dev)
  return ret;
 }
 
-static struct dev_pm_ops glamo_mci_pm_ops = {
+static const struct dev_pm_ops glamo_mci_pm_ops = {
  .suspend    = glamo_mci_suspend,
  .resume     = glamo_mci_resume,
-    .freeze     = glamo_mci_suspend,
-    .thaw       = glamo_mci_resume,
+ .freeze     = glamo_mci_suspend,
+ .thaw       = glamo_mci_resume,
 };
 #define GLAMO_MCI_PM_OPS (&glamo_mci_pm_ops)
 
--
1.7.1


Reply | Threaded
Open this post in threaded view
|

[PATCH 4/5] glamo-mci: Make the read_worker work in its own thread

Thibaut Girka
In reply to this post by Thibaut Girka
---
 drivers/mfd/glamo/glamo-mci.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/glamo/glamo-mci.c b/drivers/mfd/glamo/glamo-mci.c
index 94ebec9..f49bdde 100644
--- a/drivers/mfd/glamo/glamo-mci.c
+++ b/drivers/mfd/glamo/glamo-mci.c
@@ -52,9 +52,8 @@ struct glamo_mci_host {
 
  unsigned char request_counter;
 
+ struct workqueue_struct *workqueue;
  struct work_struct read_work;
-
- unsigned clk_enabled:1;
 };
 
 static void glamo_mci_send_request(struct mmc_host *mmc,
@@ -259,7 +258,7 @@ static irqreturn_t glamo_mci_irq(int irq, void *data)
 
 #if 0
  if (cmd->data->flags & MMC_DATA_READ)
- return;
+ return IRQ_HANDLED;
 #endif
 
  status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
@@ -283,8 +282,10 @@ static irqreturn_t glamo_mci_irq(int irq, void *data)
  if (mrq->stop)
  glamo_mci_send_command(host, mrq->stop);
 
+#if 1
  if (cmd->data->flags & MMC_DATA_READ)
  do_pio_read(host, cmd->data);
+#endif
 
  if (mrq->stop)
  mrq->stop->error = glamo_mci_wait_idle(host, jiffies + HZ);
@@ -313,6 +314,7 @@ static void glamo_mci_read_worker(struct work_struct *work)
  cmd = host->mrq->cmd;
  sg = cmd->data->sg;
  do {
+ /* TODO: How to get rid of that? */
  status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
 
  if (status & (GLAMO_STAT1_MMC_RTOUT | GLAMO_STAT1_MMC_DTOUT))
@@ -337,12 +339,15 @@ static void glamo_mci_read_worker(struct work_struct *work)
  from_ptr += sg->length >> 1;
 
  data_read += sg->length;
+
  sg = sg_next(sg);
  }
 
  } while (sg);
  cmd->data->bytes_xfered = data_read;
 
+ /* TODO: Delete everything from now on, and let the IRQ handler do it? */
+
  do {
  status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
  } while (!(status & GLAMO_STAT1_MMC_IDLE));
@@ -350,6 +355,7 @@ static void glamo_mci_read_worker(struct work_struct *work)
  if (host->mrq->stop)
  glamo_mci_send_command(host, host->mrq->stop);
 
+ /* TODO: Replace by glamo_mci_wait_idle? */
  do {
  status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
  } while (!(status & GLAMO_STAT1_MMC_IDLE));
@@ -543,7 +549,7 @@ static void glamo_mci_send_command(struct glamo_mci_host *host,
    By starting to copy data when it's avaiable we can increase
    throughput by up to 30%. */
  if (cmd->data && (cmd->data->flags & MMC_DATA_READ))
- schedule_work(&host->read_work);
+ queue_work(host->workqueue, &host->read_work);
 #endif
 
 }
@@ -731,11 +737,11 @@ static int glamo_mci_probe(struct platform_device *pdev)
  if (core->pdata)
  host->pdata = core->pdata->mmc_data;
  host->power_mode = MMC_POWER_OFF;
- host->clk_enabled = 0;
  host->core = core;
  host->irq = platform_get_irq(pdev, 0);
 
  INIT_WORK(&host->read_work, glamo_mci_read_worker);
+ host->workqueue = create_singlethread_workqueue("glamo-mci-read");
 
  host->regulator = regulator_get(pdev->dev.parent, "SD_3V3");
  if (IS_ERR(host->regulator)) {
@@ -852,7 +858,6 @@ static int glamo_mci_probe(struct platform_device *pdev)
 
 probe_mmc_host_disable:
  mmc_host_disable(mmc);
-probe_freeirq:
  free_irq(host->irq, host);
 probe_iounmap_data:
  iounmap(host->data_base);
@@ -867,6 +872,7 @@ probe_free_mem_region_mmio:
 probe_regulator_put:
  regulator_put(host->regulator);
 probe_free_host:
+ destroy_workqueue(host->workqueue);
  mmc_free_host(mmc);
 probe_out:
  return ret;
@@ -877,6 +883,9 @@ static int glamo_mci_remove(struct platform_device *pdev)
  struct mmc_host *mmc = platform_get_drvdata(pdev);
  struct glamo_mci_host *host = mmc_priv(mmc);
 
+ flush_workqueue(host->workqueue);
+ destroy_workqueue(host->workqueue);
+
  mmc_host_enable(mmc);
  mmc_remove_host(mmc);
  mmc_host_disable(mmc);
--
1.7.1


Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5] glamo-mci: eliminate some duplicated code in glamo_mci_read_worker

Thibaut Girka
In reply to this post by Thibaut Girka
---
 drivers/mfd/glamo/glamo-mci.c |   48 ++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/mfd/glamo/glamo-mci.c b/drivers/mfd/glamo/glamo-mci.c
index f49bdde..63728c5 100644
--- a/drivers/mfd/glamo/glamo-mci.c
+++ b/drivers/mfd/glamo/glamo-mci.c
@@ -168,6 +168,7 @@ static int glamo_mci_clock_enable(struct mmc_host *mmc)
 }
 
 
+#ifndef GLAMO_MCI_WORKER
 static void do_pio_read(struct glamo_mci_host *host, struct mmc_data *data)
 {
  struct sg_mapping_iter miter;
@@ -189,6 +190,7 @@ static void do_pio_read(struct glamo_mci_host *host, struct mmc_data *data)
  dev_dbg(&host->pdev->dev, "pio_read(): "
  "complete (no more data).\n");
 }
+#endif
 
 static void do_pio_write(struct glamo_mci_host *host, struct mmc_data *data)
 {
@@ -256,11 +258,6 @@ static irqreturn_t glamo_mci_irq(int irq, void *data)
  mrq = host->mrq;
  cmd = mrq->cmd;
 
-#if 0
- if (cmd->data->flags & MMC_DATA_READ)
- return IRQ_HANDLED;
-#endif
-
  status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
  dev_dbg(&host->pdev->dev, "status = 0x%04x\n", status);
 
@@ -282,9 +279,11 @@ static irqreturn_t glamo_mci_irq(int irq, void *data)
  if (mrq->stop)
  glamo_mci_send_command(host, mrq->stop);
 
-#if 1
  if (cmd->data->flags & MMC_DATA_READ)
+#ifndef GLAMO_MCI_WORKER
  do_pio_read(host, cmd->data);
+#else
+ flush_workqueue(host->workqueue);
 #endif
 
  if (mrq->stop)
@@ -297,6 +296,7 @@ done:
  return IRQ_HANDLED;
 }
 
+#ifdef GLAMO_MCI_WORKER
 static void glamo_mci_read_worker(struct work_struct *work)
 {
  struct glamo_mci_host *host = container_of(work, struct glamo_mci_host,
@@ -314,7 +314,13 @@ static void glamo_mci_read_worker(struct work_struct *work)
  cmd = host->mrq->cmd;
  sg = cmd->data->sg;
  do {
- /* TODO: How to get rid of that? */
+ /*
+ * TODO: How to get rid of that?
+ * Maybe just drop it... In fact, it is already handled in
+ * the IRQ handler, maybe we should only check cmd->error.
+ * But the question is: what happens between the moment
+ * the error occurs, and the moment the IRQ handler handles it?
+ */
  status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
 
  if (status & (GLAMO_STAT1_MMC_RTOUT | GLAMO_STAT1_MMC_DTOUT))
@@ -324,7 +330,7 @@ static void glamo_mci_read_worker(struct work_struct *work)
  if (cmd->error) {
  dev_info(&host->pdev->dev, "Error after cmd: 0x%x\n",
  status);
- goto done;
+ return;
  }
 
  blocks_ready = glamo_reg_read(host, GLAMO_REG_MMC_RB_BLKCNT);
@@ -345,24 +351,8 @@ static void glamo_mci_read_worker(struct work_struct *work)
 
  } while (sg);
  cmd->data->bytes_xfered = data_read;
-
- /* TODO: Delete everything from now on, and let the IRQ handler do it? */
-
- do {
- status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
- } while (!(status & GLAMO_STAT1_MMC_IDLE));
-
- if (host->mrq->stop)
- glamo_mci_send_command(host, host->mrq->stop);
-
- /* TODO: Replace by glamo_mci_wait_idle? */
- do {
- status = glamo_reg_read(host, GLAMO_REG_MMC_RB_STAT1);
- } while (!(status & GLAMO_STAT1_MMC_IDLE));
-done:
- host->mrq = NULL;
- glamo_mci_request_done(host, cmd->mrq);
 }
+#endif
 
 static void glamo_mci_send_command(struct glamo_mci_host *host,
    struct mmc_command *cmd)
@@ -544,7 +534,7 @@ static void glamo_mci_send_command(struct glamo_mci_host *host,
  }
  }
 
-#if 0
+#ifdef GLAMO_MCI_WORKER
  /* We'll only get an interrupt when all data has been transfered.
    By starting to copy data when it's avaiable we can increase
    throughput by up to 30%. */
@@ -740,8 +730,10 @@ static int glamo_mci_probe(struct platform_device *pdev)
  host->core = core;
  host->irq = platform_get_irq(pdev, 0);
 
+#ifdef GLAMO_MCI_WORKER
  INIT_WORK(&host->read_work, glamo_mci_read_worker);
  host->workqueue = create_singlethread_workqueue("glamo-mci-read");
+#endif
 
  host->regulator = regulator_get(pdev->dev.parent, "SD_3V3");
  if (IS_ERR(host->regulator)) {
@@ -872,7 +864,9 @@ probe_free_mem_region_mmio:
 probe_regulator_put:
  regulator_put(host->regulator);
 probe_free_host:
+#ifdef GLAMO_MCI_WORKER
  destroy_workqueue(host->workqueue);
+#endif
  mmc_free_host(mmc);
 probe_out:
  return ret;
@@ -883,8 +877,10 @@ static int glamo_mci_remove(struct platform_device *pdev)
  struct mmc_host *mmc = platform_get_drvdata(pdev);
  struct glamo_mci_host *host = mmc_priv(mmc);
 
+#ifdef GLAMO_MCI_WORKER
  flush_workqueue(host->workqueue);
  destroy_workqueue(host->workqueue);
+#endif
 
  mmc_host_enable(mmc);
  mmc_remove_host(mmc);
--
1.7.1


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/5] glamo-mci: free irq after removing host, not before

Lars-Peter Clausen
In reply to this post by Thibaut Girka
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Thibaut Girka wrote:
> ---
>  drivers/mfd/glamo/glamo-mci.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/glamo/glamo-mci.c b/drivers/mfd/glamo/glamo-mci.c
> index c0c58f1..f44dfc8 100644
> --- a/drivers/mfd/glamo/glamo-mci.c
> +++ b/drivers/mfd/glamo/glamo-mci.c
> @@ -878,12 +878,13 @@ static int glamo_mci_remove(struct
platform_device *pdev)

>      struct mmc_host    *mmc = platform_get_drvdata(pdev);
>      struct glamo_mci_host *host = mmc_priv(mmc);
>
> -    free_irq(host->irq, host);
> -
>      mmc_host_enable(mmc);
>      mmc_remove_host(mmc);
>      mmc_host_disable(mmc);
>
> +    synchronize_irq(host->irq);
> +    free_irq(host->irq, host);
> +
>      iounmap(host->mmio_base);
>      iounmap(host->data_base);
>      release_mem_region(host->mmio_mem->start,
Hi

IMO we have to free the IRQ before we free the host. The irq handler
uses the mmc_host struct and if it kicks in after the host has been
remove we'll work on invalid data.

- - Lars
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkwdI9oACgkQBX4mSR26RiOg7gCgijvWgM+R2lYRxD85VQCMPxjq
etgAn3a1zq21bfHd5yvJmXu8T6DJd3bW
=zxSY
-----END PGP SIGNATURE-----


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/5] glamo-mci: free irq after removing host, not before

Lars-Peter Clausen
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Lars-Peter Clausen wrote:

> Thibaut Girka wrote:
>> --- drivers/mfd/glamo/glamo-mci.c |    5 +++-- 1 files changed, 3
>> insertions(+), 2 deletions(-)
>
>> diff --git a/drivers/mfd/glamo/glamo-mci.c
> b/drivers/mfd/glamo/glamo-mci.c
>> index c0c58f1..f44dfc8 100644 --- a/drivers/mfd/glamo/glamo-mci.c
>>  +++ b/drivers/mfd/glamo/glamo-mci.c @@ -878,12 +878,13 @@ static
>> int glamo_mci_remove(struct
> platform_device *pdev)
>> struct mmc_host    *mmc = platform_get_drvdata(pdev); struct
>> glamo_mci_host *host = mmc_priv(mmc);
>
>> -    free_irq(host->irq, host); - mmc_host_enable(mmc);
>> mmc_remove_host(mmc); mmc_host_disable(mmc);
>
>> +    synchronize_irq(host->irq); +    free_irq(host->irq, host);
>> + iounmap(host->mmio_base); iounmap(host->data_base);
>> release_mem_region(host->mmio_mem->start,
> Hi
>
> IMO we have to free the IRQ before we free the host. The irq
> handler uses the mmc_host struct and if it kicks in after the host
> has been remove we'll work on invalid data.
>
> - Lars
Hm... never mind what i said. Your patch is ok...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkwdJYwACgkQBX4mSR26RiPzIgCdGq4Qe5fw/9ROsMnRrqU+/1q4
lKIAnjINjI/9cpi3nBbi3nJTZvg/Vpov
=+RFN
-----END PGP SIGNATURE-----


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] glamo-mci: Make use of mmc_host_enable/disable instead of custom timer.

Lars-Peter Clausen
In reply to this post by Thibaut Girka
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi

I applied all your patches, thanks :)
You sometime had unrelated changes, please try to avoid that in future
patches.

- - Lars

Thibaut Girka wrote:

> A custom timer was used to suspend the mmc host when idle. MMC core
> now provides a generic way to do that. ---
> drivers/mfd/glamo/glamo-mci.c |   70
> ++++++++++++++++++++-------------------- 1 files changed, 35
> insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mfd/glamo/glamo-mci.c
> b/drivers/mfd/glamo/glamo-mci.c index 1d3f9af..08d0154 100644 ---
> a/drivers/mfd/glamo/glamo-mci.c +++ b/drivers/mfd/glamo/glamo-mci.c
>  @@ -52,8 +52,6 @@ struct glamo_mci_host {
>
> unsigned char request_counter;
>
> -    struct timer_list disable_timer; - struct work_struct read_work;
>
> unsigned clk_enabled:1; @@ -156,22 +154,18 @@ static void
> glamo_mci_reset(struct glamo_mci_host *host)
>
> }
>
> -static void glamo_mci_clock_disable(struct glamo_mci_host *host)
> -{ -    glamo_engine_suspend(host->core, GLAMO_ENGINE_MMC); -} -
> -static void glamo_mci_clock_enable(struct glamo_mci_host *host)
> +static int glamo_mci_clock_disable(struct mmc_host *mmc, int lazy)
>  { -    del_timer_sync(&host->disable_timer); - -
> glamo_engine_enable(host->core, GLAMO_ENGINE_MMC); +    struct
> glamo_mci_host *host = mmc_priv(mmc); +
> glamo_engine_suspend(host->core, GLAMO_ENGINE_MMC); +    return 0; }
>
> -static void glamo_mci_disable_timer(unsigned long data) +static
> int glamo_mci_clock_enable(struct mmc_host *mmc) { -    struct
> glamo_mci_host *host = (struct glamo_mci_host *)data; -
> glamo_mci_clock_disable(host); +    struct glamo_mci_host *host =
> mmc_priv(mmc); +    glamo_engine_enable(host->core, GLAMO_ENGINE_MMC);
>  +    return 0; }
>
>
> @@ -220,13 +214,9 @@ static int glamo_mci_set_card_clock(struct
> glamo_mci_host *host, int freq) { int real_rate = 0;
>
> -    if (freq) { -        glamo_mci_clock_enable(host); +    if (freq)
> real_rate = glamo_engine_reclock(host->core, GLAMO_ENGINE_MMC, -
> freq); -    } else { -        glamo_mci_clock_disable(host); -    } +
> freq);
>
> return real_rate; } @@ -251,7 +241,6 @@ static int
> glamo_mci_wait_idle(struct glamo_mci_host *host, static void
> glamo_mci_request_done(struct glamo_mci_host *host, struct
> mmc_request *mrq) { -    mod_timer(&host->disable_timer, jiffies + HZ
> / 16); mmc_request_done(host->mmc, mrq); }
>
> @@ -584,7 +573,6 @@ static void glamo_mci_send_request(struct
> mmc_host *mmc, struct glamo_mci_host *host = mmc_priv(mmc); struct
> mmc_command *cmd = mrq->cmd;
>
> -    glamo_mci_clock_enable(host); host->request_counter++; if
> (cmd->data) { if (glamo_mci_prepare_pio(host, cmd->data)) { @@
> -643,8 +631,6 @@ static void glamo_mci_set_power_mode(struct
> glamo_mci_host *host, break; case MMC_POWER_OFF: default: -
> glamo_engine_disable(host->core, GLAMO_ENGINE_MMC); - ret =
> regulator_disable(host->regulator); if (ret)
> dev_warn(&host->pdev->dev, @@ -663,6 +649,8 @@ static void
> glamo_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) int
> sd_drive; int ret;
>
> +    mmc_host_enable(mmc); + /* Set power */
> glamo_mci_set_power_mode(host, ios->power_mode);
>
> @@ -674,6 +662,7 @@ static void glamo_mci_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios) else host->vdd = ios->vdd; } + rate =
> glamo_mci_set_card_clock(host, ios->clock);
>
> if ((ios->power_mode == MMC_POWER_ON) || @@ -698,6 +687,11 @@
> static void glamo_mci_set_ios(struct mmc_host *mmc, struct mmc_ios
> *ios) glamo_reg_set_bit_mask(host, GLAMO_REG_MMC_BASIC,
> GLAMO_BASIC_MMC_EN_4BIT_DATA | 0xb0, bus_width | sd_drive << 6); +
> +    if (host->power_mode == MMC_POWER_OFF) +
> mmc_host_disable(host->mmc); +    else +
> mmc_host_lazy_disable(host->mmc); }
>
>
> @@ -710,6 +704,8 @@ static int glamo_mci_get_ro(struct mmc_host
> *mmc) }
>
> static struct mmc_host_ops glamo_mci_ops = { +    .enable        =
> glamo_mci_clock_enable, +    .disable    = glamo_mci_clock_disable,
> .request    = glamo_mci_send_request, .set_ios    = glamo_mci_set_ios,
> .get_ro        = glamo_mci_get_ro, @@ -839,12 +835,12 @@ static int
> glamo_mci_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, mmc);
>
> -    glamo_engine_enable(host->core, GLAMO_ENGINE_MMC); -
> glamo_mci_reset(host); -    glamo_engine_disable(host->core,
> GLAMO_ENGINE_MMC); +    mmc->caps |= MMC_CAP_DISABLE; +
> mmc_set_disable_delay(mmc, 1000 / 16);
>
> -    setup_timer(&host->disable_timer, glamo_mci_disable_timer, -
> (unsigned long)host); +    mmc_host_enable(mmc); +
> glamo_mci_reset(host); +    mmc_host_lazy_disable(mmc);
>
> ret = mmc_add_host(mmc); if (ret) { @@ -881,7 +877,10 @@ static int
> glamo_mci_remove(struct platform_device *pdev)
>
> free_irq(host->irq, host);
>
> +    mmc_host_enable(mmc); mmc_remove_host(mmc); +
> mmc_host_disable(mmc); + iounmap(host->mmio_base);
> iounmap(host->data_base); release_mem_region(host->mmio_mem->start,
>  @@ -892,8 +891,6 @@ static int glamo_mci_remove(struct
> platform_device *pdev) regulator_put(host->regulator);
>
> mmc_free_host(mmc); - -    glamo_engine_disable(host->core,
> GLAMO_ENGINE_MMC); return 0; }
>
> @@ -908,7 +905,9 @@ static int glamo_mci_suspend(struct device
> *dev)
>
> disable_irq(host->irq);
>
> +    mmc_host_enable(mmc); ret = mmc_suspend_host(mmc, PMSG_SUSPEND);
> +    mmc_host_disable(mmc);
>
> return ret; } @@ -919,16 +918,17 @@ static int
> glamo_mci_resume(struct device *dev) struct glamo_mci_host *host =
> mmc_priv(mmc); int ret;
>
> - +    mmc_host_enable(mmc); glamo_mci_reset(host); -
> glamo_engine_enable(host->core, GLAMO_ENGINE_MMC); -    mdelay(10);
>  +    mdelay(10);
>
> ret = mmc_resume_host(host->mmc);
>
> enable_irq(host->irq);
>
> -    return 0; +    mmc_host_lazy_disable(host->mmc); + +    return ret; }
>
> static struct dev_pm_ops glamo_mci_pm_ops = {

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkwdMuEACgkQBX4mSR26RiPOpACfXfccWB9NKvDnLi+f+7oNZD6u
C70AnR9d1lfs1aXqClw9FAm5x9iCsGeC
=//mc
-----END PGP SIGNATURE-----