[PATCH 0/4] proper multi MSI handling for designware host

[PATCH 0/4] proper multi MSI handling for designware host

[Lucas Stach]

This series implements multiple MSI setup and teardown in terms
of the msichip infrastructure for the designware host controller.
It removes quite a bit of homegrown multi MSI handling from the
driver, which I suspect wasn't really tested before.

The series is currently based on pci/next.

Functionality was tested with an FPGA connected to an i.MX6 SoC.
I have verified that the FPGA is able to trigger the second MSI
and the irq is properly routed into the driver.

Lucas Stach (4):
  PCI: allow MSI chip providers to implement their own multiple MSI
    setup
  PCI: designware: remove bogus multiple MSI setup
  PCI: designware: remove open-coded bitmap operations
  PCI: designware: implement multiple MSI irq setup

 drivers/pci/host/pcie-designware.c | 141 +++++++++++++++----------------------
 drivers/pci/msi.c                  |   3 +
 include/linux/msi.h                |   2 +
 3 files changed, 63 insertions(+), 83 deletions(-)

-- 
2.0.0.rc2

[PATCH 1/4] PCI: allow MSI chip providers to implement their own multiple MSI setup

[Lucas Stach]

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/msi.c   | 3 +++
 include/linux/msi.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 27a7e67ddfe4..c45399d3061a 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -68,9 +68,12 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
 
 int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
+	struct msi_chip *chip = dev->bus->msi;
 	struct msi_desc *entry;
 	int ret;
 
+	if (chip && chip->setup_irqs)
+		return chip->setup_irqs(chip, dev, nvec, type);
 	/*
 	 * If an architecture wants to support multiple MSI, it needs to
 	 * override arch_setup_msi_irqs()
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 92a2f991262a..d813f898194c 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -75,6 +75,8 @@ struct msi_chip {
 
 	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
 			 struct msi_desc *desc);
+	int (*setup_irqs)(struct msi_chip *chip, struct pci_dev *dev,
+			  int nvec, int type);
 	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
 	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
 			    int nvec, int type);
-- 
2.0.0.rc2

[PATCH 2/4] PCI: designware: remove bogus multiple MSI setup

[Lucas Stach]

The setup_irq function is supposed to set up exactly one MSI
irq. Multiple IRQ setup is handled differently, to respect
the choices made by the upper layers.

Also only clear one MSI irq at a time, the PCI core will
call into this function multiple times if it has to tear
down more than one MSI irq.

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

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 1eaf4df3618a..1f04e978f877 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -296,37 +296,10 @@ no_valid_irq:
 	return -ENOSPC;
 }
 
-static void clear_irq(unsigned int irq)
-{
-	unsigned int pos, nvec;
-	struct msi_desc *msi;
-	struct pcie_port *pp;
-	struct irq_data *data = irq_get_irq_data(irq);
-
-	/* get the port structure */
-	msi = irq_data_get_msi(data);
-	pp = sys_to_pcie(msi->dev->bus->sysdata);
-	if (!pp) {
-		BUG();
-		return;
-	}
-
-	/* undo what was done in assign_irq */
-	pos = data->hwirq;
-	nvec = 1 << msi->msi_attrib.multiple;
-
-	clear_irq_range(pp, irq, nvec, pos);
-
-	/* all irqs cleared; reset attributes */
-	msi->irq = 0;
-	msi->msi_attrib.multiple = 0;
-}
-
 static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
 			struct msi_desc *desc)
 {
-	int irq, pos, msgvec;
-	u16 msg_ctr;
+	int irq, pos;
 	struct msi_msg msg;
 	struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
 
@@ -335,24 +308,10 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
 		return -EINVAL;
 	}
 
-	pci_read_config_word(pdev, desc->msi_attrib.pos+PCI_MSI_FLAGS,
-				&msg_ctr);
-	msgvec = (msg_ctr&PCI_MSI_FLAGS_QSIZE) >> 4;
-	if (msgvec == 0)
-		msgvec = (msg_ctr & PCI_MSI_FLAGS_QMASK) >> 1;
-	if (msgvec > 5)
-		msgvec = 0;
-
-	irq = assign_irq((1 << msgvec), desc, &pos);
+	irq = assign_irq(1, desc, &pos);
 	if (irq < 0)
 		return irq;
 
-	/*
-	 * write_msi_msg() will update PCI_MSI_FLAGS so there is
-	 * no need to explicitly call pci_write_config_word().
-	 */
-	desc->msi_attrib.multiple = msgvec;
-
 	msg.address_lo = virt_to_phys((void *)pp->msi_data);
 	msg.address_hi = 0x0;
 	msg.data = pos;
@@ -363,7 +322,19 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
 
 static void dw_msi_teardown_irq(struct msi_chip *chip, unsigned int irq)
 {
-	clear_irq(irq);
+	struct msi_desc *msi;
+	struct pcie_port *pp;
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	/* get the port structure */
+	msi = irq_data_get_msi(data);
+	pp = sys_to_pcie(msi->dev->bus->sysdata);
+	if (!pp) {
+		BUG();
+		return;
+	}
+
+	clear_irq_range(pp, irq, 1, data->hwirq);
 }
 
 static struct msi_chip dw_pcie_msi_chip = {
-- 
2.0.0.rc2

[PATCH 3/4] PCI: designware: remove open-coded bitmap operations

[Lucas Stach]

Replace them by using the standard kernel bitmap ops.
No functional change, but makes the code a lot cleaner.

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

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 1f04e978f877..ed54b24e264d 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -193,30 +193,6 @@ void dw_pcie_msi_init(struct pcie_port *pp)
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4, 0);
 }
 
-static int find_valid_pos0(struct pcie_port *pp, int msgvec, int pos, int *pos0)
-{
-	int flag = 1;
-
-	do {
-		pos = find_next_zero_bit(pp->msi_irq_in_use,
-				MAX_MSI_IRQS, pos);
-		/*if you have reached to the end then get out from here.*/
-		if (pos == MAX_MSI_IRQS)
-			return -ENOSPC;
-		/*
-		 * Check if this position is at correct offset.nvec is always a
-		 * power of two. pos0 must be nvec bit aligned.
-		 */
-		if (pos % msgvec)
-			pos += msgvec - (pos % msgvec);
-		else
-			flag = 0;
-	} while (flag);
-
-	*pos0 = pos;
-	return 0;
-}
-
 static void clear_irq_range(struct pcie_port *pp, unsigned int irq_base,
 			    unsigned int nvec, unsigned int pos)
 {
@@ -224,7 +200,6 @@ static void clear_irq_range(struct pcie_port *pp, unsigned int irq_base,
 
 	for (i = 0; i < nvec; i++) {
 		irq_set_msi_desc_off(irq_base, i, NULL);
-		clear_bit(pos + i, pp->msi_irq_in_use);
 		/* Disable corresponding interrupt on MSI controller */
 		res = ((pos + i) / 32) * 12;
 		bit = (pos + i) % 32;
@@ -232,11 +207,13 @@ static void clear_irq_range(struct pcie_port *pp, unsigned int irq_base,
 		val &= ~(1 << bit);
 		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
 	}
+
+	bitmap_release_region(pp->msi_irq_in_use, pos, order_base_2(nvec));
 }
 
 static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 {
-	int res, bit, irq, pos0, pos1, i;
+	int res, bit, irq, pos0, i;
 	u32 val;
 	struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);
 
@@ -245,23 +222,10 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 		return -EINVAL;
 	}
 
-	pos0 = find_first_zero_bit(pp->msi_irq_in_use,
-			MAX_MSI_IRQS);
-	if (pos0 % no_irqs) {
-		if (find_valid_pos0(pp, no_irqs, pos0, &pos0))
-			goto no_valid_irq;
-	}
-	if (no_irqs > 1) {
-		pos1 = find_next_bit(pp->msi_irq_in_use,
-				MAX_MSI_IRQS, pos0);
-		/* there must be nvec number of consecutive free bits */
-		while ((pos1 - pos0) < no_irqs) {
-			if (find_valid_pos0(pp, no_irqs, pos1, &pos0))
-				goto no_valid_irq;
-			pos1 = find_next_bit(pp->msi_irq_in_use,
-					MAX_MSI_IRQS, pos0);
-		}
-	}
+	pos0 = bitmap_find_free_region(pp->msi_irq_in_use, MAX_MSI_IRQS,
+				       order_base_2(no_irqs));
+	if (pos0 < 0)
+		goto no_valid_irq;
 
 	irq = irq_find_mapping(pp->irq_domain, pos0);
 	if (!irq)
@@ -279,7 +243,6 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 			clear_irq_range(pp, irq, i, pos0);
 			goto no_valid_irq;
 		}
-		set_bit(pos0 + i, pp->msi_irq_in_use);
 		/*Enable corresponding interrupt in MSI interrupt controller */
 		res = ((pos0 + i) / 32) * 12;
 		bit = (pos0 + i) % 32;
-- 
2.0.0.rc2

[PATCH 4/4] PCI: designware: implement multiple MSI irq setup

[Lucas Stach]

Allows to properly set up multiple MSI irqs per
device.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
The ifdef is needed to avoid a compile error on
!CONFIG_PCI_MSI, as msi_list is only part of pci_dev
when the kernel is compiled with MSI support.
---
 drivers/pci/host/pcie-designware.c | 41 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index ed54b24e264d..985eb89fae94 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -252,6 +252,9 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
 	}
 
 	*pos = pos0;
+	desc->nvec_used = no_irqs;
+	desc->msi_attrib.multiple = order_base_2(no_irqs);
+
 	return irq;
 
 no_valid_irq:
@@ -282,6 +285,43 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
 
 	return 0;
 }
+#ifdef CONFIG_PCI_MSI
+static int dw_msi_setup_irqs(struct msi_chip *chip, struct pci_dev *pdev,
+			     int nvec, int type)
+{
+	int irq, pos;
+	struct msi_desc *desc;
+	struct msi_msg msg;
+	struct pcie_port *pp = sys_to_pcie(pdev->bus->sysdata);
+
+	if (!pp)
+		return -EINVAL;
+
+	/* MSI-X interrupts are not supported */
+	if (type == PCI_CAP_ID_MSIX)
+		return -EINVAL;
+
+	WARN_ON(!list_is_singular(&pdev->msi_list));
+	desc = list_entry(pdev->msi_list.next, struct msi_desc, list);
+
+	irq = assign_irq(nvec, desc, &pos);
+	if (irq < 0)
+		return irq;
+
+	msg.address_lo = virt_to_phys((void *)pp->msi_data);
+	msg.address_hi = 0x0;
+	msg.data = pos;
+	write_msi_msg(irq, &msg);
+
+	return 0;
+}
+#else
+static int dw_msi_setup_irqs(struct msi_chip *chip, struct pci_dev *pdev,
+			     int nvec, int type)
+{
+	return -ENOSYS;
+}
+#endif
 
 static void dw_msi_teardown_irq(struct msi_chip *chip, unsigned int irq)
 {
@@ -302,6 +342,7 @@ static void dw_msi_teardown_irq(struct msi_chip *chip, unsigned int irq)
 
 static struct msi_chip dw_pcie_msi_chip = {
 	.setup_irq = dw_msi_setup_irq,
+	.setup_irqs = dw_msi_setup_irqs,
 	.teardown_irq = dw_msi_teardown_irq,
 };
 
-- 
2.0.0.rc2

Re: [PATCH 0/4] proper multi MSI handling for designware host

[Jingoo Han]

On Thursday, June 05, 2014 11:46 PM, Lucas Stach wrote:
> 
> This series implements multiple MSI setup and teardown in terms
> of the msichip infrastructure for the designware host controller.
> It removes quite a bit of homegrown multi MSI handling from the
> driver, which I suspect wasn't really tested before.
> 
> The series is currently based on pci/next.
> 
> Functionality was tested with an FPGA connected to an i.MX6 SoC.
> I have verified that the FPGA is able to trigger the second MSI
> and the irq is properly routed into the driver.

(+cc Marek Vasut, Richard Zhu, Kishon Vijay Abraham I, Pratyush Anand)

Hi Lucas Stach,

As usual, this is a good job! :-)
I tested these patches on Exynos5440 platform with 2 LAN cards.
It works properly. I have no objection about these patches.
However, I hope that others will test with their platform.
I will review your patches. Thank you.

Best regards,
Jingoo Han
> 
> Lucas Stach (4):
>   PCI: allow MSI chip providers to implement their own multiple MSI
>     setup
>   PCI: designware: remove bogus multiple MSI setup
>   PCI: designware: remove open-coded bitmap operations
>   PCI: designware: implement multiple MSI irq setup
> 
>  drivers/pci/host/pcie-designware.c | 141 +++++++++++++++----------------------
>  drivers/pci/msi.c                  |   3 +
>  include/linux/msi.h                |   2 +
>  3 files changed, 63 insertions(+), 83 deletions(-)
> 
> --
> 2.0.0.rc2

Re: [PATCH 1/4] PCI: allow MSI chip providers to implement their own multiple MSI setup

[Jingoo Han]

On Thursday, June 05, 2014 11:46 PM, Lucas Stach wrote:
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

(+cc Marek Vasut, Richard Zhu, Kishon Vijay Abraham I, Pratyush Anand)

Reviewed-by: Jingoo Han <jg1.han@samsung.com>

However, I want more detailed commit message as possible.
Thank you.

Best regards,
Jingoo Han

> ---
>  drivers/pci/msi.c   | 3 +++
>  include/linux/msi.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 27a7e67ddfe4..c45399d3061a 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -68,9 +68,12 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> 
>  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  {
> +	struct msi_chip *chip = dev->bus->msi;
>  	struct msi_desc *entry;
>  	int ret;
> 
> +	if (chip && chip->setup_irqs)
> +		return chip->setup_irqs(chip, dev, nvec, type);
>  	/*
>  	 * If an architecture wants to support multiple MSI, it needs to
>  	 * override arch_setup_msi_irqs()
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 92a2f991262a..d813f898194c 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -75,6 +75,8 @@ struct msi_chip {
> 
>  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
>  			 struct msi_desc *desc);
> +	int (*setup_irqs)(struct msi_chip *chip, struct pci_dev *dev,
> +			  int nvec, int type);
>  	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
>  	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
>  			    int nvec, int type);
> --
> 2.0.0.rc2

Re: [PATCH 1/4] PCI: allow MSI chip providers to implement their own multiple MSI setup

[Marek Vasut]

On Thursday, June 12, 2014 at 12:25:35 PM, Jingoo Han wrote:
> On Thursday, June 05, 2014 11:46 PM, Lucas Stach wrote:
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> 
> (+cc Marek Vasut, Richard Zhu, Kishon Vijay Abraham I, Pratyush Anand)
> 
> Reviewed-by: Jingoo Han <jg1.han@samsung.com>
> 
> However, I want more detailed commit message as possible.
> Thank you.

Fully agree on that.

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 1/4] PCI: allow MSI chip providers to implement their own multiple MSI setup

[Pratyush Anand]

 Hi Lucas,


On Thu, Jun 5, 2014 at 8:16 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/pci/msi.c   | 3 +++
>  include/linux/msi.h | 2 ++
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 27a7e67ddfe4..c45399d3061a 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -68,9 +68,12 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
>
>  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  {
> +       struct msi_chip *chip = dev->bus->msi;
>         struct msi_desc *entry;
>         int ret;
>
> +       if (chip && chip->setup_irqs)

I think, you should also check here for nvec > 1

> +               return chip->setup_irqs(chip, dev, nvec, type);

Before return, shouldn't we set chip_data for all desc->irq?

>         /*
>          * If an architecture wants to support multiple MSI, it needs to
>          * override arch_setup_msi_irqs()

This comment can be modified like "If an architecture wants
to support multiple MSI, it needs to either override arch_setup_msi_irqs()
or provide support of setup_irqs."

Regards
Pratyush

Re: [PATCH 1/4] PCI: allow MSI chip providers to implement their own multiple MSI setup

[Lucas Stach]

Hi Pratyush,

Am Freitag, den 13.06.2014, 11:12 +0530 schrieb Pratyush Anand:
>  Hi Lucas,
> 
> 
> On Thu, Jun 5, 2014 at 8:16 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/pci/msi.c   | 3 +++
> >  include/linux/msi.h | 2 ++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 27a7e67ddfe4..c45399d3061a 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -68,9 +68,12 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> >
> >  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> >  {
> > +       struct msi_chip *chip = dev->bus->msi;
> >         struct msi_desc *entry;
> >         int ret;
> >
> > +       if (chip && chip->setup_irqs)
> 
> I think, you should also check here for nvec > 1

No, nvec == 1 is just a special case and is perfectly valid.
> 
> > +               return chip->setup_irqs(chip, dev, nvec, type);
> 
> Before return, shouldn't we set chip_data for all desc->irq?

Setting chip_data is done in the MSI irq domains map callback and is the
same for single or multiple MSI setup. No need to do anything special
here.

> >         /*
> >          * If an architecture wants to support multiple MSI, it needs to
> >          * override arch_setup_msi_irqs()
> 
> This comment can be modified like "If an architecture wants
> to support multiple MSI, it needs to either override arch_setup_msi_irqs()
> or provide support of setup_irqs."
> 
Right, I'll extend that comment for v2.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/4] PCI: allow MSI chip providers to implement their own multiple MSI setup

[Pratyush Anand]

Hi Lucas,



On Fri, Jun 13, 2014 at 7:50 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Hi Pratyush,
>
> Am Freitag, den 13.06.2014, 11:12 +0530 schrieb Pratyush Anand:
>>  Hi Lucas,
>>
>>
>> On Thu, Jun 5, 2014 at 8:16 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> > ---
>> >  drivers/pci/msi.c   | 3 +++
>> >  include/linux/msi.h | 2 ++
>> >  2 files changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> > index 27a7e67ddfe4..c45399d3061a 100644
>> > --- a/drivers/pci/msi.c
>> > +++ b/drivers/pci/msi.c
>> > @@ -68,9 +68,12 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
>> >
>> >  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>> >  {
>> > +       struct msi_chip *chip = dev->bus->msi;
>> >         struct msi_desc *entry;
>> >         int ret;
>> >
>> > +       if (chip && chip->setup_irqs)
>>
>> I think, you should also check here for nvec > 1
>
> No, nvec == 1 is just a special case and is perfectly valid.

So it means that the platform which supports setup_irqs need not
support setup_irq. May be it can be
documented atleast as a comment somewhere and then setup_irq can be
removed from designware driver.

>>
>> > +               return chip->setup_irqs(chip, dev, nvec, type);
>>
>> Before return, shouldn't we set chip_data for all desc->irq?
>
> Setting chip_data is done in the MSI irq domains map callback and is the
> same for single or multiple MSI setup. No need to do anything special
> here.

But I see arch_setup_msi_irq function in this file doing it.

Regards
Pratyush

Re: [PATCH 1/4] PCI: allow MSI chip providers to implement their own multiple MSI setup

[Lucas Stach]

Am Samstag, den 14.06.2014, 14:15 +0530 schrieb Pratyush Anand:
> Hi Lucas,
> 
> 
> 
> On Fri, Jun 13, 2014 at 7:50 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Hi Pratyush,
> >
> > Am Freitag, den 13.06.2014, 11:12 +0530 schrieb Pratyush Anand:
> >>  Hi Lucas,
> >>
> >>
> >> On Thu, Jun 5, 2014 at 8:16 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> >> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> >> > ---
> >> >  drivers/pci/msi.c   | 3 +++
> >> >  include/linux/msi.h | 2 ++
> >> >  2 files changed, 5 insertions(+)
> >> >
> >> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> >> > index 27a7e67ddfe4..c45399d3061a 100644
> >> > --- a/drivers/pci/msi.c
> >> > +++ b/drivers/pci/msi.c
> >> > @@ -68,9 +68,12 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> >> >
> >> >  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> >> >  {
> >> > +       struct msi_chip *chip = dev->bus->msi;
> >> >         struct msi_desc *entry;
> >> >         int ret;
> >> >
> >> > +       if (chip && chip->setup_irqs)
> >>
> >> I think, you should also check here for nvec > 1
> >
> > No, nvec == 1 is just a special case and is perfectly valid.
> 
> So it means that the platform which supports setup_irqs need not
> support setup_irq. May be it can be
> documented atleast as a comment somewhere and then setup_irq can be
> removed from designware driver.
> 
While 1 irq is a completely valid number of irqs in the multi MSI case
the requirement is actually the other way around: setting up a single
irq is required to be implemented by every MSI chip provider, while the
ability to set up multiple MSIs is optional.

As the semantics of both entry points is slightly different you can not
just remove the single MSI irq setup function if you support the
extended multi MSI setup.

> >>
> >> > +               return chip->setup_irqs(chip, dev, nvec, type);
> >>
> >> Before return, shouldn't we set chip_data for all desc->irq?
> >
> > Setting chip_data is done in the MSI irq domains map callback and is the
> > same for single or multiple MSI setup. No need to do anything special
> > here.
> 
> But I see arch_setup_msi_irq function in this file doing it.
> 
Right, but I think this is wrong and should be corrected. Both Tegra and
designware are setting this up in their MSI irq domain map callback,
which is the right place to do this IMO. I haven't looked at the other
MSI chip providers yet, but will do so.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |