Re: [PATCH 00/18] ARM: Add minimal Raspberry Pi 4 support

[Nicolas Saenz Julienne <nsaenzjulienne@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 6204 bytes --]

On Tue, 2019-07-23 at 18:26 +0200, Stefan Wahren wrote:
> Hi Nicolas,
> 
> thanks for your work, but i'm a little bit sceptical about these
> changes. So here some thoughts.
> 
> Am 23.07.19 um 15:32 schrieb Nicolas Saenz Julienne:
> > On Tue, 2019-07-23 at 11:34 +0200, Christoph Hellwig wrote:
> > > On Mon, Jul 22, 2019 at 08:10:17PM +0200, Stefan Wahren wrote:
> > > > i rebased this series also and got this only on the RPi 4.
> > > > 
> > > > After reverting the following:
> > > > 
> > > > 79a986721de dma-mapping: remove dma_max_pfn
> > > > 7559d612dff0 mmc: core: let the dma map ops handle bouncing
> > > > 
> > > > This crash disappear, but wifi seems to be still broken.
> > > > 
> > > > Would be nice, if you can investigate further.
> > > That means dma addressing on this system doesn't just work for some
> > > memory, and the mmc bounce buffering was papering over that just for
> > > mmc.  Do you have highmem on this system?
> > > 
> > > You might want to try this series, which has been submitted upstream:
> > > 
> > > 
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/arm-swiotlb
> > Hi Christoph,
> > I tried your series on top of Stefan's, it has no effect. I guess it's no
> > surprise as with mult_v7_defconfig, you get SWIOTLB=n & LPAE=n.
> > 
> > FYI DMA addressing constraints for RPi4 are the following: devices can only
> > access the first GB of ram even though the board might have up to 4GB of
> > ram.
> > The DMA addresses are aliased with a 0xc0000000 offset. So 0x00000000 phys
> > is
> > aliased to 0xc0000000 in DMA. This is the same as for an RFC you commented
> > last
> > week trying to fix similar issues for arm64.
> > 
> > You state in "arm: use swiotlb for bounce buffer on LPAE configs" that "The
> > DMA
> > API requires that 32-bit DMA masks are always supported". If I understand it
> > correctly this device breaks that assumption. Which implies we need a bounce
> > buffer system in place for any straming DMA user.
> > 
> > It seems we're unable to use dma-direct/swiotlb, so I enabled arm's
> > dmabounce
> > on all devices hooked into RPi's limited interconnect, which fixes this
> > issue.
> Does it fix the wifi issue too?

Well it works as long as I revert this: 901bb98918 ("nl80211: require and
validate vendor command policy"). Which has nothing to do with DMA anyways.

Was this the issue you where seeing?

[    4.969679] WARNING: CPU: 2 PID: 21 at net/wireless/core.c:868 wiphy_register+0x8e8/0xbdc [cfg80211]
[...]
[    4.969974] ieee80211 phy0: brcmf_cfg80211_attach: Could not register wiphy device (-22)

> > Any thoughts on this?
> > 
> > diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> > index 5e5f1fabc3d4..3db8deed83a6 100644
> > --- a/arch/arm/mach-bcm/Kconfig
> > +++ b/arch/arm/mach-bcm/Kconfig
> > @@ -168,6 +168,7 @@ config ARCH_BCM2835
> >         select PINCTRL
> >         select PINCTRL_BCM2835
> >         select MFD_CORE
> > +       select DMABOUNCE
> >         help
> >           This enables support for the Broadcom BCM2835 and BCM2836 SoCs.
> >           This SoC is used in the Raspberry Pi and Roku 2 devices.
> > diff --git a/arch/arm/mach-bcm/board_bcm2835.c b/arch/arm/mach-
> > bcm/board_bcm2835.c
> > index c09cf25596af..be788849c4bb 100644
> > --- a/arch/arm/mach-bcm/board_bcm2835.c
> > +++ b/arch/arm/mach-bcm/board_bcm2835.c
> > @@ -3,6 +3,8 @@
> >   * Copyright (C) 2010 Broadcom
> >   */
> > 
> > +#include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> >  #include <linux/init.h>
> >  #include <linux/irqchip.h>
> >  #include <linux/of_address.h>
> > @@ -24,8 +26,37 @@ static const char * const bcm2835_compat[] = {
> >         NULL
> >  };
> > 
> > +static int bcm2835_needs_bounce(struct device *dev, dma_addr_t dma_addr,
> > size_t size)
> > +{
> > +       /*
> > +        * The accepted dma addresses are [0xc0000000, 0xffffffff] which map
> > to
> > +        * ram's [0x00000000, 0x3fffffff].
> > +        */
> > +       return dma_addr < 3ULL * SZ_1G;
> > +}
> > +
> > +/*
> > + * Setup DMA mask to 1GB on devices hanging from soc interconnect
> > + */
> > +static int bcm2835_platform_notify(struct device *dev)
> > +{
> > +       if (dev->parent && !strcmp("soc", dev_name(dev->parent))) {
> > +               dev->dma_mask = &dev->coherent_dma_mask;
> > +               dev->coherent_dma_mask = DMA_BIT_MASK(30); /* 1GB */
> Shouldn't this come from the device tree?

Yes, actually I could use the 'dma-ranges' parsing code I suggested on the
arm64 RFC. The same goes with 'dma_zone_size = SZ_1G', it ideally should be
calculated based on the device-tree.

The way I see it I'm not sure it's worth the effort, in arm64 we have no choice
as there are no board files. But here we seem to be the only ones with this
specific DMA addressing constraint, so fixing it in arm/common doesn't seem
like it's going to benefit anyone else. Let's see how the arm arch maintainers
react though.

There is one catch though. I missed it earlier as I was excited to see the
board boot, but some devices are failing to set their DMA masks:

[    1.989576] dwc2 fe980000.usb: can't set coherent DMA mask: -5

It seems that other users of dmabounce also implement their own
dma_supported(). I have to look into it.

> > +               dmabounce_register_dev(dev, 2048, 4096,
> > bcm2835_needs_bounce);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +void __init bcm2835_init_early(void)
> > +{
> > +       platform_notify = bcm2835_platform_notify;
> > +}
> > +
> >  DT_MACHINE_START(BCM2835, "BCM2835")
> >         .dma_zone_size  = SZ_1G,
> >         .dt_compat = bcm2835_compat,
> >         .smp = smp_ops(bcm2836_smp_ops),
> > +       .init_early = bcm2835_init_early,
> 
> The sum of all these changes make me think, that we should start a new
> board for BCM2711 instead of extending BCM2835.
> 

I agree, I did it locally but merged it into the current board file to make the
patch smaller.

> Best regards
> Stefan Wahren
> 
> >  MACHINE_END
> > 
> >  Regards,
> >  Nicolas
> > 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]