AHCI: Fix AHCI driver for ARM platform.

[mmel]

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.

[mav]

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]

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

[andrew]

sys/dev/ahci/ahci.c

486 ↗

(On Diff #10397)

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]

sys/dev/ahci/ahci.c

486 ↗

(On Diff #10397)

Also the correct macro for arm64 is __aarch64__.

[mmel]

Use proper macro for ARM64 platform.

[mmel]

sys/dev/ahci/ahci.c

486 ↗

(On Diff #10399)

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]

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

[andrew]

sys/dev/ahci/ahci.c

486 ↗

(On Diff #10399)

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]

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]

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]

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]

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?