[PATCH v1 0/3] babble error workaround for am335x platform

[PATCH v1 0/3] babble error workaround for am335x platform

[Ravi Babu]

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(-)

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

[Ravi Babu]

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 <ravibabu@ti.com>
---
 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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

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 <ravibabu@ti.com>
> ---
>  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Ravi Babu]

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

Signed-off-by: Ravi Babu <ravibabu@ti.com>
---
 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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]

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 <ravibabu@ti.com>
> ---
>  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[B, Ravi]

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 <ravibabu@ti.com>
> > ---
> >  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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

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 <ravibabu@ti.com>
> > > ---
> > >  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[B, Ravi]

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

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

[Ravi Babu]

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

Signed-off-by: Ravi Babu <ravibabu@ti.com>
---
 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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]