Re: [PATCH v3 01/14] PCI: tegra: Convert to MSI domains

[Marc Zyngier <maz@kernel.org>]

Hi Jon,

On Mon, 19 Apr 2021 21:02:10 +0100,
Jon Hunter <jonathanh@nvidia.com> wrote:
> 
> 
> On 19/04/2021 20:19, Jon Hunter wrote:
> > Hi Marc,
> > 
> > On 30/03/2021 16:11, Marc Zyngier wrote:
> >> In anticipation of the removal of the msi_controller structure, convert
> >> the Tegra host controller driver to MSI domains.
> >>
> >> We end-up with the usual two domain structure, the top one being a
> >> generic PCI/MSI domain, the bottom one being Tegra-specific and handling
> >> the actual HW interrupt allocation.
> >>
> >> While at it, convert the normal interrupt handler to a chained handler,
> >> handle the controller's MSI IRQ edge triggered, support multiple MSIs
> >> per device and use the AFI_MSI_EN_VEC* registers to provide MSI masking.
> >>
> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >> [treding@nvidia.com: fix, clean up and address TODOs from Marc's draft]
> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >> Signed-off-by: Marc Zyngier <maz@kernel.org>
> > 
> > 
> > This change is breaking a suspend test that we are running on Tegra124
> > Jetson-TK1. The Tegra124 Jetson TK1 uses a PCI based ethernet device ...
> > 
> > $ lspci
> > 00:02.0 PCI bridge: NVIDIA Corporation TegraK1 PCIe x1 Bridge (rev a1)
> > 01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 0c)
> > 
> > After resuming from suspend, networking is no longer working. The reason
> > why this breaks our suspend test is because that setup is using NFS for
> > the rootfs. I am looking into it, but if anyone has any thoughts please
> > let me know.
> 
> 
> So the following does appear to fix it ...
> 
> diff --git a/drivers/pci/controller/pci-tegra.c
> b/drivers/pci/controller/pci-tegra.c
> index eaba7b2fab4a..558f02e0693d 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -1802,13 +1802,17 @@ static void tegra_pcie_enable_msi(struct
> tegra_pcie *pcie)
>  {
>         const struct tegra_pcie_soc *soc = pcie->soc;
>         struct tegra_msi *msi = &pcie->msi;
> -       u32 reg;
> +       u32 i, reg;
> 
>         afi_writel(pcie, msi->phys >> soc->msi_base_shift,
> AFI_MSI_FPCI_BAR_ST);
>         afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
>         /* this register is in 4K increments */
>         afi_writel(pcie, 1, AFI_MSI_BAR_SZ);
> 
> +       /* enable all MSI vectors */
> +       for (i = 0; i < 8; i++)
> +               afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC(i));
> +
>         /* and unmask the MSI interrupt */
>         reg = afi_readl(pcie, AFI_INTR_MASK);
>         reg |= AFI_INTR_MASK_MSI_MASK;
> @@ -1837,13 +1841,17 @@ static void tegra_pcie_msi_teardown(struct
> tegra_pcie *pcie)
> 
>  static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
>  {
> -       u32 value;
> +       u32 i, value;
> 
>         /* mask the MSI interrupt */
>         value = afi_readl(pcie, AFI_INTR_MASK);
>         value &= ~AFI_INTR_MASK_MSI_MASK;
>         afi_writel(pcie, value, AFI_INTR_MASK);
> 
> +       /* disable all MSI vectors */
> +       for (i = 0; i < 8; i++)
> +               afi_writel(pcie, 0, AFI_MSI_EN_VEC(i));
> +
>         return 0;
>  }
> 
> 
> Any reason why that code was removed?

Two reasons spring to mind: it obviously wasn't tested enough, and
part of the above code isn't quite right. You can't just blindly
enable all MSIs, irrespective of what has been actually enabled.

The following should hopefully cure it (compile tested only). Please
let me know.

	M.

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index eaba7b2fab4a..507b23d43ad1 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -1802,13 +1802,19 @@ static void tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 {
 	const struct tegra_pcie_soc *soc = pcie->soc;
 	struct tegra_msi *msi = &pcie->msi;
-	u32 reg;
+	u32 reg, msi_state[INT_PCI_MSI_NR / 32];
+	int i;
 
 	afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST);
 	/* this register is in 4K increments */
 	afi_writel(pcie, 1, AFI_MSI_BAR_SZ);
 
+	/* Restore the MSI allocation state */
+	bitmap_to_arr32(msi_state, msi->used, INT_PCI_MSI_NR);
+	for (i = 0; i < ARRAY_SIZE(msi_state); i++)
+		afi_writel(pcie, msi_state[i], AFI_MSI_EN_VEC(i));
+
 	/* and unmask the MSI interrupt */
 	reg = afi_readl(pcie, AFI_INTR_MASK);
 	reg |= AFI_INTR_MASK_MSI_MASK;

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek