Linux-ide Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Douthit <stephend@silicom-usa.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: "axboe@kernel.dk" <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	"linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] libata/ahci: Drop PCS quirk for Denverton and beyond
Date: Fri, 30 Aug 2019 17:05:56 +0000
Message-ID: <9d40bccf-d133-b78e-f6b4-683868e20ddf@silicom-usa.com> (raw)
In-Reply-To: <CAPcyv4h2tNRC0w1mKjnt6tuZrUcUTh+y1YCeQKTb+b8HftEWZg@mail.gmail.com>

On 8/30/19 12:00 PM, Dan Williams wrote:
> On Fri, Aug 30, 2019 at 8:47 AM Stephen Douthit
> <stephend@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 <stephend@silicom-usa.com>
>>> Cc: Christoph Hellwig <hch@infradead.org>
>>> Signed-off-by: Dan Williams <dan.j.williams@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 <stephend@silicom-usa.com>

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-29 23:30 Dan Williams
2019-08-30 15:47 ` Stephen Douthit
2019-08-30 16:00   ` Dan Williams
2019-08-30 17:05     ` Stephen Douthit [this message]
2019-08-31  2:54 ` Jens Axboe
2019-09-03 11:21 Ioannis Barkas

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9d40bccf-d133-b78e-f6b4-683868e20ddf@silicom-usa.com \
    --to=stephend@silicom-usa.com \
    --cc=axboe@kernel.dk \
    --cc=dan.j.williams@intel.com \
    --cc=hch@infradead.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ide Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ide/0 linux-ide/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ide linux-ide/ https://lore.kernel.org/linux-ide \
		linux-ide@vger.kernel.org linux-ide@archiver.kernel.org
	public-inbox-index linux-ide


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ide


AGPL code for this site: git clone https://public-inbox.org/ public-inbox