On Thu, Mar 06, 2014 at 04:13:10PM +0800, Peter Chen wrote:
> 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?
There are some cases I try to delete the same timer but can't decide
those cases occur in which order, so if the timer is the last active
one, next deletion should do nothing.
> - 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.
>
I will add list_empty(active_timers) at the beginning, that can fix it,
thanks!
> > 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.
>
OK, I will directly embed it.
> 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.
>
That's true, I will create a separated patch for it.
> >
> > 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 will remove all ADP related stuff.
> >
> > > - 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
