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

Message ID 20170828142307.30061-2-l.stach@pengutronix.de
State Changes Requested
Headers show
Series
  • DWC host without MSI controller
Related show

Commit Message

Lucas Stach Aug. 28, 2017, 2:23 p.m.
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(-)

Comments

Bjorn Helgaas Aug. 31, 2017, 4:58 p.m. | #1
[+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
Fabio Estevam Aug. 31, 2017, 5:32 p.m. | #2
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.
Bjorn Helgaas Sept. 25, 2017, 11:54 p.m. | #3
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

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