View Change

12 comments:

• File src/include/ahci.h:

  • Patch Set #2, Line 22: #define AHCI_CAP_NP 0x1f

    please name it _MASK or something similar

  • Patch Set #2, Line 40:

    odd whitespace

  • Patch Set #2, Line 48: #define AHCI_VS_MAJOR(x) ((x >> 16) & 0xf)

    ((((x) >> 24) & 0xf) * 10 + (((x) >> 16) & 0xf))

  • Patch Set #2, Line 49: #define AHCI_VS_MINOR(x) ((x >> 8) & 0xf)

    ((x) & 0xf ? ((((x) >> 8) & 0xf) * 10 + ((x) & 0xf)) : (((x) >> 8) & 0xf))

    At least this seems how they write it, no trailing 0. OTOH, this makes
    it harder for comparisons, so maybe we should just print 1.10 as the
    hardware reports instead of 1.1.

    (Intel seems to love to make it confusing. I guess 0.95 should have been
    0.9.5, then their counting would make sense.)

• File src/southbridge/intel/common/ahci.h:

  • Patch Set #2, Line 27: const u32 clear_vnd);

    Qualifications of the parameter type like the `const` here are meaningless in
    an interface. For the caller, it's just noise.
    (Exception are pointers to qualified types ofc., where the qualification
    becomes part of the pointer type.)

• File src/southbridge/intel/common/ahci.c:

  • Patch Set #2, Line 55: * Spec: "No other ABAR registers should be accessed before this."

    I can't find this quote?

    Spec talks about software and I'm sure it doesn't apply to Intel firmware,
    rather AHCI drivers. It also says that AE depends on SAM which is only
    configured later.

  • Patch Set #2, Line 67: BIOS_DEBUG

    I think BIOS_INFO would suit most of the messages better.

  • Patch Set #2, Line 73: ports

    + 1

  • Patch Set #2, Line 75: : enable power management

    it's more than that

  • Patch Set #2, Line 79: if (cccs && major >= 1 && minor >= 1)

    || major > 1

    Versions are easier to compare in an encoded form. For instance with

        #define AHCI_VERSION (mj, mn) ((mj) << 16 | (mn)))

    this could become

        if (read32(abar + AHCI_VS) >= AHCI_VERSION(1, 10))

  • Patch Set #2, Line 80: reg32 |= AHCI_CAP_CCCS;

    Note, this wasn't explicitly set for bd82x6x.

  • Patch Set #2, Line 102: if (major >= 1 && minor >= 2) {

    Hmmm, it seems ICH9 advertises 1.20, but doesn't document this register.

To view, visit change 37137. To unsubscribe, or for help writing mail filters, visit settings.