On 27 January 2015 at 04:18, John Youn <John.Youn@xxxxxxxxxxxx> wrote: >> From: Vincent Pelletier [mailto:plr.vincent@xxxxxxxxx] >> Sent: Friday, January 23, 2015 12:19 AM >> >> On Fri, 23 Jan 2015 02:38:37 +0000, John Youn <John.Youn@xxxxxxxxxxxx> >> wrote: >> > Having the __raw functions everywhere is not pretty and probably not a >> > good idea either. >> > >> > I would rather have a dwc2_writel/dwc2_readl (like in dwc3), and we >> > can figure out what needs to happen in there to support your platform. >> > >> > As for readl/writel or the __raw equivalents, I am not sure. I haven't >> > run the in-kernel driver on a big endian system so I don't know what >> > the issue is. But I suspect there may be consequences to other >> > platforms if we simply change this to __raw_readl/__raw_writel. >> >> (focussing on readl, but the following also applies to writel) >> >> readl is defined as __le32_to_cpu(__raw_readl(...)), so using readl on >> little endian is equivalent to __raw_readl, as __le32_to_cpu is a no-op. >> >> On big endian, it causes problems because __le32_to_cpu swaps bytes >> around, causing GSNPSID to be read as 0xnn2n544f instead of expected >> 0x4f542nnn - and likewise for all other registers. >> >> An earlier version (sent to linux-usb a week ago with not enough CC, >> but I cannot find it in archives now) defined dwc2_readl as >> le32_to_cpu(readl(...)) >> which is equivalent to >> le32_to_cpu(le32_to_cpu(__raw_readl(...))) >> and which, on big-endian, makes le32_to_cpu(le32_to_cpu()) a no-op and >> (ignoring possible compiler optimisation) swaps bytes twice per call. >> On little endian, it is still equivalent to __raw_readl(...). >> >> As for the prettiness of calling double-underscore functions, >> speaking for myself I'm not used to Linux development enough to tell. >> My python developer reflex tell me it's not how it should be. >> OTOH there is no "raw_readl" or so alias, and other USB drivers >> reference __raw_readl (example as of v3.14.28): >> linux/drivers/usb$ git grep -l __raw_readl >> gadget/at91_udc.c >> gadget/atmel_usba_udc.c >> gadget/atmel_usba_udc.h >> gadget/fsl_udc_core.c >> gadget/pxa27x_udc.h >> host/ehci-omap.c >> host/ehci-orion.c >> host/ehci-w90x900.c >> host/ehci.h >> host/isp1760-hcd.c >> host/ohci-da8xx.c >> host/ohci-nxp.c >> host/ohci-pxa27x.c >> musb/da8xx.c >> musb/davinci.c >> musb/musb_dsps.c >> musb/musb_io.h >> phy/phy-fsl-usb.c >> >> Would it be better to wrap __raw_readl in a macro and call that >> everywhere rather than calling __raw_readl itself ? > > I was thinking more along the lines of what dwc3 does. See > drivers/usb/dwc3/io.h. > > One solution would be to do as above, and put in a compile-time check > for your platform and call the appropriate access function. > > I believe writel/readl also preserves memory ordering so it might > affect other platforms to just use the __raw equivalent in its place. > Broadly speaking readl(x) can be split to three separate functions. I think it evaluates to something like below for dwc2: static inline u32 dwc2_readl(const volatile void __iomem *addr) { u32 value = __raw_readl(addr); /* Raw access to io */ value = le32_to_cpu(value); /* Ensure little-endian byte order */ mb(); /* Prevent re-ordering of io-accesses */ return value; } Now le32_to_cpu is a no-op for little-endian platforms and breaks dwc2 for big endian ones (as Vincent explained, it swaps bytes unnecessarily for big endian platforms). So I'm thinking of dropping it and re-spinning the patch with something like below: static inline u32 dwc2_readl(const volatile void __iomem *addr) { u32 value = __raw_readl(addr); /* Raw access to io */ mb(); /* Prevent re-ordering of io-accesses */ return value; } and then replacing all readl's in dwc2 with dwc2_readl (and same for writel). That way nothing should get broken on little-endian as we're only dropping a no-op and we get to use the same code for big-endian platforms too. Would that be ok? I really would not want to add platform specific io-access functionalities if we can manage with a generic approach. -- Antti -- 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
