drivers/usb/ehci: Use platform-specific accessors
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH RESEND 1/1 drivers/usb/ehci: Use platform-specific accessors
Hi Marek, Vladimir,
Quoted 12 lines · Marek Vasut
Quoted 10 lines · Vladimir Boroda
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. Thereadl()andwritel()accessors already perform endian port to cpu conversion, so no extra conversion was necessary. The double conversion caused incorrect reading of USB EHCI registers.
Quoted 2 lines
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.
Quoted 4 lines
Quoted 4 lines
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()andwritel()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----------------------------------
drivers/usb/host/ehci.h +0 -0
@@ -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
PATCH RESEND 1/1 drivers/usb/ehci: Use platform-specific accessors
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
drivers/usb/host/ehci.h +0 -0
@@ -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 <[email protected]> wrote:
Hi Marek, Vladimir,
Quoted 12 lines · Marek Vasut
Quoted 10 lines · Vladimir Boroda
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. Thereadl()andwritel()accessors already perform endian port to cpu conversion, so no extra conversion was necessary. The double conversion caused incorrect reading of USB EHCI registers.
Quoted 2 lines
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.
Quoted 4 lines
Quoted 4 lines
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()andwritel()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----------------------------------
drivers/usb/host/ehci.h +0 -0
@@ -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