[U-Boot] [PATCH 1/2] drivers: USB: OHCI: allow compilation for 64-bit targets

Jagan Teki jagan at openedev.com
Wed Oct 26 08:56:39 CEST 2016


On Tue, Oct 25, 2016 at 4:16 AM, André Przywara <andre.przywara at arm.com> wrote:
> On 24/10/16 09:20, Jagan Teki wrote:
>> On Sun, Oct 23, 2016 at 3:22 AM, André Przywara <andre.przywara at 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.przywara at 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.przywara at 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.

Though, it is a parameter change - this is not related to the current
patch, are you agree? If so please update the commit message about
this change so-that in long run we can get some history about this
change.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.


More information about the U-Boot mailing list