On 5/23/2014 3:01 PM, Daniel Mack wrote:
Before accessing any of an endpoint's CSR registers, make sure the
correct endpoint is selected. Otherwise, data read from or written to
the registers is likely to affect the wrong endpoint as long as the
connected device has more than one endpoint.
This, of course, leads to all sorts of strange effects such as stream
starvation and driver internal state machine confusion due to spurious
interrupts.
Signed-off-by: Daniel Mack <zonque@xxxxxxxxx>
---
drivers/usb/musb/musb_cppi41.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index a11bbb6..db497dd 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -58,14 +58,17 @@ struct cppi41_dma_controller {
static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel)
{
+ struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
+ struct musb *musb = hw_ep->musb;
u16 csr;
u8 toggle;
if (cppi41_channel->is_tx)
return;
- if (!is_host_active(cppi41_channel->controller->musb))
+ if (!is_host_active(musb))
return;
+ musb_ep_select(musb->mregs, hw_ep->epnum);
save_rx_toggle is called from cppi41_configure_channel as part of
channel_program() from musb_host_rx()
and musb_ep_program(). Both these functions call musb_ep_select() before
calling channel_program().
Both musb_ep_program() and musb_host_rx() are called with IRQ's disabled.
Still do we need this musb_ep_select() in save_rx_toggle? Or am I confused?
csr = musb_readw(cppi41_channel->hw_ep->regs, MUSB_RXCSR);
toggle = csr & MUSB_RXCSR_H_DATATOGGLE ? 1 : 0;
@@ -74,15 +77,18 @@ static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel)
static void update_rx_toggle(struct cppi41_dma_channel *cppi41_channel)
{
+ struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
+ struct musb *musb = hw_ep->musb;
u16 csr;
u8 toggle;
if (cppi41_channel->is_tx)
return;
- if (!is_host_active(cppi41_channel->controller->musb))
+ if (!is_host_active(musb))
return;
- csr = musb_readw(cppi41_channel->hw_ep->regs, MUSB_RXCSR);
+ musb_ep_select(musb->mregs, hw_ep->epnum);
Yes it makes sense to do musb_ep_select() in update_rx_toggle().
+ csr = musb_readw(hw_ep->regs, MUSB_RXCSR);
toggle = csr & MUSB_RXCSR_H_DATATOGGLE ? 1 : 0;
/*
@@ -107,6 +113,7 @@ static bool musb_is_tx_fifo_empty(struct musb_hw_ep *hw_ep)
void __iomem *epio = musb->endpoints[epnum].regs;
u16 csr;
+ musb_ep_select(musb->mregs, hw_ep->epnum);
Yes it makes sense here.
csr = musb_readw(epio, MUSB_TXCSR);
if (csr & MUSB_TXCSR_TXPKTRDY)
return false;
@@ -173,6 +180,7 @@ static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel)
dma_async_issue_pending(dc);
if (!cppi41_channel->is_tx) {
+ musb_ep_select(musb->mregs, hw_ep->epnum);
Since in update_rx_toggle () we did musb_epe_select(), do we really it
again here?
csr = musb_readw(epio, MUSB_RXCSR);
csr |= MUSB_RXCSR_H_REQPKT;
musb_writew(epio, MUSB_RXCSR, csr);
@@ -551,6 +559,7 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel)
{
struct cppi41_dma_channel *cppi41_channel = channel->private_data;
struct cppi41_dma_controller *controller = cppi41_channel->controller;
+ struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
struct musb *musb = controller->musb;
void __iomem *epio = cppi41_channel->hw_ep->regs;
int tdbit;
@@ -565,6 +574,8 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel)
if (cppi41_channel->channel.status == MUSB_DMA_STATUS_FREE)
return 0;
musb_ep_select is done before calling channel_abort.
+ musb_ep_select(musb->mregs, hw_ep->epnum);
+
list_del_init(&cppi41_channel->tx_check);
if (is_tx) {
csr = musb_readw(epio, MUSB_TXCSR);
--
-George
--
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
