[U-Boot] [PATCH v3 1/4] arm: add missing writes{bwql}, reads{bwql}.

[U-Boot] [PATCH v3 1/4] arm: add missing writes{bwql}, reads{bwql}.

[Purna Chandra Mandal]

ARM arch defines __raw_writes[bwql], __raw_reads[bwql] in io.h
but not writes[bwql], reads[bwql] as required by some drivers.
Some of the drivers are defining writes{bwlq} or reads{bwlq} as
wrapper of their "__raw" version.
To avoid that lets add the wrapper in arch itself.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
---

Changes in v3: None
Changes in v2: None

 arch/arm/include/asm/io.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 75773bd..9d185a6 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -284,6 +284,13 @@ static inline void __raw_readsl(unsigned long addr, void *data, int longlen)
 #define insw_p(port,to,len)		insw(port,to,len)
 #define insl_p(port,to,len)		insl(port,to,len)
 
+#define writesl(a, d, s)	__raw_writesl((unsigned long)a, d, s)
+#define readsl(a, d, s)		__raw_readsl((unsigned long)a, d, s)
+#define writesw(a, d, s)	__raw_writesw((unsigned long)a, d, s)
+#define readsw(a, d, s)		__raw_readsw((unsigned long)a, d, s)
+#define writesb(a, d, s)	__raw_writesb((unsigned long)a, d, s)
+#define readsb(a, d, s)		__raw_readsb((unsigned long)a, d, s)
+
 /*
  * ioremap and friends.
  *
-- 
1.8.3.1

[U-Boot] [PATCH v3 2/4] drivers: musb-new: remove writes{bwlq} and reads{bwlq}.

[Purna Chandra Mandal]

Moved definition of writes{bwlq} and reads{bwlq} into arch.
There is no need of having arch specific wrapper in driver.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
---

Changes in v3: None
Changes in v2: None

 drivers/usb/musb-new/linux-compat.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/musb-new/linux-compat.h b/drivers/usb/musb-new/linux-compat.h
index 46f83d9..526f4f2 100644
--- a/drivers/usb/musb-new/linux-compat.h
+++ b/drivers/usb/musb-new/linux-compat.h
@@ -13,13 +13,6 @@
 		printf(fmt, ##args);		\
 	ret_warn; })
 
-#define writesl(a, d, s) __raw_writesl((unsigned long)a, d, s)
-#define readsl(a, d, s) __raw_readsl((unsigned long)a, d, s)
-#define writesw(a, d, s) __raw_writesw((unsigned long)a, d, s)
-#define readsw(a, d, s) __raw_readsw((unsigned long)a, d, s)
-#define writesb(a, d, s) __raw_writesb((unsigned long)a, d, s)
-#define readsb(a, d, s) __raw_readsb((unsigned long)a, d, s)
-
 #define device_init_wakeup(dev, a) do {} while (0)
 
 #define platform_data device_data
-- 
1.8.3.1

[U-Boot] [PATCH v3 2/4] drivers: musb-new: remove writes{bwlq} and reads{bwlq}.

[Marek Vasut]

On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
> Moved definition of writes{bwlq} and reads{bwlq} into arch.
> There is no need of having arch specific wrapper in driver.

And so the patch does ... what exactly ? I cannot figure it out just by
reading the commit message, sorry.

The patch itself is fine of course, but please fix the commit message.

> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/usb/musb-new/linux-compat.h | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/linux-compat.h b/drivers/usb/musb-new/linux-compat.h
> index 46f83d9..526f4f2 100644
> --- a/drivers/usb/musb-new/linux-compat.h
> +++ b/drivers/usb/musb-new/linux-compat.h
> @@ -13,13 +13,6 @@
>  		printf(fmt, ##args);		\
>  	ret_warn; })
>  
> -#define writesl(a, d, s) __raw_writesl((unsigned long)a, d, s)
> -#define readsl(a, d, s) __raw_readsl((unsigned long)a, d, s)
> -#define writesw(a, d, s) __raw_writesw((unsigned long)a, d, s)
> -#define readsw(a, d, s) __raw_readsw((unsigned long)a, d, s)
> -#define writesb(a, d, s) __raw_writesb((unsigned long)a, d, s)
> -#define readsb(a, d, s) __raw_readsb((unsigned long)a, d, s)
> -
>  #define device_init_wakeup(dev, a) do {} while (0)
>  
>  #define platform_data device_data
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 2/4] drivers: musb-new: remove writes{bwlq} and reads{bwlq}.

[Purna Chandra Mandal]

On 03/15/2016 11:40 PM, Marek Vasut wrote:

> On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
>> Moved definition of writes{bwlq} and reads{bwlq} into arch.
>> There is no need of having arch specific wrapper in driver.
> And so the patch does ... what exactly ? I cannot figure it out just by
> reading the commit message, sorry.
>
> The patch itself is fine of course, but please fix the commit message.

I hope new commit message will do.
"Definition of writes{bwlq}, reads{bwlq} are now added into arch specific asm/io.h.
 So removing them from driver to fix re-definition error.
"

>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  drivers/usb/musb-new/linux-compat.h | 7 -------
>>  1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/usb/musb-new/linux-compat.h b/drivers/usb/musb-new/linux-compat.h
>> index 46f83d9..526f4f2 100644
>> --- a/drivers/usb/musb-new/linux-compat.h
>> +++ b/drivers/usb/musb-new/linux-compat.h
>> @@ -13,13 +13,6 @@
>>  		printf(fmt, ##args);		\
>>  	ret_warn; })
>>  
>> -#define writesl(a, d, s) __raw_writesl((unsigned long)a, d, s)
>> -#define readsl(a, d, s) __raw_readsl((unsigned long)a, d, s)
>> -#define writesw(a, d, s) __raw_writesw((unsigned long)a, d, s)
>> -#define readsw(a, d, s) __raw_readsw((unsigned long)a, d, s)
>> -#define writesb(a, d, s) __raw_writesb((unsigned long)a, d, s)
>> -#define readsb(a, d, s) __raw_readsb((unsigned long)a, d, s)
>> -
>>  #define device_init_wakeup(dev, a) do {} while (0)
>>  
>>  #define platform_data device_data
>>
>

[U-Boot] [PATCH v3 2/4] drivers: musb-new: remove writes{bwlq} and reads{bwlq}.

[Marek Vasut]

On 03/16/2016 07:36 AM, Purna Chandra Mandal wrote:
> On 03/15/2016 11:40 PM, Marek Vasut wrote:
> 
>> On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
>>> Moved definition of writes{bwlq} and reads{bwlq} into arch.
>>> There is no need of having arch specific wrapper in driver.
>> And so the patch does ... what exactly ? I cannot figure it out just by
>> reading the commit message, sorry.
>>
>> The patch itself is fine of course, but please fix the commit message.
> 
> I hope new commit message will do.
> "Definition of writes{bwlq}, reads{bwlq} are now added into arch specific asm/io.h.
>  So removing them from driver to fix re-definition error.
> "

Yes please :)

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 3/4] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.

[Purna Chandra Mandal]

This driver adds support of PIC32 MUSB OTG controller as dual role device.
It implements platform specific glue to reuse musb core.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
---

Changes in v3: None
Changes in v2: None

 drivers/usb/musb-new/Kconfig     |   7 +
 drivers/usb/musb-new/Makefile    |   1 +
 drivers/usb/musb-new/musb_core.c |   2 +-
 drivers/usb/musb-new/pic32.c     | 294 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 303 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/musb-new/pic32.c

diff --git a/drivers/usb/musb-new/Kconfig b/drivers/usb/musb-new/Kconfig
index 6a6cb93..4e8a543 100644
--- a/drivers/usb/musb-new/Kconfig
+++ b/drivers/usb/musb-new/Kconfig
@@ -15,6 +15,13 @@ config USB_MUSB_GADGET
 
 if USB_MUSB_HOST || USB_MUSB_GADGET
 
+config USB_MUSB_PIC32
+	bool "Enable Microchip PIC32 DRC USB controller"
+	depends on DM_USB && MACH_PIC32
+	help
+	  Say y to enable PIC32 USB DRC controller support
+	  if it is available on your Microchip PIC32 platform.
+
 config USB_MUSB_SUNXI
 	bool "Enable sunxi OTG / DRC USB controller"
 	depends on ARCH_SUNXI
diff --git a/drivers/usb/musb-new/Makefile b/drivers/usb/musb-new/Makefile
index 072d516..df1c3c8 100644
--- a/drivers/usb/musb-new/Makefile
+++ b/drivers/usb/musb-new/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_USB_MUSB_HOST) += musb_host.o musb_core.o musb_uboot.o
 obj-$(CONFIG_USB_MUSB_DSPS) += musb_dsps.o
 obj-$(CONFIG_USB_MUSB_AM35X) += am35x.o
 obj-$(CONFIG_USB_MUSB_OMAP2PLUS) += omap2430.o
+obj-$(CONFIG_USB_MUSB_PIC32) += pic32.o
 obj-$(CONFIG_USB_MUSB_SUNXI) += sunxi.o
 
 ccflags-y := $(call cc-option,-Wno-unused-variable) \
diff --git a/drivers/usb/musb-new/musb_core.c b/drivers/usb/musb-new/musb_core.c
index a6d6af6..dd0443c 100644
--- a/drivers/usb/musb-new/musb_core.c
+++ b/drivers/usb/musb-new/musb_core.c
@@ -259,7 +259,7 @@ void musb_write_fifo(struct musb_hw_ep *hw_ep, u16 len, const u8 *src)
 	}
 }
 
-#if !defined(CONFIG_USB_MUSB_AM35X)
+#if !defined(CONFIG_USB_MUSB_AM35X) && !defined(CONFIG_USB_MUSB_PIC32)
 /*
  * Unload an endpoint's FIFO
  */
diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c
new file mode 100644
index 0000000..980a971
--- /dev/null
+++ b/drivers/usb/musb-new/pic32.c
@@ -0,0 +1,294 @@
+/*
+ * Microchip PIC32 MUSB "glue layer"
+ *
+ * Copyright (C) 2015, Microchip Technology Inc.
+ *  Cristian Birsan <cristian.birsan@microchip.com>
+ *  Purna Chandra Mandal <purna.mandal@microchip.com>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * Based on the dsps "glue layer" code.
+ */
+
+#include <common.h>
+#include <linux/usb/musb.h>
+#include "linux-compat.h"
+#include "musb_core.h"
+#include "musb_uboot.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define PIC32_TX_EP_MASK	0x0f		/* EP0 + 7 Tx EPs */
+#define PIC32_RX_EP_MASK	0x0e		/* 7 Rx EPs */
+
+#define MUSB_SOFTRST		0x7f
+#define  MUSB_SOFTRST_NRST	BIT(0)
+#define  MUSB_SOFTRST_NRSTX	BIT(1)
+
+#define USBCRCON		0
+#define  USBCRCON_USBWKUPEN	BIT(0)  /* Enable Wakeup Interrupt */
+#define  USBCRCON_USBRIE	BIT(1)  /* Enable Remote resume Interrupt */
+#define  USBCRCON_USBIE		BIT(2)  /* Enable USB General interrupt */
+#define  USBCRCON_SENDMONEN	BIT(3)  /* Enable Session End VBUS monitoring */
+#define  USBCRCON_BSVALMONEN	BIT(4)  /* Enable B-Device VBUS monitoring */
+#define  USBCRCON_ASVALMONEN	BIT(5)  /* Enable A-Device VBUS monitoring */
+#define  USBCRCON_VBUSMONEN	BIT(6)  /* Enable VBUS monitoring */
+#define  USBCRCON_PHYIDEN	BIT(7)  /* PHY ID monitoring enable */
+#define  USBCRCON_USBIDVAL	BIT(8)  /* USB ID value */
+#define  USBCRCON_USBIDOVEN	BIT(9)  /* USB ID override enable */
+#define  USBCRCON_USBWK		BIT(24) /* USB Wakeup Status */
+#define  USBCRCON_USBRF		BIT(25) /* USB Resume Status */
+#define  USBCRCON_USBIF		BIT(26) /* USB General Interrupt Status */
+
+static void __iomem *musb_glue;
+
+/* pic32_musb_disable - disable HDRC */
+static void pic32_musb_disable(struct musb *musb)
+{
+}
+
+/* pic32_musb_enable - enable HDRC */
+static int pic32_musb_enable(struct musb *musb)
+{
+	/* soft reset by NRSTx */
+	musb_writeb(musb->mregs, MUSB_SOFTRST, MUSB_SOFTRST_NRSTX);
+	/* set mode */
+	musb_platform_set_mode(musb, musb->board_mode);
+
+	return 0;
+}
+
+static irqreturn_t pic32_interrupt(int irq, void *hci)
+{
+	struct musb  *musb = hci;
+	irqreturn_t ret = IRQ_NONE;
+	u32 epintr, usbintr;
+
+	/* Get usb core interrupts */
+	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
+	if (musb->int_usb)
+		musb_writeb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
+
+	/* Get endpoint interrupts */
+	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX) & PIC32_RX_EP_MASK;
+	if (musb->int_rx)
+		musb_writew(musb->mregs, MUSB_INTRRX, musb->int_rx);
+
+	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX) & PIC32_TX_EP_MASK;
+	if (musb->int_tx)
+		musb_writew(musb->mregs, MUSB_INTRTX, musb->int_tx);
+
+	/* Drop spurious RX and TX if device is disconnected */
+	if (musb->int_usb & MUSB_INTR_DISCONNECT) {
+		musb->int_tx = 0;
+		musb->int_rx = 0;
+	}
+
+	if (musb->int_tx || musb->int_rx || musb->int_usb)
+		ret |= musb_interrupt(musb);
+
+	return ret;
+}
+
+static int pic32_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct device *dev = musb->controller;
+
+	switch (mode) {
+	case MUSB_HOST:
+		clrsetbits_le32(musb_glue + USBCRCON,
+				USBCRCON_USBIDVAL, USBCRCON_USBIDOVEN);
+		break;
+	case MUSB_PERIPHERAL:
+		setbits_le32(musb_glue + USBCRCON,
+			     USBCRCON_USBIDVAL | USBCRCON_USBIDOVEN);
+		break;
+	case MUSB_OTG:
+		dev_err(dev, "MUSB OTG mode enabled\n");
+		break;
+	default:
+		dev_err(dev, "unsupported mode %d\n", mode);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int pic32_musb_init(struct musb *musb)
+{
+	u32 ctrl, hwvers;
+	u8 power;
+
+	/* Returns zero if not clocked */
+	hwvers = musb_read_hwvers(musb->mregs);
+	if (!hwvers)
+		return -ENODEV;
+
+	/* Reset the musb */
+	power = musb_readb(musb->mregs, MUSB_POWER);
+	power = power | MUSB_POWER_RESET;
+	musb_writeb(musb->mregs, MUSB_POWER, power);
+	mdelay(100);
+
+	/* Start the on-chip PHY and its PLL. */
+	power = power & ~MUSB_POWER_RESET;
+	musb_writeb(musb->mregs, MUSB_POWER, power);
+
+	musb->isr = pic32_interrupt;
+
+	ctrl =  USBCRCON_USBIF | USBCRCON_USBRF |
+		USBCRCON_USBWK | USBCRCON_USBIDOVEN |
+		USBCRCON_PHYIDEN | USBCRCON_USBIE |
+		USBCRCON_USBRIE | USBCRCON_USBWKUPEN |
+		USBCRCON_VBUSMONEN;
+	writel(ctrl, musb_glue + USBCRCON);
+
+	return 0;
+}
+
+/* PIC32 supports only 32bit read operation */
+void musb_read_fifo(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
+{
+	void __iomem *fifo = hw_ep->fifo;
+	u32 val;
+	int i;
+
+	/* Read for 32bit-aligned destination address */
+	if (likely((0x03 & (unsigned long)dst) == 0) && len >= 4) {
+		readsl(fifo, dst, len / 4);
+		dst += len & ~0x03;
+		len &= 0x03;
+	}
+
+	/*
+	 * Now read the remaining 1 to 3 byte or complete length if
+	 * unaligned address.
+	 */
+	if (len > 4) {
+		for (i = 0; i < (len / 4); i++) {
+			*(u32 *)dst = musb_readl(fifo, 0);
+			dst += 4;
+		}
+		len &= 0x03;
+	}
+
+	if (len > 0) {
+		val = musb_readl(fifo, 0);
+		memcpy(dst, &val, len);
+	}
+}
+
+const struct musb_platform_ops pic32_musb_ops = {
+	.init		= pic32_musb_init,
+	.set_mode	= pic32_musb_set_mode,
+	.disable	= pic32_musb_disable,
+	.enable		= pic32_musb_enable,
+};
+
+/* PIC32 default FIFO config - fits in 8KB */
+static struct musb_fifo_cfg pic32_musb_fifo_config[] = {
+	{ .hw_ep_num = 1, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 1, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 2, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 2, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 3, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 3, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 4, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 4, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 5, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 5, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 6, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 6, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 7, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 7, .style = FIFO_RX, .maxpacket = 512, },
+};
+
+static struct musb_hdrc_config pic32_musb_config = {
+	.fifo_cfg	= pic32_musb_fifo_config,
+	.fifo_cfg_size	= ARRAY_SIZE(pic32_musb_fifo_config),
+	.multipoint     = 1,
+	.dyn_fifo       = 1,
+	.num_eps        = 8,
+	.ram_bits       = 11,
+};
+
+/* PIC32 has one MUSB controller which can be host or gadget */
+static struct musb_hdrc_platform_data pic32_musb_plat = {
+	.mode           = MUSB_HOST,
+	.config         = &pic32_musb_config,
+	.power          = 250,		/* 500mA */
+	.platform_ops	= &pic32_musb_ops,
+};
+
+static int musb_usb_probe(struct udevice *dev)
+{
+	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
+	struct musb_host_data *mdata = dev_get_priv(dev);
+	struct fdt_resource mc, glue;
+	void *fdt = (void *)gd->fdt_blob;
+	int node = dev->of_offset;
+	void __iomem *mregs;
+	int ret;
+
+	priv->desc_before_addr = true;
+
+	ret = fdt_get_named_resource(fdt, node, "reg", "reg-names",
+				     "mc", &mc);
+	if (ret < 0) {
+		printf("pic32-musb: resource \"mc\" not found\n");
+		return ret;
+	}
+
+	ret = fdt_get_named_resource(fdt, node, "reg", "reg-names",
+				     "control", &glue);
+	if (ret < 0) {
+		printf("pic32-musb: resource \"control\" not found\n");
+		return ret;
+	}
+
+	mregs = ioremap(mc.start, fdt_resource_size(&mc));
+	musb_glue = ioremap(glue.start, fdt_resource_size(&glue));
+
+#ifdef CONFIG_USB_MUSB_HOST
+	/* init controller */
+	mdata->host = musb_init_controller(&pic32_musb_plat, NULL, mregs);
+	if (!mdata->host)
+		return -EIO;
+
+	ret = musb_lowlevel_init(mdata);
+#else
+	pic32_musb_plat.mode = MUSB_PERIPHERAL;
+	ret = musb_register(&pic32_musb_plat, NULL, mregs);
+#endif
+	if (ret == 0)
+		printf("PIC32 MUSB OTG\n");
+
+	return ret;
+}
+
+static int musb_usb_remove(struct udevice *dev)
+{
+	struct musb_host_data *mdata = dev_get_priv(dev);
+
+	musb_stop(mdata->host);
+
+	return 0;
+}
+
+static const struct udevice_id pic32_musb_ids[] = {
+	{ .compatible = "microchip,pic32mzda-usb" },
+	{ }
+};
+
+U_BOOT_DRIVER(usb_musb) = {
+	.name		= "pic32-musb",
+	.id		= UCLASS_USB,
+	.of_match	= pic32_musb_ids,
+	.probe		= musb_usb_probe,
+	.remove		= musb_usb_remove,
+#ifdef CONFIG_USB_MUSB_HOST
+	.ops		= &musb_usb_ops,
+#endif
+	.platdata_auto_alloc_size = sizeof(struct usb_platdata),
+	.priv_auto_alloc_size = sizeof(struct musb_host_data),
+};
-- 
1.8.3.1

[U-Boot] [PATCH v3 3/4] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.

[Marek Vasut]

On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
> This driver adds support of PIC32 MUSB OTG controller as dual role device.
> It implements platform specific glue to reuse musb core.
> 
> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>

[...]

> diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c
> new file mode 100644
> index 0000000..980a971
> --- /dev/null
> +++ b/drivers/usb/musb-new/pic32.c
> @@ -0,0 +1,294 @@
> +/*
> + * Microchip PIC32 MUSB "glue layer"
> + *
> + * Copyright (C) 2015, Microchip Technology Inc.
> + *  Cristian Birsan <cristian.birsan@microchip.com>
> + *  Purna Chandra Mandal <purna.mandal@microchip.com>
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + *
> + * Based on the dsps "glue layer" code.
> + */
> +
> +#include <common.h>
> +#include <linux/usb/musb.h>
> +#include "linux-compat.h"
> +#include "musb_core.h"
> +#include "musb_uboot.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define PIC32_TX_EP_MASK	0x0f		/* EP0 + 7 Tx EPs */
> +#define PIC32_RX_EP_MASK	0x0e		/* 7 Rx EPs */
> +
> +#define MUSB_SOFTRST		0x7f
> +#define  MUSB_SOFTRST_NRST	BIT(0)
> +#define  MUSB_SOFTRST_NRSTX	BIT(1)
> +
> +#define USBCRCON		0
> +#define  USBCRCON_USBWKUPEN	BIT(0)  /* Enable Wakeup Interrupt */
> +#define  USBCRCON_USBRIE	BIT(1)  /* Enable Remote resume Interrupt */
> +#define  USBCRCON_USBIE		BIT(2)  /* Enable USB General interrupt */
> +#define  USBCRCON_SENDMONEN	BIT(3)  /* Enable Session End VBUS monitoring */
> +#define  USBCRCON_BSVALMONEN	BIT(4)  /* Enable B-Device VBUS monitoring */
> +#define  USBCRCON_ASVALMONEN	BIT(5)  /* Enable A-Device VBUS monitoring */
> +#define  USBCRCON_VBUSMONEN	BIT(6)  /* Enable VBUS monitoring */
> +#define  USBCRCON_PHYIDEN	BIT(7)  /* PHY ID monitoring enable */
> +#define  USBCRCON_USBIDVAL	BIT(8)  /* USB ID value */
> +#define  USBCRCON_USBIDOVEN	BIT(9)  /* USB ID override enable */
> +#define  USBCRCON_USBWK		BIT(24) /* USB Wakeup Status */
> +#define  USBCRCON_USBRF		BIT(25) /* USB Resume Status */
> +#define  USBCRCON_USBIF		BIT(26) /* USB General Interrupt Status */
> +
> +static void __iomem *musb_glue;

What would happen once you make a chip with two MUSB controllers ?

> +/* pic32_musb_disable - disable HDRC */
> +static void pic32_musb_disable(struct musb *musb)
> +{

Is there no way to shut down the MUSB on the PIC32 ?

> +}
> +
> +/* pic32_musb_enable - enable HDRC */
> +static int pic32_musb_enable(struct musb *musb)
> +{
> +	/* soft reset by NRSTx */
> +	musb_writeb(musb->mregs, MUSB_SOFTRST, MUSB_SOFTRST_NRSTX);
> +	/* set mode */
> +	musb_platform_set_mode(musb, musb->board_mode);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t pic32_interrupt(int irq, void *hci)
> +{
> +	struct musb  *musb = hci;
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 epintr, usbintr;
> +
> +	/* Get usb core interrupts */

You mean "get" or "ack" here ?

> +	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
> +	if (musb->int_usb)
> +		musb_writeb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
> +
> +	/* Get endpoint interrupts */

DTTO

> +	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX) & PIC32_RX_EP_MASK;
> +	if (musb->int_rx)
> +		musb_writew(musb->mregs, MUSB_INTRRX, musb->int_rx);

Same here

> +	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX) & PIC32_TX_EP_MASK;
> +	if (musb->int_tx)
> +		musb_writew(musb->mregs, MUSB_INTRTX, musb->int_tx);
> +
> +	/* Drop spurious RX and TX if device is disconnected */
> +	if (musb->int_usb & MUSB_INTR_DISCONNECT) {
> +		musb->int_tx = 0;
> +		musb->int_rx = 0;
> +	}
> +
> +	if (musb->int_tx || musb->int_rx || musb->int_usb)
> +		ret |= musb_interrupt(musb);
> +
> +	return ret;
> +}
> +
> +static int pic32_musb_set_mode(struct musb *musb, u8 mode)
> +{
> +	struct device *dev = musb->controller;
> +
> +	switch (mode) {
> +	case MUSB_HOST:
> +		clrsetbits_le32(musb_glue + USBCRCON,
> +				USBCRCON_USBIDVAL, USBCRCON_USBIDOVEN);
> +		break;
> +	case MUSB_PERIPHERAL:
> +		setbits_le32(musb_glue + USBCRCON,
> +			     USBCRCON_USBIDVAL | USBCRCON_USBIDOVEN);
> +		break;
> +	case MUSB_OTG:
> +		dev_err(dev, "MUSB OTG mode enabled\n");

So having the core in OTG mode is an error ? Why ?

> +		break;
> +	default:
> +		dev_err(dev, "unsupported mode %d\n", mode);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pic32_musb_init(struct musb *musb)
> +{
> +	u32 ctrl, hwvers;
> +	u8 power;
> +
> +	/* Returns zero if not clocked */
> +	hwvers = musb_read_hwvers(musb->mregs);
> +	if (!hwvers)
> +		return -ENODEV;
> +
> +	/* Reset the musb */
> +	power = musb_readb(musb->mregs, MUSB_POWER);
> +	power = power | MUSB_POWER_RESET;
> +	musb_writeb(musb->mregs, MUSB_POWER, power);
> +	mdelay(100);
> +
> +	/* Start the on-chip PHY and its PLL. */
> +	power = power & ~MUSB_POWER_RESET;
> +	musb_writeb(musb->mregs, MUSB_POWER, power);
> +
> +	musb->isr = pic32_interrupt;
> +
> +	ctrl =  USBCRCON_USBIF | USBCRCON_USBRF |
> +		USBCRCON_USBWK | USBCRCON_USBIDOVEN |
> +		USBCRCON_PHYIDEN | USBCRCON_USBIE |
> +		USBCRCON_USBRIE | USBCRCON_USBWKUPEN |
> +		USBCRCON_VBUSMONEN;
> +	writel(ctrl, musb_glue + USBCRCON);
> +
> +	return 0;
> +}
> +
> +/* PIC32 supports only 32bit read operation */
> +void musb_read_fifo(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
> +{
> +	void __iomem *fifo = hw_ep->fifo;
> +	u32 val;
> +	int i;

This could become:
 bulk_len = len / 4;
 bulk_rem = len % 4;
 readsl(fifo, dst, bulk_len);
 if (rem) {
  dst += len & ~0x3;
  tmp = readl(fifo);
  copy the remaining bytes according to endianness
 }

This is 10 LoC , not 50 ;-)

> +	/* Read for 32bit-aligned destination address */
> +	if (likely((0x03 & (unsigned long)dst) == 0) && len >= 4) {
> +		readsl(fifo, dst, len / 4);
> +		dst += len & ~0x03;
> +		len &= 0x03;
> +	}
> +
> +	/*
> +	 * Now read the remaining 1 to 3 byte or complete length if
> +	 * unaligned address.
> +	 */
> +	if (len > 4) {
> +		for (i = 0; i < (len / 4); i++) {
> +			*(u32 *)dst = musb_readl(fifo, 0);
> +			dst += 4;
> +		}
> +		len &= 0x03;
> +	}
> +
> +	if (len > 0) {
> +		val = musb_readl(fifo, 0);
> +		memcpy(dst, &val, len);
> +	}
> +}

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 3/4] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.

[Purna Chandra Mandal]

On 03/15/2016 11:49 PM, Marek Vasut wrote:

> On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
>> This driver adds support of PIC32 MUSB OTG controller as dual role device.
>> It implements platform specific glue to reuse musb core.
>>
>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
> [...]
>
>> diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c
>> new file mode 100644
>> index 0000000..980a971
>> --- /dev/null
>> +++ b/drivers/usb/musb-new/pic32.c
>> @@ -0,0 +1,294 @@
>> +/*
>> + * Microchip PIC32 MUSB "glue layer"
>> + *
>> + * Copyright (C) 2015, Microchip Technology Inc.
>> + *  Cristian Birsan <cristian.birsan@microchip.com>
>> + *  Purna Chandra Mandal <purna.mandal@microchip.com>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + *
>> + * Based on the dsps "glue layer" code.
>> + */
>> +
>> +#include <common.h>
>> +#include <linux/usb/musb.h>
>> +#include "linux-compat.h"
>> +#include "musb_core.h"
>> +#include "musb_uboot.h"
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#define PIC32_TX_EP_MASK	0x0f		/* EP0 + 7 Tx EPs */
>> +#define PIC32_RX_EP_MASK	0x0e		/* 7 Rx EPs */
>> +
>> +#define MUSB_SOFTRST		0x7f
>> +#define  MUSB_SOFTRST_NRST	BIT(0)
>> +#define  MUSB_SOFTRST_NRSTX	BIT(1)
>> +
>> +#define USBCRCON		0
>> +#define  USBCRCON_USBWKUPEN	BIT(0)  /* Enable Wakeup Interrupt */
>> +#define  USBCRCON_USBRIE	BIT(1)  /* Enable Remote resume Interrupt */
>> +#define  USBCRCON_USBIE		BIT(2)  /* Enable USB General interrupt */
>> +#define  USBCRCON_SENDMONEN	BIT(3)  /* Enable Session End VBUS monitoring */
>> +#define  USBCRCON_BSVALMONEN	BIT(4)  /* Enable B-Device VBUS monitoring */
>> +#define  USBCRCON_ASVALMONEN	BIT(5)  /* Enable A-Device VBUS monitoring */
>> +#define  USBCRCON_VBUSMONEN	BIT(6)  /* Enable VBUS monitoring */
>> +#define  USBCRCON_PHYIDEN	BIT(7)  /* PHY ID monitoring enable */
>> +#define  USBCRCON_USBIDVAL	BIT(8)  /* USB ID value */
>> +#define  USBCRCON_USBIDOVEN	BIT(9)  /* USB ID override enable */
>> +#define  USBCRCON_USBWK		BIT(24) /* USB Wakeup Status */
>> +#define  USBCRCON_USBRF		BIT(25) /* USB Resume Status */
>> +#define  USBCRCON_USBIF		BIT(26) /* USB General Interrupt Status */
>> +
>> +static void __iomem *musb_glue;
> What would happen once you make a chip with two MUSB controllers ?

Currently PIC32 has only one MUSB controller and only one glue reg-space.
Don't know how the reg-map will be in future when PIC32 will have multiple
MUSB controllers. Assuming that glue address map will be separate for
each controller we can add logic to support multiple MUSB controller.

IMO, better if we don't assume something of the future and bloat logic.

>> +/* pic32_musb_disable - disable HDRC */
>> +static void pic32_musb_disable(struct musb *musb)
>> +{
> Is there no way to shut down the MUSB on the PIC32 ?

There is no way to disable MUSB.

>> +}
>> +
>> +/* pic32_musb_enable - enable HDRC */
>> +static int pic32_musb_enable(struct musb *musb)
>> +{
>> +	/* soft reset by NRSTx */
>> +	musb_writeb(musb->mregs, MUSB_SOFTRST, MUSB_SOFTRST_NRSTX);
>> +	/* set mode */
>> +	musb_platform_set_mode(musb, musb->board_mode);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t pic32_interrupt(int irq, void *hci)
>> +{
>> +	struct musb  *musb = hci;
>> +	irqreturn_t ret = IRQ_NONE;
>> +	u32 epintr, usbintr;
>> +
>> +	/* Get usb core interrupts */
> You mean "get" or "ack" here ?

I meant read-and-ack. Will update comment.

>> +	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
>> +	if (musb->int_usb)
>> +		musb_writeb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
>> +
>> +	/* Get endpoint interrupts */
> DTTO

I meant read-and-ack.

>> +	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX) & PIC32_RX_EP_MASK;
>> +	if (musb->int_rx)
>> +		musb_writew(musb->mregs, MUSB_INTRRX, musb->int_rx);
> Same here

ack. Will update comment.

>> +	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX) & PIC32_TX_EP_MASK;
>> +	if (musb->int_tx)
>> +		musb_writew(musb->mregs, MUSB_INTRTX, musb->int_tx);
>> +
>> +	/* Drop spurious RX and TX if device is disconnected */
>> +	if (musb->int_usb & MUSB_INTR_DISCONNECT) {
>> +		musb->int_tx = 0;
>> +		musb->int_rx = 0;
>> +	}
>> +
>> +	if (musb->int_tx || musb->int_rx || musb->int_usb)
>> +		ret |= musb_interrupt(musb);
>> +
>> +	return ret;
>> +}
>> +
>> +static int pic32_musb_set_mode(struct musb *musb, u8 mode)
>> +{
>> +	struct device *dev = musb->controller;
>> +
>> +	switch (mode) {
>> +	case MUSB_HOST:
>> +		clrsetbits_le32(musb_glue + USBCRCON,
>> +				USBCRCON_USBIDVAL, USBCRCON_USBIDOVEN);
>> +		break;
>> +	case MUSB_PERIPHERAL:
>> +		setbits_le32(musb_glue + USBCRCON,
>> +			     USBCRCON_USBIDVAL | USBCRCON_USBIDOVEN);
>> +		break;
>> +	case MUSB_OTG:
>> +		dev_err(dev, "MUSB OTG mode enabled\n");
> So having the core in OTG mode is an error ? Why ?

In PIC32 we haven't tested OTG. We use the controller as dual-role not as OTG.
In future we might enable that. 

>> +		break;
>> +	default:
>> +		dev_err(dev, "unsupported mode %d\n", mode);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int pic32_musb_init(struct musb *musb)
>> +{
>> +	u32 ctrl, hwvers;
>> +	u8 power;
>> +
>> +	/* Returns zero if not clocked */
>> +	hwvers = musb_read_hwvers(musb->mregs);
>> +	if (!hwvers)
>> +		return -ENODEV;
>> +
>> +	/* Reset the musb */
>> +	power = musb_readb(musb->mregs, MUSB_POWER);
>> +	power = power | MUSB_POWER_RESET;
>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>> +	mdelay(100);
>> +
>> +	/* Start the on-chip PHY and its PLL. */
>> +	power = power & ~MUSB_POWER_RESET;
>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>> +
>> +	musb->isr = pic32_interrupt;
>> +
>> +	ctrl =  USBCRCON_USBIF | USBCRCON_USBRF |
>> +		USBCRCON_USBWK | USBCRCON_USBIDOVEN |
>> +		USBCRCON_PHYIDEN | USBCRCON_USBIE |
>> +		USBCRCON_USBRIE | USBCRCON_USBWKUPEN |
>> +		USBCRCON_VBUSMONEN;
>> +	writel(ctrl, musb_glue + USBCRCON);
>> +
>> +	return 0;
>> +}
>> +
>> +/* PIC32 supports only 32bit read operation */
>> +void musb_read_fifo(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
>> +{
>> +	void __iomem *fifo = hw_ep->fifo;
>> +	u32 val;
>> +	int i;
> This could become:
>  bulk_len = len / 4;
>  bulk_rem = len % 4;
>  readsl(fifo, dst, bulk_len);
>  if (rem) {
>   dst += len & ~0x3;
>   tmp = readl(fifo);
>   copy the remaining bytes according to endianness
>  }
>
> This is 10 LoC , not 50 ;-)

May be comments in my code are not very clear :)

All the extra code is for handling (4-byte-word) alignment of destination buffer.
Writing word to unaligned address will generate exception in MIPS which is not handled
in U-Boot software.
Note MIPS can't handle unaligned access in h/w unless specific unaligned
instructions are used.

>> +	/* Read for 32bit-aligned destination address */
>> +	if (likely((0x03 & (unsigned long)dst) == 0) && len >= 4) {
>> +		readsl(fifo, dst, len / 4);
>> +		dst += len & ~0x03;
>> +		len &= 0x03;
>> +	}
>> +
>> +	/*
>> +	 * Now read the remaining 1 to 3 byte or complete length if
>> +	 * unaligned address.
>> +	 */
>> +	if (len > 4) {
>> +		for (i = 0; i < (len / 4); i++) {
>> +			*(u32 *)dst = musb_readl(fifo, 0);
>> +			dst += 4;
>> +		}
>> +		len &= 0x03;
>> +	}
>> +
>> +	if (len > 0) {
>> +		val = musb_readl(fifo, 0);
>> +		memcpy(dst, &val, len);
>> +	}
>> +}
> [...]
>

[U-Boot] [PATCH v3 3/4] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.

[Marek Vasut]

On 03/16/2016 10:58 AM, Purna Chandra Mandal wrote:
> On 03/15/2016 11:49 PM, Marek Vasut wrote:
> 
>> On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
>>> This driver adds support of PIC32 MUSB OTG controller as dual role device.
>>> It implements platform specific glue to reuse musb core.
>>>
>>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>> [...]
>>
>>> diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c
>>> new file mode 100644
>>> index 0000000..980a971
>>> --- /dev/null
>>> +++ b/drivers/usb/musb-new/pic32.c
>>> @@ -0,0 +1,294 @@
>>> +/*
>>> + * Microchip PIC32 MUSB "glue layer"
>>> + *
>>> + * Copyright (C) 2015, Microchip Technology Inc.
>>> + *  Cristian Birsan <cristian.birsan@microchip.com>
>>> + *  Purna Chandra Mandal <purna.mandal@microchip.com>
>>> + *
>>> + * SPDX-License-Identifier:     GPL-2.0+
>>> + *
>>> + * Based on the dsps "glue layer" code.
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <linux/usb/musb.h>
>>> +#include "linux-compat.h"
>>> +#include "musb_core.h"
>>> +#include "musb_uboot.h"
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +#define PIC32_TX_EP_MASK	0x0f		/* EP0 + 7 Tx EPs */
>>> +#define PIC32_RX_EP_MASK	0x0e		/* 7 Rx EPs */
>>> +
>>> +#define MUSB_SOFTRST		0x7f
>>> +#define  MUSB_SOFTRST_NRST	BIT(0)
>>> +#define  MUSB_SOFTRST_NRSTX	BIT(1)
>>> +
>>> +#define USBCRCON		0
>>> +#define  USBCRCON_USBWKUPEN	BIT(0)  /* Enable Wakeup Interrupt */
>>> +#define  USBCRCON_USBRIE	BIT(1)  /* Enable Remote resume Interrupt */
>>> +#define  USBCRCON_USBIE		BIT(2)  /* Enable USB General interrupt */
>>> +#define  USBCRCON_SENDMONEN	BIT(3)  /* Enable Session End VBUS monitoring */
>>> +#define  USBCRCON_BSVALMONEN	BIT(4)  /* Enable B-Device VBUS monitoring */
>>> +#define  USBCRCON_ASVALMONEN	BIT(5)  /* Enable A-Device VBUS monitoring */
>>> +#define  USBCRCON_VBUSMONEN	BIT(6)  /* Enable VBUS monitoring */
>>> +#define  USBCRCON_PHYIDEN	BIT(7)  /* PHY ID monitoring enable */
>>> +#define  USBCRCON_USBIDVAL	BIT(8)  /* USB ID value */
>>> +#define  USBCRCON_USBIDOVEN	BIT(9)  /* USB ID override enable */
>>> +#define  USBCRCON_USBWK		BIT(24) /* USB Wakeup Status */
>>> +#define  USBCRCON_USBRF		BIT(25) /* USB Resume Status */
>>> +#define  USBCRCON_USBIF		BIT(26) /* USB General Interrupt Status */
>>> +
>>> +static void __iomem *musb_glue;
>> What would happen once you make a chip with two MUSB controllers ?
> 
> Currently PIC32 has only one MUSB controller and only one glue reg-space.
> Don't know how the reg-map will be in future when PIC32 will have multiple
> MUSB controllers. Assuming that glue address map will be separate for
> each controller we can add logic to support multiple MUSB controller.
> 
> IMO, better if we don't assume something of the future and bloat logic.

If you switch this to driver model, you will need to weed out all the
static global variables anyway. Better do it now.

>>> +/* pic32_musb_disable - disable HDRC */
>>> +static void pic32_musb_disable(struct musb *musb)
>>> +{
>> Is there no way to shut down the MUSB on the PIC32 ?
> 
> There is no way to disable MUSB.

Yet another broken chip design. Can't you put the controller into reset
and gate the clock for it ?

>>> +}
>>> +
>>> +/* pic32_musb_enable - enable HDRC */
>>> +static int pic32_musb_enable(struct musb *musb)
>>> +{
>>> +	/* soft reset by NRSTx */
>>> +	musb_writeb(musb->mregs, MUSB_SOFTRST, MUSB_SOFTRST_NRSTX);
>>> +	/* set mode */
>>> +	musb_platform_set_mode(musb, musb->board_mode);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static irqreturn_t pic32_interrupt(int irq, void *hci)
>>> +{
>>> +	struct musb  *musb = hci;
>>> +	irqreturn_t ret = IRQ_NONE;
>>> +	u32 epintr, usbintr;
>>> +
>>> +	/* Get usb core interrupts */
>> You mean "get" or "ack" here ?
> 
> I meant read-and-ack. Will update comment.

Thanks

>>> +	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
>>> +	if (musb->int_usb)
>>> +		musb_writeb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
>>> +
>>> +	/* Get endpoint interrupts */
>> DTTO
> 
> I meant read-and-ack.
> 
>>> +	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX) & PIC32_RX_EP_MASK;
>>> +	if (musb->int_rx)
>>> +		musb_writew(musb->mregs, MUSB_INTRRX, musb->int_rx);
>> Same here
> 
> ack. Will update comment.
> 
>>> +	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX) & PIC32_TX_EP_MASK;
>>> +	if (musb->int_tx)
>>> +		musb_writew(musb->mregs, MUSB_INTRTX, musb->int_tx);
>>> +
>>> +	/* Drop spurious RX and TX if device is disconnected */
>>> +	if (musb->int_usb & MUSB_INTR_DISCONNECT) {
>>> +		musb->int_tx = 0;
>>> +		musb->int_rx = 0;
>>> +	}
>>> +
>>> +	if (musb->int_tx || musb->int_rx || musb->int_usb)
>>> +		ret |= musb_interrupt(musb);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int pic32_musb_set_mode(struct musb *musb, u8 mode)
>>> +{
>>> +	struct device *dev = musb->controller;
>>> +
>>> +	switch (mode) {
>>> +	case MUSB_HOST:
>>> +		clrsetbits_le32(musb_glue + USBCRCON,
>>> +				USBCRCON_USBIDVAL, USBCRCON_USBIDOVEN);
>>> +		break;
>>> +	case MUSB_PERIPHERAL:
>>> +		setbits_le32(musb_glue + USBCRCON,
>>> +			     USBCRCON_USBIDVAL | USBCRCON_USBIDOVEN);
>>> +		break;
>>> +	case MUSB_OTG:
>>> +		dev_err(dev, "MUSB OTG mode enabled\n");
>> So having the core in OTG mode is an error ? Why ?
> 
> In PIC32 we haven't tested OTG. We use the controller as dual-role not as OTG.
> In future we might enable that. 

Then you might want to say "support for OTG is unimplemented" ?

>>> +		break;
>>> +	default:
>>> +		dev_err(dev, "unsupported mode %d\n", mode);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int pic32_musb_init(struct musb *musb)
>>> +{
>>> +	u32 ctrl, hwvers;
>>> +	u8 power;
>>> +
>>> +	/* Returns zero if not clocked */
>>> +	hwvers = musb_read_hwvers(musb->mregs);
>>> +	if (!hwvers)
>>> +		return -ENODEV;
>>> +
>>> +	/* Reset the musb */
>>> +	power = musb_readb(musb->mregs, MUSB_POWER);
>>> +	power = power | MUSB_POWER_RESET;
>>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>>> +	mdelay(100);
>>> +
>>> +	/* Start the on-chip PHY and its PLL. */
>>> +	power = power & ~MUSB_POWER_RESET;
>>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>>> +
>>> +	musb->isr = pic32_interrupt;
>>> +
>>> +	ctrl =  USBCRCON_USBIF | USBCRCON_USBRF |
>>> +		USBCRCON_USBWK | USBCRCON_USBIDOVEN |
>>> +		USBCRCON_PHYIDEN | USBCRCON_USBIE |
>>> +		USBCRCON_USBRIE | USBCRCON_USBWKUPEN |
>>> +		USBCRCON_VBUSMONEN;
>>> +	writel(ctrl, musb_glue + USBCRCON);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/* PIC32 supports only 32bit read operation */
>>> +void musb_read_fifo(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
>>> +{
>>> +	void __iomem *fifo = hw_ep->fifo;
>>> +	u32 val;
>>> +	int i;
>> This could become:
>>  bulk_len = len / 4;
>>  bulk_rem = len % 4;
>>  readsl(fifo, dst, bulk_len);
>>  if (rem) {
>>   dst += len & ~0x3;
>>   tmp = readl(fifo);
>>   copy the remaining bytes according to endianness
>>  }
>>
>> This is 10 LoC , not 50 ;-)
> 
> May be comments in my code are not very clear :)
> 
> All the extra code is for handling (4-byte-word) alignment of destination buffer.
> Writing word to unaligned address will generate exception in MIPS which is not handled
> in U-Boot software.
> Note MIPS can't handle unaligned access in h/w unless specific unaligned
> instructions are used.

Ouch, did you ever see unaligned access here ?

>>> +	/* Read for 32bit-aligned destination address */
>>> +	if (likely((0x03 & (unsigned long)dst) == 0) && len >= 4) {
>>> +		readsl(fifo, dst, len / 4);
>>> +		dst += len & ~0x03;
>>> +		len &= 0x03;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Now read the remaining 1 to 3 byte or complete length if
>>> +	 * unaligned address.
>>> +	 */
>>> +	if (len > 4) {
>>> +		for (i = 0; i < (len / 4); i++) {
>>> +			*(u32 *)dst = musb_readl(fifo, 0);
>>> +			dst += 4;
>>> +		}
>>> +		len &= 0x03;
>>> +	}
>>> +
>>> +	if (len > 0) {
>>> +		val = musb_readl(fifo, 0);
>>> +		memcpy(dst, &val, len);
>>> +	}
>>> +}
>> [...]
>>
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 3/4] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.

[Purna Chandra Mandal]

On 03/16/2016 09:18 PM, Marek Vasut wrote:

> On 03/16/2016 10:58 AM, Purna Chandra Mandal wrote:
>> On 03/15/2016 11:49 PM, Marek Vasut wrote:
>>
>>> On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
>>>> This driver adds support of PIC32 MUSB OTG controller as dual role device.
>>>> It implements platform specific glue to reuse musb core.
>>>>
>>>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>>>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>>> [...]
>>>
>>>> diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c
>>>> new file mode 100644
>>>> index 0000000..980a971
>>>> --- /dev/null
>>>> +++ b/drivers/usb/musb-new/pic32.c
>>>> @@ -0,0 +1,294 @@
>>>> +/*
>>>> + * Microchip PIC32 MUSB "glue layer"
>>>> + *
>>>> + * Copyright (C) 2015, Microchip Technology Inc.
>>>> + *  Cristian Birsan <cristian.birsan@microchip.com>
>>>> + *  Purna Chandra Mandal <purna.mandal@microchip.com>
>>>> + *
>>>> + * SPDX-License-Identifier:     GPL-2.0+
>>>> + *
>>>> + * Based on the dsps "glue layer" code.
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <linux/usb/musb.h>
>>>> +#include "linux-compat.h"
>>>> +#include "musb_core.h"
>>>> +#include "musb_uboot.h"
>>>> +
>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>> +
>>>> +#define PIC32_TX_EP_MASK	0x0f		/* EP0 + 7 Tx EPs */
>>>> +#define PIC32_RX_EP_MASK	0x0e		/* 7 Rx EPs */
>>>> +
>>>> +#define MUSB_SOFTRST		0x7f
>>>> +#define  MUSB_SOFTRST_NRST	BIT(0)
>>>> +#define  MUSB_SOFTRST_NRSTX	BIT(1)
>>>> +
>>>> +#define USBCRCON		0
>>>> +#define  USBCRCON_USBWKUPEN	BIT(0)  /* Enable Wakeup Interrupt */
>>>> +#define  USBCRCON_USBRIE	BIT(1)  /* Enable Remote resume Interrupt */
>>>> +#define  USBCRCON_USBIE		BIT(2)  /* Enable USB General interrupt */
>>>> +#define  USBCRCON_SENDMONEN	BIT(3)  /* Enable Session End VBUS monitoring */
>>>> +#define  USBCRCON_BSVALMONEN	BIT(4)  /* Enable B-Device VBUS monitoring */
>>>> +#define  USBCRCON_ASVALMONEN	BIT(5)  /* Enable A-Device VBUS monitoring */
>>>> +#define  USBCRCON_VBUSMONEN	BIT(6)  /* Enable VBUS monitoring */
>>>> +#define  USBCRCON_PHYIDEN	BIT(7)  /* PHY ID monitoring enable */
>>>> +#define  USBCRCON_USBIDVAL	BIT(8)  /* USB ID value */
>>>> +#define  USBCRCON_USBIDOVEN	BIT(9)  /* USB ID override enable */
>>>> +#define  USBCRCON_USBWK		BIT(24) /* USB Wakeup Status */
>>>> +#define  USBCRCON_USBRF		BIT(25) /* USB Resume Status */
>>>> +#define  USBCRCON_USBIF		BIT(26) /* USB General Interrupt Status */
>>>> +
>>>> +static void __iomem *musb_glue;
>>> What would happen once you make a chip with two MUSB controllers ?
>> Currently PIC32 has only one MUSB controller and only one glue reg-space.
>> Don't know how the reg-map will be in future when PIC32 will have multiple
>> MUSB controllers. Assuming that glue address map will be separate for
>> each controller we can add logic to support multiple MUSB controller.
>>
>> IMO, better if we don't assume something of the future and bloat logic.
> If you switch this to driver model, you will need to weed out all the
> static global variables anyway. Better do it now.

Thanks. Will do.

>>>> +/* pic32_musb_disable - disable HDRC */
>>>> +static void pic32_musb_disable(struct musb *musb)
>>>> +{
>>> Is there no way to shut down the MUSB on the PIC32 ?
>> There is no way to disable MUSB.
> Yet another broken chip design. Can't you put the controller into reset
> and gate the clock for it ?

USB clock can't be gated! USB controller clock is derived from peripheral bus
clock(PBCLK5) which is shared with other modules and USB Phy clock derived from
UPLL can;t be gated at all.

>>>> +}
>>>> +
>>>> +/* pic32_musb_enable - enable HDRC */
>>>> +static int pic32_musb_enable(struct musb *musb)
>>>> +{
>>>> +	/* soft reset by NRSTx */
>>>> +	musb_writeb(musb->mregs, MUSB_SOFTRST, MUSB_SOFTRST_NRSTX);
>>>> +	/* set mode */
>>>> +	musb_platform_set_mode(musb, musb->board_mode);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t pic32_interrupt(int irq, void *hci)
>>>> +{
>>>> +	struct musb  *musb = hci;
>>>> +	irqreturn_t ret = IRQ_NONE;
>>>> +	u32 epintr, usbintr;
>>>> +
>>>> +	/* Get usb core interrupts */
>>> You mean "get" or "ack" here ?
>> I meant read-and-ack. Will update comment.
> Thanks
>
>>>> +	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
>>>> +	if (musb->int_usb)
>>>> +		musb_writeb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
>>>> +
>>>> +	/* Get endpoint interrupts */
>>> DTTO
>> I meant read-and-ack.
>>
>>>> +	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX) & PIC32_RX_EP_MASK;
>>>> +	if (musb->int_rx)
>>>> +		musb_writew(musb->mregs, MUSB_INTRRX, musb->int_rx);
>>> Same here
>> ack. Will update comment.
>>
>>>> +	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX) & PIC32_TX_EP_MASK;
>>>> +	if (musb->int_tx)
>>>> +		musb_writew(musb->mregs, MUSB_INTRTX, musb->int_tx);
>>>> +
>>>> +	/* Drop spurious RX and TX if device is disconnected */
>>>> +	if (musb->int_usb & MUSB_INTR_DISCONNECT) {
>>>> +		musb->int_tx = 0;
>>>> +		musb->int_rx = 0;
>>>> +	}
>>>> +
>>>> +	if (musb->int_tx || musb->int_rx || musb->int_usb)
>>>> +		ret |= musb_interrupt(musb);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int pic32_musb_set_mode(struct musb *musb, u8 mode)
>>>> +{
>>>> +	struct device *dev = musb->controller;
>>>> +
>>>> +	switch (mode) {
>>>> +	case MUSB_HOST:
>>>> +		clrsetbits_le32(musb_glue + USBCRCON,
>>>> +				USBCRCON_USBIDVAL, USBCRCON_USBIDOVEN);
>>>> +		break;
>>>> +	case MUSB_PERIPHERAL:
>>>> +		setbits_le32(musb_glue + USBCRCON,
>>>> +			     USBCRCON_USBIDVAL | USBCRCON_USBIDOVEN);
>>>> +		break;
>>>> +	case MUSB_OTG:
>>>> +		dev_err(dev, "MUSB OTG mode enabled\n");
>>> So having the core in OTG mode is an error ? Why ?
>> In PIC32 we haven't tested OTG. We use the controller as dual-role not as OTG.
>> In future we might enable that. 
> Then you might want to say "support for OTG is unimplemented" ?

Will update.

>>>> +		break;
>>>> +	default:
>>>> +		dev_err(dev, "unsupported mode %d\n", mode);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int pic32_musb_init(struct musb *musb)
>>>> +{
>>>> +	u32 ctrl, hwvers;
>>>> +	u8 power;
>>>> +
>>>> +	/* Returns zero if not clocked */
>>>> +	hwvers = musb_read_hwvers(musb->mregs);
>>>> +	if (!hwvers)
>>>> +		return -ENODEV;
>>>> +
>>>> +	/* Reset the musb */
>>>> +	power = musb_readb(musb->mregs, MUSB_POWER);
>>>> +	power = power | MUSB_POWER_RESET;
>>>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>>>> +	mdelay(100);
>>>> +
>>>> +	/* Start the on-chip PHY and its PLL. */
>>>> +	power = power & ~MUSB_POWER_RESET;
>>>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>>>> +
>>>> +	musb->isr = pic32_interrupt;
>>>> +
>>>> +	ctrl =  USBCRCON_USBIF | USBCRCON_USBRF |
>>>> +		USBCRCON_USBWK | USBCRCON_USBIDOVEN |
>>>> +		USBCRCON_PHYIDEN | USBCRCON_USBIE |
>>>> +		USBCRCON_USBRIE | USBCRCON_USBWKUPEN |
>>>> +		USBCRCON_VBUSMONEN;
>>>> +	writel(ctrl, musb_glue + USBCRCON);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/* PIC32 supports only 32bit read operation */
>>>> +void musb_read_fifo(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
>>>> +{
>>>> +	void __iomem *fifo = hw_ep->fifo;
>>>> +	u32 val;
>>>> +	int i;
>>> This could become:
>>>  bulk_len = len / 4;
>>>  bulk_rem = len % 4;
>>>  readsl(fifo, dst, bulk_len);
>>>  if (rem) {
>>>   dst += len & ~0x3;
>>>   tmp = readl(fifo);
>>>   copy the remaining bytes according to endianness
>>>  }
>>>
>>> This is 10 LoC , not 50 ;-)
>> May be comments in my code are not very clear :)
>>
>> All the extra code is for handling (4-byte-word) alignment of destination buffer.
>> Writing word to unaligned address will generate exception in MIPS which is not handled
>> in U-Boot software.
>> Note MIPS can't handle unaligned access in h/w unless specific unaligned
>> instructions are used.
> Ouch, did you ever see unaligned access here ?

Not seen unaligned access here but not sure of!
Char array can be starting from any arbitrary address unless it is
malloc()'d or special care is taken to make it aligned at word boundary.

I will take your logic as I'm not facing real problem.

>>>> +	/* Read for 32bit-aligned destination address */
>>>> +	if (likely((0x03 & (unsigned long)dst) == 0) && len >= 4) {
>>>> +		readsl(fifo, dst, len / 4);
>>>> +		dst += len & ~0x03;
>>>> +		len &= 0x03;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Now read the remaining 1 to 3 byte or complete length if
>>>> +	 * unaligned address.
>>>> +	 */
>>>> +	if (len > 4) {
>>>> +		for (i = 0; i < (len / 4); i++) {
>>>> +			*(u32 *)dst = musb_readl(fifo, 0);
>>>> +			dst += 4;
>>>> +		}
>>>> +		len &= 0x03;
>>>> +	}
>>>> +
>>>> +	if (len > 0) {
>>>> +		val = musb_readl(fifo, 0);
>>>> +		memcpy(dst, &val, len);
>>>> +	}
>>>> +}
>>> [...]
>>>

[U-Boot] [PATCH v3 3/4] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.

[Marek Vasut]

On 03/17/2016 10:58 AM, Purna Chandra Mandal wrote:
> On 03/16/2016 09:18 PM, Marek Vasut wrote:
> 
>> On 03/16/2016 10:58 AM, Purna Chandra Mandal wrote:
>>> On 03/15/2016 11:49 PM, Marek Vasut wrote:
>>>
>>>> On 03/15/2016 01:44 PM, Purna Chandra Mandal wrote:
>>>>> This driver adds support of PIC32 MUSB OTG controller as dual role device.
>>>>> It implements platform specific glue to reuse musb core.
>>>>>
>>>>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>>>>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>>>> [...]
>>>>
>>>>> diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c
>>>>> new file mode 100644
>>>>> index 0000000..980a971
>>>>> --- /dev/null
>>>>> +++ b/drivers/usb/musb-new/pic32.c
>>>>> @@ -0,0 +1,294 @@
>>>>> +/*
>>>>> + * Microchip PIC32 MUSB "glue layer"
>>>>> + *
>>>>> + * Copyright (C) 2015, Microchip Technology Inc.
>>>>> + *  Cristian Birsan <cristian.birsan@microchip.com>
>>>>> + *  Purna Chandra Mandal <purna.mandal@microchip.com>
>>>>> + *
>>>>> + * SPDX-License-Identifier:     GPL-2.0+
>>>>> + *
>>>>> + * Based on the dsps "glue layer" code.
>>>>> + */
>>>>> +
>>>>> +#include <common.h>
>>>>> +#include <linux/usb/musb.h>
>>>>> +#include "linux-compat.h"
>>>>> +#include "musb_core.h"
>>>>> +#include "musb_uboot.h"
>>>>> +
>>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>>> +
>>>>> +#define PIC32_TX_EP_MASK	0x0f		/* EP0 + 7 Tx EPs */
>>>>> +#define PIC32_RX_EP_MASK	0x0e		/* 7 Rx EPs */
>>>>> +
>>>>> +#define MUSB_SOFTRST		0x7f
>>>>> +#define  MUSB_SOFTRST_NRST	BIT(0)
>>>>> +#define  MUSB_SOFTRST_NRSTX	BIT(1)
>>>>> +
>>>>> +#define USBCRCON		0
>>>>> +#define  USBCRCON_USBWKUPEN	BIT(0)  /* Enable Wakeup Interrupt */
>>>>> +#define  USBCRCON_USBRIE	BIT(1)  /* Enable Remote resume Interrupt */
>>>>> +#define  USBCRCON_USBIE		BIT(2)  /* Enable USB General interrupt */
>>>>> +#define  USBCRCON_SENDMONEN	BIT(3)  /* Enable Session End VBUS monitoring */
>>>>> +#define  USBCRCON_BSVALMONEN	BIT(4)  /* Enable B-Device VBUS monitoring */
>>>>> +#define  USBCRCON_ASVALMONEN	BIT(5)  /* Enable A-Device VBUS monitoring */
>>>>> +#define  USBCRCON_VBUSMONEN	BIT(6)  /* Enable VBUS monitoring */
>>>>> +#define  USBCRCON_PHYIDEN	BIT(7)  /* PHY ID monitoring enable */
>>>>> +#define  USBCRCON_USBIDVAL	BIT(8)  /* USB ID value */
>>>>> +#define  USBCRCON_USBIDOVEN	BIT(9)  /* USB ID override enable */
>>>>> +#define  USBCRCON_USBWK		BIT(24) /* USB Wakeup Status */
>>>>> +#define  USBCRCON_USBRF		BIT(25) /* USB Resume Status */
>>>>> +#define  USBCRCON_USBIF		BIT(26) /* USB General Interrupt Status */
>>>>> +
>>>>> +static void __iomem *musb_glue;
>>>> What would happen once you make a chip with two MUSB controllers ?
>>> Currently PIC32 has only one MUSB controller and only one glue reg-space.
>>> Don't know how the reg-map will be in future when PIC32 will have multiple
>>> MUSB controllers. Assuming that glue address map will be separate for
>>> each controller we can add logic to support multiple MUSB controller.
>>>
>>> IMO, better if we don't assume something of the future and bloat logic.
>> If you switch this to driver model, you will need to weed out all the
>> static global variables anyway. Better do it now.
> 
> Thanks. Will do.

Thanks!

>>>>> +/* pic32_musb_disable - disable HDRC */
>>>>> +static void pic32_musb_disable(struct musb *musb)
>>>>> +{
>>>> Is there no way to shut down the MUSB on the PIC32 ?
>>> There is no way to disable MUSB.
>> Yet another broken chip design. Can't you put the controller into reset
>> and gate the clock for it ?
> 
> USB clock can't be gated! USB controller clock is derived from peripheral bus
> clock(PBCLK5) which is shared with other modules and USB Phy clock derived from
> UPLL can;t be gated at all.

I see, oh well, then it cannot be helped.

>>>>> +}
>>>>> +
>>>>> +/* pic32_musb_enable - enable HDRC */
>>>>> +static int pic32_musb_enable(struct musb *musb)
>>>>> +{
>>>>> +	/* soft reset by NRSTx */
>>>>> +	musb_writeb(musb->mregs, MUSB_SOFTRST, MUSB_SOFTRST_NRSTX);
>>>>> +	/* set mode */
>>>>> +	musb_platform_set_mode(musb, musb->board_mode);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t pic32_interrupt(int irq, void *hci)
>>>>> +{
>>>>> +	struct musb  *musb = hci;
>>>>> +	irqreturn_t ret = IRQ_NONE;
>>>>> +	u32 epintr, usbintr;
>>>>> +
>>>>> +	/* Get usb core interrupts */
>>>> You mean "get" or "ack" here ?
>>> I meant read-and-ack. Will update comment.
>> Thanks
>>
>>>>> +	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
>>>>> +	if (musb->int_usb)
>>>>> +		musb_writeb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
>>>>> +
>>>>> +	/* Get endpoint interrupts */
>>>> DTTO
>>> I meant read-and-ack.
>>>
>>>>> +	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX) & PIC32_RX_EP_MASK;
>>>>> +	if (musb->int_rx)
>>>>> +		musb_writew(musb->mregs, MUSB_INTRRX, musb->int_rx);
>>>> Same here
>>> ack. Will update comment.
>>>
>>>>> +	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX) & PIC32_TX_EP_MASK;
>>>>> +	if (musb->int_tx)
>>>>> +		musb_writew(musb->mregs, MUSB_INTRTX, musb->int_tx);
>>>>> +
>>>>> +	/* Drop spurious RX and TX if device is disconnected */
>>>>> +	if (musb->int_usb & MUSB_INTR_DISCONNECT) {
>>>>> +		musb->int_tx = 0;
>>>>> +		musb->int_rx = 0;
>>>>> +	}
>>>>> +
>>>>> +	if (musb->int_tx || musb->int_rx || musb->int_usb)
>>>>> +		ret |= musb_interrupt(musb);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int pic32_musb_set_mode(struct musb *musb, u8 mode)
>>>>> +{
>>>>> +	struct device *dev = musb->controller;
>>>>> +
>>>>> +	switch (mode) {
>>>>> +	case MUSB_HOST:
>>>>> +		clrsetbits_le32(musb_glue + USBCRCON,
>>>>> +				USBCRCON_USBIDVAL, USBCRCON_USBIDOVEN);
>>>>> +		break;
>>>>> +	case MUSB_PERIPHERAL:
>>>>> +		setbits_le32(musb_glue + USBCRCON,
>>>>> +			     USBCRCON_USBIDVAL | USBCRCON_USBIDOVEN);
>>>>> +		break;
>>>>> +	case MUSB_OTG:
>>>>> +		dev_err(dev, "MUSB OTG mode enabled\n");
>>>> So having the core in OTG mode is an error ? Why ?
>>> In PIC32 we haven't tested OTG. We use the controller as dual-role not as OTG.
>>> In future we might enable that. 
>> Then you might want to say "support for OTG is unimplemented" ?
> 
> Will update.

Thanks

>>>>> +		break;
>>>>> +	default:
>>>>> +		dev_err(dev, "unsupported mode %d\n", mode);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int pic32_musb_init(struct musb *musb)
>>>>> +{
>>>>> +	u32 ctrl, hwvers;
>>>>> +	u8 power;
>>>>> +
>>>>> +	/* Returns zero if not clocked */
>>>>> +	hwvers = musb_read_hwvers(musb->mregs);
>>>>> +	if (!hwvers)
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	/* Reset the musb */
>>>>> +	power = musb_readb(musb->mregs, MUSB_POWER);
>>>>> +	power = power | MUSB_POWER_RESET;
>>>>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>>>>> +	mdelay(100);
>>>>> +
>>>>> +	/* Start the on-chip PHY and its PLL. */
>>>>> +	power = power & ~MUSB_POWER_RESET;
>>>>> +	musb_writeb(musb->mregs, MUSB_POWER, power);
>>>>> +
>>>>> +	musb->isr = pic32_interrupt;
>>>>> +
>>>>> +	ctrl =  USBCRCON_USBIF | USBCRCON_USBRF |
>>>>> +		USBCRCON_USBWK | USBCRCON_USBIDOVEN |
>>>>> +		USBCRCON_PHYIDEN | USBCRCON_USBIE |
>>>>> +		USBCRCON_USBRIE | USBCRCON_USBWKUPEN |
>>>>> +		USBCRCON_VBUSMONEN;
>>>>> +	writel(ctrl, musb_glue + USBCRCON);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +/* PIC32 supports only 32bit read operation */
>>>>> +void musb_read_fifo(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
>>>>> +{
>>>>> +	void __iomem *fifo = hw_ep->fifo;
>>>>> +	u32 val;
>>>>> +	int i;
>>>> This could become:
>>>>  bulk_len = len / 4;
>>>>  bulk_rem = len % 4;
>>>>  readsl(fifo, dst, bulk_len);
>>>>  if (rem) {
>>>>   dst += len & ~0x3;
>>>>   tmp = readl(fifo);
>>>>   copy the remaining bytes according to endianness
>>>>  }
>>>>
>>>> This is 10 LoC , not 50 ;-)
>>> May be comments in my code are not very clear :)
>>>
>>> All the extra code is for handling (4-byte-word) alignment of destination buffer.
>>> Writing word to unaligned address will generate exception in MIPS which is not handled
>>> in U-Boot software.
>>> Note MIPS can't handle unaligned access in h/w unless specific unaligned
>>> instructions are used.
>> Ouch, did you ever see unaligned access here ?
> 
> Not seen unaligned access here but not sure of!
> Char array can be starting from any arbitrary address unless it is
> malloc()'d or special care is taken to make it aligned at word boundary.
> 
> I will take your logic as I'm not facing real problem.

The USB stack should be using only aligned buffers, so what you can try
to do here is trap unaligned accesses and warn about them. Maybe factor
out the unaligned access code and trigger it only if unaligned access
really happens.

>>>>> +	/* Read for 32bit-aligned destination address */
>>>>> +	if (likely((0x03 & (unsigned long)dst) == 0) && len >= 4) {
>>>>> +		readsl(fifo, dst, len / 4);
>>>>> +		dst += len & ~0x03;
>>>>> +		len &= 0x03;
>>>>> +	}
>>>>> +
>>>>> +	/*
>>>>> +	 * Now read the remaining 1 to 3 byte or complete length if
>>>>> +	 * unaligned address.
>>>>> +	 */
>>>>> +	if (len > 4) {
>>>>> +		for (i = 0; i < (len / 4); i++) {
>>>>> +			*(u32 *)dst = musb_readl(fifo, 0);
>>>>> +			dst += 4;
>>>>> +		}
>>>>> +		len &= 0x03;
>>>>> +	}
>>>>> +
>>>>> +	if (len > 0) {
>>>>> +		val = musb_readl(fifo, 0);
>>>>> +		memcpy(dst, &val, len);
>>>>> +	}
>>>>> +}
>>>> [...]
>>>>
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v3 4/4] board: pic32mzda: enable USB-host, USB-storage support.

[Purna Chandra Mandal]

Enable MUSB host and USB storage support for Microchip
PIC32MZ[DA] Starter Kit.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>

---

Changes in v3:
- add arch specific reads{bwlq}, writes{bwlq} in respective arch io.h
- remove reads{bwlq}, writes{bwlq} in musb-new driver

Changes in v2:
- compilation fix in drivers/usb/musb-new/linux-compat.h seperated
- compilation fix in drivers/gadget/f_mass_storage.c seperated

 arch/mips/dts/pic32mzda.dtsi   | 10 ++++++++++
 arch/mips/dts/pic32mzda_sk.dts |  4 ++++
 configs/pic32mzdask_defconfig  |  6 +++++-
 include/configs/pic32mzdask.h  |  7 +++++++
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/mips/dts/pic32mzda.dtsi b/arch/mips/dts/pic32mzda.dtsi
index 7d180d9..57e4500 100644
--- a/arch/mips/dts/pic32mzda.dtsi
+++ b/arch/mips/dts/pic32mzda.dtsi
@@ -171,4 +171,14 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 	};
+
+	usb: musb at 1f8e3000 {
+		compatible = "microchip,pic32mzda-usb";
+		reg = <0x1f8e3000 0x1000>,
+		      <0x1f884000 0x1000>;
+		reg-names = "mc", "control";
+		interrupts = <132 IRQ_TYPE_EDGE_RISING>,
+			     <133 IRQ_TYPE_LEVEL_HIGH>;
+		status = "disabled";
+	};
 };
diff --git a/arch/mips/dts/pic32mzda_sk.dts b/arch/mips/dts/pic32mzda_sk.dts
index e5ce0bd..0a7847e 100644
--- a/arch/mips/dts/pic32mzda_sk.dts
+++ b/arch/mips/dts/pic32mzda_sk.dts
@@ -52,4 +52,8 @@
 	ethernet_phy: lan8740_phy at 0 {
 		reg = <0>;
 	};
+};
+
+&usb {
+	status = "okay";
 };
\ No newline at end of file
diff --git a/configs/pic32mzdask_defconfig b/configs/pic32mzdask_defconfig
index 1dbe1b5..544112f 100644
--- a/configs/pic32mzdask_defconfig
+++ b/configs/pic32mzdask_defconfig
@@ -12,6 +12,7 @@ CONFIG_SYS_PROMPT="dask # "
 CONFIG_LOOPW=y
 CONFIG_CMD_MEMTEST=y
 CONFIG_CMD_MEMINFO=y
+CONFIG_CMD_USB=y
 # CONFIG_CMD_FPGA is not set
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_RARP=y
@@ -28,6 +29,9 @@ CONFIG_DM_ETH=y
 CONFIG_PIC32_ETH=y
 CONFIG_PINCTRL=y
 # CONFIG_PINCTRL_FULL is not set
-CONFIG_SYS_VSNPRINTF=y
+CONFIG_USB=y
+CONFIG_DM_USB=y
+CONFIG_USB_MUSB_HOST=y
+CONFIG_USB_STORAGE=y
 CONFIG_USE_TINY_PRINTF=y
 CONFIG_CMD_DHRYSTONE=y
diff --git a/include/configs/pic32mzdask.h b/include/configs/pic32mzdask.h
index 2d35a0b..1d5be2b 100644
--- a/include/configs/pic32mzdask.h
+++ b/include/configs/pic32mzdask.h
@@ -117,6 +117,12 @@
 #define CONFIG_GENERIC_MMC
 #define CONFIG_CMD_MMC
 
+/*--------------------------------------------------
+ * USB Configuration
+ */
+#define CONFIG_USB_MUSB_PIO_ONLY
+#define CONFIG_SYS_CACHELINE_SIZE	16
+
 /*-----------------------------------------------------------------------
  * File System Configuration
  */
@@ -167,6 +173,7 @@
 
 #define BOOT_TARGET_DEVICES(func)	\
 	func(MMC, mmc, 0)		\
+	func(USB, usb, 0)		\
 	func(DHCP, dhcp, na)
 
 #include <config_distro_bootcmd.h>
-- 
1.8.3.1

[U-Boot] [PATCH v3 4/4] board: pic32mzda: enable USB-host, USB-storage support.

[Daniel Schwierzeck]

Am 15.03.2016 um 13:44 schrieb Purna Chandra Mandal:
> Enable MUSB host and USB storage support for Microchip
> PIC32MZ[DA] Starter Kit.
> 
> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
> 
> ---
> 
> Changes in v3:
> - add arch specific reads{bwlq}, writes{bwlq} in respective arch io.h
> - remove reads{bwlq}, writes{bwlq} in musb-new driver
> 
> Changes in v2:
> - compilation fix in drivers/usb/musb-new/linux-compat.h seperated
> - compilation fix in drivers/gadget/f_mass_storage.c seperated
> 
>  arch/mips/dts/pic32mzda.dtsi   | 10 ++++++++++
>  arch/mips/dts/pic32mzda_sk.dts |  4 ++++
>  configs/pic32mzdask_defconfig  |  6 +++++-
>  include/configs/pic32mzdask.h  |  7 +++++++
>  4 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/mips/dts/pic32mzda.dtsi b/arch/mips/dts/pic32mzda.dtsi
> index 7d180d9..57e4500 100644
> --- a/arch/mips/dts/pic32mzda.dtsi
> +++ b/arch/mips/dts/pic32mzda.dtsi
> @@ -171,4 +171,14 @@
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  	};
> +
> +	usb: musb at 1f8e3000 {
> +		compatible = "microchip,pic32mzda-usb";
> +		reg = <0x1f8e3000 0x1000>,
> +		      <0x1f884000 0x1000>;
> +		reg-names = "mc", "control";
> +		interrupts = <132 IRQ_TYPE_EDGE_RISING>,
> +			     <133 IRQ_TYPE_LEVEL_HIGH>;
> +		status = "disabled";
> +	};
>  };
> diff --git a/arch/mips/dts/pic32mzda_sk.dts b/arch/mips/dts/pic32mzda_sk.dts
> index e5ce0bd..0a7847e 100644
> --- a/arch/mips/dts/pic32mzda_sk.dts
> +++ b/arch/mips/dts/pic32mzda_sk.dts
> @@ -52,4 +52,8 @@
>  	ethernet_phy: lan8740_phy at 0 {
>  		reg = <0>;
>  	};
> +};
> +
> +&usb {
> +	status = "okay";
>  };
> \ No newline at end of file
> diff --git a/configs/pic32mzdask_defconfig b/configs/pic32mzdask_defconfig
> index 1dbe1b5..544112f 100644
> --- a/configs/pic32mzdask_defconfig
> +++ b/configs/pic32mzdask_defconfig
> @@ -12,6 +12,7 @@ CONFIG_SYS_PROMPT="dask # "
>  CONFIG_LOOPW=y
>  CONFIG_CMD_MEMTEST=y
>  CONFIG_CMD_MEMINFO=y
> +CONFIG_CMD_USB=y
>  # CONFIG_CMD_FPGA is not set
>  CONFIG_CMD_GPIO=y
>  CONFIG_CMD_RARP=y
> @@ -28,6 +29,9 @@ CONFIG_DM_ETH=y
>  CONFIG_PIC32_ETH=y
>  CONFIG_PINCTRL=y
>  # CONFIG_PINCTRL_FULL is not set
> -CONFIG_SYS_VSNPRINTF=y
> +CONFIG_USB=y
> +CONFIG_DM_USB=y
> +CONFIG_USB_MUSB_HOST=y
> +CONFIG_USB_STORAGE=y
>  CONFIG_USE_TINY_PRINTF=y
>  CONFIG_CMD_DHRYSTONE=y
> diff --git a/include/configs/pic32mzdask.h b/include/configs/pic32mzdask.h
> index 2d35a0b..1d5be2b 100644
> --- a/include/configs/pic32mzdask.h
> +++ b/include/configs/pic32mzdask.h
> @@ -117,6 +117,12 @@
>  #define CONFIG_GENERIC_MMC
>  #define CONFIG_CMD_MMC
>  
> +/*--------------------------------------------------
> + * USB Configuration
> + */
> +#define CONFIG_USB_MUSB_PIO_ONLY
> +#define CONFIG_SYS_CACHELINE_SIZE	16

I see CONFIG_SYS_CACHELINE_SIZE is used in drivers/usb/ in various memalign() calls. Actually we have ARCH_DMA_MINALIGN for this case. At least for MIPS I want to get rid of CONFIG_SYS_CACHELINE_SIZE in the future becasue we have auto-detection for that. 

If possible I'd like to see a patch which replaces CONFIG_SYS_CACHELINE_SIZE with ARCH_DMA_MINALIGN in drivers/user/. Marek what do you think?

> +
>  /*-----------------------------------------------------------------------
>   * File System Configuration
>   */
> @@ -167,6 +173,7 @@
>  
>  #define BOOT_TARGET_DEVICES(func)	\
>  	func(MMC, mmc, 0)		\
> +	func(USB, usb, 0)		\
>  	func(DHCP, dhcp, na)
>  
>  #include <config_distro_bootcmd.h>
> 

-- 
- Daniel

[U-Boot] [PATCH v3 4/4] board: pic32mzda: enable USB-host, USB-storage support.

[Marek Vasut]

On 03/16/2016 10:12 AM, Daniel Schwierzeck wrote:
> 
> 
> Am 15.03.2016 um 13:44 schrieb Purna Chandra Mandal:
>> Enable MUSB host and USB storage support for Microchip
>> PIC32MZ[DA] Starter Kit.
>>
>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>>
>> ---
>>
>> Changes in v3:
>> - add arch specific reads{bwlq}, writes{bwlq} in respective arch io.h
>> - remove reads{bwlq}, writes{bwlq} in musb-new driver
>>
>> Changes in v2:
>> - compilation fix in drivers/usb/musb-new/linux-compat.h seperated
>> - compilation fix in drivers/gadget/f_mass_storage.c seperated
>>
>>  arch/mips/dts/pic32mzda.dtsi   | 10 ++++++++++
>>  arch/mips/dts/pic32mzda_sk.dts |  4 ++++
>>  configs/pic32mzdask_defconfig  |  6 +++++-
>>  include/configs/pic32mzdask.h  |  7 +++++++
>>  4 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/dts/pic32mzda.dtsi b/arch/mips/dts/pic32mzda.dtsi
>> index 7d180d9..57e4500 100644
>> --- a/arch/mips/dts/pic32mzda.dtsi
>> +++ b/arch/mips/dts/pic32mzda.dtsi
>> @@ -171,4 +171,14 @@
>>  		#address-cells = <1>;
>>  		#size-cells = <0>;
>>  	};
>> +
>> +	usb: musb at 1f8e3000 {
>> +		compatible = "microchip,pic32mzda-usb";
>> +		reg = <0x1f8e3000 0x1000>,
>> +		      <0x1f884000 0x1000>;
>> +		reg-names = "mc", "control";
>> +		interrupts = <132 IRQ_TYPE_EDGE_RISING>,
>> +			     <133 IRQ_TYPE_LEVEL_HIGH>;
>> +		status = "disabled";
>> +	};
>>  };
>> diff --git a/arch/mips/dts/pic32mzda_sk.dts b/arch/mips/dts/pic32mzda_sk.dts
>> index e5ce0bd..0a7847e 100644
>> --- a/arch/mips/dts/pic32mzda_sk.dts
>> +++ b/arch/mips/dts/pic32mzda_sk.dts
>> @@ -52,4 +52,8 @@
>>  	ethernet_phy: lan8740_phy at 0 {
>>  		reg = <0>;
>>  	};
>> +};
>> +
>> +&usb {
>> +	status = "okay";
>>  };
>> \ No newline at end of file
>> diff --git a/configs/pic32mzdask_defconfig b/configs/pic32mzdask_defconfig
>> index 1dbe1b5..544112f 100644
>> --- a/configs/pic32mzdask_defconfig
>> +++ b/configs/pic32mzdask_defconfig
>> @@ -12,6 +12,7 @@ CONFIG_SYS_PROMPT="dask # "
>>  CONFIG_LOOPW=y
>>  CONFIG_CMD_MEMTEST=y
>>  CONFIG_CMD_MEMINFO=y
>> +CONFIG_CMD_USB=y
>>  # CONFIG_CMD_FPGA is not set
>>  CONFIG_CMD_GPIO=y
>>  CONFIG_CMD_RARP=y
>> @@ -28,6 +29,9 @@ CONFIG_DM_ETH=y
>>  CONFIG_PIC32_ETH=y
>>  CONFIG_PINCTRL=y
>>  # CONFIG_PINCTRL_FULL is not set
>> -CONFIG_SYS_VSNPRINTF=y
>> +CONFIG_USB=y
>> +CONFIG_DM_USB=y
>> +CONFIG_USB_MUSB_HOST=y
>> +CONFIG_USB_STORAGE=y
>>  CONFIG_USE_TINY_PRINTF=y
>>  CONFIG_CMD_DHRYSTONE=y
>> diff --git a/include/configs/pic32mzdask.h b/include/configs/pic32mzdask.h
>> index 2d35a0b..1d5be2b 100644
>> --- a/include/configs/pic32mzdask.h
>> +++ b/include/configs/pic32mzdask.h
>> @@ -117,6 +117,12 @@
>>  #define CONFIG_GENERIC_MMC
>>  #define CONFIG_CMD_MMC
>>  
>> +/*--------------------------------------------------
>> + * USB Configuration
>> + */
>> +#define CONFIG_USB_MUSB_PIO_ONLY
>> +#define CONFIG_SYS_CACHELINE_SIZE	16
> 
> I see CONFIG_SYS_CACHELINE_SIZE is used in drivers/usb/ in various memalign() calls. Actually we have ARCH_DMA_MINALIGN for this case. At least for MIPS I want to get rid of CONFIG_SYS_CACHELINE_SIZE in the future becasue we have auto-detection for that. 
> 
> If possible I'd like to see a patch which replaces CONFIG_SYS_CACHELINE_SIZE with ARCH_DMA_MINALIGN in drivers/user/. Marek what do you think?

I think that makes sense.

>> +
>>  /*-----------------------------------------------------------------------
>>   * File System Configuration
>>   */
>> @@ -167,6 +173,7 @@
>>  
>>  #define BOOT_TARGET_DEVICES(func)	\
>>  	func(MMC, mmc, 0)		\
>> +	func(USB, usb, 0)		\
>>  	func(DHCP, dhcp, na)
>>  
>>  #include <config_distro_bootcmd.h>
>>
> 


-- 
Best regards,
Marek Vasut