[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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

Re: [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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Applied to libata/for-3.18-fixes.

Thanks.

-- 
tejun

Re: [PATCH] ahci_sunxi: Drop AHCI_HFLAG_NO_PMP flag

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]

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...-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org <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.

Thanks & Regards,

Hans

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 3254 bytes --]

Re: [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...-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org <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

Re: [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

Re: [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...-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org <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