On Fri, 22 Jan 2010, Jan Andersson wrote: > That is a solution that is more beautiful than what I first came up with > and it should fix the problem in my case for big endian mode. However, > when running with the controller hardware in little endian mode (where > it essentially byte swaps all the registers since this is a big endian > system) and ehci->big_endian_mmio = 0, ehci_readl will byte swap back > the data for me and return a value that is suitable for the macros. > Introducing a le32_to_cpu would swap the value one more time.. All right, I think I understand the problem now. As an example for the sake of argument, let's suppose that CAPLENGTH is 3 and HCIVERSION is 0x67. If the controller is in little-endian mode, this means that the memory-mapped registers will contain the following bytes (starting from offset 0): 3 0 7 6. Any CPU, whether little-endian or big-endian, using ehci_readl() would obtain 0x6703, and the macros would then yield the correct values. Now suppose the controller is in big-endian mode. The memory-mapped registers will contain the bytes: 3 0 6 7. Any CPU, whether little-endian or big-endian, using ehci_readl() would obtain 0x3067, which of course doesn't work with the HC_LENGTH and HC_VERSION macros, as you noted. So what you need to do is redefine those macros: #define HC_LENGTH(ehci, p) (0xff & ((p) >> \ (ehci_big_endian_mmio(ehci) ? 24 : 0))) #define HC_VERSION(ehci, p) (0xffff & ((p) >> \ (ehci_big_endian_mmio(ehci) ? 0 : 16))) Again, a comment would help explain the necessity for this. Of course, all the existing usages of the macros would then have to be fixed up, but that shouldn't be too hard. > I am also worried that the addition of le32_to_cpu() could break the > existing drivers. If a driver works today on a _big endian_ cpu, > wouldn't the addition of le32_to_cpu() break it? I'm guessing that this > can be an issue for ehci-ppc-of? - But I may be wrong.. The solution above should work okay with any CPU. Whether it works with other big-endian controllers is another matter -- if they implement the CAPLENGTH and HCIVERSION registers properly (i.e., as 1-byte and 2-byte registers rather than as parts of a 4-byte register) then it will. Still, you should check with the other maintainers. And don't assume that the existing code necessarily is correct; it might not be. > I am unable to produce a (software) solution that does not require user > configuration other than my first suggestion of assigning ehci->version > and ehci->caplength in the initialization code and replacing HC_LENGTH > and HC_VERSION with these variables. Is there any chance of such a patch > getting accepted? It may turn out to be necessary in the end. But let's try to avoid it if possible. > There is also another solution; Since I work for the vendor of this host > controller, I could redefine the host controller hardware to use a > "little endian layout" of capbase, even when in big endian mode. I > suppose that this could be acceptable if this is what other vendors have > done. On the other hand, it is not what is described by the > specification and it is possible that this issue comes up again when > someone wants to add support for yet another big endian host controller > implementation.. I agree, that is not an attractive option. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
