RE: [PATCH 3/3] usb: chipidea: use hrtimer for otg fsm timers

2015-02-28 Thread Peter Chen 

 
 @@ -579,8 +579,15 @@ int ci_otg_fsm_work(struct ci_hdrc *ci)
* a_idle to a_wait_vrise when power up
*/
   if ((ci-fsm.id) || (ci-id_event) ||
 - (ci-fsm.power_up))
 + (ci-fsm.power_up)) {
   ci_otg_queue_work(ci);
 + } else {
 + /* Enable data pulse irq */
 + hw_write(ci, OP_PORTSC,
 PORTSC_W1C_BITS |
 +
   PORTSC_PP, 0);
 + hw_write_otgsc(ci, OTGSC_DPIS,
 OTGSC_DPIS);
 + hw_write_otgsc(ci, OTGSC_DPIE,
 OTGSC_DPIE);
 + }
   
Can we enable data pulse enable at initialization routine?
  
   This irq should be enabled only for A-device when there is no
   session (host role, no vbus, so in A_IDLE state), and disable it after 
   receive
 its irq(SRP).
  
 
  But from the code, I don't know the state is at A_IDLE, mind to change?
 
 
 It's already under condition of A_IDLE as below:
 
 if (ci-fsm.otg-state == OTG_STATE_A_IDLE) {
   ... ...
   if () {
   ... ...
   } else {
   /* Enable data pulse irq */
   ...
   }
 }
 
 That's a change to avoid to do it in timer out(VFALL) handler, so put it here
 (after otg fsm transit to A_IDLE and will no further state transitions).
 

Oh, I have no seen it in patch file. Send your v2, then, I can have a test.

Peter
--
To unsubscribe from this list: send the line unsubscribe linux-usb 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/3] usb: chipidea: use hrtimer for otg fsm timers

2015-02-27 Thread Peter Chen 
 
 
  Why you use unsigned, but not unsigned int or unsigned long?
 
 unsigned is equal to unsigned int, currently only 13 timers are defined, so
 unsigned is okay, for possible future extension, I will change it to be
 unsigned long
 

Since I see you use unsigned long below, I have this question, it is ok
you do not change it.

 
  
   @@ -579,8 +579,15 @@ int ci_otg_fsm_work(struct ci_hdrc *ci)
  * a_idle to a_wait_vrise when power up
  */
 if ((ci-fsm.id) || (ci-id_event) ||
   - (ci-fsm.power_up))
   + (ci-fsm.power_up)) {
 ci_otg_queue_work(ci);
   + } else {
   + /* Enable data pulse irq */
   + hw_write(ci, OP_PORTSC, PORTSC_W1C_BITS |
   + PORTSC_PP, 0);
   + hw_write_otgsc(ci, OTGSC_DPIS, OTGSC_DPIS);
   + hw_write_otgsc(ci, OTGSC_DPIE, OTGSC_DPIE);
   + }
 
  Can we enable data pulse enable at initialization routine?
 
 This irq should be enabled only for A-device when there is no session (host 
 role,
 no vbus, so in A_IDLE state), and disable it after receive its irq(SRP).
 

But from the code, I don't know the state is at A_IDLE, mind to change?

 
Peter
--
To unsubscribe from this list: send the line unsubscribe linux-usb 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/3] usb: chipidea: use hrtimer for otg fsm timers

2015-02-27 Thread Li Jun 
On Sat, Feb 28, 2015 at 09:43:30AM +0800, Chen Peter-B29397 wrote:
  
  
   Why you use unsigned, but not unsigned int or unsigned long?
  
  unsigned is equal to unsigned int, currently only 13 timers are defined, 
  so
  unsigned is okay, for possible future extension, I will change it to be
  unsigned long
  
 
 Since I see you use unsigned long below, I have this question, it is ok
 you do not change it.
 
  
   
@@ -579,8 +579,15 @@ int ci_otg_fsm_work(struct ci_hdrc *ci)
 * a_idle to a_wait_vrise when power up
 */
if ((ci-fsm.id) || (ci-id_event) ||
-   (ci-fsm.power_up))
+   (ci-fsm.power_up)) {
ci_otg_queue_work(ci);
+   } else {
+   /* Enable data pulse irq */
+   hw_write(ci, OP_PORTSC, PORTSC_W1C_BITS 
|
+   
PORTSC_PP, 0);
+   hw_write_otgsc(ci, OTGSC_DPIS, 
OTGSC_DPIS);
+   hw_write_otgsc(ci, OTGSC_DPIE, 
OTGSC_DPIE);
+   }
  
   Can we enable data pulse enable at initialization routine?
  
  This irq should be enabled only for A-device when there is no session (host 
  role,
  no vbus, so in A_IDLE state), and disable it after receive its irq(SRP).
  
 
 But from the code, I don't know the state is at A_IDLE, mind to change?
 

It's already under condition of A_IDLE as below:

if (ci-fsm.otg-state == OTG_STATE_A_IDLE) {
... ...
if () {
... ...
} else {
/* Enable data pulse irq */
...
}
}

That's a change to avoid to do it in timer out(VFALL) handler, so put it here
(after otg fsm transit to A_IDLE and will no further state transitions).

Li Jun

  
 Peter
--
To unsubscribe from this list: send the line unsubscribe linux-usb 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/3] usb: chipidea: use hrtimer for otg fsm timers

2015-02-26 Thread Li Jun 
On Thu, Feb 26, 2015 at 07:25:56PM +0800, Peter Chen wrote:
 On Mon, Feb 09, 2015 at 02:54:48PM +0800, Li Jun wrote:
  From: Li Jun b47...@freescale.com
  
  Current otg fsm timers are using controller 1ms irq and count it, this patch
  is to replace it with hrtimer solution, use one hrtimer for all otg timers.
  
  Signed-off-by: Li Jun jun...@freescale.com
  ---
   drivers/usb/chipidea/ci.h  |   10 +-
   drivers/usb/chipidea/otg_fsm.c |  365 
  
   2 files changed, 191 insertions(+), 184 deletions(-)
  
  diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
  index c09381d..6256f04 100644
  --- a/drivers/usb/chipidea/ci.h
  +++ b/drivers/usb/chipidea/ci.h
  @@ -162,7 +162,10 @@ struct hw_bank {
* @role: current role
* @is_otg: if the device is otg-capable
* @fsm: otg finite state machine
  - * @fsm_timer: pointer to timer list of otg fsm
  + * @otg_fsm_hrtimer: hrtimer for otg fsm timers
  + * @hr_timeouts: time out during lists with msec
 
 It is ktime_t, any relationship with msec?
 %s/lists/list

My wrong comments, I will correct it to be time out list of active timers

 
  + * @enabled_otg_timers: bits of enabled otg timers
 
 How about enabled_otg_timer_bits?
 
sounds better, I will change

  + * @next_otg_timer: next nearest enabled timer to be expired
* @work: work for role changing
* @wq: workqueue thread
* @qh_pool: allocation pool for queue heads
  @@ -205,7 +208,10 @@ struct ci_hdrc {
  boolis_otg;
  struct usb_otg  otg;
  struct otg_fsm  fsm;
  -   struct ci_otg_fsm_timer_list*fsm_timer;
  +   struct hrtimer  otg_fsm_hrtimer;
  +   ktime_t hr_timeouts[NUM_OTG_FSM_TIMERS];
  +   unsignedenabled_otg_timers;
 
 Why you use unsigned, but not unsigned int or unsigned long?

unsigned is equal to unsigned int, currently only 13 timers are defined, so
unsigned is okay, for possible future extension, I will change it to be
unsigned long

Li Jun
 
  +   enum otg_fsm_timer  next_otg_timer;
  struct work_struct  work;
  struct workqueue_struct *wq;
   
  diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
  index ba2cb91..0af7ff0 100644
  --- a/drivers/usb/chipidea/otg_fsm.c
  +++ b/drivers/usb/chipidea/otg_fsm.c
  @@ -30,22 +30,6 @@
   #include otg.h
   #include otg_fsm.h
   
  -static struct ci_otg_fsm_timer *otg_timer_initializer
  -(struct ci_hdrc *ci, void (*function)(void *, unsigned long),
  -   unsigned long expires, unsigned long data)
  -{
  -   struct ci_otg_fsm_timer *timer;
  -
  -   timer = devm_kzalloc(ci-dev, sizeof(struct ci_otg_fsm_timer),
  -   GFP_KERNEL);
  -   if (!timer)
  -   return NULL;
  -   timer-function = function;
  -   timer-expires = expires;
  -   timer-data = data;
  -   return timer;
  -}
  -
   /* Add for otg: interact with user space app */
   static ssize_t
   get_a_bus_req(struct device *dev, struct device_attribute *attr, char *buf)
  @@ -204,36 +188,48 @@ static struct attribute_group inputs_attr_group = {
   };
   
   /*
  + * Keep this list in the same order as timers indexed
  + * by enum otg_fsm_timer in include/linux/usb/otg-fsm.h
  + */
  +static unsigned otg_timer_ms[] = {
  +   TA_WAIT_VRISE,
  +   TA_WAIT_VFALL,
  +   TA_WAIT_BCON,
  +   TA_AIDL_BDIS,
  +   TB_ASE0_BRST,
  +   TA_BIDL_ADIS,
  +   TB_SE0_SRP,
  +   TB_SRP_FAIL,
  +   0,
 
 0? No timer for it?

Means this timer(A_WAIT_ENUM) is not used, so 0 delay time is defined here.

 
  +   TB_DATA_PLS,
  +   TB_SSEND_SRP,
  +};
  +
  +/*
* Add timer to active timer list
*/
   static void ci_otg_add_timer(struct ci_hdrc *ci, enum otg_fsm_timer t)
   {
  -   struct ci_otg_fsm_timer *tmp_timer;
  -   struct ci_otg_fsm_timer *timer = ci-fsm_timer-timer_list[t];
  -   struct list_head *active_timers = ci-fsm_timer-active_timers;
  +   unsigned long flags, timer_sec, timer_nsec;
   
  if (t = NUM_OTG_FSM_TIMERS)
  return;
   
  -   /*
  -* Check if the timer is already in the active list,
  -* if so update timer count
  -*/
  -   list_for_each_entry(tmp_timer, active_timers, list)
  -   if (tmp_timer == timer) {
  -   timer-count = timer-expires;
  -   return;
  -   }
  -
  -   if (list_empty(active_timers))
  -   pm_runtime_get(ci-dev);
  -
  -   timer-count = timer-expires;
  -   list_add_tail(timer-list, active_timers);
  -
  -   /* Enable 1ms irq */
  -   if (!(hw_read_otgsc(ci, OTGSC_1MSIE)))
  -   hw_write_otgsc(ci, OTGSC_1MSIE, OTGSC_1MSIE);
  +   spin_lock_irqsave(ci-lock, flags);
  +   timer_sec = otg_timer_ms[t] / MSEC_PER_SEC;
  +   timer_nsec = (otg_timer_ms[t] % MSEC_PER_SEC) * NSEC_PER_MSEC;
  +   ci-hr_timeouts[t] =

Re: [PATCH 3/3] usb: chipidea: use hrtimer for otg fsm timers

2015-02-26 Thread Peter Chen 
On Mon, Feb 09, 2015 at 02:54:48PM +0800, Li Jun wrote:
 From: Li Jun b47...@freescale.com
 
 Current otg fsm timers are using controller 1ms irq and count it, this patch
 is to replace it with hrtimer solution, use one hrtimer for all otg timers.
 
 Signed-off-by: Li Jun jun...@freescale.com
 ---
  drivers/usb/chipidea/ci.h  |   10 +-
  drivers/usb/chipidea/otg_fsm.c |  365 
 
  2 files changed, 191 insertions(+), 184 deletions(-)
 
 diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
 index c09381d..6256f04 100644
 --- a/drivers/usb/chipidea/ci.h
 +++ b/drivers/usb/chipidea/ci.h
 @@ -162,7 +162,10 @@ struct hw_bank {
   * @role: current role
   * @is_otg: if the device is otg-capable
   * @fsm: otg finite state machine
 - * @fsm_timer: pointer to timer list of otg fsm
 + * @otg_fsm_hrtimer: hrtimer for otg fsm timers
 + * @hr_timeouts: time out during lists with msec

It is ktime_t, any relationship with msec?
%s/lists/list

 + * @enabled_otg_timers: bits of enabled otg timers

How about enabled_otg_timer_bits?

 + * @next_otg_timer: next nearest enabled timer to be expired
   * @work: work for role changing
   * @wq: workqueue thread
   * @qh_pool: allocation pool for queue heads
 @@ -205,7 +208,10 @@ struct ci_hdrc {
   boolis_otg;
   struct usb_otg  otg;
   struct otg_fsm  fsm;
 - struct ci_otg_fsm_timer_list*fsm_timer;
 + struct hrtimer  otg_fsm_hrtimer;
 + ktime_t hr_timeouts[NUM_OTG_FSM_TIMERS];
 + unsignedenabled_otg_timers;

Why you use unsigned, but not unsigned int or unsigned long?

 + enum otg_fsm_timer  next_otg_timer;
   struct work_struct  work;
   struct workqueue_struct *wq;
  
 diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
 index ba2cb91..0af7ff0 100644
 --- a/drivers/usb/chipidea/otg_fsm.c
 +++ b/drivers/usb/chipidea/otg_fsm.c
 @@ -30,22 +30,6 @@
  #include otg.h
  #include otg_fsm.h
  
 -static struct ci_otg_fsm_timer *otg_timer_initializer
 -(struct ci_hdrc *ci, void (*function)(void *, unsigned long),
 - unsigned long expires, unsigned long data)
 -{
 - struct ci_otg_fsm_timer *timer;
 -
 - timer = devm_kzalloc(ci-dev, sizeof(struct ci_otg_fsm_timer),
 - GFP_KERNEL);
 - if (!timer)
 - return NULL;
 - timer-function = function;
 - timer-expires = expires;
 - timer-data = data;
 - return timer;
 -}
 -
  /* Add for otg: interact with user space app */
  static ssize_t
  get_a_bus_req(struct device *dev, struct device_attribute *attr, char *buf)
 @@ -204,36 +188,48 @@ static struct attribute_group inputs_attr_group = {
  };
  
  /*
 + * Keep this list in the same order as timers indexed
 + * by enum otg_fsm_timer in include/linux/usb/otg-fsm.h
 + */
 +static unsigned otg_timer_ms[] = {
 + TA_WAIT_VRISE,
 + TA_WAIT_VFALL,
 + TA_WAIT_BCON,
 + TA_AIDL_BDIS,
 + TB_ASE0_BRST,
 + TA_BIDL_ADIS,
 + TB_SE0_SRP,
 + TB_SRP_FAIL,
 + 0,

0? No timer for it?

 + TB_DATA_PLS,
 + TB_SSEND_SRP,
 +};
 +
 +/*
   * Add timer to active timer list
   */
  static void ci_otg_add_timer(struct ci_hdrc *ci, enum otg_fsm_timer t)
  {
 - struct ci_otg_fsm_timer *tmp_timer;
 - struct ci_otg_fsm_timer *timer = ci-fsm_timer-timer_list[t];
 - struct list_head *active_timers = ci-fsm_timer-active_timers;
 + unsigned long flags, timer_sec, timer_nsec;
  
   if (t = NUM_OTG_FSM_TIMERS)
   return;
  
 - /*
 -  * Check if the timer is already in the active list,
 -  * if so update timer count
 -  */
 - list_for_each_entry(tmp_timer, active_timers, list)
 - if (tmp_timer == timer) {
 - timer-count = timer-expires;
 - return;
 - }
 -
 - if (list_empty(active_timers))
 - pm_runtime_get(ci-dev);
 -
 - timer-count = timer-expires;
 - list_add_tail(timer-list, active_timers);
 -
 - /* Enable 1ms irq */
 - if (!(hw_read_otgsc(ci, OTGSC_1MSIE)))
 - hw_write_otgsc(ci, OTGSC_1MSIE, OTGSC_1MSIE);
 + spin_lock_irqsave(ci-lock, flags);
 + timer_sec = otg_timer_ms[t] / MSEC_PER_SEC;
 + timer_nsec = (otg_timer_ms[t] % MSEC_PER_SEC) * NSEC_PER_MSEC;
 + ci-hr_timeouts[t] = ktime_add(ktime_get(),
 + ktime_set(timer_sec, timer_nsec));
 + ci-enabled_otg_timers |= (1  t);
 + if ((ci-next_otg_timer == NUM_OTG_FSM_TIMERS) ||
 + (ci-hr_timeouts[ci-next_otg_timer].tv64 
 + ci-hr_timeouts[t].tv64)) {
 + ci-next_otg_timer = t;
 + hrtimer_start_range_ns(ci-otg_fsm_hrtimer,
 +

[PATCH 3/3] usb: chipidea: use hrtimer for otg fsm timers

2015-02-09 Thread Li Jun 
From: Li Jun b47...@freescale.com

Current otg fsm timers are using controller 1ms irq and count it, this patch
is to replace it with hrtimer solution, use one hrtimer for all otg timers.

Signed-off-by: Li Jun jun...@freescale.com
---
 drivers/usb/chipidea/ci.h  |   10 +-
 drivers/usb/chipidea/otg_fsm.c |  365 
 2 files changed, 191 insertions(+), 184 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index c09381d..6256f04 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -162,7 +162,10 @@ struct hw_bank {
  * @role: current role
  * @is_otg: if the device is otg-capable
  * @fsm: otg finite state machine
- * @fsm_timer: pointer to timer list of otg fsm
+ * @otg_fsm_hrtimer: hrtimer for otg fsm timers
+ * @hr_timeouts: time out during lists with msec
+ * @enabled_otg_timers: bits of enabled otg timers
+ * @next_otg_timer: next nearest enabled timer to be expired
  * @work: work for role changing
  * @wq: workqueue thread
  * @qh_pool: allocation pool for queue heads
@@ -205,7 +208,10 @@ struct ci_hdrc {
boolis_otg;
struct usb_otg  otg;
struct otg_fsm  fsm;
-   struct ci_otg_fsm_timer_list*fsm_timer;
+   struct hrtimer  otg_fsm_hrtimer;
+   ktime_t hr_timeouts[NUM_OTG_FSM_TIMERS];
+   unsignedenabled_otg_timers;
+   enum otg_fsm_timer  next_otg_timer;
struct work_struct  work;
struct workqueue_struct *wq;
 
diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
index ba2cb91..0af7ff0 100644
--- a/drivers/usb/chipidea/otg_fsm.c
+++ b/drivers/usb/chipidea/otg_fsm.c
@@ -30,22 +30,6 @@
 #include otg.h
 #include otg_fsm.h
 
-static struct ci_otg_fsm_timer *otg_timer_initializer
-(struct ci_hdrc *ci, void (*function)(void *, unsigned long),
-   unsigned long expires, unsigned long data)
-{
-   struct ci_otg_fsm_timer *timer;
-
-   timer = devm_kzalloc(ci-dev, sizeof(struct ci_otg_fsm_timer),
-   GFP_KERNEL);
-   if (!timer)
-   return NULL;
-   timer-function = function;
-   timer-expires = expires;
-   timer-data = data;
-   return timer;
-}
-
 /* Add for otg: interact with user space app */
 static ssize_t
 get_a_bus_req(struct device *dev, struct device_attribute *attr, char *buf)
@@ -204,36 +188,48 @@ static struct attribute_group inputs_attr_group = {
 };
 
 /*
+ * Keep this list in the same order as timers indexed
+ * by enum otg_fsm_timer in include/linux/usb/otg-fsm.h
+ */
+static unsigned otg_timer_ms[] = {
+   TA_WAIT_VRISE,
+   TA_WAIT_VFALL,
+   TA_WAIT_BCON,
+   TA_AIDL_BDIS,
+   TB_ASE0_BRST,
+   TA_BIDL_ADIS,
+   TB_SE0_SRP,
+   TB_SRP_FAIL,
+   0,
+   TB_DATA_PLS,
+   TB_SSEND_SRP,
+};
+
+/*
  * Add timer to active timer list
  */
 static void ci_otg_add_timer(struct ci_hdrc *ci, enum otg_fsm_timer t)
 {
-   struct ci_otg_fsm_timer *tmp_timer;
-   struct ci_otg_fsm_timer *timer = ci-fsm_timer-timer_list[t];
-   struct list_head *active_timers = ci-fsm_timer-active_timers;
+   unsigned long flags, timer_sec, timer_nsec;
 
if (t = NUM_OTG_FSM_TIMERS)
return;
 
-   /*
-* Check if the timer is already in the active list,
-* if so update timer count
-*/
-   list_for_each_entry(tmp_timer, active_timers, list)
-   if (tmp_timer == timer) {
-   timer-count = timer-expires;
-   return;
-   }
-
-   if (list_empty(active_timers))
-   pm_runtime_get(ci-dev);
-
-   timer-count = timer-expires;
-   list_add_tail(timer-list, active_timers);
-
-   /* Enable 1ms irq */
-   if (!(hw_read_otgsc(ci, OTGSC_1MSIE)))
-   hw_write_otgsc(ci, OTGSC_1MSIE, OTGSC_1MSIE);
+   spin_lock_irqsave(ci-lock, flags);
+   timer_sec = otg_timer_ms[t] / MSEC_PER_SEC;
+   timer_nsec = (otg_timer_ms[t] % MSEC_PER_SEC) * NSEC_PER_MSEC;
+   ci-hr_timeouts[t] = ktime_add(ktime_get(),
+   ktime_set(timer_sec, timer_nsec));
+   ci-enabled_otg_timers |= (1  t);
+   if ((ci-next_otg_timer == NUM_OTG_FSM_TIMERS) ||
+   (ci-hr_timeouts[ci-next_otg_timer].tv64 
+   ci-hr_timeouts[t].tv64)) {
+   ci-next_otg_timer = t;
+   hrtimer_start_range_ns(ci-otg_fsm_hrtimer,
+   ci-hr_timeouts[t], NSEC_PER_MSEC,
+   HRTIMER_MODE_ABS);
+   }
+   spin_unlock_irqrestore(ci-lock, flags);
 }
 
 /*
@@ -241,174 +237,177 @@ static void