Date: Mon, 9 Jul 2018 09:10:57 -0700 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Andrew Turner <andrew@fubar.geek.nz>, Wojciech Macek <wma@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r336130 - head/sys/dev/pci Message-ID: <f7ff165d-d194-d5d2-6b72-c1776715de50@freebsd.org> In-Reply-To: <A92C6AB0-81A3-4A90-91BF-60C8AC28D3FC@fubar.geek.nz> References: <201807090900.w6990GCZ081981@repo.freebsd.org> <A92C6AB0-81A3-4A90-91BF-60C8AC28D3FC@fubar.geek.nz>
next in thread | previous in thread | raw e-mail | index | archive | help
Maybe this file should be renamed and/or split anyway? It's not actually very generic and is used only on (some) ARM systems. -Nathan On 07/09/18 09:00, Andrew Turner wrote: > This breaks at least armv7. Please either fix, or revert this. > > I have some comments below I was planning on adding to the review, but you committed before I had the time to do so. > >> On 9 Jul 2018, at 10:00, Wojciech Macek <wma@freebsd.org> wrote: >> >> Author: wma >> Date: Mon Jul 9 09:00:16 2018 >> New Revision: 336130 >> URL: https://svnweb.freebsd.org/changeset/base/336130 >> >> Log: >> ARM64: Add quirk mechanism to pci_host_generic_acpi >> >> Add few quirks which are necessary to use AHCI on ThX2 >> >> Submitted by: Patryk Duda <pdk@semihalf.com> >> Obtained from: Semihalf >> Sponsored by: Cavium >> Differential revision: https://reviews.freebsd.org/D15929 >> >> Modified: >> head/sys/dev/pci/pci_host_generic.c >> >> Modified: head/sys/dev/pci/pci_host_generic.c >> ============================================================================== >> --- head/sys/dev/pci/pci_host_generic.c Mon Jul 9 08:55:07 2018 (r336129) >> +++ head/sys/dev/pci/pci_host_generic.c Mon Jul 9 09:00:16 2018 (r336130) >> @@ -69,6 +69,25 @@ __FBSDID("$FreeBSD$"); >> (((func) & PCIE_FUNC_MASK) << PCIE_FUNC_SHIFT) | \ >> ((reg) & PCIE_REG_MASK)) >> >> +typedef void (*pci_host_generic_quirk_function)(device_t); >> + >> +struct pci_host_generic_quirk_entry { >> + int impl; >> + int part; >> + int var; >> + int rev; >> + pci_host_generic_quirk_function func; >> +}; >> + >> +struct pci_host_generic_block_entry { >> + int impl; >> + int part; >> + int var; >> + int rev; >> + int bus; >> + int slot; >> +}; >> + >> /* Forward prototypes */ >> >> static uint32_t generic_pcie_read_config(device_t dev, u_int bus, u_int slot, >> @@ -80,7 +99,22 @@ static int generic_pcie_read_ivar(device_t dev, device >> uintptr_t *result); >> static int generic_pcie_write_ivar(device_t dev, device_t child, int index, >> uintptr_t value); >> +static void pci_host_generic_apply_quirks(device_t); >> +static void thunderx2_ahci_bar_quirk(device_t); >> >> +struct pci_host_generic_quirk_entry pci_host_generic_quirks[] = >> +{ >> + {CPU_IMPL_CAVIUM, CPU_PART_THUNDERX2, 0, 0, thunderx2_ahci_bar_quirk}, >> + {0, 0, 0, 0, NULL} > This breaks non-arm64, please fix. > >> +}; >> + >> +struct pci_host_generic_block_entry pci_host_generic_blocked[] = >> +{ >> + /* ThunderX2 AHCI on second socket */ >> + {CPU_IMPL_CAVIUM, CPU_PART_THUNDERX2, 0, 0, 0x80, 0x10}, >> + {0, 0, 0, 0, 0, 0} > And this. > >> +}; >> + >> int >> pci_host_generic_core_attach(device_t dev) >> { >> @@ -134,9 +168,30 @@ pci_host_generic_core_attach(device_t dev) >> return (error); >> } >> >> + pci_host_generic_apply_quirks(dev); >> + >> return (0); >> } >> >> +static void >> +pci_host_generic_apply_quirks(device_t dev) >> +{ >> + struct pci_host_generic_quirk_entry *quirk; >> + >> + quirk = pci_host_generic_quirks; >> + while (1) { >> + if (quirk->impl == 0) >> + break; >> + >> + if (CPU_MATCH(CPU_IMPL_MASK | CPU_PART_MASK, >> + quirk->impl, quirk->part, quirk->var, quirk->rev) && >> + quirk->func != NULL) > This is arm64 specific. > >> + quirk->func(dev); >> + >> + quirk++; >> + } >> +} >> + >> static uint32_t >> generic_pcie_read_config(device_t dev, u_int bus, u_int slot, >> u_int func, u_int reg, int bytes) >> @@ -146,11 +201,25 @@ generic_pcie_read_config(device_t dev, u_int bus, u_in >> bus_space_tag_t t; >> uint64_t offset; >> uint32_t data; >> + struct pci_host_generic_block_entry *block; >> >> if ((bus > PCI_BUSMAX) || (slot > PCI_SLOTMAX) || >> (func > PCI_FUNCMAX) || (reg > PCIE_REGMAX)) >> return (~0U); >> >> + block = pci_host_generic_blocked; >> + while (1) { >> + if (block->impl == 0) >> + break; >> + >> + if (CPU_MATCH(CPU_IMPL_MASK | CPU_PART_MASK, >> + block->impl, block->part, block->var, block->rev) && >> + block->bus == bus && block->slot == slot) > This is also arm64 specific. > >> + return (~0); >> + >> + block++; >> + } >> + >> sc = device_get_softc(dev); >> >> offset = PCIE_ADDR_OFFSET(bus, slot, func, reg); >> @@ -392,3 +461,20 @@ static device_method_t generic_pcie_methods[] = { >> >> DEFINE_CLASS_0(pcib, generic_pcie_core_driver, >> generic_pcie_methods, sizeof(struct generic_pcie_core_softc)); >> + >> +static void thunderx2_ahci_bar_quirk(device_t dev) > This should be dependent on arm64 and an appropriate SOC_ check. > >> +{ >> + >> + /* >> + * XXX: >> + * On ThunderX2, AHCI BAR2 address is wrong. It needs to precisely >> + * match the one described in datasheet. Fixup it unconditionally. >> + */ >> + if (device_get_unit(dev) == 0) { >> + device_printf(dev, "running AHCI BAR fixup\n"); >> + PCIB_WRITE_CONFIG(dev, 0, 16, 0, 0x18, 0x01440000, 4); >> + PCIB_WRITE_CONFIG(dev, 0, 16, 0, 0x1c, 0x40, 4); >> + PCIB_WRITE_CONFIG(dev, 0, 16, 1, 0x18, 0x01450000, 4); >> + PCIB_WRITE_CONFIG(dev, 0, 16, 1, 0x1c, 0x40, 4); >> + } >> +} > Andrew > > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?f7ff165d-d194-d5d2-6b72-c1776715de50>