| Message ID | 1258352362-11231-1-git-send-email-vivek.mahajan@freescale.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Nov 15, 2009 at 10:19 PM, Vivek Mahajan <vivek.mahajan@freescale.com> wrote: > The following patch adds MSI support. Some platforms > may have broken MSI, so those are defaulted to use > legacy PCI interrupts. > > Signed-off-by: Vivek Mahajan <vivek.mahajan@freescale.com> > --- > drivers/ata/sata_sil24.c | 9 +++++++++ > 1 files changed, 9 insertions(+), 0 deletions(-) > > diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c > index e6946fc..1370df6 100644 > --- a/drivers/ata/sata_sil24.c > +++ b/drivers/ata/sata_sil24.c > @@ -417,6 +417,10 @@ static struct ata_port_operations sil24_ops = { > #endif > }; > > +static int sata_sil24_msi; /* Disable MSI */ > +module_param_named(msi, sata_sil24_msi, bool, S_IRUGO); > +MODULE_PARM_DESC(msi, "Enable MSI (Default: false)"); Vivek, Do we even still need the parameter? I'm thinking either MSI works with a chipset or it doesn't. The kernel has globals to "know" which state is true. If the parameter is needed, when this driver is compiled into the kernel, how is "msi" parameter specified? I think the parameter needs to be documented and fit in with other "msi" parameters. See "nomsi" in Documentation/kernel-parameters.txt. If you want to keep this module parameter, can I suggest calling the exported parameter "sata_sil24_msi"? I'm not able to test this since the chipset I have sata_sil24 devices plugged into don't support MSI (older AMD/Nvidia chipset). :( > + > /* > * Use bits 30-31 of port_flags to encode available port numbers. > * Current maxium is 4. > @@ -1340,6 +1344,11 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > > sil24_init_controller(host); > > + if (sata_sil24_msi && !!pci_msi_enable(pdev)) { > + dev_printk(KERN_INFO, &pdev->dev, "Using MSI\n"); > + pci_intx(pdev, 0); pci_intx() isn't documented in MSI-HOWTO.txt - because it's already called: pci_msi_enable() -> pci_msi_enable_block() -> msi_capability_init() -> pci_intx_for_msi(dev, 0) -> pci_intx(pdev, 0); (thanks to willy (Mathew Wilcox) for pointing me at msi_capability_init() - I overlooked it) Please add "Reviewed-by: Grant Grundler <grundler@google.com>" once the variable name is changed and the pci_intx() call is removed. cheers, grant > + } > + > pci_set_master(pdev); > return ata_host_activate(host, pdev->irq, sil24_interrupt, IRQF_SHARED, > &sil24_sht); > -- > 1.5.6.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/16/2009 01:19 AM, Vivek Mahajan wrote: > The following patch adds MSI support. Some platforms > may have broken MSI, so those are defaulted to use > legacy PCI interrupts. > > Signed-off-by: Vivek Mahajan<vivek.mahajan@freescale.com> > --- > drivers/ata/sata_sil24.c | 9 +++++++++ > 1 files changed, 9 insertions(+), 0 deletions(-) applied -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Grant Grundler [mailto:grundler@google.com] > Sent: Monday, November 16, 2009 11:08 PM > > +static int sata_sil24_msi; /* Disable MSI */ > > +module_param_named(msi, sata_sil24_msi, bool, S_IRUGO); > > +MODULE_PARM_DESC(msi, "Enable MSI (Default: false)"); > > Vivek, > Do we even still need the parameter? I'm thinking either MSI > works with a chipset or it doesn't. The kernel has globals to > "know" which state is true. Sometimes even in a platform, some PCIe endpoints do very well with MSI while others may have to resort to legacy ints. Should we let the endpoints make the final call. > > If the parameter is needed, when this driver is compiled into > the kernel, how is "msi" parameter specified? > I think the parameter needs to be documented and fit in with > other "msi" parameters. > See "nomsi" in Documentation/kernel-parameters.txt. In this case "msi" is supposed to be passed via insmod and not via kernel cmdline. If the driver is built-in the kernel, then force sata_sil24_msi = 1 in the driver to enable it. > > If you want to keep this module parameter, can I suggest > calling the exported parameter "sata_sil24_msi"? > Will do it in the subsequent patch. > pci_intx() isn't documented in MSI-HOWTO.txt - because it's > already called: > pci_msi_enable() -> pci_msi_enable_block() -> > msi_capability_init() > -> pci_intx_for_msi(dev, 0) -> pci_intx(pdev, 0); > > (thanks to willy (Mathew Wilcox) for pointing me at > msi_capability_init() - I overlooked it) > > Please add "Reviewed-by: Grant Grundler > <grundler@google.com>" once the variable name is changed and > the pci_intx() call is removed. Will take care in the upcoming patch > > cheers, > grant Thanks, Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/17/2009 01:59 AM, Mahajan Vivek-B08308 wrote: >> From: Grant Grundler [mailto:grundler@google.com] >> Sent: Monday, November 16, 2009 11:08 PM >>> +static int sata_sil24_msi; /* Disable MSI */ >>> +module_param_named(msi, sata_sil24_msi, bool, S_IRUGO); >>> +MODULE_PARM_DESC(msi, "Enable MSI (Default: false)"); >> >> Vivek, >> Do we even still need the parameter? I'm thinking either MSI >> works with a chipset or it doesn't. The kernel has globals to >> "know" which state is true. > > Sometimes even in a platform, some PCIe endpoints do very > well with MSI while others may have to resort to legacy ints. > Should we let the endpoints make the final call. > >> >> If the parameter is needed, when this driver is compiled into >> the kernel, how is "msi" parameter specified? >> I think the parameter needs to be documented and fit in with >> other "msi" parameters. >> See "nomsi" in Documentation/kernel-parameters.txt. > > In this case "msi" is supposed to be passed via insmod and > not via kernel cmdline. If the driver is built-in the kernel, > then force sata_sil24_msi = 1 in the driver to enable it. First, the original patch was just fine, and it was applied. You should have received email confirmation of this already. Second, all module options are available on the kernel command line, when a module is built into the kernel. You supply a module name prefix to each module option, on the kernel command line. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Jeff Garzik [mailto:jeff@garzik.org] > Sent: Tuesday, November 17, 2009 1:12 PM > > In this case "msi" is supposed to be passed via insmod and not via > > kernel cmdline. If the driver is built-in the kernel, then force > > sata_sil24_msi = 1 in the driver to enable it. > > First, the original patch was just fine, and it was applied. > You should have received email confirmation of this already. Yes, I did. > Second, all module options are available on the kernel > command line, when a module is built into the kernel. You > supply a module name prefix to each module option, on the > kernel command line. Correct, sata_sil24.msi enables it. > > Jeff Thanks, Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c index e6946fc..1370df6 100644 --- a/drivers/ata/sata_sil24.c +++ b/drivers/ata/sata_sil24.c @@ -417,6 +417,10 @@ static struct ata_port_operations sil24_ops = { #endif }; +static int sata_sil24_msi; /* Disable MSI */ +module_param_named(msi, sata_sil24_msi, bool, S_IRUGO); +MODULE_PARM_DESC(msi, "Enable MSI (Default: false)"); + /* * Use bits 30-31 of port_flags to encode available port numbers. * Current maxium is 4. @@ -1340,6 +1344,11 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) sil24_init_controller(host); + if (sata_sil24_msi && !pci_enable_msi(pdev)) { + dev_printk(KERN_INFO, &pdev->dev, "Using MSI\n"); + pci_intx(pdev, 0); + } + pci_set_master(pdev); return ata_host_activate(host, pdev->irq, sil24_interrupt, IRQF_SHARED, &sil24_sht);
The following patch adds MSI support. Some platforms may have broken MSI, so those are defaulted to use legacy PCI interrupts. Signed-off-by: Vivek Mahajan <vivek.mahajan@freescale.com> --- drivers/ata/sata_sil24.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)