Re: [PATCH] PCI: dwc: Move allocate and map page for msi out of dw_pcie_msi_init()

[Rob Herring]

On Thu, Sep 24, 2020 at 8:33 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Thu, 24 Sep 2020 at 15:45, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> >
> > On Thu, 24 Sep 2020 at 15:28, Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Sep 24, 2020 at 5:00 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, 23 Sep 2020 at 08:28, Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> > > > > program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> > > > > may lose power during suspend-to-RAM, so when we resume, we want to
> > > > > redo the latter but not the former. If designware based driver (for
> > > > > example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the
> > > > > previous msi page will be leaked.
> > > > >
> > > > > Move the allocate and map msi page from dw_pcie_msi_init() to
> > > > > dw_pcie_host_init() to fix this problem.
> > > > >
> > > > > Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
> > > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
> > > >
> > > > Why do you allocate a page for this in the first place? Isn't
> > > > PCIE_MSI_ADDR_HI:PCIE_MSI_ADDR_LO simply a magic DMA address that
> > > > never gets forwarded across to the CPU side of the host bridge, and
> > > > triggers a SPI instead, which gets handled by reading
> > > > PCIE_MSI_INTR0_STATUS ?
> > >
> > > My question too after digging into this some more. I've asked the
> > > question on the thread that further complicated all this changing from
> > > virt_to_phys() to dma_map_page()[1].
> > >
> > > > Couldn't you just map the zero page instead?
> > >
> > > Why a page even? You could use PCIE_MSI_ADDR_LO address itself even.
> > > Or just an address in the driver data which is what some other drivers
> > > do.
> > >
> >
> > PCIE_MSI_ADDR_LO itself could collide with an actual DRAM address if
> > any translation is applied on inbound transactions. Using an actual
> > DRAM address avoids that.

Good point, and the inbound windows could be less than all DRAM if
there's any restrictions. However, the DWC doesn't do any inbound
setup (at least for platforms with iATU), so I guess the default is
all traffic is forwarded.

> ... although the MSI doorbell register on the GIC, for instance, needs
> to be DMA addressable as well, of course.

There's at least one DWC driver that has its own doorbell register
too. I suppose there's a way for the host driver to retrieve the GIC
address if configuration is needed. Doesn't seem to be needed on DWC
given the above.

Rob