On Tue, Nov 13, 2012 at 02:25:59PM +0200, Alexander Shishkin wrote:
Alex, Thanks for review.
> > At new design, when role switch occurs, the gadget just calls
> > usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
> > reset controller, it will not free any device/gadget structure
>
> ...but I do agree that this is much better.
> One thing we need to address here is CI13XXX_PULLUP_ON_VBUS. Since we
> now handle vbus session in the core driver, we should just retire this
> flag. And this vbus_disconnect() method won't work if PULLUP_ON_VBUS is
> not set anyway, since vbus_session is a nop in that case.
Agree, I will change.
>
> > - Add vbus connect and disconnect to core interrupt handler, it can
> > notify udc driver by calling
> > usb_gadget_vbus_disconnect/usb_gadget_vbus_connect.
> >
> > - Add otg.c to implement struct usb_otg, in that case, calling otg_set_peripheral
> > will not be failed at udc.c. Besides, we enable id interrupt at
> > ci_hdrc_otg_init.
>
> Can you make this a separate patch?
OK
>
> > - Add special case handling, like connecting to host during boot up at device
> > mode, usb device at the MicroB to A cable at host mode, etc.
>
> This can probably be a separate patch too, since it's kind of a separate
> issue.
OK
> > +#define OTGSC_1MSIS BIT(21)
> > +#define OTGSC_DPIS BIT(22)
> > #define OTGSC_IDIE BIT(24)
> > #define OTGSC_AVVIE BIT(25)
> > #define OTGSC_ASVIE BIT(26)
> > #define OTGSC_BSVIE BIT(27)
> > #define OTGSC_BSEIE BIT(28)
> > +#define OTGSC_1MSIE BIT(29)
> > +#define OTGSC_DPIE BIT(30)
> > +#define OTGSC_INT_EN_BITS (BIT(24) | BIT(25) | BIT(26) \
> > + | BIT(27) | BIT(28) | BIT(29) \
> > + | BIT(30))
> > +#define OTGSC_INT_STATUS_BITS (BIT(16) | BIT(17) | BIT(18) \
> > + | BIT(19) | BIT(20) | BIT(21) \
> > + | BIT(22))
>
> Why not use the OTGSC_* defines instead of bit numbers?
OK, will change
>
> > struct usb_hcd *hcd;
> > + /* events handled at ci_role_work */
> > +#define ID 0
> > +#define B_SESS_VALID 1
> > + unsigned long events;
>
> I would just use bools instead.
We may add other otg events (like AVBUS) to support fully otg function.
>
> > + struct usb_otg otg;
> > };
> >
> > static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index f69d029..b50b77a 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -73,6 +73,7 @@
> > #include "bits.h"
> > #include "host.h"
> > #include "debug.h"
> > +#include "otg.h"
> >
> > /* Controller register map */
> > static uintptr_t ci_regs_nolpm[] = {
> > @@ -264,25 +265,138 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci)
> > return role;
> > }
> >
> > +#define CI_WAIT_VBUS_STABLE_TIMEOUT 500
> > /**
> > - * ci_role_work - perform role changing based on ID pin
> > - * @work: work struct
> > + * ci_wait_vbus_stable
> > + * When usb role switches, we need to turn on/off internal vbus
> > + * regulaor, sometimes, the real vbus value may not lower fast
> > + * enough due to capacitance, and we do want the vbus lower
> > + * than 0.8v if it is device mode, and higher than 4.4v, if it
> > + * is host mode.
> > + *
> > + * @low: true, Wait vbus lower than B_SESSION_VALID
> > + * : false, Wait vbus higher than A_VBUS_VALID
>
> Instead of this you could just pass otgsc bit mask here (OTGSC_BSV or
> OTGSC_AVV), would make it shorter and easier to read from the caller.
>
OK, will change
> > */
> > -static void ci_role_work(struct work_struct *work)
> > +static void ci_wait_vbus_stable(struct ci13xxx *ci, bool low)
> > +{
> > + unsigned long timeout;
> > + u32 otgsc = hw_read(ci, OP_OTGSC, ~0);
> > +
> > + timeout = jiffies + CI_WAIT_VBUS_STABLE_TIMEOUT;
> > +
> > + if (low) {
> > + while (otgsc & OTGSC_BSV) {
> > + if (time_after(jiffies, timeout)) {
> > + dev_err(ci->dev, "wait vbus lower than\
> > + B_SESS_VALID timeout!\n");
> > + return;
> > + }
> > + msleep(20);
> > + otgsc = hw_read(ci, OP_OTGSC, ~0);
> > + }
> > + } else {
> > + while (!(otgsc & OTGSC_AVV)) {
> > + if (time_after(jiffies, timeout)) {
> > + dev_err(ci->dev, "wait vbus higher than\
> > + A_VBUS_VALID timeout!\n");
> > + return;
> > + }
> > + msleep(20);
> > + otgsc = hw_read(ci, OP_OTGSC, ~0);
> > + }
> > +
> > + }
> > +}
> > +
> > +static void ci_handle_id_switch(struct ci13xxx *ci)
> > {
> > - struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
> > enum ci_role role = ci_otg_role(ci);
> >
> > if (role != ci->role) {
> > dev_dbg(ci->dev, "switching from %s to %s\n",
> > ci_role(ci)->name, ci->roles[role]->name);
> >
> > - ci_role_stop(ci);
> > - ci_role_start(ci, role);
> > - enable_irq(ci->irq);
> > + /* 1. Finish the current role */
> > + if (ci->role == CI_ROLE_GADGET) {
> > + usb_gadget_vbus_disconnect(&ci->gadget);
> > + /* host doesn't care B_SESSION_VALID event */
> > + hw_write(ci, OP_OTGSC, OTGSC_BSVIE, ~OTGSC_BSVIE);
> > + hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> > + ci->role = CI_ROLE_END;
> > + /* reset controller */
> > + hw_write(ci, OP_USBCMD, USBCMD_RST, USBCMD_RST);
> > + while (hw_read(ci, OP_USBCMD, USBCMD_RST))
> > + udelay(10);
>
> I would prefer we use hw_device_reset() everywhere where we reset the
> controller. Actually, same goes for run/stop bit.
>
OK, I will use hw_device_reset(ci, USBMODE_CM_IDLE) to reset controller
For setting run/stop bit, I will use hw_device_state at ci13xxx_pullup
> > + /* Disable all interrupts bits */
> > + hw_write(ci, OP_USBINTR, 0xffffffff, 0);
> > + hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, ~OTGSC_INT_EN_BITS);
>
> hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, 0);
> will work to the same effect and is a bit easier on the eyes.
OK, I will change.
>
> >
> > if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> > + dev_info(dev, "support otg\n");
>
> Looks more like a dev_dbg() to me.
OK, will change.
>
> > ci->is_otg = true;
> > /* ID pin needs 1ms debouce time, we delay 2ms for safe */
> > mdelay(2);
> > @@ -475,13 +617,53 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> > : CI_ROLE_GADGET;
> > }
> >
> > + if (ci->is_otg) {
> > + /* if otg is supported, create struct usb_otg */
> > + ci_hdrc_otg_init(ci);
> > +
> > + /*
> > + * If it is host role at otg port, start gadget role first
> > + * as we need to allocate device struct.
> > + */
> > + if (ci->role == CI_ROLE_HOST) {
> > +
> > + ret = ci_role_start(ci, CI_ROLE_GADGET);
>
> This is kind of a hack. In the worst case you could move gadegt
> initialization to the role's init method. Or we could add a role
> destructor method and then
> 1) allocate gadget stuff on first call to role start
> 2) make role stop a nop
> 3) deallocate gadget stuff on role destroy()
> 4) call role's destroy() in device remove path instead of role stop
> 5) call ci_role_start/ci_role_stop from ci_handle_id_switch()
> unconditionally.
> Does this make sense to you?
I prefer to just add gadget initialization at the role's init method.
Or we need to add some role_create and role_destroy API for device,
and add some conditional code.
For host, everything is ok
ci_hdrc_probe: call ci_role_start at host mode
ci_hdrc_remove: call ci_role_stop
ci_handle_id_switch: call ci_role_start/ci_role_stop when role changes.
For device, we can't make things the same.
ci_hdrc_probe: call udc_start at both device/host mode.
ci_hdrc_remove: call ci_role_stop
ci_handle_id_switch: call usb_gadget_vbus_connect/usb_gadget_vbus_disconnect
> >
> > + otgsc = hw_read(ci, OP_OTGSC, ~0);
> > + /*
> > + * if it is device mode:
> > + * - Enable vbus detect
> > + * - If it has already connected to host, notify udc
> > + */
> > + if (ci->role == CI_ROLE_GADGET) {
> > + hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
> > + if (otgsc & OTGSC_BSV)
> > + usb_gadget_vbus_connect(&ci->gadget);
>
> This looks a lot like ci_handle_vbus_change(), doesn't it?
Great, I will change.
>
> > + }
> > +
> > platform_set_drvdata(pdev, ci);
> > ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name,
> > ci);
> > @@ -492,8 +674,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> > if (ret)
> > goto rm_attr;
> >
> > - if (ci->is_otg)
> > - hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
> > + queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500));
>
> CI_WAIT_VBUS_STABLE_TIMEOUT?
Not exactly, it is the time for waiting platform probe finishes,
as the vbus callback will be created according to different platforms.
>
> > +int ci_hdrc_otg_init(struct ci13xxx *ci)
> > +{
> > + /* Useless at current */
> > + ci->otg.set_peripheral = ci_otg_set_peripheral;
> > + ci->otg.set_host = ci_otg_set_host;
> > + ci->transceiver->otg = &ci->otg;
>
> This will blow up if we don't have a transceiver.
>
will add
if (!IS_ERR_OR_NULL(ci->transceiver)) {
ci->transceiver->otg = &ci->otg;
}
--
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
