| Message ID | 20220110015018.26359-12-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: aardvark controller fixes BATCH 4 | expand |
On Mon, Jan 10, 2022 at 02:50:06AM +0100, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > MSI address for receiving MSI interrupts needs to be correctly set before > enabling processing of MSI interrupts. > > Move code for setting PCIE_MSI_ADDR_LOW_REG and PCIE_MSI_ADDR_HIGH_REG > from advk_pcie_init_msi_irq_domain() to advk_pcie_setup_hw(), before > enabling PCIE_CORE_CTRL2_MSI_ENABLE. > > After this we can remove the now unused member msi_msg, which was used > only for MSI doorbell address. MSI address can be any address which cannot > be used to DMA to. So change it to the address of the main struct advk_pcie. > > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver") > Signed-off-by: Pali Rohár <pali@kernel.org> > Acked-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Marek Behún <kabel@kernel.org> > Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support") > --- > drivers/pci/controller/pci-aardvark.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > index 51fedbcb41fb..79102704d82f 100644 > --- a/drivers/pci/controller/pci-aardvark.c > +++ b/drivers/pci/controller/pci-aardvark.c > @@ -278,7 +278,6 @@ struct advk_pcie { > raw_spinlock_t msi_irq_lock; > DECLARE_BITMAP(msi_used, MSI_IRQ_NUM); > struct mutex msi_used_lock; > - u16 msi_msg; > int link_gen; > struct pci_bridge_emul bridge; > struct gpio_desc *reset_gpio; > @@ -473,6 +472,7 @@ static void advk_pcie_disable_ob_win(struct advk_pcie *pcie, u8 win_num) > > static void advk_pcie_setup_hw(struct advk_pcie *pcie) > { > + phys_addr_t msi_addr; > u32 reg; > int i; > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) > reg |= LANE_COUNT_1; > advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); > > + /* Set MSI address */ > + msi_addr = virt_to_phys(pcie); Strictly speaking, msi_addr should be a pci_bus_addr_t, not a phys_addr_t, and virt_to_phys() doesn't return a bus address. > + advk_writel(pcie, lower_32_bits(msi_addr), PCIE_MSI_ADDR_LOW_REG); > + advk_writel(pcie, upper_32_bits(msi_addr), PCIE_MSI_ADDR_HIGH_REG); > + > /* Enable MSI */ > reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG); > reg |= PCIE_CORE_CTRL2_MSI_ENABLE; > @@ -1184,10 +1189,10 @@ static void advk_msi_irq_compose_msi_msg(struct irq_data *data, > struct msi_msg *msg) > { > struct advk_pcie *pcie = irq_data_get_irq_chip_data(data); > - phys_addr_t msi_msg = virt_to_phys(&pcie->msi_msg); > + phys_addr_t msi_addr = virt_to_phys(pcie); > > - msg->address_lo = lower_32_bits(msi_msg); > - msg->address_hi = upper_32_bits(msi_msg); > + msg->address_lo = lower_32_bits(msi_addr); > + msg->address_hi = upper_32_bits(msi_addr); > msg->data = data->hwirq; > } > > @@ -1346,18 +1351,10 @@ static struct msi_domain_info advk_msi_domain_info = { > static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie) > { > struct device *dev = &pcie->pdev->dev; > - phys_addr_t msi_msg_phys; > > raw_spin_lock_init(&pcie->msi_irq_lock); > mutex_init(&pcie->msi_used_lock); > > - msi_msg_phys = virt_to_phys(&pcie->msi_msg); > - > - advk_writel(pcie, lower_32_bits(msi_msg_phys), > - PCIE_MSI_ADDR_LOW_REG); > - advk_writel(pcie, upper_32_bits(msi_msg_phys), > - PCIE_MSI_ADDR_HIGH_REG); > - > pcie->msi_inner_domain = > irq_domain_add_linear(NULL, MSI_IRQ_NUM, > &advk_msi_domain_ops, pcie); > -- > 2.34.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, 17 Feb 2022 11:14:52 -0600 Bjorn Helgaas <helgaas@kernel.org> wrote: > > + phys_addr_t msi_addr; > > u32 reg; > > int i; > > > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) > > reg |= LANE_COUNT_1; > > advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); > > > > + /* Set MSI address */ > > + msi_addr = virt_to_phys(pcie); > > Strictly speaking, msi_addr should be a pci_bus_addr_t, not a > phys_addr_t, and virt_to_phys() doesn't return a bus address. Dear Bjorn, the problem here is that as far as we know currently there is no function that converts a virtual address to pci_bus_addr_t like virt_to_phys() does to convert to phys_addr_t. On Armada 3720 there are PCIe Controller Address Decoder Registers, which such a translating function would need to consult to do the translation. But the default settings of these registers is to map PCIe addresses 1 to 1 to physical addresses, and no driver changes these registers. Pali says that other drivers also use phys_addr_t, and most hardware maps 1 to 1 by default. So we think that until at least an API for such a function exists, we shuld leave it as it is. I am not against converting the phys_addr_to to a pci_bus_addr_t, but Pali thinks that for now we should leave even that as it is, because the virt_to_phys() function returns phys_addr_t. We can add a comment there explaining this, if you want. What do you think? Marek
On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote: > On Thu, 17 Feb 2022 11:14:52 -0600 > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > + phys_addr_t msi_addr; > > > u32 reg; > > > int i; > > > > > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) > > > reg |= LANE_COUNT_1; > > > advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); > > > > > > + /* Set MSI address */ > > > + msi_addr = virt_to_phys(pcie); > > > > Strictly speaking, msi_addr should be a pci_bus_addr_t, not a > > phys_addr_t, and virt_to_phys() doesn't return a bus address. > > the problem here is that as far as we know currently there is no > function that converts a virtual address to pci_bus_addr_t like > virt_to_phys() does to convert to phys_addr_t. > > On Armada 3720 there are PCIe Controller Address Decoder Registers, > which such a translating function would need to consult to do the > translation. But the default settings of these registers is to map PCIe > addresses 1 to 1 to physical addresses, and no driver changes these > registers. The poorly-named pcibios_resource_to_bus() (I think the name is my fault) is the way to convert a CPU physical address to a PCI bus address. This is implemented in terms of the host bridge windows and the translation offset in struct resource_entry, which should be set up via the pci_add_resource_offset() called from devm_of_pci_get_host_bridge_resources(). > Pali says that other drivers also use phys_addr_t, and most hardware > maps 1 to 1 by default. Yes. I think they're all technically incorrect. Most systems do map CPU phys == PCI bus, but I point it out because it's a case where copying that pattern to new drivers will eventually bite us. > So we think that until at least an API for such a function exists, we > shuld leave it as it is. I am not against converting the phys_addr_to > to a pci_bus_addr_t, but Pali thinks that for now we should leave even > that as it is, because the virt_to_phys() function returns phys_addr_t. > > We can add a comment there explaining this, if you want. > > What do you think? > > Marek
On Wednesday 23 February 2022 12:13:12 Bjorn Helgaas wrote: > On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote: > > On Thu, 17 Feb 2022 11:14:52 -0600 > > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > + phys_addr_t msi_addr; > > > > u32 reg; > > > > int i; > > > > > > > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) > > > > reg |= LANE_COUNT_1; > > > > advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); > > > > > > > > + /* Set MSI address */ > > > > + msi_addr = virt_to_phys(pcie); > > > > > > Strictly speaking, msi_addr should be a pci_bus_addr_t, not a > > > phys_addr_t, and virt_to_phys() doesn't return a bus address. > > > > the problem here is that as far as we know currently there is no > > function that converts a virtual address to pci_bus_addr_t like > > virt_to_phys() does to convert to phys_addr_t. > > > > On Armada 3720 there are PCIe Controller Address Decoder Registers, > > which such a translating function would need to consult to do the > > translation. But the default settings of these registers is to map PCIe > > addresses 1 to 1 to physical addresses, and no driver changes these > > registers. > > The poorly-named pcibios_resource_to_bus() (I think the name is my > fault) is the way to convert a CPU physical address to a PCI bus > address. But here it is needed to do something different. It is needed to do inverse mapping of function which converts PCI bus address to CPU physical address of CPU memory. So to converting CPU physical address of CPU memory to PCI bus address from PCI bus point of view. I think that this information is stored in dma_ranges member of struct pci_host_bridge. But function pcibios_resource_to_bus() looks at the ->windows member which converts CPU physical address of PCI memory (not CPU memory) to PCI bus address, which is something different. So pcibios_resource_to_bus() would not work here and it may return incorrect values (as PCI memory may be different from CPU point of view and PCI bus point of view). > This is implemented in terms of the host bridge windows and the > translation offset in struct resource_entry, which should be set up > via the pci_add_resource_offset() called from > devm_of_pci_get_host_bridge_resources(). > > > Pali says that other drivers also use phys_addr_t, and most hardware > > maps 1 to 1 by default. > > Yes. I think they're all technically incorrect. Most systems do map > CPU phys == PCI bus, but I point it out because it's a case where > copying that pattern to new drivers will eventually bite us. I agree, it is incorrect but I do not see a way how to do it correctly because of missing function (which for pci-aardvark should return identity). > > So we think that until at least an API for such a function exists, we > > shuld leave it as it is. I am not against converting the phys_addr_to > > to a pci_bus_addr_t, but Pali thinks that for now we should leave even > > that as it is, because the virt_to_phys() function returns phys_addr_t. > > > > We can add a comment there explaining this, if you want. > > > > What do you think? > > > > Marek
[+cc Rob] On Thu, Feb 24, 2022 at 01:59:01PM +0100, Pali Rohár wrote: > On Wednesday 23 February 2022 12:13:12 Bjorn Helgaas wrote: > > On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote: > > > On Thu, 17 Feb 2022 11:14:52 -0600 > > > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > > > + phys_addr_t msi_addr; > > > > > u32 reg; > > > > > int i; > > > > > > > > > > @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) > > > > > reg |= LANE_COUNT_1; > > > > > advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); > > > > > > > > > > + /* Set MSI address */ > > > > > + msi_addr = virt_to_phys(pcie); > > > > > > > > Strictly speaking, msi_addr should be a pci_bus_addr_t, not a > > > > phys_addr_t, and virt_to_phys() doesn't return a bus address. On second thought, probably a dma_addr_t, not a pci_bus_addr_t. > > > the problem here is that as far as we know currently there is no > > > function that converts a virtual address to pci_bus_addr_t like > > > virt_to_phys() does to convert to phys_addr_t. > > > > > > On Armada 3720 there are PCIe Controller Address Decoder Registers, > > > which such a translating function would need to consult to do the > > > translation. But the default settings of these registers is to map PCIe > > > addresses 1 to 1 to physical addresses, and no driver changes these > > > registers. > > > > The poorly-named pcibios_resource_to_bus() (I think the name is my > > fault) is the way to convert a CPU physical address to a PCI bus > > address. > > But here it is needed to do something different. It is needed to do > inverse mapping of function which converts PCI bus address to CPU > physical address of CPU memory. So to converting CPU physical address of > CPU memory to PCI bus address from PCI bus point of view. > > I think that this information is stored in dma_ranges member of struct > pci_host_bridge. But function pcibios_resource_to_bus() looks at the > ->windows member which converts CPU physical address of PCI memory (not > CPU memory) to PCI bus address, which is something different. So > pcibios_resource_to_bus() would not work here and it may return > incorrect values (as PCI memory may be different from CPU point of view > and PCI bus point of view). Oh, sorry, indeed you are correct and I was completely on the wrong track. pcibios_resource_to_bus() is what you need for doing MMIO -- CPU accesses to things on PCI. MSI is the reverse. From the device's point of view, an MSI is basically a DMA as you allude to above, so I would expect the DMA API to be involved somehow. I do see a couple drivers using the DMA API for this: struct pcie_port { dma_addr_t msi_data; }; dw_pcie_host_init pp->msi_data = dma_map_single_attrs(..., &pp->msi_msg, ...) dw_pcie_setup_rc dw_pcie_msi_init u64 msi_target = (u64)pp->msi_data; dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target)); dw_pci_setup_msi_msg u64 msi_target = (u64)pp->msi_data; msg->address_lo = lower_32_bits(msi_target); ----------------------------------------------------------- struct tegra_msi { dma_addr_t phys; }; tegra_pcie_probe tegra_pcie_msi_setup msi->virt = dma_alloc_attrs(..., &msi->phys, ...); tegra_pcie_enable_msi afi_writel(pcie, msi->phys, ...); tegra_compose_msi_msg msg->address_low = lower_32_bits(msi->phys); Bjorn
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 51fedbcb41fb..79102704d82f 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -278,7 +278,6 @@ struct advk_pcie { raw_spinlock_t msi_irq_lock; DECLARE_BITMAP(msi_used, MSI_IRQ_NUM); struct mutex msi_used_lock; - u16 msi_msg; int link_gen; struct pci_bridge_emul bridge; struct gpio_desc *reset_gpio; @@ -473,6 +472,7 @@ static void advk_pcie_disable_ob_win(struct advk_pcie *pcie, u8 win_num) static void advk_pcie_setup_hw(struct advk_pcie *pcie) { + phys_addr_t msi_addr; u32 reg; int i; @@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) reg |= LANE_COUNT_1; advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); + /* Set MSI address */ + msi_addr = virt_to_phys(pcie); + advk_writel(pcie, lower_32_bits(msi_addr), PCIE_MSI_ADDR_LOW_REG); + advk_writel(pcie, upper_32_bits(msi_addr), PCIE_MSI_ADDR_HIGH_REG); + /* Enable MSI */ reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG); reg |= PCIE_CORE_CTRL2_MSI_ENABLE; @@ -1184,10 +1189,10 @@ static void advk_msi_irq_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) { struct advk_pcie *pcie = irq_data_get_irq_chip_data(data); - phys_addr_t msi_msg = virt_to_phys(&pcie->msi_msg); + phys_addr_t msi_addr = virt_to_phys(pcie); - msg->address_lo = lower_32_bits(msi_msg); - msg->address_hi = upper_32_bits(msi_msg); + msg->address_lo = lower_32_bits(msi_addr); + msg->address_hi = upper_32_bits(msi_addr); msg->data = data->hwirq; } @@ -1346,18 +1351,10 @@ static struct msi_domain_info advk_msi_domain_info = { static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie) { struct device *dev = &pcie->pdev->dev; - phys_addr_t msi_msg_phys; raw_spin_lock_init(&pcie->msi_irq_lock); mutex_init(&pcie->msi_used_lock); - msi_msg_phys = virt_to_phys(&pcie->msi_msg); - - advk_writel(pcie, lower_32_bits(msi_msg_phys), - PCIE_MSI_ADDR_LOW_REG); - advk_writel(pcie, upper_32_bits(msi_msg_phys), - PCIE_MSI_ADDR_HIGH_REG); - pcie->msi_inner_domain = irq_domain_add_linear(NULL, MSI_IRQ_NUM, &advk_msi_domain_ops, pcie);