Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320832 > unrolled thread

[PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann <arnd@arndb.de> - 2016-01-28 17:20 +0100
    Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Robert Jarzmik <robert.jarzmik@free.fr> - 2016-01-29 11:20 +0100
    Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio khalasa@piap.pl (Krzysztof Hałasa) - 2016-01-29 17:20 +0100
      Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann <arnd@arndb.de> - 2016-01-29 18:10 +0100
      Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-01-29 19:10 +0100
        Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio Arnd Bergmann <arnd@arndb.de> - 2016-01-29 22:10 +0100

#1320832 — [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio

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

[toc] | [next] | [standalone]

#1321629

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

[toc] | [prev] | [next] | [standalone]

#1321857

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

[toc] | [prev] | [next] | [standalone]

#1321891

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

[toc] | [prev] | [next] | [standalone]

#1321942

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

[toc] | [prev] | [next] | [standalone]

#1322070

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web