RE: [PATCH 3/3] usb: chipidea: use hrtimer for otg fsm timers
> > > > > @@ -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
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
> >
> > 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
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
> >
> > 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
> > ---
> > 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_writ
Re: [PATCH 3/3] usb: chipidea: use hrtimer for otg fsm timers
On Mon, Feb 09, 2015 at 02:54:48PM +0800, Li Jun wrote:
> From: Li Jun
>
> 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
> ---
> 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)) {
> +
