Re: [PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

2012-09-09 Thread S, Venkatraman 
On Sat, Sep 8, 2012 at 3:29 AM, Kevin Hilman
khil...@deeprootsystems.com wrote:
 Venkatraman S svenk...@ti.com writes:

 omap hsmmc controller IP has a built in timer that can be programmed to
 guard against unresponsive operations. But its range is very narrow,
 and the maximum countable time is a few seconds.

 Card maintenance operations like BKOPS and MMC_ERASE and long
 stream writes like packed command require timers of order of
 several minutes, much beyond the capability of the IP timer.
 So get rid of using the IP timer entirely and use kernel's hrtimer
 functionality for guarding the device operations.
 As part of this change, a workaround that disabled timeouts for
 MMC_ERASE command is removed, and the arbitary timing of 100ms
 is used only when the timeout is not explicitly specified by core.

 A trivial change to get rid of unnecessary dealiasing of host-data
 in omap_hsmmc_do_irq is also included.

 Signed-off-by: Venkatraman S svenk...@ti.com

 Dumb question: if the timers needed are on the order of minutes, why do
 you need to use high-resolution timers?  I'm guessing the normal
 kernel-internal timers should suffice here (see linux/timer.h,
 init_timer().)

Both sub ms and long duration timers are needed, based on the type of operation.
The eMMC standard has evolved to define new commands which require
such long duration
operations. The short (control) commands do need high precision.

 Also, if they're on the order of minutes, I assume the actual firing
 doesn't have to be precise, so deferrable timers can probably be used
 (and thus help PM by coalescing wakeup events.)

 If my assumptions are true, it might be worth considering using the
 normal timers, and use init_timer_deferrable().

I'll certainly look into this. One way is to use the IP timer for
short commands and
deferrable timers for long ones - but I reckon it would be messy.

I've actually put this patch on hold - as someone pointed out to me that
this violates some sections of eMMC spec.


 Kevin
--
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: [PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

2012-09-07 Thread Kevin Hilman 
Venkatraman S svenk...@ti.com writes:

 omap hsmmc controller IP has a built in timer that can be programmed to
 guard against unresponsive operations. But its range is very narrow,
 and the maximum countable time is a few seconds.

 Card maintenance operations like BKOPS and MMC_ERASE and long
 stream writes like packed command require timers of order of
 several minutes, much beyond the capability of the IP timer.
 So get rid of using the IP timer entirely and use kernel's hrtimer
 functionality for guarding the device operations.
 As part of this change, a workaround that disabled timeouts for
 MMC_ERASE command is removed, and the arbitary timing of 100ms
 is used only when the timeout is not explicitly specified by core.

 A trivial change to get rid of unnecessary dealiasing of host-data
 in omap_hsmmc_do_irq is also included.

 Signed-off-by: Venkatraman S svenk...@ti.com

Dumb question: if the timers needed are on the order of minutes, why do
you need to use high-resolution timers?  I'm guessing the normal
kernel-internal timers should suffice here (see linux/timer.h,
init_timer().)

Also, if they're on the order of minutes, I assume the actual firing
doesn't have to be precise, so deferrable timers can probably be used
(and thus help PM by coalescing wakeup events.) 

If my assumptions are true, it might be worth considering using the
normal timers, and use init_timer_deferrable().

Kevin
--
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

[PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

2012-08-28 Thread Venkatraman S 
omap hsmmc controller IP has a built in timer that can be programmed to
guard against unresponsive operations. But its range is very narrow,
and the maximum countable time is a few seconds.

Card maintenance operations like BKOPS and MMC_ERASE and long
stream writes like packed command require timers of order of
several minutes, much beyond the capability of the IP timer.
So get rid of using the IP timer entirely and use kernel's hrtimer
functionality for guarding the device operations.
As part of this change, a workaround that disabled timeouts for
MMC_ERASE command is removed, and the arbitary timing of 100ms
is used only when the timeout is not explicitly specified by core.

A trivial change to get rid of unnecessary dealiasing of host-data
in omap_hsmmc_do_irq is also included.

Signed-off-by: Venkatraman S svenk...@ti.com
---

v1-v2:
   Fix typos in commit message.
   Add checks to handle subtle races between MMC IRQ and HRTIMER IRQ
   Mark set_guard_timer function as static
   (Felipe's comment to use macros for INT_EN_MASK is done as a separate patch )
 drivers/mmc/host/omap_hsmmc.c | 96 ++-
 1 file changed, 50 insertions(+), 46 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 9afdd20..57e86a4 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -79,7 +79,7 @@
 #define CLKD_SHIFT 6
 #define DTO_MASK   0x000F
 #define DTO_SHIFT  16
-#define INT_EN_MASK0x307F0033
+#define INT_EN_MASK0x306E0033
 #define BWR_ENABLE (1  4)
 #define BRR_ENABLE (1  5)
 #define DTO_ENABLE (1  20)
@@ -160,6 +160,7 @@ struct omap_hsmmc_host {
unsigned intdma_sg_idx;
unsigned char   bus_mode;
unsigned char   power_mode;
+   unsigned intns_per_clk_cycle;
int suspended;
int irq;
int use_dma, dma_ch;
@@ -172,6 +173,7 @@ struct omap_hsmmc_host {
int reqs_blocked;
int use_reg;
int req_in_progress;
+   struct hrtimer  guard_timer;
struct omap_hsmmc_next  next_data;
 
struct  omap_mmc_platform_data  *pdata;
@@ -455,10 +457,6 @@ static void omap_hsmmc_enable_irq(struct omap_hsmmc_host 
*host,
else
irq_mask = INT_EN_MASK;
 
-   /* Disable timeout for erases */
-   if (cmd-opcode == MMC_ERASE)
-   irq_mask = ~DTO_ENABLE;
-
OMAP_HSMMC_WRITE(host-base, STAT, STAT_CLEAR);
OMAP_HSMMC_WRITE(host-base, ISE, irq_mask);
OMAP_HSMMC_WRITE(host-base, IE, irq_mask);
@@ -508,6 +506,9 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host 
*host)
 time_before(jiffies, timeout))
cpu_relax();
 
+   if (ios-clock)
+   host-ns_per_clk_cycle = DIV_ROUND_UP(NSEC_PER_SEC, ios-clock);
+
omap_hsmmc_start_clock(host);
 }
 
@@ -824,7 +825,7 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct 
mmc_data *data)
omap_hsmmc_request_done(host, mrq);
return;
}
-
+   hrtimer_cancel(host-guard_timer);
host-data = NULL;
 
if (!data-error)
@@ -859,8 +860,11 @@ omap_hsmmc_cmd_done(struct omap_hsmmc_host *host, struct 
mmc_command *cmd)
cmd-resp[0] = OMAP_HSMMC_READ(host-base, RSP10);
}
}
-   if ((host-data == NULL  !host-response_busy) || cmd-error)
+   if ((host-data == NULL  !host-response_busy) || cmd-error) {
+   if (cmd-error != -ETIMEDOUT)
+   hrtimer_cancel(host-guard_timer);
omap_hsmmc_request_done(host, cmd-mrq);
+   }
 }
 
 /*
@@ -992,7 +996,7 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host *host, 
int status)
hsmmc_command_incomplete(host, -EILSEQ);
 
end_cmd = 1;
-   if (host-data || host-response_busy) {
+   if (data || host-response_busy) {
end_trans = 1;
host-response_busy = 0;
}
@@ -1292,41 +1296,35 @@ static int omap_hsmmc_start_dma_transfer(struct 
omap_hsmmc_host *host,
return 0;
 }
 
-static void set_data_timeout(struct omap_hsmmc_host *host,
-unsigned int timeout_ns,
-unsigned int timeout_clks)
+static void set_guard_timer(struct omap_hsmmc_host *host,
+   unsigned long timeout_ms, unsigned long timeout_ns,
+   unsigned int timeout_clks)
 {
-   unsigned int timeout, cycle_ns;
-   uint32_t reg, clkd, dto = 0;
+   ktime_t gtime;
+   unsigned int sec, nsec;
 
-   reg = OMAP_HSMMC_READ(host-base, SYSCTL);
-   clkd = (reg  CLKD_MASK)  CLKD_SHIFT;
-

Re: [PATCHv2 1/2] mmc: omap_hsmmc: convert from IP timer to hrtimer

2012-08-28 Thread Felipe Balbi 
On Tue, Aug 28, 2012 at 06:49:06PM +0530, Venkatraman S wrote:
 omap hsmmc controller IP has a built in timer that can be programmed to
 guard against unresponsive operations. But its range is very narrow,
 and the maximum countable time is a few seconds.
 
 Card maintenance operations like BKOPS and MMC_ERASE and long
 stream writes like packed command require timers of order of
 several minutes, much beyond the capability of the IP timer.
 So get rid of using the IP timer entirely and use kernel's hrtimer
 functionality for guarding the device operations.
 As part of this change, a workaround that disabled timeouts for
 MMC_ERASE command is removed, and the arbitary timing of 100ms
 is used only when the timeout is not explicitly specified by core.
 
 A trivial change to get rid of unnecessary dealiasing of host-data
 in omap_hsmmc_do_irq is also included.
 
 Signed-off-by: Venkatraman S svenk...@ti.com

Now it looks good:

Reviewed-by: Felipe Balbi ba...@ti.com

 ---
 
 v1-v2:
Fix typos in commit message.
Add checks to handle subtle races between MMC IRQ and HRTIMER IRQ
Mark set_guard_timer function as static
(Felipe's comment to use macros for INT_EN_MASK is done as a separate 
 patch )
  drivers/mmc/host/omap_hsmmc.c | 96 
 ++-
  1 file changed, 50 insertions(+), 46 deletions(-)
 
 diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
 index 9afdd20..57e86a4 100644
 --- a/drivers/mmc/host/omap_hsmmc.c
 +++ b/drivers/mmc/host/omap_hsmmc.c
 @@ -79,7 +79,7 @@
  #define CLKD_SHIFT   6
  #define DTO_MASK 0x000F
  #define DTO_SHIFT16
 -#define INT_EN_MASK  0x307F0033
 +#define INT_EN_MASK  0x306E0033
  #define BWR_ENABLE   (1  4)
  #define BRR_ENABLE   (1  5)
  #define DTO_ENABLE   (1  20)
 @@ -160,6 +160,7 @@ struct omap_hsmmc_host {
   unsigned intdma_sg_idx;
   unsigned char   bus_mode;
   unsigned char   power_mode;
 + unsigned intns_per_clk_cycle;
   int suspended;
   int irq;
   int use_dma, dma_ch;
 @@ -172,6 +173,7 @@ struct omap_hsmmc_host {
   int reqs_blocked;
   int use_reg;
   int req_in_progress;
 + struct hrtimer  guard_timer;
   struct omap_hsmmc_next  next_data;
  
   struct  omap_mmc_platform_data  *pdata;
 @@ -455,10 +457,6 @@ static void omap_hsmmc_enable_irq(struct omap_hsmmc_host 
 *host,
   else
   irq_mask = INT_EN_MASK;
  
 - /* Disable timeout for erases */
 - if (cmd-opcode == MMC_ERASE)
 - irq_mask = ~DTO_ENABLE;
 -
   OMAP_HSMMC_WRITE(host-base, STAT, STAT_CLEAR);
   OMAP_HSMMC_WRITE(host-base, ISE, irq_mask);
   OMAP_HSMMC_WRITE(host-base, IE, irq_mask);
 @@ -508,6 +506,9 @@ static void omap_hsmmc_set_clock(struct omap_hsmmc_host 
 *host)
time_before(jiffies, timeout))
   cpu_relax();
  
 + if (ios-clock)
 + host-ns_per_clk_cycle = DIV_ROUND_UP(NSEC_PER_SEC, ios-clock);
 +
   omap_hsmmc_start_clock(host);
  }
  
 @@ -824,7 +825,7 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct 
 mmc_data *data)
   omap_hsmmc_request_done(host, mrq);
   return;
   }
 -
 + hrtimer_cancel(host-guard_timer);
   host-data = NULL;
  
   if (!data-error)
 @@ -859,8 +860,11 @@ omap_hsmmc_cmd_done(struct omap_hsmmc_host *host, struct 
 mmc_command *cmd)
   cmd-resp[0] = OMAP_HSMMC_READ(host-base, RSP10);
   }
   }
 - if ((host-data == NULL  !host-response_busy) || cmd-error)
 + if ((host-data == NULL  !host-response_busy) || cmd-error) {
 + if (cmd-error != -ETIMEDOUT)
 + hrtimer_cancel(host-guard_timer);
   omap_hsmmc_request_done(host, cmd-mrq);
 + }
  }
  
  /*
 @@ -992,7 +996,7 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host 
 *host, int status)
   hsmmc_command_incomplete(host, -EILSEQ);
  
   end_cmd = 1;
 - if (host-data || host-response_busy) {
 + if (data || host-response_busy) {
   end_trans = 1;
   host-response_busy = 0;
   }
 @@ -1292,41 +1296,35 @@ static int omap_hsmmc_start_dma_transfer(struct 
 omap_hsmmc_host *host,
   return 0;
  }
  
 -static void set_data_timeout(struct omap_hsmmc_host *host,
 -  unsigned int timeout_ns,
 -  unsigned int timeout_clks)
 +static void set_guard_timer(struct omap_hsmmc_host *host,
 + unsigned long timeout_ms, unsigned long timeout_ns,
 + unsigned int timeout_clks)
  {
 - unsigned int timeout, cycle_ns;
 - uint32_t reg, clkd, dto = 0;
 + ktime_t gtime;
 +