Driver for i.MX6 AHCI controller

[rogiel_rogiel.com]

Summary

This patch, based on the NetBSD implementation, implements the AHCI/SATA controller for the i.MX6 SoC.

I have tested this driver on the current Wandboard Quad and it has been working fine for both read and write operations.

$ dmesg | grep ahci
ahci0: <i.MX6 Integrated AHCI controller> mem 0x2200000-0x2203fff irq 14 on simplebus0
ahci0: AHCI v1.30 with 1 3Gbps ports, Port Multiplier supported
ahcich0: <AHCI channel> at channel 0 on ahci0
ada0 at ahcich0 bus 0 scbus0 target 0 lun 0

$ dmesg | grep ada0
ada0: <Hitachi HTS545050B9A300 PB4OC60F> ATA8-ACS SATA 2.x device
ada0: Serial Number 110303PBN403M7FN7JBE
ada0: 300.000MB/s transfers (SATA 2.x, UDMA6, PIO 8192bytes)
ada0: Command Queueing enabled
ada0: 476940MB (976773168 512 byte sectors)

$ dd if=/dev/zero of=/dev/ada0 bs=1m count=1024
1024+0 records in
1024+0 records out
1073741824 bytes transferred in 13.720023 secs (78260935 bytes/sec)

[rogiel_rogiel.com]

Added more diff context

[• ian]

I get a runtime KASSERT:

ahci0: <i.MX6 Integrated AHCI controller> mem 0x2200000-0x2203fff irq 14 on simplebus0
panic: imx_iomux_gpr_get bad regnum 52, max 13

This is because imx_iomux_gpr_get() wants the simple register number (0-13), not the IOMUX_GPR13 constant which is the offset of the register in mmio space. (Yet another reason why I've come to hate the existance of fooreg.h files; all that stuff is private to the corresponding foo.c file, and should just be defined in there, imo).

After fixing that in the code I've now got the driver running on a wandboard and it seems to be working great. Thanks for submitting this, it can be committed as soon as we tidy up the minor style stuff.

sys/arm/freescale/imx/imx6_ahci.c

49 ↗

(On Diff #29546)

style(9) says tab after #define.

A lot of the imx stuff uses a style where a main register name is

#define<tab>REGNAME<tab>0xnnnn

and then bitfields within that register are

#define<tab><space><space>REGNAME_BITNAME<tab><space><space>(1u << something)

I'm not sure who started that style but it's pretty common in arm stuff, especially freescale.

74 ↗

(On Diff #29546)

style(9) requires a space between if/while/for and the opening paren.

83 ↗

(On Diff #29546)

If 'on' is a bool, just define it that way in the prototype and pass a proper true/false from the caller, the !! thing is ugly and hard to read.

100 ↗

(On Diff #29546)

style(9) requires a blank line between local decls and the first line of code.

106 ↗

(On Diff #29546)

Usually only informative messages are conditional on bootverbose; errors should probably always print, unless it's more of a warning that doesn't prevent proper operation.

131 ↗

(On Diff #29546)

The code to execute in an if or else block has to start on the line after the if/else.

205 ↗

(On Diff #29546)

Need to check whether the device has been disabled before checking the compat. Like:

	if (!ofw_bus_status_okay(dev))
		return (ENXIO);

220 ↗

(On Diff #29546)

style(9) likes to see all the int/any-same-type vars stacked up on one line, in alphabetical order:

int error, pllstat, timeout;

(For the record, I *hate* that rule, I much prefer 1-per-line.)

240 ↗

(On Diff #29546)

More style(9)...

Don't use boolean tests on pointers, explicitly compare ==/!= NULL. Continuation indents are always 4 spaces, whether that visually makes any sense or not. That all leads to:

if ((ctlr->r_mem = bus_alloc_resource_any(dev, SYS_RES_MEMORY,
    &ctlr->r_rid, RF_ACTIVE)) != NULL) {

254 ↗

(On Diff #29546)

Technically, style(9) requires that these lines all be filled out to as close to 80 chars as possible. This is one of the places where I draw the line and say "screw the style rules, readability counts more". I would organize them like this:

v &= ~(
    IOMUX_GPR13_SATA_PHY_8(7)    |
    IOMUX_GPR13_SATA_PHY_7(0x1f) |
​    IOMUX_GPR13_SATA_PHY_6(7)    |
​    IOMUX_GPR13_SATA_SPEED(1)    |
​    IOMUX_GPR13_SATA_PHY_5(1)    |
​    IOMUX_GPR13_SATA_PHY_4(7)    |
​    IOMUX_GPR13_SATA_PHY_3(0xf)  |
​    IOMUX_GPR13_SATA_PHY_2(0x1f) |
​    IOMUX_GPR13_SATA_PHY_1(1)    |
​    IOMUX_GPR13_SATA_PHY_0(1));

Basically, with a 4-char indent and then some other violations that make it readable.

284 ↗

(On Diff #29546)

It seems strange that pllstat is checked for both < 0 and for something bitmapped.

309 ↗

(On Diff #29546)

Branch labels go at the left margin (is there anything style(9) doesn't cover? believe it or not, yes).

Also, the return value below needs parens:

return (error);

317 ↗

(On Diff #29546)

style(9) (learning to hate phrase yet?) requires a blank line when there are no local var decls (dumb rule).

335 ↗

(On Diff #29546)

It's common, but not mandatory, to align the 2nd arg of all the DEVMETHOD lines with spaces after the comma, for readability.

sys/arm/freescale/imx/imx6_ccm.c

318 ↗

(On Diff #29546)

I get build error here because the new imx6_ccmvar.h isn't included in imx6_ccm.c.

I think I would be inclined to put the new prototype into the existing imx_ccm.h file instead of creating a whole new file for it; the 'imx6' in the name makes it clear where to find the implementation code. The imx6 ccm stuff is all bogus anyway, I've just never had the time to write a real ccm driver.

sys/arm/freescale/imx/imx6_ccmreg.h

132 ↗

(On Diff #29546)

When something is left-shifted by 31 bits it needs to be unsigned to avoid warnings from compiler and static analyzers these days. I've gotten in the habbit of just always using '1u << ' lately.

sys/arm/freescale/imx/std.imx6

13 ↗

(On Diff #29546)

This should be in arm/conf/IMX6.

The devices that go into a std.<platform> file are ones that are so mandatory that the platform can't boot without them, but they can't be listed as 'standard' in the files.<platform> because they're shared by several platforms and their definitions have to be in sys/conf/files.

[rogiel_rogiel.com]

In D11177#233055, @ian wrote:

I get a runtime KASSERT:

ahci0: <i.MX6 Integrated AHCI controller> mem 0x2200000-0x2203fff irq 14 on simplebus0
panic: imx_iomux_gpr_get bad regnum 52, max 13

This is because imx_iomux_gpr_get() wants the simple register number (0-13), not the IOMUX_GPR13 constant which is the offset of the register in mmio space. (Yet another reason why I've come to hate the existance of fooreg.h files; all that stuff is private to the corresponding foo.c file, and should just be defined in there, imo).

After fixing that in the code I've now got the driver running on a wandboard and it seems to be working great. Thanks for submitting this, it can be committed as soon as we tidy up the minor style stuff.

Sorry for taking this long to reply. I was out without a WB around so couldn't test any changes made.

Well... i didn't caught that kernel panic because I was building on 11 without asserts. I find interesting that it worked even without the configuration, maybe u-boot already did some magic for me? This should be a panic in any situation though, not only with assertions.

As soon as I can test on the WB I will make sure no panics happen and I will update with the newest patch.

sys/arm/freescale/imx/imx6_ahci.c

220 ↗

(On Diff #29546)

Yeah.. that rules sounds pretty stupid. I "fixed" it according to the rules, but I would rather keep it as is, unless this is a problem.

284 ↗

(On Diff #29546)

I believe I wanted to return error codes as negative integer, but that is not how the function was implemented at all and that was a bad solution anyways.

I think using a pointer here would be the right way.