On Thu, Nov 24, 2011 at 09:59:12PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 11/24/2011 04:46 PM, Felipe Balbi wrote:
>
> >we are compiling the driver always with full OTG
> >capabilities, so that board_mode trick becomes
> >useless.
>
> >Signed-off-by: Felipe Balbi<balbi@xxxxxx>
> >---
>
> >I would like to get Acks from blackfin, da8xx and davinci guys, please.
>
> After having looked thru the patch, I have a few questions...
>
> >diff --git a/drivers/usb/musb/am35x.c b/drivers/usb/musb/am35x.c
> >index e233d2b..34c91ac 100644
> >--- a/drivers/usb/musb/am35x.c
> >+++ b/drivers/usb/musb/am35x.c
> [...]
> >@@ -269,8 +262,7 @@ static irqreturn_t am35x_musb_interrupt(int irq, void *hci)
> > u8 devctl = musb_readb(mregs, MUSB_DEVCTL);
> > int err;
> >
> >- err = is_host_enabled(musb) && (musb->int_usb&
> >- MUSB_INTR_VBUSERROR);
> >+ err = (musb->int_usb & MUSB_INTR_VBUSERROR);
>
> Could drop now useless parens...
indeed.
> >@@ -324,8 +316,7 @@ eoi:
> > }
> >
> > /* Poll for ID change */
> >- if (is_otg_enabled(musb) && musb->xceiv->state == OTG_STATE_B_IDLE)
> >- mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
> >+ mod_timer(&otg_workaround, jiffies + POLL_SECONDS * HZ);
>
> Er, not sure why you dropped the whole *if* here, I think it should've been:
>
> if (musb->xceiv->state == OTG_STATE_B_IDLE)
that's correct.
>
> >diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
> >index 5e7cfba..1817dc8 100644
> >--- a/drivers/usb/musb/blackfin.c
> >+++ b/drivers/usb/musb/blackfin.c
> [...]
> >@@ -324,8 +316,7 @@ static int bfin_musb_set_power(struct otg_transceiver *x, unsigned mA)
> >
> > static void bfin_musb_try_idle(struct musb *musb, unsigned long timeout)
> > {
> >- if (!is_otg_enabled(musb) && is_host_enabled(musb))
> >- mod_timer(&musb_conn_timer, jiffies + TIMER_DELAY);
> >+ /* REVISIT is this really correct ? */
>
> Looks like the whole function needs to be dropped now...
true.
> >diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> >index 2613bfd..30614e7 100644
> >--- a/drivers/usb/musb/da8xx.c
> >+++ b/drivers/usb/musb/da8xx.c
> [...]
> >@@ -331,8 +324,7 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
> > u8 devctl = musb_readb(mregs, MUSB_DEVCTL);
> > int err;
> >
> >- err = is_host_enabled(musb) && (musb->int_usb&
> >- MUSB_INTR_VBUSERROR);
> >+ err = (musb->int_usb & USB_INTR_VBUSERROR);
>
> Could drop now useless parens...
correct.
> > if (err) {
> > /*
> > * The Mentor core doesn't debounce VBUS as needed
>
> >diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c
> >index f9a3f62..e160c5e 100644
> >--- a/drivers/usb/musb/davinci.c
> >+++ b/drivers/usb/musb/davinci.c
> [...]
> >@@ -315,8 +312,7 @@ static irqreturn_t davinci_musb_interrupt(int irq, void *__hci)
> > u8 devctl = musb_readb(mregs, MUSB_DEVCTL);
> > int err = musb->int_usb & MUSB_INTR_VBUSERROR;
> >
> >- err = is_host_enabled(musb)
> >- && (musb->int_usb & MUSB_INTR_VBUSERROR);
> >+ err = (musb->int_usb & MUSB_INTR_VBUSERROR);
>
> Could drop now useless parens...
correct.
> >@@ -419,12 +413,8 @@ static int davinci_musb_init(struct musb *musb)
> > if (cpu_is_davinci_dm355()) {
> > u32 deepsleep = __raw_readl(DM355_DEEPSLEEP);
> >
> >- if (is_host_enabled(musb)) {
> >- deepsleep&= ~DRVVBUS_OVERRIDE;
> >- } else {
> >- deepsleep&= ~DRVVBUS_FORCE;
> >- deepsleep |= DRVVBUS_OVERRIDE;
> >- }
> >+ deepsleep&= ~DRVVBUS_FORCE;
> >+ deepsleep |= DRVVBUS_OVERRIDE;
>
> Not clear why -- is_host_enabled(musb) is true in OTG mode...
nice catch. my bad.
> [...]
> >diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> >index 5793095..f925fea 100644
> >--- a/drivers/usb/musb/musb_core.c
> >+++ b/drivers/usb/musb/musb_core.c
> [...]
> >@@ -1957,59 +1943,11 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
> > musb->irq_wake = 0;
> > }
> >
> >- /* host side needs more setup */
> >- if (is_host_enabled(musb)) {
> >- struct usb_hcd *hcd = musb_to_hcd(musb);
> >-
> >- otg_set_host(musb->xceiv,&hcd->self);
> >+ MUSB_DEV_MODE(musb);
> >+ musb->xceiv->default_a = 0;
> >+ musb->xceiv->state = OTG_STATE_B_IDLE;
> >
> >- if (is_otg_enabled(musb))
> >- hcd->self.otg_port = 1;
> >- musb->xceiv->host =&hcd->self;
> >- hcd->power_budget = 2 * (plat->power ? : 250);
> >-
> >- /* program PHY to use external vBus if required */
> >- if (plat->extvbus) {
> >- u8 busctl = musb_read_ulpi_buscontrol(musb->mregs);
> >- busctl |= MUSB_ULPI_USE_EXTVBUS;
> >- musb_write_ulpi_buscontrol(musb->mregs, busctl);
> >- }
> >- }
>
> Not clear why are dropping the above block -- is_host_enabled() is
> true in OTG mode, so I think you should have kept it, just moved out
> of *if*.
that's right.
> [...]
>
> >- } else /* peripheral is enabled */ {
> >- MUSB_DEV_MODE(musb);
> >- musb->xceiv->default_a = 0;
> >- musb->xceiv->state = OTG_STATE_B_IDLE;
> >-
> >- status = musb_gadget_setup(musb);
> >-
> >- dev_dbg(musb->controller, "%s mode, status %d, dev%02x\n",
> >- is_otg_enabled(musb) ? "OTG" : "PERIPHERAL",
> >- status,
> >- musb_readb(musb->mregs, MUSB_DEVCTL));
>
> You think this dev_dbg() became worthless?
yes. It will always print OTG and DEVCTL. DEVCTL you can get from the
debugfs register dump interface at any time.
> >-
> >- }
> >+ status = musb_gadget_setup(musb);
> > if (status< 0)
> > goto fail3;
> >
> >@@ -2025,28 +1963,13 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
> > goto fail5;
> > #endif
> >
> >- dev_info(dev, "USB %s mode controller at %p using %s, IRQ %d\n",
> >- ({char *s;
> >- switch (musb->board_mode) {
> >- case MUSB_HOST: s = "Host"; break;
> >- case MUSB_PERIPHERAL: s = "Peripheral"; break;
> >- default: s = "OTG"; break;
> >- }; s; }),
> >- ctrl,
> >- (is_dma_capable()&& musb->dma_controller)
> >- ? "DMA" : "PIO",
> >- musb->nIrq);
>
> Er, this dev_info() is useful even without musb->board_mode...
it's not. IRQ you can fetch from /proc/interrupts, mode is always OTG,
and DMA is done correctly on Kconfig. Users don't need to see that on
every boot.
> [...]
> >diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> >index 3d28fb8..af94d10 100644
> >--- a/drivers/usb/musb/musb_core.h
> >+++ b/drivers/usb/musb/musb_core.h
> >@@ -71,10 +71,6 @@ struct musb_ep;
> > #include<linux/usb/hcd.h>
> > #include "musb_host.h"
> >
> >-#define is_peripheral_enabled(musb) ((musb)->board_mode != MUSB_HOST)
> >-#define is_host_enabled(musb) ((musb)->board_mode != MUSB_PERIPHERAL)
> >-#define is_otg_enabled(musb) ((musb)->board_mode == MUSB_OTG)
> >-
> > /* NOTE: otg and peripheral-only state machines start at B_IDLE.
> > * OTG or host-only go to A_IDLE when ID is sensed.
> > */
>
> You didn't drop 'board_mode' itself from 'struct musb'...
indeed.
> >diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> >index 87d78c0..22220fe 100644
> >--- a/drivers/usb/musb/musb_gadget.c
> >+++ b/drivers/usb/musb/musb_gadget.c
> [...]
> >@@ -1898,11 +1897,14 @@ static int musb_gadget_start(struct usb_gadget *g,
> > struct usb_gadget_driver *driver)
> > {
> > struct musb *musb = gadget_to_musb(g);
> >+ struct usb_hcd *hcd = musb_to_hcd(musb);
> > unsigned long flags;
> >- int retval = -EINVAL;
> >+ int retval = 0;
> [...]
> >@@ -1916,49 +1918,28 @@ static int musb_gadget_start(struct usb_gadget *g,
> >
> > otg_set_peripheral(musb->xceiv,&musb->g);
> > musb->xceiv->state = OTG_STATE_B_IDLE;
> >+ spin_unlock_irqrestore(&musb->lock, flags);
> >
> > /*
> >- * FIXME this ignores the softconnect flag. Drivers are
> >- * allowed hold the peripheral inactive until for example
> >- * userspace hooks up printer hardware or DSP codecs, so
> >- * hosts only see fully functional devices.
> >+ * REVISIT: funcall to other code, which also
> >+ * handles power budgeting ... this way also
> >+ * ensures HdrcStart is indirectly called.
> > */
> >+ retval = usb_add_hcd(musb_to_hcd(musb), -1, 0);
>
> Er, you declared/init'ed 'hcd' abobe, why didn't you use it here?
hehe, go figure :-)
> >diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> >index e9f80ad..56ca2e4 100644
> >--- a/drivers/usb/musb/musb_virthub.c
> >+++ b/drivers/usb/musb/musb_virthub.c
> [...]
> >@@ -270,8 +267,6 @@ int musb_hub_control(
> > musb_port_suspend(musb, false);
> > break;
> > case USB_PORT_FEAT_POWER:
> >- if (!(is_otg_enabled(musb) && hcd->self.is_b_host))
>
> Er, not sure this *if* should be completely dropped, shouldn't it be:
>
> if (!hcd->self.is_b_host)
yes, it should.
> >- musb_platform_set_vbus(musb, 0);
> > break;
> > case USB_PORT_FEAT_C_CONNECTION:
> > case USB_PORT_FEAT_C_ENABLE:
> >@@ -356,18 +351,6 @@ int musb_hub_control(
> >
> > switch (wValue) {
> > case USB_PORT_FEAT_POWER:
> >- /* NOTE: this controller has a strange state machine
> >- * that involves "requesting sessions" according to
> >- * magic side effects from incompletely-described
> >- * rules about startup...
> >- *
> >- * This call is what really starts the host mode; be
> >- * very careful about side effects if you reorder any
> >- * initialization logic, e.g. for OTG, or change any
> >- * logic relating to VBUS power-up.
> >- */
> >- if (!(is_otg_enabled(musb) && hcd->self.is_b_host))
>
> Same comment as above.
same reply.
--
balbi
Attachment:
signature.asc
Description: Digital signature
