| Message ID | 1341830970-12779-1-git-send-email-Shengzhou.Liu@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 07/09/2012 05:49 AM, Shengzhou Liu wrote: > On some platforms, in RC mode, root port has neither MSI/MSI-X nor INTx > interrupt generated, which are available only in EP mode on those platform. > In this case, we try to use other interrupt if supported (i.e. there is the > shared error interrupt on platform P1010, P3041, P4080, etc) to have AER, > Hot-plug, etc, services to work. > > Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> > --- > Documentation/kernel-parameters.txt | 4 ++++ > drivers/pci/pcie/portdrv_core.c | 19 +++++++++++++++++++ > 2 files changed, 23 insertions(+), 0 deletions(-) > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt > index a92c5eb..af97c81 100644 > --- a/Documentation/kernel-parameters.txt > +++ b/Documentation/kernel-parameters.txt > @@ -2218,6 +2218,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted. > nomsi Do not use MSI for native PCIe PME signaling (this makes > all PCIe root ports use INTx for all services). > > + pcie_irq= [PCIE] Native PCIe root port interrupt options: > + other Try to use other interrupt when root port has > + neither MSI/MSI-X nor INTx support. Why does the user need to specify this? Shouldn't this be a matter of communication between kernel internals? > @@ -216,6 +227,14 @@ static int init_service_irqs(struct pci_dev *dev, int *irqs, int mask) > if (!pci_enable_msi(dev) || dev->pin) > irq = dev->irq; > > + /* > + * On some platforms, root port has neither MSI/MSI-X nor INTx > + * interrupt support in RC mode, so try to use other interrupt(i.e. > + * shared interrupt if supported). > + */ > + else if (port_other_interrupt_enabled && dev->irq) > + irq = dev->irq; Is there any reason to not use dev->irq if it is non-zero? -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Tuesday, July 10, 2012 12:39 AM > To: Liu Shengzhou-B36685 > Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org > Subject: Re: [PATCH] PCI: Add pcie_irq=other to enable non MSI/INTx interrupt > for port service driver > > On 07/09/2012 05:49 AM, Shengzhou Liu wrote: > > On some platforms, in RC mode, root port has neither MSI/MSI-X nor > > INTx interrupt generated, which are available only in EP mode on those > platform. > > In this case, we try to use other interrupt if supported (i.e. there > > is the shared error interrupt on platform P1010, P3041, P4080, etc) to > > have AER, Hot-plug, etc, services to work. > > > > Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> > > --- > > Documentation/kernel-parameters.txt | 4 ++++ > > drivers/pci/pcie/portdrv_core.c | 19 +++++++++++++++++++ > > 2 files changed, 23 insertions(+), 0 deletions(-) > > > > diff --git a/Documentation/kernel-parameters.txt > > b/Documentation/kernel-parameters.txt > > index a92c5eb..af97c81 100644 > > --- a/Documentation/kernel-parameters.txt > > +++ b/Documentation/kernel-parameters.txt > > @@ -2218,6 +2218,10 @@ bytes respectively. Such letter suffixes can also be > entirely omitted. > > nomsi Do not use MSI for native PCIe PME signaling (this makes > > all PCIe root ports use INTx for all services). > > > > + pcie_irq= [PCIE] Native PCIe root port interrupt options: > > + other Try to use other interrupt when root port has > > + neither MSI/MSI-X nor INTx support. > > Why does the user need to specify this? Shouldn't this be a matter of > communication between kernel internals? > The "other interrupt" appears a non-standard interrupt way compared to MSI/MSI-X and INTx in point of PCIe spec. The intent of specifying this is to have an intervention and confirmation manually to avoid causing unexpected issue on some unknown platforms. I'm glad to remove the specified kernel parameter if it would be accepted upstream. > > @@ -216,6 +227,14 @@ static int init_service_irqs(struct pci_dev *dev, int > *irqs, int mask) > > if (!pci_enable_msi(dev) || dev->pin) > > irq = dev->irq; > > > > + /* > > + * On some platforms, root port has neither MSI/MSI-X nor INTx > > + * interrupt support in RC mode, so try to use other interrupt(i.e. > > + * shared interrupt if supported). > > + */ > > + else if (port_other_interrupt_enabled && dev->irq) > > + irq = dev->irq; > > Is there any reason to not use dev->irq if it is non-zero? > > -Scott I guess a thinking of the original author did so is based on the point of PCIe spec, whether INTx is supported or not is decided by the "PCI Express Interrupt Pin Register(Read only)". Is there the case on some other unknown platforms that dev->irq is non-zero due to incorrect initialization of irq allocation, but actually it doesn't support MSI/MSIX/INTx or any other interrupt? If we don't need to consider this case, we can use dev->irq directly instead of dev->pin to support "other interrupt". Thanks, Shengzhou
On 07/10/2012 01:13 AM, Liu Shengzhou-B36685 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Tuesday, July 10, 2012 12:39 AM >> To: Liu Shengzhou-B36685 >> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; linuxppc- >> dev@lists.ozlabs.org >> Subject: Re: [PATCH] PCI: Add pcie_irq=other to enable non MSI/INTx interrupt >> for port service driver >> >> On 07/09/2012 05:49 AM, Shengzhou Liu wrote: >>> On some platforms, in RC mode, root port has neither MSI/MSI-X nor >>> INTx interrupt generated, which are available only in EP mode on those >> platform. >>> In this case, we try to use other interrupt if supported (i.e. there >>> is the shared error interrupt on platform P1010, P3041, P4080, etc) to >>> have AER, Hot-plug, etc, services to work. >>> >>> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> >>> --- >>> Documentation/kernel-parameters.txt | 4 ++++ >>> drivers/pci/pcie/portdrv_core.c | 19 +++++++++++++++++++ >>> 2 files changed, 23 insertions(+), 0 deletions(-) >>> >>> diff --git a/Documentation/kernel-parameters.txt >>> b/Documentation/kernel-parameters.txt >>> index a92c5eb..af97c81 100644 >>> --- a/Documentation/kernel-parameters.txt >>> +++ b/Documentation/kernel-parameters.txt >>> @@ -2218,6 +2218,10 @@ bytes respectively. Such letter suffixes can also be >> entirely omitted. >>> nomsi Do not use MSI for native PCIe PME signaling (this makes >>> all PCIe root ports use INTx for all services). >>> >>> + pcie_irq= [PCIE] Native PCIe root port interrupt options: >>> + other Try to use other interrupt when root port has >>> + neither MSI/MSI-X nor INTx support. >> >> Why does the user need to specify this? Shouldn't this be a matter of >> communication between kernel internals? >> > > The "other interrupt" appears a non-standard interrupt way compared to MSI/MSI-X and INTx in point of PCIe spec. It still shouldn't be the user's responsibility to pass this in. > The intent of specifying this is to have an intervention and > confirmation manually to avoid causing unexpected issue on some > unknown platforms. > > I'm glad to remove the specified kernel parameter if it would be accepted upstream. Hopefully someone will comment if there is harm in doing this unconditionally. If there is, then we should handle this via a quirk or similar mechanism. -Scott
On Jul 10, 2012, at 10:31 AM, Scott Wood wrote: > On 07/10/2012 01:13 AM, Liu Shengzhou-B36685 wrote: >> >> >>> -----Original Message----- >>> From: Wood Scott-B07421 >>> Sent: Tuesday, July 10, 2012 12:39 AM >>> To: Liu Shengzhou-B36685 >>> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; linuxppc- >>> dev@lists.ozlabs.org >>> Subject: Re: [PATCH] PCI: Add pcie_irq=other to enable non MSI/INTx interrupt >>> for port service driver >>> >>> On 07/09/2012 05:49 AM, Shengzhou Liu wrote: >>>> On some platforms, in RC mode, root port has neither MSI/MSI-X nor >>>> INTx interrupt generated, which are available only in EP mode on those >>> platform. >>>> In this case, we try to use other interrupt if supported (i.e. there >>>> is the shared error interrupt on platform P1010, P3041, P4080, etc) to >>>> have AER, Hot-plug, etc, services to work. >>>> >>>> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> >>>> --- >>>> Documentation/kernel-parameters.txt | 4 ++++ >>>> drivers/pci/pcie/portdrv_core.c | 19 +++++++++++++++++++ >>>> 2 files changed, 23 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/Documentation/kernel-parameters.txt >>>> b/Documentation/kernel-parameters.txt >>>> index a92c5eb..af97c81 100644 >>>> --- a/Documentation/kernel-parameters.txt >>>> +++ b/Documentation/kernel-parameters.txt >>>> @@ -2218,6 +2218,10 @@ bytes respectively. Such letter suffixes can also be >>> entirely omitted. >>>> nomsi Do not use MSI for native PCIe PME signaling (this makes >>>> all PCIe root ports use INTx for all services). >>>> >>>> + pcie_irq= [PCIE] Native PCIe root port interrupt options: >>>> + other Try to use other interrupt when root port has >>>> + neither MSI/MSI-X nor INTx support. >>> >>> Why does the user need to specify this? Shouldn't this be a matter of >>> communication between kernel internals? >>> >> >> The "other interrupt" appears a non-standard interrupt way compared to MSI/MSI-X and INTx in point of PCIe spec. > > It still shouldn't be the user's responsibility to pass this in. I agree with Scott, this should be done in board code or as a PCI fixup, not as a command line arg. >> The intent of specifying this is to have an intervention and >> confirmation manually to avoid causing unexpected issue on some >> unknown platforms. >> >> I'm glad to remove the specified kernel parameter if it would be accepted upstream. > > Hopefully someone will comment if there is harm in doing this > unconditionally. If there is, then we should handle this via a quirk or > similar mechanism. > > -Scott If we need to do it via quirk, wondering if we can use the pci_dev_flags and set the flag in a quirk. - k
> -----Original Message----- > From: Kumar Gala [mailto:galak@kernel.crashing.org] > Sent: Tuesday, July 10, 2012 11:50 PM > To: Wood Scott-B07421 > Cc: Liu Shengzhou-B36685; bhelgaas@google.com; Wood Scott-B07421; linuxppc- > dev@lists.ozlabs.org; linux-pci@vger.kernel.org > Subject: Re: [PATCH] PCI: Add pcie_irq=other to enable non MSI/INTx interrupt > for port service driver > > > On Jul 10, 2012, at 10:31 AM, Scott Wood wrote: > > > On 07/10/2012 01:13 AM, Liu Shengzhou-B36685 wrote: > >> > >> > >>> -----Original Message----- > >>> From: Wood Scott-B07421 > >>> Sent: Tuesday, July 10, 2012 12:39 AM > >>> To: Liu Shengzhou-B36685 > >>> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; linuxppc- > >>> dev@lists.ozlabs.org > >>> Subject: Re: [PATCH] PCI: Add pcie_irq=other to enable non MSI/INTx > >>> interrupt for port service driver > >>> > >>> On 07/09/2012 05:49 AM, Shengzhou Liu wrote: > >>>> On some platforms, in RC mode, root port has neither MSI/MSI-X nor > >>>> INTx interrupt generated, which are available only in EP mode on > >>>> those > >>> platform. > >>>> In this case, we try to use other interrupt if supported (i.e. > >>>> there is the shared error interrupt on platform P1010, P3041, > >>>> P4080, etc) to have AER, Hot-plug, etc, services to work. > >>>> > >>>> Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> > >>>> --- > >>>> Documentation/kernel-parameters.txt | 4 ++++ > >>>> drivers/pci/pcie/portdrv_core.c | 19 +++++++++++++++++++ > >>>> 2 files changed, 23 insertions(+), 0 deletions(-) > >>>> > >>>> diff --git a/Documentation/kernel-parameters.txt > >>>> b/Documentation/kernel-parameters.txt > >>>> index a92c5eb..af97c81 100644 > >>>> --- a/Documentation/kernel-parameters.txt > >>>> +++ b/Documentation/kernel-parameters.txt > >>>> @@ -2218,6 +2218,10 @@ bytes respectively. Such letter suffixes can > >>>> also be > >>> entirely omitted. > >>>> nomsi Do not use MSI for native PCIe PME signaling (this makes > >>>> all PCIe root ports use INTx for all services). > >>>> > >>>> + pcie_irq= [PCIE] Native PCIe root port interrupt options: > >>>> + other Try to use other interrupt when root port has > >>>> + neither MSI/MSI-X nor INTx support. > >>> > >>> Why does the user need to specify this? Shouldn't this be a matter > >>> of communication between kernel internals? > >>> > >> > >> The "other interrupt" appears a non-standard interrupt way compared to > MSI/MSI-X and INTx in point of PCIe spec. > > > > It still shouldn't be the user's responsibility to pass this in. > > I agree with Scott, this should be done in board code or as a PCI fixup, not > as a command line arg. > > >> The intent of specifying this is to have an intervention and > >> confirmation manually to avoid causing unexpected issue on some > >> unknown platforms. > >> > >> I'm glad to remove the specified kernel parameter if it would be accepted > upstream. > > > > Hopefully someone will comment if there is harm in doing this > > unconditionally. If there is, then we should handle this via a quirk > > or similar mechanism. > > > > -Scott > > If we need to do it via quirk, wondering if we can use the pci_dev_flags and > set the flag in a quirk. > > - k Yes, it's better to do that with quirt way, I'm going to submit a new patch with that way. Thanks, Shengzhou
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index a92c5eb..af97c81 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -2218,6 +2218,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted. nomsi Do not use MSI for native PCIe PME signaling (this makes all PCIe root ports use INTx for all services). + pcie_irq= [PCIE] Native PCIe root port interrupt options: + other Try to use other interrupt when root port has + neither MSI/MSI-X nor INTx support. + pcmv= [HW,PCMCIA] BadgePAD 4 pd. [PARIDE] diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 75915b3..653679e 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -30,6 +30,17 @@ static int __init pciehp_setup(char *str) } __setup("pcie_hp=", pciehp_setup); +bool port_other_interrupt_enabled; + +static int __init portservice_setup(char *str) +{ + if (!strncmp(str, "other", 5)) + port_other_interrupt_enabled = true; + + return 1; +} +__setup("pcie_irq=", portservice_setup); + /** * release_pcie_device - free PCI Express port service device structure * @dev: Port service device to release @@ -216,6 +227,14 @@ static int init_service_irqs(struct pci_dev *dev, int *irqs, int mask) if (!pci_enable_msi(dev) || dev->pin) irq = dev->irq; + /* + * On some platforms, root port has neither MSI/MSI-X nor INTx + * interrupt support in RC mode, so try to use other interrupt(i.e. + * shared interrupt if supported). + */ + else if (port_other_interrupt_enabled && dev->irq) + irq = dev->irq; + no_msi: for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) irqs[i] = irq;
On some platforms, in RC mode, root port has neither MSI/MSI-X nor INTx interrupt generated, which are available only in EP mode on those platform. In this case, we try to use other interrupt if supported (i.e. there is the shared error interrupt on platform P1010, P3041, P4080, etc) to have AER, Hot-plug, etc, services to work. Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com> --- Documentation/kernel-parameters.txt | 4 ++++ drivers/pci/pcie/portdrv_core.c | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 0 deletions(-)