On 24/10/16 09:20, Jagan Teki wrote: > On Sun, Oct 23, 2016 at 3:22 AM, André Przywara <andre.przyw...@arm.com> > wrote: >> On 22/10/16 18:10, Jagan Teki wrote: >> >> Hi, >> >>> On Fri, Oct 21, 2016 at 6:54 AM, Andre Przywara <andre.przyw...@arm.com> >>> wrote: >>>> OHCI has a known limitation of allowing only 32-bit DMA buffer >>>> addresses, so we have a lot of u32 variables around, which are assigned >>>> to pointers and vice versa. This obviously creates issues with 64-bit >>>> systems, so the compiler complains here and there. >>>> To allow compilation for 64-bit boards which use only memory below 4GB >>>> anyway (and to avoid more invasive fixes), adjust some casts and types >>>> and assume that the EDs and TDs are all located in the lower 4GB. >>>> This fixes compilation of the OHCI driver for the Pine64. >>>> >>>> Signed-off-by: Andre Przywara <andre.przyw...@arm.com> >>>> --- >>>> drivers/usb/host/ohci-hcd.c | 21 +++++++++++---------- >>>> drivers/usb/host/ohci-sunxi.c | 2 +- >>>> drivers/usb/host/ohci.h | 11 +++++++---- >>>> 3 files changed, 19 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c >>>> index ccbfc02..0f6d03e 100644 >>>> --- a/drivers/usb/host/ohci-hcd.c >>>> +++ b/drivers/usb/host/ohci-hcd.c >>>> @@ -682,7 +682,7 @@ static int ep_link(ohci_t *ohci, ed_t *edi) >>>> ed->hwNextED = 0; >>>> flush_dcache_ed(ed); >>>> if (ohci->ed_controltail == NULL) >>>> - ohci_writel(ed, &ohci->regs->ed_controlhead); >>>> + ohci_writel((uintptr_t)ed, >>>> &ohci->regs->ed_controlhead); >>>> else >>>> ohci->ed_controltail->hwNextED = >>>> m32_swap((unsigned >>>> long)ed); >>>> @@ -700,7 +700,7 @@ static int ep_link(ohci_t *ohci, ed_t *edi) >>>> ed->hwNextED = 0; >>>> flush_dcache_ed(ed); >>>> if (ohci->ed_bulktail == NULL) >>>> - ohci_writel(ed, &ohci->regs->ed_bulkhead); >>>> + ohci_writel((uintptr_t)ed, >>>> &ohci->regs->ed_bulkhead); >>>> else >>>> ohci->ed_bulktail->hwNextED = >>>> m32_swap((unsigned >>>> long)ed); >>>> @@ -753,7 +753,7 @@ static void periodic_unlink(struct ohci *ohci, >>>> volatile struct ed *ed, >>>> >>>> /* ED might have been unlinked through another path */ >>>> while (*ed_p != 0) { >>>> - if (((struct ed *) >>>> + if (((struct ed *)(uintptr_t) >>>> m32_swap((unsigned long)ed_p)) == >>>> ed) { >>>> *ed_p = ed->hwNextED; >>>> aligned_ed_p = (unsigned long)ed_p; >>>> @@ -762,7 +762,7 @@ static void periodic_unlink(struct ohci *ohci, >>>> volatile struct ed *ed, >>>> aligned_ed_p + ARCH_DMA_MINALIGN); >>>> break; >>>> } >>>> - ed_p = &(((struct ed *) >>>> + ed_p = &(((struct ed *)(uintptr_t) >>>> m32_swap((unsigned >>>> long)ed_p))->hwNextED); >>>> } >>>> } >>>> @@ -798,7 +798,7 @@ static int ep_unlink(ohci_t *ohci, ed_t *edi) >>>> if (ohci->ed_controltail == ed) { >>>> ohci->ed_controltail = ed->ed_prev; >>>> } else { >>>> - ((ed_t *)m32_swap( >>>> + ((ed_t *)(uintptr_t)m32_swap( >>>> *((__u32 *)&ed->hwNextED)))->ed_prev = >>>> ed->ed_prev; >>>> } >>>> break; >>>> @@ -819,7 +819,7 @@ static int ep_unlink(ohci_t *ohci, ed_t *edi) >>>> if (ohci->ed_bulktail == ed) { >>>> ohci->ed_bulktail = ed->ed_prev; >>>> } else { >>>> - ((ed_t *)m32_swap( >>>> + ((ed_t *)(uintptr_t)m32_swap( >>>> *((__u32 *)&ed->hwNextED)))->ed_prev = >>>> ed->ed_prev; >>>> } >>>> break; >>>> @@ -914,12 +914,13 @@ static void td_fill(ohci_t *ohci, unsigned int info, >>>> >>>> /* fill the old dummy TD */ >>>> td = urb_priv->td [index] = >>>> - (td_t *)(m32_swap(urb_priv->ed->hwTailP) & >>>> ~0xf); >>>> + (td_t *)(uintptr_t) >>>> + (m32_swap(urb_priv->ed->hwTailP) & ~0xf); >>>> >>>> td->ed = urb_priv->ed; >>>> td->next_dl_td = NULL; >>>> td->index = index; >>>> - td->data = (__u32)data; >>>> + td->data = (uintptr_t)data; >>>> #ifdef OHCI_FILL_TRACE >>>> if (usb_pipebulk(urb_priv->pipe) && usb_pipeout(urb_priv->pipe)) { >>>> for (i = 0; i < len; i++) >>>> @@ -1099,7 +1100,7 @@ static void check_status(td_t *td_list) >>>> * we reverse the reversed done-list */ >>>> static td_t *dl_reverse_done_list(ohci_t *ohci) >>>> { >>>> - __u32 td_list_hc; >>>> + uintptr_t td_list_hc; >>>> td_t *td_rev = NULL; >>>> td_t *td_list = NULL; >>>> >>>> @@ -1862,7 +1863,7 @@ static int hc_start(ohci_t *ohci) >>>> ohci_writel(0, &ohci->regs->ed_controlhead); >>>> ohci_writel(0, &ohci->regs->ed_bulkhead); >>>> >>>> - ohci_writel((__u32)ohci->hcca, >>>> + ohci_writel((uintptr_t)ohci->hcca, >>>> &ohci->regs->hcca); /* reset clears this */ >>>> >>>> fminterval = 0x2edf; >>>> diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c >>>> index 2a1e8bf..0689374 100644 >>>> --- a/drivers/usb/host/ohci-sunxi.c >>>> +++ b/drivers/usb/host/ohci-sunxi.c >>>> @@ -51,7 +51,7 @@ static int ohci_usb_probe(struct udevice *dev) >>>> extra_ahb_gate_mask = 1 << AHB_GATE_OFFSET_USB_EHCI0; >>>> #endif >>>> priv->usb_gate_mask = CCM_USB_CTRL_OHCI0_CLK; >>>> - priv->phy_index = ((u32)regs - (SUNXI_USB1_BASE + 0x400)) / >>>> BASE_DIST; >>>> + priv->phy_index = ((uintptr_t)regs - (SUNXI_USB1_BASE + 0x400)) / >>>> BASE_DIST; >>>> priv->ahb_gate_mask <<= priv->phy_index * AHB_CLK_DIST; >>>> extra_ahb_gate_mask <<= priv->phy_index * AHB_CLK_DIST; >>>> priv->usb_gate_mask <<= priv->phy_index; >>>> diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h >>>> index 9b0c4a2..db0924c 100644 >>>> --- a/drivers/usb/host/ohci.h >>>> +++ b/drivers/usb/host/ohci.h >>>> @@ -10,12 +10,15 @@ >>>> /* >>>> * e.g. PCI controllers need this >>>> */ >>>> + >>>> +#include <asm/io.h> >>>> + >>>> #ifdef CONFIG_SYS_OHCI_SWAP_REG_ACCESS >>>> -# define ohci_readl(a) __swap_32(*((volatile u32 *)(a))) >>>> -# define ohci_writel(a, b) (*((volatile u32 *)(b)) = __swap_32((volatile >>>> u32)a)) >>>> +# define ohci_readl(a) __swap_32(readl(a)) >>>> +# define ohci_writel(v, a) writel(__swap_32(v), a) >>> >>> Not sure about writel here, why v? volatile casting to a here becomes v?
Looking at this again I am not really sure what you mean. The definition of ohci_writel() was a bit "naive", assuming that just using a volatile cast fulfills all architectural requirements of a non-cachable, device MMIO write. On ARM we are loosing the barrier compared to writel(), for instance. Since OHCI is used by many architectures, I think we should stick with the arch-native writel() semantics, and just add the swap on top. >> >> I was getting really confused about a and b here, especially since b is >> the address and a the value, in contrast to ohci_readl, where a is the >> address. >> So I thought I rather stick with the traditional writel() definitions, >> which use "v" for value and "a" or "c" for the address. >> >> It shouldn't change anything in the callers, really, except from the >> (desired) behaviour of not warning anymore. > > OK - Better to make this as a separate patch since this seems unrelated > change. TBH this was just a drive-by fix, so I'd rather drop this than posting another pointless fix. My hope was that by re-using writel() we could just fix this on the way. Please note that *this* bit does not change the behaviour at all, it just changes the parameter names in the macro definition. Cheers, Andre. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot