[PATCH] PCI: tegra: Use different MSI target address for Tegra20

[PATCH] PCI: tegra: Use different MSI target address for Tegra20

[Thierry Reding]

The Tegra20 PCIe controller has a different address range for MSI, so
select a different target address.

Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/host/pci-tegra.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index e8e1ddbaabc9..5b02ea59524b 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	 * none of the Tegra SoCs that contain this PCI host bridge can
 	 * address more than 16 GiB of system memory, the last 4 KiB of
 	 * these 1012 GiB is a good candidate.
+	 *
+	 * Unfortunately, Tegra20 is slightly different in that the physical
+	 * address for this MSI region is limited to the lower 32 bits of the
+	 * address map, so the address that we pick is going to have to be
+	 * located somewhere within the region addressable by the CPU and
+	 * on-SoC controllers. To be on the safe side, we select an address
+	 * from a region that is marked unused (0xf0010000 - 0xfffeffff).
 	 */
-	msi->phys = 0xfcfffff000;
+	if (soc->msi_base_shift > 0)
+		msi->phys = 0xfcfffff000;
+	else
+		msi->phys = 0x00f0010000;
 
 	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
-- 
2.14.1

Re: [PATCH] PCI: tegra: Use different MSI target address for Tegra20

[vidya sagar]

Inline...

On Sat, Sep 23, 2017 at 11:47 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> The Tegra20 PCIe controller has a different address range for MSI, so
> select a different target address.
>
> Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/host/pci-tegra.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index e8e1ddbaabc9..5b02ea59524b 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>          * none of the Tegra SoCs that contain this PCI host bridge can
>          * address more than 16 GiB of system memory, the last 4 KiB of
>          * these 1012 GiB is a good candidate.
> +        *
> +        * Unfortunately, Tegra20 is slightly different in that the physical
> +        * address for this MSI region is limited to the lower 32 bits of the
> +        * address map, so the address that we pick is going to have to be
> +        * located somewhere within the region addressable by the CPU and
> +        * on-SoC controllers. To be on the safe side, we select an address
> +        * from a region that is marked unused (0xf0010000 - 0xfffeffff).
>          */
> -       msi->phys = 0xfcfffff000;
> +       if (soc->msi_base_shift > 0)
> +               msi->phys = 0xfcfffff000;
> +       else
> +               msi->phys = 0x00f0010000;

Can we use this for later Tegra chip versions as well?
Reason being, if an end point's config space says that it cannot
support >32-bit MSI addresses (Marvel SATA controller being one
example), with the current code, we still allocate an address of
>32-bits, resulting in end point not being able to generate MSI
interrupt (as it discards >32-bits where generating upstream memory
write transaction for MSI)

>
>         afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
>         afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
> --
> 2.14.1
>

Re: [PATCH] PCI: tegra: Use different MSI target address for Tegra20

[Thierry Reding]

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

On Mon, Sep 25, 2017 at 10:44:29AM +0530, vidya sagar wrote:
> Inline...
> 
> On Sat, Sep 23, 2017 at 11:47 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >
> > The Tegra20 PCIe controller has a different address range for MSI, so
> > select a different target address.
> >
> > Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/pci/host/pci-tegra.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > index e8e1ddbaabc9..5b02ea59524b 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> >          * none of the Tegra SoCs that contain this PCI host bridge can
> >          * address more than 16 GiB of system memory, the last 4 KiB of
> >          * these 1012 GiB is a good candidate.
> > +        *
> > +        * Unfortunately, Tegra20 is slightly different in that the physical
> > +        * address for this MSI region is limited to the lower 32 bits of the
> > +        * address map, so the address that we pick is going to have to be
> > +        * located somewhere within the region addressable by the CPU and
> > +        * on-SoC controllers. To be on the safe side, we select an address
> > +        * from a region that is marked unused (0xf0010000 - 0xfffeffff).
> >          */
> > -       msi->phys = 0xfcfffff000;
> > +       if (soc->msi_base_shift > 0)
> > +               msi->phys = 0xfcfffff000;
> > +       else
> > +               msi->phys = 0x00f0010000;
> 
> Can we use this for later Tegra chip versions as well?
> Reason being, if an end point's config space says that it cannot
> support >32-bit MSI addresses (Marvel SATA controller being one
> example), with the current code, we still allocate an address of
> >32-bits, resulting in end point not being able to generate MSI
> interrupt (as it discards >32-bits where generating upstream memory
> write transaction for MSI)

I hadn't considered this case. My recollection was that the PCI express
specification required every endpoint to support 64-bit MSI, but looking
again, legacy endpoints are allowed to support 32-bit MSI only, and we
could technically have one of those connected via a PCI-to-PCIe bridge.

The reason I chose 0xf0010000 for Tegra20 is because that is part of a
region that has never been used. For Tegra30 and later that address is
within the address range of system memory.

Testing has shown that the memory at the destination address isn't
actually written (the PCI host bridge seems to decode the address and
notice that it is the MSI target address and not commit the data to
memory), so technically it shouldn't matter what address we use. I'm
somewhat reluctant to use a random address, though, because having it
point to something within system memory could get confusing (if that
address shows up somewhere, how are people supposed to know this has
to do with MSI?).

Another possibility would be to use some address from within the PCIe
controller's address space that is unused. We have the following as the
current mappings:

  Tegra186:
    0x10000000-0x10000fff  port 0 configuration space
    0x10001000-0x10001fff  port 1 configuration space
    0x10003000-0x100037ff  PADS registers
    0x10003800-0x10003fff  AFI registers
    0x10004000-0x10004fff  port 2 configuration space
    0x40000000-0x4fffffff  configuration space
    0x50000000-0x5000ffff  downstream I/O
    0x50100000-0x57ffffff  non-prefetchable memory
    0x58000000-0x7fffffff  prefetchable memory

  Tegra210 and Tegra124:
    0x01000000-0x01000fff  port 0 configuration space
    0x01001000-0x01001fff  port 1 configuration space
    0x01003000-0x010037ff  PADS registers
    0x01003800-0x01003fff  AFI registers
    0x02000000-0x11ffffff  configuration space
    0x12000000-0x1200ffff  downstream I/O
    0x13000000-0x1fffffff  non-prefetchable memory
    0x20000000-0x3fffffff  prefetchable memory

  Tegra30:
    0x00000000-0x00000fff  port 0 configuration space
    0x00001000-0x00001fff  port 1 configuration space
    0x00003000-0x000037ff  PADS registers
    0x00003800-0x00003fff  AFI registers
    0x00004000-0x00004fff  port 2 configuration space
    0x02000000-0x0200ffff  downstream I/O
    0x10000000-0x1fffffff  configuration space
    0x20000000-0x27ffffff  non-prefetchable memory
    0x28000000-0x3fffffff  prefetchable memory

  Tegra20:
    0x80000000-0x80000fff  port 0 configuration space
    0x80001000-0x80001fff  port 1 configuration space
    0x80003000-0x800037ff  PADS registers
    0x80003800-0x80003fff  AFI registers
    0x82000000-0x8200ffff  downstream I/O
    0x90000000-0x9fffffff  configuration space
    0xa0000000-0xa07fffff  non-prefetchable memory
    0xa8000000-0xbfffffff  prefetchable memory

Something like the following might work as MSI target addresses, though
I haven't tested that these actually work as expected:

  Tegra186: 0x10fff000-0x10ffffff
  Tegra210: 0x01fff000-0x01ffffff
  Tegra124: 0x01fff000-0x01ffffff
  Tegra30:  0x01fff000-0x01ffffff
  Tegra20:  0x81fff000-0x81ffffff

Any thought on that?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] PCI: tegra: Use different MSI target address for Tegra20

[Thierry Reding]

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

On Mon, Sep 25, 2017 at 10:44:29AM +0530, vidya sagar wrote:
> Inline...
> 
> On Sat, Sep 23, 2017 at 11:47 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> >
> > The Tegra20 PCIe controller has a different address range for MSI, so
> > select a different target address.
> >
> > Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/pci/host/pci-tegra.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > index e8e1ddbaabc9..5b02ea59524b 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> >          * none of the Tegra SoCs that contain this PCI host bridge can
> >          * address more than 16 GiB of system memory, the last 4 KiB of
> >          * these 1012 GiB is a good candidate.
> > +        *
> > +        * Unfortunately, Tegra20 is slightly different in that the physical
> > +        * address for this MSI region is limited to the lower 32 bits of the
> > +        * address map, so the address that we pick is going to have to be
> > +        * located somewhere within the region addressable by the CPU and
> > +        * on-SoC controllers. To be on the safe side, we select an address
> > +        * from a region that is marked unused (0xf0010000 - 0xfffeffff).
> >          */
> > -       msi->phys = 0xfcfffff000;
> > +       if (soc->msi_base_shift > 0)
> > +               msi->phys = 0xfcfffff000;
> > +       else
> > +               msi->phys = 0x00f0010000;
> 
> Can we use this for later Tegra chip versions as well?
> Reason being, if an end point's config space says that it cannot
> support >32-bit MSI addresses (Marvel SATA controller being one
> example), with the current code, we still allocate an address of
> >32-bits, resulting in end point not being able to generate MSI
> interrupt (as it discards >32-bits where generating upstream memory
> write transaction for MSI)

Looks like the universal fix for this may not be trivial, so perhaps in
the meantime this one could go in in order to restore MSI for TrimSlice
(and other Tegra20 boards)?

Bjorn: do you have any objections to taking this into v4.13 via a stable
backport? 32-bit only MSI were never guaranteed to work because prior to
this patch the address was taken from an allocated page, which may or
may not have been > 4 GiB.

We could then go and fix 32-bit MSI hopefully in time for 4.14, but it'd
be a shame if v4.13 keeps being broken just because we're trying to
settle on the right solution for 32-bit MSI.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] PCI: tegra: Use different MSI target address for Tegra20

[Bjorn Helgaas]

On Thu, Sep 28, 2017 at 04:19:12PM +0200, Thierry Reding wrote:
> On Mon, Sep 25, 2017 at 10:44:29AM +0530, vidya sagar wrote:
> > Inline...
> > 
> > On Sat, Sep 23, 2017 at 11:47 AM, Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> > >
> > > The Tegra20 PCIe controller has a different address range for MSI, so
> > > select a different target address.
> > >
> > > Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  drivers/pci/host/pci-tegra.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > index e8e1ddbaabc9..5b02ea59524b 100644
> > > --- a/drivers/pci/host/pci-tegra.c
> > > +++ b/drivers/pci/host/pci-tegra.c
> > > @@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> > >          * none of the Tegra SoCs that contain this PCI host bridge can
> > >          * address more than 16 GiB of system memory, the last 4 KiB of
> > >          * these 1012 GiB is a good candidate.
> > > +        *
> > > +        * Unfortunately, Tegra20 is slightly different in that the physical
> > > +        * address for this MSI region is limited to the lower 32 bits of the
> > > +        * address map, so the address that we pick is going to have to be
> > > +        * located somewhere within the region addressable by the CPU and
> > > +        * on-SoC controllers. To be on the safe side, we select an address
> > > +        * from a region that is marked unused (0xf0010000 - 0xfffeffff).
> > >          */
> > > -       msi->phys = 0xfcfffff000;
> > > +       if (soc->msi_base_shift > 0)
> > > +               msi->phys = 0xfcfffff000;
> > > +       else
> > > +               msi->phys = 0x00f0010000;
> > 
> > Can we use this for later Tegra chip versions as well?
> > Reason being, if an end point's config space says that it cannot
> > support >32-bit MSI addresses (Marvel SATA controller being one
> > example), with the current code, we still allocate an address of
> > >32-bits, resulting in end point not being able to generate MSI
> > interrupt (as it discards >32-bits where generating upstream memory
> > write transaction for MSI)
> 
> Looks like the universal fix for this may not be trivial, so perhaps in
> the meantime this one could go in in order to restore MSI for TrimSlice
> (and other Tegra20 boards)?
> 
> Bjorn: do you have any objections to taking this into v4.13 via a stable
> backport? 32-bit only MSI were never guaranteed to work because prior to
> this patch the address was taken from an allocated page, which may or
> may not have been > 4 GiB.
> 
> We could then go and fix 32-bit MSI hopefully in time for 4.14, but it'd
> be a shame if v4.13 keeps being broken just because we're trying to
> settle on the right solution for 32-bit MSI.

I don't know your hardware, so I don't have any suggestions about your
strategy for choosing addresses.

To me it looks slightly obscure to use "soc->msi_base_shift == 0" to
identify SoCs that can only use 32-bit MSI, but maybe that makes sense
if you know the hardware.

The stable backport policy is that the fix must first exist in Linus'
tree.  The normal path is for fixes to be merged for v4.15, so they
wouldn't appear in Linus' tree until after v4.14 releases.

If I merge a fix via for-linus, it could be merged to Linus' tree
before v4.14 and could be backported to v4.13 before v4.14.  But we'd
have to make a case for doing that, e.g., this fixes a regression or
something we broke during the v4.14 merge window, or it's some other
serious bug fix.

Bjorn

Re: [PATCH] PCI: tegra: Use different MSI target address for Tegra20

[Thierry Reding]

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

On Thu, Sep 28, 2017 at 09:57:19PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 28, 2017 at 04:19:12PM +0200, Thierry Reding wrote:
> > On Mon, Sep 25, 2017 at 10:44:29AM +0530, vidya sagar wrote:
> > > Inline...
> > > 
> > > On Sat, Sep 23, 2017 at 11:47 AM, Thierry Reding
> > > <thierry.reding@gmail.com> wrote:
> > > >
> > > > The Tegra20 PCIe controller has a different address range for MSI, so
> > > > select a different target address.
> > > >
> > > > Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > >  drivers/pci/host/pci-tegra.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > index e8e1ddbaabc9..5b02ea59524b 100644
> > > > --- a/drivers/pci/host/pci-tegra.c
> > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > @@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> > > >          * none of the Tegra SoCs that contain this PCI host bridge can
> > > >          * address more than 16 GiB of system memory, the last 4 KiB of
> > > >          * these 1012 GiB is a good candidate.
> > > > +        *
> > > > +        * Unfortunately, Tegra20 is slightly different in that the physical
> > > > +        * address for this MSI region is limited to the lower 32 bits of the
> > > > +        * address map, so the address that we pick is going to have to be
> > > > +        * located somewhere within the region addressable by the CPU and
> > > > +        * on-SoC controllers. To be on the safe side, we select an address
> > > > +        * from a region that is marked unused (0xf0010000 - 0xfffeffff).
> > > >          */
> > > > -       msi->phys = 0xfcfffff000;
> > > > +       if (soc->msi_base_shift > 0)
> > > > +               msi->phys = 0xfcfffff000;
> > > > +       else
> > > > +               msi->phys = 0x00f0010000;
> > > 
> > > Can we use this for later Tegra chip versions as well?
> > > Reason being, if an end point's config space says that it cannot
> > > support >32-bit MSI addresses (Marvel SATA controller being one
> > > example), with the current code, we still allocate an address of
> > > >32-bits, resulting in end point not being able to generate MSI
> > > interrupt (as it discards >32-bits where generating upstream memory
> > > write transaction for MSI)
> > 
> > Looks like the universal fix for this may not be trivial, so perhaps in
> > the meantime this one could go in in order to restore MSI for TrimSlice
> > (and other Tegra20 boards)?
> > 
> > Bjorn: do you have any objections to taking this into v4.13 via a stable
> > backport? 32-bit only MSI were never guaranteed to work because prior to
> > this patch the address was taken from an allocated page, which may or
> > may not have been > 4 GiB.
> > 
> > We could then go and fix 32-bit MSI hopefully in time for 4.14, but it'd
> > be a shame if v4.13 keeps being broken just because we're trying to
> > settle on the right solution for 32-bit MSI.
> 
> I don't know your hardware, so I don't have any suggestions about your
> strategy for choosing addresses.
> 
> To me it looks slightly obscure to use "soc->msi_base_shift == 0" to
> identify SoCs that can only use 32-bit MSI, but maybe that makes sense
> if you know the hardware.

I should probably clarify this change. msi_base_shift == 0 applies only
to Tegra20. This is because the implementation of the PCI host bridge on
Tegra20 has a bug that doesn't actually shift the address written to the
AFI_MSI_FPCI_BAR_ST by 8 bits to the left for matching. This was fixed
in Tegra30 (and later), hence why we have this special case in the
driver. It works around the issue on Tegra20.

A side-effect of that is that MSIs on Tegra20 can only go to the lower
32 bits of the physical address space, whereas later chips can address
the full 40 bits.

As Vidya points out, using any MSI target address > 4 GiB causes devices
with 32-bit MSI only capability to not work because they will truncate
the address and therefore the MSI controller won't be able to match at
any time.

So a proper fix is to select an MSI target address < 4 GiB for all the
chips, which would fix the regression *and* enable 32-bit MSI. However,
currently the only known regression is on TrimSlice (Tegra20), where a
change in MSI target address will fix the regression.

For all other SoCs, commit d7bd554f27c9 ("PCI: tegra: Do not allocate
MSI target memory") technically regresses for 32-bit MSI only capable
endpoints. However, since before that commit the MSI target address was
derived from an allocated page, 32-bit MSI were not guaranteed to work
either. On any system with > 2 GiB of RAM the MSI target page is likely
to be located in a region that 32-bit MSI could not address.

> The stable backport policy is that the fix must first exist in Linus'
> tree.  The normal path is for fixes to be merged for v4.15, so they
> wouldn't appear in Linus' tree until after v4.14 releases.
> 
> If I merge a fix via for-linus, it could be merged to Linus' tree
> before v4.14 and could be backported to v4.13 before v4.14.  But we'd
> have to make a case for doing that, e.g., this fixes a regression or
> something we broke during the v4.14 merge window, or it's some other
> serious bug fix.

The offending commit d7bd554f27c9 ("PCI: tegra: Do not allocate MSI
target memory") was introduced in v4.13 and it causes a regression on
Tegra20, in particular the TrimSlice board. Other devices could be
affected, but the TrimSlice is the only one I'm aware of that people
still care about and use.

I think this meets all the criteria for merging via for-linus. I'd like
to see this fixed in v4.14 and backported to v4.13. I think we have two
options at this point: one is to apply this fix, which addresses the
TrimSlice regression but doesn't fix a potential regression on Tegra30
and later (I'm not aware of any being reported, though). I have a local
patch (attached), which should also fix that. An alternative would be to
revert the offending commit (it reverts cleanly on linux-next). In both
cases the fix/revert should be backported to v4.13.

Vidya, Jon, Mikko: can you test the attached patch? It works for me on
Tegra20 (TrimSlice), Tegra30 (Beaver), Tegra124 (Jetson TK1) and
Tegra210 (Jetson TX1, with a Realtek NIC attached). It'd be good to get
some broader coverage, though.

Thierry
--- >8 ---
From 36108b8facc443170f91d64bb8d6bed79f5de0ba Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Wed, 27 Sep 2017 11:42:31 +0200
Subject: [PATCH] PCI: tegra: Enable the use of 32-bit MSI

Using a value above the 32-bit boundary as MSI target address prevents
devices that are only 32-bit MSI capable from using MSI for signalling
interrupts.

Fix this by relocating the MSI target address to somewhere within the
region in the address map reserved for PCI. Addresses were chosen to
be located in unused regions in the default configurations. While some
configuration could redefine the regions to include the new MSI target
addresses, doing so shouldn't cause any issues because this memory is
not actually accessed by MSI.

Reported-by: Vidya Sagar <vidias@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/host/pci-tegra.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 5b02ea59524b..1026f9dbbbd7 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -242,6 +242,7 @@ struct tegra_msi {
 struct tegra_pcie_soc {
 	unsigned int num_ports;
 	unsigned int msi_base_shift;
+	u64 msi_target;
 	u32 pads_pll_ctl;
 	u32 tx_ref_sel;
 	u32 pads_refclk_cfg0;
@@ -1554,27 +1555,24 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	 * MSI writes, which means that the MSI target address doesn't have
 	 * to point to actual physical memory. Rather than allocating one 4
 	 * KiB page of system memory that's never used, we can simply pick
-	 * an arbitrary address within an area reserved for system memory
-	 * in the FPCI address map.
+	 * an arbitrary address within a reserved area in the FPCI address
+	 * map.
 	 *
-	 * However, in order to avoid confusion, we pick an address that
-	 * doesn't map to physical memory. The FPCI address map reserves a
-	 * 1012 GiB region for system memory and memory-mapped I/O. Since
-	 * none of the Tegra SoCs that contain this PCI host bridge can
-	 * address more than 16 GiB of system memory, the last 4 KiB of
-	 * these 1012 GiB is a good candidate.
+	 * Note that MSI writes are never committed to memory, so it doesn't
+	 * really matter which address we pick. However, since the address
+	 * may show up at some point and potentially confuse users, the best
+	 * solution would be to pick an address that is obviously not within
+	 * system memory. The FPCI address map contains a range that cannot
+	 * be accessed by any Tegra CPU, but unfortunately that excludes the
+	 * usage for 32-bit MSI capable devices.
 	 *
-	 * Unfortunately, Tegra20 is slightly different in that the physical
-	 * address for this MSI region is limited to the lower 32 bits of the
-	 * address map, so the address that we pick is going to have to be
-	 * located somewhere within the region addressable by the CPU and
-	 * on-SoC controllers. To be on the safe side, we select an address
-	 * from a region that is marked unused (0xf0010000 - 0xfffeffff).
+	 * Instead, in order to support both 32-bit and 64-bit MSI, choose a
+	 * memory address within the PCI MMIO region that is currently not
+	 * used for configuration space, downstream I/O, prefetchable or non
+	 * prefetchable memory. This is hard-coded on a per-chip basis and
+	 * is hopefully clear enough not to confuse anyone.
 	 */
-	if (soc->msi_base_shift > 0)
-		msi->phys = 0xfcfffff000;
-	else
-		msi->phys = 0x00f0010000;
+	msi->phys = soc->msi_target;
 
 	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
@@ -2097,6 +2095,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 static const struct tegra_pcie_soc tegra20_pcie = {
 	.num_ports = 2,
 	.msi_base_shift = 0,
+	.msi_target = 0x81fff000,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA20,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
 	.pads_refclk_cfg0 = 0xfa5cfa5c,
@@ -2111,6 +2110,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 static const struct tegra_pcie_soc tegra30_pcie = {
 	.num_ports = 3,
 	.msi_base_shift = 8,
+	.msi_target = 0x01fff000,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0xfa5cfa5c,
@@ -2126,6 +2126,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 static const struct tegra_pcie_soc tegra124_pcie = {
 	.num_ports = 2,
 	.msi_base_shift = 8,
+	.msi_target = 0x01fff000,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0x44ac44ac,
@@ -2140,6 +2141,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 static const struct tegra_pcie_soc tegra210_pcie = {
 	.num_ports = 2,
 	.msi_base_shift = 8,
+	.msi_target = 0x01fff000,
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0x90b890b8,
-- 
2.14.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] PCI: tegra: Use different MSI target address for Tegra20

[Bjorn Helgaas]

On Fri, Sep 29, 2017 at 09:22:07AM +0200, Thierry Reding wrote:
> On Thu, Sep 28, 2017 at 09:57:19PM -0500, Bjorn Helgaas wrote:
> > On Thu, Sep 28, 2017 at 04:19:12PM +0200, Thierry Reding wrote:
> > > On Mon, Sep 25, 2017 at 10:44:29AM +0530, vidya sagar wrote:
> > > > Inline...
> > > > 
> > > > On Sat, Sep 23, 2017 at 11:47 AM, Thierry Reding
> > > > <thierry.reding@gmail.com> wrote:
> > > > >
> > > > > The Tegra20 PCIe controller has a different address range for MSI, so
> > > > > select a different target address.
> > > > >
> > > > > Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
> > > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > > ---
> > > > >  drivers/pci/host/pci-tegra.c | 12 +++++++++++-
> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > > index e8e1ddbaabc9..5b02ea59524b 100644
> > > > > --- a/drivers/pci/host/pci-tegra.c
> > > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > > @@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> > > > >          * none of the Tegra SoCs that contain this PCI host bridge can
> > > > >          * address more than 16 GiB of system memory, the last 4 KiB of
> > > > >          * these 1012 GiB is a good candidate.
> > > > > +        *
> > > > > +        * Unfortunately, Tegra20 is slightly different in that the physical
> > > > > +        * address for this MSI region is limited to the lower 32 bits of the
> > > > > +        * address map, so the address that we pick is going to have to be
> > > > > +        * located somewhere within the region addressable by the CPU and
> > > > > +        * on-SoC controllers. To be on the safe side, we select an address
> > > > > +        * from a region that is marked unused (0xf0010000 - 0xfffeffff).
> > > > >          */
> > > > > -       msi->phys = 0xfcfffff000;
> > > > > +       if (soc->msi_base_shift > 0)
> > > > > +               msi->phys = 0xfcfffff000;
> > > > > +       else
> > > > > +               msi->phys = 0x00f0010000;
> > > > 
> > > > Can we use this for later Tegra chip versions as well?
> > > > Reason being, if an end point's config space says that it cannot
> > > > support >32-bit MSI addresses (Marvel SATA controller being one
> > > > example), with the current code, we still allocate an address of
> > > > >32-bits, resulting in end point not being able to generate MSI
> > > > interrupt (as it discards >32-bits where generating upstream memory
> > > > write transaction for MSI)
> > > 
> > > Looks like the universal fix for this may not be trivial, so perhaps in
> > > the meantime this one could go in in order to restore MSI for TrimSlice
> > > (and other Tegra20 boards)?
> > > 
> > > Bjorn: do you have any objections to taking this into v4.13 via a stable
> > > backport? 32-bit only MSI were never guaranteed to work because prior to
> > > this patch the address was taken from an allocated page, which may or
> > > may not have been > 4 GiB.
> > > 
> > > We could then go and fix 32-bit MSI hopefully in time for 4.14, but it'd
> > > be a shame if v4.13 keeps being broken just because we're trying to
> > > settle on the right solution for 32-bit MSI.
> > 
> > I don't know your hardware, so I don't have any suggestions about your
> > strategy for choosing addresses.
> > 
> > To me it looks slightly obscure to use "soc->msi_base_shift == 0" to
> > identify SoCs that can only use 32-bit MSI, but maybe that makes sense
> > if you know the hardware.
> 
> I should probably clarify this change. msi_base_shift == 0 applies only
> to Tegra20. This is because the implementation of the PCI host bridge on
> Tegra20 has a bug that doesn't actually shift the address written to the
> AFI_MSI_FPCI_BAR_ST by 8 bits to the left for matching. This was fixed
> in Tegra30 (and later), hence why we have this special case in the
> driver. It works around the issue on Tegra20.
> 
> A side-effect of that is that MSIs on Tegra20 can only go to the lower
> 32 bits of the physical address space, whereas later chips can address
> the full 40 bits.
> 
> As Vidya points out, using any MSI target address > 4 GiB causes devices
> with 32-bit MSI only capability to not work because they will truncate
> the address and therefore the MSI controller won't be able to match at
> any time.
> 
> So a proper fix is to select an MSI target address < 4 GiB for all the
> chips, which would fix the regression *and* enable 32-bit MSI. However,
> currently the only known regression is on TrimSlice (Tegra20), where a
> change in MSI target address will fix the regression.
> 
> For all other SoCs, commit d7bd554f27c9 ("PCI: tegra: Do not allocate
> MSI target memory") technically regresses for 32-bit MSI only capable
> endpoints. However, since before that commit the MSI target address was
> derived from an allocated page, 32-bit MSI were not guaranteed to work
> either. On any system with > 2 GiB of RAM the MSI target page is likely
> to be located in a region that 32-bit MSI could not address.
> 
> > The stable backport policy is that the fix must first exist in Linus'
> > tree.  The normal path is for fixes to be merged for v4.15, so they
> > wouldn't appear in Linus' tree until after v4.14 releases.
> > 
> > If I merge a fix via for-linus, it could be merged to Linus' tree
> > before v4.14 and could be backported to v4.13 before v4.14.  But we'd
> > have to make a case for doing that, e.g., this fixes a regression or
> > something we broke during the v4.14 merge window, or it's some other
> > serious bug fix.
> 
> The offending commit d7bd554f27c9 ("PCI: tegra: Do not allocate MSI
> target memory") was introduced in v4.13 and it causes a regression on
> Tegra20, in particular the TrimSlice board. Other devices could be
> affected, but the TrimSlice is the only one I'm aware of that people
> still care about and use.
> 
> I think this meets all the criteria for merging via for-linus. I'd like
> to see this fixed in v4.14 and backported to v4.13. I think we have two
> options at this point: one is to apply this fix, which addresses the
> TrimSlice regression but doesn't fix a potential regression on Tegra30
> and later (I'm not aware of any being reported, though). I have a local
> patch (attached), which should also fix that. An alternative would be to
> revert the offending commit (it reverts cleanly on linux-next). In both
> cases the fix/revert should be backported to v4.13.

Ok, I see three options and I can't tell which one you're suggesting:

  1) Apply "this fix" (I assume this is your original patch that tests
  msi_base_shift)

  2) Apply the patch below that adds a .msi_target field

  3) Revert d7bd554f27c9

I'd like your proposal, with a changelog that includes the rationale
for including in v4.14 and the stable backport indication.

I'm going to mark the patches in this thread as "changes requested" so
patchwork will pick up whatever resolution you propose.

> Vidya, Jon, Mikko: can you test the attached patch? It works for me on
> Tegra20 (TrimSlice), Tegra30 (Beaver), Tegra124 (Jetson TK1) and
> Tegra210 (Jetson TX1, with a Realtek NIC attached). It'd be good to get
> some broader coverage, though.
> 
> Thierry
> --- >8 ---
> From 36108b8facc443170f91d64bb8d6bed79f5de0ba Mon Sep 17 00:00:00 2001
> From: Thierry Reding <treding@nvidia.com>
> Date: Wed, 27 Sep 2017 11:42:31 +0200
> Subject: [PATCH] PCI: tegra: Enable the use of 32-bit MSI
> 
> Using a value above the 32-bit boundary as MSI target address prevents
> devices that are only 32-bit MSI capable from using MSI for signalling
> interrupts.
> 
> Fix this by relocating the MSI target address to somewhere within the
> region in the address map reserved for PCI. Addresses were chosen to
> be located in unused regions in the default configurations. While some
> configuration could redefine the regions to include the new MSI target
> addresses, doing so shouldn't cause any issues because this memory is
> not actually accessed by MSI.
> 
> Reported-by: Vidya Sagar <vidias@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/host/pci-tegra.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 5b02ea59524b..1026f9dbbbd7 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -242,6 +242,7 @@ struct tegra_msi {
>  struct tegra_pcie_soc {
>  	unsigned int num_ports;
>  	unsigned int msi_base_shift;
> +	u64 msi_target;
>  	u32 pads_pll_ctl;
>  	u32 tx_ref_sel;
>  	u32 pads_refclk_cfg0;
> @@ -1554,27 +1555,24 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>  	 * MSI writes, which means that the MSI target address doesn't have
>  	 * to point to actual physical memory. Rather than allocating one 4
>  	 * KiB page of system memory that's never used, we can simply pick
> -	 * an arbitrary address within an area reserved for system memory
> -	 * in the FPCI address map.
> +	 * an arbitrary address within a reserved area in the FPCI address
> +	 * map.
>  	 *
> -	 * However, in order to avoid confusion, we pick an address that
> -	 * doesn't map to physical memory. The FPCI address map reserves a
> -	 * 1012 GiB region for system memory and memory-mapped I/O. Since
> -	 * none of the Tegra SoCs that contain this PCI host bridge can
> -	 * address more than 16 GiB of system memory, the last 4 KiB of
> -	 * these 1012 GiB is a good candidate.
> +	 * Note that MSI writes are never committed to memory, so it doesn't
> +	 * really matter which address we pick. However, since the address
> +	 * may show up at some point and potentially confuse users, the best
> +	 * solution would be to pick an address that is obviously not within
> +	 * system memory. The FPCI address map contains a range that cannot
> +	 * be accessed by any Tegra CPU, but unfortunately that excludes the
> +	 * usage for 32-bit MSI capable devices.
>  	 *
> -	 * Unfortunately, Tegra20 is slightly different in that the physical
> -	 * address for this MSI region is limited to the lower 32 bits of the
> -	 * address map, so the address that we pick is going to have to be
> -	 * located somewhere within the region addressable by the CPU and
> -	 * on-SoC controllers. To be on the safe side, we select an address
> -	 * from a region that is marked unused (0xf0010000 - 0xfffeffff).
> +	 * Instead, in order to support both 32-bit and 64-bit MSI, choose a
> +	 * memory address within the PCI MMIO region that is currently not
> +	 * used for configuration space, downstream I/O, prefetchable or non
> +	 * prefetchable memory. This is hard-coded on a per-chip basis and
> +	 * is hopefully clear enough not to confuse anyone.
>  	 */
> -	if (soc->msi_base_shift > 0)
> -		msi->phys = 0xfcfffff000;
> -	else
> -		msi->phys = 0x00f0010000;
> +	msi->phys = soc->msi_target;
>  
>  	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
>  	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
> @@ -2097,6 +2095,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  static const struct tegra_pcie_soc tegra20_pcie = {
>  	.num_ports = 2,
>  	.msi_base_shift = 0,
> +	.msi_target = 0x81fff000,
>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA20,
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
>  	.pads_refclk_cfg0 = 0xfa5cfa5c,
> @@ -2111,6 +2110,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>  static const struct tegra_pcie_soc tegra30_pcie = {
>  	.num_ports = 3,
>  	.msi_base_shift = 8,
> +	.msi_target = 0x01fff000,
>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>  	.pads_refclk_cfg0 = 0xfa5cfa5c,
> @@ -2126,6 +2126,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>  static const struct tegra_pcie_soc tegra124_pcie = {
>  	.num_ports = 2,
>  	.msi_base_shift = 8,
> +	.msi_target = 0x01fff000,
>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>  	.pads_refclk_cfg0 = 0x44ac44ac,
> @@ -2140,6 +2141,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>  static const struct tegra_pcie_soc tegra210_pcie = {
>  	.num_ports = 2,
>  	.msi_base_shift = 8,
> +	.msi_target = 0x01fff000,
>  	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>  	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>  	.pads_refclk_cfg0 = 0x90b890b8,
> -- 
> 2.14.1

[PATCH] Revert "PCI: tegra: Do not allocate MSI target memory"

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This reverts commit d7bd554f27c942e6b8b54100b4044f9be1038edf.

It turns out that Tegra20 has a bug in the implementation of the MSI
target address register (which is worked around by the existence of the
struct tegra_pcie_soc.msi_base_shift parameter) that restricts the MSI
target memory to the lower 32 bits of physical memory on that particular
generation. The offending patch causes a regression on TrimSlice, which
is a Tegra20-based device and has a PCI network interface card.

An initial, simpler fix was to change the MSI target address for Tegra20
only, but it was pointed out that the offending commit also prevents the
use of 32-bit only MSI capable devices, even on later chips. Technically
this was never guaranteed to work with the prior code in the first place
because the allocated page could have resided beyond the 4 GiB boundary,
but it is still possible that this could've introduced a regression.

The proper fix that was settled on is to select a fixed address within
the lowest 32 bits of physical address space that is otherwise unused,
but testing of that patch has provided mixed results that are not fully
understood yet.

Given all of the above and the relative urgency to get this fixed in
v4.13, revert the offending commit until a universal fix is found.

Cc: stable@vger.kernel.org # 4.13.x
Reported-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/host/pci-tegra.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 9c40da54f88a..1987fec1f126 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -233,6 +233,7 @@ struct tegra_msi {
 	struct msi_controller chip;
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
 	struct irq_domain *domain;
+	unsigned long pages;
 	struct mutex lock;
 	u64 phys;
 	int irq;
@@ -1529,22 +1530,9 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 		goto err;
 	}
 
-	/*
-	 * The PCI host bridge on Tegra contains some logic that intercepts
-	 * MSI writes, which means that the MSI target address doesn't have
-	 * to point to actual physical memory. Rather than allocating one 4
-	 * KiB page of system memory that's never used, we can simply pick
-	 * an arbitrary address within an area reserved for system memory
-	 * in the FPCI address map.
-	 *
-	 * However, in order to avoid confusion, we pick an address that
-	 * doesn't map to physical memory. The FPCI address map reserves a
-	 * 1012 GiB region for system memory and memory-mapped I/O. Since
-	 * none of the Tegra SoCs that contain this PCI host bridge can
-	 * address more than 16 GiB of system memory, the last 4 KiB of
-	 * these 1012 GiB is a good candidate.
-	 */
-	msi->phys = 0xfcfffff000;
+	/* setup AFI/FPCI range */
+	msi->pages = __get_free_pages(GFP_KERNEL, 0);
+	msi->phys = virt_to_phys((void *)msi->pages);
 
 	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
@@ -1596,6 +1584,8 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
 	afi_writel(pcie, 0, AFI_MSI_EN_VEC6);
 	afi_writel(pcie, 0, AFI_MSI_EN_VEC7);
 
+	free_pages(msi->pages, 0);
+
 	if (msi->irq > 0)
 		free_irq(msi->irq, pcie);
 
-- 
2.14.1

Re: [PATCH] Revert "PCI: tegra: Do not allocate MSI target memory"

[Bjorn Helgaas]

On Mon, Oct 09, 2017 at 12:29:35PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This reverts commit d7bd554f27c942e6b8b54100b4044f9be1038edf.
> 
> It turns out that Tegra20 has a bug in the implementation of the MSI
> target address register (which is worked around by the existence of the
> struct tegra_pcie_soc.msi_base_shift parameter) that restricts the MSI
> target memory to the lower 32 bits of physical memory on that particular
> generation. The offending patch causes a regression on TrimSlice, which
> is a Tegra20-based device and has a PCI network interface card.
> 
> An initial, simpler fix was to change the MSI target address for Tegra20
> only, but it was pointed out that the offending commit also prevents the
> use of 32-bit only MSI capable devices, even on later chips. Technically
> this was never guaranteed to work with the prior code in the first place
> because the allocated page could have resided beyond the 4 GiB boundary,
> but it is still possible that this could've introduced a regression.
> 
> The proper fix that was settled on is to select a fixed address within
> the lowest 32 bits of physical address space that is otherwise unused,
> but testing of that patch has provided mixed results that are not fully
> understood yet.
> 
> Given all of the above and the relative urgency to get this fixed in
> v4.13, revert the offending commit until a universal fix is found.
> 
> Cc: stable@vger.kernel.org # 4.13.x
> Reported-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

I applied this revert to for-linus for v4.14, thanks!

> ---
>  drivers/pci/host/pci-tegra.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 9c40da54f88a..1987fec1f126 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -233,6 +233,7 @@ struct tegra_msi {
>  	struct msi_controller chip;
>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>  	struct irq_domain *domain;
> +	unsigned long pages;
>  	struct mutex lock;
>  	u64 phys;
>  	int irq;
> @@ -1529,22 +1530,9 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>  		goto err;
>  	}
>  
> -	/*
> -	 * The PCI host bridge on Tegra contains some logic that intercepts
> -	 * MSI writes, which means that the MSI target address doesn't have
> -	 * to point to actual physical memory. Rather than allocating one 4
> -	 * KiB page of system memory that's never used, we can simply pick
> -	 * an arbitrary address within an area reserved for system memory
> -	 * in the FPCI address map.
> -	 *
> -	 * However, in order to avoid confusion, we pick an address that
> -	 * doesn't map to physical memory. The FPCI address map reserves a
> -	 * 1012 GiB region for system memory and memory-mapped I/O. Since
> -	 * none of the Tegra SoCs that contain this PCI host bridge can
> -	 * address more than 16 GiB of system memory, the last 4 KiB of
> -	 * these 1012 GiB is a good candidate.
> -	 */
> -	msi->phys = 0xfcfffff000;
> +	/* setup AFI/FPCI range */
> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> +	msi->phys = virt_to_phys((void *)msi->pages);
>  
>  	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
>  	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
> @@ -1596,6 +1584,8 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
>  	afi_writel(pcie, 0, AFI_MSI_EN_VEC6);
>  	afi_writel(pcie, 0, AFI_MSI_EN_VEC7);
>  
> +	free_pages(msi->pages, 0);
> +
>  	if (msi->irq > 0)
>  		free_irq(msi->irq, pcie);
>  
> -- 
> 2.14.1
> 

Re: [PATCH] PCI: tegra: Use different MSI target address for Tegra20

[vidya sagar]

I checked this patch and there seems to be some issue with generating
MSI interrupts having these addresses as MSI target addresses.

On Tue, Oct 3, 2017 at 1:49 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Sep 29, 2017 at 09:22:07AM +0200, Thierry Reding wrote:
>> On Thu, Sep 28, 2017 at 09:57:19PM -0500, Bjorn Helgaas wrote:
>> > On Thu, Sep 28, 2017 at 04:19:12PM +0200, Thierry Reding wrote:
>> > > On Mon, Sep 25, 2017 at 10:44:29AM +0530, vidya sagar wrote:
>> > > > Inline...
>> > > >
>> > > > On Sat, Sep 23, 2017 at 11:47 AM, Thierry Reding
>> > > > <thierry.reding@gmail.com> wrote:
>> > > > >
>> > > > > The Tegra20 PCIe controller has a different address range for MSI, so
>> > > > > select a different target address.
>> > > > >
>> > > > > Fixes: d7bd554f27c9 ("PCI: tegra: Do not allocate MSI target memory")
>> > > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
>> > > > > ---
>> > > > >  drivers/pci/host/pci-tegra.c | 12 +++++++++++-
>> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
>> > > > >
>> > > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> > > > > index e8e1ddbaabc9..5b02ea59524b 100644
>> > > > > --- a/drivers/pci/host/pci-tegra.c
>> > > > > +++ b/drivers/pci/host/pci-tegra.c
>> > > > > @@ -1563,8 +1563,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>> > > > >          * none of the Tegra SoCs that contain this PCI host bridge can
>> > > > >          * address more than 16 GiB of system memory, the last 4 KiB of
>> > > > >          * these 1012 GiB is a good candidate.
>> > > > > +        *
>> > > > > +        * Unfortunately, Tegra20 is slightly different in that the physical
>> > > > > +        * address for this MSI region is limited to the lower 32 bits of the
>> > > > > +        * address map, so the address that we pick is going to have to be
>> > > > > +        * located somewhere within the region addressable by the CPU and
>> > > > > +        * on-SoC controllers. To be on the safe side, we select an address
>> > > > > +        * from a region that is marked unused (0xf0010000 - 0xfffeffff).
>> > > > >          */
>> > > > > -       msi->phys = 0xfcfffff000;
>> > > > > +       if (soc->msi_base_shift > 0)
>> > > > > +               msi->phys = 0xfcfffff000;
>> > > > > +       else
>> > > > > +               msi->phys = 0x00f0010000;
>> > > >
>> > > > Can we use this for later Tegra chip versions as well?
>> > > > Reason being, if an end point's config space says that it cannot
>> > > > support >32-bit MSI addresses (Marvel SATA controller being one
>> > > > example), with the current code, we still allocate an address of
>> > > > >32-bits, resulting in end point not being able to generate MSI
>> > > > interrupt (as it discards >32-bits where generating upstream memory
>> > > > write transaction for MSI)
>> > >
>> > > Looks like the universal fix for this may not be trivial, so perhaps in
>> > > the meantime this one could go in in order to restore MSI for TrimSlice
>> > > (and other Tegra20 boards)?
>> > >
>> > > Bjorn: do you have any objections to taking this into v4.13 via a stable
>> > > backport? 32-bit only MSI were never guaranteed to work because prior to
>> > > this patch the address was taken from an allocated page, which may or
>> > > may not have been > 4 GiB.
>> > >
>> > > We could then go and fix 32-bit MSI hopefully in time for 4.14, but it'd
>> > > be a shame if v4.13 keeps being broken just because we're trying to
>> > > settle on the right solution for 32-bit MSI.
>> >
>> > I don't know your hardware, so I don't have any suggestions about your
>> > strategy for choosing addresses.
>> >
>> > To me it looks slightly obscure to use "soc->msi_base_shift == 0" to
>> > identify SoCs that can only use 32-bit MSI, but maybe that makes sense
>> > if you know the hardware.
>>
>> I should probably clarify this change. msi_base_shift == 0 applies only
>> to Tegra20. This is because the implementation of the PCI host bridge on
>> Tegra20 has a bug that doesn't actually shift the address written to the
>> AFI_MSI_FPCI_BAR_ST by 8 bits to the left for matching. This was fixed
>> in Tegra30 (and later), hence why we have this special case in the
>> driver. It works around the issue on Tegra20.
>>
>> A side-effect of that is that MSIs on Tegra20 can only go to the lower
>> 32 bits of the physical address space, whereas later chips can address
>> the full 40 bits.
>>
>> As Vidya points out, using any MSI target address > 4 GiB causes devices
>> with 32-bit MSI only capability to not work because they will truncate
>> the address and therefore the MSI controller won't be able to match at
>> any time.
>>
>> So a proper fix is to select an MSI target address < 4 GiB for all the
>> chips, which would fix the regression *and* enable 32-bit MSI. However,
>> currently the only known regression is on TrimSlice (Tegra20), where a
>> change in MSI target address will fix the regression.
>>
>> For all other SoCs, commit d7bd554f27c9 ("PCI: tegra: Do not allocate
>> MSI target memory") technically regresses for 32-bit MSI only capable
>> endpoints. However, since before that commit the MSI target address was
>> derived from an allocated page, 32-bit MSI were not guaranteed to work
>> either. On any system with > 2 GiB of RAM the MSI target page is likely
>> to be located in a region that 32-bit MSI could not address.
>>
>> > The stable backport policy is that the fix must first exist in Linus'
>> > tree.  The normal path is for fixes to be merged for v4.15, so they
>> > wouldn't appear in Linus' tree until after v4.14 releases.
>> >
>> > If I merge a fix via for-linus, it could be merged to Linus' tree
>> > before v4.14 and could be backported to v4.13 before v4.14.  But we'd
>> > have to make a case for doing that, e.g., this fixes a regression or
>> > something we broke during the v4.14 merge window, or it's some other
>> > serious bug fix.
>>
>> The offending commit d7bd554f27c9 ("PCI: tegra: Do not allocate MSI
>> target memory") was introduced in v4.13 and it causes a regression on
>> Tegra20, in particular the TrimSlice board. Other devices could be
>> affected, but the TrimSlice is the only one I'm aware of that people
>> still care about and use.
>>
>> I think this meets all the criteria for merging via for-linus. I'd like
>> to see this fixed in v4.14 and backported to v4.13. I think we have two
>> options at this point: one is to apply this fix, which addresses the
>> TrimSlice regression but doesn't fix a potential regression on Tegra30
>> and later (I'm not aware of any being reported, though). I have a local
>> patch (attached), which should also fix that. An alternative would be to
>> revert the offending commit (it reverts cleanly on linux-next). In both
>> cases the fix/revert should be backported to v4.13.
>
> Ok, I see three options and I can't tell which one you're suggesting:
>
>   1) Apply "this fix" (I assume this is your original patch that tests
>   msi_base_shift)
>
>   2) Apply the patch below that adds a .msi_target field
>
>   3) Revert d7bd554f27c9
>
> I'd like your proposal, with a changelog that includes the rationale
> for including in v4.14 and the stable backport indication.
>
> I'm going to mark the patches in this thread as "changes requested" so
> patchwork will pick up whatever resolution you propose.
>
>> Vidya, Jon, Mikko: can you test the attached patch? It works for me on
>> Tegra20 (TrimSlice), Tegra30 (Beaver), Tegra124 (Jetson TK1) and
>> Tegra210 (Jetson TX1, with a Realtek NIC attached). It'd be good to get
>> some broader coverage, though.
>>
>> Thierry
>> --- >8 ---
>> From 36108b8facc443170f91d64bb8d6bed79f5de0ba Mon Sep 17 00:00:00 2001
>> From: Thierry Reding <treding@nvidia.com>
>> Date: Wed, 27 Sep 2017 11:42:31 +0200
>> Subject: [PATCH] PCI: tegra: Enable the use of 32-bit MSI
>>
>> Using a value above the 32-bit boundary as MSI target address prevents
>> devices that are only 32-bit MSI capable from using MSI for signalling
>> interrupts.
>>
>> Fix this by relocating the MSI target address to somewhere within the
>> region in the address map reserved for PCI. Addresses were chosen to
>> be located in unused regions in the default configurations. While some
>> configuration could redefine the regions to include the new MSI target
>> addresses, doing so shouldn't cause any issues because this memory is
>> not actually accessed by MSI.
>>
>> Reported-by: Vidya Sagar <vidias@nvidia.com>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>>  drivers/pci/host/pci-tegra.c | 38 ++++++++++++++++++++------------------
>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index 5b02ea59524b..1026f9dbbbd7 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -242,6 +242,7 @@ struct tegra_msi {
>>  struct tegra_pcie_soc {
>>       unsigned int num_ports;
>>       unsigned int msi_base_shift;
>> +     u64 msi_target;
>>       u32 pads_pll_ctl;
>>       u32 tx_ref_sel;
>>       u32 pads_refclk_cfg0;
>> @@ -1554,27 +1555,24 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>>        * MSI writes, which means that the MSI target address doesn't have
>>        * to point to actual physical memory. Rather than allocating one 4
>>        * KiB page of system memory that's never used, we can simply pick
>> -      * an arbitrary address within an area reserved for system memory
>> -      * in the FPCI address map.
>> +      * an arbitrary address within a reserved area in the FPCI address
>> +      * map.
>>        *
>> -      * However, in order to avoid confusion, we pick an address that
>> -      * doesn't map to physical memory. The FPCI address map reserves a
>> -      * 1012 GiB region for system memory and memory-mapped I/O. Since
>> -      * none of the Tegra SoCs that contain this PCI host bridge can
>> -      * address more than 16 GiB of system memory, the last 4 KiB of
>> -      * these 1012 GiB is a good candidate.
>> +      * Note that MSI writes are never committed to memory, so it doesn't
>> +      * really matter which address we pick. However, since the address
>> +      * may show up at some point and potentially confuse users, the best
>> +      * solution would be to pick an address that is obviously not within
>> +      * system memory. The FPCI address map contains a range that cannot
>> +      * be accessed by any Tegra CPU, but unfortunately that excludes the
>> +      * usage for 32-bit MSI capable devices.
>>        *
>> -      * Unfortunately, Tegra20 is slightly different in that the physical
>> -      * address for this MSI region is limited to the lower 32 bits of the
>> -      * address map, so the address that we pick is going to have to be
>> -      * located somewhere within the region addressable by the CPU and
>> -      * on-SoC controllers. To be on the safe side, we select an address
>> -      * from a region that is marked unused (0xf0010000 - 0xfffeffff).
>> +      * Instead, in order to support both 32-bit and 64-bit MSI, choose a
>> +      * memory address within the PCI MMIO region that is currently not
>> +      * used for configuration space, downstream I/O, prefetchable or non
>> +      * prefetchable memory. This is hard-coded on a per-chip basis and
>> +      * is hopefully clear enough not to confuse anyone.
>>        */
>> -     if (soc->msi_base_shift > 0)
>> -             msi->phys = 0xfcfffff000;
>> -     else
>> -             msi->phys = 0x00f0010000;
>> +     msi->phys = soc->msi_target;
>>
>>       afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
>>       afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
>> @@ -2097,6 +2095,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>>  static const struct tegra_pcie_soc tegra20_pcie = {
>>       .num_ports = 2,
>>       .msi_base_shift = 0,
>> +     .msi_target = 0x81fff000,
>>       .pads_pll_ctl = PADS_PLL_CTL_TEGRA20,
>>       .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
>>       .pads_refclk_cfg0 = 0xfa5cfa5c,
>> @@ -2111,6 +2110,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>>  static const struct tegra_pcie_soc tegra30_pcie = {
>>       .num_ports = 3,
>>       .msi_base_shift = 8,
>> +     .msi_target = 0x01fff000,
>>       .pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>>       .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>>       .pads_refclk_cfg0 = 0xfa5cfa5c,
>> @@ -2126,6 +2126,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>>  static const struct tegra_pcie_soc tegra124_pcie = {
>>       .num_ports = 2,
>>       .msi_base_shift = 8,
>> +     .msi_target = 0x01fff000,
>>       .pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>>       .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>>       .pads_refclk_cfg0 = 0x44ac44ac,
>> @@ -2140,6 +2141,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>>  static const struct tegra_pcie_soc tegra210_pcie = {
>>       .num_ports = 2,
>>       .msi_base_shift = 8,
>> +     .msi_target = 0x01fff000,
>>       .pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
>>       .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
>>       .pads_refclk_cfg0 = 0x90b890b8,
>> --
>> 2.14.1
>
>