[U-Boot] [RESEND] drivers/usb/ehci: Use platform-specific accessors

[U-Boot] [RESEND] drivers/usb/ehci: Use platform-specific accessors

[Alexey Brodkin]

Hi Marek, Vladimir,

On Sun, 2017-10-29 at 11:00 +0100, Marek Vasut wrote:
> On 10/27/2017 02:42 PM, Vladimir Boroda wrote:
> > 
> > Alexey/Marek,
> > 
> > It appears that the "drivers/usb/ehci: Use platform-specific accessors"
> > patch that was submitted in June (and currently adopted in U-Boot
> > releases) kills USB EHCI functionality on PowerPC and likely all other
> > big-endian platforms.  The readl() and writel() accessors already
> > perform endian port to cpu conversion, so no extra conversion was
> > necessary.  The double conversion caused incorrect reading of USB EHCI
> > registers.
> 
> Give Alexey a few days, I've met him at a conference a few days ago so
> he's probably still traveling.

Yep indeed I was recovering after the last trip.

> > I can propose a patch to fix this issue, currently not sure how to
> > submit it for U-Boot approval.  Or you may want to pull the previous
> > patch (or replace the readl() and writel() with some endian-agnostic
> > register reading functions).

Indeed I do see a problem with existing implementation of ehci_{readl|writel}.
Could you please try the following fix which I believe is right now:
------------------------------------->8----------------------------------
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 7c39becd247e..7080ae8bded2 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -101,11 +101,11 @@ struct usb_linux_config_descriptor {
 } __attribute__ ((packed));
 
 #if defined CONFIG_EHCI_DESC_BIG_ENDIAN
-#define ehci_readl(x)          cpu_to_be32(readl(x))
-#define ehci_writel(a, b)      writel(cpu_to_be32(b), a)
+#define ehci_readl(x)          be32_to_cpu(__raw_readl(x))
+#define ehci_writel(a, b)      __raw_writel(cpu_to_be32(a), b)
 #else
-#define ehci_readl(x)          cpu_to_le32(readl(x))
-#define ehci_writel(a, b)      writel(cpu_to_le32(b), a)
+#define ehci_readl(x)          readl(x)
+#define ehci_writel(a, b)      writel(b, a)
 #endif
 
 #if defined CONFIG_EHCI_MMIO_BIG_ENDIAN
------------------------------------->8----------------------------------

I don't have BE platform handy now so your "Tested-by" will be very
nice to have. On LE platform the change above doesn't cause any problems
[as expected] :)

-Alexey

[U-Boot] [RESEND] drivers/usb/ehci: Use platform-specific accessors

[Vladimir Boroda]

Alexey,
I tested the patch on PowerPC.  Seems to work fine, but you have to make sure to set the proper order of arguments for when the EHCI registers are big endian:  __raw_writel(cpu_to_be32(b), a)
To test this patch I had to verify that __raw_ functions are supported for PowerPC, so I created another patch, more in line with your original intent.  Here it is:------------------------------------------------------diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 7c39bec..aeb9745 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -101,11 +101,15 @@ struct usb_linux_config_descriptor {
 } __attribute__ ((packed));

 #if defined CONFIG_EHCI_DESC_BIG_ENDIAN
-#define ehci_readl(x)          cpu_to_be32(readl(x))
-#define ehci_writel(a, b)      writel(cpu_to_be32(b), a)
+/*
+ * On big-endian platforms readl() and writel() perform automatic conversion
+ * from and to little endian.
+ */
+#define ehci_readl(x)          be32_to_cpu(__raw_readl(x))
+#define ehci_writel(a, b)      __raw_writel(cpu_to_be32(b), a)
 #else
-#define ehci_readl(x)          cpu_to_le32(readl(x))
-#define ehci_writel(a, b)      writel(cpu_to_le32(b), a)
+#define ehci_readl(x)          le32_to_cpu(__raw_readl(x))
+#define ehci_writel(a, b)      __raw_writel(cpu_to_le32(b), a)
 #endif

 #if defined CONFIG_EHCI_MMIO_BIG_ENDIAN
------------------------------------------------------Thanks,Vladimir
 

    On Monday, October 30, 2017, 7:49:02 AM EDT, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:  
 
 Hi Marek, Vladimir,

On Sun, 2017-10-29 at 11:00 +0100, Marek Vasut wrote:
> On 10/27/2017 02:42 PM, Vladimir Boroda wrote:
> > 
> > Alexey/Marek,
> > 
> > It appears that the "drivers/usb/ehci: Use platform-specific accessors"
> > patch that was submitted in June (and currently adopted in U-Boot
> > releases) kills USB EHCI functionality on PowerPC and likely all other
> > big-endian platforms.  The readl() and writel() accessors already
> > perform endian port to cpu conversion, so no extra conversion was
> > necessary.  The double conversion caused incorrect reading of USB EHCI
> > registers.
> 
> Give Alexey a few days, I've met him at a conference a few days ago so
> he's probably still traveling.

Yep indeed I was recovering after the last trip.

> > I can propose a patch to fix this issue, currently not sure how to
> > submit it for U-Boot approval.  Or you may want to pull the previous
> > patch (or replace the readl() and writel() with some endian-agnostic
> > register reading functions).

Indeed I do see a problem with existing implementation of ehci_{readl|writel}.
Could you please try the following fix which I believe is right now:
------------------------------------->8----------------------------------
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 7c39becd247e..7080ae8bded2 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -101,11 +101,11 @@ struct usb_linux_config_descriptor {
 } __attribute__ ((packed));
 
 #if defined CONFIG_EHCI_DESC_BIG_ENDIAN
-#define ehci_readl(x)          cpu_to_be32(readl(x))
-#define ehci_writel(a, b)      writel(cpu_to_be32(b), a)
+#define ehci_readl(x)          be32_to_cpu(__raw_readl(x))
+#define ehci_writel(a, b)      __raw_writel(cpu_to_be32(a), b)
 #else
-#define ehci_readl(x)          cpu_to_le32(readl(x))
-#define ehci_writel(a, b)      writel(cpu_to_le32(b), a)
+#define ehci_readl(x)          readl(x)
+#define ehci_writel(a, b)      writel(b, a)
 #endif
 
 #if defined CONFIG_EHCI_MMIO_BIG_ENDIAN
------------------------------------->8----------------------------------

I don't have BE platform handy now so your "Tested-by" will be very
nice to have. On LE platform the change above doesn't cause any problems
[as expected] :)

-Alexey

[U-Boot] [RESEND] drivers/usb/ehci: Use platform-specific accessors

[Vladimir Boroda]

This patch should have been rejected.  It no longer breaks the build, but it break the functionality on big-endian systems. The readl() and writel() macros already do the endian conversion assuming the port is in little-endian format.  So after this patch the EHCI registers are now read incorrectly, at least on PPC.  

You should remove the le32_to_cpu() macros if using the readl() and writel() accessors.  

[U-Boot] [RESEND] drivers/usb/ehci: Use platform-specific accessors

[Alexey Brodkin]

From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>

Current implementation doesn't allow utilization of platform-specific
reads and writes.

But some arches or platforms may want to use their accessors that do
some extra work like adding barriers for data serialization etc.

Interesting enough OHCI accessors already do that so just aligning
EHCI to it now.

This is a resend of http://patchwork.ozlabs.org/patch/726714/
Back in the day this patch broke some PPC and Sandbox boards
as they we missing inclusion of "asm/io.h". Those missing items were
fixed with:
 1) http://patchwork.ozlabs.org/patch/751397/
 2) http://patchwork.ozlabs.org/patch/771099/

So now it should be safe to apply this patch.
FWIW TravisCI builds everything with all 3 patches in place,
see https://travis-ci.org/abrodkin/u-boot/builds/239563813

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
Acked-by: Marek Vasut <marex@denx.de>
---
 drivers/usb/host/ehci.h | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 734d7f036278..2ab830df5155 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -102,13 +102,11 @@ struct usb_linux_config_descriptor {
 } __attribute__ ((packed));
 
 #if defined CONFIG_EHCI_DESC_BIG_ENDIAN
-#define ehci_readl(x)		cpu_to_be32((*((volatile u32 *)(x))))
-#define ehci_writel(a, b)	(*((volatile u32 *)(a)) = \
-					cpu_to_be32(((volatile u32)b)))
+#define ehci_readl(x)		cpu_to_be32(readl(x))
+#define ehci_writel(a, b)	writel(cpu_to_be32(b), a)
 #else
-#define ehci_readl(x)		cpu_to_le32((*((volatile u32 *)(x))))
-#define ehci_writel(a, b)	(*((volatile u32 *)(a)) = \
-					cpu_to_le32(((volatile u32)b)))
+#define ehci_readl(x)		cpu_to_le32(readl(x))
+#define ehci_writel(a, b)	writel(cpu_to_le32(b), a)
 #endif
 
 #if defined CONFIG_EHCI_MMIO_BIG_ENDIAN
-- 
2.7.5