Date: Mon, 12 Jan 2015 17:21:24 +0100 From: =?UTF-8?Q?Micha=C5=82_Stanek?= <mst@semihalf.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-current@freebsd.org, freebsd-embedded@freebsd.org Subject: Re: [PATCH] Add support for 64-bit AHCI BAR. Message-ID: <CAMiGqYhiGZNSqLkOP3hJodwZ-T4WbpH3i_BrC2J08rGbQ%2BLBDQ@mail.gmail.com> In-Reply-To: <20150109172715.GU42409@kib.kiev.ua> References: <CAMiGqYhKn49LrdFGS4DeGSHDOuVO1Ab0mwubyqGXzxFLmNGoEQ@mail.gmail.com> <20150108203959.GR42409@kib.kiev.ua> <CAMiGqYhi=SU6hg-utRLUi=HauE5Q1qxdYifAxaGNdbVXReaS%2BA@mail.gmail.com> <20150109172715.GU42409@kib.kiev.ua>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
2015-01-09 18:27 GMT+01:00 Konstantin Belousov <kostikbel@gmail.com>:
> On Fri, Jan 09, 2015 at 06:07:39PM +0100, Micha?? Stanek wrote:
> > 2015-01-08 21:40 GMT+01:00 Konstantin Belousov <kostikbel@gmail.com>:
> >
> > > > However, if
> > > > AHCI uses 64-bit base addresses, then this register consists of two
> > > dwords
> > > > starting at offset 0x20 - BAR4 and BAR5. This is the case on our
> arm64
> > > > target and possibly other platforms using 64-bit BARs for AHCI.
> > > Is it specified anywhere, or just a quirk of the specific
> implementation ?
> > > If it is a quirk, would it make sense to also check the vendor or
> device
> > > id before applying the logic ?
> > >
> > >
> > Yes, indeed it is a quirk as I just found out that our platform vendor
> > actually uses BAR(0) as AHCI ABAR, while BAR(4) is used for something
> else.
> > I found it implemented as a quirk in Linux AHCI code.
> > The BAR is still 64-bit but in a different position than AHCI spec
> stated.
> > I changed it as you suggested, the new patch is in the attachment. Please
> > take a look.
> This is probably technically correct (I am not AHCI code author), but note
> that we have more structured quirks mechanism than directly checking vendor
> and device id. Look at the ahci_ids table and the quirks member.
>
> Add a bit declaring the need of the quirk and test the bit, instead of
> the vendor/devid.
>
OK, I added AHCI_Q_ABAR0 as a new quirk and test the bit in
ahci_pci_attach. It works the same. New patch is in the attachment.
> >
> > >
> > > > The following patch adds a check for the extended BAR in
> > > ahci_pci_attach()
> > > > and sets the 'rid' in bus_alloc_resource_any accordingly. It fixes
> the
> > > > allocation error on our platform.
> > > >
> > > > Please review and test this patch on other platforms. If there are no
> > > > issues then it will be committed in a week.
> > > >
> > >
>
> > From b6220884d9e71d7c4fc1c2a22ade374fc023c831 Mon Sep 17 00:00:00 2001
> > From: Michal Stanek <mst@semihalf.com>
> > Date: Fri, 9 Jan 2015 17:20:38 +0100
> > Subject: [PATCH] Add quirk for Cavium AHCI BAR location
> >
> > ---
> > sys/dev/ahci/ahci_pci.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/sys/dev/ahci/ahci_pci.c b/sys/dev/ahci/ahci_pci.c
> > index 43723a6..dce4acb 100644
> > --- a/sys/dev/ahci/ahci_pci.c
> > +++ b/sys/dev/ahci/ahci_pci.c
> > @@ -373,7 +373,6 @@ ahci_pci_attach(device_t dev)
> > int error, i;
> > uint32_t devid = pci_get_devid(dev);
> > uint8_t revid = pci_get_revid(dev);
> > - struct pci_map *map;
> >
> > i = 0;
> > while (ahci_ids[i].id != 0 &&
> > @@ -392,11 +391,9 @@ ahci_pci_attach(device_t dev)
> > ctlr->subvendorid = pci_get_subvendor(dev);
> > ctlr->subdeviceid = pci_get_subdevice(dev);
> >
> > - /* AHCI Base Address is BAR(5) by default, unless BARs are 64-bit
> */
> > - map = pci_find_bar(dev, PCIR_BAR(4));
> > - if (map != NULL &&
> > - ((map->pm_value & PCIM_BAR_MEM_TYPE) == PCIM_BAR_MEM_64))
> > - ctlr->r_rid = PCIR_BAR(4);
> > + /* Default AHCI Base Address is BAR(5), Cavium uses BAR(0) */
> > + if (ctlr->vendorid == 0x177d && ctlr->deviceid == 0xa01c)
> > + ctlr->r_rid = PCIR_BAR(0);
> > else
> > ctlr->r_rid = PCIR_BAR(5);
> > if (!(ctlr->r_mem = bus_alloc_resource_any(dev, SYS_RES_MEMORY,
> > --
> > 2.2.1
> >
>
>
[-- Attachment #2 --]
From 5006e3dff4346e4a49b4edbbe398799b94da656f Mon Sep 17 00:00:00 2001
From: Michal Stanek <mst@semihalf.com>
Date: Wed, 7 Jan 2015 18:28:18 +0100
Subject: [PATCH] Add quirk for Cavium AHCI BAR location
Quirk added to override default BAR(5) rid for AHCI.
---
sys/dev/ahci/ahci.h | 1 +
sys/dev/ahci/ahci_pci.c | 9 +++++++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/sys/dev/ahci/ahci.h b/sys/dev/ahci/ahci.h
index 462f204..fe8fc95 100644
--- a/sys/dev/ahci/ahci.h
+++ b/sys/dev/ahci/ahci.h
@@ -572,6 +572,7 @@ enum ahci_err_type {
#define AHCI_Q_ATI_PMP_BUG 0x2000
#define AHCI_Q_MAXIO_64K 0x4000
#define AHCI_Q_SATA1_UNIT0 0x8000 /* need better method for this */
+#define AHCI_Q_ABAR0 0x10000
#define AHCI_Q_BIT_STRING \
"\020" \
diff --git a/sys/dev/ahci/ahci_pci.c b/sys/dev/ahci/ahci_pci.c
index af26951..8574e47 100644
--- a/sys/dev/ahci/ahci_pci.c
+++ b/sys/dev/ahci/ahci_pci.c
@@ -287,6 +287,7 @@ static const struct {
{0x11841039, 0x00, "SiS 966", 0},
{0x11851039, 0x00, "SiS 968", 0},
{0x01861039, 0x00, "SiS 968", 0},
+ {0xa01c177d, 0x00, "ThunderX SATA", AHCI_Q_ABAR0},
{0x00000000, 0x00, NULL, 0}
};
@@ -386,12 +387,16 @@ ahci_pci_attach(device_t dev)
pci_get_subvendor(dev) == 0x1043 &&
pci_get_subdevice(dev) == 0x81e4)
ctlr->quirks |= AHCI_Q_SATA1_UNIT0;
- /* if we have a memory BAR(5) we are likely on an AHCI part */
ctlr->vendorid = pci_get_vendor(dev);
ctlr->deviceid = pci_get_device(dev);
ctlr->subvendorid = pci_get_subvendor(dev);
ctlr->subdeviceid = pci_get_subdevice(dev);
- ctlr->r_rid = PCIR_BAR(5);
+
+ /* Default AHCI Base Address is BAR(5), Cavium uses BAR(0) */
+ if (ctlr->quirks & AHCI_Q_ABAR0)
+ ctlr->r_rid = PCIR_BAR(0);
+ else
+ ctlr->r_rid = PCIR_BAR(5);
if (!(ctlr->r_mem = bus_alloc_resource_any(dev, SYS_RES_MEMORY,
&ctlr->r_rid, RF_ACTIVE)))
return ENXIO;
--
2.2.1
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAMiGqYhiGZNSqLkOP3hJodwZ-T4WbpH3i_BrC2J08rGbQ%2BLBDQ>