Page MenuHomeFreeBSD

Enable optional soft reset in AHCI
ClosedPublic

Authored by mw_semihalf.com on Jan 17 2017, 4:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 9:33 PM
Unknown Object (File)
Sun, Mar 3, 7:31 AM
Unknown Object (File)
Sun, Mar 3, 7:08 AM
Unknown Object (File)
Jan 2 2024, 6:07 PM
Unknown Object (File)
Dec 20 2023, 12:48 AM
Unknown Object (File)
Dec 13 2023, 6:17 AM
Unknown Object (File)
Dec 7 2023, 10:53 AM
Unknown Object (File)
Nov 13 2023, 7:33 AM

Details

Summary

It occurred that some Marvell integrated controllers
require additional time after soft reset to work properly.
Introduce new quirk (AHCI_Q_MRVL_SR_DEL), that enable
such operation.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mw_semihalf.com retitled this revision from to Enable optional soft reset in AHCI.
mw_semihalf.com updated this object.
mw_semihalf.com edited the test plan for this revision. (Show Details)
mw_semihalf.com added reviewers: imp, ian, zbb, fabient.
mw_semihalf.com set the repository for this revision to rS FreeBSD src repository - subversion.
mw_semihalf.com added a subscriber: ARM.
zbb requested changes to this revision.EditedJan 17 2017, 6:52 PM
zbb edited edge metadata.

I think we should change this commit after all. I would prefer not to see soft_reset_del uninitialized for other drivers than Armada AHCI even though it is a part of ahci_controller structure. The better solution would be to create another quirk and activate it in the Armada AHCI wrapper according to the FDT property. We can then hard code the delay value in the generic AHCI driver and call it when the quirk is active.

This revision now requires changes to proceed.Jan 17 2017, 6:52 PM

Is there any explanation from vendor why is this delay needed? The idea of fixed delay looks odd to me.

Just checked in the documentation and it's not vendor specific but a generic AHCI/SATA requirement. "Serial ATA Revision 2.6" in section 13.1.1 "Software Reset" (it was pointed in AHCI Spec rev.1.3.1 in section 10.4.1 "Software Reset") states:

"host software is required to toggle the SRST bit no faster than 5 us"

Moreover the linux drivers/ata/libahci.c in according place issues even longer delay with following comment:

/* spec says at least 5us, but be generous and sleep for 1ms */

It's just a guess, but the issue might have remained unnoticed, because until now only AHCI over PCI controllers were supported and somehow 5us constraint between commands was met (e.g. Marvell 92xx AHCI controller with exactly same silicon inside works without the modification in question). The problem appeared once AHCI attached to internal bus support was added for Armada 38x.

Anyway, do you have any objections to add uncoditional delay (how big?) when toggling AHCI reset?

I know those quotes you've shown here. But if you look on existing code, DELAY(10) is already executed at least once after submitting each part of soft reset (set/unset).

Not that I radically against this change, but I still think that either you are doing something wrong, or this hardware behaves wrong.

I of course noticed DELAY(10), but it seems to me that it's part of the polling sequence, similar to what is done in ahci_exec_polled_cmd() (called by ahci_do_softreset()), isn't it? 1ms delay is done afterwards. I'm mentioning linux driver here, because the Armada38x AHCI is in fact quirkless according to the spec and works with pure linux driver - this is why I'm a bit confused by problems here.

In FreeBSD we also do not anything special, but simply allocate platform resources and initialize AHCI with generic commands, similar to what is done for PCI AHCI drivers. Please see: https://reviews.freebsd.org/D9222 - do you notice anything suspicious?

In case we don't find a generic solution, would quirk approach, proposed by zbb be acceptable?

I of course noticed DELAY(10), but it seems to me that it's part of the polling sequence, similar to what is done in ahci_exec_polled_cmd() (called by ahci_do_softreset()), isn't it?

Soft reset is always polled in AHCI, because device does not generate interrupts for it. When soft reset bit is set, HBA just ignores all statuses from device, since it is not sane yet. And that is why that 5us delay is required by specs. When soft reset bit is cleared, device sends regular status update, clearing busy status, which HBA/driver wait in semi-regular way.

1ms delay is done afterwards. I'm mentioning linux driver here, because the Armada38x AHCI is in fact quirkless according to the spec and works with pure linux driver - this is why I'm a bit confused by problems here.

Since you have mentioned Marvell here, I must ask you whether you looked on AHCI_Q_NOBSYRES quirk already present in the driver for some Marvell chips? On my experiments I found that some Marvell chips don't wait for device respond on soft reset, clearing status immediately. I believe that is a bug, while in Linux it may be workarounded by mentioned 1ms delay. Please try to set that quirk and see whether it help.

Thanks, I noticed it, will test the mentioned quirk and let know.

I've tested AHCI_Q_NOBSYRES quirk, it's not working for this Marvell's controller:
ahcich0: Timeout on slot 15 port 0
ahcich0: is 00000000 cs 00000000 ss 00000000 rs 00008000 tfd 150 serr 00000000 cmd 00718f17
ahcich0: AHCI reset...
ahcich0: SATA connect time=300us status=00000133
ahcich0: AHCI reset: device found
ahcich0: AHCI reset: device ready after 0ms
(aprobe0:ahcich0:0:0:0): ATA_IDENTIFY. ACB: ec 00 00 00 00 40 00 00 00 00 00 00
(aprobe0:ahcich0:0:0:0): CAM status: Command timeout
(aprobe0:ahcich0:0:0:0): Retrying command
panic: run_interrupt_driven_config_hooks: waited too long

mw_semihalf.com updated this object.
mw_semihalf.com edited edge metadata.
mw_semihalf.com removed rS FreeBSD src repository - subversion as the repository for this revision.

Add new quirk instead of optional field in struct ahci_channel.

mav edited edge metadata.

I have no big objections. But does it really needs 50ms delay if you say Linux does just 1ms?

This revision was automatically updated to reflect the committed changes.