On 01/28/2015 02:02 AM, Antti Seppälä wrote: > 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. > Ok, I'm fine with that. John -- 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
