Linux-ide Archive on lore.kernel.org
 help / color / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: Stephen Douthit <stephend@silicom-usa.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 09:00:50 -0700
Message-ID: <CAPcyv4h2tNRC0w1mKjnt6tuZrUcUTh+y1YCeQKTb+b8HftEWZg@mail.gmail.com> (raw)
In-Reply-To: <63de1aac-ff06-729e-0db4-bc1a8a799dc0@silicom-usa.com>

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.

  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 [this message]
2019-08-30 17:05     ` Stephen Douthit
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=CAPcyv4h2tNRC0w1mKjnt6tuZrUcUTh+y1YCeQKTb+b8HftEWZg@mail.gmail.com \
    --to=dan.j.williams@intel.com \
    --cc=axboe@kernel.dk \
    --cc=hch@infradead.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stephend@silicom-usa.com \
    /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