[U-Boot] [PATCH v1 1/2] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.

[Marek Vasut]

On Tuesday, February 09, 2016 at 02:26:37 PM, Purna Chandra Mandal wrote:
> On 02/09/2016 06:48 PM, Marek Vasut wrote:
> > On Tuesday, February 09, 2016 at 01:02:49 PM, Purna Chandra Mandal wrote:
> >> From: Cristian Birsan <cristian.birsan at microchip.com>
> >> 
> >> 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 at microchip.com>
> >> Signed-off-by: Purna Chandra Mandal <purna.mandal at microchip.com>
> >> ---
> >> 
> >>  drivers/usb/gadget/f_mass_storage.c |   2 +
> >>  drivers/usb/musb-new/Kconfig        |   7 +
> >>  drivers/usb/musb-new/Makefile       |   1 +
> >>  drivers/usb/musb-new/linux-compat.h |   2 +
> >>  drivers/usb/musb-new/musb_core.c    |   2 +-
> >>  drivers/usb/musb-new/pic32.c        | 294
> >> 
> >> ++++++++++++++++++++++++++++++++++++ 6 files changed, 307 insertions(+),
> >> 1 deletion(-)
> >> 
> >>  create mode 100644 drivers/usb/musb-new/pic32.c
> >> 
> >> diff --git a/drivers/usb/gadget/f_mass_storage.c
> >> b/drivers/usb/gadget/f_mass_storage.c index 1ecb92a..8ca02f2 100644
> >> --- a/drivers/usb/gadget/f_mass_storage.c
> >> +++ b/drivers/usb/gadget/f_mass_storage.c
> >> @@ -283,6 +283,7 @@ static const char fsg_string_interface[] = "Mass
> >> Storage"; struct kref {int x; };
> >> 
> >>  struct completion {int x; };
> >> 
> >> +#if !defined(CONFIG_MIPS)
> > 
> > Why is this change needed, endianness issues ?
> > 
> > Also, you should put this into separate patch.
> 
> This is to fix compilation error with MIPS.
> MIPS already implements these functions in "arch/mips/include/asm/bitops.h"
> (recently added). I'll add this in separate patch.

Ouch, I don't like seeing a driver become a platform abstraction library :(
Unless you explicitly want to learn how far this rabbit hole goes, I won't
push you to fix this. But if you really enjoy pain, patches are welcome to
implement some sort of generic __weak set_bit and clear_bit() code.

> >>  inline void set_bit(int nr, volatile void *addr)
> >>  {
> >>  
> >>  	int	mask;
> >> 
> >> @@ -302,6 +303,7 @@ inline void clear_bit(int nr, volatile void *addr)
> >> 
> >>  	mask = 1 << (nr & 0x1f);
> >>  	*a &= ~mask;
> >>  
> >>  }
> >> 
> >> +#endif
> >> 
> >>  struct fsg_dev;
> >>  struct fsg_common;
> > 
> > [...]
> > 
> >> diff --git a/drivers/usb/musb-new/linux-compat.h
> >> b/drivers/usb/musb-new/linux-compat.h index 46f83d9..9ac48c1 100644
> >> --- a/drivers/usb/musb-new/linux-compat.h
> >> +++ b/drivers/usb/musb-new/linux-compat.h
> >> @@ -13,12 +13,14 @@
> >> 
> >>  		printf(fmt, ##args);		\
> >>  	
> >>  	ret_warn; })
> >> 
> >> +#if !defined(CONFIG_MIPS)
> >> 
> >>  #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)
> >> 
> >> +#endif
> > 
> > Why is this needed?
> 
> To fix compilation error with MIPS.
> MIPS already implements these macros in "arch/mips/include/asm/io.h".

OK, but these macros are also implemented on ARM systems and it's not a
problem there.We should just nuke this whole section in a separate patch
and see if some platform catches fire and fix that one instead.

> >>  #define device_init_wakeup(dev, a) do {} while (0)
> > 
> > [...]
> > 
> >> +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);
> > 
> > Is pic32 mipsel ? Or is the core doing endian swapping ?
> > 
> > I would expect _be32() stuff on mips.
> 
> In PIC32 MIPS is little-endian.

Wow, neat :) Thanks for clarifying!

> >> +		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;
> >> +}
> > 
> > [...]
> > 
> > Looks good otherwise.