| Message ID | 1477013070-29836-2-git-send-email-andre.przywara@arm.com |
|---|---|
| State | Accepted |
| Commit | 57faca19a82fc9b43a227824c30aeb76a43d1957 |
| Delegated to: | Marek Vasut |
| Headers | show |
On Fri, Oct 21, 2016 at 6:54 AM, Andre Przywara <andre.przywara@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@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? thanks!
On 22/10/16 18:10, Jagan Teki wrote: Hi, > On Fri, Oct 21, 2016 at 6:54 AM, Andre Przywara <andre.przywara@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@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? 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. Cheers, Andre.
On Sun, Oct 23, 2016 at 3:22 AM, André Przywara <andre.przywara@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@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@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? > > 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. thanks!
On 24/10/16 09:20, Jagan Teki wrote: > On Sun, Oct 23, 2016 at 3:22 AM, André Przywara <andre.przywara@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@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@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.
On Tue, Oct 25, 2016 at 4:16 AM, André Przywara <andre.przywara@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@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@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@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!
Hi, On 21-10-16 03:24, Andre Przywara 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@arm.com> Patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans p.s. About the ohci_writel macro changes also giving the macro parameters more sensible names, I believe it is fine to do this while at it and that this does not need to be split out. > --- > 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) > #else > -# define ohci_readl(a) (*((volatile u32 *)(a))) > -# define ohci_writel(a, b) (*((volatile u32 *)(b)) = ((volatile u32)a)) > +# define ohci_readl(a) readl(a) > +# define ohci_writel(v, a) writel(v, a) > #endif /* CONFIG_SYS_OHCI_SWAP_REG_ACCESS */ > > #if ARCH_DMA_MINALIGN > 16 >
On 10/29/2016 02:50 PM, Hans de Goede wrote: > Hi, > > On 21-10-16 03:24, Andre Przywara 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@arm.com> > > Patch looks good to me: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Applied, thanks. Andre, it would be nice if you CC'd me on the original submission. > Regards, > > Hans > > p.s. > > About the ohci_writel macro changes also giving the macro parameters > more sensible names, I believe it is fine to do this while at it and > that this does not need to be split out. Indeed, I am fine with it as well.
On 29/10/16 18:42, Marek Vasut wrote: > On 10/29/2016 02:50 PM, Hans de Goede wrote: >> Hi, >> >> On 21-10-16 03:24, Andre Przywara 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@arm.com> >> >> Patch looks good to me: >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Applied, thanks. > > Andre, it would be nice if you CC'd me on the original submission. Yeah, sorry about that. I think I had you in To: on my --dry-run test, but somehow managed to drop you after fixing "just one more thing". Thanks for applying this! Cheers, Andre. > >> Regards, >> >> Hans >> >> p.s. >> >> About the ohci_writel macro changes also giving the macro parameters >> more sensible names, I believe it is fine to do this while at it and >> that this does not need to be split out. > > Indeed, I am fine with it as well. >
On 10/30/2016 10:51 AM, André Przywara wrote: > On 29/10/16 18:42, Marek Vasut wrote: >> On 10/29/2016 02:50 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 21-10-16 03:24, Andre Przywara 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@arm.com> >>> >>> Patch looks good to me: >>> >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> >> Applied, thanks. >> >> Andre, it would be nice if you CC'd me on the original submission. > > Yeah, sorry about that. I think I had you in To: on my --dry-run test, > but somehow managed to drop you after fixing "just one more thing". > > Thanks for applying this! np, PR is out too.
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) #else -# define ohci_readl(a) (*((volatile u32 *)(a))) -# define ohci_writel(a, b) (*((volatile u32 *)(b)) = ((volatile u32)a)) +# define ohci_readl(a) readl(a) +# define ohci_writel(v, a) writel(v, a) #endif /* CONFIG_SYS_OHCI_SWAP_REG_ACCESS */ #if ARCH_DMA_MINALIGN > 16
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@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(-)