diff mbox

[V3,00/21] MMCONFIG refactoring and support for ARM64 PCI hostbridge init based on ACPI

Message ID 20160114134440.GA8520@xora-haswell.xora.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Graeme Gregory Jan. 14, 2016, 1:44 p.m. UTC
On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> From the functionality point of view this series might be split into the
> following logic parts:
> 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
>    PCI config regions and used when necessary.
> 2. Move non-arch specific bits to the core code.
> 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> 4. Enable above driver on ARM64
> 
> Patches has been built on top of 4.4 and can be found here:
> git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> 
> NOTE, this patch set depends on Matthew's patches:
> http://www.spinics.net/lists/linux-pci/msg45950.html
> https://github.com/Vality/linux/tree/pci-fixes
> 
> This has been tested on Cavium ThunderX server and QEMU.
> Any help in reviewing and testing is very appreciated.

I have tested this on my AMD Overdrive so

Tested-by: Graeme Gregory <graeme.gregory@linaro.org>

But to actually get my r8169 network card working I also need the
following patch.


I suspect we need to set coherent_dma_mask somewhere in the platform but I do
not know where this should happen. Hopefully an ARM64 expert can help.

Graeme

Comments

Catalin Marinas Jan. 14, 2016, 2 p.m. UTC | #1
On Thu, Jan 14, 2016 at 01:44:40PM +0000, Graeme Gregory wrote:
> On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > From the functionality point of view this series might be split into the
> > following logic parts:
> > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> >    PCI config regions and used when necessary.
> > 2. Move non-arch specific bits to the core code.
> > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > 4. Enable above driver on ARM64
> > 
> > Patches has been built on top of 4.4 and can be found here:
> > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > 
> > NOTE, this patch set depends on Matthew's patches:
> > http://www.spinics.net/lists/linux-pci/msg45950.html
> > https://github.com/Vality/linux/tree/pci-fixes
> > 
> > This has been tested on Cavium ThunderX server and QEMU.
> > Any help in reviewing and testing is very appreciated.
> 
> I have tested this on my AMD Overdrive so
> 
> Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> 
> But to actually get my r8169 network card working I also need the
> following patch.
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2fbf840..40e24e2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>         set_dev_node(&dev->dev, pcibus_to_node(bus));
>         dev->dev.dma_mask = &dev->dma_mask;
>         dev->dev.dma_parms = &dev->dma_parms;
> -       dev->dev.coherent_dma_mask = 0xffffffffull;
> +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
>         pci_dma_configure(dev);
> 
>         pci_set_dma_max_seg_size(dev, 65536);

With OF, we get the coherent_dma_mask set by of_dma_configure(). But I
have no idea how you do this with ACPI.
Mark Salter Jan. 14, 2016, 2:01 p.m. UTC | #2
On Thu, 2016-01-14 at 13:44 +0000, Graeme Gregory wrote:
> On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > From the functionality point of view this series might be split into the
> > following logic parts:
> > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> >    PCI config regions and used when necessary.
> > 2. Move non-arch specific bits to the core code.
> > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > 4. Enable above driver on ARM64
> > 
> > Patches has been built on top of 4.4 and can be found here:
> > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > 
> > NOTE, this patch set depends on Matthew's patches:
> > http://www.spinics.net/lists/linux-pci/msg45950.html
> > https://github.com/Vality/linux/tree/pci-fixes
> > 
> > This has been tested on Cavium ThunderX server and QEMU.
> > Any help in reviewing and testing is very appreciated.
> 
> I have tested this on my AMD Overdrive so
> 
> Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> 
> But to actually get my r8169 network card working I also need the
> following patch.
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2fbf840..40e24e2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>         set_dev_node(&dev->dev, pcibus_to_node(bus));
>         dev->dev.dma_mask = &dev->dma_mask;
>         dev->dev.dma_parms = &dev->dma_parms;
> -       dev->dev.coherent_dma_mask = 0xffffffffull;
> +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
>         pci_dma_configure(dev);
> 
>         pci_set_dma_max_seg_size(dev, 65536);
> 
> I suspect we need to set coherent_dma_mask somewhere in the platform but I do
> not know where this should happen. Hopefully an ARM64 expert can help.

I've run into a number of cards which don't work on some arm64 machines where there are
no DMA address below 4G. For the realtek card you can use r8169.use_dac=1 on the cmdline
to get it to work.

> 
> Graeme
>
Mark Salter Jan. 14, 2016, 2:09 p.m. UTC | #3
On Thu, 2016-01-14 at 14:00 +0000, Catalin Marinas wrote:
> On Thu, Jan 14, 2016 at 01:44:40PM +0000, Graeme Gregory wrote:
> > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > From the functionality point of view this series might be split into the
> > > following logic parts:
> > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > >    PCI config regions and used when necessary.
> > > 2. Move non-arch specific bits to the core code.
> > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > 4. Enable above driver on ARM64
> > > 
> > > Patches has been built on top of 4.4 and can be found here:
> > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > 
> > > NOTE, this patch set depends on Matthew's patches:
> > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > https://github.com/Vality/linux/tree/pci-fixes
> > > 
> > > This has been tested on Cavium ThunderX server and QEMU.
> > > Any help in reviewing and testing is very appreciated.
> > 
> > I have tested this on my AMD Overdrive so
> > 
> > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > 
> > But to actually get my r8169 network card working I also need the
> > following patch.
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 2fbf840..40e24e2 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> >         dev->dev.dma_mask = &dev->dma_mask;
> >         dev->dev.dma_parms = &dev->dma_parms;
> > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> >         pci_dma_configure(dev);
> > 
> >         pci_set_dma_max_seg_size(dev, 65536);
> 
> With OF, we get the coherent_dma_mask set by of_dma_configure(). But I
> have no idea how you do this with ACPI.
> 
That doesn't get called for a PCI device (not listed in DT) does it?
Graeme Gregory Jan. 14, 2016, 2:15 p.m. UTC | #4
On Thu, Jan 14, 2016 at 09:01:40AM -0500, Mark Salter wrote:
> On Thu, 2016-01-14 at 13:44 +0000, Graeme Gregory wrote:
> > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > From the functionality point of view this series might be split into the
> > > following logic parts:
> > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > >    PCI config regions and used when necessary.
> > > 2. Move non-arch specific bits to the core code.
> > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > 4. Enable above driver on ARM64
> > > 
> > > Patches has been built on top of 4.4 and can be found here:
> > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > 
> > > NOTE, this patch set depends on Matthew's patches:
> > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > https://github.com/Vality/linux/tree/pci-fixes
> > > 
> > > This has been tested on Cavium ThunderX server and QEMU.
> > > Any help in reviewing and testing is very appreciated.
> > 
> > I have tested this on my AMD Overdrive so
> > 
> > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > 
> > But to actually get my r8169 network card working I also need the
> > following patch.
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 2fbf840..40e24e2 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> >         dev->dev.dma_mask = &dev->dma_mask;
> >         dev->dev.dma_parms = &dev->dma_parms;
> > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> >         pci_dma_configure(dev);
> > 
> >         pci_set_dma_max_seg_size(dev, 65536);
> > 
> > I suspect we need to set coherent_dma_mask somewhere in the platform but I do
> > not know where this should happen. Hopefully an ARM64 expert can help.
> 
> I've run into a number of cards which don't work on some arm64 machines where there are
> no DMA address below 4G. For the realtek card you can use r8169.use_dac=1 on the cmdline
> to get it to work.
> 
I need r8169.use_dac=1 and this patch to get card to work.

Graeme
Mark Salter Jan. 14, 2016, 2:24 p.m. UTC | #5
On Thu, 2016-01-14 at 14:15 +0000, Graeme Gregory wrote:
> On Thu, Jan 14, 2016 at 09:01:40AM -0500, Mark Salter wrote:
> > On Thu, 2016-01-14 at 13:44 +0000, Graeme Gregory wrote:
> > > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > > From the functionality point of view this series might be split into the
> > > > following logic parts:
> > > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > > >    PCI config regions and used when necessary.
> > > > 2. Move non-arch specific bits to the core code.
> > > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > > 4. Enable above driver on ARM64
> > > > 
> > > > Patches has been built on top of 4.4 and can be found here:
> > > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > > 
> > > > NOTE, this patch set depends on Matthew's patches:
> > > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > > https://github.com/Vality/linux/tree/pci-fixes
> > > > 
> > > > This has been tested on Cavium ThunderX server and QEMU.
> > > > Any help in reviewing and testing is very appreciated.
> > > 
> > > I have tested this on my AMD Overdrive so
> > > 
> > > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > > 
> > > But to actually get my r8169 network card working I also need the
> > > following patch.
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 2fbf840..40e24e2 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> > >         dev->dev.dma_mask = &dev->dma_mask;
> > >         dev->dev.dma_parms = &dev->dma_parms;
> > > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> > >         pci_dma_configure(dev);
> > > 
> > >         pci_set_dma_max_seg_size(dev, 65536);
> > > 
> > > I suspect we need to set coherent_dma_mask somewhere in the platform but I do
> > > not know where this should happen. Hopefully an ARM64 expert can help.
> > 
> > I've run into a number of cards which don't work on some arm64 machines where there are
> > no DMA address below 4G. For the realtek card you can use r8169.use_dac=1 on the cmdline
> > to get it to work.
> > 
> I need r8169.use_dac=1 and this patch to get card to work.
> 

Hmm. I didn't need a patch in the past but I don't think I've used that card with
this patch series. I will try that when I get a chance...

> Graeme
>
Catalin Marinas Jan. 14, 2016, 2:50 p.m. UTC | #6
On Thu, Jan 14, 2016 at 09:09:59AM -0500, Mark Salter wrote:
> On Thu, 2016-01-14 at 14:00 +0000, Catalin Marinas wrote:
> > On Thu, Jan 14, 2016 at 01:44:40PM +0000, Graeme Gregory wrote:
> > > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > > From the functionality point of view this series might be split into the
> > > > following logic parts:
> > > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > > >    PCI config regions and used when necessary.
> > > > 2. Move non-arch specific bits to the core code.
> > > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > > 4. Enable above driver on ARM64
> > > > 
> > > > Patches has been built on top of 4.4 and can be found here:
> > > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > > 
> > > > NOTE, this patch set depends on Matthew's patches:
> > > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > > https://github.com/Vality/linux/tree/pci-fixes
> > > > 
> > > > This has been tested on Cavium ThunderX server and QEMU.
> > > > Any help in reviewing and testing is very appreciated.
> > > 
> > > I have tested this on my AMD Overdrive so
> > > 
> > > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > > 
> > > But to actually get my r8169 network card working I also need the
> > > following patch.
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 2fbf840..40e24e2 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> > >         dev->dev.dma_mask = &dev->dma_mask;
> > >         dev->dev.dma_parms = &dev->dma_parms;
> > > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> > >         pci_dma_configure(dev);
> > > 
> > >         pci_set_dma_max_seg_size(dev, 65536);
> > 
> > With OF, we get the coherent_dma_mask set by of_dma_configure(). But I
> > have no idea how you do this with ACPI.
> 
> That doesn't get called for a PCI device (not listed in DT) does it?

pci_device_add
  pci_dma_configure
    of_dma_configure(&dev->dev, bridge->parent->of_node);

So it gets configured based on the bridge information the device is
attached to.
Mark Salter Jan. 14, 2016, 2:59 p.m. UTC | #7
On Thu, 2016-01-14 at 14:50 +0000, Catalin Marinas wrote:
> On Thu, Jan 14, 2016 at 09:09:59AM -0500, Mark Salter wrote:
> > On Thu, 2016-01-14 at 14:00 +0000, Catalin Marinas wrote:
> > > On Thu, Jan 14, 2016 at 01:44:40PM +0000, Graeme Gregory wrote:
> > > > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > > > From the functionality point of view this series might be split into the
> > > > > following logic parts:
> > > > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > > > >    PCI config regions and used when necessary.
> > > > > 2. Move non-arch specific bits to the core code.
> > > > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > > > 4. Enable above driver on ARM64
> > > > > 
> > > > > Patches has been built on top of 4.4 and can be found here:
> > > > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > > > 
> > > > > NOTE, this patch set depends on Matthew's patches:
> > > > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > > > https://github.com/Vality/linux/tree/pci-fixes
> > > > > 
> > > > > This has been tested on Cavium ThunderX server and QEMU.
> > > > > Any help in reviewing and testing is very appreciated.
> > > > 
> > > > I have tested this on my AMD Overdrive so
> > > > 
> > > > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > > > 
> > > > But to actually get my r8169 network card working I also need the
> > > > following patch.
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 2fbf840..40e24e2 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > > >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> > > >         dev->dev.dma_mask = &dev->dma_mask;
> > > >         dev->dev.dma_parms = &dev->dma_parms;
> > > > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > > > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> > > >         pci_dma_configure(dev);
> > > > 
> > > >         pci_set_dma_max_seg_size(dev, 65536);
> > > 
> > > With OF, we get the coherent_dma_mask set by of_dma_configure(). But I
> > > have no idea how you do this with ACPI.
> > 
> > That doesn't get called for a PCI device (not listed in DT) does it?
> 
> pci_device_add
>   pci_dma_configure
>     of_dma_configure(&dev->dev, bridge->parent->of_node);
> 
> So it gets configured based on the bridge information the device is
> attached to.
> 

Oh right. And there is an ACPI path in pci_dma_configure but it only sets the
coherency. But I still don't know why Graeme is having issues. I just tried
an r8169 card on Mustang and Seattle with this patch series in a 4.4 kernel
and it worked fine with r8169.use_dac=1.
Graeme Gregory Jan. 15, 2016, 12:12 p.m. UTC | #8
On Thu, Jan 14, 2016 at 09:24:41AM -0500, Mark Salter wrote:
> On Thu, 2016-01-14 at 14:15 +0000, Graeme Gregory wrote:
> > On Thu, Jan 14, 2016 at 09:01:40AM -0500, Mark Salter wrote:
> > > On Thu, 2016-01-14 at 13:44 +0000, Graeme Gregory wrote:
> > > > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > > > From the functionality point of view this series might be split into the
> > > > > following logic parts:
> > > > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > > > >    PCI config regions and used when necessary.
> > > > > 2. Move non-arch specific bits to the core code.
> > > > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > > > 4. Enable above driver on ARM64
> > > > > 
> > > > > Patches has been built on top of 4.4 and can be found here:
> > > > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > > > 
> > > > > NOTE, this patch set depends on Matthew's patches:
> > > > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > > > https://github.com/Vality/linux/tree/pci-fixes
> > > > > 
> > > > > This has been tested on Cavium ThunderX server and QEMU.
> > > > > Any help in reviewing and testing is very appreciated.
> > > > 
> > > > I have tested this on my AMD Overdrive so
> > > > 
> > > > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > > > 
> > > > But to actually get my r8169 network card working I also need the
> > > > following patch.
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 2fbf840..40e24e2 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > > >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> > > >         dev->dev.dma_mask = &dev->dma_mask;
> > > >         dev->dev.dma_parms = &dev->dma_parms;
> > > > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > > > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> > > >         pci_dma_configure(dev);
> > > > 
> > > >         pci_set_dma_max_seg_size(dev, 65536);
> > > > 
> > > > I suspect we need to set coherent_dma_mask somewhere in the platform but I do
> > > > not know where this should happen. Hopefully an ARM64 expert can help.
> > > 
> > > I've run into a number of cards which don't work on some arm64 machines where there are
> > > no DMA address below 4G. For the realtek card you can use r8169.use_dac=1 on the cmdline
> > > to get it to work.
> > > 
> > I need r8169.use_dac=1 and this patch to get card to work.
> > 
> 
> Hmm. I didn't need a patch in the past but I don't think I've used that card with
> this patch series. I will try that when I get a chance...
> 

From my previous debugging on this issue I was getting an address bigger
than 32bits. As r8169 is a whole range of chips are guess they are not
all equal?

I can stick debug stuff in if someone tells me where to put them.

Graeme
Graeme Gregory Jan. 18, 2016, 2:04 p.m. UTC | #9
On Fri, Jan 15, 2016 at 12:12:31PM +0000, Graeme Gregory wrote:
> On Thu, Jan 14, 2016 at 09:24:41AM -0500, Mark Salter wrote:
> > On Thu, 2016-01-14 at 14:15 +0000, Graeme Gregory wrote:
> > > On Thu, Jan 14, 2016 at 09:01:40AM -0500, Mark Salter wrote:
> > > > On Thu, 2016-01-14 at 13:44 +0000, Graeme Gregory wrote:
> > > > > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > > > > From the functionality point of view this series might be split into the
> > > > > > following logic parts:
> > > > > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > > > > >    PCI config regions and used when necessary.
> > > > > > 2. Move non-arch specific bits to the core code.
> > > > > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > > > > 4. Enable above driver on ARM64
> > > > > > 
> > > > > > Patches has been built on top of 4.4 and can be found here:
> > > > > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > > > > 
> > > > > > NOTE, this patch set depends on Matthew's patches:
> > > > > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > > > > https://github.com/Vality/linux/tree/pci-fixes
> > > > > > 
> > > > > > This has been tested on Cavium ThunderX server and QEMU.
> > > > > > Any help in reviewing and testing is very appreciated.
> > > > > 
> > > > > I have tested this on my AMD Overdrive so
> > > > > 
> > > > > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > > > > 
> > > > > But to actually get my r8169 network card working I also need the
> > > > > following patch.
> > > > > 
> > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > > index 2fbf840..40e24e2 100644
> > > > > --- a/drivers/pci/probe.c
> > > > > +++ b/drivers/pci/probe.c
> > > > > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > > > >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> > > > >         dev->dev.dma_mask = &dev->dma_mask;
> > > > >         dev->dev.dma_parms = &dev->dma_parms;
> > > > > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > > > > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> > > > >         pci_dma_configure(dev);
> > > > > 
> > > > >         pci_set_dma_max_seg_size(dev, 65536);
> > > > > 
> > > > > I suspect we need to set coherent_dma_mask somewhere in the platform but I do
> > > > > not know where this should happen. Hopefully an ARM64 expert can help.
> > > > 
> > > > I've run into a number of cards which don't work on some arm64 machines where there are
> > > > no DMA address below 4G. For the realtek card you can use r8169.use_dac=1 on the cmdline
> > > > to get it to work.
> > > > 
> > > I need r8169.use_dac=1 and this patch to get card to work.
> > > 
> > 
> > Hmm. I didn't need a patch in the past but I don't think I've used that card with
> > this patch series. I will try that when I get a chance...
> > 
> 
> From my previous debugging on this issue I was getting an address bigger
> than 32bits. As r8169 is a whole range of chips are guess they are not
> all equal?
> 
> I can stick debug stuff in if someone tells me where to put them.
> 

After some private debugging with Mark it turned out that the difference
between our configurations was I did not have.

CONFIG_DMA_CMA=y

With this enabled then the card works without coherent mask hack.

Thanks

Graeme
Bjorn Helgaas Jan. 19, 2016, 8:25 p.m. UTC | #10
On Mon, Jan 18, 2016 at 02:04:05PM +0000, Graeme Gregory wrote:
> On Fri, Jan 15, 2016 at 12:12:31PM +0000, Graeme Gregory wrote:
> > On Thu, Jan 14, 2016 at 09:24:41AM -0500, Mark Salter wrote:
> > > On Thu, 2016-01-14 at 14:15 +0000, Graeme Gregory wrote:
> > > > On Thu, Jan 14, 2016 at 09:01:40AM -0500, Mark Salter wrote:
> > > > > On Thu, 2016-01-14 at 13:44 +0000, Graeme Gregory wrote:
> > > > > > On Wed, Jan 13, 2016 at 02:20:46PM +0100, Tomasz Nowicki wrote:
> > > > > > > From the functionality point of view this series might be split into the
> > > > > > > following logic parts:
> > > > > > > 1. Make MMCONFIG code arch-agnostic which allows all architectures to collect
> > > > > > >    PCI config regions and used when necessary.
> > > > > > > 2. Move non-arch specific bits to the core code.
> > > > > > > 3. Use MMCONFIG code and implement generic ACPI based PCI host controller driver.
> > > > > > > 4. Enable above driver on ARM64
> > > > > > > 
> > > > > > > Patches has been built on top of 4.4 and can be found here:
> > > > > > > git@github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v3)
> > > > > > > 
> > > > > > > NOTE, this patch set depends on Matthew's patches:
> > > > > > > http://www.spinics.net/lists/linux-pci/msg45950.html
> > > > > > > https://github.com/Vality/linux/tree/pci-fixes
> > > > > > > 
> > > > > > > This has been tested on Cavium ThunderX server and QEMU.
> > > > > > > Any help in reviewing and testing is very appreciated.
> > > > > > 
> > > > > > I have tested this on my AMD Overdrive so
> > > > > > 
> > > > > > Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> > > > > > 
> > > > > > But to actually get my r8169 network card working I also need the
> > > > > > following patch.
> > > > > > 
> > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > > > index 2fbf840..40e24e2 100644
> > > > > > --- a/drivers/pci/probe.c
> > > > > > +++ b/drivers/pci/probe.c
> > > > > > @@ -1717,7 +1717,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > > > > >         set_dev_node(&dev->dev, pcibus_to_node(bus));
> > > > > >         dev->dev.dma_mask = &dev->dma_mask;
> > > > > >         dev->dev.dma_parms = &dev->dma_parms;
> > > > > > -       dev->dev.coherent_dma_mask = 0xffffffffull;
> > > > > > +       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> > > > > >         pci_dma_configure(dev);
> > > > > > 
> > > > > >         pci_set_dma_max_seg_size(dev, 65536);
> > > > > > 
> > > > > > I suspect we need to set coherent_dma_mask somewhere in the platform but I do
> > > > > > not know where this should happen. Hopefully an ARM64 expert can help.
> > > > > 
> > > > > I've run into a number of cards which don't work on some arm64 machines where there are
> > > > > no DMA address below 4G. For the realtek card you can use r8169.use_dac=1 on the cmdline
> > > > > to get it to work.
> > > > > 
> > > > I need r8169.use_dac=1 and this patch to get card to work.
> > > > 
> > > 
> > > Hmm. I didn't need a patch in the past but I don't think I've used that card with
> > > this patch series. I will try that when I get a chance...
> > > 
> > 
> > From my previous debugging on this issue I was getting an address bigger
> > than 32bits. As r8169 is a whole range of chips are guess they are not
> > all equal?
> > 
> > I can stick debug stuff in if someone tells me where to put them.
> > 
> 
> After some private debugging with Mark it turned out that the difference
> between our configurations was I did not have.
> 
> CONFIG_DMA_CMA=y
> 
> With this enabled then the card works without coherent mask hack.

I didn't follow the whole discussion here, but is this a case where
the driver could fail more gracefully than it did?  Can we do anything
to make this easier for the next person who trips over the same problem?

Bjorn
Russell King - ARM Linux Jan. 19, 2016, 8:40 p.m. UTC | #11
On Tue, Jan 19, 2016 at 02:25:13PM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 18, 2016 at 02:04:05PM +0000, Graeme Gregory wrote:
> > After some private debugging with Mark it turned out that the difference
> > between our configurations was I did not have.
> > 
> > CONFIG_DMA_CMA=y
> > 
> > With this enabled then the card works without coherent mask hack.
> 
> I didn't follow the whole discussion here, but is this a case where
> the driver could fail more gracefully than it did?  Can we do anything
> to make this easier for the next person who trips over the same problem?

I've not followed the discussion at all, but reading what was in the
quoted parts of the mail makes me somewhat suspicious.

The way PCI drivers (or in fact any driver) are supposed to work is:

- the bus code sets up a default mask (32-bit DMA in the case of PCI)
- the driver calls dma_set_mask() or similar function with the mask
  the _driver_ wants to use.  Arch DMA code decides whether the mask
  can be supported, and if it can, it re-sets the DMA mask.  If the
  mask can't be supported, it returns an error.

The driver is doing things correctly:

        if ((sizeof(dma_addr_t) > 4) &&
            !pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) && use_dac) {
                tp->cp_cmd |= PCIDAC;
                dev->features |= NETIF_F_HIGHDMA;
        } else {
                rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));

which says: if the size of a DMA address supports 64-bit, and we can
set a 64-bit DMA address mask (iow, the platform allows it), and
we're permitted to use DAC, use DAC, otherwise try to set a 32-bit
DMA mask.

It shouldn't matter one bit what the mask is before that point.

However, what the driver fails to do is to deal with the coherent
mask - it appears to be missing a call to pci_set_consistent_dma_mask().
That may be where the issue is.

It may be worth doing what I did with the DMA API, and introducing
pci_set_mask_and_coherent() which sets both masks together - which
will probably allow some PCI driver code to be simplified.
Mark Salter Jan. 19, 2016, 11:37 p.m. UTC | #12
On Tue, 2016-01-19 at 20:40 +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 19, 2016 at 02:25:13PM -0600, Bjorn Helgaas wrote:
> > On Mon, Jan 18, 2016 at 02:04:05PM +0000, Graeme Gregory wrote:
> > > After some private debugging with Mark it turned out that the difference
> > > between our configurations was I did not have.
> > > 
> > > CONFIG_DMA_CMA=y
> > > 
> > > With this enabled then the card works without coherent mask hack.
> > 
> > I didn't follow the whole discussion here, but is this a case where
> > the driver could fail more gracefully than it did?  Can we do anything
> > to make this easier for the next person who trips over the same problem?
> 
> I've not followed the discussion at all, but reading what was in the
> quoted parts of the mail makes me somewhat suspicious.
> 
> The way PCI drivers (or in fact any driver) are supposed to work is:
> 
> - the bus code sets up a default mask (32-bit DMA in the case of PCI)
> - the driver calls dma_set_mask() or similar function with the mask
>   the _driver_ wants to use.  Arch DMA code decides whether the mask
>   can be supported, and if it can, it re-sets the DMA mask.  If the
>   mask can't be supported, it returns an error.
> 
> The driver is doing things correctly:
> 
>         if ((sizeof(dma_addr_t) > 4) &&
>             !pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) && use_dac) {
>                 tp->cp_cmd |= PCIDAC;
>                 dev->features |= NETIF_F_HIGHDMA;
>         } else {
>                 rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> 
> which says: if the size of a DMA address supports 64-bit, and we can
> set a 64-bit DMA address mask (iow, the platform allows it), and
> we're permitted to use DAC, use DAC, otherwise try to set a 32-bit
> DMA mask.
> 
> It shouldn't matter one bit what the mask is before that point.
> 
> However, what the driver fails to do is to deal with the coherent
> mask - it appears to be missing a call to pci_set_consistent_dma_mask().
> That may be where the issue is.

It is.

The driver works with devicetree because of_dma_configure() sets up dma
and coherent mask for PCI devices based on root bridge info in the DT.
So the driver gets by without setting coherent mask explicitly. In the
ACPI case, the coherent mask doesn't get set because there isn't DMA
info in the ACPI tables (I think IORT table/IOMMU support will add this
in the future). Using CONFIG_DMA_CMA avoids the issue by allowing CMA
memory to be used for DMA while avoiding a check against coherent mask.

> 
> It may be worth doing what I did with the DMA API, and introducing
> pci_set_mask_and_coherent() which sets both masks together - which
> will probably allow some PCI driver code to be simplified.
>
diff mbox

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2fbf840..40e24e2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1717,7 +1717,7 @@  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
        set_dev_node(&dev->dev, pcibus_to_node(bus));
        dev->dev.dma_mask = &dev->dma_mask;
        dev->dev.dma_parms = &dev->dma_parms;
-       dev->dev.coherent_dma_mask = 0xffffffffull;
+       dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
        pci_dma_configure(dev);

        pci_set_dma_max_seg_size(dev, 65536);