diff mbox

ata_piix: make DVD Drive recognisable on systems with Intel Sandybridge chipsets(v1)

Message ID 1317433412-15963-1-git-send-email-ming.lei@canonical.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Ming Lei Oct. 1, 2011, 1:43 a.m. UTC 
From: Ming Lei <ming.lei@canonical.com>

This quirk patch fixes one kind of bug inside some Intel Sandybridge
chipsets, see reports from

       https://bugzilla.kernel.org/show_bug.cgi?id=40592.

Many guys also have reported the problem before:

	https://bugs.launchpad.net/bugs/737388
	https://bugs.launchpad.net/bugs/794642
	https://bugs.launchpad.net/bugs/782389
	......

With help from Tejun, the problem is found to be caused by 32bit PIO
mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
for some Sandybridge CPT chipsets.

Seth also tested the patch on all five affected chipsets
(pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found
the patch does fix the problem.

Tested-by: Heasley, Seth <seth.heasley@intel.com>
Cc: Alan Cox <alan@linux.intel.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/ata_piix.c |   37 ++++++++++++++++++++++++++++++++-----
 include/linux/libata.h |    1 +
 2 files changed, 33 insertions(+), 5 deletions(-)

Comments

Tejun Heo Oct. 1, 2011, 4:38 a.m. UTC | #1 
On Sat, Oct 01, 2011 at 09:43:32AM +0800, ming.lei@canonical.com wrote:
> From: Ming Lei <ming.lei@canonical.com>
> 
> This quirk patch fixes one kind of bug inside some Intel Sandybridge
> chipsets, see reports from
> 
>        https://bugzilla.kernel.org/show_bug.cgi?id=40592.
> 
> Many guys also have reported the problem before:
> 
> 	https://bugs.launchpad.net/bugs/737388
> 	https://bugs.launchpad.net/bugs/794642
> 	https://bugs.launchpad.net/bugs/782389
> 	......
> 
> With help from Tejun, the problem is found to be caused by 32bit PIO
> mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
> for some Sandybridge CPT chipsets.
> 
> Seth also tested the patch on all five affected chipsets
> (pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found
> the patch does fix the problem.
> 
> Tested-by: Heasley, Seth <seth.heasley@intel.com>
> Cc: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Looks good to me but this should have been Acked-by.  SOB is used to
annotate how a patch travels upstream.

Thanks.
Sergei Shtylyov Oct. 5, 2011, 9:45 a.m. UTC | #2 
On 01.10.2011 5:43, ming.lei@canonical.com wrote:

> From: Ming Lei <ming.lei@canonical.com>

> This quirk patch fixes one kind of bug inside some Intel Sandybridge
> chipsets, see reports from

>         https://bugzilla.kernel.org/show_bug.cgi?id=40592.

> Many guys also have reported the problem before:

> 	https://bugs.launchpad.net/bugs/737388
> 	https://bugs.launchpad.net/bugs/794642
> 	https://bugs.launchpad.net/bugs/782389
> 	......

> With help from Tejun, the problem is found to be caused by 32bit PIO
> mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
> for some Sandybridge CPT chipsets.

> Seth also tested the patch on all five affected chipsets
> (pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found
> the patch does fix the problem.

> Tested-by: Heasley, Seth<seth.heasley@intel.com>
> Cc: Alan Cox<alan@linux.intel.com>
> Signed-off-by: Ming Lei<ming.lei@canonical.com>
> Signed-off-by: Tejun Heo<htejun@gmail.com>
> ---
>   drivers/ata/ata_piix.c |   37 ++++++++++++++++++++++++++++++++-----
>   include/linux/libata.h |    1 +
>   2 files changed, 33 insertions(+), 5 deletions(-)

> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 43107e9..70095be 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -113,6 +113,8 @@ enum {
>   	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS,
>   	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
>
> +	PIIX_FLAG_PIO16		= ATA_FLAG_PIO16,

    It's not clear why are you declaring a ganeric flag and then add a local 
name for it...

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index efd6f98..dc68de5 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -207,6 +207,7 @@ enum {
>   	ATA_FLAG_SW_ACTIVITY	= (1 << 22), /* driver supports sw activity
>   					      * led */
>   	ATA_FLAG_NO_DIPM	= (1 << 23), /* host not happy with DIPM */
> +	ATA_FLAG_PIO16		= (1 << 24),  /*16bit PIO */

    Please, fix the formatting. Or better totally remove this.

WBR, Sergei
--
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
Ming Lei Oct. 6, 2011, 9:27 a.m. UTC | #3 
On Wed, Oct 5, 2011 at 5:45 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> On 01.10.2011 5:43, ming.lei@canonical.com wrote:
>
>> From: Ming Lei <ming.lei@canonical.com>
>
>> This quirk patch fixes one kind of bug inside some Intel Sandybridge
>> chipsets, see reports from
>
>>        https://bugzilla.kernel.org/show_bug.cgi?id=40592.
>
>> Many guys also have reported the problem before:
>
>>        https://bugs.launchpad.net/bugs/737388
>>        https://bugs.launchpad.net/bugs/794642
>>        https://bugs.launchpad.net/bugs/782389
>>        ......
>
>> With help from Tejun, the problem is found to be caused by 32bit PIO
>> mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
>> for some Sandybridge CPT chipsets.
>
>> Seth also tested the patch on all five affected chipsets
>> (pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found
>> the patch does fix the problem.
>
>> Tested-by: Heasley, Seth<seth.heasley@intel.com>
>> Cc: Alan Cox<alan@linux.intel.com>
>> Signed-off-by: Ming Lei<ming.lei@canonical.com>
>> Signed-off-by: Tejun Heo<htejun@gmail.com>
>> ---
>>  drivers/ata/ata_piix.c |   37 ++++++++++++++++++++++++++++++++-----
>>  include/linux/libata.h |    1 +
>>  2 files changed, 33 insertions(+), 5 deletions(-)
>
>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>> index 43107e9..70095be 100644
>> --- a/drivers/ata/ata_piix.c
>> +++ b/drivers/ata/ata_piix.c
>> @@ -113,6 +113,8 @@ enum {
>>        PIIX_PATA_FLAGS         = ATA_FLAG_SLAVE_POSS,
>>        PIIX_SATA_FLAGS         = ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
>>
>> +       PIIX_FLAG_PIO16         = ATA_FLAG_PIO16,
>
>   It's not clear why are you declaring a ganeric flag and then add a local
> name for it...

It is just same with other flags, isn't it?

>
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index efd6f98..dc68de5 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -207,6 +207,7 @@ enum {
>>        ATA_FLAG_SW_ACTIVITY    = (1 << 22), /* driver supports sw activity
>>                                              * led */
>>        ATA_FLAG_NO_DIPM        = (1 << 23), /* host not happy with DIPM */
>> +       ATA_FLAG_PIO16          = (1 << 24),  /*16bit PIO */
>
>   Please, fix the formatting. Or better totally remove this.

It is used to describe if the controller only supports 16bit PIO, and it
is introduced to fix the problem on SNB chips.

thanks,
--
Ming Lei
--
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
Sergei Shtylyov Oct. 6, 2011, 10:39 a.m. UTC | #4 
Hello.

On 06-10-2011 13:27, Ming Lei wrote:

>>> This quirk patch fixes one kind of bug inside some Intel Sandybridge
>>> chipsets, see reports from

>>>         https://bugzilla.kernel.org/show_bug.cgi?id=40592.

>>> Many guys also have reported the problem before:

>>>         https://bugs.launchpad.net/bugs/737388
>>>         https://bugs.launchpad.net/bugs/794642
>>>         https://bugs.launchpad.net/bugs/782389
>>>         ......

>>> With help from Tejun, the problem is found to be caused by 32bit PIO
>>> mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
>>> for some Sandybridge CPT chipsets.

>>> Seth also tested the patch on all five affected chipsets
>>> (pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found
>>> the patch does fix the problem.

>>> Tested-by: Heasley, Seth<seth.heasley@intel.com>
>>> Cc: Alan Cox<alan@linux.intel.com>
>>> Signed-off-by: Ming Lei<ming.lei@canonical.com>
>>> Signed-off-by: Tejun Heo<htejun@gmail.com>
>>> ---
>>>   drivers/ata/ata_piix.c |   37 ++++++++++++++++++++++++++++++++-----
>>>   include/linux/libata.h |    1 +
>>>   2 files changed, 33 insertions(+), 5 deletions(-)

>>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>>> index 43107e9..70095be 100644
>>> --- a/drivers/ata/ata_piix.c
>>> +++ b/drivers/ata/ata_piix.c
>>> @@ -113,6 +113,8 @@ enum {
>>>         PIIX_PATA_FLAGS         = ATA_FLAG_SLAVE_POSS,
>>>         PIIX_SATA_FLAGS         = ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
>>>
>>> +       PIIX_FLAG_PIO16         = ATA_FLAG_PIO16,

>>    It's not clear why are you declaring a ganeric flag and then add a local
>> name for it...

> It is just same with other flags, isn't it?

    No. Read the the ata_piix.c source attentively, looking for PIIX_FLAG_* 
definitions.

>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>> index efd6f98..dc68de5 100644
>>> --- a/include/linux/libata.h
>>> +++ b/include/linux/libata.h
>>> @@ -207,6 +207,7 @@ enum {
>>>         ATA_FLAG_SW_ACTIVITY    = (1<<  22), /* driver supports sw activity
>>>                                               * led */
>>>         ATA_FLAG_NO_DIPM        = (1<<  23), /* host not happy with DIPM */
>>> +       ATA_FLAG_PIO16          = (1<<  24),  /*16bit PIO */
>>
>>    Please, fix the formatting. Or better totally remove this.

> It is used to describe if the controller only supports 16bit PIO, and it
> is introduced to fix the problem on SNB chips.

    I don't yet see why it should be a generic flag: we have 
ata_bmdma_port_ops and ata_bmdma32_port_ops to be used for 16-bit and 32-bit 
PIO. You're only using this flag locally to ata_piix.c, hence it should be 
local to that file.

> thanks,
> --
> Ming Lei

WBR, Sergei
--
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
Ming Lei Oct. 6, 2011, 11:53 a.m. UTC | #5 
On Thu, Oct 6, 2011 at 6:39 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 06-10-2011 13:27, Ming Lei wrote:
>
>>>> This quirk patch fixes one kind of bug inside some Intel Sandybridge
>>>> chipsets, see reports from
>
>>>>        https://bugzilla.kernel.org/show_bug.cgi?id=40592.
>
>>>> Many guys also have reported the problem before:
>
>>>>        https://bugs.launchpad.net/bugs/737388
>>>>        https://bugs.launchpad.net/bugs/794642
>>>>        https://bugs.launchpad.net/bugs/782389
>>>>        ......
>
>>>> With help from Tejun, the problem is found to be caused by 32bit PIO
>>>> mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
>>>> for some Sandybridge CPT chipsets.
>
>>>> Seth also tested the patch on all five affected chipsets
>>>> (pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found
>>>> the patch does fix the problem.
>
>>>> Tested-by: Heasley, Seth<seth.heasley@intel.com>
>>>> Cc: Alan Cox<alan@linux.intel.com>
>>>> Signed-off-by: Ming Lei<ming.lei@canonical.com>
>>>> Signed-off-by: Tejun Heo<htejun@gmail.com>
>>>> ---
>>>>  drivers/ata/ata_piix.c |   37 ++++++++++++++++++++++++++++++++-----
>>>>  include/linux/libata.h |    1 +
>>>>  2 files changed, 33 insertions(+), 5 deletions(-)
>
>>>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>>>> index 43107e9..70095be 100644
>>>> --- a/drivers/ata/ata_piix.c
>>>> +++ b/drivers/ata/ata_piix.c
>>>> @@ -113,6 +113,8 @@ enum {
>>>>        PIIX_PATA_FLAGS         = ATA_FLAG_SLAVE_POSS,
>>>>        PIIX_SATA_FLAGS         = ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
>>>>
>>>> +       PIIX_FLAG_PIO16         = ATA_FLAG_PIO16,
>
>>>   It's not clear why are you declaring a ganeric flag and then add a
>>> local
>>> name for it...
>
>> It is just same with other flags, isn't it?
>
>   No. Read the the ata_piix.c source attentively, looking for PIIX_FLAG_*
> definitions.
>
>>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>>> index efd6f98..dc68de5 100644
>>>> --- a/include/linux/libata.h
>>>> +++ b/include/linux/libata.h
>>>> @@ -207,6 +207,7 @@ enum {
>>>>        ATA_FLAG_SW_ACTIVITY    = (1<<  22), /* driver supports sw
>>>> activity
>>>>                                              * led */
>>>>        ATA_FLAG_NO_DIPM        = (1<<  23), /* host not happy with DIPM
>>>> */
>>>> +       ATA_FLAG_PIO16          = (1<<  24),  /*16bit PIO */
>>>
>>>   Please, fix the formatting. Or better totally remove this.
>
>> It is used to describe if the controller only supports 16bit PIO, and it
>> is introduced to fix the problem on SNB chips.
>
>   I don't yet see why it should be a generic flag: we have
> ata_bmdma_port_ops and ata_bmdma32_port_ops to be used for 16-bit and 32-bit
> PIO. You're only using this flag locally to ata_piix.c, hence it should be
> local to that file.

This flag is to stored into ata_port flags, so it is better to define a global
flag. Otherwise, you have to select a value carefully which must not be
defined in ata_port flags already, which way is very error-prone and not
friendly.

Anyway, I have not see obvious drawbacks to define a global ata_port
flag.

thanks,
--
Ming Lei
--
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
Sergei Shtylyov Oct. 6, 2011, 2:06 p.m. UTC | #6 
Hello.

On 10/06/2011 03:53 PM, Ming Lei wrote:

>>>>> This quirk patch fixes one kind of bug inside some Intel Sandybridge
>>>>> chipsets, see reports from

>>>>>         https://bugzilla.kernel.org/show_bug.cgi?id=40592.

>>>>> Many guys also have reported the problem before:

>>>>>         https://bugs.launchpad.net/bugs/737388
>>>>>         https://bugs.launchpad.net/bugs/794642
>>>>>         https://bugs.launchpad.net/bugs/782389
>>>>>         ......

>>>>> With help from Tejun, the problem is found to be caused by 32bit PIO
>>>>> mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
>>>>> for some Sandybridge CPT chipsets.

>>>>> Seth also tested the patch on all five affected chipsets
>>>>> (pci device ID: 0x1c00, 0x1c01, 0x1d00, 0x1e00, 0x1e01), and found
>>>>> the patch does fix the problem.

>>>>> Tested-by: Heasley, Seth<seth.heasley@intel.com>
>>>>> Cc: Alan Cox<alan@linux.intel.com>
>>>>> Signed-off-by: Ming Lei<ming.lei@canonical.com>
>>>>> Signed-off-by: Tejun Heo<htejun@gmail.com>
>>>>> ---
>>>>>   drivers/ata/ata_piix.c |   37 ++++++++++++++++++++++++++++++++-----
>>>>>   include/linux/libata.h |    1 +
>>>>>   2 files changed, 33 insertions(+), 5 deletions(-)

>>>>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>>>>> index 43107e9..70095be 100644
>>>>> --- a/drivers/ata/ata_piix.c
>>>>> +++ b/drivers/ata/ata_piix.c
>>>>> @@ -113,6 +113,8 @@ enum {
>>>>>         PIIX_PATA_FLAGS         = ATA_FLAG_SLAVE_POSS,
>>>>>         PIIX_SATA_FLAGS         = ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
>>>>>
>>>>> +       PIIX_FLAG_PIO16         = ATA_FLAG_PIO16,

>>>>    It's not clear why are you declaring a ganeric flag and then add a
>>>> local
>>>> name for it...

>>> It is just same with other flags, isn't it?

>>    No. Read the the ata_piix.c source attentively, looking for PIIX_FLAG_*
>> definitions.

>>>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>>>> index efd6f98..dc68de5 100644
>>>>> --- a/include/linux/libata.h
>>>>> +++ b/include/linux/libata.h
>>>>> @@ -207,6 +207,7 @@ enum {
>>>>>         ATA_FLAG_SW_ACTIVITY    = (1<<    22), /* driver supports sw
>>>>> activity
>>>>>                                               * led */
>>>>>         ATA_FLAG_NO_DIPM        = (1<<    23), /* host not happy with DIPM
>>>>> */
>>>>> +       ATA_FLAG_PIO16          = (1<<    24),  /*16bit PIO */

>>>>    Please, fix the formatting. Or better totally remove this.

>>> It is used to describe if the controller only supports 16bit PIO, and it
>>> is introduced to fix the problem on SNB chips.

>>    I don't yet see why it should be a generic flag: we have
>> ata_bmdma_port_ops and ata_bmdma32_port_ops to be used for 16-bit and 32-bit
>> PIO. You're only using this flag locally to ata_piix.c, hence it should be
>> local to that file.

> This flag is to stored into ata_port flags, so it is better to define a global
> flag. Otherwise, you have to select a value carefully which must not be
> defined in ata_port flags already, which way is very error-prone and not
> friendly.

    If everyone starts defining global flags for their local cases, we'll run 
out of flags rather quickly. :-)

> Anyway, I have not see obvious drawbacks to define a global ata_port
> flag.

    Then at least there's no need to define a local alias for the global flag, 
don't you think?

> thanks,
> --
> Ming Lei

WBR, Sergei
--
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
Tejun Heo Oct. 6, 2011, 4:54 p.m. UTC | #7 
Hello,

On Thu, Oct 06, 2011 at 07:53:46PM +0800, Ming Lei wrote:
> This flag is to stored into ata_port flags, so it is better to define a global
> flag. Otherwise, you have to select a value carefully which must not be
> defined in ata_port flags already, which way is very error-prone and not
> friendly.

Bits 24:31 of ata_port flags are reserved for driver-specific uses.
Also, this is controller-wide not port-wide, so you can also add
piix_host_priv->flags and use it.

> Anyway, I have not see obvious drawbacks to define a global ata_port
> flag.

Sergei is right here.  Please move it to driver specific flags.  If we
end up doing this for multiple controllers, then we can move the flag
(and handling of it too) to generic code.

Thank you.

--
tejun
--
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
Ming Lei Oct. 7, 2011, 3:31 a.m. UTC | #8 
Hi,

On Fri, Oct 7, 2011 at 12:54 AM, Tejun Heo <htejun@gmail.com> wrote:
> Hello,
>
> On Thu, Oct 06, 2011 at 07:53:46PM +0800, Ming Lei wrote:
>> This flag is to stored into ata_port flags, so it is better to define a global
>> flag. Otherwise, you have to select a value carefully which must not be
>> defined in ata_port flags already, which way is very error-prone and not
>> friendly.
>
> Bits 24:31 of ata_port flags are reserved for driver-specific uses.
> Also, this is controller-wide not port-wide, so you can also add
> piix_host_priv->flags and use it.
>
>> Anyway, I have not see obvious drawbacks to define a global ata_port
>> flag.
>
> Sergei is right here.  Please move it to driver specific flags.  If we
> end up doing this for multiple controllers, then we can move the flag
> (and handling of it too) to generic code.

OK, will update the patch to use local flag.

thanks,
--
Ming Lei
--
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 mbox

Patch

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 43107e9..70095be 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -113,6 +113,8 @@  enum {
 	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS,
 	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
 
+	PIIX_FLAG_PIO16		= ATA_FLAG_PIO16,
+
 	PIIX_80C_PRI		= (1 << 5) | (1 << 4),
 	PIIX_80C_SEC		= (1 << 7) | (1 << 6),
 
@@ -147,6 +149,7 @@  enum piix_controller_ids {
 	ich8m_apple_sata,	/* locks up on second port enable */
 	tolapai_sata,
 	piix_pata_vmw,			/* PIIX4 for VMware, spurious DMA_ERR */
+	ich8_sata_snb,
 };
 
 struct piix_map_db {
@@ -177,6 +180,7 @@  static int piix_sidpr_scr_write(struct ata_link *link,
 static int piix_sidpr_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			      unsigned hints);
 static bool piix_irq_check(struct ata_port *ap);
+static int piix_port_start(struct ata_port *ap);
 #ifdef CONFIG_PM
 static int piix_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
 static int piix_pci_device_resume(struct pci_dev *pdev);
@@ -298,21 +302,21 @@  static const struct pci_device_id piix_pci_tbl[] = {
 	/* SATA Controller IDE (PCH) */
 	{ 0x8086, 0x3b2e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata },
 	/* SATA Controller IDE (CPT) */
-	{ 0x8086, 0x1c00, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata },
+	{ 0x8086, 0x1c00, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_snb },
 	/* SATA Controller IDE (CPT) */
-	{ 0x8086, 0x1c01, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata },
+	{ 0x8086, 0x1c01, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_snb },
 	/* SATA Controller IDE (CPT) */
 	{ 0x8086, 0x1c08, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_2port_sata },
 	/* SATA Controller IDE (CPT) */
 	{ 0x8086, 0x1c09, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_2port_sata },
 	/* SATA Controller IDE (PBG) */
-	{ 0x8086, 0x1d00, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata },
+	{ 0x8086, 0x1d00, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_snb },
 	/* SATA Controller IDE (PBG) */
 	{ 0x8086, 0x1d08, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_2port_sata },
 	/* SATA Controller IDE (Panther Point) */
-	{ 0x8086, 0x1e00, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata },
+	{ 0x8086, 0x1e00, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_snb },
 	/* SATA Controller IDE (Panther Point) */
-	{ 0x8086, 0x1e01, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata },
+	{ 0x8086, 0x1e01, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_sata_snb },
 	/* SATA Controller IDE (Panther Point) */
 	{ 0x8086, 0x1e08, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich8_2port_sata },
 	/* SATA Controller IDE (Panther Point) */
@@ -338,6 +342,7 @@  static struct scsi_host_template piix_sht = {
 static struct ata_port_operations piix_sata_ops = {
 	.inherits		= &ata_bmdma32_port_ops,
 	.sff_irq_check		= piix_irq_check,
+	.port_start		= piix_port_start,
 };
 
 static struct ata_port_operations piix_pata_ops = {
@@ -478,6 +483,7 @@  static const struct piix_map_db *piix_map_db_table[] = {
 	[ich8_2port_sata]	= &ich8_2port_map_db,
 	[ich8m_apple_sata]	= &ich8m_apple_map_db,
 	[tolapai_sata]		= &tolapai_map_db,
+	[ich8_sata_snb]		= &ich8_map_db,
 };
 
 static struct ata_port_info piix_port_info[] = {
@@ -606,6 +612,19 @@  static struct ata_port_info piix_port_info[] = {
 		.port_ops	= &piix_vmw_ops,
 	},
 
+	/*
+	 * some Sandybridge chipsets have broken 32 mode up to now,
+	 * see https://bugzilla.kernel.org/show_bug.cgi?id=40592
+	 */
+	[ich8_sata_snb] =
+	{
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR | PIIX_FLAG_PIO16,
+		.pio_mask	= ATA_PIO4,
+		.mwdma_mask	= ATA_MWDMA2,
+		.udma_mask	= ATA_UDMA6,
+		.port_ops	= &piix_sata_ops,
+	},
+
 };
 
 static struct pci_bits piix_enable_bits[] = {
@@ -649,6 +668,14 @@  static const struct ich_laptop ich_laptop[] = {
 	{ 0, }
 };
 
+static int piix_port_start(struct ata_port *ap)
+{
+	if (!(ap->flags & PIIX_FLAG_PIO16))
+		ap->pflags |= ATA_PFLAG_PIO32 | ATA_PFLAG_PIO32CHANGE;
+
+	return ata_bmdma_port_start(ap);
+}
+
 /**
  *	ich_pata_cable_detect - Probe host controller cable detect info
  *	@ap: Port for which cable detect info is desired
diff --git a/include/linux/libata.h b/include/linux/libata.h
index efd6f98..dc68de5 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -207,6 +207,7 @@  enum {
 	ATA_FLAG_SW_ACTIVITY	= (1 << 22), /* driver supports sw activity
 					      * led */
 	ATA_FLAG_NO_DIPM	= (1 << 23), /* host not happy with DIPM */
+	ATA_FLAG_PIO16		= (1 << 24),  /*16bit PIO */
 
 	/* bits 24:31 of ap->flags are reserved for LLD specific flags */