On 8/30/19 12:00 PM, Dan Williams wrote:
> On Fri, Aug 30, 2019 at 8:47 AM Stephen Douthit
> <steph...@silicom-usa.com> wrote:
>>
>> On 8/29/19 7:30 PM, Dan Williams wrote:
>>> The Linux ahci driver has historically implemented a configuration fixup
>>> for platforms / platform-firmware that fails to enable the ports prior
>>> to OS hand-off at boot. The fixup was originally implemented way back
>>> before ahci moved from drivers/scsi/ to drivers/ata/, and was updated in
>>> 2007 via commit 49f290903935 "ahci: update PCS programming". The quirk
>>> sets a port-enable bitmap in the PCS register at offset 0x92.
>>>
>>> This quirk could be applied generically up until the arrival of the
>>> Denverton (DNV) platform. The DNV AHCI controller architecture supports
>>> more than 6 ports and along with that the PCS register location and
>>> format were updated to allow for more possible ports in the bitmap. DNV
>>> AHCI expands the register to 32-bits and moves it to offset 0x94.
>>>
>>> As it stands there are no known problem reports with existing Linux
>>> trying to set bits at offset 0x92 which indicates that the quirk is not
>>> applicable. Likely it is not applicable on a wider range of platforms,
>>> but it is difficult to discern which platforms if any still depend on
>>> the quirk.
>>>
>>> Rather than try to fix the PCS quirk to consider the DNV register layout
>>> instead require explicit opt-in. The assumption is that the OS driver
>>> need not touch this register, and platforms can be added with a new
>>> boad_ahci_pcs7 board-id when / if problematic platforms are found in the
>>> future. The logic in ahci_intel_pcs_quirk() looks for all Intel AHCI
>>> instances with "legacy" board-ids and otherwise skips the quirk if the
>>> board was matched by class-code.
>>>
>>> Reported-by: Stephen Douthit <steph...@silicom-usa.com>
>>> Cc: Christoph Hellwig <h...@infradead.org>
>>> Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
>>> ---
>>> Changes since v2:
>>> - Use board_ahci_pcs7 to opt Denverton out of the PCS quirk.
>>>
>>> drivers/ata/ahci.c | 116
>>> +++++++++++++++++++++++++++++++---------------------
>>> drivers/ata/ahci.h | 2 +
>>> 2 files changed, 71 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index f7652baa6337..3e63294304c7 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -65,6 +65,12 @@ enum board_ids {
>>> board_ahci_sb700, /* for SB700 and SB800 */
>>> board_ahci_vt8251,
>>>
>>> + /*
>>> + * board IDs for Intel chipsets that support more than 6 ports
>>> + * *and* end up needing the PCS quirk.
>>> + */
>>> + board_ahci_pcs7,
>>> +
>>> /* aliases */
>>> board_ahci_mcp_linux = board_ahci_mcp65,
>>> board_ahci_mcp67 = board_ahci_mcp65,
>>> @@ -220,6 +226,12 @@ static const struct ata_port_info ahci_port_info[] = {
>>> .udma_mask = ATA_UDMA6,
>>> .port_ops = &ahci_vt8251_ops,
>>> },
>>> + [board_ahci_pcs7] = {
>>> + .flags = AHCI_FLAG_COMMON,
>>> + .pio_mask = ATA_PIO4,
>>> + .udma_mask = ATA_UDMA6,
>>> + .port_ops = &ahci_ops,
>>> + },
>>> };
>>>
>>> static const struct pci_device_id ahci_pci_tbl[] = {
>>> @@ -264,26 +276,26 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>> { PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
>>> { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_mobile }, /* PCH M RAID */
>>> { PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b0), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b1), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b2), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b3), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b4), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b5), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b6), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19b7), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19bE), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19bF), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c0), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c1), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c2), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c3), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c4), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c5), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c6), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19c7), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19cE), board_ahci }, /* DNV AHCI */
>>> - { PCI_VDEVICE(INTEL, 0x19cF), board_ahci }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b0), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b1), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b2), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b3), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b4), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b5), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b6), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19b7), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19bE), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19bF), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c0), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c1), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c2), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c3), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c4), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c5), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c6), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19c7), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19cE), board_ahci_pcs7 }, /* DNV AHCI */
>>> + { PCI_VDEVICE(INTEL, 0x19cF), board_ahci_pcs7 }, /* DNV AHCI */
>>
>> I think you want to invert this scheme and mark the old controllers as
>> board_ahci_pcs6 and leave Denverton/newer controllers as board_ahci.
>>
>> Otherwise the quirk below runs for any Intel controllers that matched
>> based on the AHCI class code, since their board_id will be board_ahci
>> which is < board_ahci_pcs7.
>
> It shouldn't apply for the class code because the id->vendor in that
> case will be PCI_ANY_ID not PCI_VENDOR_ID_INTEL.
Got it. I just assumed (incorrectly) the vendor ID was from the device
config space and not the match table.
In that case:
Reviewed-by: Stephen Douthit <steph...@silicom-usa.com>
