[PATCH 0/3] DWC host without MSI controller

[PATCH 0/3] DWC host without MSI controller

[Lucas Stach]

Hi all,

this small series tries to fix/workaround a serious design flaw of the DWC PCIe
host controller: it is unable to work with both legacy and MSI IRQs enabled at
the same time. As soon as the first MSI is enabled in the DWC MSI controller,
the host stops forwarding legacy IRQs.

If the MSI controller is present, MSIs will be used for the PCIe port services
IRQs, leaving endpoint devices which don't support MSIs unable to raise IRQs.
It is only safe to enable the MSI controller if it is validated that all PCIe
devices and drivers in the system support working MSIs. As most devices
support falling back to using legacy PCIe IRQs if MSI support is missing it is
much safer to disable the MSI by default and only enable it on validated
systems.

Feedback welcome.

Regards,
Lucas

Lucas Stach (3):
  PCI: designware: only register MSI controller when MSI irq line is
    valid
  PCI: imx6: allow MSI irq to be absent
  ARM: dts: imx6qdl: remove MSI irq line

 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  8 ++++----
 arch/arm/boot/dts/imx6qdl.dtsi                     |  2 --
 drivers/pci/dwc/pci-imx6.c                         | 23 +++++++++++-----------
 drivers/pci/dwc/pcie-designware-host.c             |  4 ++--
 4 files changed, 17 insertions(+), 20 deletions(-)

-- 
2.11.0

[PATCH 1/3] PCI: designware: only register MSI controller when MSI irq line is valid

[Lucas Stach]

The MSI part of the controller isn't essential, so the host controller can
be registered without the MSI controller being present. This allows the
host to work in PCIe legancy interrupt only mode, if the IRQ line for the
MSI controller is missing.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/dwc/pcie-designware-host.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index d29c020da082..8494089f088d 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -381,7 +381,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (ret)
 		pci->num_viewport = 2;
 
-	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+	if (IS_ENABLED(CONFIG_PCI_MSI) && pp->msi_irq > 0) {
 		if (!pp->ops->msi_host_init) {
 			pp->irq_domain = irq_domain_add_linear(dev->of_node,
 						MAX_MSI_IRQS, &msi_domain_ops,
@@ -412,7 +412,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	bridge->ops = &dw_pcie_ops;
 	bridge->map_irq = of_irq_parse_and_map_pci;
 	bridge->swizzle_irq = pci_common_swizzle;
-	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+	if (IS_ENABLED(CONFIG_PCI_MSI) && pp->msi_irq > 0) {
 		bridge->msi = &dw_pcie_msi_chip;
 		dw_pcie_msi_chip.dev = dev;
 	}
-- 
2.11.0

[PATCH 1/3] PCI: designware: only register MSI controller when MSI irq line is valid

[Bjorn Helgaas]

[+cc Kishon, Thomas, Niklas, Jesper, Zhou, Gabriele, Xiaowei, Binghui,
Stanimir, Pratyush, Kukjin, Krzysztof, Richard, Murali, Minghuan, Mingkai,
Roy]

Sorry about the unwieldy cc list.  It seems like this affects every
DesignWare-based driver.

On Mon, Aug 28, 2017 at 04:23:05PM +0200, Lucas Stach wrote:
> The MSI part of the controller isn't essential, so the host controller can
> be registered without the MSI controller being present. This allows the
> host to work in PCIe legancy interrupt only mode, if the IRQ line for the

s/legancy/legacy/

> MSI controller is missing.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/pci/dwc/pcie-designware-host.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
> index d29c020da082..8494089f088d 100644
> --- a/drivers/pci/dwc/pcie-designware-host.c
> +++ b/drivers/pci/dwc/pcie-designware-host.c
> @@ -381,7 +381,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	if (ret)
>  		pci->num_viewport = 2;
>  
> -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +	if (IS_ENABLED(CONFIG_PCI_MSI) && pp->msi_irq > 0) {
>  		if (!pp->ops->msi_host_init) {
>  			pp->irq_domain = irq_domain_add_linear(dev->of_node,
>  						MAX_MSI_IRQS, &msi_domain_ops,
> @@ -412,7 +412,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	bridge->ops = &dw_pcie_ops;
>  	bridge->map_irq = of_irq_parse_and_map_pci;
>  	bridge->swizzle_irq = pci_common_swizzle;
> -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +	if (IS_ENABLED(CONFIG_PCI_MSI) && pp->msi_irq > 0) {
>  		bridge->msi = &dw_pcie_msi_chip;
>  		dw_pcie_msi_chip.dev = dev;
>  	}

Here are the callers of dw_pcie_host_init():

  armada8k_add_pcie_port
  artpec6_add_pcie_port       # sets pp->msi_irq
  dra7xx_add_pcie_port
  dw_plat_add_pcie_port       # sets pp->msi_irq
  exynos_add_pcie_port        # sets pp->msi_irq
  hisi_add_pcie_port
  imx6_add_pcie_port          # sets pp->msi_irq
  kirin_add_pcie_port
  ks_dw_pcie_host_init        # sets pp->ops->msi_host_init
  ls_add_pcie_port            # sets pp->ops->msi_host_init
  qcom_pcie_probe             # sets pp->msi_irq
  spear13xx_add_pcie_port

For the drivers that set pp->msi_irq (artpec6, dw_plat, exynos, imx6,
qcom), it seems like you'd want a similar follow-up change for all of
them to make the MSI IRQ optional, but you only changed imx6.  What
about the others?

For the drivers that don't set pp->msi_irq and don't set
pp->ops->msi_host_init (armada8k, dra7xx, hisi, kirin, spear13xx),
this patch means they no longer set up pp->irq_domain.  Do you intend
that?

Incidental observations not strictly related to this series:

  - It looks like exynos_add_pcie_port() incorrectly assumes
    platform_get_irq() returns zero on failure (twice).

  - It'd be nice if qcom_pcie_probe() followed the same
    add_pcie_port() structure as the other drivers.

Bjorn

[PATCH 1/3] PCI: designware: only register MSI controller when MSI irq line is valid

[Fabio Estevam]

Hi Bjorn,

On Thu, Aug 31, 2017 at 1:58 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> Incidental observations not strictly related to this series:
>
>   - It looks like exynos_add_pcie_port() incorrectly assumes
>     platform_get_irq() returns zero on failure (twice).

I will send a series cleaning up the error handling in
platform_get_irq() for the various PCI drivers.

[PATCH 1/3] PCI: designware: only register MSI controller when MSI irq line is valid

[Bjorn Helgaas]

On Thu, Aug 31, 2017 at 11:58:17AM -0500, Bjorn Helgaas wrote:
> [+cc Kishon, Thomas, Niklas, Jesper, Zhou, Gabriele, Xiaowei, Binghui,
> Stanimir, Pratyush, Kukjin, Krzysztof, Richard, Murali, Minghuan, Mingkai,
> Roy]
> 
> Sorry about the unwieldy cc list.  It seems like this affects every
> DesignWare-based driver.
> 
> On Mon, Aug 28, 2017 at 04:23:05PM +0200, Lucas Stach wrote:
> > The MSI part of the controller isn't essential, so the host controller can
> > be registered without the MSI controller being present. This allows the
> > host to work in PCIe legancy interrupt only mode, if the IRQ line for the
> 
> s/legancy/legacy/
> 
> > MSI controller is missing.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/pci/dwc/pcie-designware-host.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
> > index d29c020da082..8494089f088d 100644
> > --- a/drivers/pci/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/dwc/pcie-designware-host.c
> > @@ -381,7 +381,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  	if (ret)
> >  		pci->num_viewport = 2;
> >  
> > -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +	if (IS_ENABLED(CONFIG_PCI_MSI) && pp->msi_irq > 0) {
> >  		if (!pp->ops->msi_host_init) {
> >  			pp->irq_domain = irq_domain_add_linear(dev->of_node,
> >  						MAX_MSI_IRQS, &msi_domain_ops,
> > @@ -412,7 +412,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  	bridge->ops = &dw_pcie_ops;
> >  	bridge->map_irq = of_irq_parse_and_map_pci;
> >  	bridge->swizzle_irq = pci_common_swizzle;
> > -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +	if (IS_ENABLED(CONFIG_PCI_MSI) && pp->msi_irq > 0) {
> >  		bridge->msi = &dw_pcie_msi_chip;
> >  		dw_pcie_msi_chip.dev = dev;
> >  	}
> 
> Here are the callers of dw_pcie_host_init():
> 
>   armada8k_add_pcie_port
>   artpec6_add_pcie_port       # sets pp->msi_irq
>   dra7xx_add_pcie_port
>   dw_plat_add_pcie_port       # sets pp->msi_irq
>   exynos_add_pcie_port        # sets pp->msi_irq
>   hisi_add_pcie_port
>   imx6_add_pcie_port          # sets pp->msi_irq
>   kirin_add_pcie_port
>   ks_dw_pcie_host_init        # sets pp->ops->msi_host_init
>   ls_add_pcie_port            # sets pp->ops->msi_host_init
>   qcom_pcie_probe             # sets pp->msi_irq
>   spear13xx_add_pcie_port
> 
> For the drivers that set pp->msi_irq (artpec6, dw_plat, exynos, imx6,
> qcom), it seems like you'd want a similar follow-up change for all of
> them to make the MSI IRQ optional, but you only changed imx6.  What
> about the others?
> 
> For the drivers that don't set pp->msi_irq and don't set
> pp->ops->msi_host_init (armada8k, dra7xx, hisi, kirin, spear13xx),
> this patch means they no longer set up pp->irq_domain.  Do you intend
> that?

Ping, I'm looking for a v2 that addresses this.  No hurry, just
making sure you're not waiting on me.

Bjorn

[PATCH 2/3] PCI: imx6: allow MSI irq to be absent

[Lucas Stach]

The host can fall back to PCIe legacy IRQ only operation if the MSI irq is
missing, thus allowing systems to work with peripherals that don't support
MSI irqs.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  8 ++++----
 drivers/pci/dwc/pci-imx6.c                         | 23 +++++++++++-----------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index cf92d3ba5a26..d85db21503f4 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -10,14 +10,14 @@ Required properties:
 	- "fsl,imx6qp-pcie"
 	- "fsl,imx7d-pcie"
 - reg: base address and length of the PCIe controller
-- interrupts: A list of interrupt outputs of the controller. Must contain an
-  entry for each entry in the interrupt-names property.
-- interrupt-names: Must include the following entries:
-	- "msi": The interrupt that is asserted when an MSI is received
 - clock-names: Must include the following additional entries:
 	- "pcie_phy"
 
 Optional properties:
+- interrupts: A list of interrupt outputs of the controller. Must contain an
+  entry for each entry in the interrupt-names property.
+- interrupt-names: Must include the following entries:
+        - "msi": The interrupt that is asserted when an MSI is received
 - fsl,tx-deemph-gen1: Gen1 De-emphasis value. Default: 0
 - fsl,tx-deemph-gen2-3p5db: Gen2 (3.5db) De-emphasis value. Default: 0
 - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20
diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
index bf5c3616e344..d2507272b3ab 100644
--- a/drivers/pci/dwc/pci-imx6.c
+++ b/drivers/pci/dwc/pci-imx6.c
@@ -671,18 +671,17 @@ static int imx6_add_pcie_port(struct imx6_pcie *imx6_pcie,
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		pp->msi_irq = platform_get_irq_byname(pdev, "msi");
-		if (pp->msi_irq <= 0) {
-			dev_err(dev, "failed to get MSI irq\n");
-			return -ENODEV;
-		}
-
-		ret = devm_request_irq(dev, pp->msi_irq,
-				       imx6_pcie_msi_handler,
-				       IRQF_SHARED | IRQF_NO_THREAD,
-				       "mx6-pcie-msi", imx6_pcie);
-		if (ret) {
-			dev_err(dev, "failed to request MSI irq\n");
-			return ret;
+		if (pp->msi_irq > 0) {
+			ret = devm_request_irq(dev, pp->msi_irq,
+					       imx6_pcie_msi_handler,
+					       IRQF_SHARED | IRQF_NO_THREAD,
+					       "mx6-pcie-msi", imx6_pcie);
+			if (ret) {
+				dev_err(dev, "failed to request MSI irq\n");
+				return ret;
+			}
+		} else {
+			dev_info(dev, "missing MSI irq, MSI support unavailable\n");
 		}
 	}
 
-- 
2.11.0

[PATCH 3/3] ARM: dts: imx6qdl: remove MSI irq line

[Lucas Stach]

The DWC PCIe host controller doesn't support MSI and legacy IRQs at
the same time. If the MSI controller is in use (which is always the case,
as PCIe port serives are using MSI IRQs when available), legacy endpoint
devices are unable to raise an IRQ.

Remove the MSI irq line to inhibit the MSI controller from being used,
which is a much better default, as most enpoint devices are able to fall
back to legacy PCIe IRQs, if MSI are not available.

Systems which are validated to work in MSI only mode can opt-in to use
the MSI controller by adding back the MSI irq line in the board DT.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/boot/dts/imx6qdl.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index a9723b94bafa..0f47a9d4024e 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -209,8 +209,6 @@
 			ranges = <0x81000000 0 0          0x01f80000 0 0x00010000 /* downstream I/O */
 				  0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; /* non-prefetchable memory */
 			num-lanes = <1>;
-			interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-names = "msi";
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0x7>;
 			interrupt-map = <0 0 0 1 &gpc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
-- 
2.11.0

[PATCH 0/3] DWC host without MSI controller

[Tim Harvey]

On Mon, Aug 28, 2017 at 7:23 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Hi all,
>
> this small series tries to fix/workaround a serious design flaw of the DWC PCIe
> host controller: it is unable to work with both legacy and MSI IRQs enabled at
> the same time. As soon as the first MSI is enabled in the DWC MSI controller,
> the host stops forwarding legacy IRQs.
>
> If the MSI controller is present, MSIs will be used for the PCIe port services
> IRQs, leaving endpoint devices which don't support MSIs unable to raise IRQs.
> It is only safe to enable the MSI controller if it is validated that all PCIe
> devices and drivers in the system support working MSIs. As most devices
> support falling back to using legacy PCIe IRQs if MSI support is missing it is
> much safer to disable the MSI by default and only enable it on validated
> systems.
>
> Feedback welcome.
>
> Regards,
> Lucas
>
> Lucas Stach (3):
>   PCI: designware: only register MSI controller when MSI irq line is
>     valid
>   PCI: imx6: allow MSI irq to be absent
>   ARM: dts: imx6qdl: remove MSI irq line
>
>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  8 ++++----
>  arch/arm/boot/dts/imx6qdl.dtsi                     |  2 --
>  drivers/pci/dwc/pci-imx6.c                         | 23 +++++++++++-----------
>  drivers/pci/dwc/pcie-designware-host.c             |  4 ++--
>  4 files changed, 17 insertions(+), 20 deletions(-)
>

Lucas,

Thank you for following up on this!

I tested it with and without the third patch that removes msi from the
dt thus with both an ath9k 802.11 device which only supports legacy
interrupts and a Marvell sky2 device which supports msi as well as
legacy interrupts and it works as expected:
 - without msi enabled in dts (default): both sky2 and ath9k work
 - with msi enabled in dt: sky2 works ath9k does not

Tested-by: Tim Harvey <tharvey@gateworks.com>

Regards,

Tim

[PATCH 0/3] DWC host without MSI controller

[Fabio Estevam]

Hi Bjorn,

On Mon, Aug 28, 2017 at 1:59 PM, Tim Harvey <tharvey@gateworks.com> wrote:
> On Mon, Aug 28, 2017 at 7:23 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
>> Hi all,
>>
>> this small series tries to fix/workaround a serious design flaw of the DWC PCIe
>> host controller: it is unable to work with both legacy and MSI IRQs enabled at
>> the same time. As soon as the first MSI is enabled in the DWC MSI controller,
>> the host stops forwarding legacy IRQs.
>>
>> If the MSI controller is present, MSIs will be used for the PCIe port services
>> IRQs, leaving endpoint devices which don't support MSIs unable to raise IRQs.
>> It is only safe to enable the MSI controller if it is validated that all PCIe
>> devices and drivers in the system support working MSIs. As most devices
>> support falling back to using legacy PCIe IRQs if MSI support is missing it is
>> much safer to disable the MSI by default and only enable it on validated
>> systems.
>>
>> Feedback welcome.
>>
>> Regards,
>> Lucas
>>
>> Lucas Stach (3):
>>   PCI: designware: only register MSI controller when MSI irq line is
>>     valid
>>   PCI: imx6: allow MSI irq to be absent
>>   ARM: dts: imx6qdl: remove MSI irq line
>>
>>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  8 ++++----
>>  arch/arm/boot/dts/imx6qdl.dtsi                     |  2 --
>>  drivers/pci/dwc/pci-imx6.c                         | 23 +++++++++++-----------
>>  drivers/pci/dwc/pcie-designware-host.c             |  4 ++--
>>  4 files changed, 17 insertions(+), 20 deletions(-)
>>
>
> Lucas,
>
> Thank you for following up on this!
>
> I tested it with and without the third patch that removes msi from the
> dt thus with both an ath9k 802.11 device which only supports legacy
> interrupts and a Marvell sky2 device which supports msi as well as
> legacy interrupts and it works as expected:
>  - without msi enabled in dts (default): both sky2 and ath9k work
>  - with msi enabled in dt: sky2 works ath9k does not
>
> Tested-by: Tim Harvey <tharvey@gateworks.com>

Any comments about this series?

[PATCH 0/3] DWC host without MSI controller

[Lucas Stach]

Hi Fabio,

Am Montag, den 09.10.2017, 09:14 -0300 schrieb Fabio Estevam:
> Hi Bjorn,
> 
> On Mon, Aug 28, 2017 at 1:59 PM, Tim Harvey <tharvey@gateworks.com>
> wrote:
> > On Mon, Aug 28, 2017 at 7:23 AM, Lucas Stach <l.stach@pengutronix.d
> > e> wrote:
> > > Hi all,
> > > 
> > > this small series tries to fix/workaround a serious design flaw
> > > of the DWC PCIe
> > > host controller: it is unable to work with both legacy and MSI
> > > IRQs enabled at
> > > the same time. As soon as the first MSI is enabled in the DWC MSI
> > > controller,
> > > the host stops forwarding legacy IRQs.
> > > 
> > > If the MSI controller is present, MSIs will be used for the PCIe
> > > port services
> > > IRQs, leaving endpoint devices which don't support MSIs unable to
> > > raise IRQs.
> > > It is only safe to enable the MSI controller if it is validated
> > > that all PCIe
> > > devices and drivers in the system support working MSIs. As most
> > > devices
> > > support falling back to using legacy PCIe IRQs if MSI support is
> > > missing it is
> > > much safer to disable the MSI by default and only enable it on
> > > validated
> > > systems.
> > > 
> > > Feedback welcome.
> > > 
> > > Regards,
> > > Lucas
> > > 
> > > Lucas Stach (3):
> > > ? PCI: designware: only register MSI controller when MSI irq line
> > > is
> > > ????valid
> > > ? PCI: imx6: allow MSI irq to be absent
> > > ? ARM: dts: imx6qdl: remove MSI irq line
> > > 
> > > ?.../devicetree/bindings/pci/fsl,imx6q-pcie.txt?????|??8 ++++----
> > > ?arch/arm/boot/dts/imx6qdl.dtsi?????????????????????|??2 --
> > > ?drivers/pci/dwc/pci-imx6.c?????????????????????????| 23
> > > +++++++++++-----------
> > > ?drivers/pci/dwc/pcie-designware-host.c?????????????|??4 ++--
> > > ?4 files changed, 17 insertions(+), 20 deletions(-)
> > > 
> > 
> > Lucas,
> > 
> > Thank you for following up on this!
> > 
> > I tested it with and without the third patch that removes msi from
> > the
> > dt thus with both an ath9k 802.11 device which only supports legacy
> > interrupts and a Marvell sky2 device which supports msi as well as
> > legacy interrupts and it works as expected:
> > ?- without msi enabled in dts (default): both sky2 and ath9k work
> > ?- with msi enabled in dt: sky2 works ath9k does not
> > 
> > Tested-by: Tim Harvey <tharvey@gateworks.com>
> 
> Any comments about this series?

Bjorn already commented on the series and I'm trying free up a slot to
address the feedback.

Regards,
Lucas