[PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Vivek Mahajan]

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)");
+
 /*
  * 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);
-- 
1.5.6.5

Re: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Grant Grundler]

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
>

Re: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Grant Grundler]

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>
> ---
> =C2=A0drivers/ata/sata_sil24.c | =C2=A0 =C2=A09 +++++++++
> =C2=A01 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 =3D {
> =C2=A0#endif
> =C2=A0};
>
> +static int sata_sil24_msi; =C2=A0 =C2=A0/* 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, h=
ow
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). :(


> +
> =C2=A0/*
> =C2=A0* Use bits 30-31 of port_flags to encode available port numbers.
> =C2=A0* Current maxium is 4.
> @@ -1340,6 +1344,11 @@ static int sil24_init_one(struct pci_dev *pdev, co=
nst struct pci_device_id *ent)
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0sil24_init_controller(host);
>
> + =C2=A0 =C2=A0 =C2=A0 if (sata_sil24_msi && !!pci_msi_enable(pdev)) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_printk(KERN_INFO, =
&pdev->dev, "Using MSI\n");
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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

> + =C2=A0 =C2=A0 =C2=A0 }
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0pci_set_master(pdev);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return ata_host_activate(host, pdev->irq, sil2=
4_interrupt, IRQF_SHARED,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &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 =C2=A0http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Jeff Garzik]

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

RE: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Mahajan Vivek-B08308]

> 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
 

RE: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Mahajan Vivek-B08308]

> From: Grant Grundler [mailto:grundler@google.com]=20
> Sent: Monday, November 16, 2009 11:08 PM
> > +static int sata_sil24_msi; =A0 =A0/* Disable MSI */=20
> > +module_param_named(msi, sata_sil24_msi, bool, S_IRUGO);=20
> > +MODULE_PARM_DESC(msi, "Enable MSI (Default: false)");
>=20
> Vivek,
> Do we even still need the parameter? I'm thinking either MSI=20
> works with a chipset or it doesn't. The kernel has globals to=20
> "know" which state is true.

Sometimes even in a platform, some PCIe endpoints do very=20
well with MSI while others may have to resort to legacy ints.=20
Should we let the endpoints make the final call.

>=20
> If the parameter is needed, when this driver is compiled into=20
> the kernel, how is "msi" parameter specified?
> I think the parameter needs to be documented and fit in with=20
> other "msi" parameters.
> See "nomsi" in Documentation/kernel-parameters.txt.

In this case "msi" is supposed to be passed via insmod and=20
not via kernel cmdline. If the driver is built-in the kernel,=20
then force sata_sil24_msi =3D 1 in the driver to enable it.

>=20
> If you want to keep this module parameter, can I suggest=20
> calling the exported parameter "sata_sil24_msi"?
>=20

Will do it in the subsequent patch.

> pci_intx() isn't documented in MSI-HOWTO.txt  - because it's=20
> already called:
>     pci_msi_enable() -> pci_msi_enable_block() ->=20
> msi_capability_init()
>        -> pci_intx_for_msi(dev, 0) -> pci_intx(pdev, 0);
>=20
> (thanks to willy (Mathew Wilcox) for pointing me at
> msi_capability_init() - I overlooked it)
>=20
> Please add "Reviewed-by: Grant Grundler=20
> <grundler@google.com>" once the variable name is changed and=20
> the pci_intx() call is removed.

Will take care in the upcoming patch

>=20
> cheers,
> grant

Thanks,
Vivek
=20

Re: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Jeff Garzik]

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

Re: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Jeff Garzik]

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

RE: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Mahajan Vivek-B08308]

> 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 

RE: [PATCH 1/1] ata/sata_sil24: MSI support, disabled by default

[Mahajan Vivek-B08308]

> From: Jeff Garzik [mailto:jeff@garzik.org]=20
> Sent: Tuesday, November 17, 2009 1:12 PM
> > In this case "msi" is supposed to be passed via insmod and not via=20
> > kernel cmdline. If the driver is built-in the kernel, then force=20
> > sata_sil24_msi =3D 1 in the driver to enable it.
>=20
> First, the original patch was just fine, and it was applied. =20
> You should have received email confirmation of this already.

Yes, I did.

> Second, all module options are available on the kernel=20
> command line, when a module is built into the kernel.  You=20
> supply a module name prefix to each module option, on the=20
> kernel command line.

Correct, sata_sil24.msi enables it.

>=20
> 	Jeff

Thanks,
Vivek=20