2013-05-29 13:07:10

by B, Ravi

[permalink] [raw]
Subject: [PATCH v1 0/3] babble error workaround for am335x platform

During the babble condition on usb bus, the musb controller removes the
session and stops host mode functionality. All the devices connected
to root port will be disconnected due to this babble event.

As part of recovery of babble bus condition, restarting the
controller is needed as all devices got disconnected. Just setting the
session bit would not be sufficient as the configured endpoint fifo
register getting changed during this condition as confirmed by HW/IP
owner of musb controller.

This patch set series adds the support to handle the babble
error recovery mechanism, this can be extended to other musb platforms.

This patch has been verified on tree [1] and tested on am335x platforms.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git

Ravi Babu (3):
usb: musb: core: added musb_restart() API to handle babble condition
usb: musb: core: added babble recovery func-ptr to musb->ops
usb: musb: dsps: handle babble condition for dsps platform

drivers/usb/musb/musb_core.c | 30 ++++++++++++++++++++++++++++++
drivers/usb/musb/musb_core.h | 8 ++++++++
drivers/usb/musb/musb_dsps.c | 34 +++++++++++++++++++++++++++++++++-
3 files changed, 71 insertions(+), 1 deletions(-)


2013-05-29 13:07:17

by B, Ravi

[permalink] [raw]
Subject: [PATCH v1 3/3] usb: musb: dsps: handle babble condition for dsps platform

Adding babble recovery mechanism for dsps platform and as part
of babble recovery the controller will be restarted.

Signed-off-by: Ravi Babu <[email protected]>
---
drivers/usb/musb/musb_dsps.c | 34 +++++++++++++++++++++++++++++++++-
1 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 958c6b6..efe95e1 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -294,9 +294,17 @@ static irqreturn_t dsps_interrupt(int irq, void *hci)
* value but DEVCTL.BDEVICE is invalid without DEVCTL.SESSION set.
* Also, DRVVBUS pulses for SRP (but not at 5V) ...
*/
- if (is_host_active(musb) && usbintr & MUSB_INTR_BABBLE)
+ if (is_host_active(musb) && usbintr & MUSB_INTR_BABBLE) {
pr_info("CAUTION: musb: Babble Interrupt Occurred\n");

+ /* during babble condition the musb controller removes
+ * session (or stops) and no longer in host mode. Hence
+ * all the devices connected to root hub gets disconnected
+ */
+ musb->int_usb = MUSB_INTR_BABBLE | MUSB_INTR_DISCONNECT;
+ musb->int_tx = musb->int_rx = 0;
+ }
+
if (usbintr & ((1 << wrp->drvvbus) << wrp->usb_shift)) {
int drvvbus = dsps_readl(reg_base, wrp->status);
void __iomem *mregs = musb->mregs;
@@ -428,6 +436,29 @@ static int dsps_musb_exit(struct musb *musb)
return 0;
}

+static void dsps_musb_restart(struct musb *musb)
+{
+ struct device *dev = musb->controller;
+ struct dsps_glue *glue = dev_get_drvdata(dev->parent);
+ const struct dsps_musb_wrapper *wrp = glue->wrp;
+ void __iomem *reg_base = musb->ctrl_base;
+
+ /* Reset the musb */
+ dsps_writel(reg_base, wrp->control, (1 << wrp->reset));
+ udelay(100);
+
+ /* Stop the on-chip PHY and its PLL. */
+ usb_phy_vbus_off(musb->xceiv);
+ udelay(100);
+
+ /* Start the on-chip PHY and its PLL. */
+ usb_phy_vbus_on(musb->xceiv);
+ udelay(100);
+
+ /* reinit the endpoint fifo table and restart musb */
+ musb_restart(musb);
+}
+
static struct musb_platform_ops dsps_ops = {
.init = dsps_musb_init,
.exit = dsps_musb_exit,
@@ -436,6 +467,7 @@ static struct musb_platform_ops dsps_ops = {
.disable = dsps_musb_disable,

.try_idle = dsps_musb_try_idle,
+ .babble_recovery = dsps_musb_restart,
};

static u64 musb_dmamask = DMA_BIT_MASK(32);
--
1.7.0.4

2013-05-29 13:07:13

by B, Ravi

[permalink] [raw]
Subject: [PATCH v1 1/3] usb: musb: core: added musb_restart() API to handle babble condition

Added musb_restart() API, used for restart of the musb controller by
the glue layer, when there is babble condition occured on the bus.

During babble condition, the musb controller will remove the session
and no longer in host-mode. As part of recovery this API can be used
to restart the musb controller.

Signed-off-by: Ravi Babu <[email protected]>
---
drivers/usb/musb/musb_core.c | 24 ++++++++++++++++++++++++
drivers/usb/musb/musb_core.h | 1 +
2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 37a261a..ab6fa39 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1373,6 +1373,30 @@ static int ep_config_from_hw(struct musb *musb)
return 0;
}

+/*
+ * musb_restart - restarts musb controller
+ * @param musb the controller
+ */
+int musb_restart(struct musb *musb)
+{
+ int status = 0;
+
+ /* during babble condition the musb controller removes the
+ * session bit and the fifo table initialized value get changed
+ */
+ if (musb->dyn_fifo)
+ status = ep_config_from_table(musb);
+ else
+ status = ep_config_from_hw(musb);
+
+ /* starts session */
+ if (!status)
+ musb_start(musb);
+
+ return status;
+}
+EXPORT_SYMBOL_GPL(musb_restart);
+
enum { MUSB_CONTROLLER_MHDRC, MUSB_CONTROLLER_HDRC, };

/* Initialize MUSB (M)HDRC part of the USB hardware subsystem;
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 7fb4819..f96e899 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -522,6 +522,7 @@ extern const char musb_driver_name[];

extern void musb_start(struct musb *musb);
extern void musb_stop(struct musb *musb);
+extern int musb_restart(struct musb *musb);

extern void musb_write_fifo(struct musb_hw_ep *ep, u16 len, const u8 *src);
extern void musb_read_fifo(struct musb_hw_ep *ep, u16 len, u8 *dst);
--
1.7.0.4

2013-05-29 13:07:45

by B, Ravi

[permalink] [raw]
Subject: [PATCH v1 2/3] usb: musb: core: added babble recovery func-ptr to musb->ops

Adding babble_recovery operation as part of musb->ops, used
to recover from babble condition during babble interrupt.

Signed-off-by: Ravi Babu <[email protected]>
---
drivers/usb/musb/musb_core.c | 6 ++++++
drivers/usb/musb/musb_core.h | 7 +++++++
2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index ab6fa39..411c29d 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -857,6 +857,12 @@ b_host:
}
}

+ /* handle babble condition */
+ if (int_usb & MUSB_INTR_BABBLE) {
+ pr_info("babble: restarting the musb controller..");
+ musb_babble_recovery(musb);
+ }
+
#if 0
/* REVISIT ... this would be for multiplexing periodic endpoints, or
* supporting transfer phasing to prevent exceeding ISO bandwidth
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index f96e899..bf37dc9 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -213,6 +213,8 @@ struct musb_platform_ops {
int (*adjust_channel_params)(struct dma_channel *channel,
u16 packet_sz, u8 *mode,
dma_addr_t *dma_addr, u32 *len);
+
+ void (*babble_recovery)(struct musb *musb);
};

/*
@@ -590,4 +592,9 @@ static inline int musb_platform_exit(struct musb *musb)
return musb->ops->exit(musb);
}

+static inline void musb_babble_recovery(struct musb *musb)
+{
+ if (musb->ops->babble_recovery)
+ musb->ops->babble_recovery(musb);
+}
#endif /* __MUSB_CORE_H__ */
--
1.7.0.4

2013-06-26 08:25:08

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v1 1/3] usb: musb: core: added musb_restart() API to handle babble condition

On Wed, May 29, 2013 at 06:37:02PM +0530, Ravi Babu wrote:
> Added musb_restart() API, used for restart of the musb controller by
> the glue layer, when there is babble condition occured on the bus.
>
> During babble condition, the musb controller will remove the session
> and no longer in host-mode. As part of recovery this API can be used
> to restart the musb controller.
>
> Signed-off-by: Ravi Babu <[email protected]>
> ---
> drivers/usb/musb/musb_core.c | 24 ++++++++++++++++++++++++
> drivers/usb/musb/musb_core.h | 1 +
> 2 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 37a261a..ab6fa39 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1373,6 +1373,30 @@ static int ep_config_from_hw(struct musb *musb)
> return 0;
> }
>
> +/*
> + * musb_restart - restarts musb controller
> + * @param musb the controller
> + */
> +int musb_restart(struct musb *musb)
> +{
> + int status = 0;
> +
> + /* during babble condition the musb controller removes the
> + * session bit and the fifo table initialized value get changed
> + */
> + if (musb->dyn_fifo)
> + status = ep_config_from_table(musb);
> + else
> + status = ep_config_from_hw(musb);
> +
> + /* starts session */
> + if (!status)
> + musb_start(musb);
> +
> + return status;
> +}
> +EXPORT_SYMBOL_GPL(musb_restart);

this sort of function should never be exposed outside of MUSB core
itself. This points to a big problem somewhere else.

let me continue reading the other two patches...

--
balbi


Attachments:
(No filename) (1.56 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-26 08:26:29

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v1 2/3] usb: musb: core: added babble recovery func-ptr to musb->ops

Hi,

On Wed, May 29, 2013 at 06:37:03PM +0530, Ravi Babu wrote:
> Adding babble_recovery operation as part of musb->ops, used
> to recover from babble condition during babble interrupt.
>
> Signed-off-by: Ravi Babu <[email protected]>
> ---
> drivers/usb/musb/musb_core.c | 6 ++++++
> drivers/usb/musb/musb_core.h | 7 +++++++
> 2 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index ab6fa39..411c29d 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -857,6 +857,12 @@ b_host:
> }
> }
>
> + /* handle babble condition */
> + if (int_usb & MUSB_INTR_BABBLE) {
> + pr_info("babble: restarting the musb controller..");
> + musb_babble_recovery(musb);
> + }
> +
> #if 0
> /* REVISIT ... this would be for multiplexing periodic endpoints, or
> * supporting transfer phasing to prevent exceeding ISO bandwidth
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index f96e899..bf37dc9 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -213,6 +213,8 @@ struct musb_platform_ops {
> int (*adjust_channel_params)(struct dma_channel *channel,
> u16 packet_sz, u8 *mode,
> dma_addr_t *dma_addr, u32 *len);
> +
> + void (*babble_recovery)(struct musb *musb);

I don't get why can't 'babble_recovery' be generic. Why do we need each
glue layer to implement it ?

--
balbi


Attachments:
(No filename) (1.44 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-26 08:30:12

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v1 3/3] usb: musb: dsps: handle babble condition for dsps platform

Hi,

On Wed, May 29, 2013 at 06:37:04PM +0530, Ravi Babu wrote:
> @@ -428,6 +436,29 @@ static int dsps_musb_exit(struct musb *musb)
> return 0;
> }
>
> +static void dsps_musb_restart(struct musb *musb)
> +{
> + struct device *dev = musb->controller;
> + struct dsps_glue *glue = dev_get_drvdata(dev->parent);
> + const struct dsps_musb_wrapper *wrp = glue->wrp;
> + void __iomem *reg_base = musb->ctrl_base;
> +
> + /* Reset the musb */
> + dsps_writel(reg_base, wrp->control, (1 << wrp->reset));
> + udelay(100);
> +
> + /* Stop the on-chip PHY and its PLL. */
> + usb_phy_vbus_off(musb->xceiv);
> + udelay(100);
> +
> + /* Start the on-chip PHY and its PLL. */
> + usb_phy_vbus_on(musb->xceiv);
> + udelay(100);
> +
> + /* reinit the endpoint fifo table and restart musb */
> + musb_restart(musb);

everything here, except for the glue reset, is generic. The thing is
that I don't really think the IP needs to be reset to recover from
babble.

Babble is a valid error condition, it's not even fatal right.

--
balbi


Attachments:
(No filename) (1.00 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-26 08:37:47

by B, Ravi

[permalink] [raw]
Subject: RE: [PATCH v1 2/3] usb: musb: core: added babble recovery func-ptr to musb->ops

Felipe

>
> On Wed, May 29, 2013 at 06:37:03PM +0530, Ravi Babu wrote:
> > Adding babble_recovery operation as part of musb->ops, used to recover
> > from babble condition during babble interrupt.
> >
> > Signed-off-by: Ravi Babu <[email protected]>
> > ---
> > drivers/usb/musb/musb_core.c | 6 ++++++
> > drivers/usb/musb/musb_core.h | 7 +++++++
> > 2 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_core.c
> > b/drivers/usb/musb/musb_core.c index ab6fa39..411c29d 100644
> > --- a/drivers/usb/musb/musb_core.c
> > +++ b/drivers/usb/musb/musb_core.c
> > @@ -857,6 +857,12 @@ b_host:
> > }
> > }
> >
> > + /* handle babble condition */
> > + if (int_usb & MUSB_INTR_BABBLE) {
> > + pr_info("babble: restarting the musb controller..");
> > + musb_babble_recovery(musb);
> > + }
> > +
> > #if 0
> > /* REVISIT ... this would be for multiplexing periodic endpoints, or
> > * supporting transfer phasing to prevent exceeding ISO bandwidth
> > diff --git a/drivers/usb/musb/musb_core.h
> > b/drivers/usb/musb/musb_core.h index f96e899..bf37dc9 100644
> > --- a/drivers/usb/musb/musb_core.h
> > +++ b/drivers/usb/musb/musb_core.h
> > @@ -213,6 +213,8 @@ struct musb_platform_ops {
> > int (*adjust_channel_params)(struct dma_channel *channel,
> > u16 packet_sz, u8 *mode,
> > dma_addr_t *dma_addr, u32 *len);
> > +
> > + void (*babble_recovery)(struct musb *musb);
>
> I don't get why can't 'babble_recovery' be generic. Why do we need each glue
> layer to implement it ?
>

Babble is generic, but recovery mechanism is nothing but "reset of usbss" which is SoC dependent and followed by generic restart of the musb controller.
That is why musb_restart() API is exported used by all glue in babble recovery.

--
Ravi B

2013-06-26 08:53:45

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v1 2/3] usb: musb: core: added babble recovery func-ptr to musb->ops

On Wed, Jun 26, 2013 at 10:37:39AM +0200, B, Ravi wrote:
> Felipe
>
> >
> > On Wed, May 29, 2013 at 06:37:03PM +0530, Ravi Babu wrote:
> > > Adding babble_recovery operation as part of musb->ops, used to recover
> > > from babble condition during babble interrupt.
> > >
> > > Signed-off-by: Ravi Babu <[email protected]>
> > > ---
> > > drivers/usb/musb/musb_core.c | 6 ++++++
> > > drivers/usb/musb/musb_core.h | 7 +++++++
> > > 2 files changed, 13 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/usb/musb/musb_core.c
> > > b/drivers/usb/musb/musb_core.c index ab6fa39..411c29d 100644
> > > --- a/drivers/usb/musb/musb_core.c
> > > +++ b/drivers/usb/musb/musb_core.c
> > > @@ -857,6 +857,12 @@ b_host:
> > > }
> > > }
> > >
> > > + /* handle babble condition */
> > > + if (int_usb & MUSB_INTR_BABBLE) {
> > > + pr_info("babble: restarting the musb controller..");
> > > + musb_babble_recovery(musb);
> > > + }
> > > +
> > > #if 0
> > > /* REVISIT ... this would be for multiplexing periodic endpoints, or
> > > * supporting transfer phasing to prevent exceeding ISO bandwidth
> > > diff --git a/drivers/usb/musb/musb_core.h
> > > b/drivers/usb/musb/musb_core.h index f96e899..bf37dc9 100644
> > > --- a/drivers/usb/musb/musb_core.h
> > > +++ b/drivers/usb/musb/musb_core.h
> > > @@ -213,6 +213,8 @@ struct musb_platform_ops {
> > > int (*adjust_channel_params)(struct dma_channel *channel,
> > > u16 packet_sz, u8 *mode,
> > > dma_addr_t *dma_addr, u32 *len);
> > > +
> > > + void (*babble_recovery)(struct musb *musb);
> >
> > I don't get why can't 'babble_recovery' be generic. Why do we need each glue
> > layer to implement it ?
> >
>
> Babble is generic, but recovery mechanism is nothing but "reset of
> usbss" which is SoC dependent and followed by generic restart of the
> musb controller.

and that's what I don't get. Why do you need to reset usbss ?

--
balbi


Attachments:
(No filename) (1.88 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-26 09:09:03

by B, Ravi

[permalink] [raw]
Subject: RE: [PATCH v1 2/3] usb: musb: core: added babble recovery func-ptr to musb->ops

Hi Felipe

> > > > @@ -213,6 +213,8 @@ struct musb_platform_ops {
> > > > int (*adjust_channel_params)(struct dma_channel *channel,
> > > > u16 packet_sz, u8 *mode,
> > > > dma_addr_t *dma_addr, u32 *len);
> > > > +
> > > > + void (*babble_recovery)(struct musb *musb);
> > >
> > > I don't get why can't 'babble_recovery' be generic. Why do we need
> > > each glue layer to implement it ?
> > >
> >
> > Babble is generic, but recovery mechanism is nothing but "reset of
> > usbss" which is SoC dependent and followed by generic restart of the
> > musb controller.
>
> and that's what I don't get. Why do you need to reset usbss ?

On babble condition, the session bit is removed by mentor, hence to make musb work
1) setting the session alone will not help.
2) restart the usbss and setting session also not helped.
3) musb works only after usbss reset followed by epfifo table init and re-enable all interrupts, then set the session.

The mentor IP guys (synopsis) confirmed that during babble condition, controller is stopped. Only recover is to restart completely, usbss reset, reinit epfifo table, & set the session.

>
> --
> balbi