[PATCH] PCI: mvebu: setup BAR0 to internal-regs

[PATCH] PCI: mvebu: setup BAR0 to internal-regs

[Shmuel Hazan]

According to the Armada XP datasheet, section 10.2.6: "in order for
the device to do a write to the MSI doorbell address, it needs to write
to a register in the internal registers space".

As a result of the requirement above, without this patch, MSI won't
function and therefore some devices won't operate properly without
pci=nomsi.

Tested on an Armada 385 board with the following PCIe devices:
	- Wilocity Wil6200 rev 2 (wil6210)
	- Qualcomm Atheros QCA6174 (ath10k_pci)

Both failed to get a response from the device after loading the
firmware and seem to operate properly with this patch.

Signed-off-by: Shmuel Hazan <sh@tkos.co.il>
---

Fixed a few commit message related issues mentioned by Bjorn Helgaas
<helgaas@kernel.org>. 
 
---
 drivers/pci/controller/pci-mvebu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 153a64676bc9..101c06602aa1 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -105,6 +105,7 @@ struct mvebu_pcie_port {
 	struct mvebu_pcie_window memwin;
 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
+	struct resource regs;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)
@@ -149,7 +150,9 @@ static void mvebu_pcie_set_local_dev_nr(struct mvebu_pcie_port *port, int nr)
 
 /*
  * Setup PCIE BARs and Address Decode Wins:
- * BAR[0,2] -> disabled, BAR[1] -> covers all DRAM banks
+ * BAR[0] -> internal registers (needed for MSI)
+ * BAR[1] -> covers all DRAM banks
+ * BAR[2] -> Disabled
  * WIN[0-3] -> DRAM bank[0-3]
  */
 static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
@@ -203,6 +206,12 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
 	mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
 		     PCIE_BAR_CTRL_OFF(1));
+
+	/*
+	 * Point BAR[0] to the device's internal registers.
+	 */
+	mvebu_writel(port, round_down(port->regs.start, SZ_1M), PCIE_BAR_LO_OFF(0));
+	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(0));
 }
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
@@ -708,14 +717,13 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev,
 					      struct device_node *np,
 					      struct mvebu_pcie_port *port)
 {
-	struct resource regs;
 	int ret = 0;
 
-	ret = of_address_to_resource(np, 0, &regs);
+	ret = of_address_to_resource(np, 0, &port->regs);
 	if (ret)
 		return (void __iomem *)ERR_PTR(ret);
 
-	return devm_ioremap_resource(&pdev->dev, &regs);
+	return devm_ioremap_resource(&pdev->dev, &port->regs);
 }
 
 #define DT_FLAGS_TO_TYPE(flags)       (((flags) >> 24) & 0x03)
-- 
2.27.0

Re: [PATCH] PCI: mvebu: setup BAR0 to internal-regs

[Bjorn Helgaas]

Please capitalize the first letter of your subject ("Setup" in this
case).

It would also be good if the subject included a clue about the reason
for the patch, i.e., apparently this fixes MSIs?

On Sun, Jun 21, 2020 at 08:30:33AM +0300, Shmuel Hazan wrote:
> According to the Armada XP datasheet, section 10.2.6: "in order for
> the device to do a write to the MSI doorbell address, it needs to write
> to a register in the internal registers space".
> 
> As a result of the requirement above, without this patch, MSI won't
> function and therefore some devices won't operate properly without
> pci=nomsi.

Does that mean MSIs never worked at all with mvebu?

Were they broken only for certain Armada controllers?  The patch
doesn't look specific to a particular controller, so, I assume MSIs
just never worked anywhere?  Or did they once work, but got broken and
this fixes the regression?

> Tested on an Armada 385 board with the following PCIe devices:
> 	- Wilocity Wil6200 rev 2 (wil6210)
> 	- Qualcomm Atheros QCA6174 (ath10k_pci)
> 
> Both failed to get a response from the device after loading the
> firmware and seem to operate properly with this patch.
> 
> Signed-off-by: Shmuel Hazan <sh@tkos.co.il>
> ---
> 
> Fixed a few commit message related issues mentioned by Bjorn Helgaas
> <helgaas@kernel.org>. 
>  
> ---
>  drivers/pci/controller/pci-mvebu.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 153a64676bc9..101c06602aa1 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -105,6 +105,7 @@ struct mvebu_pcie_port {
>  	struct mvebu_pcie_window memwin;
>  	struct mvebu_pcie_window iowin;
>  	u32 saved_pcie_stat;
> +	struct resource regs;
>  };
>  
>  static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)
> @@ -149,7 +150,9 @@ static void mvebu_pcie_set_local_dev_nr(struct mvebu_pcie_port *port, int nr)
>  
>  /*
>   * Setup PCIE BARs and Address Decode Wins:
> - * BAR[0,2] -> disabled, BAR[1] -> covers all DRAM banks
> + * BAR[0] -> internal registers (needed for MSI)
> + * BAR[1] -> covers all DRAM banks
> + * BAR[2] -> Disabled
>   * WIN[0-3] -> DRAM bank[0-3]
>   */
>  static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
> @@ -203,6 +206,12 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
>  	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
>  	mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
>  		     PCIE_BAR_CTRL_OFF(1));
> +
> +	/*
> +	 * Point BAR[0] to the device's internal registers.
> +	 */
> +	mvebu_writel(port, round_down(port->regs.start, SZ_1M), PCIE_BAR_LO_OFF(0));
> +	mvebu_writel(port, 0, PCIE_BAR_HI_OFF(0));
>  }
>  
>  static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
> @@ -708,14 +717,13 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev,
>  					      struct device_node *np,
>  					      struct mvebu_pcie_port *port)
>  {
> -	struct resource regs;
>  	int ret = 0;
>  
> -	ret = of_address_to_resource(np, 0, &regs);
> +	ret = of_address_to_resource(np, 0, &port->regs);
>  	if (ret)
>  		return (void __iomem *)ERR_PTR(ret);
>  
> -	return devm_ioremap_resource(&pdev->dev, &regs);
> +	return devm_ioremap_resource(&pdev->dev, &port->regs);
>  }
>  
>  #define DT_FLAGS_TO_TYPE(flags)       (((flags) >> 24) & 0x03)
> -- 
> 2.27.0
> 

Re: [PATCH] PCI: mvebu: setup BAR0 to internal-regs

[Thomas Petazzoni]

Hello,

On Mon, 22 Jun 2020 12:25:16 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> > As a result of the requirement above, without this patch, MSI won't
> > function and therefore some devices won't operate properly without
> > pci=nomsi.  
> 
> Does that mean MSIs never worked at all with mvebu?

They definitely worked. I think what happens is that this register was
normally setup by the vendor-specific bootloader, and thanks to
firmware initialization, Linux had MSIs working properly.

With other bootloaders that initialize the PCIe block differently, or
even not at all, it became clear this init was missing in Linux.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] PCI: mvebu: setup BAR0 to internal-regs

[Bjorn Helgaas]

On Mon, Jun 22, 2020 at 08:40:33PM +0200, Thomas Petazzoni wrote:
> On Mon, 22 Jun 2020 12:25:16 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > > As a result of the requirement above, without this patch, MSI won't
> > > function and therefore some devices won't operate properly without
> > > pci=nomsi.  
> > 
> > Does that mean MSIs never worked at all with mvebu?
> 
> They definitely worked. I think what happens is that this register was
> normally setup by the vendor-specific bootloader, and thanks to
> firmware initialization, Linux had MSIs working properly.
> 
> With other bootloaders that initialize the PCIe block differently, or
> even not at all, it became clear this init was missing in Linux.

That would be very useful information to include in the commit log.

Are there any other similar bugs lurking?  Other registers where we
implicitly rely on the bootloader to do something?

Bjorn

Re: [PATCH] PCI: mvebu: setup BAR0 to internal-regs

[Shmuel H.]

Hi Thomas, Bjorn,

On 6/23/20 1:49 AM, Bjorn Helgaas wrote:
> On Mon, Jun 22, 2020 at 08:40:33PM +0200, Thomas Petazzoni wrote:
>> On Mon, 22 Jun 2020 12:25:16 -0500
>> Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>>>> As a result of the requirement above, without this patch, MSI won't
>>>> function and therefore some devices won't operate properly without
>>>> pci=nomsi.  
>>> Does that mean MSIs never worked at all with mvebu?
>> They definitely worked. I think what happens is that this register was
>> normally setup by the vendor-specific bootloader, and thanks to
>> firmware initialization, Linux had MSIs working properly.
>>
>> With other bootloaders that initialize the PCIe block differently, or
>> even not at all, it became clear this init was missing in Linux.
> That would be very useful information to include in the commit log.

Thanks. I will add it to the commit message.

>
> Are there any other similar bugs lurking?  Other registers where we
> implicitly rely on the bootloader to do something?

I don't think that any PCI  driver writer can answer this question for
sure.

I can however confirm that this driver, on the a385, works with no
u-boot PCIe initialization. As for other subsystems PCI is dependent on
(e.g. SerDes, etc), I can't say for sure.

-- 
- Shmuel Hazan

mailto:sh@tkos.co.il | tel:+972-523-746-435 | http://tkos.co.il