Page MenuHomeFreeBSD

Introduce Annapurna Labs AHCI support
ClosedPublic

Authored by zbb on Mar 6 2015, 4:49 PM.
Tags
None
Referenced Files
F105044228: D2018.diff
Wed, Dec 11, 9:43 PM
Unknown Object (File)
Thu, Nov 28, 7:00 AM
Unknown Object (File)
Thu, Nov 28, 7:00 AM
Unknown Object (File)
Thu, Nov 28, 7:00 AM
Unknown Object (File)
Thu, Nov 28, 6:37 AM
Unknown Object (File)
Tue, Nov 26, 8:36 AM
Unknown Object (File)
Oct 28 2024, 9:06 AM
Unknown Object (File)
Oct 17 2024, 9:52 PM
Subscribers

Details

Reviewers
imp
mav
smh
Summary
Overview:    
* implemented quirk for forcing SATA interface enable
* restore value to status register - this enables link autonegotiation

Modifications:
* devid:vendorid field
* quirk for forcing PI setting (BIOS is doing that on PC-like systems)
* write to capabilites field to enable phy link initialization

Submitted by: Wojciech Macek <wma@semihalf.com>
Obtained from: Semihalf

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

zbb retitled this revision from to Introduce Annapurna Labs AHCI support.
zbb updated this object.
zbb edited the test plan for this revision. (Show Details)
zbb added reviewers: mav, imp, smh.
zbb added a subscriber: Unknown Object (MLST).
sys/dev/ahci/ahci.c
150–153

Playing devil's advocate: why not do this always? Or, why do we need to do it at all? Is this an erratum for the Annapurna parts, or is how the Annapurna parts being used different so we only see it there?

199–201

Why don't other cards need this? What makes Annapurna cards special? Is this something the spec says we should be doing, but aren't? Is there some other way to know than a big giant table this is required?

sys/dev/ahci/ahci.c
150–153

Jakub Palider wrote:

Yes, actually this is how things are expected to look like in general, I
mean restore caps. But we have no way to check it on other platforms at
the moment. If you think this does not mess up other configurations it
would be fine to perform the operation on a normal basis.

199–201

Jakub Palider wrote:

This is something bootloaders may set up or not. This way we run
independently and make SATAs operational. This also allows to fiddle
around if someone feels like experimenting...

sys/dev/ahci/ahci.c
150–153

AHCI specification defines this register as read-only. It tells nothing about "restoring" this register. I don't like idea of writing to read-only registers.

199–201

According to specification, this is something that platform/BIOS must set. I know that Linux sets it always, but I believe that this is spec violation. If there is some BROKEN hardware that require it to be set by the driver, I can accept hardware-specific quirk or some tunable, but not global.

sys/dev/ahci/ahci.c
150–153

Jakub Palider wrote:

I have overlooked it in the specs and took the CAP register RW for
granted. You are right, writing to RO registers doesn't sound fine, and
we don't like it either. But we have little choice but to perform the
writes to make it operational.
You say adding it to quirks is not proper place? Where then?

199–201

Jakub Palider wrote:

I'd prefer read MAY/SHOULD/MUST but the spec simply says that it is
loaded by the BIOS. However in reality that does not always happen and
higher level stuff suffers from broken implementations. Linux does it
and although it is not our reference it shows a way to just make things
work. Again - is adding platform quirks wrong way?

sys/dev/ahci/ahci.c
199–201

I haven't told that something is wrong with the quirk. It is fine to me, if it is really needed, that is I am not sure about.

So what do we do with this patch?

sys/dev/ahci/ahci.c
150–153

We should document this then. Add a comment that states that some platforms, specifically BLAH, require writing to this read-only register so a quirk is provided to update them and prevent writing to standard conforming devices. This way we know we can eliminate it if it turns out that the current root-cause analysis proves to be inaccurate or better worked around.

199–201

I'd add a comment here as well so we know why we do this weird, odd-ball thing. It will also help reduce the temptation to cut and paste it for devices that clearly don't need it.

imp edited edge metadata.

Thanks for the comments. Still not sure what an Annapurna is....

This revision is now accepted and ready to land.Mar 14 2015, 1:29 AM
mav edited edge metadata.