Hi Thierry, On Fri, 04 Sep 2020 11:56:13 +0100, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > > From: Marc Zyngier <maz@xxxxxxxxxx> > > 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. > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx> > [treding@xxxxxxxxxx: fix, clean up and address TODOs from Marc's draft] > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> > --- > This is basically Marc's patch from here: > > https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/tegra-msi&id=8ba6858f07a7554d4e56a6d5103cd914dcf23af0 > > That didn't work as-is, but it was pretty easy to get it to build and > run properly. Once I had that I went through Marc's list of TODOs and > addressed them, which was also not very difficult. > > I haven't done any extensive testing, but I can boot fine over NFS with > a PCI Ethernet card. Given how fundamental this is, I would expect that > to be a good enough test. > > Marc, I've kept your authorship on this. There are substantial changes > from your version, but you had laid out the plan for all these changes, > so it seemed fair. However, let me know if you're not comfortable with > taking responsibility for any potential bugs I've introduced. Thanks a lot for picking this up at short notice. Definitely happy with you preserving the authorship, though I wouldn't object if you did it the other way around. > I had originally considered sending this out as a series with more > intermediate steps, but it turns out most of the TODO's that Marc had > listed are related, so this could've been at maximum two or three > patches. That didn't seem worth splitting up for. I think most of this > work is still all related to the MSI domain conversion, so it makes > sense to keep it in one patch. > > drivers/pci/controller/pci-tegra.c | 316 +++++++++++++++-------------- > 1 file changed, 165 insertions(+), 151 deletions(-) > > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c > index c1d34353c29b..f7f92718ce40 100644 > --- a/drivers/pci/controller/pci-tegra.c > +++ b/drivers/pci/controller/pci-tegra.c [...] > +static void tegra_msi_irq_mask(struct irq_data *d) > +{ > + struct tegra_msi *msi = irq_data_get_irq_chip_data(d); > + struct tegra_pcie *pcie = msi_to_pcie(msi); > + unsigned int index = d->hwirq / 32; > + u32 value; > + > + value = afi_readl(pcie, AFI_MSI_EN_VEC(index)); > + value &= ~BIT(d->hwirq % 32); > + afi_writel(pcie, value, AFI_MSI_EN_VEC(index)); You need some extra locking here, as this register looks shared with another 31 MSIs. Same for the unmask. [...] > +static int tegra_allocate_domains(struct tegra_msi *msi) > { > - irq_set_chip_and_handler(irq, &tegra_msi_irq_chip, handle_simple_irq); > - irq_set_chip_data(irq, domain->host_data); > + struct tegra_pcie *pcie = container_of(msi, struct tegra_pcie, msi); > + struct fwnode_handle *fwnode = of_node_to_fwnode(pcie->dev->of_node); > + struct irq_domain *parent; > > - tegra_cpuidle_pcie_irqs_in_use(); > + msi->top.name = "Tegra PCIe MSI"; > + msi->top.irq_ack = tegra_msi_top_irq_ack; > + msi->top.irq_mask = tegra_msi_top_irq_mask; > + msi->top.irq_unmask = tegra_msi_top_irq_unmask; > + > + msi->info.flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_PCI_MSIX; > + msi->info.chip = &msi->top; > + > + msi->bottom.name = "Tegra MSI"; > + msi->bottom.irq_ack = tegra_msi_irq_ack; > + msi->bottom.irq_mask = tegra_msi_irq_mask; > + msi->bottom.irq_unmask = tegra_msi_irq_unmask; > + msi->bottom.irq_set_affinity = tegra_msi_set_affinity; > + msi->bottom.irq_compose_msi_msg = tegra_compose_msi_msg; It's not obvious to me why these irq_chip and msi_domain_info are dynamically allocated, given that they only contain static data, which should be common across PCIe controllers. Am I missing something here? The reason I'm being picky about it is that I'm trying to get to a point where we can make most struct irq_chip to be const and thus marked read-only (PM is getting in the way at the moment). The rest looks good to me, and only the above locking issue is a real showstopper. Thanks, M. -- Without deviation from the norm, progress is not possible.
