Message ID | 20200621053032.5262-1-sh@tkos.co.il |
---|---|
State | New |
Headers | show |
Series | PCI: mvebu: setup BAR0 to internal-regs | expand |
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, ®s); > + ret = of_address_to_resource(np, 0, &port->regs); > if (ret) > return (void __iomem *)ERR_PTR(ret); > > - return devm_ioremap_resource(&pdev->dev, ®s); > + return devm_ioremap_resource(&pdev->dev, &port->regs); > } > > #define DT_FLAGS_TO_TYPE(flags) (((flags) >> 24) & 0x03) > -- > 2.27.0 >
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
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
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.
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, ®s); + ret = of_address_to_resource(np, 0, &port->regs); if (ret) return (void __iomem *)ERR_PTR(ret); - return devm_ioremap_resource(&pdev->dev, ®s); + return devm_ioremap_resource(&pdev->dev, &port->regs); } #define DT_FLAGS_TO_TYPE(flags) (((flags) >> 24) & 0x03)
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(-)