linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] USB changes for rare warnings
@ 2016-01-28 16:15 Arnd Bergmann
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
                   ` (3 more replies)
  0 siblings, 4 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:15 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman

Hi Felipe,

This set of patches addresses warnings I got in randconfig builds,
in the USB drivers. The first three patches are for the pxa25x
UDC driver and are a larger scale cleanup triggered by finding
the initial bug. The other four are relatively simple but still
need to be reviewed properly, as I have done only compile-time
testing. After these patches, I get no other warnings for USB
drivers at the moment.

Arnd Bergmann (7):
  usb: gadget: pxa25x_udc: move register definitions from arch
  usb: gadget: pxa25x_udc cleanup
  usb: gadget: pxa25x_udc: use readl/writel for mmio
  usb: fsl: drop USB_FSL_MPH_DR_OF Kconfig symbol
  usb: isp1301-omap: mark power_up as __maybe_unused
  usb: musb: use %pad format string from dma_addr_t
  usb: musb/ux500: remove duplicate check for dma_is_compatible

 arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 198 ---------
 arch/arm/mach-pxa/include/mach/pxa25x-udc.h     | 163 --------
 drivers/usb/Makefile                            |   2 +-
 drivers/usb/gadget/udc/Kconfig                  |   1 -
 drivers/usb/gadget/udc/pxa25x_udc.c             | 530 +++++++++++++++++-------
 drivers/usb/gadget/udc/pxa25x_udc.h             |  11 +-
 drivers/usb/host/Kconfig                        |   4 -
 drivers/usb/host/Makefile                       |   3 +-
 drivers/usb/musb/musbhsdma.c                    |   8 +-
 drivers/usb/musb/tusb6010_omap.c                |   4 +-
 drivers/usb/musb/ux500_dma.c                    |   3 -
 drivers/usb/phy/phy-isp1301-omap.c              |   2 +-
 12 files changed, 400 insertions(+), 529 deletions(-)

-- 
2.7.0

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
  2016-01-28 16:15 [PATCH 0/7] USB changes for rare warnings Arnd Bergmann
@ 2016-01-28 16:17 ` Arnd Bergmann
  2016-01-28 16:17   ` [PATCH 2/7] usb: gadget: pxa25x_udc cleanup Arnd Bergmann
                     ` (4 more replies)
  2016-01-28 16:20 ` [PATCH 4/7] usb: fsl: drop USB_FSL_MPH_DR_OF Kconfig symbol Arnd Bergmann
                   ` (2 subsequent siblings)
  3 siblings, 5 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:17 UTC (permalink / raw)
  To: Felipe Balbi, Imre Kaloz, Krzysztof Halasa, Russell King,
	Daniel Mack, Haojian Zhuang, Robert Jarzmik, Felipe Balbi,
	Greg Kroah-Hartman
  Cc: linux-arm-kernel, Arnd Bergmann, linux-usb, linux-kernel

ixp4xx and pxa25x both use this driver and provide a slightly
different set of register definitions for it. Aside from that,
the definition in the ixp4xx-regs.h header conflicts with the
on in the pxa27x device driver when compile-testing that:

In file included from ../drivers/usb/gadget/udc/pxa27x_udc.c:37:0:
../drivers/usb/gadget/udc/pxa27x_udc.h:26:0: warning: "UDCCR" redefined
 #define UDCCR  0x0000  /* UDC Control Register */
 ^
In file included from ../arch/arm/mach-ixp4xx/include/mach/hardware.h:27:0,
                 from ../arch/arm/mach-ixp4xx/include/mach/io.h:18,
                 from ../arch/arm/include/asm/io.h:194,
                 from ../include/linux/io.h:25,
                 from ../include/linux/irq.h:24,
                 from ../drivers/usb/gadget/udc/pxa27x_udc.c:23:
../arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h:415:0: note: this is the location of the previous definition
 #define UDCCR  IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0000)

This addresses both issues by moving all the definitions into the
pxa25x_udc driver itself. It turns out the only difference between
them was 'UDCCS_IO_ROF', and that could well be a mistake when it
was incorrectly copied from pxa25x to ixp4xx.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 198 ------------------------
 arch/arm/mach-pxa/include/mach/pxa25x-udc.h     | 163 -------------------
 drivers/usb/gadget/udc/pxa25x_udc.c             | 161 ++++++++++++++++++-
 3 files changed, 155 insertions(+), 367 deletions(-)

diff --git a/arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h b/arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h
index c5bae9c035d5..b7ddd27419c2 100644
--- a/arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h
+++ b/arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h
@@ -395,204 +395,6 @@
 #define CRP_AD_CBE_BESL         20
 #define CRP_AD_CBE_WRITE	0x00010000
 
-
-/*
- * USB Device Controller
- *
- * These are used by the USB gadget driver, so they don't follow the
- * IXP4XX_ naming convetions.
- *
- */
-# define IXP4XX_USB_REG(x)       (*((volatile u32 *)(x)))
-
-/* UDC Undocumented - Reserved1 */
-#define UDC_RES1	IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0004)  
-/* UDC Undocumented - Reserved2 */
-#define UDC_RES2	IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0008)  
-/* UDC Undocumented - Reserved3 */
-#define UDC_RES3	IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x000C)  
-/* UDC Control Register */
-#define UDCCR		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0000)  
-/* UDC Endpoint 0 Control/Status Register */
-#define UDCCS0		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0010)  
-/* UDC Endpoint 1 (IN) Control/Status Register */
-#define UDCCS1		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0014)  
-/* UDC Endpoint 2 (OUT) Control/Status Register */
-#define UDCCS2		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0018)  
-/* UDC Endpoint 3 (IN) Control/Status Register */
-#define UDCCS3		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x001C)  
-/* UDC Endpoint 4 (OUT) Control/Status Register */
-#define UDCCS4		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0020)  
-/* UDC Endpoint 5 (Interrupt) Control/Status Register */
-#define UDCCS5		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0024)  
-/* UDC Endpoint 6 (IN) Control/Status Register */
-#define UDCCS6		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0028)  
-/* UDC Endpoint 7 (OUT) Control/Status Register */
-#define UDCCS7		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x002C)  
-/* UDC Endpoint 8 (IN) Control/Status Register */
-#define UDCCS8		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0030)  
-/* UDC Endpoint 9 (OUT) Control/Status Register */
-#define UDCCS9		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0034)  
-/* UDC Endpoint 10 (Interrupt) Control/Status Register */
-#define UDCCS10		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0038)  
-/* UDC Endpoint 11 (IN) Control/Status Register */
-#define UDCCS11		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x003C)  
-/* UDC Endpoint 12 (OUT) Control/Status Register */
-#define UDCCS12		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0040)  
-/* UDC Endpoint 13 (IN) Control/Status Register */
-#define UDCCS13		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0044)  
-/* UDC Endpoint 14 (OUT) Control/Status Register */
-#define UDCCS14		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0048)  
-/* UDC Endpoint 15 (Interrupt) Control/Status Register */
-#define UDCCS15		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x004C)  
-/* UDC Frame Number Register High */
-#define UFNRH		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0060)  
-/* UDC Frame Number Register Low */
-#define UFNRL		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0064)  
-/* UDC Byte Count Reg 2 */
-#define UBCR2		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0068)  
-/* UDC Byte Count Reg 4 */
-#define UBCR4		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x006c)  
-/* UDC Byte Count Reg 7 */
-#define UBCR7		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0070)  
-/* UDC Byte Count Reg 9 */
-#define UBCR9		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0074)  
-/* UDC Byte Count Reg 12 */
-#define UBCR12		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0078)  
-/* UDC Byte Count Reg 14 */
-#define UBCR14		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x007c)  
-/* UDC Endpoint 0 Data Register */
-#define UDDR0		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0080)  
-/* UDC Endpoint 1 Data Register */
-#define UDDR1		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0100)  
-/* UDC Endpoint 2 Data Register */
-#define UDDR2		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0180)  
-/* UDC Endpoint 3 Data Register */
-#define UDDR3		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0200)  
-/* UDC Endpoint 4 Data Register */
-#define UDDR4		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0400)  
-/* UDC Endpoint 5 Data Register */
-#define UDDR5		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x00A0)  
-/* UDC Endpoint 6 Data Register */
-#define UDDR6		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0600)  
-/* UDC Endpoint 7 Data Register */
-#define UDDR7		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0680)  
-/* UDC Endpoint 8 Data Register */
-#define UDDR8		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0700)  
-/* UDC Endpoint 9 Data Register */
-#define UDDR9		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0900)  
-/* UDC Endpoint 10 Data Register */
-#define UDDR10		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x00C0)  
-/* UDC Endpoint 11 Data Register */
-#define UDDR11		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0B00)  
-/* UDC Endpoint 12 Data Register */
-#define UDDR12		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0B80)  
-/* UDC Endpoint 13 Data Register */
-#define UDDR13		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0C00)  
-/* UDC Endpoint 14 Data Register */
-#define UDDR14		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0E00)  
-/* UDC Endpoint 15 Data Register */
-#define UDDR15		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x00E0)  
-/* UDC Interrupt Control Register 0 */
-#define UICR0		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0050)  
-/* UDC Interrupt Control Register 1 */
-#define UICR1		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0054)  
-/* UDC Status Interrupt Register 0 */
-#define USIR0		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0058)  
-/* UDC Status Interrupt Register 1 */
-#define USIR1		IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x005C)  
-
-#define UDCCR_UDE	(1 << 0)	/* UDC enable */
-#define UDCCR_UDA	(1 << 1)	/* UDC active */
-#define UDCCR_RSM	(1 << 2)	/* Device resume */
-#define UDCCR_RESIR	(1 << 3)	/* Resume interrupt request */
-#define UDCCR_SUSIR	(1 << 4)	/* Suspend interrupt request */
-#define UDCCR_SRM	(1 << 5)	/* Suspend/resume interrupt mask */
-#define UDCCR_RSTIR	(1 << 6)	/* Reset interrupt request */
-#define UDCCR_REM	(1 << 7)	/* Reset interrupt mask */
-
-#define UDCCS0_OPR	(1 << 0)	/* OUT packet ready */
-#define UDCCS0_IPR	(1 << 1)	/* IN packet ready */
-#define UDCCS0_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS0_DRWF	(1 << 3)	/* Device remote wakeup feature */
-#define UDCCS0_SST	(1 << 4)	/* Sent stall */
-#define UDCCS0_FST	(1 << 5)	/* Force stall */
-#define UDCCS0_RNE	(1 << 6)	/* Receive FIFO no empty */
-#define UDCCS0_SA	(1 << 7)	/* Setup active */
-
-#define UDCCS_BI_TFS	(1 << 0)	/* Transmit FIFO service */
-#define UDCCS_BI_TPC	(1 << 1)	/* Transmit packet complete */
-#define UDCCS_BI_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS_BI_TUR	(1 << 3)	/* Transmit FIFO underrun */
-#define UDCCS_BI_SST	(1 << 4)	/* Sent stall */
-#define UDCCS_BI_FST	(1 << 5)	/* Force stall */
-#define UDCCS_BI_TSP	(1 << 7)	/* Transmit short packet */
-
-#define UDCCS_BO_RFS	(1 << 0)	/* Receive FIFO service */
-#define UDCCS_BO_RPC	(1 << 1)	/* Receive packet complete */
-#define UDCCS_BO_DME	(1 << 3)	/* DMA enable */
-#define UDCCS_BO_SST	(1 << 4)	/* Sent stall */
-#define UDCCS_BO_FST	(1 << 5)	/* Force stall */
-#define UDCCS_BO_RNE	(1 << 6)	/* Receive FIFO not empty */
-#define UDCCS_BO_RSP	(1 << 7)	/* Receive short packet */
-
-#define UDCCS_II_TFS	(1 << 0)	/* Transmit FIFO service */
-#define UDCCS_II_TPC	(1 << 1)	/* Transmit packet complete */
-#define UDCCS_II_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS_II_TUR	(1 << 3)	/* Transmit FIFO underrun */
-#define UDCCS_II_TSP	(1 << 7)	/* Transmit short packet */
-
-#define UDCCS_IO_RFS	(1 << 0)	/* Receive FIFO service */
-#define UDCCS_IO_RPC	(1 << 1)	/* Receive packet complete */
-#define UDCCS_IO_ROF	(1 << 3)	/* Receive overflow */
-#define UDCCS_IO_DME	(1 << 3)	/* DMA enable */
-#define UDCCS_IO_RNE	(1 << 6)	/* Receive FIFO not empty */
-#define UDCCS_IO_RSP	(1 << 7)	/* Receive short packet */
-
-#define UDCCS_INT_TFS	(1 << 0)	/* Transmit FIFO service */
-#define UDCCS_INT_TPC	(1 << 1)	/* Transmit packet complete */
-#define UDCCS_INT_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS_INT_TUR	(1 << 3)	/* Transmit FIFO underrun */
-#define UDCCS_INT_SST	(1 << 4)	/* Sent stall */
-#define UDCCS_INT_FST	(1 << 5)	/* Force stall */
-#define UDCCS_INT_TSP	(1 << 7)	/* Transmit short packet */
-
-#define UICR0_IM0	(1 << 0)	/* Interrupt mask ep 0 */
-#define UICR0_IM1	(1 << 1)	/* Interrupt mask ep 1 */
-#define UICR0_IM2	(1 << 2)	/* Interrupt mask ep 2 */
-#define UICR0_IM3	(1 << 3)	/* Interrupt mask ep 3 */
-#define UICR0_IM4	(1 << 4)	/* Interrupt mask ep 4 */
-#define UICR0_IM5	(1 << 5)	/* Interrupt mask ep 5 */
-#define UICR0_IM6	(1 << 6)	/* Interrupt mask ep 6 */
-#define UICR0_IM7	(1 << 7)	/* Interrupt mask ep 7 */
-
-#define UICR1_IM8	(1 << 0)	/* Interrupt mask ep 8 */
-#define UICR1_IM9	(1 << 1)	/* Interrupt mask ep 9 */
-#define UICR1_IM10	(1 << 2)	/* Interrupt mask ep 10 */
-#define UICR1_IM11	(1 << 3)	/* Interrupt mask ep 11 */
-#define UICR1_IM12	(1 << 4)	/* Interrupt mask ep 12 */
-#define UICR1_IM13	(1 << 5)	/* Interrupt mask ep 13 */
-#define UICR1_IM14	(1 << 6)	/* Interrupt mask ep 14 */
-#define UICR1_IM15	(1 << 7)	/* Interrupt mask ep 15 */
-
-#define USIR0_IR0	(1 << 0)	/* Interrupt request ep 0 */
-#define USIR0_IR1	(1 << 1)	/* Interrupt request ep 1 */
-#define USIR0_IR2	(1 << 2)	/* Interrupt request ep 2 */
-#define USIR0_IR3	(1 << 3)	/* Interrupt request ep 3 */
-#define USIR0_IR4	(1 << 4)	/* Interrupt request ep 4 */
-#define USIR0_IR5	(1 << 5)	/* Interrupt request ep 5 */
-#define USIR0_IR6	(1 << 6)	/* Interrupt request ep 6 */
-#define USIR0_IR7	(1 << 7)	/* Interrupt request ep 7 */
-
-#define USIR1_IR8	(1 << 0)	/* Interrupt request ep 8 */
-#define USIR1_IR9	(1 << 1)	/* Interrupt request ep 9 */
-#define USIR1_IR10	(1 << 2)	/* Interrupt request ep 10 */
-#define USIR1_IR11	(1 << 3)	/* Interrupt request ep 11 */
-#define USIR1_IR12	(1 << 4)	/* Interrupt request ep 12 */
-#define USIR1_IR13	(1 << 5)	/* Interrupt request ep 13 */
-#define USIR1_IR14	(1 << 6)	/* Interrupt request ep 14 */
-#define USIR1_IR15	(1 << 7)	/* Interrupt request ep 15 */
-
 #define DCMD_LENGTH	0x01fff		/* length mask (max = 8K - 1) */
 
 /* "fuse" bits of IXP_EXP_CFG2 */
diff --git a/arch/arm/mach-pxa/include/mach/pxa25x-udc.h b/arch/arm/mach-pxa/include/mach/pxa25x-udc.h
index 1b80a4805a60..e69de29bb2d1 100644
--- a/arch/arm/mach-pxa/include/mach/pxa25x-udc.h
+++ b/arch/arm/mach-pxa/include/mach/pxa25x-udc.h
@@ -1,163 +0,0 @@
-#ifndef _ASM_ARCH_PXA25X_UDC_H
-#define _ASM_ARCH_PXA25X_UDC_H
-
-#ifdef _ASM_ARCH_PXA27X_UDC_H
-#error "You can't include both PXA25x and PXA27x UDC support"
-#endif
-
-#define UDC_RES1	__REG(0x40600004)  /* UDC Undocumented - Reserved1 */
-#define UDC_RES2	__REG(0x40600008)  /* UDC Undocumented - Reserved2 */
-#define UDC_RES3	__REG(0x4060000C)  /* UDC Undocumented - Reserved3 */
-
-#define UDCCR		__REG(0x40600000)  /* UDC Control Register */
-#define UDCCR_UDE	(1 << 0)	/* UDC enable */
-#define UDCCR_UDA	(1 << 1)	/* UDC active */
-#define UDCCR_RSM	(1 << 2)	/* Device resume */
-#define UDCCR_RESIR	(1 << 3)	/* Resume interrupt request */
-#define UDCCR_SUSIR	(1 << 4)	/* Suspend interrupt request */
-#define UDCCR_SRM	(1 << 5)	/* Suspend/resume interrupt mask */
-#define UDCCR_RSTIR	(1 << 6)	/* Reset interrupt request */
-#define UDCCR_REM	(1 << 7)	/* Reset interrupt mask */
-
-#define UDCCS0		__REG(0x40600010)  /* UDC Endpoint 0 Control/Status Register */
-#define UDCCS0_OPR	(1 << 0)	/* OUT packet ready */
-#define UDCCS0_IPR	(1 << 1)	/* IN packet ready */
-#define UDCCS0_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS0_DRWF	(1 << 3)	/* Device remote wakeup feature */
-#define UDCCS0_SST	(1 << 4)	/* Sent stall */
-#define UDCCS0_FST	(1 << 5)	/* Force stall */
-#define UDCCS0_RNE	(1 << 6)	/* Receive FIFO no empty */
-#define UDCCS0_SA	(1 << 7)	/* Setup active */
-
-/* Bulk IN - Endpoint 1,6,11 */
-#define UDCCS1		__REG(0x40600014)  /* UDC Endpoint 1 (IN) Control/Status Register */
-#define UDCCS6		__REG(0x40600028)  /* UDC Endpoint 6 (IN) Control/Status Register */
-#define UDCCS11		__REG(0x4060003C)  /* UDC Endpoint 11 (IN) Control/Status Register */
-
-#define UDCCS_BI_TFS	(1 << 0)	/* Transmit FIFO service */
-#define UDCCS_BI_TPC	(1 << 1)	/* Transmit packet complete */
-#define UDCCS_BI_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS_BI_TUR	(1 << 3)	/* Transmit FIFO underrun */
-#define UDCCS_BI_SST	(1 << 4)	/* Sent stall */
-#define UDCCS_BI_FST	(1 << 5)	/* Force stall */
-#define UDCCS_BI_TSP	(1 << 7)	/* Transmit short packet */
-
-/* Bulk OUT - Endpoint 2,7,12 */
-#define UDCCS2		__REG(0x40600018)  /* UDC Endpoint 2 (OUT) Control/Status Register */
-#define UDCCS7		__REG(0x4060002C)  /* UDC Endpoint 7 (OUT) Control/Status Register */
-#define UDCCS12		__REG(0x40600040)  /* UDC Endpoint 12 (OUT) Control/Status Register */
-
-#define UDCCS_BO_RFS	(1 << 0)	/* Receive FIFO service */
-#define UDCCS_BO_RPC	(1 << 1)	/* Receive packet complete */
-#define UDCCS_BO_DME	(1 << 3)	/* DMA enable */
-#define UDCCS_BO_SST	(1 << 4)	/* Sent stall */
-#define UDCCS_BO_FST	(1 << 5)	/* Force stall */
-#define UDCCS_BO_RNE	(1 << 6)	/* Receive FIFO not empty */
-#define UDCCS_BO_RSP	(1 << 7)	/* Receive short packet */
-
-/* Isochronous IN - Endpoint 3,8,13 */
-#define UDCCS3		__REG(0x4060001C)  /* UDC Endpoint 3 (IN) Control/Status Register */
-#define UDCCS8		__REG(0x40600030)  /* UDC Endpoint 8 (IN) Control/Status Register */
-#define UDCCS13		__REG(0x40600044)  /* UDC Endpoint 13 (IN) Control/Status Register */
-
-#define UDCCS_II_TFS	(1 << 0)	/* Transmit FIFO service */
-#define UDCCS_II_TPC	(1 << 1)	/* Transmit packet complete */
-#define UDCCS_II_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS_II_TUR	(1 << 3)	/* Transmit FIFO underrun */
-#define UDCCS_II_TSP	(1 << 7)	/* Transmit short packet */
-
-/* Isochronous OUT - Endpoint 4,9,14 */
-#define UDCCS4		__REG(0x40600020)  /* UDC Endpoint 4 (OUT) Control/Status Register */
-#define UDCCS9		__REG(0x40600034)  /* UDC Endpoint 9 (OUT) Control/Status Register */
-#define UDCCS14		__REG(0x40600048)  /* UDC Endpoint 14 (OUT) Control/Status Register */
-
-#define UDCCS_IO_RFS	(1 << 0)	/* Receive FIFO service */
-#define UDCCS_IO_RPC	(1 << 1)	/* Receive packet complete */
-#define UDCCS_IO_ROF	(1 << 2)	/* Receive overflow */
-#define UDCCS_IO_DME	(1 << 3)	/* DMA enable */
-#define UDCCS_IO_RNE	(1 << 6)	/* Receive FIFO not empty */
-#define UDCCS_IO_RSP	(1 << 7)	/* Receive short packet */
-
-/* Interrupt IN - Endpoint 5,10,15 */
-#define UDCCS5		__REG(0x40600024)  /* UDC Endpoint 5 (Interrupt) Control/Status Register */
-#define UDCCS10		__REG(0x40600038)  /* UDC Endpoint 10 (Interrupt) Control/Status Register */
-#define UDCCS15		__REG(0x4060004C)  /* UDC Endpoint 15 (Interrupt) Control/Status Register */
-
-#define UDCCS_INT_TFS	(1 << 0)	/* Transmit FIFO service */
-#define UDCCS_INT_TPC	(1 << 1)	/* Transmit packet complete */
-#define UDCCS_INT_FTF	(1 << 2)	/* Flush Tx FIFO */
-#define UDCCS_INT_TUR	(1 << 3)	/* Transmit FIFO underrun */
-#define UDCCS_INT_SST	(1 << 4)	/* Sent stall */
-#define UDCCS_INT_FST	(1 << 5)	/* Force stall */
-#define UDCCS_INT_TSP	(1 << 7)	/* Transmit short packet */
-
-#define UFNRH		__REG(0x40600060)  /* UDC Frame Number Register High */
-#define UFNRL		__REG(0x40600064)  /* UDC Frame Number Register Low */
-#define UBCR2		__REG(0x40600068)  /* UDC Byte Count Reg 2 */
-#define UBCR4		__REG(0x4060006c)  /* UDC Byte Count Reg 4 */
-#define UBCR7		__REG(0x40600070)  /* UDC Byte Count Reg 7 */
-#define UBCR9		__REG(0x40600074)  /* UDC Byte Count Reg 9 */
-#define UBCR12		__REG(0x40600078)  /* UDC Byte Count Reg 12 */
-#define UBCR14		__REG(0x4060007c)  /* UDC Byte Count Reg 14 */
-#define UDDR0		__REG(0x40600080)  /* UDC Endpoint 0 Data Register */
-#define UDDR1		__REG(0x40600100)  /* UDC Endpoint 1 Data Register */
-#define UDDR2		__REG(0x40600180)  /* UDC Endpoint 2 Data Register */
-#define UDDR3		__REG(0x40600200)  /* UDC Endpoint 3 Data Register */
-#define UDDR4		__REG(0x40600400)  /* UDC Endpoint 4 Data Register */
-#define UDDR5		__REG(0x406000A0)  /* UDC Endpoint 5 Data Register */
-#define UDDR6		__REG(0x40600600)  /* UDC Endpoint 6 Data Register */
-#define UDDR7		__REG(0x40600680)  /* UDC Endpoint 7 Data Register */
-#define UDDR8		__REG(0x40600700)  /* UDC Endpoint 8 Data Register */
-#define UDDR9		__REG(0x40600900)  /* UDC Endpoint 9 Data Register */
-#define UDDR10		__REG(0x406000C0)  /* UDC Endpoint 10 Data Register */
-#define UDDR11		__REG(0x40600B00)  /* UDC Endpoint 11 Data Register */
-#define UDDR12		__REG(0x40600B80)  /* UDC Endpoint 12 Data Register */
-#define UDDR13		__REG(0x40600C00)  /* UDC Endpoint 13 Data Register */
-#define UDDR14		__REG(0x40600E00)  /* UDC Endpoint 14 Data Register */
-#define UDDR15		__REG(0x406000E0)  /* UDC Endpoint 15 Data Register */
-
-#define UICR0		__REG(0x40600050)  /* UDC Interrupt Control Register 0 */
-
-#define UICR0_IM0	(1 << 0)	/* Interrupt mask ep 0 */
-#define UICR0_IM1	(1 << 1)	/* Interrupt mask ep 1 */
-#define UICR0_IM2	(1 << 2)	/* Interrupt mask ep 2 */
-#define UICR0_IM3	(1 << 3)	/* Interrupt mask ep 3 */
-#define UICR0_IM4	(1 << 4)	/* Interrupt mask ep 4 */
-#define UICR0_IM5	(1 << 5)	/* Interrupt mask ep 5 */
-#define UICR0_IM6	(1 << 6)	/* Interrupt mask ep 6 */
-#define UICR0_IM7	(1 << 7)	/* Interrupt mask ep 7 */
-
-#define UICR1		__REG(0x40600054)  /* UDC Interrupt Control Register 1 */
-
-#define UICR1_IM8	(1 << 0)	/* Interrupt mask ep 8 */
-#define UICR1_IM9	(1 << 1)	/* Interrupt mask ep 9 */
-#define UICR1_IM10	(1 << 2)	/* Interrupt mask ep 10 */
-#define UICR1_IM11	(1 << 3)	/* Interrupt mask ep 11 */
-#define UICR1_IM12	(1 << 4)	/* Interrupt mask ep 12 */
-#define UICR1_IM13	(1 << 5)	/* Interrupt mask ep 13 */
-#define UICR1_IM14	(1 << 6)	/* Interrupt mask ep 14 */
-#define UICR1_IM15	(1 << 7)	/* Interrupt mask ep 15 */
-
-#define USIR0		__REG(0x40600058)  /* UDC Status Interrupt Register 0 */
-
-#define USIR0_IR0	(1 << 0)	/* Interrupt request ep 0 */
-#define USIR0_IR1	(1 << 1)	/* Interrupt request ep 1 */
-#define USIR0_IR2	(1 << 2)	/* Interrupt request ep 2 */
-#define USIR0_IR3	(1 << 3)	/* Interrupt request ep 3 */
-#define USIR0_IR4	(1 << 4)	/* Interrupt request ep 4 */
-#define USIR0_IR5	(1 << 5)	/* Interrupt request ep 5 */
-#define USIR0_IR6	(1 << 6)	/* Interrupt request ep 6 */
-#define USIR0_IR7	(1 << 7)	/* Interrupt request ep 7 */
-
-#define USIR1		__REG(0x4060005C)  /* UDC Status Interrupt Register 1 */
-
-#define USIR1_IR8	(1 << 0)	/* Interrupt request ep 8 */
-#define USIR1_IR9	(1 << 1)	/* Interrupt request ep 9 */
-#define USIR1_IR10	(1 << 2)	/* Interrupt request ep 10 */
-#define USIR1_IR11	(1 << 3)	/* Interrupt request ep 11 */
-#define USIR1_IR12	(1 << 4)	/* Interrupt request ep 12 */
-#define USIR1_IR13	(1 << 5)	/* Interrupt request ep 13 */
-#define USIR1_IR14	(1 << 6)	/* Interrupt request ep 14 */
-#define USIR1_IR15	(1 << 7)	/* Interrupt request ep 15 */
-
-#endif
diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c b/drivers/usb/gadget/udc/pxa25x_udc.c
index b82cb14850b6..26537f50374d 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.c
+++ b/drivers/usb/gadget/udc/pxa25x_udc.c
@@ -48,18 +48,167 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
 
-/*
- * This driver is PXA25x only.  Grab the right register definitions.
- */
-#ifdef CONFIG_ARCH_PXA
-#include <mach/pxa25x-udc.h>
 #include <mach/hardware.h>
-#endif
 
 #ifdef CONFIG_ARCH_LUBBOCK
 #include <mach/lubbock.h>
 #endif
 
+#ifdef CONFIG_ARCH_IXP4XX
+#define __UDC_REG(x) (*((volatile u32 *)(IXP4XX_USB_BASE_VIRT + (x))))
+#endif
+
+#ifdef CONFIG_ARCH_PXA
+#define __UDC_REG(x) __REG(0x40600000 + (x))
+#endif
+
+#define UDCCR		__UDC_REG(0x0000)  /* UDC Control Register */
+#define UDC_RES1	__UDC_REG(0x0004)  /* UDC Undocumented - Reserved1 */
+#define UDC_RES2	__UDC_REG(0x0008)  /* UDC Undocumented - Reserved2 */
+#define UDC_RES3	__UDC_REG(0x000C)  /* UDC Undocumented - Reserved3 */
+#define UDCCS0		__UDC_REG(0x0010)  /* UDC Endpoint 0 Control/Status Register */
+#define UDCCS1		__UDC_REG(0x0014)  /* UDC Endpoint 1 (IN) Control/Status Register */
+#define UDCCS2		__UDC_REG(0x0018)  /* UDC Endpoint 2 (OUT) Control/Status Register */
+#define UDCCS3		__UDC_REG(0x001C)  /* UDC Endpoint 3 (IN) Control/Status Register */
+#define UDCCS4		__UDC_REG(0x0020)  /* UDC Endpoint 4 (OUT) Control/Status Register */
+#define UDCCS5		__UDC_REG(0x0024)  /* UDC Endpoint 5 (Interrupt) Control/Status Register */
+#define UDCCS6		__UDC_REG(0x0028)  /* UDC Endpoint 6 (IN) Control/Status Register */
+#define UDCCS7		__UDC_REG(0x002C)  /* UDC Endpoint 7 (OUT) Control/Status Register */
+#define UDCCS8		__UDC_REG(0x0030)  /* UDC Endpoint 8 (IN) Control/Status Register */
+#define UDCCS9		__UDC_REG(0x0034)  /* UDC Endpoint 9 (OUT) Control/Status Register */
+#define UDCCS10		__UDC_REG(0x0038)  /* UDC Endpoint 10 (Interrupt) Control/Status Register */
+#define UDCCS11		__UDC_REG(0x003C)  /* UDC Endpoint 11 (IN) Control/Status Register */
+#define UDCCS12		__UDC_REG(0x0040)  /* UDC Endpoint 12 (OUT) Control/Status Register */
+#define UDCCS13		__UDC_REG(0x0044)  /* UDC Endpoint 13 (IN) Control/Status Register */
+#define UDCCS14		__UDC_REG(0x0048)  /* UDC Endpoint 14 (OUT) Control/Status Register */
+#define UDCCS15		__UDC_REG(0x004C)  /* UDC Endpoint 15 (Interrupt) Control/Status Register */
+#define UFNRH		__UDC_REG(0x0060)  /* UDC Frame Number Register High */
+#define UFNRL		__UDC_REG(0x0064)  /* UDC Frame Number Register Low */
+#define UBCR2		__UDC_REG(0x0068)  /* UDC Byte Count Reg 2 */
+#define UBCR4		__UDC_REG(0x006c)  /* UDC Byte Count Reg 4 */
+#define UBCR7		__UDC_REG(0x0070)  /* UDC Byte Count Reg 7 */
+#define UBCR9		__UDC_REG(0x0074)  /* UDC Byte Count Reg 9 */
+#define UBCR12		__UDC_REG(0x0078)  /* UDC Byte Count Reg 12 */
+#define UBCR14		__UDC_REG(0x007c)  /* UDC Byte Count Reg 14 */
+#define UDDR0		__UDC_REG(0x0080)  /* UDC Endpoint 0 Data Register */
+#define UDDR1		__UDC_REG(0x0100)  /* UDC Endpoint 1 Data Register */
+#define UDDR2		__UDC_REG(0x0180)  /* UDC Endpoint 2 Data Register */
+#define UDDR3		__UDC_REG(0x0200)  /* UDC Endpoint 3 Data Register */
+#define UDDR4		__UDC_REG(0x0400)  /* UDC Endpoint 4 Data Register */
+#define UDDR5		__UDC_REG(0x00A0)  /* UDC Endpoint 5 Data Register */
+#define UDDR6		__UDC_REG(0x0600)  /* UDC Endpoint 6 Data Register */
+#define UDDR7		__UDC_REG(0x0680)  /* UDC Endpoint 7 Data Register */
+#define UDDR8		__UDC_REG(0x0700)  /* UDC Endpoint 8 Data Register */
+#define UDDR9		__UDC_REG(0x0900)  /* UDC Endpoint 9 Data Register */
+#define UDDR10		__UDC_REG(0x00C0)  /* UDC Endpoint 10 Data Register */
+#define UDDR11		__UDC_REG(0x0B00)  /* UDC Endpoint 11 Data Register */
+#define UDDR12		__UDC_REG(0x0B80)  /* UDC Endpoint 12 Data Register */
+#define UDDR13		__UDC_REG(0x0C00)  /* UDC Endpoint 13 Data Register */
+#define UDDR14		__UDC_REG(0x0E00)  /* UDC Endpoint 14 Data Register */
+#define UDDR15		__UDC_REG(0x00E0)  /* UDC Endpoint 15 Data Register */
+
+#define UICR0		__UDC_REG(0x0050)  /* UDC Interrupt Control Register 0 */
+#define UICR1		__UDC_REG(0x0054)  /* UDC Interrupt Control Register 1 */
+
+#define USIR0		__UDC_REG(0x0058)  /* UDC Status Interrupt Register 0 */
+#define USIR1		__UDC_REG(0x005C)  /* UDC Status Interrupt Register 1 */
+
+#define UDCCR_UDE	(1 << 0)	/* UDC enable */
+#define UDCCR_UDA	(1 << 1)	/* UDC active */
+#define UDCCR_RSM	(1 << 2)	/* Device resume */
+#define UDCCR_RESIR	(1 << 3)	/* Resume interrupt request */
+#define UDCCR_SUSIR	(1 << 4)	/* Suspend interrupt request */
+#define UDCCR_SRM	(1 << 5)	/* Suspend/resume interrupt mask */
+#define UDCCR_RSTIR	(1 << 6)	/* Reset interrupt request */
+#define UDCCR_REM	(1 << 7)	/* Reset interrupt mask */
+
+#define UDCCS0_OPR	(1 << 0)	/* OUT packet ready */
+#define UDCCS0_IPR	(1 << 1)	/* IN packet ready */
+#define UDCCS0_FTF	(1 << 2)	/* Flush Tx FIFO */
+#define UDCCS0_DRWF	(1 << 3)	/* Device remote wakeup feature */
+#define UDCCS0_SST	(1 << 4)	/* Sent stall */
+#define UDCCS0_FST	(1 << 5)	/* Force stall */
+#define UDCCS0_RNE	(1 << 6)	/* Receive FIFO no empty */
+#define UDCCS0_SA	(1 << 7)	/* Setup active */
+
+#define UDCCS_BI_TFS	(1 << 0)	/* Transmit FIFO service */
+#define UDCCS_BI_TPC	(1 << 1)	/* Transmit packet complete */
+#define UDCCS_BI_FTF	(1 << 2)	/* Flush Tx FIFO */
+#define UDCCS_BI_TUR	(1 << 3)	/* Transmit FIFO underrun */
+#define UDCCS_BI_SST	(1 << 4)	/* Sent stall */
+#define UDCCS_BI_FST	(1 << 5)	/* Force stall */
+#define UDCCS_BI_TSP	(1 << 7)	/* Transmit short packet */
+
+#define UDCCS_BO_RFS	(1 << 0)	/* Receive FIFO service */
+#define UDCCS_BO_RPC	(1 << 1)	/* Receive packet complete */
+#define UDCCS_BO_DME	(1 << 3)	/* DMA enable */
+#define UDCCS_BO_SST	(1 << 4)	/* Sent stall */
+#define UDCCS_BO_FST	(1 << 5)	/* Force stall */
+#define UDCCS_BO_RNE	(1 << 6)	/* Receive FIFO not empty */
+#define UDCCS_BO_RSP	(1 << 7)	/* Receive short packet */
+
+#define UDCCS_II_TFS	(1 << 0)	/* Transmit FIFO service */
+#define UDCCS_II_TPC	(1 << 1)	/* Transmit packet complete */
+#define UDCCS_II_FTF	(1 << 2)	/* Flush Tx FIFO */
+#define UDCCS_II_TUR	(1 << 3)	/* Transmit FIFO underrun */
+#define UDCCS_II_TSP	(1 << 7)	/* Transmit short packet */
+
+#define UDCCS_IO_RFS	(1 << 0)	/* Receive FIFO service */
+#define UDCCS_IO_RPC	(1 << 1)	/* Receive packet complete */
+#ifdef CONFIG_ARCH_IXP4XX /* FIXME: is this right?, datasheed says '2' */
+#define UDCCS_IO_ROF	(1 << 3)	/* Receive overflow */
+#endif
+#ifdef CONFIG_ARCH_PXA
+#define UDCCS_IO_ROF	(1 << 2)	/* Receive overflow */
+#endif
+#define UDCCS_IO_DME	(1 << 3)	/* DMA enable */
+#define UDCCS_IO_RNE	(1 << 6)	/* Receive FIFO not empty */
+#define UDCCS_IO_RSP	(1 << 7)	/* Receive short packet */
+
+#define UDCCS_INT_TFS	(1 << 0)	/* Transmit FIFO service */
+#define UDCCS_INT_TPC	(1 << 1)	/* Transmit packet complete */
+#define UDCCS_INT_FTF	(1 << 2)	/* Flush Tx FIFO */
+#define UDCCS_INT_TUR	(1 << 3)	/* Transmit FIFO underrun */
+#define UDCCS_INT_SST	(1 << 4)	/* Sent stall */
+#define UDCCS_INT_FST	(1 << 5)	/* Force stall */
+#define UDCCS_INT_TSP	(1 << 7)	/* Transmit short packet */
+
+#define UICR0_IM0	(1 << 0)	/* Interrupt mask ep 0 */
+#define UICR0_IM1	(1 << 1)	/* Interrupt mask ep 1 */
+#define UICR0_IM2	(1 << 2)	/* Interrupt mask ep 2 */
+#define UICR0_IM3	(1 << 3)	/* Interrupt mask ep 3 */
+#define UICR0_IM4	(1 << 4)	/* Interrupt mask ep 4 */
+#define UICR0_IM5	(1 << 5)	/* Interrupt mask ep 5 */
+#define UICR0_IM6	(1 << 6)	/* Interrupt mask ep 6 */
+#define UICR0_IM7	(1 << 7)	/* Interrupt mask ep 7 */
+
+#define UICR1_IM8	(1 << 0)	/* Interrupt mask ep 8 */
+#define UICR1_IM9	(1 << 1)	/* Interrupt mask ep 9 */
+#define UICR1_IM10	(1 << 2)	/* Interrupt mask ep 10 */
+#define UICR1_IM11	(1 << 3)	/* Interrupt mask ep 11 */
+#define UICR1_IM12	(1 << 4)	/* Interrupt mask ep 12 */
+#define UICR1_IM13	(1 << 5)	/* Interrupt mask ep 13 */
+#define UICR1_IM14	(1 << 6)	/* Interrupt mask ep 14 */
+#define UICR1_IM15	(1 << 7)	/* Interrupt mask ep 15 */
+
+#define USIR0_IR0	(1 << 0)	/* Interrupt request ep 0 */
+#define USIR0_IR1	(1 << 1)	/* Interrupt request ep 1 */
+#define USIR0_IR2	(1 << 2)	/* Interrupt request ep 2 */
+#define USIR0_IR3	(1 << 3)	/* Interrupt request ep 3 */
+#define USIR0_IR4	(1 << 4)	/* Interrupt request ep 4 */
+#define USIR0_IR5	(1 << 5)	/* Interrupt request ep 5 */
+#define USIR0_IR6	(1 << 6)	/* Interrupt request ep 6 */
+#define USIR0_IR7	(1 << 7)	/* Interrupt request ep 7 */
+
+#define USIR1_IR8	(1 << 0)	/* Interrupt request ep 8 */
+#define USIR1_IR9	(1 << 1)	/* Interrupt request ep 9 */
+#define USIR1_IR10	(1 << 2)	/* Interrupt request ep 10 */
+#define USIR1_IR11	(1 << 3)	/* Interrupt request ep 11 */
+#define USIR1_IR12	(1 << 4)	/* Interrupt request ep 12 */
+#define USIR1_IR13	(1 << 5)	/* Interrupt request ep 13 */
+#define USIR1_IR14	(1 << 6)	/* Interrupt request ep 14 */
+#define USIR1_IR15	(1 << 7)	/* Interrupt request ep 15 */
+
 /*
  * This driver handles the USB Device Controller (UDC) in Intel's PXA 25x
  * series processors.  The UDC for the IXP 4xx series is very similar.
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/7] usb: gadget: pxa25x_udc cleanup
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
@ 2016-01-28 16:17   ` Arnd Bergmann
  2016-01-29 10:13     ` Robert Jarzmik
  2016-01-28 16:17   ` [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:17 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman

This removes the dependency on the mach/hardware.h header file
from the pxa25x_udc driver after the register definitions were
already unified in the previous patch.

Following the model of pxa27x_udc (and basically all other drivers
in the kernel), we define the register numbers as offsets from
the register base address and use accessor functions to read/write
them.

For the moment, this still leaves the direct pointer dereference
in place, instead of using readl/writel, so this patch should
not be changing the behavior of the driver, other than using
ioremap() on the platform resource to replace the hardcoded
virtual address pointers.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/gadget/udc/pxa25x_udc.c | 468 +++++++++++++++++++++---------------
 drivers/usb/gadget/udc/pxa25x_udc.h |  10 +-
 2 files changed, 275 insertions(+), 203 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c b/drivers/usb/gadget/udc/pxa25x_udc.c
index 26537f50374d..5db429f3cfb2 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.c
+++ b/drivers/usb/gadget/udc/pxa25x_udc.c
@@ -48,70 +48,63 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
 
-#include <mach/hardware.h>
-
 #ifdef CONFIG_ARCH_LUBBOCK
 #include <mach/lubbock.h>
 #endif
 
-#ifdef CONFIG_ARCH_IXP4XX
-#define __UDC_REG(x) (*((volatile u32 *)(IXP4XX_USB_BASE_VIRT + (x))))
-#endif
-
-#ifdef CONFIG_ARCH_PXA
-#define __UDC_REG(x) __REG(0x40600000 + (x))
-#endif
-
-#define UDCCR		__UDC_REG(0x0000)  /* UDC Control Register */
-#define UDC_RES1	__UDC_REG(0x0004)  /* UDC Undocumented - Reserved1 */
-#define UDC_RES2	__UDC_REG(0x0008)  /* UDC Undocumented - Reserved2 */
-#define UDC_RES3	__UDC_REG(0x000C)  /* UDC Undocumented - Reserved3 */
-#define UDCCS0		__UDC_REG(0x0010)  /* UDC Endpoint 0 Control/Status Register */
-#define UDCCS1		__UDC_REG(0x0014)  /* UDC Endpoint 1 (IN) Control/Status Register */
-#define UDCCS2		__UDC_REG(0x0018)  /* UDC Endpoint 2 (OUT) Control/Status Register */
-#define UDCCS3		__UDC_REG(0x001C)  /* UDC Endpoint 3 (IN) Control/Status Register */
-#define UDCCS4		__UDC_REG(0x0020)  /* UDC Endpoint 4 (OUT) Control/Status Register */
-#define UDCCS5		__UDC_REG(0x0024)  /* UDC Endpoint 5 (Interrupt) Control/Status Register */
-#define UDCCS6		__UDC_REG(0x0028)  /* UDC Endpoint 6 (IN) Control/Status Register */
-#define UDCCS7		__UDC_REG(0x002C)  /* UDC Endpoint 7 (OUT) Control/Status Register */
-#define UDCCS8		__UDC_REG(0x0030)  /* UDC Endpoint 8 (IN) Control/Status Register */
-#define UDCCS9		__UDC_REG(0x0034)  /* UDC Endpoint 9 (OUT) Control/Status Register */
-#define UDCCS10		__UDC_REG(0x0038)  /* UDC Endpoint 10 (Interrupt) Control/Status Register */
-#define UDCCS11		__UDC_REG(0x003C)  /* UDC Endpoint 11 (IN) Control/Status Register */
-#define UDCCS12		__UDC_REG(0x0040)  /* UDC Endpoint 12 (OUT) Control/Status Register */
-#define UDCCS13		__UDC_REG(0x0044)  /* UDC Endpoint 13 (IN) Control/Status Register */
-#define UDCCS14		__UDC_REG(0x0048)  /* UDC Endpoint 14 (OUT) Control/Status Register */
-#define UDCCS15		__UDC_REG(0x004C)  /* UDC Endpoint 15 (Interrupt) Control/Status Register */
-#define UFNRH		__UDC_REG(0x0060)  /* UDC Frame Number Register High */
-#define UFNRL		__UDC_REG(0x0064)  /* UDC Frame Number Register Low */
-#define UBCR2		__UDC_REG(0x0068)  /* UDC Byte Count Reg 2 */
-#define UBCR4		__UDC_REG(0x006c)  /* UDC Byte Count Reg 4 */
-#define UBCR7		__UDC_REG(0x0070)  /* UDC Byte Count Reg 7 */
-#define UBCR9		__UDC_REG(0x0074)  /* UDC Byte Count Reg 9 */
-#define UBCR12		__UDC_REG(0x0078)  /* UDC Byte Count Reg 12 */
-#define UBCR14		__UDC_REG(0x007c)  /* UDC Byte Count Reg 14 */
-#define UDDR0		__UDC_REG(0x0080)  /* UDC Endpoint 0 Data Register */
-#define UDDR1		__UDC_REG(0x0100)  /* UDC Endpoint 1 Data Register */
-#define UDDR2		__UDC_REG(0x0180)  /* UDC Endpoint 2 Data Register */
-#define UDDR3		__UDC_REG(0x0200)  /* UDC Endpoint 3 Data Register */
-#define UDDR4		__UDC_REG(0x0400)  /* UDC Endpoint 4 Data Register */
-#define UDDR5		__UDC_REG(0x00A0)  /* UDC Endpoint 5 Data Register */
-#define UDDR6		__UDC_REG(0x0600)  /* UDC Endpoint 6 Data Register */
-#define UDDR7		__UDC_REG(0x0680)  /* UDC Endpoint 7 Data Register */
-#define UDDR8		__UDC_REG(0x0700)  /* UDC Endpoint 8 Data Register */
-#define UDDR9		__UDC_REG(0x0900)  /* UDC Endpoint 9 Data Register */
-#define UDDR10		__UDC_REG(0x00C0)  /* UDC Endpoint 10 Data Register */
-#define UDDR11		__UDC_REG(0x0B00)  /* UDC Endpoint 11 Data Register */
-#define UDDR12		__UDC_REG(0x0B80)  /* UDC Endpoint 12 Data Register */
-#define UDDR13		__UDC_REG(0x0C00)  /* UDC Endpoint 13 Data Register */
-#define UDDR14		__UDC_REG(0x0E00)  /* UDC Endpoint 14 Data Register */
-#define UDDR15		__UDC_REG(0x00E0)  /* UDC Endpoint 15 Data Register */
-
-#define UICR0		__UDC_REG(0x0050)  /* UDC Interrupt Control Register 0 */
-#define UICR1		__UDC_REG(0x0054)  /* UDC Interrupt Control Register 1 */
-
-#define USIR0		__UDC_REG(0x0058)  /* UDC Status Interrupt Register 0 */
-#define USIR1		__UDC_REG(0x005C)  /* UDC Status Interrupt Register 1 */
+static void __iomem *pxa25x_udc_reg_base;
+#define __UDC_REG(x) (*((volatile u32 *)(pxa25x_udc_reg_base + (x))))
+
+#define UDCCR	 0x0000 /* UDC Control Register */
+#define UDC_RES1 0x0004 /* UDC Undocumented - Reserved1 */
+#define UDC_RES2 0x0008 /* UDC Undocumented - Reserved2 */
+#define UDC_RES3 0x000C /* UDC Undocumented - Reserved3 */
+#define UDCCS0	 0x0010 /* UDC Endpoint 0 Control/Status Register */
+#define UDCCS1	 0x0014 /* UDC Endpoint 1 (IN) Control/Status Register */
+#define UDCCS2	 0x0018 /* UDC Endpoint 2 (OUT) Control/Status Register */
+#define UDCCS3	 0x001C /* UDC Endpoint 3 (IN) Control/Status Register */
+#define UDCCS4	 0x0020 /* UDC Endpoint 4 (OUT) Control/Status Register */
+#define UDCCS5	 0x0024 /* UDC Endpoint 5 (Interrupt) Control/Status Register */
+#define UDCCS6	 0x0028 /* UDC Endpoint 6 (IN) Control/Status Register */
+#define UDCCS7	 0x002C /* UDC Endpoint 7 (OUT) Control/Status Register */
+#define UDCCS8	 0x0030 /* UDC Endpoint 8 (IN) Control/Status Register */
+#define UDCCS9	 0x0034 /* UDC Endpoint 9 (OUT) Control/Status Register */
+#define UDCCS10	 0x0038 /* UDC Endpoint 10 (Interrupt) Control/Status Register */
+#define UDCCS11	 0x003C /* UDC Endpoint 11 (IN) Control/Status Register */
+#define UDCCS12	 0x0040 /* UDC Endpoint 12 (OUT) Control/Status Register */
+#define UDCCS13	 0x0044 /* UDC Endpoint 13 (IN) Control/Status Register */
+#define UDCCS14	 0x0048 /* UDC Endpoint 14 (OUT) Control/Status Register */
+#define UDCCS15	 0x004C /* UDC Endpoint 15 (Interrupt) Control/Status Register */
+#define UFNRH	 0x0060 /* UDC Frame Number Register High */
+#define UFNRL	 0x0064 /* UDC Frame Number Register Low */
+#define UBCR2	 0x0068 /* UDC Byte Count Reg 2 */
+#define UBCR4	 0x006c /* UDC Byte Count Reg 4 */
+#define UBCR7	 0x0070 /* UDC Byte Count Reg 7 */
+#define UBCR9	 0x0074 /* UDC Byte Count Reg 9 */
+#define UBCR12	 0x0078 /* UDC Byte Count Reg 12 */
+#define UBCR14	 0x007c /* UDC Byte Count Reg 14 */
+#define UDDR0	 0x0080 /* UDC Endpoint 0 Data Register */
+#define UDDR1	 0x0100 /* UDC Endpoint 1 Data Register */
+#define UDDR2	 0x0180 /* UDC Endpoint 2 Data Register */
+#define UDDR3	 0x0200 /* UDC Endpoint 3 Data Register */
+#define UDDR4	 0x0400 /* UDC Endpoint 4 Data Register */
+#define UDDR5	 0x00A0 /* UDC Endpoint 5 Data Register */
+#define UDDR6	 0x0600 /* UDC Endpoint 6 Data Register */
+#define UDDR7	 0x0680 /* UDC Endpoint 7 Data Register */
+#define UDDR8	 0x0700 /* UDC Endpoint 8 Data Register */
+#define UDDR9	 0x0900 /* UDC Endpoint 9 Data Register */
+#define UDDR10	 0x00C0 /* UDC Endpoint 10 Data Register */
+#define UDDR11	 0x0B00 /* UDC Endpoint 11 Data Register */
+#define UDDR12	 0x0B80 /* UDC Endpoint 12 Data Register */
+#define UDDR13	 0x0C00 /* UDC Endpoint 13 Data Register */
+#define UDDR14	 0x0E00 /* UDC Endpoint 14 Data Register */
+#define UDDR15	 0x00E0 /* UDC Endpoint 15 Data Register */
+
+#define UICR0	 0x0050 /* UDC Interrupt Control Register 0 */
+#define UICR1	 0x0054 /* UDC Interrupt Control Register 1 */
+
+#define USIR0	 0x0058 /* UDC Status Interrupt Register 0 */
+#define USIR1	 0x005C /* UDC Status Interrupt Register 1 */
 
 #define UDCCR_UDE	(1 << 0)	/* UDC enable */
 #define UDCCR_UDA	(1 << 1)	/* UDC active */
@@ -299,25 +292,41 @@ static void pullup_on(void)
 		mach->udc_command(PXA2XX_UDC_CMD_CONNECT);
 }
 
-static void pio_irq_enable(int bEndpointAddress)
+static inline void udc_set_reg(struct pxa25x_udc *dev, u32 reg, u32 uicr)
+{
+	__UDC_REG(reg) = uicr;
+}
+
+static inline u32 udc_get_reg(struct pxa25x_udc *dev, u32 reg)
 {
-        bEndpointAddress &= 0xf;
+	return __UDC_REG(reg);
+}
+
+static void pio_irq_enable(struct pxa25x_ep *ep)
+{
+	u32 bEndpointAddress = ep->bEndpointAddress & 0xf;
+
         if (bEndpointAddress < 8)
-                UICR0 &= ~(1 << bEndpointAddress);
+		udc_set_reg(ep->dev, UICR0, udc_get_reg(ep->dev, UICR0) &
+						~(1 << bEndpointAddress));
         else {
                 bEndpointAddress -= 8;
-                UICR1 &= ~(1 << bEndpointAddress);
+		udc_set_reg(ep->dev, UICR1, udc_get_reg(ep->dev, UICR1) &
+						~(1 << bEndpointAddress));
 	}
 }
 
-static void pio_irq_disable(int bEndpointAddress)
+static void pio_irq_disable(struct pxa25x_ep *ep)
 {
-        bEndpointAddress &= 0xf;
+	u32 bEndpointAddress = ep->bEndpointAddress & 0xf;
+
         if (bEndpointAddress < 8)
-                UICR0 |= 1 << bEndpointAddress;
+                udc_set_reg(ep->dev, UICR0, udc_get_reg(ep->dev, UICR0) |
+						(1 << bEndpointAddress));
         else {
                 bEndpointAddress -= 8;
-                UICR1 |= 1 << bEndpointAddress;
+                udc_set_reg(ep->dev, UICR1, udc_get_reg(ep->dev, UICR1) |
+						(1 << bEndpointAddress));
         }
 }
 
@@ -326,22 +335,61 @@ static void pio_irq_disable(int bEndpointAddress)
  */
 #define UDCCR_MASK_BITS         (UDCCR_REM | UDCCR_SRM | UDCCR_UDE)
 
-static inline void udc_set_mask_UDCCR(int mask)
+static inline void udc_set_mask_UDCCR(struct pxa25x_udc *dev, int mask)
 {
-	UDCCR = (UDCCR & UDCCR_MASK_BITS) | (mask & UDCCR_MASK_BITS);
+	u32 udccr = __UDC_REG(UDCCR);
+
+	__UDC_REG(UDCCR) = (udccr & UDCCR_MASK_BITS) | (mask & UDCCR_MASK_BITS);
 }
 
-static inline void udc_clear_mask_UDCCR(int mask)
+static inline void udc_clear_mask_UDCCR(struct pxa25x_udc *dev, int mask)
 {
-	UDCCR = (UDCCR & UDCCR_MASK_BITS) & ~(mask & UDCCR_MASK_BITS);
+	u32 udccr = __UDC_REG(UDCCR);
+
+	__UDC_REG(UDCCR) = (udccr & UDCCR_MASK_BITS) & ~(mask & UDCCR_MASK_BITS);
 }
 
-static inline void udc_ack_int_UDCCR(int mask)
+static inline void udc_ack_int_UDCCR(struct pxa25x_udc *dev, int mask)
 {
 	/* udccr contains the bits we dont want to change */
-	__u32 udccr = UDCCR & UDCCR_MASK_BITS;
+	u32 udccr = __UDC_REG(UDCCR) & UDCCR_MASK_BITS;
 
-	UDCCR = udccr | (mask & ~UDCCR_MASK_BITS);
+	__UDC_REG(UDCCR) = udccr | (mask & ~UDCCR_MASK_BITS);
+}
+
+static inline u32 udc_ep_get_UDCCS(struct pxa25x_ep *ep)
+{
+	return __UDC_REG(ep->regoff_udccs);
+}
+
+static inline void udc_ep_set_UDCCS(struct pxa25x_ep *ep, u32 data)
+{
+	__UDC_REG(ep->regoff_udccs) = data;
+}
+
+static inline u32 udc_ep0_get_UDCCS(struct pxa25x_udc *dev)
+{
+	return __UDC_REG(UDCCS0);
+}
+
+static inline void udc_ep0_set_UDCCS(struct pxa25x_udc *dev, u32 data)
+{
+	__UDC_REG(UDCCS0) = data;
+}
+
+static inline u32 udc_ep_get_UDDR(struct pxa25x_ep *ep)
+{
+	return __UDC_REG(ep->regoff_uddr);
+}
+
+static inline void udc_ep_set_UDDR(struct pxa25x_ep *ep, u32 data)
+{
+	__UDC_REG(ep->regoff_uddr) = data;
+}
+
+static inline u32 udc_ep_get_UBCR(struct pxa25x_ep *ep)
+{
+	return __UDC_REG(ep->regoff_ubcr);
 }
 
 /*
@@ -507,7 +555,7 @@ static inline void ep0_idle (struct pxa25x_udc *dev)
 }
 
 static int
-write_packet(volatile u32 *uddr, struct pxa25x_request *req, unsigned max)
+write_packet(struct pxa25x_ep *ep, struct pxa25x_request *req, unsigned max)
 {
 	u8		*buf;
 	unsigned	length, count;
@@ -521,7 +569,7 @@ write_packet(volatile u32 *uddr, struct pxa25x_request *req, unsigned max)
 
 	count = length;
 	while (likely(count--))
-		*uddr = *buf++;
+		udc_ep_set_UDDR(ep, *buf++);
 
 	return length;
 }
@@ -541,7 +589,7 @@ write_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		unsigned	count;
 		int		is_last, is_short;
 
-		count = write_packet(ep->reg_uddr, req, max);
+		count = write_packet(ep, req, max);
 
 		/* last packet is usually short (or a zlp) */
 		if (unlikely (count != max))
@@ -565,15 +613,15 @@ write_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		 * double buffering might work.  TSP, TPC, and TFS
 		 * bit values are the same for all normal IN endpoints.
 		 */
-		*ep->reg_udccs = UDCCS_BI_TPC;
+		udc_ep_set_UDCCS(ep, UDCCS_BI_TPC);
 		if (is_short)
-			*ep->reg_udccs = UDCCS_BI_TSP;
+			udc_ep_set_UDCCS(ep, UDCCS_BI_TSP);
 
 		/* requests complete when all IN data is in the FIFO */
 		if (is_last) {
 			done (ep, req, 0);
 			if (list_empty(&ep->queue))
-				pio_irq_disable (ep->bEndpointAddress);
+				pio_irq_disable(ep);
 			return 1;
 		}
 
@@ -581,7 +629,7 @@ write_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		// double buffering is off in the default fifo mode, which
 		// prevents TFS from being set here.
 
-	} while (*ep->reg_udccs & UDCCS_BI_TFS);
+	} while (udc_ep_get_UDCCS(ep) & UDCCS_BI_TFS);
 	return 0;
 }
 
@@ -591,20 +639,21 @@ write_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 static inline
 void ep0start(struct pxa25x_udc *dev, u32 flags, const char *tag)
 {
-	UDCCS0 = flags|UDCCS0_SA|UDCCS0_OPR;
-	USIR0 = USIR0_IR0;
+	udc_ep0_set_UDCCS(dev, flags|UDCCS0_SA|UDCCS0_OPR);
+	udc_set_reg(dev, USIR0, USIR0_IR0);
 	dev->req_pending = 0;
 	DBG(DBG_VERY_NOISY, "%s %s, %02x/%02x\n",
-		__func__, tag, UDCCS0, flags);
+		__func__, tag, udc_ep0_get_UDCCS(dev), flags);
 }
 
 static int
 write_ep0_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 {
+	struct pxa25x_udc *dev = ep->dev;
 	unsigned	count;
 	int		is_short;
 
-	count = write_packet(&UDDR0, req, EP0_FIFO_SIZE);
+	count = write_packet(&dev->ep[0], req, EP0_FIFO_SIZE);
 	ep->dev->stats.write.bytes += count;
 
 	/* last packet "must be" short (or a zlp) */
@@ -617,7 +666,7 @@ write_ep0_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		if (ep->dev->req_pending)
 			ep0start(ep->dev, UDCCS0_IPR, "short IN");
 		else
-			UDCCS0 = UDCCS0_IPR;
+			udc_ep0_set_UDCCS(dev, UDCCS0_IPR);
 
 		count = req->req.length;
 		done (ep, req, 0);
@@ -633,9 +682,9 @@ write_ep0_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		if (count >= EP0_FIFO_SIZE) {
 			count = 100;
 			do {
-				if ((UDCCS0 & UDCCS0_OPR) != 0) {
+				if ((udc_ep0_get_UDCCS(dev) & UDCCS0_OPR) != 0) {
 					/* clear OPR, generate ack */
-					UDCCS0 = UDCCS0_OPR;
+					udc_ep0_set_UDCCS(dev, UDCCS0_OPR);
 					break;
 				}
 				count--;
@@ -670,7 +719,7 @@ read_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		 * UDCCS_{BO,IO}_RPC are all the same bit value.
 		 * UDCCS_{BO,IO}_RNE are all the same bit value.
 		 */
-		udccs = *ep->reg_udccs;
+		udccs = udc_ep_get_UDCCS(ep);
 		if (unlikely ((udccs & UDCCS_BO_RPC) == 0))
 			break;
 		buf = req->req.buf + req->req.actual;
@@ -679,7 +728,7 @@ read_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 
 		/* read all bytes from this packet */
 		if (likely (udccs & UDCCS_BO_RNE)) {
-			count = 1 + (0x0ff & *ep->reg_ubcr);
+			count = 1 + (0x0ff & udc_ep_get_UBCR(ep));
 			req->req.actual += min (count, bufferspace);
 		} else /* zlp */
 			count = 0;
@@ -689,7 +738,7 @@ read_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 			is_short ? "/S" : "",
 			req, req->req.actual, req->req.length);
 		while (likely (count-- != 0)) {
-			u8	byte = (u8) *ep->reg_uddr;
+			u8	byte = (u8) udc_ep_get_UDDR(ep);
 
 			if (unlikely (bufferspace == 0)) {
 				/* this happens when the driver's buffer
@@ -705,7 +754,7 @@ read_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 				bufferspace--;
 			}
 		}
-		*ep->reg_udccs =  UDCCS_BO_RPC;
+		udc_ep_set_UDCCS(ep, UDCCS_BO_RPC);
 		/* RPC/RSP/RNE could now reflect the other packet buffer */
 
 		/* iso is one request per packet */
@@ -720,7 +769,7 @@ read_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		if (is_short || req->req.actual == req->req.length) {
 			done (ep, req, 0);
 			if (list_empty(&ep->queue))
-				pio_irq_disable (ep->bEndpointAddress);
+				pio_irq_disable(ep);
 			return 1;
 		}
 
@@ -744,7 +793,7 @@ read_ep0_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 	buf = req->req.buf + req->req.actual;
 	bufferspace = req->req.length - req->req.actual;
 
-	while (UDCCS0 & UDCCS0_RNE) {
+	while (udc_ep_get_UDCCS(ep) & UDCCS0_RNE) {
 		byte = (u8) UDDR0;
 
 		if (unlikely (bufferspace == 0)) {
@@ -762,7 +811,7 @@ read_ep0_fifo (struct pxa25x_ep *ep, struct pxa25x_request *req)
 		}
 	}
 
-	UDCCS0 = UDCCS0_OPR | UDCCS0_IPR;
+	udc_ep_set_UDCCS(ep, UDCCS0_OPR | UDCCS0_IPR);
 
 	/* completion */
 	if (req->req.actual >= req->req.length)
@@ -836,8 +885,8 @@ pxa25x_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
 					DBG(DBG_VERBOSE, "ep0 config ack%s\n",
 						dev->has_cfr ?  "" : " raced");
 					if (dev->has_cfr)
-						UDCCFR = UDCCFR_AREN|UDCCFR_ACM
-							|UDCCFR_MB1;
+						udc_set_reg(dev, UDCCFR, UDCCFR_AREN |
+							    UDCCFR_ACM | UDCCFR_MB1);
 					done(ep, req, 0);
 					dev->ep0state = EP0_END_XFER;
 					local_irq_restore (flags);
@@ -845,7 +894,7 @@ pxa25x_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
 				}
 				if (dev->req_pending)
 					ep0start(dev, UDCCS0_IPR, "OUT");
-				if (length == 0 || ((UDCCS0 & UDCCS0_RNE) != 0
+				if (length == 0 || ((udc_ep0_get_UDCCS(dev) & UDCCS0_RNE) != 0
 						&& read_ep0_fifo(ep, req))) {
 					ep0_idle(dev);
 					done(ep, req, 0);
@@ -860,16 +909,16 @@ pxa25x_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
 			}
 		/* can the FIFO can satisfy the request immediately? */
 		} else if ((ep->bEndpointAddress & USB_DIR_IN) != 0) {
-			if ((*ep->reg_udccs & UDCCS_BI_TFS) != 0
+			if ((udc_ep_get_UDCCS(ep) & UDCCS_BI_TFS) != 0
 					&& write_fifo(ep, req))
 				req = NULL;
-		} else if ((*ep->reg_udccs & UDCCS_BO_RFS) != 0
+		} else if ((udc_ep_get_UDCCS(ep) & UDCCS_BO_RFS) != 0
 				&& read_fifo(ep, req)) {
 			req = NULL;
 		}
 
 		if (likely(req && ep->ep.desc))
-			pio_irq_enable(ep->bEndpointAddress);
+			pio_irq_enable(ep);
 	}
 
 	/* pio or dma irq handler advances the queue. */
@@ -896,7 +945,7 @@ static void nuke(struct pxa25x_ep *ep, int status)
 		done(ep, req, status);
 	}
 	if (ep->ep.desc)
-		pio_irq_disable (ep->bEndpointAddress);
+		pio_irq_disable(ep);
 }
 
 
@@ -956,14 +1005,14 @@ static int pxa25x_ep_set_halt(struct usb_ep *_ep, int value)
 	local_irq_save(flags);
 
 	if ((ep->bEndpointAddress & USB_DIR_IN) != 0
-			&& ((*ep->reg_udccs & UDCCS_BI_TFS) == 0
+			&& ((udc_ep_get_UDCCS(ep) & UDCCS_BI_TFS) == 0
 			   || !list_empty(&ep->queue))) {
 		local_irq_restore(flags);
 		return -EAGAIN;
 	}
 
 	/* FST bit is the same for control, bulk in, bulk out, interrupt in */
-	*ep->reg_udccs = UDCCS_BI_FST|UDCCS_BI_FTF;
+	udc_ep_set_UDCCS(ep, UDCCS_BI_FST|UDCCS_BI_FTF);
 
 	/* ep0 needs special care */
 	if (!ep->ep.desc) {
@@ -975,7 +1024,7 @@ static int pxa25x_ep_set_halt(struct usb_ep *_ep, int value)
 	} else {
 		unsigned i;
 		for (i = 0; i < 1000; i += 20) {
-			if (*ep->reg_udccs & UDCCS_BI_SST)
+			if (udc_ep_get_UDCCS(ep) & UDCCS_BI_SST)
 				break;
 			udelay(20);
 		}
@@ -999,10 +1048,10 @@ static int pxa25x_ep_fifo_status(struct usb_ep *_ep)
 	if ((ep->bEndpointAddress & USB_DIR_IN) != 0)
 		return -EOPNOTSUPP;
 	if (ep->dev->gadget.speed == USB_SPEED_UNKNOWN
-			|| (*ep->reg_udccs & UDCCS_BO_RFS) == 0)
+			|| (udc_ep_get_UDCCS(ep) & UDCCS_BO_RFS) == 0)
 		return 0;
 	else
-		return (*ep->reg_ubcr & 0xfff) + 1;
+		return (udc_ep_get_UBCR(ep) & 0xfff) + 1;
 }
 
 static void pxa25x_ep_fifo_flush(struct usb_ep *_ep)
@@ -1019,15 +1068,15 @@ static void pxa25x_ep_fifo_flush(struct usb_ep *_ep)
 
 	/* for OUT, just read and discard the FIFO contents. */
 	if ((ep->bEndpointAddress & USB_DIR_IN) == 0) {
-		while (((*ep->reg_udccs) & UDCCS_BO_RNE) != 0)
-			(void) *ep->reg_uddr;
+		while (((udc_ep_get_UDCCS(ep)) & UDCCS_BO_RNE) != 0)
+			(void)udc_ep_get_UDDR(ep);
 		return;
 	}
 
 	/* most IN status is the same, but ISO can't stall */
-	*ep->reg_udccs = UDCCS_BI_TPC|UDCCS_BI_FTF|UDCCS_BI_TUR
+	udc_ep_set_UDCCS(ep, UDCCS_BI_TPC|UDCCS_BI_FTF|UDCCS_BI_TUR
 		| (ep->bmAttributes == USB_ENDPOINT_XFER_ISOC
-			? 0 : UDCCS_BI_SST);
+			? 0 : UDCCS_BI_SST));
 }
 
 
@@ -1054,15 +1103,23 @@ static struct usb_ep_ops pxa25x_ep_ops = {
 
 static int pxa25x_udc_get_frame(struct usb_gadget *_gadget)
 {
-	return ((UFNRH & 0x07) << 8) | (UFNRL & 0xff);
+	struct pxa25x_udc	*dev;
+
+	dev = container_of(_gadget, struct pxa25x_udc, gadget);
+	return ((udc_get_reg(dev, UFNRH) & 0x07) << 8) |
+		(udc_get_reg(dev, UFNRL) & 0xff);
 }
 
 static int pxa25x_udc_wakeup(struct usb_gadget *_gadget)
 {
+	struct pxa25x_udc	*udc;
+
+	udc = container_of(_gadget, struct pxa25x_udc, gadget);
+
 	/* host may not have enabled remote wakeup */
-	if ((UDCCS0 & UDCCS0_DRWF) == 0)
+	if ((udc_ep0_get_UDCCS(udc) & UDCCS0_DRWF) == 0)
 		return -EHOSTUNREACH;
-	udc_set_mask_UDCCR(UDCCR_RSM);
+	udc_set_mask_UDCCR(udc, UDCCR_RSM);
 	return 0;
 }
 
@@ -1183,9 +1240,11 @@ udc_seq_show(struct seq_file *m, void *_d)
 	/* registers for device and ep0 */
 	seq_printf(m,
 		"uicr %02X.%02X, usir %02X.%02x, ufnr %02X.%02X\n",
-		UICR1, UICR0, USIR1, USIR0, UFNRH, UFNRL);
+		udc_get_reg(dev, UICR1), udc_get_reg(dev, UICR0),
+		udc_get_reg(dev, USIR1), udc_get_reg(dev, USIR0),
+		udc_get_reg(dev, UFNRH), udc_get_reg(dev, UFNRL));
 
-	tmp = UDCCR;
+	tmp = udc_get_reg(dev, UDCCR);
 	seq_printf(m,
 		"udccr %02X =%s%s%s%s%s%s%s%s\n", tmp,
 		(tmp & UDCCR_REM) ? " rem" : "",
@@ -1197,7 +1256,7 @@ udc_seq_show(struct seq_file *m, void *_d)
 		(tmp & UDCCR_UDA) ? " uda" : "",
 		(tmp & UDCCR_UDE) ? " ude" : "");
 
-	tmp = UDCCS0;
+	tmp = udc_ep0_get_UDCCS(dev);
 	seq_printf(m,
 		"udccs0 %02X =%s%s%s%s%s%s%s%s\n", tmp,
 		(tmp & UDCCS0_SA) ? " sa" : "",
@@ -1210,7 +1269,7 @@ udc_seq_show(struct seq_file *m, void *_d)
 		(tmp & UDCCS0_OPR) ? " opr" : "");
 
 	if (dev->has_cfr) {
-		tmp = UDCCFR;
+		tmp = udc_get_reg(dev, UDCCFR);
 		seq_printf(m,
 			"udccfr %02X =%s%s\n", tmp,
 			(tmp & UDCCFR_AREN) ? " aren" : "",
@@ -1236,7 +1295,7 @@ udc_seq_show(struct seq_file *m, void *_d)
 			desc = ep->ep.desc;
 			if (!desc)
 				continue;
-			tmp = *dev->ep [i].reg_udccs;
+			tmp = udc_ep_get_UDCCS(&dev->ep[i]);
 			seq_printf(m,
 				"%s max %d %s udccs %02x irqs %lu\n",
 				ep->ep.name, usb_endpoint_maxp(desc),
@@ -1300,14 +1359,15 @@ static const struct file_operations debug_fops = {
 static void udc_disable(struct pxa25x_udc *dev)
 {
 	/* block all irqs */
-	udc_set_mask_UDCCR(UDCCR_SRM|UDCCR_REM);
-	UICR0 = UICR1 = 0xff;
-	UFNRH = UFNRH_SIM;
+	udc_set_mask_UDCCR(dev, UDCCR_SRM|UDCCR_REM);
+	udc_set_reg(dev, UICR0, 0xff);
+	udc_set_reg(dev, UICR1, 0xff);
+	udc_set_reg(dev, UFNRH, UFNRH_SIM);
 
 	/* if hardware supports it, disconnect from usb */
 	pullup_off();
 
-	udc_clear_mask_UDCCR(UDCCR_UDE);
+	udc_clear_mask_UDCCR(dev, UDCCR_UDE);
 
 	ep0_idle (dev);
 	dev->gadget.speed = USB_SPEED_UNKNOWN;
@@ -1349,10 +1409,10 @@ static void udc_reinit(struct pxa25x_udc *dev)
  */
 static void udc_enable (struct pxa25x_udc *dev)
 {
-	udc_clear_mask_UDCCR(UDCCR_UDE);
+	udc_clear_mask_UDCCR(dev, UDCCR_UDE);
 
 	/* try to clear these bits before we enable the udc */
-	udc_ack_int_UDCCR(UDCCR_SUSIR|/*UDCCR_RSTIR|*/UDCCR_RESIR);
+	udc_ack_int_UDCCR(dev, UDCCR_SUSIR|/*UDCCR_RSTIR|*/UDCCR_RESIR);
 
 	ep0_idle(dev);
 	dev->gadget.speed = USB_SPEED_UNKNOWN;
@@ -1364,15 +1424,15 @@ static void udc_enable (struct pxa25x_udc *dev)
 	 * - if RESET is already in progress, ack interrupt
 	 * - unmask reset interrupt
 	 */
-	udc_set_mask_UDCCR(UDCCR_UDE);
-	if (!(UDCCR & UDCCR_UDA))
-		udc_ack_int_UDCCR(UDCCR_RSTIR);
+	udc_set_mask_UDCCR(dev, UDCCR_UDE);
+	if (!(udc_get_reg(dev, UDCCR) & UDCCR_UDA))
+		udc_ack_int_UDCCR(dev, UDCCR_RSTIR);
 
 	if (dev->has_cfr /* UDC_RES2 is defined */) {
 		/* pxa255 (a0+) can avoid a set_config race that could
 		 * prevent gadget drivers from configuring correctly
 		 */
-		UDCCFR = UDCCFR_ACM | UDCCFR_MB1;
+		udc_set_reg(dev, UDCCFR, UDCCFR_ACM | UDCCFR_MB1);
 	} else {
 		/* "USB test mode" for pxa250 errata 40-42 (stepping a0, a1)
 		 * which could result in missing packets and interrupts.
@@ -1380,15 +1440,15 @@ static void udc_enable (struct pxa25x_udc *dev)
 		 * double buffers or not; ACM/AREN bits fit into the holes.
 		 * zero bits (like USIR0_IRx) disable double buffering.
 		 */
-		UDC_RES1 = 0x00;
-		UDC_RES2 = 0x00;
+		udc_set_reg(dev, UDC_RES1, 0x00);
+		udc_set_reg(dev, UDC_RES2, 0x00);
 	}
 
 	/* enable suspend/resume and reset irqs */
-	udc_clear_mask_UDCCR(UDCCR_SRM | UDCCR_REM);
+	udc_clear_mask_UDCCR(dev, UDCCR_SRM | UDCCR_REM);
 
 	/* enable ep0 irqs */
-	UICR0 &= ~UICR0_IM0;
+	udc_set_reg(dev, UICR0, udc_get_reg(dev, UICR0) & ~UICR0_IM0);
 
 	/* if hardware supports it, pullup D+ and wait for reset */
 	pullup_on();
@@ -1557,9 +1617,9 @@ static void udc_watchdog(unsigned long _dev)
 
 	local_irq_disable();
 	if (dev->ep0state == EP0_STALL
-			&& (UDCCS0 & UDCCS0_FST) == 0
-			&& (UDCCS0 & UDCCS0_SST) == 0) {
-		UDCCS0 = UDCCS0_FST|UDCCS0_FTF;
+			&& (udc_ep0_get_UDCCS(dev) & UDCCS0_FST) == 0
+			&& (udc_ep0_get_UDCCS(dev) & UDCCS0_SST) == 0) {
+		udc_ep0_set_UDCCS(dev, UDCCS0_FST|UDCCS0_FTF);
 		DBG(DBG_VERBOSE, "ep0 re-stall\n");
 		start_watchdog(dev);
 	}
@@ -1568,7 +1628,7 @@ static void udc_watchdog(unsigned long _dev)
 
 static void handle_ep0 (struct pxa25x_udc *dev)
 {
-	u32			udccs0 = UDCCS0;
+	u32			udccs0 = udc_ep0_get_UDCCS(dev);
 	struct pxa25x_ep	*ep = &dev->ep [0];
 	struct pxa25x_request	*req;
 	union {
@@ -1585,7 +1645,7 @@ static void handle_ep0 (struct pxa25x_udc *dev)
 	/* clear stall status */
 	if (udccs0 & UDCCS0_SST) {
 		nuke(ep, -EPIPE);
-		UDCCS0 = UDCCS0_SST;
+		udc_ep0_set_UDCCS(dev, UDCCS0_SST);
 		del_timer(&dev->timer);
 		ep0_idle(dev);
 	}
@@ -1600,7 +1660,7 @@ static void handle_ep0 (struct pxa25x_udc *dev)
 	switch (dev->ep0state) {
 	case EP0_IDLE:
 		/* late-breaking status? */
-		udccs0 = UDCCS0;
+		udccs0 = udc_ep0_get_UDCCS(dev);
 
 		/* start control request? */
 		if (likely((udccs0 & (UDCCS0_OPR|UDCCS0_SA|UDCCS0_RNE))
@@ -1611,14 +1671,14 @@ static void handle_ep0 (struct pxa25x_udc *dev)
 
 			/* read SETUP packet */
 			for (i = 0; i < 8; i++) {
-				if (unlikely(!(UDCCS0 & UDCCS0_RNE))) {
+				if (unlikely(!(udc_ep0_get_UDCCS(dev) & UDCCS0_RNE))) {
 bad_setup:
 					DMSG("SETUP %d!\n", i);
 					goto stall;
 				}
 				u.raw [i] = (u8) UDDR0;
 			}
-			if (unlikely((UDCCS0 & UDCCS0_RNE) != 0))
+			if (unlikely((udc_ep0_get_UDCCS(dev) & UDCCS0_RNE) != 0))
 				goto bad_setup;
 
 got_setup:
@@ -1694,7 +1754,7 @@ config_change:
 					 */
 				}
 				DBG(DBG_VERBOSE, "protocol STALL, "
-					"%02x err %d\n", UDCCS0, i);
+					"%02x err %d\n", udc_ep0_get_UDCCS(dev), i);
 stall:
 				/* the watchdog timer helps deal with cases
 				 * where udc seems to clear FST wrongly, and
@@ -1741,12 +1801,12 @@ stall:
 			 * - IPR cleared
 			 * - OPR got set, without SA (likely status stage)
 			 */
-			UDCCS0 = udccs0 & (UDCCS0_SA|UDCCS0_OPR);
+			udc_ep0_set_UDCCS(dev, udccs0 & (UDCCS0_SA|UDCCS0_OPR));
 		}
 		break;
 	case EP0_IN_DATA_PHASE:			/* GET_DESCRIPTOR etc */
 		if (udccs0 & UDCCS0_OPR) {
-			UDCCS0 = UDCCS0_OPR|UDCCS0_FTF;
+			udc_ep0_set_UDCCS(dev, UDCCS0_OPR|UDCCS0_FTF);
 			DBG(DBG_VERBOSE, "ep0in premature status\n");
 			if (req)
 				done(ep, req, 0);
@@ -1780,14 +1840,14 @@ stall:
 		 * also appears after some config change events.
 		 */
 		if (udccs0 & UDCCS0_OPR)
-			UDCCS0 = UDCCS0_OPR;
+			udc_ep0_set_UDCCS(dev, UDCCS0_OPR);
 		ep0_idle(dev);
 		break;
 	case EP0_STALL:
-		UDCCS0 = UDCCS0_FST;
+		udc_ep0_set_UDCCS(dev, UDCCS0_FST);
 		break;
 	}
-	USIR0 = USIR0_IR0;
+	udc_set_reg(dev, USIR0, USIR0_IR0);
 }
 
 static void handle_ep(struct pxa25x_ep *ep)
@@ -1807,14 +1867,14 @@ static void handle_ep(struct pxa25x_ep *ep)
 
 		// TODO check FST handling
 
-		udccs = *ep->reg_udccs;
+		udccs = udc_ep_get_UDCCS(ep);
 		if (unlikely(is_in)) {	/* irq from TPC, SST, or (ISO) TUR */
 			tmp = UDCCS_BI_TUR;
 			if (likely(ep->bmAttributes == USB_ENDPOINT_XFER_BULK))
 				tmp |= UDCCS_BI_SST;
 			tmp &= udccs;
 			if (likely (tmp))
-				*ep->reg_udccs = tmp;
+				udc_ep_set_UDCCS(ep, tmp);
 			if (req && likely ((udccs & UDCCS_BI_TFS) != 0))
 				completed = write_fifo(ep, req);
 
@@ -1825,13 +1885,13 @@ static void handle_ep(struct pxa25x_ep *ep)
 				tmp = UDCCS_IO_ROF | UDCCS_IO_DME;
 			tmp &= udccs;
 			if (likely(tmp))
-				*ep->reg_udccs = tmp;
+				udc_ep_set_UDCCS(ep, tmp);
 
 			/* fifos can hold packets, ready for reading... */
 			if (likely(req)) {
 				completed = read_fifo(ep, req);
 			} else
-				pio_irq_disable (ep->bEndpointAddress);
+				pio_irq_disable(ep);
 		}
 		ep->pio_irqs++;
 	} while (completed);
@@ -1852,13 +1912,13 @@ pxa25x_udc_irq(int irq, void *_dev)
 
 	dev->stats.irqs++;
 	do {
-		u32		udccr = UDCCR;
+		u32		udccr = udc_get_reg(dev, UDCCR);
 
 		handled = 0;
 
 		/* SUSpend Interrupt Request */
 		if (unlikely(udccr & UDCCR_SUSIR)) {
-			udc_ack_int_UDCCR(UDCCR_SUSIR);
+			udc_ack_int_UDCCR(dev, UDCCR_SUSIR);
 			handled = 1;
 			DBG(DBG_VERBOSE, "USB suspend\n");
 
@@ -1871,7 +1931,7 @@ pxa25x_udc_irq(int irq, void *_dev)
 
 		/* RESume Interrupt Request */
 		if (unlikely(udccr & UDCCR_RESIR)) {
-			udc_ack_int_UDCCR(UDCCR_RESIR);
+			udc_ack_int_UDCCR(dev, UDCCR_RESIR);
 			handled = 1;
 			DBG(DBG_VERBOSE, "USB resume\n");
 
@@ -1883,10 +1943,10 @@ pxa25x_udc_irq(int irq, void *_dev)
 
 		/* ReSeT Interrupt Request - USB reset */
 		if (unlikely(udccr & UDCCR_RSTIR)) {
-			udc_ack_int_UDCCR(UDCCR_RSTIR);
+			udc_ack_int_UDCCR(dev, UDCCR_RSTIR);
 			handled = 1;
 
-			if ((UDCCR & UDCCR_UDA) == 0) {
+			if ((udc_get_reg(dev, UDCCR) & UDCCR_UDA) == 0) {
 				DBG(DBG_VERBOSE, "USB reset start\n");
 
 				/* reset driver and endpoints,
@@ -1902,8 +1962,10 @@ pxa25x_udc_irq(int irq, void *_dev)
 			}
 
 		} else {
-			u32	usir0 = USIR0 & ~UICR0;
-			u32	usir1 = USIR1 & ~UICR1;
+			u32	usir0 = udc_get_reg(dev, USIR0) &
+					~udc_get_reg(dev, UICR0);
+			u32	usir1 = udc_get_reg(dev, USIR1) &
+					~udc_get_reg(dev, UICR1);
 			int	i;
 
 			if (unlikely (!usir0 && !usir1))
@@ -1924,13 +1986,15 @@ pxa25x_udc_irq(int irq, void *_dev)
 
 				if (i && (usir0 & tmp)) {
 					handle_ep(&dev->ep[i]);
-					USIR0 |= tmp;
+					udc_set_reg(dev, USIR0,
+						udc_get_reg(dev, USIR0) | tmp);
 					handled = 1;
 				}
 #ifndef	CONFIG_USB_PXA25X_SMALL
 				if (usir1 & tmp) {
 					handle_ep(&dev->ep[i+8]);
-					USIR1 |= tmp;
+					udc_set_reg(dev, USIR1,
+						udc_get_reg(dev, USIR1) | tmp);
 					handled = 1;
 				}
 #endif
@@ -1975,8 +2039,8 @@ static struct pxa25x_udc memory = {
 						USB_EP_CAPS_DIR_ALL),
 		},
 		.dev		= &memory,
-		.reg_udccs	= &UDCCS0,
-		.reg_uddr	= &UDDR0,
+		.regoff_udccs	= UDCCS0,
+		.regoff_uddr	= UDDR0,
 	},
 
 	/* first group of endpoints */
@@ -1992,8 +2056,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= BULK_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 1,
 		.bmAttributes	= USB_ENDPOINT_XFER_BULK,
-		.reg_udccs	= &UDCCS1,
-		.reg_uddr	= &UDDR1,
+		.regoff_udccs	= UDCCS1,
+		.regoff_uddr	= UDDR1,
 	},
 	.ep[2] = {
 		.ep = {
@@ -2007,9 +2071,9 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= BULK_FIFO_SIZE,
 		.bEndpointAddress = 2,
 		.bmAttributes	= USB_ENDPOINT_XFER_BULK,
-		.reg_udccs	= &UDCCS2,
-		.reg_ubcr	= &UBCR2,
-		.reg_uddr	= &UDDR2,
+		.regoff_udccs	= UDCCS2,
+		.regoff_ubcr	= UBCR2,
+		.regoff_uddr	= UDDR2,
 	},
 #ifndef CONFIG_USB_PXA25X_SMALL
 	.ep[3] = {
@@ -2024,8 +2088,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= ISO_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 3,
 		.bmAttributes	= USB_ENDPOINT_XFER_ISOC,
-		.reg_udccs	= &UDCCS3,
-		.reg_uddr	= &UDDR3,
+		.regoff_udccs	= UDCCS3,
+		.regoff_uddr	= UDDR3,
 	},
 	.ep[4] = {
 		.ep = {
@@ -2039,9 +2103,9 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= ISO_FIFO_SIZE,
 		.bEndpointAddress = 4,
 		.bmAttributes	= USB_ENDPOINT_XFER_ISOC,
-		.reg_udccs	= &UDCCS4,
-		.reg_ubcr	= &UBCR4,
-		.reg_uddr	= &UDDR4,
+		.regoff_udccs	= UDCCS4,
+		.regoff_ubcr	= UBCR4,
+		.regoff_uddr	= UDDR4,
 	},
 	.ep[5] = {
 		.ep = {
@@ -2054,8 +2118,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= INT_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 5,
 		.bmAttributes	= USB_ENDPOINT_XFER_INT,
-		.reg_udccs	= &UDCCS5,
-		.reg_uddr	= &UDDR5,
+		.regoff_udccs	= UDCCS5,
+		.regoff_uddr	= UDDR5,
 	},
 
 	/* second group of endpoints */
@@ -2071,8 +2135,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= BULK_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 6,
 		.bmAttributes	= USB_ENDPOINT_XFER_BULK,
-		.reg_udccs	= &UDCCS6,
-		.reg_uddr	= &UDDR6,
+		.regoff_udccs	= UDCCS6,
+		.regoff_uddr	= UDDR6,
 	},
 	.ep[7] = {
 		.ep = {
@@ -2086,9 +2150,9 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= BULK_FIFO_SIZE,
 		.bEndpointAddress = 7,
 		.bmAttributes	= USB_ENDPOINT_XFER_BULK,
-		.reg_udccs	= &UDCCS7,
-		.reg_ubcr	= &UBCR7,
-		.reg_uddr	= &UDDR7,
+		.regoff_udccs	= UDCCS7,
+		.regoff_ubcr	= UBCR7,
+		.regoff_uddr	= UDDR7,
 	},
 	.ep[8] = {
 		.ep = {
@@ -2102,8 +2166,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= ISO_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 8,
 		.bmAttributes	= USB_ENDPOINT_XFER_ISOC,
-		.reg_udccs	= &UDCCS8,
-		.reg_uddr	= &UDDR8,
+		.regoff_udccs	= UDCCS8,
+		.regoff_uddr	= UDDR8,
 	},
 	.ep[9] = {
 		.ep = {
@@ -2117,9 +2181,9 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= ISO_FIFO_SIZE,
 		.bEndpointAddress = 9,
 		.bmAttributes	= USB_ENDPOINT_XFER_ISOC,
-		.reg_udccs	= &UDCCS9,
-		.reg_ubcr	= &UBCR9,
-		.reg_uddr	= &UDDR9,
+		.regoff_udccs	= UDCCS9,
+		.regoff_ubcr	= UBCR9,
+		.regoff_uddr	= UDDR9,
 	},
 	.ep[10] = {
 		.ep = {
@@ -2132,8 +2196,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= INT_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 10,
 		.bmAttributes	= USB_ENDPOINT_XFER_INT,
-		.reg_udccs	= &UDCCS10,
-		.reg_uddr	= &UDDR10,
+		.regoff_udccs	= UDCCS10,
+		.regoff_uddr	= UDDR10,
 	},
 
 	/* third group of endpoints */
@@ -2149,8 +2213,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= BULK_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 11,
 		.bmAttributes	= USB_ENDPOINT_XFER_BULK,
-		.reg_udccs	= &UDCCS11,
-		.reg_uddr	= &UDDR11,
+		.regoff_udccs	= UDCCS11,
+		.regoff_uddr	= UDDR11,
 	},
 	.ep[12] = {
 		.ep = {
@@ -2164,9 +2228,9 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= BULK_FIFO_SIZE,
 		.bEndpointAddress = 12,
 		.bmAttributes	= USB_ENDPOINT_XFER_BULK,
-		.reg_udccs	= &UDCCS12,
-		.reg_ubcr	= &UBCR12,
-		.reg_uddr	= &UDDR12,
+		.regoff_udccs	= UDCCS12,
+		.regoff_ubcr	= UBCR12,
+		.regoff_uddr	= UDDR12,
 	},
 	.ep[13] = {
 		.ep = {
@@ -2180,8 +2244,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= ISO_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 13,
 		.bmAttributes	= USB_ENDPOINT_XFER_ISOC,
-		.reg_udccs	= &UDCCS13,
-		.reg_uddr	= &UDDR13,
+		.regoff_udccs	= UDCCS13,
+		.regoff_uddr	= UDDR13,
 	},
 	.ep[14] = {
 		.ep = {
@@ -2195,9 +2259,9 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= ISO_FIFO_SIZE,
 		.bEndpointAddress = 14,
 		.bmAttributes	= USB_ENDPOINT_XFER_ISOC,
-		.reg_udccs	= &UDCCS14,
-		.reg_ubcr	= &UBCR14,
-		.reg_uddr	= &UDDR14,
+		.regoff_udccs	= UDCCS14,
+		.regoff_ubcr	= UBCR14,
+		.regoff_uddr	= UDDR14,
 	},
 	.ep[15] = {
 		.ep = {
@@ -2210,8 +2274,8 @@ static struct pxa25x_udc memory = {
 		.fifo_size	= INT_FIFO_SIZE,
 		.bEndpointAddress = USB_DIR_IN | 15,
 		.bmAttributes	= USB_ENDPOINT_XFER_INT,
-		.reg_udccs	= &UDCCS15,
-		.reg_uddr	= &UDDR15,
+		.regoff_udccs	= UDCCS15,
+		.regoff_uddr	= UDDR15,
 	},
 #endif /* !CONFIG_USB_PXA25X_SMALL */
 };
@@ -2258,6 +2322,7 @@ static int pxa25x_udc_probe(struct platform_device *pdev)
 	struct pxa25x_udc *dev = &memory;
 	int retval, irq;
 	u32 chiprev;
+	struct resource *res;
 
 	pr_info("%s: version %s\n", driver_name, DRIVER_VERSION);
 
@@ -2303,6 +2368,11 @@ static int pxa25x_udc_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pxa25x_udc_reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pxa25x_udc_reg_base))
+		return PTR_ERR(pxa25x_udc_reg_base);
+
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk))
 		return PTR_ERR(dev->clk);
diff --git a/drivers/usb/gadget/udc/pxa25x_udc.h b/drivers/usb/gadget/udc/pxa25x_udc.h
index 3fe5931dc21a..f884513f7390 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.h
+++ b/drivers/usb/gadget/udc/pxa25x_udc.h
@@ -56,9 +56,9 @@ struct pxa25x_ep {
 	 * UDDR = UDC Endpoint Data Register (the fifo)
 	 * DRCM = DMA Request Channel Map
 	 */
-	volatile u32				*reg_udccs;
-	volatile u32				*reg_ubcr;
-	volatile u32				*reg_uddr;
+	u32					regoff_udccs;
+	u32					regoff_ubcr;
+	u32					regoff_uddr;
 };
 
 struct pxa25x_request {
@@ -197,6 +197,8 @@ dump_udccs0(const char *label)
 		(udccs0 & UDCCS0_OPR) ? " opr" : "");
 }
 
+static inline u32 udc_ep_get_UDCCS(struct pxa25x_ep *);
+
 static void __maybe_unused
 dump_state(struct pxa25x_udc *dev)
 {
@@ -228,7 +230,7 @@ dump_state(struct pxa25x_udc *dev)
 	for (i = 1; i < PXA_UDC_NUM_ENDPOINTS; i++) {
 		if (dev->ep[i].ep.desc == NULL)
 			continue;
-		DMSG ("udccs%d = %02x\n", i, *dev->ep->reg_udccs);
+		DMSG ("udccs%d = %02x\n", i, udc_ep_get_UDCCS(&dev->ep[i]));
 	}
 }
 
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
  2016-01-28 16:17   ` [PATCH 2/7] usb: gadget: pxa25x_udc cleanup Arnd Bergmann
@ 2016-01-28 16:17   ` Arnd Bergmann
  2016-01-29 10:17     ` Robert Jarzmik
  2016-01-29 16:18     ` Krzysztof Hałasa
  2016-01-29  9:32   ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Robert Jarzmik
                     ` (2 subsequent siblings)
  4 siblings, 2 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:17 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman

This converts the pxa25x udc driver to use readl/writel as normal
driver should do, rather than dereferencing __iomem pointers
themselves.

Based on the earlier preparation work, we can now also pass
the register start in the device pointer so we no longer need
the global variable.

The unclear part here is for IXP4xx, which supports both big-endian
and little-endian configurations. So far, the driver has done
no byteswap in either case. I suspect that is wrong and it would
actually need to swap in one or the other case, but I don't know
which. It's also possible that there is some magic setting in
the chip that makes the endianess of the MMIO register match the
CPU, and in that case, the code actually does the right thing
for all configurations, both before and after this patch.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/gadget/udc/pxa25x_udc.c | 61 ++++++++++++++++++++++++-------------
 drivers/usb/gadget/udc/pxa25x_udc.h |  1 +
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c b/drivers/usb/gadget/udc/pxa25x_udc.c
index 5db429f3cfb2..6d3acbf3b311 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.c
+++ b/drivers/usb/gadget/udc/pxa25x_udc.c
@@ -52,9 +52,6 @@
 #include <mach/lubbock.h>
 #endif
 
-static void __iomem *pxa25x_udc_reg_base;
-#define __UDC_REG(x) (*((volatile u32 *)(pxa25x_udc_reg_base + (x))))
-
 #define UDCCR	 0x0000 /* UDC Control Register */
 #define UDC_RES1 0x0004 /* UDC Undocumented - Reserved1 */
 #define UDC_RES2 0x0008 /* UDC Undocumented - Reserved2 */
@@ -292,16 +289,36 @@ static void pullup_on(void)
 		mach->udc_command(PXA2XX_UDC_CMD_CONNECT);
 }
 
-static inline void udc_set_reg(struct pxa25x_udc *dev, u32 reg, u32 uicr)
+#if defined(CONFIG_ARCH_IXP4XX) && defined(CONFIG_CPU_BIG_ENDIAN)
+/*
+ * not sure if this is the correct behavior on ixp4xx in both
+ * bit-endian and little-endian modes, but it's what the driver
+ * has always done using direct pointer dereferences:
+ * We assume that there is a byteswap done in hardware at the
+ * MMIO register that matches what the CPU setting is, so we
+ * never swap in software.
+ */
+static inline void udc_set_reg(struct pxa25x_udc *dev, u32 reg, u32 val)
 {
-	__UDC_REG(reg) = uicr;
+	iowrite32be(val, dev->regs + reg);
 }
 
 static inline u32 udc_get_reg(struct pxa25x_udc *dev, u32 reg)
 {
-	return __UDC_REG(reg);
+	return ioread32be(dev->regs + reg);
+}
+#else
+static inline void udc_set_reg(struct pxa25x_udc *dev, u32 reg, u32 val)
+{
+	writel(val, dev->regs + reg);
 }
 
+static inline u32 udc_get_reg(struct pxa25x_udc *dev, u32 reg)
+{
+	return readl(dev->regs + reg);
+}
+#endif
+
 static void pio_irq_enable(struct pxa25x_ep *ep)
 {
 	u32 bEndpointAddress = ep->bEndpointAddress & 0xf;
@@ -337,59 +354,59 @@ static void pio_irq_disable(struct pxa25x_ep *ep)
 
 static inline void udc_set_mask_UDCCR(struct pxa25x_udc *dev, int mask)
 {
-	u32 udccr = __UDC_REG(UDCCR);
+	u32 udccr = udc_get_reg(dev, UDCCR);
 
-	__UDC_REG(UDCCR) = (udccr & UDCCR_MASK_BITS) | (mask & UDCCR_MASK_BITS);
+	udc_set_reg(dev, (udccr & UDCCR_MASK_BITS) | (mask & UDCCR_MASK_BITS), UDCCR);
 }
 
 static inline void udc_clear_mask_UDCCR(struct pxa25x_udc *dev, int mask)
 {
-	u32 udccr = __UDC_REG(UDCCR);
+	u32 udccr = udc_get_reg(dev, UDCCR);
 
-	__UDC_REG(UDCCR) = (udccr & UDCCR_MASK_BITS) & ~(mask & UDCCR_MASK_BITS);
+	udc_set_reg(dev, (udccr & UDCCR_MASK_BITS) & ~(mask & UDCCR_MASK_BITS), UDCCR);
 }
 
 static inline void udc_ack_int_UDCCR(struct pxa25x_udc *dev, int mask)
 {
 	/* udccr contains the bits we dont want to change */
-	u32 udccr = __UDC_REG(UDCCR) & UDCCR_MASK_BITS;
+	u32 udccr = udc_get_reg(dev, UDCCR) & UDCCR_MASK_BITS;
 
-	__UDC_REG(UDCCR) = udccr | (mask & ~UDCCR_MASK_BITS);
+	udc_set_reg(dev, udccr | (mask & ~UDCCR_MASK_BITS), UDCCR);
 }
 
 static inline u32 udc_ep_get_UDCCS(struct pxa25x_ep *ep)
 {
-	return __UDC_REG(ep->regoff_udccs);
+	return udc_get_reg(ep->dev, ep->regoff_udccs);
 }
 
 static inline void udc_ep_set_UDCCS(struct pxa25x_ep *ep, u32 data)
 {
-	__UDC_REG(ep->regoff_udccs) = data;
+	udc_set_reg(ep->dev, data, ep->regoff_udccs);
 }
 
 static inline u32 udc_ep0_get_UDCCS(struct pxa25x_udc *dev)
 {
-	return __UDC_REG(UDCCS0);
+	return udc_get_reg(dev, UDCCS0);
 }
 
 static inline void udc_ep0_set_UDCCS(struct pxa25x_udc *dev, u32 data)
 {
-	__UDC_REG(UDCCS0) = data;
+	udc_set_reg(dev, data, UDCCS0);
 }
 
 static inline u32 udc_ep_get_UDDR(struct pxa25x_ep *ep)
 {
-	return __UDC_REG(ep->regoff_uddr);
+	return udc_get_reg(ep->dev, ep->regoff_uddr);
 }
 
 static inline void udc_ep_set_UDDR(struct pxa25x_ep *ep, u32 data)
 {
-	__UDC_REG(ep->regoff_uddr) = data;
+	udc_set_reg(ep->dev, data, ep->regoff_uddr);
 }
 
 static inline u32 udc_ep_get_UBCR(struct pxa25x_ep *ep)
 {
-	return __UDC_REG(ep->regoff_ubcr);
+	return udc_get_reg(ep->dev, ep->regoff_ubcr);
 }
 
 /*
@@ -2369,9 +2386,9 @@ static int pxa25x_udc_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pxa25x_udc_reg_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(pxa25x_udc_reg_base))
-		return PTR_ERR(pxa25x_udc_reg_base);
+	dev->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dev->regs))
+		return PTR_ERR(dev->regs);
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk))
diff --git a/drivers/usb/gadget/udc/pxa25x_udc.h b/drivers/usb/gadget/udc/pxa25x_udc.h
index f884513f7390..4b8b72d7ab37 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.h
+++ b/drivers/usb/gadget/udc/pxa25x_udc.h
@@ -125,6 +125,7 @@ struct pxa25x_udc {
 #ifdef CONFIG_USB_GADGET_DEBUG_FS
 	struct dentry				*debugfs_udc;
 #endif
+	void __iomem				*regs;
 };
 #define to_pxa25x(g)	(container_of((g), struct pxa25x_udc, gadget))
 
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 4/7] usb: fsl: drop USB_FSL_MPH_DR_OF Kconfig symbol
  2016-01-28 16:15 [PATCH 0/7] USB changes for rare warnings Arnd Bergmann
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
@ 2016-01-28 16:20 ` Arnd Bergmann
  2016-01-28 16:23 ` [PATCH 5/7] usb: isp1301-omap: mark power_up as __maybe_unused Arnd Bergmann
  2016-02-03 18:12 ` [PATCH 0/7] USB changes for rare warnings Felipe Balbi
  3 siblings, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:20 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman, Nikhil Badola,
	Alan Stern, Chunfeng Yun

The USB_FSL_MPH_DR_OF symbol is used to ensure the code that interprets
the DR device node is built whenever one of the two drivers (EHCI or
UDC) for the platform is enabled. However, if CONFIG_USB is disabled
and we only support gadget mode, this causes a Kconfig warning:

warning: (USB_FSL_USB2) selects USB_FSL_MPH_DR_OF which has unmet direct dependencies (USB_SUPPORT && USB)

We can avoid this warning by simply no longer using the symbol,
and making sure we enter the drivers/usb/host/ directory when
the UDC driver is enabled that needs the file, and then we use
Makefile syntax to ensure the file is built-in if needed.

There is currently a dependency on CONFIG_OF, but this is redundant,
as we already know that this is set unconditionally for the platforms
that use this driver.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/Makefile           | 2 +-
 drivers/usb/gadget/udc/Kconfig | 1 -
 drivers/usb/host/Kconfig       | 4 ----
 drivers/usb/host/Makefile      | 3 ++-
 4 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index d5c57f1e98fd..dca78565eb55 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -26,7 +26,7 @@ obj-$(CONFIG_USB_U132_HCD)	+= host/
 obj-$(CONFIG_USB_R8A66597_HCD)	+= host/
 obj-$(CONFIG_USB_HWA_HCD)	+= host/
 obj-$(CONFIG_USB_IMX21_HCD)	+= host/
-obj-$(CONFIG_USB_FSL_MPH_DR_OF)	+= host/
+obj-$(CONFIG_USB_FSL_USB2)	+= host/
 obj-$(CONFIG_USB_FOTG210_HCD)	+= host/
 obj-$(CONFIG_USB_MAX3421_HCD)	+= host/
 
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 753c29bd11ad..d38cd69b470e 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -74,7 +74,6 @@ config USB_BCM63XX_UDC
 config USB_FSL_USB2
 	tristate "Freescale Highspeed USB DR Peripheral Controller"
 	depends on FSL_SOC || ARCH_MXC
-	select USB_FSL_MPH_DR_OF if OF
 	help
 	   Some of Freescale PowerPC and i.MX processors have a High Speed
 	   Dual-Role(DR) USB controller, which supports device mode.
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 1f117c360ebb..2f73cc5fab71 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -121,9 +121,6 @@ config USB_EHCI_TT_NEWSCHED
 
 	  If unsure, say Y.
 
-config USB_FSL_MPH_DR_OF
-	tristate
-
 if USB_EHCI_HCD
 
 config USB_EHCI_PCI
@@ -156,7 +153,6 @@ config USB_EHCI_FSL
 	tristate "Support for Freescale PPC on-chip EHCI USB controller"
 	depends on FSL_SOC
 	select USB_EHCI_ROOT_HUB_TT
-	select USB_FSL_MPH_DR_OF if OF
 	---help---
 	  Variation of ARC USB block used in some Freescale chips.
 
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 65a06b4382bf..a9ddd3c9ec94 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -74,7 +74,8 @@ obj-$(CONFIG_USB_U132_HCD)	+= u132-hcd.o
 obj-$(CONFIG_USB_R8A66597_HCD)	+= r8a66597-hcd.o
 obj-$(CONFIG_USB_HWA_HCD)	+= hwa-hc.o
 obj-$(CONFIG_USB_IMX21_HCD)	+= imx21-hcd.o
-obj-$(CONFIG_USB_FSL_MPH_DR_OF)	+= fsl-mph-dr-of.o
+obj-$(CONFIG_USB_FSL_USB2)	+= fsl-mph-dr-of.o
+obj-$(CONFIG_USB_EHCI_FSL)	+= fsl-mph-dr-of.o
 obj-$(CONFIG_USB_EHCI_FSL)	+= ehci-fsl.o
 obj-$(CONFIG_USB_HCD_BCMA)	+= bcma-hcd.o
 obj-$(CONFIG_USB_HCD_SSB)	+= ssb-hcd.o
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 5/7] usb: isp1301-omap: mark power_up as __maybe_unused
  2016-01-28 16:15 [PATCH 0/7] USB changes for rare warnings Arnd Bergmann
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
  2016-01-28 16:20 ` [PATCH 4/7] usb: fsl: drop USB_FSL_MPH_DR_OF Kconfig symbol Arnd Bergmann
@ 2016-01-28 16:23 ` Arnd Bergmann
  2016-01-28 16:23   ` [PATCH 6/7] usb: musb: use %pad format string from dma_addr_t Arnd Bergmann
  2016-01-28 16:23   ` [PATCH 7/7] usb: musb/ux500: remove duplicate check for dma_is_compatible Arnd Bergmann
  2016-02-03 18:12 ` [PATCH 0/7] USB changes for rare warnings Felipe Balbi
  3 siblings, 2 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:23 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman, linux-omap

The power_up function is used for otg or udc mode, but nost when
the driver is only configured for host mode:

drivers/usb/phy/phy-isp1301-omap.c:261:13: error: 'power_up' defined but not used [-Werror=unused-function]

This marks the function __maybe_unused to avoid the warning and
silently drop the definition when it is unused.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/phy/phy-isp1301-omap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-isp1301-omap.c b/drivers/usb/phy/phy-isp1301-omap.c
index 3af263cc0caa..8d111ec653e4 100644
--- a/drivers/usb/phy/phy-isp1301-omap.c
+++ b/drivers/usb/phy/phy-isp1301-omap.c
@@ -258,7 +258,7 @@ static void power_down(struct isp1301 *isp)
 	isp1301_clear_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
 }
 
-static void power_up(struct isp1301 *isp)
+static void __maybe_unused power_up(struct isp1301 *isp)
 {
 	// isp1301_clear_bits(isp, ISP1301_MODE_CONTROL_2, MC2_GLOBAL_PWR_DN);
 	isp1301_clear_bits(isp, ISP1301_MODE_CONTROL_1, MC1_SUSPEND);
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 6/7] usb: musb: use %pad format string from dma_addr_t
  2016-01-28 16:23 ` [PATCH 5/7] usb: isp1301-omap: mark power_up as __maybe_unused Arnd Bergmann
@ 2016-01-28 16:23   ` Arnd Bergmann
  2016-01-28 17:50     ` Tony Lindgren
  2016-01-28 16:23   ` [PATCH 7/7] usb: musb/ux500: remove duplicate check for dma_is_compatible Arnd Bergmann
  1 sibling, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:23 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman, linux-omap

The musb driver prints DMA addresses in a few places, using the
0x%x format string. This is wrong on 64-bit architectures (which
need %lx) and 32-bit ARM with CONFIG_LPAE set (which needs
%llx), otherwise we print the wrong data, as gcc warns:

musb/musbhsdma.c: In function 'configure_channel':
musb/musbhsdma.c:120:53: error: format '%x' expects argument of type 'unsigned int', but argument 6 has type 'dma_addr_t {aka long long unsigned int}' [-Werror=format=]
  dev_dbg(musb->controller, "%p, pkt_sz %d, addr 0x%x, len %d, mode %d\n",
musb/musbhsdma.c: In function 'dma_channel_program':
musb/musbhsdma.c:155:53: error: format '%x' expects argument of type 'unsigned int', but argument 7 has type 'dma_addr_t {aka long long unsigned int}' [-Werror=format=]
  dev_dbg(musb->controller, "ep%d-%s pkt_sz %d, dma_addr 0x%x length %d, mode %d\n",
musb/tusb6010_omap.c: In function 'tusb_omap_dma_program':
musb/tusb6010_omap.c:313:53: error: format '%x' expects argument of type 'unsigned int', but argument 7 has type 'dma_addr_t {aka long long unsigned int}' [-Werror=format=]
  dev_dbg(musb->controller, "ep%i %s dma ch%i dma: %08x len: %u(%u) packet_sz: %i(%i)\n",

This uses the %pad format string, which prints a dma_addr_t that
gets passed by reference, which works for all combinations.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/musb/musbhsdma.c     | 8 ++++----
 drivers/usb/musb/tusb6010_omap.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index 7539c3188ffc..8abfe4ec62fb 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -117,8 +117,8 @@ static void configure_channel(struct dma_channel *channel,
 	u8 bchannel = musb_channel->idx;
 	u16 csr = 0;
 
-	dev_dbg(musb->controller, "%p, pkt_sz %d, addr 0x%x, len %d, mode %d\n",
-			channel, packet_sz, dma_addr, len, mode);
+	dev_dbg(musb->controller, "%p, pkt_sz %d, addr %pad, len %d, mode %d\n",
+			channel, packet_sz, &dma_addr, len, mode);
 
 	if (mode) {
 		csr |= 1 << MUSB_HSDMA_MODE1_SHIFT;
@@ -152,10 +152,10 @@ static int dma_channel_program(struct dma_channel *channel,
 	struct musb_dma_controller *controller = musb_channel->controller;
 	struct musb *musb = controller->private_data;
 
-	dev_dbg(musb->controller, "ep%d-%s pkt_sz %d, dma_addr 0x%x length %d, mode %d\n",
+	dev_dbg(musb->controller, "ep%d-%s pkt_sz %d, dma_addr %pad length %d, mode %d\n",
 		musb_channel->epnum,
 		musb_channel->transmit ? "Tx" : "Rx",
-		packet_sz, dma_addr, len, mode);
+		packet_sz, &dma_addr, len, mode);
 
 	BUG_ON(channel->status == MUSB_DMA_STATUS_UNKNOWN ||
 		channel->status == MUSB_DMA_STATUS_BUSY);
diff --git a/drivers/usb/musb/tusb6010_omap.c b/drivers/usb/musb/tusb6010_omap.c
index 4c82077da475..e6959ccb4453 100644
--- a/drivers/usb/musb/tusb6010_omap.c
+++ b/drivers/usb/musb/tusb6010_omap.c
@@ -310,9 +310,9 @@ static int tusb_omap_dma_program(struct dma_channel *channel, u16 packet_sz,
 
 	dma_params.frame_count	= chdat->transfer_len / 32; /* Burst sz frame */
 
-	dev_dbg(musb->controller, "ep%i %s dma ch%i dma: %08x len: %u(%u) packet_sz: %i(%i)\n",
+	dev_dbg(musb->controller, "ep%i %s dma ch%i dma: %pad len: %u(%u) packet_sz: %i(%i)\n",
 		chdat->epnum, chdat->tx ? "tx" : "rx",
-		ch, dma_addr, chdat->transfer_len, len,
+		ch, &dma_addr, chdat->transfer_len, len,
 		chdat->transfer_packet_sz, packet_sz);
 
 	/*
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 7/7] usb: musb/ux500: remove duplicate check for dma_is_compatible
  2016-01-28 16:23 ` [PATCH 5/7] usb: isp1301-omap: mark power_up as __maybe_unused Arnd Bergmann
  2016-01-28 16:23   ` [PATCH 6/7] usb: musb: use %pad format string from dma_addr_t Arnd Bergmann
@ 2016-01-28 16:23   ` Arnd Bergmann
  1 sibling, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:23 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman

When dma_addr_t is 64-bit, we get a warning about an invalid cast
in the call to ux500_dma_is_compatible() from ux500_dma_channel_program():

drivers/usb/musb/ux500_dma.c: In function 'ux500_dma_channel_program':
drivers/usb/musb/ux500_dma.c:210:51: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  if (!ux500_dma_is_compatible(channel, packet_sz, (void *)dma_addr, len))

The problem is that ux500_dma_is_compatible() is called from the
main musb driver on the virtual address, but here we pass in a
DMA address, so the types are fundamentally different but it works
because the function only checks the alignment of the buffer and
that is the same.

We could work around this by adding another cast, but I have checked
that the buffer we get passed here is already checked before it
gets mapped, so the second check seems completely unnecessary
and removing it must be the cleanest solution.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/musb/ux500_dma.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/musb/ux500_dma.c b/drivers/usb/musb/ux500_dma.c
index d0b6a1cd7f62..c92a295049ad 100644
--- a/drivers/usb/musb/ux500_dma.c
+++ b/drivers/usb/musb/ux500_dma.c
@@ -207,9 +207,6 @@ static int ux500_dma_channel_program(struct dma_channel *channel,
 	BUG_ON(channel->status == MUSB_DMA_STATUS_UNKNOWN ||
 		channel->status == MUSB_DMA_STATUS_BUSY);
 
-	if (!ux500_dma_is_compatible(channel, packet_sz, (void *)dma_addr, len))
-		return false;
-
 	channel->status = MUSB_DMA_STATUS_BUSY;
 	channel->actual_len = 0;
 	ret = ux500_configure_channel(channel, packet_sz, mode, dma_addr, len);
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 6/7] usb: musb: use %pad format string from dma_addr_t
  2016-01-28 16:23   ` [PATCH 6/7] usb: musb: use %pad format string from dma_addr_t Arnd Bergmann
@ 2016-01-28 17:50     ` Tony Lindgren
  0 siblings, 0 replies; 36+ messages in thread
From: Tony Lindgren @ 2016-01-28 17:50 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Felipe Balbi, linux-arm-kernel, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman, linux-omap

* Arnd Bergmann <arnd@arndb.de> [160128 08:26]:
> The musb driver prints DMA addresses in a few places, using the
> 0x%x format string. This is wrong on 64-bit architectures (which
> need %lx) and 32-bit ARM with CONFIG_LPAE set (which needs
> %llx), otherwise we print the wrong data, as gcc warns:
> 
> musb/musbhsdma.c: In function 'configure_channel':
> musb/musbhsdma.c:120:53: error: format '%x' expects argument of type 'unsigned int', but argument 6 has type 'dma_addr_t {aka long long unsigned int}' [-Werror=format=]
>   dev_dbg(musb->controller, "%p, pkt_sz %d, addr 0x%x, len %d, mode %d\n",
> musb/musbhsdma.c: In function 'dma_channel_program':
> musb/musbhsdma.c:155:53: error: format '%x' expects argument of type 'unsigned int', but argument 7 has type 'dma_addr_t {aka long long unsigned int}' [-Werror=format=]
>   dev_dbg(musb->controller, "ep%d-%s pkt_sz %d, dma_addr 0x%x length %d, mode %d\n",
> musb/tusb6010_omap.c: In function 'tusb_omap_dma_program':
> musb/tusb6010_omap.c:313:53: error: format '%x' expects argument of type 'unsigned int', but argument 7 has type 'dma_addr_t {aka long long unsigned int}' [-Werror=format=]
>   dev_dbg(musb->controller, "ep%i %s dma ch%i dma: %08x len: %u(%u) packet_sz: %i(%i)\n",
> 
> This uses the %pad format string, which prints a dma_addr_t that
> gets passed by reference, which works for all combinations.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Looks good to me:

Acked-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
  2016-01-28 16:17   ` [PATCH 2/7] usb: gadget: pxa25x_udc cleanup Arnd Bergmann
  2016-01-28 16:17   ` [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann
@ 2016-01-29  9:32   ` Robert Jarzmik
  2016-01-29 10:07     ` Arnd Bergmann
  2016-01-29 15:55   ` Krzysztof Hałasa
  2016-02-17 15:08   ` Felipe Balbi
  4 siblings, 1 reply; 36+ messages in thread
From: Robert Jarzmik @ 2016-01-29  9:32 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Felipe Balbi, Imre Kaloz, Krzysztof Halasa, Russell King,
	Daniel Mack, Haojian Zhuang, Felipe Balbi, Greg Kroah-Hartman,
	linux-arm-kernel, linux-usb, linux-kernel

Arnd Bergmann <arnd@arndb.de> writes:

> ixp4xx and pxa25x both use this driver and provide a slightly
> different set of register definitions for it. Aside from that,
> the definition in the ixp4xx-regs.h header conflicts with the
> on in the pxa27x device driver when compile-testing that:
>
> In file included from ../drivers/usb/gadget/udc/pxa27x_udc.c:37:0:
> ../drivers/usb/gadget/udc/pxa27x_udc.h:26:0: warning: "UDCCR" redefined
>  #define UDCCR  0x0000  /* UDC Control Register */
>  ^
> In file included from ../arch/arm/mach-ixp4xx/include/mach/hardware.h:27:0,
>                  from ../arch/arm/mach-ixp4xx/include/mach/io.h:18,
>                  from ../arch/arm/include/asm/io.h:194,
>                  from ../include/linux/io.h:25,
>                  from ../include/linux/irq.h:24,
>                  from ../drivers/usb/gadget/udc/pxa27x_udc.c:23:
> ../arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h:415:0: note: this is the location of the previous definition
>  #define UDCCR  IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0000)
>
> This addresses both issues by moving all the definitions into the
> pxa25x_udc driver itself. It turns out the only difference between
> them was 'UDCCS_IO_ROF', and that could well be a mistake when it
> was incorrectly copied from pxa25x to ixp4xx.
Hi Arnd,

Is there a reason to have chosen to move into pxa25_udc.c instead of
drivers/usb/gadget/udc/pxa25x_udc.h ? pxa27x_udc has a .h in the same directory
with register definitions, hence the question.

Cheers.

--
Robert

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
  2016-01-29  9:32   ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Robert Jarzmik
@ 2016-01-29 10:07     ` Arnd Bergmann
  2016-01-29 15:26       ` Robert Jarzmik
  0 siblings, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-29 10:07 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: Felipe Balbi, Imre Kaloz, Krzysztof Halasa, Russell King,
	Daniel Mack, Haojian Zhuang, Felipe Balbi, Greg Kroah-Hartman,
	linux-arm-kernel, linux-usb, linux-kernel

On Friday 29 January 2016 10:32:07 Robert Jarzmik wrote:
> > This addresses both issues by moving all the definitions into the
> > pxa25x_udc driver itself. It turns out the only difference between
> > them was 'UDCCS_IO_ROF', and that could well be a mistake when it
> > was incorrectly copied from pxa25x to ixp4xx.
> Hi Arnd,
> 
> Is there a reason to have chosen to move into pxa25_udc.c instead of
> drivers/usb/gadget/udc/pxa25x_udc.h ? pxa27x_udc has a .h in the same directory
> with register definitions, hence the question.

Yes: The register definitions are only used in a single .c file and
are not an interface between files, so it's better to have them in
the file that uses them.

I could have continued the cleanup and moved the two headers into the
respective drivers as well, but I had to stop somewhere ;-)

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/7] usb: gadget: pxa25x_udc cleanup
  2016-01-28 16:17   ` [PATCH 2/7] usb: gadget: pxa25x_udc cleanup Arnd Bergmann
@ 2016-01-29 10:13     ` Robert Jarzmik
  0 siblings, 0 replies; 36+ messages in thread
From: Robert Jarzmik @ 2016-01-29 10:13 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Felipe Balbi, linux-arm-kernel, Felipe Balbi, linux-usb,
	linux-kernel, Haojian Zhuang, Daniel Mack, Imre Kaloz,
	Krzysztof Halasa, Greg Kroah-Hartman

Arnd Bergmann <arnd@arndb.de> writes:

> This removes the dependency on the mach/hardware.h header file
> from the pxa25x_udc driver after the register definitions were
> already unified in the previous patch.
>
> Following the model of pxa27x_udc (and basically all other drivers
> in the kernel), we define the register numbers as offsets from
> the register base address and use accessor functions to read/write
> them.
>
> For the moment, this still leaves the direct pointer dereference
> in place, instead of using readl/writel, so this patch should
> not be changing the behavior of the driver, other than using
> ioremap() on the platform resource to replace the hardcoded
> virtual address pointers.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

It's too bad coccinelle cannot make that work more automatic, isn't it ? :)

Cheers.

--
Robert

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-01-28 16:17   ` [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann
@ 2016-01-29 10:17     ` Robert Jarzmik
  2016-01-29 16:18     ` Krzysztof Hałasa
  1 sibling, 0 replies; 36+ messages in thread
From: Robert Jarzmik @ 2016-01-29 10:17 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Felipe Balbi, linux-arm-kernel, Felipe Balbi, linux-usb,
	linux-kernel, Haojian Zhuang, Daniel Mack, Imre Kaloz,
	Krzysztof Halasa, Greg Kroah-Hartman

Arnd Bergmann <arnd@arndb.de> writes:

> This converts the pxa25x udc driver to use readl/writel as normal
> driver should do, rather than dereferencing __iomem pointers
> themselves.
>
> Based on the earlier preparation work, we can now also pass
> the register start in the device pointer so we no longer need
> the global variable.
>
> The unclear part here is for IXP4xx, which supports both big-endian
> and little-endian configurations. So far, the driver has done
> no byteswap in either case. I suspect that is wrong and it would
> actually need to swap in one or the other case, but I don't know
> which. It's also possible that there is some magic setting in
> the chip that makes the endianess of the MMIO register match the
> CPU, and in that case, the code actually does the right thing
> for all configurations, both before and after this patch.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
For pxa25x_udc:
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
  2016-01-29 10:07     ` Arnd Bergmann
@ 2016-01-29 15:26       ` Robert Jarzmik
  0 siblings, 0 replies; 36+ messages in thread
From: Robert Jarzmik @ 2016-01-29 15:26 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Felipe Balbi, Imre Kaloz, Krzysztof Halasa, Russell King,
	Daniel Mack, Haojian Zhuang, Felipe Balbi, Greg Kroah-Hartman,
	linux-arm-kernel, linux-usb, linux-kernel

Arnd Bergmann <arnd@arndb.de> writes:

> On Friday 29 January 2016 10:32:07 Robert Jarzmik wrote:
>> > This addresses both issues by moving all the definitions into the
>> > pxa25x_udc driver itself. It turns out the only difference between
>> > them was 'UDCCS_IO_ROF', and that could well be a mistake when it
>> > was incorrectly copied from pxa25x to ixp4xx.
>> Hi Arnd,
>> 
>> Is there a reason to have chosen to move into pxa25_udc.c instead of
>> drivers/usb/gadget/udc/pxa25x_udc.h ? pxa27x_udc has a .h in the same directory
>> with register definitions, hence the question.
>
> Yes: The register definitions are only used in a single .c file and
> are not an interface between files, so it's better to have them in
> the file that uses them.
>
> I could have continued the cleanup and moved the two headers into the
> respective drivers as well, but I had to stop somewhere ;-)
Fair enough.

Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

-- 
Robert

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
                     ` (2 preceding siblings ...)
  2016-01-29  9:32   ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Robert Jarzmik
@ 2016-01-29 15:55   ` Krzysztof Hałasa
  2016-02-17 15:08   ` Felipe Balbi
  4 siblings, 0 replies; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-01-29 15:55 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Felipe Balbi, Imre Kaloz, Russell King, Daniel Mack,
	Haojian Zhuang, Robert Jarzmik, Felipe Balbi, Greg Kroah-Hartman,
	linux-arm-kernel, linux-usb, linux-kernel

Arnd Bergmann <arnd@arndb.de> writes:

> This addresses both issues by moving all the definitions into the
> pxa25x_udc driver itself. It turns out the only difference between
> them was 'UDCCS_IO_ROF', and that could well be a mistake when it
> was incorrectly copied from pxa25x to ixp4xx.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Both the programming manual and the Intel's library code say it's
bit 2. Must be a mistake in the old code. Unfortunately I think we
won't be able to check it on real hardware, the UDC on IXP4xx boards
isn't usually connected. Also, this bug wouldn't likely affect
operation (unless there is an overflow event, which would be
currently undetected).

I guess we as well very safely change it to 1 << 2.


For the IXP4xx part,
Acked-by: Krzysztof Halasa <khalasa@piap.pl>
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-01-28 16:17   ` [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann
  2016-01-29 10:17     ` Robert Jarzmik
@ 2016-01-29 16:18     ` Krzysztof Hałasa
  2016-01-29 17:06       ` Arnd Bergmann
  2016-01-29 18:03       ` Sergei Shtylyov
  1 sibling, 2 replies; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-01-29 16:18 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Felipe Balbi, linux-arm-kernel, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Greg Kroah-Hartman

Arnd Bergmann <arnd@arndb.de> writes:

> The unclear part here is for IXP4xx, which supports both big-endian
> and little-endian configurations. So far, the driver has done
> no byteswap in either case. I suspect that is wrong and it would
> actually need to swap in one or the other case, but I don't know
> which.

If at all, I guess it should swap in LE mode. But it's far from certain.

> It's also possible that there is some magic setting in
> the chip that makes the endianess of the MMIO register match the
> CPU, and in that case, the code actually does the right thing
> for all configurations, both before and after this patch.

This is IMHO most probable.

Actually, the IXP4xx is "natural" in BE mode (except for PCI) and
normally in LE mode it's order-coherent, meaning 32-bit "integer"
accesses need no swapping, but 8-bit and (mostly unused) 16-bit
transfers need swapping.

Anyway, I think readl()/writel() do the right thing: in BE mode they
swap PCI accesses and don't swap normal registers, in LE mode nothing is
swapped. LE data-coherent mode (which has never landed in the
official kernel) is a bit different, but still, readl()/writel() do the
right thing.
I remember the "string" (block) functions preserve 8-bit ordering, and
thus 32-bit values transfered using them may need swapping.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-01-29 16:18     ` Krzysztof Hałasa
@ 2016-01-29 17:06       ` Arnd Bergmann
  2016-02-15  7:33         ` Krzysztof Hałasa
  2016-01-29 18:03       ` Sergei Shtylyov
  1 sibling, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-29 17:06 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Krzysztof Hałasa, Felipe Balbi, Greg Kroah-Hartman,
	linux-usb, linux-kernel, Felipe Balbi, Haojian Zhuang,
	Daniel Mack, Imre Kaloz, Robert Jarzmik

On Friday 29 January 2016 17:18:09 Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > The unclear part here is for IXP4xx, which supports both big-endian
> > and little-endian configurations. So far, the driver has done
> > no byteswap in either case. I suspect that is wrong and it would
> > actually need to swap in one or the other case, but I don't know
> > which.
> 
> If at all, I guess it should swap in LE mode. But it's far from certain.
> 
> > It's also possible that there is some magic setting in
> > the chip that makes the endianess of the MMIO register match the
> > CPU, and in that case, the code actually does the right thing
> > for all configurations, both before and after this patch.
> 
> This is IMHO most probable.
> 
> Actually, the IXP4xx is "natural" in BE mode (except for PCI) and
> normally in LE mode it's order-coherent, meaning 32-bit "integer"
> accesses need no swapping, but 8-bit and (mostly unused) 16-bit
> transfers need swapping.

I see.

> Anyway, I think readl()/writel() do the right thing: in BE mode they
> swap PCI accesses and don't swap normal registers, in LE mode nothing is
> swapped.

This seems to be true when CONFIG_IXP4XX_INDIRECT_PCI is set, but
not otherwise. For the indirect variant, writel() is a __raw_writel()
without barrier or byteswap on non-PCI memory, while with that
option disabled, we use the standard implementation that has both
a byteswap and a barrier.

According to your description, that would mean the version without
indirect PCI access is broken and it appears to have been that way
since before the start of git history in 2.6.12.

It's possible that nobody cared because all drivers for non-PCI
devices on ixp4xx (the on chip ones) just use __raw_readl or
direct pointer references.

> LE data-coherent mode (which has never landed in the
> official kernel) is a bit different, but still, readl()/writel() do the
> right thing.
> I remember the "string" (block) functions preserve 8-bit ordering, and
> thus 32-bit values transfered using them may need swapping.

This is the normal behavior, yes: readsl/writesl should not swap data,
and you should not use them to access registers other than FIFOs
that contain byte-sequential data. If the bus swaps data, then the
implementation of the readsl/writesl function has to swap it back,
as the indirect versions of these do.

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-01-29 16:18     ` Krzysztof Hałasa
  2016-01-29 17:06       ` Arnd Bergmann
@ 2016-01-29 18:03       ` Sergei Shtylyov
  2016-01-29 21:02         ` Arnd Bergmann
  1 sibling, 1 reply; 36+ messages in thread
From: Sergei Shtylyov @ 2016-01-29 18:03 UTC (permalink / raw)
  To: Krzysztof Hałasa, Arnd Bergmann
  Cc: Felipe Balbi, linux-arm-kernel, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Greg Kroah-Hartman

Hello.

On 01/29/2016 07:18 PM, Krzysztof Hałasa wrote:

>> The unclear part here is for IXP4xx, which supports both big-endian
>> and little-endian configurations. So far, the driver has done
>> no byteswap in either case. I suspect that is wrong and it would
>> actually need to swap in one or the other case, but I don't know
>> which.
>
> If at all, I guess it should swap in LE mode. But it's far from certain.
>
>> It's also possible that there is some magic setting in
>> the chip that makes the endianess of the MMIO register match the
>> CPU, and in that case, the code actually does the right thing
>> for all configurations, both before and after this patch.
>
> This is IMHO most probable.
>
> Actually, the IXP4xx is "natural" in BE mode (except for PCI) and
> normally in LE mode it's order-coherent, meaning 32-bit "integer"
> accesses need no swapping, but 8-bit and (mostly unused) 16-bit
> transfers need swapping.
>
> Anyway, I think readl()/writel() do the right thing: in BE mode they
> swap PCI accesses and don't swap normal registers,

    Alas, readl()/writel() don't know what registers you are calling them for, 
they were designed for PCI which is little-endian, so they will swap in BE mode.

MBR, Sergei

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-01-29 18:03       ` Sergei Shtylyov
@ 2016-01-29 21:02         ` Arnd Bergmann
  0 siblings, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-01-29 21:02 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Sergei Shtylyov, Krzysztof Hałasa, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, linux-kernel, Felipe Balbi,
	Haojian Zhuang, Daniel Mack, Imre Kaloz, Robert Jarzmik

On Friday 29 January 2016 21:03:40 Sergei Shtylyov wrote:
> On 01/29/2016 07:18 PM, Krzysztof Hałasa wrote:
> 
> >> The unclear part here is for IXP4xx, which supports both big-endian
> >> and little-endian configurations. So far, the driver has done
> >> no byteswap in either case. I suspect that is wrong and it would
> >> actually need to swap in one or the other case, but I don't know
> >> which.
> >
> > If at all, I guess it should swap in LE mode. But it's far from certain.
> >
> >> It's also possible that there is some magic setting in
> >> the chip that makes the endianess of the MMIO register match the
> >> CPU, and in that case, the code actually does the right thing
> >> for all configurations, both before and after this patch.
> >
> > This is IMHO most probable.
> >
> > Actually, the IXP4xx is "natural" in BE mode (except for PCI) and
> > normally in LE mode it's order-coherent, meaning 32-bit "integer"
> > accesses need no swapping, but 8-bit and (mostly unused) 16-bit
> > transfers need swapping.
> >
> > Anyway, I think readl()/writel() do the right thing: in BE mode they
> > swap PCI accesses and don't swap normal registers,
> 
>     Alas, readl()/writel() don't know what registers you are calling them for, 
> they were designed for PCI which is little-endian, so they will swap in BE mode.
> 

In general, that is correct, but as Krzysztof said, ixp4xx is rather
special here, and it implements its own readl/writel functions
that behave differently for PCI spaces than for on-chip addresses,
See arch/arm/mach-ixp4xx/include/mach/io.h.

This platform evidently does its own tricks with byte swapping so
while a normal big-endian platform has to swap on readl (but not
readsl), ixp4xx never does any swaps when CONFIG_IXP4XX_INDIRECT_PCI
is set (neither readl nor readsl, neither PCI nor on-chip MMU)

However if CONFIG_IXP4XX_INDIRECT_PCI is disabled (which I think
is almost always the case), it basically behaves like all the other
platforms, and in big-endian configurations performsm swaps for
inl/readl/ioread32 but not readsl/ioread32_rep/insl (it swaps
twice for the last one).

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/7] USB changes for rare warnings
  2016-01-28 16:15 [PATCH 0/7] USB changes for rare warnings Arnd Bergmann
                   ` (2 preceding siblings ...)
  2016-01-28 16:23 ` [PATCH 5/7] usb: isp1301-omap: mark power_up as __maybe_unused Arnd Bergmann
@ 2016-02-03 18:12 ` Felipe Balbi
  2016-02-03 19:15   ` Robert Jarzmik
  3 siblings, 1 reply; 36+ messages in thread
From: Felipe Balbi @ 2016-02-03 18:12 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Arnd Bergmann, Felipe Balbi, linux-usb,
	linux-kernel, Robert Jarzmik, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Krzysztof Halasa, Greg Kroah-Hartman

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]


Hi Arnd,

Arnd Bergmann <arnd@arndb.de> writes:
> Hi Felipe,
>
> This set of patches addresses warnings I got in randconfig builds,
> in the USB drivers. The first three patches are for the pxa25x
> UDC driver and are a larger scale cleanup triggered by finding
> the initial bug. The other four are relatively simple but still
> need to be reviewed properly, as I have done only compile-time
> testing. After these patches, I get no other warnings for USB
> drivers at the moment.
>
> Arnd Bergmann (7):
>   usb: gadget: pxa25x_udc: move register definitions from arch
>   usb: gadget: pxa25x_udc cleanup
>   usb: gadget: pxa25x_udc: use readl/writel for mmio
>   usb: fsl: drop USB_FSL_MPH_DR_OF Kconfig symbol
>   usb: isp1301-omap: mark power_up as __maybe_unused
>   usb: musb: use %pad format string from dma_addr_t
>   usb: musb/ux500: remove duplicate check for dma_is_compatible
>
>  arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 198 ---------
>  arch/arm/mach-pxa/include/mach/pxa25x-udc.h     | 163 --------

for arch/arm I need Acked-by from relevant folks.

Also, do you think we need this during the -rc or can we consider it
non-critical fixes for v4.6 merge window ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/7] USB changes for rare warnings
  2016-02-03 18:12 ` [PATCH 0/7] USB changes for rare warnings Felipe Balbi
@ 2016-02-03 19:15   ` Robert Jarzmik
  2016-02-03 20:49     ` Arnd Bergmann
  0 siblings, 1 reply; 36+ messages in thread
From: Robert Jarzmik @ 2016-02-03 19:15 UTC (permalink / raw)
  To: Felipe Balbi, Arnd Bergmann
  Cc: linux-arm-kernel, Felipe Balbi, linux-usb, linux-kernel,
	Haojian Zhuang, Daniel Mack, Imre Kaloz, Krzysztof Halasa,
	Greg Kroah-Hartman

Felipe Balbi <balbi@kernel.org> writes:

> Hi Arnd,
>
>>  arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 198 ---------
>>  arch/arm/mach-pxa/include/mach/pxa25x-udc.h     | 163 --------
>
> for arch/arm I need Acked-by from relevant folks.
You already have mine for arch/arm/mach-pxa/include/mach/pxa25x-udc.h I think,
or did my mailer betray me again ...

I cannot speak for ixp4xx though.

Cheers.

-- 
Robert

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/7] USB changes for rare warnings
  2016-02-03 19:15   ` Robert Jarzmik
@ 2016-02-03 20:49     ` Arnd Bergmann
  0 siblings, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-02-03 20:49 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: Felipe Balbi, linux-arm-kernel, Felipe Balbi, linux-usb,
	linux-kernel, Haojian Zhuang, Daniel Mack, Imre Kaloz,
	Krzysztof Halasa, Greg Kroah-Hartman

On Wednesday 03 February 2016 20:15:47 Robert Jarzmik wrote:
> Felipe Balbi <balbi@kernel.org> writes:
> 
> > Hi Arnd,
> >
> >>  arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 198 ---------
> >>  arch/arm/mach-pxa/include/mach/pxa25x-udc.h     | 163 --------
> >
> > for arch/arm I need Acked-by from relevant folks.
> You already have mine for arch/arm/mach-pxa/include/mach/pxa25x-udc.h I think,
> or did my mailer betray me again ...

I saw it.

> I cannot speak for ixp4xx though.

Krzysztof Halasa <khalasa@piap.pl> gave an Ack for the first patch already,
and I think we concluded that the patch was keeping the current behavior
otherwise and is therefore not a regression, though we did not finish
the side-discussion about whether ixp4xx actually works in both BE and LE
mode with both indirect and direct I/O, or which of the four combinations
is broken.

> > Also, do you think we need this during the -rc or can we consider it
> > non-critical fixes for v4.6 merge window ?

I meant it for 4.6, sorry for not being very clear about that.
All the warnings I fixed in this series are harmless as far as I
can tell, and they only show up in rare randconfig builds.

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-01-29 17:06       ` Arnd Bergmann
@ 2016-02-15  7:33         ` Krzysztof Hałasa
  2016-02-15  9:33           ` Arnd Bergmann
  0 siblings, 1 reply; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-02-15  7:33 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

Arnd Bergmann <arnd@arndb.de> writes:

>> Anyway, I think readl()/writel() do the right thing: in BE mode they
>> swap PCI accesses and don't swap normal registers, in LE mode nothing is
>> swapped.
>
> This seems to be true when CONFIG_IXP4XX_INDIRECT_PCI is set, but
> not otherwise. For the indirect variant, writel() is a __raw_writel()
> without barrier or byteswap on non-PCI memory, while with that
> option disabled, we use the standard implementation that has both
> a byteswap and a barrier.
>
> According to your description, that would mean the version without
> indirect PCI access is broken and it appears to have been that way
> since before the start of git history in 2.6.12.
>
> It's possible that nobody cared because all drivers for non-PCI
> devices on ixp4xx (the on chip ones) just use __raw_readl or
> direct pointer references.

Well, it is possible. I recall I probably used __raw_* instead of
readl()/writel() in qmgr/npe/Ethernet drivers for this very reason.

I think Direct pointer references are only used for locations in RAM
(DMA buffers), they have their own share of problems because the
peripherals are always big endian.

I think I still have an early IXP425 board with UDC connector so I will
try to dig it up and test this code.

I wonder if we should switch the driver to use __raw_*, too. The problem
is almost nobody uses UDC with this CPU.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-15  7:33         ` Krzysztof Hałasa
@ 2016-02-15  9:33           ` Arnd Bergmann
  2016-02-15 13:51             ` Krzysztof Hałasa
  0 siblings, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-02-15  9:33 UTC (permalink / raw)
  To: Krzysztof Hałasa
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

On Monday 15 February 2016 08:33:37 Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> >> Anyway, I think readl()/writel() do the right thing: in BE mode they
> >> swap PCI accesses and don't swap normal registers, in LE mode nothing is
> >> swapped.
> >
> > This seems to be true when CONFIG_IXP4XX_INDIRECT_PCI is set, but
> > not otherwise. For the indirect variant, writel() is a __raw_writel()
> > without barrier or byteswap on non-PCI memory, while with that
> > option disabled, we use the standard implementation that has both
> > a byteswap and a barrier.
> >
> > According to your description, that would mean the version without
> > indirect PCI access is broken and it appears to have been that way
> > since before the start of git history in 2.6.12.
> >
> > It's possible that nobody cared because all drivers for non-PCI
> > devices on ixp4xx (the on chip ones) just use __raw_readl or
> > direct pointer references.
> 
> Well, it is possible. I recall I probably used __raw_* instead of
> readl()/writel() in qmgr/npe/Ethernet drivers for this very reason.
> 
> I think Direct pointer references are only used for locations in RAM
> (DMA buffers), they have their own share of problems because the
> peripherals are always big endian.
> 
> I think I still have an early IXP425 board with UDC connector so I will
> try to dig it up and test this code.
> 
> I wonder if we should switch the driver to use __raw_*, too. The problem
> is almost nobody uses UDC with this CPU.
> 

I consider the use of __raw_* accessors a bug, I don't think we should
ever do that because it hides how the hardware actually works, it doesn't
work with spinlocks, and it can lead to the compiler splitting up accesses
into byte sized ones (not on ARM with the current definition, but
possible in general).

Almost all hardware is fixed-endian, so you have to use swapping accessors
when the CPU is the other way, except for device RAM and FIFO registers
that are always used to transfer a byte stream (see the definition of
readsl() and memcpy_fromio()). When you have hardware that adds byteswaps
on the bus interface, you typically end up with MMIO registers requiring
no swap (or double swap) and readsl()/memcpy_fromio()) suddenly requiring
a swap that is counterintuitive.

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-15  9:33           ` Arnd Bergmann
@ 2016-02-15 13:51             ` Krzysztof Hałasa
  2016-02-15 16:12               ` Arnd Bergmann
  0 siblings, 1 reply; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-02-15 13:51 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

Arnd Bergmann <arnd@arndb.de> writes:

> I consider the use of __raw_* accessors a bug, I don't think we should
> ever do that because it hides how the hardware actually works, it doesn't
> work with spinlocks, and it can lead to the compiler splitting up accesses
> into byte sized ones (not on ARM with the current definition, but
> possible in general).

Well, then maybe we should fix them, or add another set.
Why don't they work with spinlocks?

To be honest, I remember this was already discussed a bit years ago.
I think I proposed back then a set of read_le32 (which would be
equivalent of current readl(), and could be named pci_readl() as well),
read_be32, read_host (without swapping).
The names could be better, though.

> Almost all hardware is fixed-endian, so you have to use swapping accessors
> when the CPU is the other way, except for device RAM and FIFO registers
> that are always used to transfer a byte stream (see the definition of
> readsl() and memcpy_fromio()). When you have hardware that adds byteswaps
> on the bus interface, you typically end up with MMIO registers requiring
> no swap (or double swap) and readsl()/memcpy_fromio()) suddenly requiring
> a swap that is counterintuitive.

Sure, but the __raw_* are used just to be sure there is absolutely no
swapping.
E.g. for IXP4xx, the registers never require swapping, thus readl() etc.
are not suitable for this. What is needed here is simple "atomic" 32-bit
straight to/from register (MM)IO (assuming 4-byte address alignment).

If not __raw_* then what?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-15 13:51             ` Krzysztof Hałasa
@ 2016-02-15 16:12               ` Arnd Bergmann
  2016-02-16  9:26                 ` Krzysztof Hałasa
  0 siblings, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-02-15 16:12 UTC (permalink / raw)
  To: Krzysztof Hałasa
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

On Monday 15 February 2016 14:51:13 Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > I consider the use of __raw_* accessors a bug, I don't think we should
> > ever do that because it hides how the hardware actually works, it doesn't
> > work with spinlocks, and it can lead to the compiler splitting up accesses
> > into byte sized ones (not on ARM with the current definition, but
> > possible in general).
> 
> Well, then maybe we should fix them, or add another set.
> Why don't they work with spinlocks?

The barriers on a spinlock synchronize between CPUs but not an external
bus, so (on some architectures) a spinlock protecting an MMIO register
does not guarantee that two CPUs doing

	spin_lock();
	__raw_writel(address);
	__raw_writel(data);
	spin_unlock();

would cause pairs of address/data to be seen on the bus.

Of course this is meaningless on ixp4xx, as there is only one CPU.

> To be honest, I remember this was already discussed a bit years ago.
> I think I proposed back then a set of read_le32 (which would be
> equivalent of current readl(), and could be named pci_readl() as well),
> read_be32, read_host (without swapping).
> The names could be better, though.

It keeps coming back, but so far the status quo has been stronger,
any it generally works for portable code either hardcoding whichever
endianess the device has, or using runtime detection when the same
device can be used in various ways (e.g. USB ehci).

On powerpc, we have in_le32/in_be32 for SoC-internal register access,
while only PCI devices are allowed to be accessed using readl().

> > Almost all hardware is fixed-endian, so you have to use swapping accessors
> > when the CPU is the other way, except for device RAM and FIFO registers
> > that are always used to transfer a byte stream (see the definition of
> > readsl() and memcpy_fromio()). When you have hardware that adds byteswaps
> > on the bus interface, you typically end up with MMIO registers requiring
> > no swap (or double swap) and readsl()/memcpy_fromio()) suddenly requiring
> > a swap that is counterintuitive.
> 
> Sure, but the __raw_* are used just to be sure there is absolutely no
> swapping.
> E.g. for IXP4xx, the registers never require swapping, thus readl() etc.
> are not suitable for this. What is needed here is simple "atomic" 32-bit
> straight to/from register (MM)IO (assuming 4-byte address alignment).
> 
> If not __raw_* then what?

I would suggest using an ixp4xx specific set of accessors that comes down
to either readl() or ioread32_be(), depending on whether CONFIG_CPU_BIG_ENDIAN
is set. That makes it clear that there is a magic bus involved and that it
works on this platform but not in portable code.

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-15 16:12               ` Arnd Bergmann
@ 2016-02-16  9:26                 ` Krzysztof Hałasa
  2016-02-16 11:26                   ` Arnd Bergmann
  0 siblings, 1 reply; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-02-16  9:26 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

Arnd Bergmann <arnd@arndb.de> writes:

> The barriers on a spinlock synchronize between CPUs but not an external
> bus, so (on some architectures) a spinlock protecting an MMIO register
> does not guarantee that two CPUs doing
>
> 	spin_lock();
> 	__raw_writel(address);
> 	__raw_writel(data);
> 	spin_unlock();
>
> would cause pairs of address/data to be seen on the bus.
>
> Of course this is meaningless on ixp4xx, as there is only one CPU.

I still don't get it. If the spinlocks synchronize between CPUs, there
can only be one CPU (or core) doing the pair of raw_writel(), so how
would it be possible to not get the address/data pair written out?
IOW, how is it different from a system with a single CPU?

> On powerpc, we have in_le32/in_be32 for SoC-internal register access,
> while only PCI devices are allowed to be accessed using readl().

Yeah, this seems like a sane solution.

> I would suggest using an ixp4xx specific set of accessors that comes down
> to either readl() or ioread32_be(), depending on whether CONFIG_CPU_BIG_ENDIAN
> is set. That makes it clear that there is a magic bus involved and that it
> works on this platform but not in portable code.

Hmm. This is actually the opposite - while there may be some magic
(swapping) in readl() and friends, there is absolutely no magic in the
__raw_readl() etc. They are essentially equivalent to
*(volatile u32 *)ptr. This is constant and doesn't depend on endianess,
PCI, anything.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-16  9:26                 ` Krzysztof Hałasa
@ 2016-02-16 11:26                   ` Arnd Bergmann
  2016-02-16 13:24                     ` Krzysztof Hałasa
  0 siblings, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-02-16 11:26 UTC (permalink / raw)
  To: Krzysztof Hałasa
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

On Tuesday 16 February 2016 10:26:14 Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > The barriers on a spinlock synchronize between CPUs but not an external
> > bus, so (on some architectures) a spinlock protecting an MMIO register
> > does not guarantee that two CPUs doing
> >
> >       spin_lock();
> >       __raw_writel(address);
> >       __raw_writel(data);
> >       spin_unlock();
> >
> > would cause pairs of address/data to be seen on the bus.
> >
> > Of course this is meaningless on ixp4xx, as there is only one CPU.
> 
> I still don't get it. If the spinlocks synchronize between CPUs, there
> can only be one CPU (or core) doing the pair of raw_writel(), so how
> would it be possible to not get the address/data pair written out?
> IOW, how is it different from a system with a single CPU?

Both writes leave the CPU core within the spinlock but are not serialized
with anything else, so there is no ordering between the CPUs when they
enter the shared bus, other than having address before data. You'd
expect to see address0, data0, address1, data1, but it could also
be e.g. address0, address1, data1, data0.

> > On powerpc, we have in_le32/in_be32 for SoC-internal register access,
> > while only PCI devices are allowed to be accessed using readl().
> 
> Yeah, this seems like a sane solution.
> 
> > I would suggest using an ixp4xx specific set of accessors that comes down
> > to either readl() or ioread32_be(), depending on whether CONFIG_CPU_BIG_ENDIAN
> > is set. That makes it clear that there is a magic bus involved and that it
> > works on this platform but not in portable code.
> 
> Hmm. This is actually the opposite - while there may be some magic
> (swapping) in readl() and friends, there is absolutely no magic in the
> __raw_readl() etc. They are essentially equivalent to
> *(volatile u32 *)ptr. This is constant and doesn't depend on endianess,
> PCI, anything.

The point is more what the common case is. Almost all machines we support
have fixed-endian devices, and the drivers are correct when using readl()
or in_le32() but are not endian-safe when using __raw_readl().

Using __raw_readl() has the big danger of someone accidentally "fixing"
the driver to work like all the others in order to solve a theoretical
endian problem, while it should really be made obvious that the hardware
actively swaps its data on the bus.

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-16 11:26                   ` Arnd Bergmann
@ 2016-02-16 13:24                     ` Krzysztof Hałasa
  2016-02-16 13:55                       ` Arnd Bergmann
  0 siblings, 1 reply; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-02-16 13:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

Arnd Bergmann <arnd@arndb.de> writes:

> Both writes leave the CPU core within the spinlock but are not serialized
> with anything else, so there is no ordering between the CPUs when they
> enter the shared bus, other than having address before data. You'd
> expect to see address0, data0, address1, data1, but it could also
> be e.g. address0, address1, data1, data0.

Ah, so it's a matter of flushing the write buffers before exiting the
spinlock-protected code.

> The point is more what the common case is. Almost all machines we support
> have fixed-endian devices, and the drivers are correct when using readl()
> or in_le32() but are not endian-safe when using __raw_readl().

Sure, e.g. PCI does it this way (eventually swapping the data lanes if
needed).

> Using __raw_readl() has the big danger of someone accidentally "fixing"
> the driver to work like all the others in order to solve a theoretical
> endian problem, while it should really be made obvious that the hardware
> actively swaps its data on the bus.

Sure - if this is the case. On-chip IXP4xx peripherals don't swap data
at all (i.e., they match CPU endianess) - accessing their registers is
like accessing a normal CPU register. That's why they don't use
PCI-style readl() etc. - however a better name than __raw_* would
probably help here.

Using __raw_* in a PCI driver would be generally wrong.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-16 13:24                     ` Krzysztof Hałasa
@ 2016-02-16 13:55                       ` Arnd Bergmann
  2016-02-17  8:36                         ` Krzysztof Hałasa
  2016-02-20 20:54                         ` Robert Jarzmik
  0 siblings, 2 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-02-16 13:55 UTC (permalink / raw)
  To: Krzysztof Hałasa
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

On Tuesday 16 February 2016 14:24:10 Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > Both writes leave the CPU core within the spinlock but are not serialized
> > with anything else, so there is no ordering between the CPUs when they
> > enter the shared bus, other than having address before data. You'd
> > expect to see address0, data0, address1, data1, but it could also
> > be e.g. address0, address1, data1, data0.
> 
> Ah, so it's a matter of flushing the write buffers before exiting the
> spinlock-protected code.

Yes, whatever the specific architecture requires. The normal readl()
functions are documented to having all the required barriers and
flushes, the __raw_* variants may (e.g. on x86) or may not have that.

> > The point is more what the common case is. Almost all machines we support
> > have fixed-endian devices, and the drivers are correct when using readl()
> > or in_le32() but are not endian-safe when using __raw_readl().
> 
> Sure, e.g. PCI does it this way (eventually swapping the data lanes if
> needed).
> 
> > Using __raw_readl() has the big danger of someone accidentally "fixing"
> > the driver to work like all the others in order to solve a theoretical
> > endian problem, while it should really be made obvious that the hardware
> > actively swaps its data on the bus.
> 
> Sure - if this is the case. On-chip IXP4xx peripherals don't swap data
> at all (i.e., they match CPU endianess) - accessing their registers is
> like accessing a normal CPU register. That's why they don't use
> PCI-style readl() etc. - however a better name than __raw_* would
> probably help here.
> 
> Using __raw_* in a PCI driver would be generally wrong.

I was really talking about built-in devices here. There are other platforms
that have an internal byteswap logic on the bus interface and they also
use that for PCI, see e.g. arch/sh/include/mach-common/mach/mangle-port.h.

ixp4xx is really special in that it performs hardware swapping for
internal devices based on CPU endianess but not on PCI devices.

I think some Broadcom MIPS systems are in the same category as IXP4xx,
but only because they have an extra byteswap on the PCI bus that they
can turn on, but very few other systems are.

Coming back to the specific pxa25x_udc case: using __raw_* accessors
in the driver would possibly end up breaking the PXA25x machines in
the (very unlikely) case that someone wants to make it work with
big-endian kernels, assuming it does not have the same hardware
byteswap logic as ixp4xx.

	Arnd

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-16 13:55                       ` Arnd Bergmann
@ 2016-02-17  8:36                         ` Krzysztof Hałasa
  2016-02-17 10:36                           ` Arnd Bergmann
  2016-02-20 20:54                         ` Robert Jarzmik
  1 sibling, 1 reply; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-02-17  8:36 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

Arnd Bergmann <arnd@arndb.de> writes:

> ixp4xx is really special in that it performs hardware swapping for
> internal devices based on CPU endianess but not on PCI devices.

Again, IXP4xx does not perform hardware (nor any other) swapping for
registers of on-chip devices. The registers are connected 1:1,
bit 0 to bit 0 etc.

(Yes, IXP4xx can be optionally programmed for such swapping, depending
on silicon revision, but it is not used in mainline kernel).

The only hardware swapping happens on PCI bus (in BE mode), to be
compatible with other platforms and non-IXP4xx-specific PCI drivers.

> Coming back to the specific pxa25x_udc case: using __raw_* accessors
> in the driver would possibly end up breaking the PXA25x machines in
> the (very unlikely) case that someone wants to make it work with
> big-endian kernels, assuming it does not have the same hardware
> byteswap logic as ixp4xx.

I'd expect both CPUs to behave in exactly the same manner, i.e., to
not swap anything on the internal bus. If true, it would mean it should
"just work" in both BE and LE modes (including BE mode on PXA, should
it be actually possible).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-17  8:36                         ` Krzysztof Hałasa
@ 2016-02-17 10:36                           ` Arnd Bergmann
  2016-02-17 16:14                             ` Krzysztof Hałasa
  0 siblings, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2016-02-17 10:36 UTC (permalink / raw)
  To: Krzysztof Hałasa
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

On Wednesday 17 February 2016 09:36:37 Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > ixp4xx is really special in that it performs hardware swapping for
> > internal devices based on CPU endianess but not on PCI devices.
> 
> Again, IXP4xx does not perform hardware (nor any other) swapping for
> registers of on-chip devices. The registers are connected 1:1,
> bit 0 to bit 0 etc.
> 
> (Yes, IXP4xx can be optionally programmed for such swapping, depending
> on silicon revision, but it is not used in mainline kernel).
> 
> The only hardware swapping happens on PCI bus (in BE mode), to be
> compatible with other platforms and non-IXP4xx-specific PCI drivers.

Ok, so I guess what this means is that ixp4xx (or xscale in general)
implements its big-endian mode by adding a byteswap on its DRAM
and PCI interfaces in be32 mode, rather than by changing the behavior of
the load/store operations (as be8 mode does) or by having the byteswap
in its load/store pipeline or the top-level AHB bridge?

It's a bit surprising but it sounds plausible and explains most of
the code we see.

I'm still unsure about __indirect_readsl()/ioread32_rep()/insl()/readsl().

insl() does a double-swap on big-endian, which seems right, as we
end up with four swaps total, preserving correct byte order.

__raw_readsl() performs no swap, which would be correct for PCI
(same swap on PCI and RAM, so byteorder is preserved), but wrong
for on-chip FIFO registers (one swap on RAM, no swap on MMIO).
This is probably fine as well, as I don't see any use of __raw_readsl()
on non-PCI devices.

However, when CONFIG_IXP4XX_INDIRECT_PCI is set, both
ioread32_rep() and readsl() call __indirect_readsl(), which
in turn swaps the data once, so I think we actually need this patch:

diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h b/arch/arm/mach-ixp4xx/include/mach/io.h
index e770858b490a..871f92f3504a 100644
--- a/arch/arm/mach-ixp4xx/include/mach/io.h
+++ b/arch/arm/mach-ixp4xx/include/mach/io.h
@@ -85,6 +85,8 @@ u8 __indirect_readb(const volatile void __iomem *p);
 u16 __indirect_readw(const volatile void __iomem *p);
 u32 __indirect_readl(const volatile void __iomem *p);
 
+/* string functions may need to swap data back to revert the byte swap on
+   big-endian __indirect_{read,write}{w,l}() accesses */
 static inline void __indirect_writesb(volatile void __iomem *bus_addr,
 				      const void *p, int count)
 {
@@ -100,7 +102,7 @@ static inline void __indirect_writesw(volatile void __iomem *bus_addr,
 	const u16 *vaddr = p;
 
 	while (count--)
-		writew(*vaddr++, bus_addr);
+		writew((u16 __force)cpu_to_le32(*vaddr++), bus_addr);
 }
 
 static inline void __indirect_writesl(volatile void __iomem *bus_addr,
@@ -108,7 +110,7 @@ static inline void __indirect_writesl(volatile void __iomem *bus_addr,
 {
 	const u32 *vaddr = p;
 	while (count--)
-		writel(*vaddr++, bus_addr);
+		writel((u32 __force)cpu_to_le32(*vaddr++), bus_addr);
 }
 
 static inline void __indirect_readsb(const volatile void __iomem *bus_addr,
@@ -126,7 +128,7 @@ static inline void __indirect_readsw(const volatile void __iomem *bus_addr,
 	u16 *vaddr = p;
 
 	while (count--)
-		*vaddr++ = readw(bus_addr);
+		*vaddr++ = (u16 __force)cpu_to_le16(readw(bus_addr));
 }
 
 static inline void __indirect_readsl(const volatile void __iomem *bus_addr,
@@ -135,7 +137,7 @@ static inline void __indirect_readsl(const volatile void __iomem *bus_addr,
 	u32 *vaddr = p;
 
 	while (count--)
-		*vaddr++ = readl(bus_addr);
+		*vaddr++ = (u32 __force)cpu_to_le32(readl(bus_addr));
 }
 
Does that make sense to you? This is essentially the same thing we already
do for inw/inl/outw/outl.

> > Coming back to the specific pxa25x_udc case: using __raw_* accessors
> > in the driver would possibly end up breaking the PXA25x machines in
> > the (very unlikely) case that someone wants to make it work with
> > big-endian kernels, assuming it does not have the same hardware
> > byteswap logic as ixp4xx.
> 
> I'd expect both CPUs to behave in exactly the same manner, i.e., to
> not swap anything on the internal bus. If true, it would mean it should
> "just work" in both BE and LE modes (including BE mode on PXA, should
> it be actually possible).

Ok, I'll change the patch accordingly and resubmit.

	Arnd

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
  2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
                     ` (3 preceding siblings ...)
  2016-01-29 15:55   ` Krzysztof Hałasa
@ 2016-02-17 15:08   ` Felipe Balbi
  2016-02-17 16:00     ` Arnd Bergmann
  4 siblings, 1 reply; 36+ messages in thread
From: Felipe Balbi @ 2016-02-17 15:08 UTC (permalink / raw)
  To: Arnd Bergmann, Imre Kaloz, Krzysztof Halasa, Russell King,
	Daniel Mack, Haojian Zhuang, Robert Jarzmik, Felipe Balbi,
	Greg Kroah-Hartman
  Cc: linux-arm-kernel, Arnd Bergmann, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]


Hi,

Arnd Bergmann <arnd@arndb.de> writes:
> ixp4xx and pxa25x both use this driver and provide a slightly
> different set of register definitions for it. Aside from that,
> the definition in the ixp4xx-regs.h header conflicts with the
> on in the pxa27x device driver when compile-testing that:
>
> In file included from ../drivers/usb/gadget/udc/pxa27x_udc.c:37:0:
> ../drivers/usb/gadget/udc/pxa27x_udc.h:26:0: warning: "UDCCR" redefined
>  #define UDCCR  0x0000  /* UDC Control Register */
>  ^
> In file included from ../arch/arm/mach-ixp4xx/include/mach/hardware.h:27:0,
>                  from ../arch/arm/mach-ixp4xx/include/mach/io.h:18,
>                  from ../arch/arm/include/asm/io.h:194,
>                  from ../include/linux/io.h:25,
>                  from ../include/linux/irq.h:24,
>                  from ../drivers/usb/gadget/udc/pxa27x_udc.c:23:
> ../arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h:415:0: note: this is the location of the previous definition
>  #define UDCCR  IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0000)
>
> This addresses both issues by moving all the definitions into the
> pxa25x_udc driver itself. It turns out the only difference between
> them was 'UDCCS_IO_ROF', and that could well be a mistake when it
> was incorrectly copied from pxa25x to ixp4xx.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

FYI, this series now sits in my testing/next. If you could just check
that I didn't mess anything up, I'd be glad.

Thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
  2016-02-17 15:08   ` Felipe Balbi
@ 2016-02-17 16:00     ` Arnd Bergmann
  0 siblings, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2016-02-17 16:00 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Felipe Balbi, Imre Kaloz, Krzysztof Halasa, Russell King,
	Daniel Mack, Haojian Zhuang, Robert Jarzmik, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, linux-kernel

On Wednesday 17 February 2016 17:08:27 Felipe Balbi wrote:
> 
> Hi,
> 
> Arnd Bergmann <arnd@arndb.de> writes:
> > ixp4xx and pxa25x both use this driver and provide a slightly
> > different set of register definitions for it. Aside from that,
> > the definition in the ixp4xx-regs.h header conflicts with the
> > on in the pxa27x device driver when compile-testing that:
> >
> > In file included from ../drivers/usb/gadget/udc/pxa27x_udc.c:37:0:
> > ../drivers/usb/gadget/udc/pxa27x_udc.h:26:0: warning: "UDCCR" redefined
> >  #define UDCCR  0x0000  /* UDC Control Register */
> >  ^
> > In file included from ../arch/arm/mach-ixp4xx/include/mach/hardware.h:27:0,
> >                  from ../arch/arm/mach-ixp4xx/include/mach/io.h:18,
> >                  from ../arch/arm/include/asm/io.h:194,
> >                  from ../include/linux/io.h:25,
> >                  from ../include/linux/irq.h:24,
> >                  from ../drivers/usb/gadget/udc/pxa27x_udc.c:23:
> > ../arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h:415:0: note: this is the location of the previous definition
> >  #define UDCCR  IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x0000)
> >
> > This addresses both issues by moving all the definitions into the
> > pxa25x_udc driver itself. It turns out the only difference between
> > them was 'UDCCS_IO_ROF', and that could well be a mistake when it
> > was incorrectly copied from pxa25x to ixp4xx.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> FYI, this series now sits in my testing/next. If you could just check
> that I didn't mess anything up, I'd be glad.
> 

Thank you for merging this and my other patches!

After the latest discussion with Krzysztof, I think it would be good
to include the patch below, either on top or folded into the last
patch of the series (whichever fits your workflow).

	Arnd
 
8<----
>From 3feea5e42eae444e122f3ad51fef9e08d758fc27 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 17 Feb 2016 16:51:40 +0100
Subject: [PATCH] usb: gadget: pxa25x_udc: document endianess better
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When I wrote the cleanup patch series, it was not clear how
exactly big-endian mode works on ixp4xx, and whether the driver
was doing this correctly. After discussing with Krzysztof Hałasa,
this has been clarified, so I can update the comment let pxa25x
big-endian (which we don't support) work the same way as ixp4xx.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c b/drivers/usb/gadget/udc/pxa25x_udc.c
index ca7abdc0eca9..33b7fb84f4fb 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.c
+++ b/drivers/usb/gadget/udc/pxa25x_udc.c
@@ -289,14 +289,14 @@ static void pullup_on(void)
 		mach->udc_command(PXA2XX_UDC_CMD_CONNECT);
 }
 
-#if defined(CONFIG_ARCH_IXP4XX) && defined(CONFIG_CPU_BIG_ENDIAN)
+#if defined(CONFIG_CPU_BIG_ENDIAN)
 /*
- * not sure if this is the correct behavior on ixp4xx in both
- * bit-endian and little-endian modes, but it's what the driver
- * has always done using direct pointer dereferences:
- * We assume that there is a byteswap done in hardware at the
- * MMIO register that matches what the CPU setting is, so we
- * never swap in software.
+ * IXP4xx has its buses wired up in a way that relies on never doing any
+ * byte swaps, independent of whether it runs in big-endian or little-endian
+ * mode, as explained by Krzysztof Hałasa.
+ *
+ * We only support pxa25x in little-endian mode, but it is very likely
+ * that it works the same way.
  */
 static inline void udc_set_reg(struct pxa25x_udc *dev, u32 reg, u32 val)
 {

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-17 10:36                           ` Arnd Bergmann
@ 2016-02-17 16:14                             ` Krzysztof Hałasa
  0 siblings, 0 replies; 36+ messages in thread
From: Krzysztof Hałasa @ 2016-02-17 16:14 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Felipe Balbi, Haojian Zhuang, Daniel Mack,
	Imre Kaloz, Robert Jarzmik

Arnd Bergmann <arnd@arndb.de> writes:

> Ok, so I guess what this means is that ixp4xx (or xscale in general)
> implements its big-endian mode by adding a byteswap on its DRAM
> and PCI interfaces in be32 mode, rather than by changing the behavior of
> the load/store operations (as be8 mode does) or by having the byteswap
> in its load/store pipeline or the top-level AHB bridge?

Hmm... IIRC, there is normally no swapping on DRAM bus. E.g. if you
write 0x12345678 to RAM and change endianness, it will still read as
0x12345678. The CPU will still be able to execute opcodes after
switching endianness, but byte-oriented data will be messed up.

PCI swaps in BE mode, so byte order is preserved but readl() must
"unswap" it.

> I'm still unsure about
> __indirect_readsl()/ioread32_rep()/insl()/readsl().

Indirect ops should behave the same as direct. I think I have tested
them at some point. The "string" operations don't have to swap (on PCI)
because the PCI bus controller does it for them (in BE mode).

> insl() does a double-swap on big-endian, which seems right, as we
> end up with four swaps total, preserving correct byte order.

static inline void insl(u32 io_addr, void *p, u32 count)
{
        u32 *vaddr = p;
        while (count--)
                *vaddr++ = le32_to_cpu(inl(io_addr));
}

inl() does indirect input (preserving value, not byte order), so there seem
to be just one swap here (le32_to_cpu) preserving byte order.

> __raw_readsl() performs no swap, which would be correct for PCI
> (same swap on PCI and RAM, so byteorder is preserved),

No, a single swap on PCI, this means the byte order is preserved :-)

> but wrong
> for on-chip FIFO registers (one swap on RAM, no swap on MMIO).

But there aren't any such registers. Basically, almost all registers are
32-bit, even if they only hold an 8-bit value.
Exceptions such as 16550 UARTs are taken care of in platform structs
(using offset = 3).

> However, when CONFIG_IXP4XX_INDIRECT_PCI is set, both
> ioread32_rep() and readsl() call __indirect_readsl(), which
> in turn swaps the data once, so I think we actually need this patch:
>
> diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h b/arch/arm/mach-ixp4xx/include/mach/io.h

> @@ -100,7 +102,7 @@ static inline void __indirect_writesw(volatile void __iomem *bus_addr,
>  	const u16 *vaddr = p;
>  
>  	while (count--)
> -		writew(*vaddr++, bus_addr);
> +		writew((u16 __force)cpu_to_le32(*vaddr++), bus_addr);
>  }

...

> Does that make sense to you? This is essentially the same thing we already
> do for inw/inl/outw/outl.

Well, we may need something like this. It seems writesw() (and thus
__indirect_writesw()) etc. use le16 values (preserving byte order), so
the above should probably use le16_to_cpu() instead (and le32_to_cpu in
__indirect_writesl()). I think the only thing that can use it on my hw
is VIA PATA adapter (throught ioread32_rep() etc). I will have to dig it
up as well.
I wouldn't rather touch this stuff without verifying that it fixes
things up.


Thanks for looking into this.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
  2016-02-16 13:55                       ` Arnd Bergmann
  2016-02-17  8:36                         ` Krzysztof Hałasa
@ 2016-02-20 20:54                         ` Robert Jarzmik
  1 sibling, 0 replies; 36+ messages in thread
From: Robert Jarzmik @ 2016-02-20 20:54 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Krzysztof Hałasa, linux-arm-kernel, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, linux-kernel, Felipe Balbi,
	Haojian Zhuang, Daniel Mack, Imre Kaloz

Arnd Bergmann <arnd@arndb.de> writes:

> Coming back to the specific pxa25x_udc case: using __raw_* accessors
> in the driver would possibly end up breaking the PXA25x machines in
> the (very unlikely) case that someone wants to make it work with
> big-endian kernels, assuming it does not have the same hardware
> byteswap logic as ixp4xx.

As far as I know, pxa25x machine implies the kernel is little endian. From an
hardware perspective, pxa25x doesn't support big endian (old ARM platform).

So I don't think considering BE for pxa25x is ... necessary.

Cheers.

-- 
Robert

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2016-02-20 20:54 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-28 16:15 [PATCH 0/7] USB changes for rare warnings Arnd Bergmann
2016-01-28 16:17 ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Arnd Bergmann
2016-01-28 16:17   ` [PATCH 2/7] usb: gadget: pxa25x_udc cleanup Arnd Bergmann
2016-01-29 10:13     ` Robert Jarzmik
2016-01-28 16:17   ` [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann
2016-01-29 10:17     ` Robert Jarzmik
2016-01-29 16:18     ` Krzysztof Hałasa
2016-01-29 17:06       ` Arnd Bergmann
2016-02-15  7:33         ` Krzysztof Hałasa
2016-02-15  9:33           ` Arnd Bergmann
2016-02-15 13:51             ` Krzysztof Hałasa
2016-02-15 16:12               ` Arnd Bergmann
2016-02-16  9:26                 ` Krzysztof Hałasa
2016-02-16 11:26                   ` Arnd Bergmann
2016-02-16 13:24                     ` Krzysztof Hałasa
2016-02-16 13:55                       ` Arnd Bergmann
2016-02-17  8:36                         ` Krzysztof Hałasa
2016-02-17 10:36                           ` Arnd Bergmann
2016-02-17 16:14                             ` Krzysztof Hałasa
2016-02-20 20:54                         ` Robert Jarzmik
2016-01-29 18:03       ` Sergei Shtylyov
2016-01-29 21:02         ` Arnd Bergmann
2016-01-29  9:32   ` [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch Robert Jarzmik
2016-01-29 10:07     ` Arnd Bergmann
2016-01-29 15:26       ` Robert Jarzmik
2016-01-29 15:55   ` Krzysztof Hałasa
2016-02-17 15:08   ` Felipe Balbi
2016-02-17 16:00     ` Arnd Bergmann
2016-01-28 16:20 ` [PATCH 4/7] usb: fsl: drop USB_FSL_MPH_DR_OF Kconfig symbol Arnd Bergmann
2016-01-28 16:23 ` [PATCH 5/7] usb: isp1301-omap: mark power_up as __maybe_unused Arnd Bergmann
2016-01-28 16:23   ` [PATCH 6/7] usb: musb: use %pad format string from dma_addr_t Arnd Bergmann
2016-01-28 17:50     ` Tony Lindgren
2016-01-28 16:23   ` [PATCH 7/7] usb: musb/ux500: remove duplicate check for dma_is_compatible Arnd Bergmann
2016-02-03 18:12 ` [PATCH 0/7] USB changes for rare warnings Felipe Balbi
2016-02-03 19:15   ` Robert Jarzmik
2016-02-03 20:49     ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).