[PATCH] ahci_sunxi: Drop AHCI_HFLAG_NO_PMP flag

[PATCH] ahci_sunxi: Drop AHCI_HFLAG_NO_PMP flag

[Hans de Goede]

This is something which we inherited from the Allwinner android kernel sources,
and I've always wanted to test if this is really necessary.

So recently I've bought a sata port multiplexer, and I've given this a test
spin on both A10 and A20 devices, and it seems to work fine:

[    2.154456] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[    2.161092] ata1.15: Port Multiplier 1.2, 0x197b:0x0325 r0, 5 ports, feat 0x5/0xf
[    2.175511] ata1.00: hard resetting link
[    2.524929] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
[    2.531430] ata1.01: hard resetting link
[    2.974465] ata1.01: link resume succeeded after 1 retries
[    3.094932] ata1.01: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[    3.101431] ata1.02: hard resetting link
[    4.174466] ata1.02: failed to resume link (SControl 0)
[    4.180065] ata1.02: SATA link down (SStatus 0 SControl 0)
(and the same for links 3 and 4)

Once the NO_PMP flag is removed it correctly sees the 2 disks which I've
attached, and I can mount and use them just fine, so lets drop the flag.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_sunxi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_sunxi.c b/drivers/ata/ahci_sunxi.c
index e44d675..3dd0224 100644
--- a/drivers/ata/ahci_sunxi.c
+++ b/drivers/ata/ahci_sunxi.c
@@ -184,7 +184,7 @@ static int ahci_sunxi_probe(struct platform_device *pdev)
 		goto disable_resources;
 
 	hpriv->flags = AHCI_HFLAG_32BIT_ONLY | AHCI_HFLAG_NO_MSI |
-		       AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ;
+		       AHCI_HFLAG_YES_NCQ;
 
 	rc = ahci_platform_init_host(pdev, hpriv, &ahci_sunxi_port_info);
 	if (rc)
-- 
2.1.0

[PATCH] ahci_sunxi: Drop AHCI_HFLAG_NO_PMP flag

[Tejun Heo]

On Thu, Nov 13, 2014 at 11:24:16AM +0100, Hans de Goede wrote:
> This is something which we inherited from the Allwinner android kernel sources,
> and I've always wanted to test if this is really necessary.
> 
> So recently I've bought a sata port multiplexer, and I've given this a test
> spin on both A10 and A20 devices, and it seems to work fine:
> 
> [    2.154456] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> [    2.161092] ata1.15: Port Multiplier 1.2, 0x197b:0x0325 r0, 5 ports, feat 0x5/0xf
> [    2.175511] ata1.00: hard resetting link
> [    2.524929] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
> [    2.531430] ata1.01: hard resetting link
> [    2.974465] ata1.01: link resume succeeded after 1 retries
> [    3.094932] ata1.01: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> [    3.101431] ata1.02: hard resetting link
> [    4.174466] ata1.02: failed to resume link (SControl 0)
> [    4.180065] ata1.02: SATA link down (SStatus 0 SControl 0)
> (and the same for links 3 and 4)
> 
> Once the NO_PMP flag is removed it correctly sees the 2 disks which I've
> attached, and I can mount and use them just fine, so lets drop the flag.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to libata/for-3.18-fixes.

Thanks.

-- 
tejun

[PATCH] ahci_sunxi: Drop AHCI_HFLAG_NO_PMP flag

[Hans de Goede]

Hi,

On 11/14/2014 12:56 PM, Hans de Goede wrote:
> Hi,
> 
> On Thursday, November 13, 2014 11:29:56 PM UTC+1, Tejun Heo wrote:
>>
>> On Thu, Nov 13, 2014 at 11:24:16AM +0100, Hans de Goede wrote: 
>>> This is something which we inherited from the Allwinner android kernel 
>> sources, 
>>> and I've always wanted to test if this is really necessary. 
>>>
>>> So recently I've bought a sata port multiplexer, and I've given this a 
>> test 
>>> spin on both A10 and A20 devices, and it seems to work fine: 
>>>
>>> [    2.154456] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) 
>>> [    2.161092] ata1.15: Port Multiplier 1.2, 0x197b:0x0325 r0, 5 ports, 
>> feat 0x5/0xf 
>>> [    2.175511] ata1.00: hard resetting link 
>>> [    2.524929] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 320) 
>>> [    2.531430] ata1.01: hard resetting link 
>>> [    2.974465] ata1.01: link resume succeeded after 1 retries 
>>> [    3.094932] ata1.01: SATA link up 3.0 Gbps (SStatus 123 SControl 300) 
>>> [    3.101431] ata1.02: hard resetting link 
>>> [    4.174466] ata1.02: failed to resume link (SControl 0) 
>>> [    4.180065] ata1.02: SATA link down (SStatus 0 SControl 0) 
>>> (and the same for links 3 and 4) 
>>>
>>> Once the NO_PMP flag is removed it correctly sees the 2 disks which I've 
>>> attached, and I can mount and use them just fine, so lets drop the flag. 
>>>
>>> Signed-off-by: Hans de Goede <hdeg...@redhat.com <javascript:>> 
>>
>> Applied to libata/for-3.18-fixes. 
>>
> 
> I just found out why Allwinner added that AHCI_HFLAG_NO_PMP flag, after 
> putting the system I used to test the port multiplexer back together this 
> morning, it seems that probing for a port multiplexer being present breaks 
> the no-port-multiplexer (so direct attached disk) case :(  I guess I should 
> have tested for that before sending the patch, but the thought of it 
> breaking normal usage did not cross my mind.
> 
> So self NACK. If it is not too late, can you please drop this from your 
> tree ?
> 
> Let me know if you want me to send a revert instead.
> 
> I'll look into fixing this another way, maybe with a quirk + an extra reset 
> after failed pmp testing, and it that does not work a module option.

Ok, so the problem seems to be that the controller fails to do a soft-reset
when attached directly to a normal device rather then to a pmp.

If I comment out this block of code in libata-core.c:

        if (sata_pmp_supported(link->ap) && ata_is_host_link(link)) {
                /* If PMP is supported, we have to do follow-up SRST.
                 * Some PMPs don't send D2H Reg FIS after hardreset if
                 * the first port is empty.  Wait only for
                 * ATA_TMOUT_PMP_SRST_WAIT.
                 */
                if (check_ready) {
                        unsigned long pmp_deadline;

                        pmp_deadline = ata_deadline(jiffies,
                                                    ATA_TMOUT_PMP_SRST_WAIT);
                        if (time_after(pmp_deadline, deadline))
                                pmp_deadline = deadline;
                        ata_wait_ready(link, pmp_deadline, check_ready);
                }
                rc = -EAGAIN;
                goto out;
        }

Then things work again. This code looks like it is a must have for
pmp use, so I'll do a follow up patch to my initial patch dropping the
AHCI_HFLAG_NO_PMP flag, which re-adds it based on a module option
(defaulting to the non-pmp case).

This way you don't have to do a forced-push to your tree to drop the
original patch (or revert it).

Regards,

Hans

[PATCH] ahci_sunxi: Drop AHCI_HFLAG_NO_PMP flag

[Tejun Heo]

On Fri, Nov 14, 2014 at 01:49:04PM +0100, Hans de Goede wrote:
> > I just found out why Allwinner added that AHCI_HFLAG_NO_PMP flag, after 
> > putting the system I used to test the port multiplexer back together this 
> > morning, it seems that probing for a port multiplexer being present breaks 
> > the no-port-multiplexer (so direct attached disk) case :(  I guess I should 
> > have tested for that before sending the patch, but the thought of it 
> > breaking normal usage did not cross my mind.
> > 
> > So self NACK. If it is not too late, can you please drop this from your 
> > tree ?

I popped the commit from libata/for-3.18-fixes.

Thanks.

-- 
tejun

[PATCH] ahci_sunxi: Drop AHCI_HFLAG_NO_PMP flag

[Hans de Goede]

Hi,

On 11/14/2014 01:49 PM, Hans de Goede wrote:
> Hi,
> 
> On 11/14/2014 12:56 PM, Hans de Goede wrote:
>> Hi,
>>
>> On Thursday, November 13, 2014 11:29:56 PM UTC+1, Tejun Heo wrote:
>>>
>>> On Thu, Nov 13, 2014 at 11:24:16AM +0100, Hans de Goede wrote: 
>>>> This is something which we inherited from the Allwinner android kernel 
>>> sources, 
>>>> and I've always wanted to test if this is really necessary. 
>>>>
>>>> So recently I've bought a sata port multiplexer, and I've given this a 
>>> test 
>>>> spin on both A10 and A20 devices, and it seems to work fine: 
>>>>
>>>> [    2.154456] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) 
>>>> [    2.161092] ata1.15: Port Multiplier 1.2, 0x197b:0x0325 r0, 5 ports, 
>>> feat 0x5/0xf 
>>>> [    2.175511] ata1.00: hard resetting link 
>>>> [    2.524929] ata1.00: SATA link up 3.0 Gbps (SStatus 123 SControl 320) 
>>>> [    2.531430] ata1.01: hard resetting link 
>>>> [    2.974465] ata1.01: link resume succeeded after 1 retries 
>>>> [    3.094932] ata1.01: SATA link up 3.0 Gbps (SStatus 123 SControl 300) 
>>>> [    3.101431] ata1.02: hard resetting link 
>>>> [    4.174466] ata1.02: failed to resume link (SControl 0) 
>>>> [    4.180065] ata1.02: SATA link down (SStatus 0 SControl 0) 
>>>> (and the same for links 3 and 4) 
>>>>
>>>> Once the NO_PMP flag is removed it correctly sees the 2 disks which I've 
>>>> attached, and I can mount and use them just fine, so lets drop the flag. 
>>>>
>>>> Signed-off-by: Hans de Goede <hdeg...@redhat.com <javascript:>> 
>>>
>>> Applied to libata/for-3.18-fixes. 
>>>
>>
>> I just found out why Allwinner added that AHCI_HFLAG_NO_PMP flag, after 
>> putting the system I used to test the port multiplexer back together this 
>> morning, it seems that probing for a port multiplexer being present breaks 
>> the no-port-multiplexer (so direct attached disk) case :(  I guess I should 
>> have tested for that before sending the patch, but the thought of it 
>> breaking normal usage did not cross my mind.
>>
>> So self NACK. If it is not too late, can you please drop this from your 
>> tree ?
>>
>> Let me know if you want me to send a revert instead.
>>
>> I'll look into fixing this another way, maybe with a quirk + an extra reset 
>> after failed pmp testing, and it that does not work a module option.
> 
> Ok, so the problem seems to be that the controller fails to do a soft-reset
> when attached directly to a normal device rather then to a pmp.
> 
> If I comment out this block of code in libata-core.c:
> 
>         if (sata_pmp_supported(link->ap) && ata_is_host_link(link)) {
>                 /* If PMP is supported, we have to do follow-up SRST.
>                  * Some PMPs don't send D2H Reg FIS after hardreset if
>                  * the first port is empty.  Wait only for
>                  * ATA_TMOUT_PMP_SRST_WAIT.
>                  */
>                 if (check_ready) {
>                         unsigned long pmp_deadline;
> 
>                         pmp_deadline = ata_deadline(jiffies,
>                                                     ATA_TMOUT_PMP_SRST_WAIT);
>                         if (time_after(pmp_deadline, deadline))
>                                 pmp_deadline = deadline;
>                         ata_wait_ready(link, pmp_deadline, check_ready);
>                 }
>                 rc = -EAGAIN;
>                 goto out;
>         }
> 
> Then things work again. This code looks like it is a must have for
> pmp use, so I'll do a follow up patch to my initial patch dropping the
> AHCI_HFLAG_NO_PMP flag, which re-adds it based on a module option
> (defaulting to the non-pmp case).
> 
> This way you don't have to do a forced-push to your tree to drop the
> original patch (or revert it).

Just saw your message that you've dropped the troublesome patch, in that
case I'll do a completely new patch, rather then a follow on patch, to
add a module option for this (unless you've a better idea).

Regards,

Hans