On Thu, Mar 06, 2014 at 02:52:17PM +0800, Li Jun wrote:
> On Wed, Mar 05, 2014 at 04:28:14PM +0800, Peter Chen wrote:
> > On Thu, Feb 27, 2014 at 07:38:25AM +0800, Li Jun wrote:
> > > Add OTG HNP and SRP operation functions implementation:
> > > - charge vbus
> > > - drive vbus
> > > - connection signaling
> > > - drive sof
> > > - start data pulse
> > > - add fsm timer
> > > - delete fsm timer
> > > - start host
> > > - start gadget
> > >
> > > Signed-off-by: Li Jun <b47624@xxxxxxxxxxxxx>
> > > ---
> > > drivers/usb/chipidea/bits.h | 11 ++
> > > drivers/usb/chipidea/ci.h | 1 +
> > > drivers/usb/chipidea/otg_fsm.c | 231 ++++++++++++++++++++++++++++++++++++++++
> > > drivers/usb/chipidea/otg_fsm.h | 23 ++++
> > > 4 files changed, 266 insertions(+)
> > >
> > > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> > > index 83d06c1..c42eb35 100644
> > > --- a/drivers/usb/chipidea/bits.h
> > > +++ b/drivers/usb/chipidea/bits.h
> > > @@ -44,9 +44,14 @@
> > > #define DEVICEADDR_USBADR (0x7FUL << 25)
> > >
> > > /* PORTSC */
> > > +#define PORTSC_CCS BIT(0)
> > > +#define PORTSC_CSC BIT(1)
> > > +#define PORTSC_PEC BIT(3)
> > > +#define PORTSC_OCC BIT(5)
> > > #define PORTSC_FPR BIT(6)
> > > #define PORTSC_SUSP BIT(7)
> > > #define PORTSC_HSP BIT(9)
> > > +#define PORTSC_PP BIT(12)
> > > #define PORTSC_PTC (0x0FUL << 16)
> > > #define PORTSC_PHCD(d) ((d) ? BIT(22) : BIT(23))
> > > /* PTS and PTW for non lpm version only */
> > > @@ -56,6 +61,9 @@
> > > #define PORTSC_PTW BIT(28)
> > > #define PORTSC_STS BIT(29)
> > >
> > > +#define PORTSC_W1C_BITS \
> > > + (PORTSC_CSC | PORTSC_PEC | PORTSC_OCC)
> > > +
> > > /* DEVLC */
> > > #define DEVLC_PFSC BIT(23)
> > > #define DEVLC_PSPD (0x03UL << 25)
> > > @@ -71,7 +79,10 @@
> > > #define PTS_HSIC 4
> > >
> > > /* OTGSC */
> > > +#define OTGSC_VD BIT(0)
> > > +#define OTGSC_VC BIT(1)
> > > #define OTGSC_IDPU BIT(5)
> > > +#define OTGSC_HADP BIT(6)
> > > #define OTGSC_ID BIT(8)
> > > #define OTGSC_AVV BIT(9)
> > > #define OTGSC_ASV BIT(10)
> > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > > index db6bf30..171b1d2 100644
> > > --- a/drivers/usb/chipidea/ci.h
> > > +++ b/drivers/usb/chipidea/ci.h
> > > @@ -175,6 +175,7 @@ struct ci_hdrc {
> > > enum ci_role role;
> > > bool is_otg;
> > > struct otg_fsm *fsm;
> > > + struct ci_otg_fsm_timer_list *fsm_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 904381e..aa24466 100644
> > > --- a/drivers/usb/chipidea/otg_fsm.c
> > > +++ b/drivers/usb/chipidea/otg_fsm.c
> > > @@ -18,12 +18,242 @@
> > > #include <linux/usb/otg.h>
> > > #include <linux/usb/gadget.h>
> > > #include <linux/usb/chipidea.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > > #include "ci.h"
> > > #include "bits.h"
> > > #include "otg.h"
> > > #include "otg_fsm.h"
> > >
> > > +/* Add timer to active timer list */
> > > +static void ci_otg_add_timer(struct ci_hdrc *ci, enum ci_otg_fsm_timer_index 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;
> > > +
> > > + if (t >= NUM_CI_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;
> > > + }
> > > +
> > > + timer->count = timer->expires;
> > > + list_add_tail(&timer->list, active_timers);
> > > +
> > > + /* Enable 1ms irq */
> > > + if (!(hw_read(ci, OP_OTGSC, OTGSC_1MSIE)))
> > > + ci_enable_otg_interrupt(ci, OTGSC_1MSIE);
> > > +}
> > > +
> > > +/* Remove timer from active timer list */
> > > +static void ci_otg_del_timer(struct ci_hdrc *ci, enum ci_otg_fsm_timer_index t)
> > > +{
> > > + struct ci_otg_fsm_timer *tmp_timer, *del_tmp;
> > > + struct ci_otg_fsm_timer *timer = ci->fsm_timer->timer_list[t];
> > > + struct list_head *active_timers = &ci->fsm_timer->active_timers;
> > > + int flag = 0;
> > > +
> > > + if (t >= NUM_CI_OTG_FSM_TIMERS)
> > > + return;
> > > +
> > > + list_for_each_entry_safe(tmp_timer, del_tmp, active_timers, list)
> > > + if (tmp_timer == timer) {
> > > + list_del(&timer->list);
> > > + flag = 1;
> > > + }
> > > +
> > > + /* Disable 1ms irq if there is no any active timer */
> > > + if ((flag == 1) && list_empty(active_timers))
> > > + ci_disable_otg_interrupt(ci, OTGSC_1MSIE);
> > > +}
> >
> > Why variable flag is needed?
> >
>
> with this flag check, driver need not do irq disable again when the timer
> was already deleted.
>
It only needs to disable 1ms timer interrupt when there is no active timer,
does my understanding is correct? If it is, my questions are:
- When it will try to delete timer even there is no active timer?
- We can add list_empty(active_timers) at the beginning of code to
avoid re-disable 1ms interrupt again.
Anyway, if it is seldom occurrence for my first question, it is ok just
delete this flag. It can keep the code clean and easy to understand.
> Li Jun
> > > +
> > > +/* -------------------------------------------------------------*/
> > > +/* Operations that will be called from OTG Finite State Machine */
> > > +/* -------------------------------------------------------------*/
> > > +static void ci_otg_fsm_add_timer(struct otg_fsm *fsm, enum otg_fsm_timer t)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> >
> > The gadget seems to not related with this and below function.
> > Can we add struct otg_fsm fsm to struct ci_hdrc, in that case
> > we can use get ci through fsm?
> >
>
> Yes, gadget is not linked to OTG fsm, I did not find a better way to get ci
> here, actually this patchset is adding a otg_fsm *pointer* instead of
> struct otg_fsm into struct ci_hdrc, directly embed otg_fsm is a big cost
> since it's also a some big structure.
I think keep the code easy to read and maintain are important just waste
tens of bytes for some controllers. I prefer to use struct than pointer
for otg_fsm at struct ci_hdrc.
I checked struct otg_fsm, lots of entries are defined with int, in fact,
it doesn't necessary, defined as unsigned xxx:1 is enough.
>
> Li Jun
> > > +
> > > + if (t < NUM_OTG_FSM_TIMERS)
> > > + ci_otg_add_timer(ci, t);
> > > + return;
> > > +}
> > > +
> > > +static void ci_otg_fsm_del_timer(struct otg_fsm *fsm, enum otg_fsm_timer t)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + if (t < NUM_OTG_FSM_TIMERS)
> > > + ci_otg_del_timer(ci, t);
> > > + return;
> > > +}
> > > +
> > > +static void ci_otg_chrg_vbus(struct otg_fsm *fsm, int on)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + if (on)
> > > + /* stop discharging, start charging */
> > > + hw_write(ci, OP_OTGSC,
> > > + OTGSC_INT_STATUS_BITS | OTGSC_VD | OTGSC_VC,
> > > + OTGSC_VC);
> > > + else
> > > + /* stop charging */
> > > + hw_write(ci, OP_OTGSC,
> > > + OTGSC_INT_STATUS_BITS | OTGSC_VC, 0);
> > > +}
> > > +
> > > +/* A-device drive vbus */
> > > +static void ci_otg_drv_vbus(struct otg_fsm *fsm, int on)
> > > +{
> > > + int ret;
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + if (on) {
> > > + hw_write(ci, OP_PORTSC, PORTSC_W1C_BITS | PORTSC_PP,
> > > + PORTSC_PP);
> > > + if (ci->platdata->reg_vbus) {
> > > + ret = regulator_enable(ci->platdata->reg_vbus);
> > > + if (ret) {
> > > + dev_err(ci->dev,
> > > + "Failed to enable vbus regulator, ret=%d\n",
> > > + ret);
> > > + return;
> > > + }
> > > + }
> > > + /* Disable data pulse irq */
> > > + ci_disable_otg_interrupt(ci, OTGSC_DPIE);
> > > + } else {
> > > + if (ci->platdata->reg_vbus)
> > > + regulator_disable(ci->platdata->reg_vbus);
> > > + hw_write(ci, OP_PORTSC, PORTSC_W1C_BITS | PORTSC_PP, 0);
> > > + }
> > > +}
> > > +
> > > +/*
> > > + * Control data line through Run Stop bit.
> > > + */
> > > +static void ci_otg_loc_conn(struct otg_fsm *fsm, int on)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + if (on)
> > > + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS);
> > > + else
> > > + hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
> > > +}
> > > +
> > > +/*
> > > + * Generate SOF by host. This is controlled through suspend/resume the
> > > + * port. In host mode, controller will automatically send SOF.
> > > + * Suspend will block the data on the port.
> > > + */
> > > +static void ci_otg_loc_sof(struct otg_fsm *fsm, int on)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + if (on)
> > > + hw_write(ci, OP_PORTSC, PORTSC_W1C_BITS | PORTSC_FPR,
> > > + PORTSC_FPR);
> > > + else
> > > + hw_write(ci, OP_PORTSC, PORTSC_W1C_BITS | PORTSC_SUSP,
> > > + PORTSC_SUSP);
> > > +}
> > > +
> > > +/* Start SRP pulsing by data-line pulsing, no v-bus pulsing followed. */
> > > +static void ci_otg_start_pulse(struct otg_fsm *fsm)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS | OTGSC_HADP,
> > > + OTGSC_HADP);
> > > +
> > > + ci_otg_add_timer(ci, B_DATA_PLS);
> > > +}
> > > +
> > > +/*
> > > + * Here use this chance to enable data pulse irq for A device
> > > + * in a_idle state since ADP is not supported.
> > > + */
> > > +static void ci_otg_start_adp_prb(struct otg_fsm *fsm)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + /* Clear exsiting DP irq */
> > > + hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, OTGSC_DPIS);
> > > + /* Enable data pulse irq */
> > > + ci_enable_otg_interrupt(ci, OTGSC_DPIE);
> > > +}
> > > +
> >
> > - What does _prb mean?
> ADP probe is used to probe attach or detach when there is no VBUS.
>
> > - Do we need to consider both ADP support and non-ADP situation?
>
> This patchset is not taking ADP into account, this will be updated
> if I can get an ADP-capable HW, but an ADP-capable HW should be able
> to work fine on HNP&SRP with this driver.
If it is, we'd better not talk ADP at code and comment.
>
> > - I think I may consider not well to introduce ci_enable_otg_xxx APIs,
> > it causes some many APIs to access otgsc registers.
> > In fact, all otgsc read/write can use unify APIs. Can you help
> > to add a patch to introduce otg_hw_read and otg_hw_write APIs,
> > and convert current change to these new APIs. The patches for
> > above changes can separate with this patchset, I can queue
> > this one first.
> >
> what are you expecting on otg_hw_read/write? Read out all bits value, and write
> some bits into it? Considering the only thing should take care is to avoid to
> touch OTGSC_INT_STATUS_BITS if the intention is not to clear irq, I can submit
> a patch for read all bits, and write some bits with OTGSC_INT_STATUS_BITS to be
> 0 if the target bits are not in OTGSC_INT_STATUS_BITS.
Yes, for read, just fix reg as OP_OTGSC, struct ci_hdrc, and mask
are needed, for write, do not clear status.
A patchset for doing that is welcome, thanks.
Peter
>
> Li Jun
> >
> > > +static int ci_otg_start_host(struct otg_fsm *fsm, int on)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + mutex_unlock(&fsm->lock);
> > > + if (on) {
> > > + if (ci->role != CI_ROLE_HOST) {
> > > + ci_role_stop(ci);
> > > + ci_role_start(ci, CI_ROLE_HOST);
> > > + }
> > > + } else {
> > > + if (ci->role == CI_ROLE_HOST) {
> > > + ci_role_stop(ci);
> > > + ci_role_start(ci, CI_ROLE_GADGET);
> > > + }
> > > + }
> > > + mutex_lock(&fsm->lock);
> > > + return 0;
> > > +}
> > > +
> > > +static int ci_otg_start_gadget(struct otg_fsm *fsm, int on)
> > > +{
> > > + struct ci_hdrc *ci = container_of(fsm->otg->gadget,
> > > + struct ci_hdrc, gadget);
> > > +
> > > + mutex_unlock(&fsm->lock);
> > > + if (on) {
> > > + if (ci->role == CI_ROLE_GADGET)
> > > + usb_gadget_vbus_connect(&ci->gadget);
> > > + } else {
> > > + if (ci->role == CI_ROLE_GADGET)
> > > + usb_gadget_vbus_disconnect(&ci->gadget);
> > > + }
> > > + mutex_lock(&fsm->lock);
> > > + return 0;
> > > +}
> > > +
> > > +static struct otg_fsm_ops ci_otg_ops = {
> > > + .chrg_vbus = ci_otg_chrg_vbus,
> > > + .drv_vbus = ci_otg_drv_vbus,
> > > + .loc_conn = ci_otg_loc_conn,
> > > + .loc_sof = ci_otg_loc_sof,
> > > + .start_pulse = ci_otg_start_pulse,
> > > + .start_adp_prb = ci_otg_start_adp_prb,
> > > +
> > > + .add_timer = ci_otg_fsm_add_timer,
> > > + .del_timer = ci_otg_fsm_del_timer,
> > > +
> > > + .start_host = ci_otg_start_host,
> > > + .start_gadget = ci_otg_start_gadget,
> > > +};
> > > +
> >
> > If no comments, please blank line between APIs.
> >
>
> I will change.
>
> > > int ci_hdrc_otg_fsm_init(struct ci_hdrc *ci)
> > > {
> > > if (ci->platdata->dr_mode != USB_DR_MODE_OTG)
> > > @@ -51,6 +281,7 @@ int ci_hdrc_otg_fsm_init(struct ci_hdrc *ci)
> > > ci->fsm->otg->phy = ci->transceiver;
> > > ci->fsm->otg->gadget = &ci->gadget;
> > > ci->transceiver->state = OTG_STATE_UNDEFINED;
> > > + ci->fsm->ops = &ci_otg_ops;
> > >
> > > mutex_init(&ci->fsm->lock);
> > >
> > > diff --git a/drivers/usb/chipidea/otg_fsm.h b/drivers/usb/chipidea/otg_fsm.h
> > > index bf20a85..1f85ad3 100644
> > > --- a/drivers/usb/chipidea/otg_fsm.h
> > > +++ b/drivers/usb/chipidea/otg_fsm.h
> > > @@ -13,6 +13,29 @@
> > >
> > > #include <linux/usb/otg-fsm.h>
> > >
> > > +enum ci_otg_fsm_timer_index {
> > > + /* CI specific timers, start from the end
> > > + * of standard and auxiliary OTG timers */
> >
> > Please use recommend multi-line comment style.
> >
>
> I will do a thorough check for comment style.
>
> > > + B_DATA_PLS = NUM_OTG_FSM_TIMERS,
> > > + B_SSEND_SRP,
> > > + B_SESS_VLD,
> > > +
> > > + NUM_CI_OTG_FSM_TIMERS,
> > > +};
> > > +
> > > +struct ci_otg_fsm_timer {
> > > + unsigned long expires; /* Number of count increase to timeout */
> > > + unsigned long count; /* Tick counter */
> > > + void (*function)(void *, unsigned long); /* Timeout function */
> > > + unsigned long data; /* Data passed to function */
> >
> > We may not need comment for data, it is the same usage for kernel timer.
> >
>
> I will remove.
>
> > > + struct list_head list;
> > > +};
> > > +
> > > +struct ci_otg_fsm_timer_list {
> > > + struct ci_otg_fsm_timer *timer_list[NUM_CI_OTG_FSM_TIMERS];
> > > + struct list_head active_timers;
> > > +};
> > > +
> > > #ifdef CONFIG_USB_OTG_FSM
> > >
> > > int ci_hdrc_otg_fsm_init(struct ci_hdrc *ci);
> > > --
> > > 1.7.9.5
> > >
> > >
> >
> > --
> >
> > Best Regards,
> > Peter Chen
> >
>
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
