| Message ID | 1401979572-32101-2-git-send-email-l.stach@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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);
Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/pci/msi.c | 3 +++ include/linux/msi.h | 2 ++ 2 files changed, 5 insertions(+)