Page MenuHomeFreeBSD
Differential D4240

AHCI: Fix AHCI driver for ARM platform.
ClosedPublic
Actions

Authored by mmel on Nov 21 2015, 11:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 12, 3:19 AM
Unknown Object (File)
Fri, Nov 29, 7:59 PM
Unknown Object (File)
Fri, Nov 29, 12:43 PM
Unknown Object (File)
Sun, Nov 24, 1:17 AM
Unknown Object (File)
Nov 21 2024, 12:59 AM
Unknown Object (File)
Nov 14 2024, 8:32 PM
Unknown Object (File)
Oct 30 2024, 11:45 PM
Unknown Object (File)
Oct 16 2024, 8:29 PM
View All Files
Subscribers
andrew
imp
mmoll
ARM
arm64

Details

Reviewers
kib
mav
Commits
rS291444: AHCI: Fix AHCI driver for ARM.
rS291427: AHCI: Use bus_dmamap_sync(9) when accessing DMA buffers.
Summary
  • use bus_dmamap_sync(9) when accessing DMA buffers.
  • use readback after write to interrupt status register.

On ARM, writes to device registers are bufferable (not combining, uncacheable). Interrupt controller is typically tightly coupled to CPU complex, while devices control registers are on busses with high latency. Because of this, we can lost ordering of write that clears interrupt source in device interrupt status register and write that reenable interrupt in interrupt controller. Only readback of device interrupt status register can ensure correct write ordering.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mmel updated this revision to Diff 10396.Nov 21 2015, 11:33 AM
mmel retitled this revision from to AHCI: Fix AHCI driver for ARM platform..
mmel updated this object.
mmel edited the test plan for this revision. (Show Details)
mmel added reviewers: mav, kib.
mmel set the repository for this revision to rS FreeBSD src repository - subversion.
mmel added subscribers: ARM, arm64.
Herald added a subscriber: imp. · View Herald TranscriptNov 21 2015, 11:33 AM
mav edited edge metadata.Nov 21 2015, 11:48 AM

Part about bus_dmamap_sync() seems fine to me.

But I strongly dislike parts about register reread! Device reads are expensive when it goes about high IOPS, and I was trying hard to avoid as many of them as possible. And now you are adding one register read per interrupt just for nothing (from x86 point of view). Please either found other way to enforce proper ordering, or make those reads ARM-specific.

mmel updated this revision to Diff 10397.Nov 21 2015, 12:14 PM
mmel edited edge metadata.

Unfortunately, at this moment, we don't know any other method.

andrew added a subscriber: andrew.Nov 21 2015, 12:25 PM
andrew added inline comments.
sys/dev/ahci/ahci.c
486

Why is this needed on arm64? We use the No Early Write Acknowledgement attribute on device memory. This means a memory barrier will only complete when the endpoint has acknowledged the write.

andrew added inline comments.Nov 21 2015, 12:26 PM
sys/dev/ahci/ahci.c
486

Also the correct macro for arm64 is __aarch64__.

mmel updated this revision to Diff 10399.Nov 21 2015, 12:43 PM

Use proper macro for ARM64 platform.

mmel added inline comments.Nov 21 2015, 12:59 PM
sys/dev/ahci/ahci.c
486

Are you sure that this is true for all crossbar/bus combinations. AHB, for example, doesn't have this functionality (at least older versions, i'm not sure about new version). This is one of reasons why interdevice ordering not works on arm, even device is mapped as SO. I haven't real reason to use readback for ARM64, so i'm ready to remove it for ARM64, if you want it.

mmel added a comment.Nov 21 2015, 1:13 PM

Oki, here is another possibility. Is new quirk (for readback) more acceptable than per arch #ifdef ?

andrew added inline comments.Nov 21 2015, 1:27 PM
sys/dev/ahci/ahci.c
486

It's not guaranteed as the attribute is just a hint, however ARM "strongly recommends that the Early Write Acknowledgement hint is not ignored by a PE, but is made available for use by the system."

They also say "This means that a DSB barrier, executed by the PE that performed the write to the No Early Write Acknowledgement location, completes only after the write has reached its endpoint in the memory system."

mmel added a comment.Nov 21 2015, 2:26 PM

After all this, new quirk seems best for me.
Andrew is right, hardcoded readback (per arch) is probably not optimal solution.
Alexander, is new one "if" in interrupt path acceptable?

mav added a comment.Nov 21 2015, 3:54 PM
In D4240#89411, @meloun-miracle-cz wrote:

Alexander, is new one "if" in interrupt path acceptable?

It is definitely better then read from device. I am just not sure for what devices are you going to add the quirk? What if some random AHCI controller will be inserted into PCIe slot on ARM board? Should it get those reads?

mmoll added a subscriber: mmoll.Nov 22 2015, 1:50 AM

I can at least confirm that with this patch my Banana Pro with a SATA SSD is running rock solid:

ahci0: <Allwinner Integrated AHCI controller> mem 0x1c18000-0x1c18fff irq 88 on simplebus0
ahci0: AHCI v1.10 with 1 3Gbps ports, Port Multiplier not supported
ahci0: quirks=0x2<NOPMP>
ahcich0: <AHCI channel> at channel 0 on ahci0
ada0 at ahcich0 bus 0 scbus0 target 0 lun 0
ada0: <KINGSTON SV300S37A60G 580ABBF0> ATA8-ACS SATA 3.x device
ada0: Serial Number 50026B725209FF66
ada0: 300.000MB/s transfers (SATA 2.x, UDMA6, PIO 512bytes)
ada0: Command Queueing enabled
ada0: 57241MB (117231408 512 byte sectors)
mmel updated this revision to Diff 10421.Nov 22 2015, 9:49 AM

Hmm, this is a good question. At this moment, i have only alc and re running on arm PCIe. None of them generates spurious interrupts. It seems that more research is needed.
Anyway, for now, i want to fix our current issue. Latest version of patch is minimal, without any impact to others platforms.
Its OK for you?
Btw, with this patch, i'm able to get 220-230MB/s for sequential read from SSD on SATA-II port. Its not bad, right?

Closed by commit rS291427: AHCI: Use bus_dmamap_sync(9) when accessing DMA buffers. (authored by mmel). · Explain WhyNov 30 2015, 7:29 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
dev/
ahci/
23 lines

Diff 10421

View Options

sys/dev/ahci/ahci.c

Loading...