diff mbox series

[v2,11/23] PCI: aardvark: Fix setting MSI address

Message ID 20220110015018.26359-12-kabel@kernel.org (mailing list archive)
State New, archived
Headers show
Series PCI: aardvark controller fixes BATCH 4 | expand

Commit Message

Marek Behún Jan. 10, 2022, 1:50 a.m. UTC
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(-)

Comments

Bjorn Helgaas Feb. 17, 2022, 5:14 p.m. UTC | #1
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
Marek Behún Feb. 18, 2022, 2:43 p.m. UTC | #2
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
Bjorn Helgaas Feb. 23, 2022, 6:13 p.m. UTC | #3
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
Pali Rohár Feb. 24, 2022, 12:59 p.m. UTC | #4
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
Bjorn Helgaas Feb. 24, 2022, 7:43 p.m. UTC | #5
[+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 mbox series

Patch

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);