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

[Lucas Stach <l.stach@pengutronix.de>]

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/  |