From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 9 Feb 2016 15:50:22 +0100 Subject: [U-Boot] [PATCH v1 1/2] drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller. In-Reply-To: <56B9E90D.7060509@microchip.com> References: <1455019370-12197-1-git-send-email-purna.mandal@microchip.com> <201602091418.58398.marex@denx.de> <56B9E90D.7060509@microchip.com> Message-ID: <201602091550.22973.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > >> > >> 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 > >> Signed-off-by: Purna Chandra Mandal > >> --- > >> > >> 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.