Update MUSB CPPI 4.1 driver to correctly manage the DA8xx

PATCH Superseded Alexandre Bailon 2 participants 7 messages Sep 20, 2017 3 messages not in the archive

Subsystems (2)

USB MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

Alexandre Bailon

author

2017-09-20 07:35 UTC #0 lore raw

v2 cover Update MUSB CPPI 4.1 driver to correctly manage the DA8xx

A couple of weeks ago, Sekhar reported a warning issue happening in CPPI 4.1. The teardown sequence was not correctly programmed. The caused was a couple of difference between the DSPS and the DA8xx. These differences are the way to program the autoreq, the teardown and the DMA mode.
This series intends to fix the teardown and to correctly program the DMA mode.

This series has been roughly tested.
I have only tried it on the DA580 LCK and the BeagleBone Black. Only the device mode (msc and ecm) have been tested.

Changes in v2:
- Fix some typo (indentation and symbols name)
- Limit the number of channels to 4 for DA8xx
- Remove duplicated defines

Alexandre Bailon (4):
usb: musb: musb_cppi41: Fix the address of teardown and autoreq
registers
usb: musb: musb_cppi41: Fix cppi41_set_dma_mode() for DA8xx
usb: musb: musb_cppi41: Configure the number of channels for DA8xx
usb: musb: da8xx: Remove duplicated defines

 drivers/usb/musb/da8xx.c       |  3 --
 drivers/usb/musb/musb_cppi41.c | 94 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 82 insertions(+), 15 deletions(-)

Signature · 6 lines

2.13.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170920073513.11650-1-abailon-rdvid1DuHRBWk0Htik3J/[email protected]

Alexandre Bailon

author

2017-09-20 07:35 UTC #2 lore raw

PATCH v2 1/4 usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

The DA8xx and DSPS platforms don't use the same address for few registers. On Da8xx, this is causing some issues (e.g. teardown that doesn't work). Configure the address of the register during the init and use them instead of constants.

Reported-by [email protected]

Signed-off-by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/usb/musb/musb_cppi41.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

drivers/usb/musb/musb_cppi41.c +19 -4

index ba255280a624..d66416a27146 100644
@@ -26,6 +26,9 @@
 
 #define MUSB_DMA_NUM_CHANNELS 15
 
+#define DA8XX_USB_AUTOREQ	0x14
+#define DA8XX_USB_TEARDOWN	0x1c
+
 struct cppi41_dma_controller {
 	struct dma_controller controller;
 	struct cppi41_dma_channel rx_channel[MUSB_DMA_NUM_CHANNELS];

@@ -35,6 +38,9 @@ struct cppi41_dma_controller {
 	u32 rx_mode;
 	u32 tx_mode;
 	u32 auto_req;
+
+	u32 tdown_reg;
+	u32 autoreq_reg;
 };
 
 static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel)

@@ -364,8 +370,8 @@ static void cppi41_set_autoreq_mode(struct cppi41_dma_channel *cppi41_channel,
 	if (new_mode == old_mode)
 		return;
 	controller->auto_req = new_mode;
-	musb_writel(controller->controller.musb->ctrl_base, USB_CTRL_AUTOREQ,
-		    new_mode);
+	musb_writel(controller->controller.musb->ctrl_base,
+		    controller->autoreq_reg, new_mode);
 }
 
 static bool cppi41_configure_channel(struct dma_channel *channel,

@@ -581,12 +587,13 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel)
 
 	do {
 		if (is_tx)
-			musb_writel(musb->ctrl_base, USB_TDOWN, tdbit);
+			musb_writel(musb->ctrl_base, controller->tdown_reg,
+				    tdbit);
 		ret = dmaengine_terminate_all(cppi41_channel->dc);
 	} while (ret == -EAGAIN);
 
 	if (is_tx) {
-		musb_writel(musb->ctrl_base, USB_TDOWN, tdbit);
+		musb_writel(musb->ctrl_base, controller->tdown_reg, tdbit);
 
 		csr = musb_readw(epio, MUSB_TXCSR);
 		if (csr & MUSB_TXCSR_TXPKTRDY) {

@@ -727,6 +734,14 @@ cppi41_dma_controller_create(struct musb *musb, void __iomem *base)
 	controller->controller.is_compatible = cppi41_is_compatible;
 	controller->controller.musb = musb;
 
+	if (musb->io.quirks & MUSB_DA8XX) {
+		controller->tdown_reg = DA8XX_USB_TEARDOWN;
+		controller->autoreq_reg = DA8XX_USB_AUTOREQ;
+	} else {
+		controller->tdown_reg = USB_TDOWN;
+		controller->autoreq_reg = USB_CTRL_AUTOREQ;
+	}
+
 	ret = cppi41_dma_controller_start(controller);
 	if (ret)
 		goto plat_get_fail;

Signature · 6 lines

2.13.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexandre Bailon

author

2017-09-20 07:35 UTC #3 lore raw

PATCH v2 2/4 usb: musb: musb_cppi41: Fix cppi41_set_dma_mode() for DA8xx

In reply to a message not in the archive

The way to configure the DMA mode on DA8xx is different from DSPS. Add a new function to configure DMA mode on DA8xx and use a callback to call the right function based on the platform.

Signed-off-by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/usb/musb/musb_cppi41.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

drivers/usb/musb/musb_cppi41.c +37 -3

index d66416a27146..b2b1306c01cf 100644
@@ -26,6 +26,7 @@
 
 #define MUSB_DMA_NUM_CHANNELS 15
 
+#define DA8XX_USB_MODE		0x10
 #define DA8XX_USB_AUTOREQ	0x14
 #define DA8XX_USB_TEARDOWN	0x1c

@@ -41,6 +42,9 @@ struct cppi41_dma_controller {
 
 	u32 tdown_reg;
 	u32 autoreq_reg;
+
+	void (*set_dma_mode)(struct cppi41_dma_channel *cppi41_channel,
+			     unsigned int mode);
 };
 
 static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel)

@@ -355,6 +359,32 @@ static void cppi41_set_dma_mode(struct cppi41_dma_channel *cppi41_channel,
 	}
 }
 
+static void da8xx_set_dma_mode(struct cppi41_dma_channel *cppi41_channel,
+		unsigned int mode)
+{
+	struct cppi41_dma_controller *controller = cppi41_channel->controller;
+	struct musb *musb = controller->controller.musb;
+	unsigned int shift;
+	u32 port;
+	u32 new_mode;
+	u32 old_mode;
+
+	old_mode = controller->tx_mode;
+	port = cppi41_channel->port_num;
+
+	shift = (port - 1) * 4;
+	if (!cppi41_channel->is_tx)
+		shift += 16;
+	new_mode = old_mode & ~(3 << shift);
+	new_mode |= mode << shift;
+
+	if (new_mode == old_mode)
+		return;
+	controller->tx_mode = new_mode;
+	musb_writel(musb->ctrl_base, DA8XX_USB_MODE, new_mode);
+}
+
+
 static void cppi41_set_autoreq_mode(struct cppi41_dma_channel *cppi41_channel,
 		unsigned mode)
 {

@@ -379,6 +409,7 @@ static bool cppi41_configure_channel(struct dma_channel *channel,
 				dma_addr_t dma_addr, u32 len)
 {
 	struct cppi41_dma_channel *cppi41_channel = channel->private_data;
+	struct cppi41_dma_controller *controller = cppi41_channel->controller;
 	struct dma_chan *dc = cppi41_channel->dc;
 	struct dma_async_tx_descriptor *dma_desc;
 	enum dma_transfer_direction direction;

@@ -404,7 +435,7 @@ static bool cppi41_configure_channel(struct dma_channel *channel,
 			musb_writel(musb->ctrl_base,
 				RNDIS_REG(cppi41_channel->port_num), len);
 			/* gen rndis */
-			cppi41_set_dma_mode(cppi41_channel,
+			controller->set_dma_mode(cppi41_channel,
 					EP_MODE_DMA_GEN_RNDIS);
 
 			/* auto req */

@@ -413,14 +444,15 @@ static bool cppi41_configure_channel(struct dma_channel *channel,
 		} else {
 			musb_writel(musb->ctrl_base,
 					RNDIS_REG(cppi41_channel->port_num), 0);
-			cppi41_set_dma_mode(cppi41_channel,
+			controller->set_dma_mode(cppi41_channel,
 					EP_MODE_DMA_TRANSPARENT);
 			cppi41_set_autoreq_mode(cppi41_channel,
 					EP_MODE_AUTOREQ_NONE);
 		}
 	} else {
 		/* fallback mode */
-		cppi41_set_dma_mode(cppi41_channel, EP_MODE_DMA_TRANSPARENT);
+		controller->set_dma_mode(cppi41_channel,
+				EP_MODE_DMA_TRANSPARENT);
 		cppi41_set_autoreq_mode(cppi41_channel, EP_MODE_AUTOREQ_NONE);
 		len = min_t(u32, packet_sz, len);
 	}

@@ -737,9 +769,11 @@ cppi41_dma_controller_create(struct musb *musb, void __iomem *base)
 	if (musb->io.quirks & MUSB_DA8XX) {
 		controller->tdown_reg = DA8XX_USB_TEARDOWN;
 		controller->autoreq_reg = DA8XX_USB_AUTOREQ;
+		controller->set_dma_mode = da8xx_set_dma_mode;
 	} else {
 		controller->tdown_reg = USB_TDOWN;
 		controller->autoreq_reg = USB_CTRL_AUTOREQ;
+		controller->set_dma_mode = cppi41_set_dma_mode;
 	}
 
 	ret = cppi41_dma_controller_start(controller);

Signature · 6 lines

2.13.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexandre Bailon

author

2017-09-20 07:35 UTC #4 lore raw

PATCH v2 3/4 usb: musb: musb_cppi41: Configure the number of channels for DA8xx

In reply to a message not in the archive

Currently, the number of channels is set to 15 but in the case of DA8xx, the number of channels is 4.
Update the driver to configure the number of channels at runtime.

Signed-off-by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/usb/musb/musb_cppi41.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

drivers/usb/musb/musb_cppi41.c +26 -5

index b2b1306c01cf..2d1468cccae0 100644
@@ -30,10 +30,12 @@
 #define DA8XX_USB_AUTOREQ	0x14
 #define DA8XX_USB_TEARDOWN	0x1c
 
+#define DA8XX_DMA_NUM_CHANNELS 4
+
 struct cppi41_dma_controller {
 	struct dma_controller controller;
-	struct cppi41_dma_channel rx_channel[MUSB_DMA_NUM_CHANNELS];
-	struct cppi41_dma_channel tx_channel[MUSB_DMA_NUM_CHANNELS];
+	struct cppi41_dma_channel *rx_channel;
+	struct cppi41_dma_channel *tx_channel;
 	struct hrtimer early_tx;
 	struct list_head early_tx_list;
 	u32 rx_mode;

@@ -45,6 +47,7 @@ struct cppi41_dma_controller {
 
 	void (*set_dma_mode)(struct cppi41_dma_channel *cppi41_channel,
 			     unsigned int mode);
+	u8 num_channels;
 };
 
 static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel)

@@ -483,7 +486,7 @@ static struct dma_channel *cppi41_dma_channel_allocate(struct dma_controller *c,
 	struct cppi41_dma_channel *cppi41_channel = NULL;
 	u8 ch_num = hw_ep->epnum - 1;
 
-	if (ch_num >= MUSB_DMA_NUM_CHANNELS)
+	if (ch_num >= controller->num_channels)
 		return NULL;
 
 	if (is_tx)

@@ -643,7 +646,7 @@ static void cppi41_release_all_dma_chans(struct cppi41_dma_controller *ctrl)
 	struct dma_chan *dc;
 	int i;
 
-	for (i = 0; i < MUSB_DMA_NUM_CHANNELS; i++) {
+	for (i = 0; i < ctrl->num_channels; i++) {
 		dc = ctrl->tx_channel[i].dc;
 		if (dc)
 			dma_release_channel(dc);

@@ -695,7 +698,7 @@ static int cppi41_dma_controller_start(struct cppi41_dma_controller *controller)
 			goto err;
 
 		ret = -EINVAL;
-		if (port > MUSB_DMA_NUM_CHANNELS || !port)
+		if (port > controller->num_channels || !port)
 			goto err;
 		if (is_tx)
 			cppi41_channel = &controller->tx_channel[port - 1];

@@ -736,6 +739,8 @@ void cppi41_dma_controller_destroy(struct dma_controller *c)
 
 	hrtimer_cancel(&controller->early_tx);
 	cppi41_dma_controller_stop(controller);
+	kfree(controller->rx_channel);
+	kfree(controller->tx_channel);
 	kfree(controller);
 }
 EXPORT_SYMBOL_GPL(cppi41_dma_controller_destroy);

@@ -744,6 +749,7 @@ struct dma_controller *
 cppi41_dma_controller_create(struct musb *musb, void __iomem *base)
 {
 	struct cppi41_dma_controller *controller;
+	int channel_size;
 	int ret = 0;
 
 	if (!musb->controller->parent->of_node) {

@@ -770,18 +776,33 @@ cppi41_dma_controller_create(struct musb *musb, void __iomem *base)
 		controller->tdown_reg = DA8XX_USB_TEARDOWN;
 		controller->autoreq_reg = DA8XX_USB_AUTOREQ;
 		controller->set_dma_mode = da8xx_set_dma_mode;
+		controller->num_channels = DA8XX_DMA_NUM_CHANNELS;
 	} else {
 		controller->tdown_reg = USB_TDOWN;
 		controller->autoreq_reg = USB_CTRL_AUTOREQ;
 		controller->set_dma_mode = cppi41_set_dma_mode;
+		controller->num_channels = MUSB_DMA_NUM_CHANNELS;
 	}
 
+	channel_size = controller->num_channels *
+			sizeof(struct cppi41_dma_channel);
+	controller->rx_channel = kzalloc(channel_size, GFP_KERNEL);
+	if (controller->rx_channel)
+		goto rx_channel_alloc_fail;
+	controller->tx_channel = kzalloc(channel_size, GFP_KERNEL);
+	if (controller->tx_channel)
+		goto tx_channel_alloc_fail;
+
 	ret = cppi41_dma_controller_start(controller);
 	if (ret)
 		goto plat_get_fail;
 	return &controller->controller;
 
 plat_get_fail:
+	kfree(controller->tx_channel);
+tx_channel_alloc_fail:
+	kfree(controller->rx_channel);
+rx_channel_alloc_fail:
 	kfree(controller);
 kzalloc_fail:
 	if (ret == -EPROBE_DEFER)

Signature · 6 lines

2.13.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170920073513.11650-4-abailon-rdvid1DuHRBWk0Htik3J/[email protected]

Sergei Shtylyov

2017-09-20 09:21 UTC #6 lore raw

Re: [PATCH v2 3/4] usb: musb: musb_cppi41: Configure the number of channels for DA8xx

Hello!

Quoted 13 lines · Alexandre Bailon

Currently, the number of channels is set to 15 but in the case of DA8xx, the number of channels is 4.
Update the driver to configure the number of channels at runtime.
Signed-off-by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/[email protected]>
---
  drivers/usb/musb/musb_cppi41.c | 31 ++++++++++++++++++++++++++-----
  1 file changed, 26 insertions(+), 5 deletions(-)
drivers/usb/musb/musb_cppi41.c +0 -0
index b2b1306c01cf..2d1468cccae0 100644

[...]

Quoted 6 lines

drivers/usb/musb/musb_cppi41.c +1 -0

@@ -770,18 +776,33 @@ cppi41_dma_controller_create(struct musb *musb, void __iomem *base)
  		controller->tdown_reg = DA8XX_USB_TEARDOWN;
  		controller->autoreq_reg = DA8XX_USB_AUTOREQ;
  		controller->set_dma_mode = da8xx_set_dma_mode;
+		controller->num_channels = DA8XX_DMA_NUM_CHANNELS;
  	} else {

drivers/usb/musb/musb_cppi41.c cppi41_dma_controller_create(struct musb *musb, void __iomem *base) · line 790

Quoted 10 lines

drivers/usb/musb/musb_cppi41.c +5 -0

  		controller->tdown_reg = USB_TDOWN;
  		controller->autoreq_reg = USB_CTRL_AUTOREQ;
  		controller->set_dma_mode = cppi41_set_dma_mode;
+		controller->num_channels = MUSB_DMA_NUM_CHANNELS;
  	}
 
+	channel_size = controller->num_channels *
+			sizeof(struct cppi41_dma_channel);
+	controller->rx_channel = kzalloc(channel_size, GFP_KERNEL);
+	if (controller->rx_channel)

How can this work? The NULL check is reversed.

drivers/usb/musb/musb_cppi41.c cppi41_dma_controller_create(struct musb *musb, void __iomem *base) · line 793

Quoted 3 lines

drivers/usb/musb/musb_cppi41.c +3 -0

+		goto rx_channel_alloc_fail;
+	controller->tx_channel = kzalloc(channel_size, GFP_KERNEL);
+	if (controller->tx_channel)

Same here.

Quoted 6 lines

drivers/usb/musb/musb_cppi41.c +2 -0

+		goto tx_channel_alloc_fail;
+
  	ret = cppi41_dma_controller_start(controller);
  	if (ret)
  		goto plat_get_fail;

drivers/usb/musb/musb_cppi41.c cppi41_dma_controller_create(struct musb *musb, void __iomem *base) · line 808

Quoted 10 lines

drivers/usb/musb/musb_cppi41.c +4 -0

  	return &controller->controller;
 
  plat_get_fail:
+	kfree(controller->tx_channel);
+tx_channel_alloc_fail:
+	kfree(controller->rx_channel);
+rx_channel_alloc_fail:
  	kfree(controller);
  kzalloc_fail:
  	if (ret == -EPROBE_DEFER)

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 294030ff-73a3-62ed-2cbf-69db659f616e-M4DtvfQ/[email protected]

Alexandre Bailon

author

2017-09-20 17:35 UTC #8 lore raw

Re: [PATCH v2 3/4] usb: musb: musb_cppi41: Configure the number of channels for DA8xx

Quoted 29 lines · Sergei Shtylyov

Hello!
Quoted 14 lines · Alexandre Bailon
Currently, the number of channels is set to 15 but in the case of DA8xx, the number of channels is 4.
Update the driver to configure the number of channels at runtime.
Signed-off-by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/[email protected]>
---
  drivers/usb/musb/musb_cppi41.c | 31 ++++++++++++++++++++++++++-----
  1 file changed, 26 insertions(+), 5 deletions(-)
drivers/usb/musb/musb_cppi41.c +0 -0
index b2b1306c01cf..2d1468cccae0 100644
[...]
Quoted 10 lines
drivers/usb/musb/musb_cppi41.c +1 -0
@@ -770,18 +776,33 @@ cppi41_dma_controller_create(struct musb *musb,
 void __iomem *base)
          controller->tdown_reg = DA8XX_USB_TEARDOWN;
          controller->autoreq_reg = DA8XX_USB_AUTOREQ;
          controller->set_dma_mode = da8xx_set_dma_mode;
+        controller->num_channels = DA8XX_DMA_NUM_CHANNELS;
      } else {
          controller->tdown_reg = USB_TDOWN;
          controller->autoreq_reg = USB_CTRL_AUTOREQ;
          controller->set_dma_mode = cppi41_set_dma_mode;

Quoted 8 lines

drivers/usb/musb/musb_cppi41.c cppi41_dma_controller_create(struct musb *musb, void __iomem *base) · line 790
Quoted 6 lines
drivers/usb/musb/musb_cppi41.c +4 -0
+        controller->num_channels = MUSB_DMA_NUM_CHANNELS;
      }
  +    channel_size = controller->num_channels *
+            sizeof(struct cppi41_dma_channel);
+    controller->rx_channel = kzalloc(channel_size, GFP_KERNEL);
+    if (controller->rx_channel)
How can this work? The NULL check is reversed.

Indeed! That's a good question! I have tested the series before to send it but I could not explain why it have worked.
Anyway, I will fix it.

Best Regards,
Alexandre
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexandre Bailon

author

2017-09-20 07:35 UTC #9 lore raw

PATCH v2 4/4 usb: musb: da8xx: Remove duplicated defines

In reply to a message not in the archive

There is some registers defined in da8xx.c though they are not used. These registers are also defined and used in musb_cppi41.c Remove these defines from da8xx.c.

Signed-off-by Alexandre Bailon <abailon-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/usb/musb/da8xx.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/usb/musb/da8xx.c +0 -3

index df88123274ca..2ec2039eee86 100644
@@ -50,10 +50,7 @@
 #define DA8XX_USB_CTRL_REG	0x04
 #define DA8XX_USB_STAT_REG	0x08
 #define DA8XX_USB_EMULATION_REG 0x0c
-#define DA8XX_USB_MODE_REG	0x10	/* Transparent, CDC, [Generic] RNDIS */
-#define DA8XX_USB_AUTOREQ_REG	0x14
 #define DA8XX_USB_SRP_FIX_TIME_REG 0x18
-#define DA8XX_USB_TEARDOWN_REG	0x1c
 #define DA8XX_USB_INTR_SRC_REG	0x20
 #define DA8XX_USB_INTR_SRC_SET_REG 0x24
 #define DA8XX_USB_INTR_SRC_CLEAR_REG 0x28

Signature · 6 lines

2.13.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html