* [PATCH v3 0/4] CM4 ACPI PCIe quirk @ 2021-08-26 7:15 Jeremy Linton 2021-08-26 7:15 ` [PATCH v3 1/4] PCI: brcmstb: Break register definitions into separate header Jeremy Linton ` (3 more replies) 0 siblings, 4 replies; 28+ messages in thread From: Jeremy Linton @ 2021-08-26 7:15 UTC (permalink / raw) To: linux-pci Cc: lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel, Jeremy Linton The PFTF CM4 is an ACPI platform that is following the Arm PCIe SMC (DEN0115) standard because its PCIe config space isn't ECAM compliant since it is split into two parts. One part describes the root port registers, and another contains a moveable window pointing at a given device's 4K config space. Thus it doesn't have an MCFG table. As Linux doesn't support the PCI/SMC, a host bridge specific _DSD is added and associated with custom ECAM ops and cfgres. The custom cfg op selects between those two regions, as well as disallowing problematic accesses. V2->V3: Rebase to -next to pickup new MAINTAINERS entries that needed updating. Enforce _DSD property is exactly the same len as the MCFG OEM field it is overriding. More commit/comment tweaks. V1->V2: Only move register definitions to new .h file, add include guards. Change quirk namespace identifier. Update Maintainers file. A number of whitespace, grammar, etc fixes. Jeremy Linton (4): PCI: brcmstb: Break register definitions into separate header PCI: brcmstb: Add ACPI config space quirk PCI/ACPI: Add Broadcom bcm2711 MCFG quirk MAINTAINERS: Widen brcmstb PCIe file scope MAINTAINERS | 6 +- drivers/acpi/pci_mcfg.c | 17 +++ drivers/pci/controller/Makefile | 1 + drivers/pci/controller/pcie-brcmstb-acpi.c | 79 +++++++++++ drivers/pci/controller/pcie-brcmstb.c | 149 +------------------- drivers/pci/controller/pcie-brcmstb.h | 155 +++++++++++++++++++++ include/linux/pci-ecam.h | 1 + 7 files changed, 257 insertions(+), 151 deletions(-) create mode 100644 drivers/pci/controller/pcie-brcmstb-acpi.c create mode 100644 drivers/pci/controller/pcie-brcmstb.h -- 2.31.1 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 1/4] PCI: brcmstb: Break register definitions into separate header 2021-08-26 7:15 [PATCH v3 0/4] CM4 ACPI PCIe quirk Jeremy Linton @ 2021-08-26 7:15 ` Jeremy Linton 2021-08-30 8:37 ` nicolas saenz julienne 2021-08-26 7:15 ` [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk Jeremy Linton ` (2 subsequent siblings) 3 siblings, 1 reply; 28+ messages in thread From: Jeremy Linton @ 2021-08-26 7:15 UTC (permalink / raw) To: linux-pci Cc: lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel, Jeremy Linton We are about to create a standalone ACPI quirk module for the bcmstb controller. Lets move the register definitions into a separate file so they can be shared between the APCI quirk and the normal host bridge driver. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/controller/pcie-brcmstb.c | 149 +------------------------ drivers/pci/controller/pcie-brcmstb.h | 155 ++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 148 deletions(-) create mode 100644 drivers/pci/controller/pcie-brcmstb.h diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index cc30215f5a43..2517735101ba 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -31,159 +31,12 @@ #include <linux/types.h> #include "../pci.h" - -/* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ -#define BRCM_PCIE_CAP_REGS 0x00ac - -/* Broadcom STB PCIe Register Offsets */ -#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1 0x0188 -#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK 0xc -#define PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN 0x0 - -#define PCIE_RC_CFG_PRIV1_ID_VAL3 0x043c -#define PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK 0xffffff - -#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY 0x04dc -#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00 - -#define PCIE_RC_DL_MDIO_ADDR 0x1100 -#define PCIE_RC_DL_MDIO_WR_DATA 0x1104 -#define PCIE_RC_DL_MDIO_RD_DATA 0x1108 - -#define PCIE_MISC_MISC_CTRL 0x4008 -#define PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK 0x1000 -#define PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK 0x2000 -#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK 0x300000 - -#define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000 -#define PCIE_MISC_MISC_CTRL_SCB1_SIZE_MASK 0x07c00000 -#define PCIE_MISC_MISC_CTRL_SCB2_SIZE_MASK 0x0000001f -#define SCB_SIZE_MASK(x) PCIE_MISC_MISC_CTRL_SCB ## x ## _SIZE_MASK - -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO 0x400c -#define PCIE_MEM_WIN0_LO(win) \ - PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 8) - -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI 0x4010 -#define PCIE_MEM_WIN0_HI(win) \ - PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8) - -#define PCIE_MISC_RC_BAR1_CONFIG_LO 0x402c -#define PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK 0x1f - -#define PCIE_MISC_RC_BAR2_CONFIG_LO 0x4034 -#define PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK 0x1f -#define PCIE_MISC_RC_BAR2_CONFIG_HI 0x4038 - -#define PCIE_MISC_RC_BAR3_CONFIG_LO 0x403c -#define PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK 0x1f - -#define PCIE_MISC_MSI_BAR_CONFIG_LO 0x4044 -#define PCIE_MISC_MSI_BAR_CONFIG_HI 0x4048 - -#define PCIE_MISC_MSI_DATA_CONFIG 0x404c -#define PCIE_MISC_MSI_DATA_CONFIG_VAL_32 0xffe06540 -#define PCIE_MISC_MSI_DATA_CONFIG_VAL_8 0xfff86540 - -#define PCIE_MISC_PCIE_CTRL 0x4064 -#define PCIE_MISC_PCIE_CTRL_PCIE_L23_REQUEST_MASK 0x1 -#define PCIE_MISC_PCIE_CTRL_PCIE_PERSTB_MASK 0x4 - -#define PCIE_MISC_PCIE_STATUS 0x4068 -#define PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK 0x80 -#define PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK 0x20 -#define PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK 0x10 -#define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_MASK 0x40 - -#define PCIE_MISC_REVISION 0x406c -#define BRCM_PCIE_HW_REV_33 0x0303 -#define BRCM_PCIE_HW_REV_3_20 0x0320 - -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT 0x4070 -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK 0xfff00000 -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_MASK 0xfff0 -#define PCIE_MEM_WIN0_BASE_LIMIT(win) \ - PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT + ((win) * 4) - -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI 0x4080 -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI_BASE_MASK 0xff -#define PCIE_MEM_WIN0_BASE_HI(win) \ - PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI + ((win) * 8) - -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI 0x4084 -#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI_LIMIT_MASK 0xff -#define PCIE_MEM_WIN0_LIMIT_HI(win) \ - PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8) - -#define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204 -#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2 -#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000 - - -#define PCIE_INTR2_CPU_BASE 0x4300 -#define PCIE_MSI_INTR2_BASE 0x4500 -/* Offsets from PCIE_INTR2_CPU_BASE and PCIE_MSI_INTR2_BASE */ -#define MSI_INT_STATUS 0x0 -#define MSI_INT_CLR 0x8 -#define MSI_INT_MASK_SET 0x10 -#define MSI_INT_MASK_CLR 0x14 - -#define PCIE_EXT_CFG_DATA 0x8000 -#define PCIE_EXT_CFG_INDEX 0x9000 - -#define PCIE_RGR1_SW_INIT_1_PERST_MASK 0x1 -#define PCIE_RGR1_SW_INIT_1_PERST_SHIFT 0x0 - -#define RGR1_SW_INIT_1_INIT_GENERIC_MASK 0x2 -#define RGR1_SW_INIT_1_INIT_GENERIC_SHIFT 0x1 -#define RGR1_SW_INIT_1_INIT_7278_MASK 0x1 -#define RGR1_SW_INIT_1_INIT_7278_SHIFT 0x0 - -/* PCIe parameters */ -#define BRCM_NUM_PCIE_OUT_WINS 0x4 -#define BRCM_INT_PCI_MSI_NR 32 -#define BRCM_INT_PCI_MSI_LEGACY_NR 8 -#define BRCM_INT_PCI_MSI_SHIFT 0 - -/* MSI target adresses */ -#define BRCM_MSI_TARGET_ADDR_LT_4GB 0x0fffffffcULL -#define BRCM_MSI_TARGET_ADDR_GT_4GB 0xffffffffcULL - -/* MDIO registers */ -#define MDIO_PORT0 0x0 -#define MDIO_DATA_MASK 0x7fffffff -#define MDIO_PORT_MASK 0xf0000 -#define MDIO_REGAD_MASK 0xffff -#define MDIO_CMD_MASK 0xfff00000 -#define MDIO_CMD_READ 0x1 -#define MDIO_CMD_WRITE 0x0 -#define MDIO_DATA_DONE_MASK 0x80000000 -#define MDIO_RD_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 1 : 0) -#define MDIO_WT_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 0 : 1) -#define SSC_REGS_ADDR 0x1100 -#define SET_ADDR_OFFSET 0x1f -#define SSC_CNTL_OFFSET 0x2 -#define SSC_CNTL_OVRD_EN_MASK 0x8000 -#define SSC_CNTL_OVRD_VAL_MASK 0x4000 -#define SSC_STATUS_OFFSET 0x1 -#define SSC_STATUS_SSC_MASK 0x400 -#define SSC_STATUS_PLL_LOCK_MASK 0x800 -#define PCIE_BRCM_MAX_MEMC 3 +#include "pcie-brcmstb.h" #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX]) #define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA]) #define PCIE_RGR1_SW_INIT_1(pcie) (pcie->reg_offsets[RGR1_SW_INIT_1]) -/* Rescal registers */ -#define PCIE_DVT_PMU_PCIE_PHY_CTRL 0xc700 -#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_NFLDS 0x3 -#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_MASK 0x4 -#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_SHIFT 0x2 -#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_MASK 0x2 -#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_SHIFT 0x1 -#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK 0x1 -#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT 0x0 - /* Forward declarations */ struct brcm_pcie; static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val); diff --git a/drivers/pci/controller/pcie-brcmstb.h b/drivers/pci/controller/pcie-brcmstb.h new file mode 100644 index 000000000000..fc20cc7ae02f --- /dev/null +++ b/drivers/pci/controller/pcie-brcmstb.h @@ -0,0 +1,155 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* Copyright (C) 2009 - 2021 Broadcom */ + +#ifndef _PCIE_BRCMSTB_H +#define _PCIE_BRCMSTB_H + +/* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ +#define BRCM_PCIE_CAP_REGS 0x00ac + +/* Broadcom STB PCIe Register Offsets */ +#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1 0x0188 +#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK 0xc +#define PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN 0x0 + +#define PCIE_RC_CFG_PRIV1_ID_VAL3 0x043c +#define PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK 0xffffff + +#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY 0x04dc +#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00 + +#define PCIE_RC_DL_MDIO_ADDR 0x1100 +#define PCIE_RC_DL_MDIO_WR_DATA 0x1104 +#define PCIE_RC_DL_MDIO_RD_DATA 0x1108 + +#define PCIE_MISC_MISC_CTRL 0x4008 +#define PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK 0x1000 +#define PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK 0x2000 +#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK 0x300000 + +#define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000 +#define PCIE_MISC_MISC_CTRL_SCB1_SIZE_MASK 0x07c00000 +#define PCIE_MISC_MISC_CTRL_SCB2_SIZE_MASK 0x0000001f +#define SCB_SIZE_MASK(x) PCIE_MISC_MISC_CTRL_SCB ## x ## _SIZE_MASK + +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO 0x400c +#define PCIE_MEM_WIN0_LO(win) \ + PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 8) + +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI 0x4010 +#define PCIE_MEM_WIN0_HI(win) \ + PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8) + +#define PCIE_MISC_RC_BAR1_CONFIG_LO 0x402c +#define PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK 0x1f + +#define PCIE_MISC_RC_BAR2_CONFIG_LO 0x4034 +#define PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK 0x1f +#define PCIE_MISC_RC_BAR2_CONFIG_HI 0x4038 + +#define PCIE_MISC_RC_BAR3_CONFIG_LO 0x403c +#define PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK 0x1f + +#define PCIE_MISC_MSI_BAR_CONFIG_LO 0x4044 +#define PCIE_MISC_MSI_BAR_CONFIG_HI 0x4048 + +#define PCIE_MISC_MSI_DATA_CONFIG 0x404c +#define PCIE_MISC_MSI_DATA_CONFIG_VAL_32 0xffe06540 +#define PCIE_MISC_MSI_DATA_CONFIG_VAL_8 0xfff86540 + +#define PCIE_MISC_PCIE_CTRL 0x4064 +#define PCIE_MISC_PCIE_CTRL_PCIE_L23_REQUEST_MASK 0x1 +#define PCIE_MISC_PCIE_CTRL_PCIE_PERSTB_MASK 0x4 + +#define PCIE_MISC_PCIE_STATUS 0x4068 +#define PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK 0x80 +#define PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK 0x20 +#define PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK 0x10 +#define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_MASK 0x40 + +#define PCIE_MISC_REVISION 0x406c +#define BRCM_PCIE_HW_REV_33 0x0303 +#define BRCM_PCIE_HW_REV_3_20 0x0320 + +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT 0x4070 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK 0xfff00000 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_MASK 0xfff0 +#define PCIE_MEM_WIN0_BASE_LIMIT(win) \ + PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT + ((win) * 4) + +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI 0x4080 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI_BASE_MASK 0xff +#define PCIE_MEM_WIN0_BASE_HI(win) \ + PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI + ((win) * 8) + +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI 0x4084 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI_LIMIT_MASK 0xff +#define PCIE_MEM_WIN0_LIMIT_HI(win) \ + PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8) + +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204 +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2 +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000 + + +#define PCIE_INTR2_CPU_BASE 0x4300 +#define PCIE_MSI_INTR2_BASE 0x4500 +/* Offsets from PCIE_INTR2_CPU_BASE and PCIE_MSI_INTR2_BASE */ +#define MSI_INT_STATUS 0x0 +#define MSI_INT_CLR 0x8 +#define MSI_INT_MASK_SET 0x10 +#define MSI_INT_MASK_CLR 0x14 + +#define PCIE_EXT_CFG_DATA 0x8000 +#define PCIE_EXT_CFG_INDEX 0x9000 + +#define PCIE_RGR1_SW_INIT_1_PERST_MASK 0x1 +#define PCIE_RGR1_SW_INIT_1_PERST_SHIFT 0x0 + +#define RGR1_SW_INIT_1_INIT_GENERIC_MASK 0x2 +#define RGR1_SW_INIT_1_INIT_GENERIC_SHIFT 0x1 +#define RGR1_SW_INIT_1_INIT_7278_MASK 0x1 +#define RGR1_SW_INIT_1_INIT_7278_SHIFT 0x0 + +/* PCIe parameters */ +#define BRCM_NUM_PCIE_OUT_WINS 0x4 +#define BRCM_INT_PCI_MSI_NR 32 +#define BRCM_INT_PCI_MSI_LEGACY_NR 8 +#define BRCM_INT_PCI_MSI_SHIFT 0 + +/* MSI target addresses */ +#define BRCM_MSI_TARGET_ADDR_LT_4GB 0x0fffffffcULL +#define BRCM_MSI_TARGET_ADDR_GT_4GB 0xffffffffcULL + +/* MDIO registers */ +#define MDIO_PORT0 0x0 +#define MDIO_DATA_MASK 0x7fffffff +#define MDIO_PORT_MASK 0xf0000 +#define MDIO_REGAD_MASK 0xffff +#define MDIO_CMD_MASK 0xfff00000 +#define MDIO_CMD_READ 0x1 +#define MDIO_CMD_WRITE 0x0 +#define MDIO_DATA_DONE_MASK 0x80000000 +#define MDIO_RD_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 1 : 0) +#define MDIO_WT_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 0 : 1) +#define SSC_REGS_ADDR 0x1100 +#define SET_ADDR_OFFSET 0x1f +#define SSC_CNTL_OFFSET 0x2 +#define SSC_CNTL_OVRD_EN_MASK 0x8000 +#define SSC_CNTL_OVRD_VAL_MASK 0x4000 +#define SSC_STATUS_OFFSET 0x1 +#define SSC_STATUS_SSC_MASK 0x400 +#define SSC_STATUS_PLL_LOCK_MASK 0x800 +#define PCIE_BRCM_MAX_MEMC 3 + +/* Rescal registers */ +#define PCIE_DVT_PMU_PCIE_PHY_CTRL 0xc700 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_NFLDS 0x3 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_MASK 0x4 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_SHIFT 0x2 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_MASK 0x2 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_SHIFT 0x1 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK 0x1 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT 0x0 + +#endif /* _PCIE_BRCMSTB_H */ -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 1/4] PCI: brcmstb: Break register definitions into separate header 2021-08-26 7:15 ` [PATCH v3 1/4] PCI: brcmstb: Break register definitions into separate header Jeremy Linton @ 2021-08-30 8:37 ` nicolas saenz julienne 0 siblings, 0 replies; 28+ messages in thread From: nicolas saenz julienne @ 2021-08-30 8:37 UTC (permalink / raw) To: Jeremy Linton, linux-pci Cc: lorenzo.pieralisi, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote: > We are about to create a standalone ACPI quirk module for the > bcmstb controller. Lets move the register definitions into a separate > file so they can be shared between the APCI quirk and the normal > host bridge driver. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- Acked-by: Nicolas Saenz Julienne <nsaenz@kernel.org> Regards, Nicolas ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-08-26 7:15 [PATCH v3 0/4] CM4 ACPI PCIe quirk Jeremy Linton 2021-08-26 7:15 ` [PATCH v3 1/4] PCI: brcmstb: Break register definitions into separate header Jeremy Linton @ 2021-08-26 7:15 ` Jeremy Linton 2021-08-30 8:36 ` nicolas saenz julienne 2021-10-05 15:32 ` Bjorn Helgaas 2021-08-26 7:15 ` [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk Jeremy Linton 2021-08-26 7:15 ` [PATCH v3 4/4] MAINTAINERS: Widen brcmstb PCIe file scope Jeremy Linton 3 siblings, 2 replies; 28+ messages in thread From: Jeremy Linton @ 2021-08-26 7:15 UTC (permalink / raw) To: linux-pci Cc: lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel, Jeremy Linton The Pi Firmware Task Force (PFTF: https://github.com/pftf) Compute Module 4 (CM4: an embedded form factor RPi4) is an ACPI platform that isn't ECAM compliant. Its config space is in two parts. One part is for the root port registers and a second moveable window pointing at a device's 4K config space. Thus it doesn't have an MCFG, and any MCFG provided would be nonsense anyway. Instead, a custom pci_ecam_ops quirk is created. The custom ops override the .init and .map_bus functions. The former to assure that cfg->win points at a single mapping that contains the root port registers and the device config window, as well as disabling MSIs due to lack of a GICv2M. map_bus() then provides the address of either the standard portion of the root port registers or to the device config window after it has been moved. Additionally, some basic bus/device filtering exist to avoid sending config transactions to invalid devices on the RP's primary or secondary bus. A basic link check is also made to assure that something is operational on the secondary side before probing the remainder of the config space. If either of these constraints are violated and a config operation is lost in the ether because an EP doesn't respond an unrecoverable SERROR is raised. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/controller/Makefile | 1 + drivers/pci/controller/pcie-brcmstb-acpi.c | 79 ++++++++++++++++++++++ include/linux/pci-ecam.h | 1 + 3 files changed, 81 insertions(+) create mode 100644 drivers/pci/controller/pcie-brcmstb-acpi.c diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile index aaf30b3dcc14..65aa6fd3ed89 100644 --- a/drivers/pci/controller/Makefile +++ b/drivers/pci/controller/Makefile @@ -57,5 +57,6 @@ ifdef CONFIG_PCI_QUIRKS obj-$(CONFIG_ARM64) += pci-thunder-ecam.o obj-$(CONFIG_ARM64) += pci-thunder-pem.o obj-$(CONFIG_ARM64) += pci-xgene.o +obj-$(CONFIG_ARM64) += pcie-brcmstb-acpi.o endif endif diff --git a/drivers/pci/controller/pcie-brcmstb-acpi.c b/drivers/pci/controller/pcie-brcmstb-acpi.c new file mode 100644 index 000000000000..528b2b3ffbd2 --- /dev/null +++ b/drivers/pci/controller/pcie-brcmstb-acpi.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * ACPI quirks for Brcm2711 PCIe host controller + * As used on the Raspberry Pi Compute Module 4 + * + * Copyright (C) 2021 Arm Ltd. + */ + +#include <linux/io.h> +#include <linux/pci.h> +#include <linux/pci-ecam.h> +#include "../pci.h" +#include "pcie-brcmstb.h" + +static int brcm_acpi_init(struct pci_config_window *cfg) +{ + /* + * This platform doesn't technically have anything that could be called + * ECAM. Its config region has root port specific registers between + * standard PCIe defined config registers. Thus the region setup by the + * generic ECAM code needs to be adjusted. The HW can access bus 0-ff + * but the footprint isn't a nice power of 2 (40k). For purposes of + * mapping the config region we are just going to squash the standard + * and nonstandard registers together rather than mapping them separately. + */ + iounmap(cfg->win); + cfg->win = pci_remap_cfgspace(cfg->res.start, resource_size(&cfg->res)); + if (!cfg->win) + goto err_exit; + + /* MSI is nonstandard as well */ + pci_no_msi(); + + return 0; +err_exit: + dev_err(cfg->parent, "PCI: Failed to remap config\n"); + return -ENOMEM; +} + +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus, + unsigned int devfn, int where) +{ + struct pci_config_window *cfg = bus->sysdata; + void __iomem *base = cfg->win; + int idx; + u32 up; + + /* Accesses to the RC go right to the RC registers if slot==0 */ + if (pci_is_root_bus(bus)) + return PCI_SLOT(devfn) ? NULL : base + where; + + /* + * Assure the link is up before sending requests downstream. This is done + * to avoid sending transactions to EPs that don't exist. Link flap + * conditions/etc make this race more probable. The resulting unrecoverable + * SERRORs will result in the machine crashing. + */ + up = readl(base + PCIE_MISC_PCIE_STATUS); + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK)) + return NULL; + + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK)) + return NULL; + + /* For devices, write to the config space index register */ + idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0); + writel(idx, base + PCIE_EXT_CFG_INDEX); + return base + PCIE_EXT_CFG_DATA + where; +} + +const struct pci_ecam_ops bcm2711_pcie_ops = { + .init = brcm_acpi_init, + .bus_shift = 1, + .pci_ops = { + .map_bus = brcm_pcie_map_conf2, + .read = pci_generic_config_read, + .write = pci_generic_config_write, + } +}; diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h index adea5a4771cf..a5de0285bb7f 100644 --- a/include/linux/pci-ecam.h +++ b/include/linux/pci-ecam.h @@ -87,6 +87,7 @@ extern const struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 * extern const struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */ extern const struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */ extern const struct pci_ecam_ops tegra194_pcie_ops; /* Tegra194 PCIe */ +extern const struct pci_ecam_ops bcm2711_pcie_ops; /* Bcm2711 PCIe */ #endif #if IS_ENABLED(CONFIG_PCI_HOST_COMMON) -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-08-26 7:15 ` [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk Jeremy Linton @ 2021-08-30 8:36 ` nicolas saenz julienne 2021-08-30 16:23 ` Jeremy Linton 2021-10-05 15:32 ` Bjorn Helgaas 1 sibling, 1 reply; 28+ messages in thread From: nicolas saenz julienne @ 2021-08-30 8:36 UTC (permalink / raw) To: Jeremy Linton, linux-pci Cc: lorenzo.pieralisi, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hi Jeremy, sorry for the late reply, I've been on vacation. On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote: [...] > +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus, > + unsigned int devfn, int where) > +{ > + struct pci_config_window *cfg = bus->sysdata; > + void __iomem *base = cfg->win; > + int idx; > + u32 up; > + > + /* Accesses to the RC go right to the RC registers if slot==0 */ > + if (pci_is_root_bus(bus)) > + return PCI_SLOT(devfn) ? NULL : base + where; > + > + /* > + * Assure the link is up before sending requests downstream. This is done > + * to avoid sending transactions to EPs that don't exist. Link flap > + * conditions/etc make this race more probable. The resulting unrecoverable > + * SERRORs will result in the machine crashing. > + */ > + up = readl(base + PCIE_MISC_PCIE_STATUS); > + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK)) > + return NULL; > + > + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK)) > + return NULL; Couldn't this be integrated in the original brcm_pcie_map_conf()? IIUC there is nothing ACPI specific about it. It'd also make for less code duplication. Regards, Nicolas ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-08-30 8:36 ` nicolas saenz julienne @ 2021-08-30 16:23 ` Jeremy Linton 2021-08-30 16:27 ` Florian Fainelli 0 siblings, 1 reply; 28+ messages in thread From: Jeremy Linton @ 2021-08-30 16:23 UTC (permalink / raw) To: nicolas saenz julienne, linux-pci Cc: lorenzo.pieralisi, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hi, On 8/30/21 3:36 AM, nicolas saenz julienne wrote: > Hi Jeremy, > sorry for the late reply, I've been on vacation. > > On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote: > > [...] > >> +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus, >> + unsigned int devfn, int where) >> +{ >> + struct pci_config_window *cfg = bus->sysdata; >> + void __iomem *base = cfg->win; >> + int idx; >> + u32 up; >> + >> + /* Accesses to the RC go right to the RC registers if slot==0 */ >> + if (pci_is_root_bus(bus)) >> + return PCI_SLOT(devfn) ? NULL : base + where; >> + >> + /* >> + * Assure the link is up before sending requests downstream. This is done >> + * to avoid sending transactions to EPs that don't exist. Link flap >> + * conditions/etc make this race more probable. The resulting unrecoverable >> + * SERRORs will result in the machine crashing. >> + */ >> + up = readl(base + PCIE_MISC_PCIE_STATUS); >> + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK)) >> + return NULL; >> + >> + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK)) >> + return NULL; > > Couldn't this be integrated in the original brcm_pcie_map_conf()? IIUC there is > nothing ACPI specific about it. It'd also make for less code duplication. That is where I started with this, but it wasn't the linkup check/etc which caused me to hoist it but the fact that if ACPI quirks are enabled they end up statically built into the kernel. While if this host bridge is enabled, it can end up being a module, and the resulting mess I created trying to satisfy the CONFIG variations. I'm not much of a fan of copy/paste programming, but that IMHO ended up being the cleanest here. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-08-30 16:23 ` Jeremy Linton @ 2021-08-30 16:27 ` Florian Fainelli 2021-08-30 17:17 ` nicolas saenz julienne 0 siblings, 1 reply; 28+ messages in thread From: Florian Fainelli @ 2021-08-30 16:27 UTC (permalink / raw) To: Jeremy Linton, nicolas saenz julienne, linux-pci Cc: lorenzo.pieralisi, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On 8/30/2021 9:23 AM, Jeremy Linton wrote: > Hi, > > On 8/30/21 3:36 AM, nicolas saenz julienne wrote: >> Hi Jeremy, >> sorry for the late reply, I've been on vacation. >> >> On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote: >> >> [...] >> >>> +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus, >>> + unsigned int devfn, int where) >>> +{ >>> + struct pci_config_window *cfg = bus->sysdata; >>> + void __iomem *base = cfg->win; >>> + int idx; >>> + u32 up; >>> + >>> + /* Accesses to the RC go right to the RC registers if slot==0 */ >>> + if (pci_is_root_bus(bus)) >>> + return PCI_SLOT(devfn) ? NULL : base + where; >>> + >>> + /* >>> + * Assure the link is up before sending requests downstream. >>> This is done >>> + * to avoid sending transactions to EPs that don't exist. Link flap >>> + * conditions/etc make this race more probable. The resulting >>> unrecoverable >>> + * SERRORs will result in the machine crashing. >>> + */ >>> + up = readl(base + PCIE_MISC_PCIE_STATUS); >>> + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK)) >>> + return NULL; >>> + >>> + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK)) >>> + return NULL; >> >> Couldn't this be integrated in the original brcm_pcie_map_conf()? IIUC >> there is >> nothing ACPI specific about it. It'd also make for less code duplication. > > That is where I started with this, but it wasn't the linkup check/etc > which caused me to hoist it but the fact that if ACPI quirks are enabled > they end up statically built into the kernel. While if this host bridge > is enabled, it can end up being a module, and the resulting mess I > created trying to satisfy the CONFIG variations. I'm not much of a fan > of copy/paste programming, but that IMHO ended up being the cleanest here. > Agreed, the open coding that is being done is reasonable IHMO, although we may have to update the link up code in both pcie-brcmstb.c and this file in the future if offsets/bits do change, nothing impossible though. -- Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-08-30 16:27 ` Florian Fainelli @ 2021-08-30 17:17 ` nicolas saenz julienne 0 siblings, 0 replies; 28+ messages in thread From: nicolas saenz julienne @ 2021-08-30 17:17 UTC (permalink / raw) To: Florian Fainelli, Jeremy Linton, linux-pci Cc: lorenzo.pieralisi, bhelgaas, rjw, lenb, robh, kw, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Mon, 2021-08-30 at 09:27 -0700, Florian Fainelli wrote: > > On 8/30/2021 9:23 AM, Jeremy Linton wrote: > > Hi, > > > > On 8/30/21 3:36 AM, nicolas saenz julienne wrote: > > > Hi Jeremy, > > > sorry for the late reply, I've been on vacation. > > > > > > On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote: > > > > > > [...] > > > > > > > +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus, > > > > + unsigned int devfn, int where) > > > > +{ > > > > + struct pci_config_window *cfg = bus->sysdata; > > > > + void __iomem *base = cfg->win; > > > > + int idx; > > > > + u32 up; > > > > + > > > > + /* Accesses to the RC go right to the RC registers if slot==0 */ > > > > + if (pci_is_root_bus(bus)) > > > > + return PCI_SLOT(devfn) ? NULL : base + where; > > > > + > > > > + /* > > > > + * Assure the link is up before sending requests downstream. > > > > This is done > > > > + * to avoid sending transactions to EPs that don't exist. Link flap > > > > + * conditions/etc make this race more probable. The resulting > > > > unrecoverable > > > > + * SERRORs will result in the machine crashing. > > > > + */ > > > > + up = readl(base + PCIE_MISC_PCIE_STATUS); > > > > + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK)) > > > > + return NULL; > > > > + > > > > + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK)) > > > > + return NULL; > > > > > > Couldn't this be integrated in the original brcm_pcie_map_conf()? IIUC > > > there is > > > nothing ACPI specific about it. It'd also make for less code duplication. > > > > That is where I started with this, but it wasn't the linkup check/etc > > which caused me to hoist it but the fact that if ACPI quirks are enabled > > they end up statically built into the kernel. While if this host bridge > > is enabled, it can end up being a module, and the resulting mess I > > created trying to satisfy the CONFIG variations. I'm not much of a fan > > of copy/paste programming, but that IMHO ended up being the cleanest here. > > > > Agreed, the open coding that is being done is reasonable IHMO, although > we may have to update the link up code in both pcie-brcmstb.c and this > file in the future if offsets/bits do change, nothing impossible though. Fair enough. Acked-by: Nicolas Saenz Julienne <nsaenz@kernel.org> Regards, Nicolas ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-08-26 7:15 ` [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk Jeremy Linton 2021-08-30 8:36 ` nicolas saenz julienne @ 2021-10-05 15:32 ` Bjorn Helgaas 2021-10-05 15:57 ` Jeremy Linton 1 sibling, 1 reply; 28+ messages in thread From: Bjorn Helgaas @ 2021-10-05 15:32 UTC (permalink / raw) To: Jeremy Linton Cc: linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: > The Pi Firmware Task Force (PFTF: https://github.com/pftf) Compute > Module 4 (CM4: an embedded form factor RPi4) is an ACPI platform that > isn't ECAM compliant. Its config space is in two parts. One part is for > the root port registers and a second moveable window pointing at a > device's 4K config space. Thus it doesn't have an MCFG, and any MCFG > provided would be nonsense anyway. > > Instead, a custom pci_ecam_ops quirk is created. The custom ops override > the .init and .map_bus functions. The former to assure that cfg->win > points at a single mapping that contains the root port registers and the > device config window, as well as disabling MSIs due to lack of a > GICv2M. map_bus() then provides the address of either the standard > portion of the root port registers or to the device config window after > it has been moved. > > Additionally, some basic bus/device filtering exist to avoid sending > config transactions to invalid devices on the RP's primary or > secondary bus. A basic link check is also made to assure that > something is operational on the secondary side before probing the > remainder of the config space. If either of these constraints are > violated and a config operation is lost in the ether because an EP > doesn't respond an unrecoverable SERROR is raised. It's not "lost"; I assume the root port raises an error because it can't send a transaction over a link that is down. Is "SERROR" an ARM64 thing? My guess is the root port would raise an Unsupported Request error or similar, and the root complex turns that into a system-specific SERROR? > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/controller/Makefile | 1 + > drivers/pci/controller/pcie-brcmstb-acpi.c | 79 ++++++++++++++++++++++ > include/linux/pci-ecam.h | 1 + > 3 files changed, 81 insertions(+) > create mode 100644 drivers/pci/controller/pcie-brcmstb-acpi.c > > diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile > index aaf30b3dcc14..65aa6fd3ed89 100644 > --- a/drivers/pci/controller/Makefile > +++ b/drivers/pci/controller/Makefile > @@ -57,5 +57,6 @@ ifdef CONFIG_PCI_QUIRKS > obj-$(CONFIG_ARM64) += pci-thunder-ecam.o > obj-$(CONFIG_ARM64) += pci-thunder-pem.o > obj-$(CONFIG_ARM64) += pci-xgene.o > +obj-$(CONFIG_ARM64) += pcie-brcmstb-acpi.o > endif > endif > diff --git a/drivers/pci/controller/pcie-brcmstb-acpi.c b/drivers/pci/controller/pcie-brcmstb-acpi.c > new file mode 100644 > index 000000000000..528b2b3ffbd2 > --- /dev/null > +++ b/drivers/pci/controller/pcie-brcmstb-acpi.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * ACPI quirks for Brcm2711 PCIe host controller > + * As used on the Raspberry Pi Compute Module 4 > + * > + * Copyright (C) 2021 Arm Ltd. > + */ > + > +#include <linux/io.h> > +#include <linux/pci.h> > +#include <linux/pci-ecam.h> > +#include "../pci.h" > +#include "pcie-brcmstb.h" > + > +static int brcm_acpi_init(struct pci_config_window *cfg) > +{ > + /* > + * This platform doesn't technically have anything that could be called > + * ECAM. Its config region has root port specific registers between > + * standard PCIe defined config registers. Thus the region setup by the > + * generic ECAM code needs to be adjusted. The HW can access bus 0-ff > + * but the footprint isn't a nice power of 2 (40k). For purposes of > + * mapping the config region we are just going to squash the standard > + * and nonstandard registers together rather than mapping them separately. Wrap this and comment below to fit in 80 columns. Nothing magic about 80 columns except for the fact that all the other code in drivers/pci fits in that width and consistency is helpful. > + */ > + iounmap(cfg->win); > + cfg->win = pci_remap_cfgspace(cfg->res.start, resource_size(&cfg->res)); > + if (!cfg->win) > + goto err_exit; > + > + /* MSI is nonstandard as well */ > + pci_no_msi(); This doesn't seem to fit in an MCFG quirk. > + return 0; > +err_exit: > + dev_err(cfg->parent, "PCI: Failed to remap config\n"); > + return -ENOMEM; > +} > + > +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus, > + unsigned int devfn, int where) > +{ > + struct pci_config_window *cfg = bus->sysdata; > + void __iomem *base = cfg->win; > + int idx; > + u32 up; > + > + /* Accesses to the RC go right to the RC registers if slot==0 */ > + if (pci_is_root_bus(bus)) > + return PCI_SLOT(devfn) ? NULL : base + where; > + > + /* > + * Assure the link is up before sending requests downstream. This is done > + * to avoid sending transactions to EPs that don't exist. Link flap > + * conditions/etc make this race more probable. The resulting unrecoverable > + * SERRORs will result in the machine crashing. Is the crash because SERROR is fundamentally unrecoverable? Is there any control over what kind of system-specific error the PCIe errors are mapped to? I know there are other systems where PCIe errors always cause a system crash, but most platforms seem to be moving toward at least the theoretical ability to recover from I/O errors. > + */ > + up = readl(base + PCIE_MISC_PCIE_STATUS); > + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK)) > + return NULL; > + > + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK)) > + return NULL; > + > + /* For devices, write to the config space index register */ > + idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0); > + writel(idx, base + PCIE_EXT_CFG_INDEX); > + return base + PCIE_EXT_CFG_DATA + where; > +} > + > +const struct pci_ecam_ops bcm2711_pcie_ops = { > + .init = brcm_acpi_init, > + .bus_shift = 1, > + .pci_ops = { > + .map_bus = brcm_pcie_map_conf2, > + .read = pci_generic_config_read, > + .write = pci_generic_config_write, > + } > +}; > diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h > index adea5a4771cf..a5de0285bb7f 100644 > --- a/include/linux/pci-ecam.h > +++ b/include/linux/pci-ecam.h > @@ -87,6 +87,7 @@ extern const struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 * > extern const struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */ > extern const struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */ > extern const struct pci_ecam_ops tegra194_pcie_ops; /* Tegra194 PCIe */ > +extern const struct pci_ecam_ops bcm2711_pcie_ops; /* Bcm2711 PCIe */ > #endif > > #if IS_ENABLED(CONFIG_PCI_HOST_COMMON) > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-05 15:32 ` Bjorn Helgaas @ 2021-10-05 15:57 ` Jeremy Linton 2021-10-05 19:43 ` Pali Rohár 0 siblings, 1 reply; 28+ messages in thread From: Jeremy Linton @ 2021-10-05 15:57 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hi, On 10/5/21 10:32 AM, Bjorn Helgaas wrote: > On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: >> The Pi Firmware Task Force (PFTF: https://github.com/pftf) Compute >> Module 4 (CM4: an embedded form factor RPi4) is an ACPI platform that >> isn't ECAM compliant. Its config space is in two parts. One part is for >> the root port registers and a second moveable window pointing at a >> device's 4K config space. Thus it doesn't have an MCFG, and any MCFG >> provided would be nonsense anyway. >> >> Instead, a custom pci_ecam_ops quirk is created. The custom ops override >> the .init and .map_bus functions. The former to assure that cfg->win >> points at a single mapping that contains the root port registers and the >> device config window, as well as disabling MSIs due to lack of a >> GICv2M. map_bus() then provides the address of either the standard >> portion of the root port registers or to the device config window after >> it has been moved. >> >> Additionally, some basic bus/device filtering exist to avoid sending >> config transactions to invalid devices on the RP's primary or >> secondary bus. A basic link check is also made to assure that >> something is operational on the secondary side before probing the >> remainder of the config space. If either of these constraints are >> violated and a config operation is lost in the ether because an EP >> doesn't respond an unrecoverable SERROR is raised. > > It's not "lost"; I assume the root port raises an error because it > can't send a transaction over a link that is down. The problem is AFAIK because the root port doesn't do that. > > Is "SERROR" an ARM64 thing? My guess is the root port would raise an > Unsupported Request error or similar, and the root complex turns that > into a system-specific SERROR? AFAIK, what is happening here the CPU core has an outstanding R/W request for which it never receives a response from the root port. So basically its an interconnect protocol violation that the CPU is complaining about rather than something PCIe specific. > >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> Acked-by: Florian Fainelli <f.fainelli@gmail.com> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >> --- >> drivers/pci/controller/Makefile | 1 + >> drivers/pci/controller/pcie-brcmstb-acpi.c | 79 ++++++++++++++++++++++ >> include/linux/pci-ecam.h | 1 + >> 3 files changed, 81 insertions(+) >> create mode 100644 drivers/pci/controller/pcie-brcmstb-acpi.c >> >> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile >> index aaf30b3dcc14..65aa6fd3ed89 100644 >> --- a/drivers/pci/controller/Makefile >> +++ b/drivers/pci/controller/Makefile >> @@ -57,5 +57,6 @@ ifdef CONFIG_PCI_QUIRKS >> obj-$(CONFIG_ARM64) += pci-thunder-ecam.o >> obj-$(CONFIG_ARM64) += pci-thunder-pem.o >> obj-$(CONFIG_ARM64) += pci-xgene.o >> +obj-$(CONFIG_ARM64) += pcie-brcmstb-acpi.o >> endif >> endif >> diff --git a/drivers/pci/controller/pcie-brcmstb-acpi.c b/drivers/pci/controller/pcie-brcmstb-acpi.c >> new file mode 100644 >> index 000000000000..528b2b3ffbd2 >> --- /dev/null >> +++ b/drivers/pci/controller/pcie-brcmstb-acpi.c >> @@ -0,0 +1,79 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * ACPI quirks for Brcm2711 PCIe host controller >> + * As used on the Raspberry Pi Compute Module 4 >> + * >> + * Copyright (C) 2021 Arm Ltd. >> + */ >> + >> +#include <linux/io.h> >> +#include <linux/pci.h> >> +#include <linux/pci-ecam.h> >> +#include "../pci.h" >> +#include "pcie-brcmstb.h" >> + >> +static int brcm_acpi_init(struct pci_config_window *cfg) >> +{ >> + /* >> + * This platform doesn't technically have anything that could be called >> + * ECAM. Its config region has root port specific registers between >> + * standard PCIe defined config registers. Thus the region setup by the >> + * generic ECAM code needs to be adjusted. The HW can access bus 0-ff >> + * but the footprint isn't a nice power of 2 (40k). For purposes of >> + * mapping the config region we are just going to squash the standard >> + * and nonstandard registers together rather than mapping them separately. > > Wrap this and comment below to fit in 80 columns. Nothing magic about > 80 columns except for the fact that all the other code in drivers/pci > fits in that width and consistency is helpful. Hmm, I thought I had wrapped these, but probably at 78, guess I will rebase/repost. > >> + */ >> + iounmap(cfg->win); >> + cfg->win = pci_remap_cfgspace(cfg->res.start, resource_size(&cfg->res)); >> + if (!cfg->win) >> + goto err_exit; >> + >> + /* MSI is nonstandard as well */ >> + pci_no_msi(); > > This doesn't seem to fit in an MCFG quirk. "pcie quirk" > >> + return 0; >> +err_exit: >> + dev_err(cfg->parent, "PCI: Failed to remap config\n"); >> + return -ENOMEM; >> +} >> + >> +static void __iomem *brcm_pcie_map_conf2(struct pci_bus *bus, >> + unsigned int devfn, int where) >> +{ >> + struct pci_config_window *cfg = bus->sysdata; >> + void __iomem *base = cfg->win; >> + int idx; >> + u32 up; >> + >> + /* Accesses to the RC go right to the RC registers if slot==0 */ >> + if (pci_is_root_bus(bus)) >> + return PCI_SLOT(devfn) ? NULL : base + where; >> + >> + /* >> + * Assure the link is up before sending requests downstream. This is done >> + * to avoid sending transactions to EPs that don't exist. Link flap >> + * conditions/etc make this race more probable. The resulting unrecoverable >> + * SERRORs will result in the machine crashing. > > Is the crash because SERROR is fundamentally unrecoverable? Is there > any control over what kind of system-specific error the PCIe errors > are mapped to? Yes, that is basically where we are because the reason for the exception can't really be pinned down. There are some thoughts about ways to work around the problem, but they aren't pretty and probably need to be done at an exception level higher than what the kernel is operating at. Hence the SMC. > > I know there are other systems where PCIe errors always cause a system > crash, but most platforms seem to be moving toward at least the > theoretical ability to recover from I/O errors. Yes, that would be part of SBSA, which of course this platform isn't. > >> + */ >> + up = readl(base + PCIE_MISC_PCIE_STATUS); >> + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK)) >> + return NULL; >> + >> + if (!(up & PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK)) >> + return NULL; >> + >> + /* For devices, write to the config space index register */ >> + idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0); >> + writel(idx, base + PCIE_EXT_CFG_INDEX); >> + return base + PCIE_EXT_CFG_DATA + where; >> +} >> + >> +const struct pci_ecam_ops bcm2711_pcie_ops = { >> + .init = brcm_acpi_init, >> + .bus_shift = 1, >> + .pci_ops = { >> + .map_bus = brcm_pcie_map_conf2, >> + .read = pci_generic_config_read, >> + .write = pci_generic_config_write, >> + } >> +}; >> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h >> index adea5a4771cf..a5de0285bb7f 100644 >> --- a/include/linux/pci-ecam.h >> +++ b/include/linux/pci-ecam.h >> @@ -87,6 +87,7 @@ extern const struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 * >> extern const struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */ >> extern const struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */ >> extern const struct pci_ecam_ops tegra194_pcie_ops; /* Tegra194 PCIe */ >> +extern const struct pci_ecam_ops bcm2711_pcie_ops; /* Bcm2711 PCIe */ >> #endif >> >> #if IS_ENABLED(CONFIG_PCI_HOST_COMMON) >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-05 15:57 ` Jeremy Linton @ 2021-10-05 19:43 ` Pali Rohár 2021-10-05 22:25 ` Jeremy Linton 0 siblings, 1 reply; 28+ messages in thread From: Pali Rohár @ 2021-10-05 19:43 UTC (permalink / raw) To: Jeremy Linton Cc: Bjorn Helgaas, linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hello! On Tuesday 05 October 2021 10:57:18 Jeremy Linton wrote: > Hi, > > On 10/5/21 10:32 AM, Bjorn Helgaas wrote: > > On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: > > > Additionally, some basic bus/device filtering exist to avoid sending > > > config transactions to invalid devices on the RP's primary or > > > secondary bus. A basic link check is also made to assure that > > > something is operational on the secondary side before probing the > > > remainder of the config space. If either of these constraints are > > > violated and a config operation is lost in the ether because an EP > > > doesn't respond an unrecoverable SERROR is raised. > > > > It's not "lost"; I assume the root port raises an error because it > > can't send a transaction over a link that is down. > > The problem is AFAIK because the root port doesn't do that. Interesting! Does it mean that PCIe Root Complex / Host Bridge (which I guess contains also logic for Root Port) does not signal transaction failure for config requests? Or it is just your opinion? Because I'm dealing with similar issues and I'm trying to find a way how to detect if some PCIe IP signal transaction error via AXI SLVERR response OR it just does not send any response back. So if you know some way how to check which one it is, I would like to know it too. > > > > Is "SERROR" an ARM64 thing? My guess is the root port would raise an > > Unsupported Request error or similar, and the root complex turns that > > into a system-specific SERROR? Yes, SError is arm64 specific. It is asynchronous CPU interrupt and syndrome code then contains what happened. > AFAIK, what is happening here the CPU core has an outstanding R/W request > for which it never receives a response from the root port. So basically its > an interconnect protocol violation that the CPU is complaining about rather > than something PCIe specific. Could you describe (ideally in commit message) which SError is triggered? Normally if kernel receive SError interrupt it also puts into dmesg or oops message also syndrome code which describe what kind of error / event occurred. It could help also to other understand what is happening there. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-05 19:43 ` Pali Rohár @ 2021-10-05 22:25 ` Jeremy Linton 2021-10-06 2:07 ` Florian Fainelli 0 siblings, 1 reply; 28+ messages in thread From: Jeremy Linton @ 2021-10-05 22:25 UTC (permalink / raw) To: Pali Rohár Cc: Bjorn Helgaas, linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hi, On 10/5/21 2:43 PM, Pali Rohár wrote: > Hello! > > On Tuesday 05 October 2021 10:57:18 Jeremy Linton wrote: >> Hi, >> >> On 10/5/21 10:32 AM, Bjorn Helgaas wrote: >>> On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: >>>> Additionally, some basic bus/device filtering exist to avoid sending >>>> config transactions to invalid devices on the RP's primary or >>>> secondary bus. A basic link check is also made to assure that >>>> something is operational on the secondary side before probing the >>>> remainder of the config space. If either of these constraints are >>>> violated and a config operation is lost in the ether because an EP >>>> doesn't respond an unrecoverable SERROR is raised. >>> >>> It's not "lost"; I assume the root port raises an error because it >>> can't send a transaction over a link that is down. >> >> The problem is AFAIK because the root port doesn't do that. > > Interesting! Does it mean that PCIe Root Complex / Host Bridge (which I > guess contains also logic for Root Port) does not signal transaction > failure for config requests? Or it is just your opinion? Because I'm > dealing with similar issues and I'm trying to find a way how to detect > if some PCIe IP signal transaction error via AXI SLVERR response OR it > just does not send any response back. So if you know some way how to > check which one it is, I would like to know it too. This is my _opinion_ based on what I've heard of some other IP integration issues, and what i've seen poking at this one from the perspective of a SW guy rather than a HW guy. So, basically worthless. But, you should consider that most of these cores/interconnects aren't aware of PCIe completion semantics so its the root ports responsibility to say, gracefully translate a non-posted write that doesn't have a completion for the interconnects its attached to, rather than tripping something generic like a SLVERR. Anyway, for this I would poke around the pile of exception registers, with your specific processors manual handy because a lot of them are implementation defined. >>> >>> Is "SERROR" an ARM64 thing? My guess is the root port would raise an >>> Unsupported Request error or similar, and the root complex turns that >>> into a system-specific SERROR? > > Yes, SError is arm64 specific. It is asynchronous CPU interrupt and > syndrome code then contains what happened. > >> AFAIK, what is happening here the CPU core has an outstanding R/W request >> for which it never receives a response from the root port. So basically its >> an interconnect protocol violation that the CPU is complaining about rather >> than something PCIe specific. > > Could you describe (ideally in commit message) which SError is > triggered? Normally if kernel receive SError interrupt it also puts into > dmesg or oops message also syndrome code which describe what kind of > error / event occurred. It could help also to other understand what is > happening there. > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-05 22:25 ` Jeremy Linton @ 2021-10-06 2:07 ` Florian Fainelli 2021-10-22 17:04 ` Florian Fainelli 0 siblings, 1 reply; 28+ messages in thread From: Florian Fainelli @ 2021-10-06 2:07 UTC (permalink / raw) To: Jeremy Linton, Pali Rohár Cc: Bjorn Helgaas, linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On 10/5/2021 3:25 PM, Jeremy Linton wrote: > Hi, > > On 10/5/21 2:43 PM, Pali Rohár wrote: >> Hello! >> >> On Tuesday 05 October 2021 10:57:18 Jeremy Linton wrote: >>> Hi, >>> >>> On 10/5/21 10:32 AM, Bjorn Helgaas wrote: >>>> On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: >>>>> Additionally, some basic bus/device filtering exist to avoid sending >>>>> config transactions to invalid devices on the RP's primary or >>>>> secondary bus. A basic link check is also made to assure that >>>>> something is operational on the secondary side before probing the >>>>> remainder of the config space. If either of these constraints are >>>>> violated and a config operation is lost in the ether because an EP >>>>> doesn't respond an unrecoverable SERROR is raised. >>>> >>>> It's not "lost"; I assume the root port raises an error because it >>>> can't send a transaction over a link that is down. >>> >>> The problem is AFAIK because the root port doesn't do that. >> >> Interesting! Does it mean that PCIe Root Complex / Host Bridge (which I >> guess contains also logic for Root Port) does not signal transaction >> failure for config requests? Or it is just your opinion? Because I'm >> dealing with similar issues and I'm trying to find a way how to detect >> if some PCIe IP signal transaction error via AXI SLVERR response OR it >> just does not send any response back. So if you know some way how to >> check which one it is, I would like to know it too. > > This is my _opinion_ based on what I've heard of some other IP > integration issues, and what i've seen poking at this one from the > perspective of a SW guy rather than a HW guy. So, basically worthless. > But, you should consider that most of these cores/interconnects aren't > aware of PCIe completion semantics so its the root ports responsibility > to say, gracefully translate a non-posted write that doesn't have a > completion for the interconnects its attached to, rather than tripping > something generic like a SLVERR. > > Anyway, for this I would poke around the pile of exception registers, > with your specific processors manual handy because a lot of them are > implementation defined. I should be able to get you an answer in the new few days whether configuration space requests also generate an error towards the ARM CPU, since memory space requests most definitively do. -- Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-06 2:07 ` Florian Fainelli @ 2021-10-22 17:04 ` Florian Fainelli 2021-10-22 17:17 ` Pali Rohár 0 siblings, 1 reply; 28+ messages in thread From: Florian Fainelli @ 2021-10-22 17:04 UTC (permalink / raw) To: Florian Fainelli, Jeremy Linton, Pali Rohár Cc: Bjorn Helgaas, linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On 10/5/21 7:07 PM, Florian Fainelli wrote: > > > On 10/5/2021 3:25 PM, Jeremy Linton wrote: >> Hi, >> >> On 10/5/21 2:43 PM, Pali Rohár wrote: >>> Hello! >>> >>> On Tuesday 05 October 2021 10:57:18 Jeremy Linton wrote: >>>> Hi, >>>> >>>> On 10/5/21 10:32 AM, Bjorn Helgaas wrote: >>>>> On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: >>>>>> Additionally, some basic bus/device filtering exist to avoid sending >>>>>> config transactions to invalid devices on the RP's primary or >>>>>> secondary bus. A basic link check is also made to assure that >>>>>> something is operational on the secondary side before probing the >>>>>> remainder of the config space. If either of these constraints are >>>>>> violated and a config operation is lost in the ether because an EP >>>>>> doesn't respond an unrecoverable SERROR is raised. >>>>> >>>>> It's not "lost"; I assume the root port raises an error because it >>>>> can't send a transaction over a link that is down. >>>> >>>> The problem is AFAIK because the root port doesn't do that. >>> >>> Interesting! Does it mean that PCIe Root Complex / Host Bridge (which I >>> guess contains also logic for Root Port) does not signal transaction >>> failure for config requests? Or it is just your opinion? Because I'm >>> dealing with similar issues and I'm trying to find a way how to detect >>> if some PCIe IP signal transaction error via AXI SLVERR response OR it >>> just does not send any response back. So if you know some way how to >>> check which one it is, I would like to know it too. >> >> This is my _opinion_ based on what I've heard of some other IP >> integration issues, and what i've seen poking at this one from the >> perspective of a SW guy rather than a HW guy. So, basically worthless. >> But, you should consider that most of these cores/interconnects aren't >> aware of PCIe completion semantics so its the root ports >> responsibility to say, gracefully translate a non-posted write that >> doesn't have a completion for the interconnects its attached to, >> rather than tripping something generic like a SLVERR. >> >> Anyway, for this I would poke around the pile of exception registers, >> with your specific processors manual handy because a lot of them are >> implementation defined. > > I should be able to get you an answer in the new few days whether > configuration space requests also generate an error towards the ARM CPU, > since memory space requests most definitively do. Did not get an answer from the design team, but going through our bug tracker, there were evidences of configuration space accesses also generating external aborts: [ 8.988237] Unhandled fault: synchronous external abort (0x96000210) at 0xffffff8009539004 [ 8.996539] Internal error: : 96000210 [#1] SMP [ 9.001107] Modules linked in: [ 9.004215] CPU: 2 PID: 6 Comm: kworker/u8:0 Not tainted 4.9.51-gstb-4.9 #1 [ 9.011216] Hardware name: BCM97278SV (DT) [ 9.015365] Workqueue: events_unbound async_run_entry_fn [ 9.020728] task: ffffffc00a4ab5c0 task.stack: ffffffc00a4e4000 [ 9.026698] PC is at pci_generic_config_read32+0x30/0xb0 [ 9.032053] LR is at pci_generic_config_read32+0x2c/0xb0 [ 9.037403] pc : [<ffffff8008394eb8>] lr : [<ffffff8008394eb4>] pstate: 800000c5 [ 9.044852] sp : ffffffc00a4e7ba0 [ 9.048197] x29: ffffffc00a4e7ba0 x28: ffffffc00a40caa8 [ 9.053574] x27: ffffffc00a40c878 x26: ffffffc00a40c820 [ 9.058949] x25: ffffff800935c77d x24: 0000000000000040 [ 9.064323] x23: ffffff80093d5ac0 x22: ffffffc00a4e7c66 [ 9.069698] x21: ffffffc00a4e7c24 x20: 0000000000000002 [ 9.075072] x19: 000000000000004c x18: ffffffffffffffff [ 9.080448] x17: fffeffffb7ffffff x16: fffffdffb6ffffff [ 9.085822] x15: 0000000000000000 x14: ffffffc009e8fdb8 [ 9.091196] x13: 0000000000000014 x12: 0000000000000000 [ 9.096571] x11: 0000000000000000 x10: 00000000000006d0 [ 9.101946] x9 : ffffffc00a4e4000 x8 : ffffffc00a4abcf0 [ 9.107322] x7 : 0000000000005ec7 x6 : 0000000000000005 [ 9.112696] x5 : ffffff8009530000 x4 : ffffff80087da530 [ 9.118073] x3 : ffffff8009539000 x2 : 000000000000004c [ 9.123448] x1 : 0000000000009004 x0 : ffffff8009539004 [ 9.128823] [ 9.130341] Process kworker/u8:0 (pid: 6, stack limit = 0xffffffc00a4e4020) [ 9.137346] Stack: (0xffffffc00a4e7ba0 to 0xffffffc00a4e8000) [ 9.143136] 7ba0: ffffffc00a4e7bd0 ffffff8008394be4 ffffff8009306000 0000000000000087 [ 9.151029] 7bc0: ffffffc009838000 ffffff800878a170 ffffffc00a4e7c30 ffffff800839b004 [ 9.158923] 7be0: ffffffc00a5bb000 0000000000000000 ffffff8009306000 ffffff80088f7048 [ 9.166814] 7c00: 0000000000000000 ffffffc00a40c800 ffffff80092d7440 000000000000004c [ 9.174706] 7c20: 0000000000000002 0000000000040933 ffffffc00a4e7c70 ffffff800839e570 [ 9.182598] 7c40: ffffffc00a5bb000 ffffff80093d5000 0000000000000000 ffffff80088f7048 [ 9.190490] 7c60: ffffffc00a5bb0a0 0000000000040933 ffffffc00a4e7c90 ffffff80083a1cc4 [ 9.198383] 7c80: ffffffc00a5bb0a0 ffffffc00a5bb000 ffffffc00a4e7cc0 ffffff800845c9cc [ 9.206275] 7ca0: ffffffc00a5bb0a0 ffffff80083a1ca0 0000000000000010 ffffffc00a4ab5c0 [ 9.214167] 7cc0: ffffffc00a4e7d00 ffffff800845cab4 ffffffc00a5bb0a0 0000000000000000 [ 9.222060] 7ce0: 0000000000000010 0000000000000000 ffffffc00a455100 ffffff800845ca80 [ 9.229953] 7d00: ffffffc00a4e7d30 ffffff800845cc84 ffffff80093d9828 ffffffc00a5bb0a0 [ 9.237845] 7d20: ffffffc009decd00 0000000000000000 ffffffc00a4e7d50 ffffff80080ba464 [ 9.245737] 7d40: ffffffc009decd20 ffffff80093a3000 ffffffc00a4e7d80 ffffff80080b0f28 [ 9.253628] 7d60: 0000000000000000 ffffffc00a454e40 ffffffc009decd20 0000000000000000 [ 9.261521] 7d80: ffffffc00a4e7dc0 ffffff80080b1130 ffffffc00a454e40 ffffffc00a40c800 [ 9.269413] 7da0: ffffffc00a454e70 ffffffc00a40c820 ffffff8009306000 ffffffc00a4e4000 [ 9.277305] 7dc0: ffffffc00a4e7e20 ffffff80080b7194 ffffffc00a494400 ffffffc00a4e4000 [ 9.285198] 7de0: ffffff80088afc00 ffffffc00a454e40 ffffff80080b10e8 0000000000000000 [ 9.293088] 7e00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.300981] 7e20: 0000000000000000 ffffff8008082900 ffffff80080b70a0 ffffffc00a494400 [ 9.308872] 7e40: 0000000000000000 0000000000000000 0000000000000000 705afddc1b1ccaa8 [ 9.316764] 7e60: 0000000000000000 ffffff80080bff90 ffffffc00a454e40 ffffffc000000000 [ 9.324656] 7e80: 0000000000000000 ffffffc00a4e7e88 ffffffc00a4e7e88 0000000000000000 [ 9.332547] 7ea0: 0000000000000000 ffffffc00a4e7ea8 ffffffc00a4e7ea8 0000000000040933 [ 9.340438] 7ec0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.348329] 7ee0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.356219] 7f00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.364111] 7f20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.372002] 7f40: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.379894] 7f60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.387785] 7f80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.395675] 7fa0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 9.403566] 7fc0: 0000000000000000 0000000000000005 0000000000000000 0000000000000000 [ 9.411458] 7fe0: 0000000000000000 0000000000000000 46525cd6105c6384 aa9421d6b6fa9074 [ 9.419343] Call trace: [ 9.421821] Exception stack(0xffffffc00a4e79b0 to 0xffffffc00a4e7ae0) [ 9.428301] 79a0: 000000000000004c 0000008000000000 [ 9.436193] 79c0: ffffffc00a4e7ba0 ffffff8008394eb8 00000000800000c5 ffffffc07fe8e440 [ 9.444086] 79e0: ffffffc00a4efd98 0000000000000007 ffffffc000000000 ffffff8009539004 [ 9.451977] 7a00: 0000000000000000 ffffffc009bfbe40 ffffff80092d7440 ffffff80092d7440 [ 9.459869] 7a20: 0000000000000002 ffffff80080c3ee8 ffffffc009bfbe40 ffffffc07fe64440 [ 9.467761] 7a40: 0000000200000000 ffffffc07fe8e440 ffffffc00a4e7a90 ffffff80080c3ee8 [ 9.475652] 7a60: 0000000000000001 0000000000040933 ffffff8009539004 0000000000009004 [ 9.483544] 7a80: 000000000000004c ffffff8009539000 ffffff80087da530 ffffff8009530000 [ 9.491435] 7aa0: 0000000000000005 0000000000005ec7 ffffffc00a4abcf0 ffffffc00a4e4000 [ 9.499327] 7ac0: 00000000000006d0 0000000000000000 0000000000000000 0000000000000014 [ 9.507223] [<ffffff8008394eb8>] pci_generic_config_read32+0x30/0xb0 [ 9.513623] [<ffffff8008394be4>] pci_bus_read_config_word+0x9c/0xc0 [ 9.519936] [<ffffff800839b004>] pci_raw_set_power_state+0x7c/0x248 [ 9.526250] [<ffffff800839e570>] pci_power_up+0x50/0x68 [ 9.531516] [<ffffff80083a1cc4>] pci_pm_resume_noirq+0x24/0xc0 [ 9.537395] [<ffffff800845c9cc>] dpm_run_callback+0x4c/0xc0 [ 9.543008] [<ffffff800845cab4>] device_resume_noirq+0x74/0x220 [ 9.548969] [<ffffff800845cc84>] async_resume_noirq+0x24/0x58 [ 9.554757] [<ffffff80080ba464>] async_run_entry_fn+0x3c/0x160 [ 9.560635] [<ffffff80080b0f28>] process_one_work+0x1d0/0x390 [ 9.566424] [<ffffff80080b1130>] worker_thread+0x48/0x4b0 [ 9.571863] [<ffffff80080b7194>] kthread+0xf4/0x108 [ 9.576782] [<ffffff8008082900>] ret_from_fork+0x10/0x50 [ 9.582136] Code: f9406005 f94008a3 d63f0060 b4000320 (b9400001) [ 9.588311] ---[ end trace efc83c99ae7412ee ]--- -- Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-22 17:04 ` Florian Fainelli @ 2021-10-22 17:17 ` Pali Rohár 2021-10-22 17:29 ` Florian Fainelli 0 siblings, 1 reply; 28+ messages in thread From: Pali Rohár @ 2021-10-22 17:17 UTC (permalink / raw) To: Florian Fainelli Cc: Jeremy Linton, Bjorn Helgaas, linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Friday 22 October 2021 10:04:36 Florian Fainelli wrote: > On 10/5/21 7:07 PM, Florian Fainelli wrote: > > > > > > On 10/5/2021 3:25 PM, Jeremy Linton wrote: > >> Hi, > >> > >> On 10/5/21 2:43 PM, Pali Rohár wrote: > >>> Hello! > >>> > >>> On Tuesday 05 October 2021 10:57:18 Jeremy Linton wrote: > >>>> Hi, > >>>> > >>>> On 10/5/21 10:32 AM, Bjorn Helgaas wrote: > >>>>> On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: > >>>>>> Additionally, some basic bus/device filtering exist to avoid sending > >>>>>> config transactions to invalid devices on the RP's primary or > >>>>>> secondary bus. A basic link check is also made to assure that > >>>>>> something is operational on the secondary side before probing the > >>>>>> remainder of the config space. If either of these constraints are > >>>>>> violated and a config operation is lost in the ether because an EP > >>>>>> doesn't respond an unrecoverable SERROR is raised. > >>>>> > >>>>> It's not "lost"; I assume the root port raises an error because it > >>>>> can't send a transaction over a link that is down. > >>>> > >>>> The problem is AFAIK because the root port doesn't do that. > >>> > >>> Interesting! Does it mean that PCIe Root Complex / Host Bridge (which I > >>> guess contains also logic for Root Port) does not signal transaction > >>> failure for config requests? Or it is just your opinion? Because I'm > >>> dealing with similar issues and I'm trying to find a way how to detect > >>> if some PCIe IP signal transaction error via AXI SLVERR response OR it > >>> just does not send any response back. So if you know some way how to > >>> check which one it is, I would like to know it too. > >> > >> This is my _opinion_ based on what I've heard of some other IP > >> integration issues, and what i've seen poking at this one from the > >> perspective of a SW guy rather than a HW guy. So, basically worthless. > >> But, you should consider that most of these cores/interconnects aren't > >> aware of PCIe completion semantics so its the root ports > >> responsibility to say, gracefully translate a non-posted write that > >> doesn't have a completion for the interconnects its attached to, > >> rather than tripping something generic like a SLVERR. > >> > >> Anyway, for this I would poke around the pile of exception registers, > >> with your specific processors manual handy because a lot of them are > >> implementation defined. > > > > I should be able to get you an answer in the new few days whether > > configuration space requests also generate an error towards the ARM CPU, > > since memory space requests most definitively do. > > Did not get an answer from the design team, but going through our bug > tracker, there were evidences of configuration space accesses also > generating external aborts: > > [ 8.988237] Unhandled fault: synchronous external abort (0x96000210) at 0xffffff8009539004 > [ 9.026698] PC is at pci_generic_config_read32+0x30/0xb0 So this is error caused by reading from config space. Can you check if also writing to config space can trigger some crash? If yes, I would like to know if write would be also synchronous or rather asynchronous abort. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-22 17:17 ` Pali Rohár @ 2021-10-22 17:29 ` Florian Fainelli 2021-10-22 17:57 ` Pali Rohár 0 siblings, 1 reply; 28+ messages in thread From: Florian Fainelli @ 2021-10-22 17:29 UTC (permalink / raw) To: Pali Rohár Cc: Jeremy Linton, Bjorn Helgaas, linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On 10/22/21 10:17 AM, Pali Rohár wrote: > On Friday 22 October 2021 10:04:36 Florian Fainelli wrote: >> On 10/5/21 7:07 PM, Florian Fainelli wrote: >>> >>> >>> On 10/5/2021 3:25 PM, Jeremy Linton wrote: >>>> Hi, >>>> >>>> On 10/5/21 2:43 PM, Pali Rohár wrote: >>>>> Hello! >>>>> >>>>> On Tuesday 05 October 2021 10:57:18 Jeremy Linton wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/5/21 10:32 AM, Bjorn Helgaas wrote: >>>>>>> On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: >>>>>>>> Additionally, some basic bus/device filtering exist to avoid sending >>>>>>>> config transactions to invalid devices on the RP's primary or >>>>>>>> secondary bus. A basic link check is also made to assure that >>>>>>>> something is operational on the secondary side before probing the >>>>>>>> remainder of the config space. If either of these constraints are >>>>>>>> violated and a config operation is lost in the ether because an EP >>>>>>>> doesn't respond an unrecoverable SERROR is raised. >>>>>>> >>>>>>> It's not "lost"; I assume the root port raises an error because it >>>>>>> can't send a transaction over a link that is down. >>>>>> >>>>>> The problem is AFAIK because the root port doesn't do that. >>>>> >>>>> Interesting! Does it mean that PCIe Root Complex / Host Bridge (which I >>>>> guess contains also logic for Root Port) does not signal transaction >>>>> failure for config requests? Or it is just your opinion? Because I'm >>>>> dealing with similar issues and I'm trying to find a way how to detect >>>>> if some PCIe IP signal transaction error via AXI SLVERR response OR it >>>>> just does not send any response back. So if you know some way how to >>>>> check which one it is, I would like to know it too. >>>> >>>> This is my _opinion_ based on what I've heard of some other IP >>>> integration issues, and what i've seen poking at this one from the >>>> perspective of a SW guy rather than a HW guy. So, basically worthless. >>>> But, you should consider that most of these cores/interconnects aren't >>>> aware of PCIe completion semantics so its the root ports >>>> responsibility to say, gracefully translate a non-posted write that >>>> doesn't have a completion for the interconnects its attached to, >>>> rather than tripping something generic like a SLVERR. >>>> >>>> Anyway, for this I would poke around the pile of exception registers, >>>> with your specific processors manual handy because a lot of them are >>>> implementation defined. >>> >>> I should be able to get you an answer in the new few days whether >>> configuration space requests also generate an error towards the ARM CPU, >>> since memory space requests most definitively do. >> >> Did not get an answer from the design team, but going through our bug >> tracker, there were evidences of configuration space accesses also >> generating external aborts: >> >> [ 8.988237] Unhandled fault: synchronous external abort (0x96000210) at 0xffffff8009539004 >> [ 9.026698] PC is at pci_generic_config_read32+0x30/0xb0 > > So this is error caused by reading from config space. > > Can you check if also writing to config space can trigger some crash? If > yes, I would like to know if write would be also synchronous or rather > asynchronous abort. Yes it does and AFAICT it always shows up as a system error interrupt, here is an example: # setpci -d *:* latency_timer=40 [ 25.909644] SError Interrupt on CPU2, code 0xbf000002 -- SError [ 25.909647] CPU: 2 PID: 1676 Comm: setpci Not tainted 5.10.70-0.2pre-ge3872e15011b #2 [ 25.909649] Hardware name: BCM972165SV_V10 (DT) [ 25.909651] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) [ 25.909652] pc : pci_user_write_config_byte+0x6c/0x78 [ 25.909654] lr : pci_user_write_config_byte+0x68/0x78 [ 25.909655] sp : ffffffc015853c20 [ 25.909656] x29: ffffffc015853c20 x28: ffffff8003053000 [ 25.909661] x27: 0000000000000000 x26: 0000000000000000 [ 25.909664] x25: 0000000000000001 x24: ffffff8004a23780 [ 25.909668] x23: ffffff80049aa000 x22: ffffffc015853d68 [ 25.909671] x21: 0000000000000040 x20: 000000000000000d [ 25.909674] x19: 000000000000000e x18: 0000000000000000 [ 25.909677] x17: 0000000000000000 x16: 0000000000000000 [ 25.909680] x15: 0000000000000000 x14: 0000000000000000 [ 25.909684] x13: 0000000000000000 x12: 0000000000000000 [ 25.909687] x11: 0000000000000000 x10: 0000000000000000 [ 25.909690] x9 : ffffffc010483214 x8 : 0000000000000000 [ 25.909693] x7 : ffffff800498df00 x6 : ffffff80049a8380 [ 25.909696] x5 : ffffffc015510000 x4 : ffffff80049a9800 [ 25.909699] x3 : 0000000000000000 x2 : 000000000000000d [ 25.909702] x1 : 0000000000000000 x0 : 0000000000000000 [ 25.909706] Kernel panic - not syncing: Asynchronous SError Interrupt [ 25.909708] CPU: 2 PID: 1676 Comm: setpci Not tainted 5.10.70-0.2pre-ge3872e15011b #2 [ 25.909710] Hardware name: BCM972165SV_V10 (DT) [ 25.909711] Call trace: [ 25.909712] dump_backtrace+0x0/0x1d0 [ 25.909713] show_stack+0x1c/0x24 [ 25.909714] dump_stack+0xd0/0x12c [ 25.909716] panic+0x128/0x308 [ 25.909717] nmi_panic+0x50/0x70 [ 25.909718] arm64_serror_panic+0x74/0x80 [ 25.909720] do_serror+0x28/0x60 [ 25.909721] el1_error+0x8c/0x10c [ 25.909722] pci_user_write_config_byte+0x6c/0x78 [ 25.909724] pci_write_config+0x7c/0x1a0 [ 25.909725] sysfs_kf_bin_write+0x64/0x84 [ 25.909727] kernfs_fop_write_iter+0xbc/0x170 [ 25.909728] new_sync_write+0x80/0xcc [ 25.909729] vfs_write+0xec/0x110 [ 25.909730] ksys_pwrite64+0x50/0x8c [ 25.909732] __arm64_sys_pwrite64+0x20/0x28 [ 25.909733] el0_svc_common.constprop.4+0x100/0x184 [ 25.909735] do_el0_svc+0x38/0x78 [ 25.909736] el0_svc+0x1c/0x28 [ 25.909737] el0_sync_handler+0x64/0x12c [ 25.909738] el0_sync+0x148/0x180 [ 25.909775] brcm-pcie 8b20000.pcie: Error: CFG Acc, 32bit, Write, Bus=1, Dev=0, Fun=0, Reg=0xc, lanes=01000000 [ 26.136082] brcm-pcie 8b20000.pcie: Type: TO=0 Abt=0 UnsupReq=0 AccTO=0 AccDsbld=1 Acc64bit=0 [ 26.144709] SMP: stopping secondary CPUs [ 26.144711] Kernel Offset: disabled [ 26.144712] CPU features: 0x0040002,24002004 [ 26.144713] Memory Limit: none -- Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk 2021-10-22 17:29 ` Florian Fainelli @ 2021-10-22 17:57 ` Pali Rohár 0 siblings, 0 replies; 28+ messages in thread From: Pali Rohár @ 2021-10-22 17:57 UTC (permalink / raw) To: Florian Fainelli Cc: Jeremy Linton, Bjorn Helgaas, linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Friday 22 October 2021 10:29:48 Florian Fainelli wrote: > On 10/22/21 10:17 AM, Pali Rohár wrote: > > On Friday 22 October 2021 10:04:36 Florian Fainelli wrote: > >> On 10/5/21 7:07 PM, Florian Fainelli wrote: > >>> > >>> > >>> On 10/5/2021 3:25 PM, Jeremy Linton wrote: > >>>> Hi, > >>>> > >>>> On 10/5/21 2:43 PM, Pali Rohár wrote: > >>>>> Hello! > >>>>> > >>>>> On Tuesday 05 October 2021 10:57:18 Jeremy Linton wrote: > >>>>>> Hi, > >>>>>> > >>>>>> On 10/5/21 10:32 AM, Bjorn Helgaas wrote: > >>>>>>> On Thu, Aug 26, 2021 at 02:15:55AM -0500, Jeremy Linton wrote: > >>>>>>>> Additionally, some basic bus/device filtering exist to avoid sending > >>>>>>>> config transactions to invalid devices on the RP's primary or > >>>>>>>> secondary bus. A basic link check is also made to assure that > >>>>>>>> something is operational on the secondary side before probing the > >>>>>>>> remainder of the config space. If either of these constraints are > >>>>>>>> violated and a config operation is lost in the ether because an EP > >>>>>>>> doesn't respond an unrecoverable SERROR is raised. > >>>>>>> > >>>>>>> It's not "lost"; I assume the root port raises an error because it > >>>>>>> can't send a transaction over a link that is down. > >>>>>> > >>>>>> The problem is AFAIK because the root port doesn't do that. > >>>>> > >>>>> Interesting! Does it mean that PCIe Root Complex / Host Bridge (which I > >>>>> guess contains also logic for Root Port) does not signal transaction > >>>>> failure for config requests? Or it is just your opinion? Because I'm > >>>>> dealing with similar issues and I'm trying to find a way how to detect > >>>>> if some PCIe IP signal transaction error via AXI SLVERR response OR it > >>>>> just does not send any response back. So if you know some way how to > >>>>> check which one it is, I would like to know it too. > >>>> > >>>> This is my _opinion_ based on what I've heard of some other IP > >>>> integration issues, and what i've seen poking at this one from the > >>>> perspective of a SW guy rather than a HW guy. So, basically worthless. > >>>> But, you should consider that most of these cores/interconnects aren't > >>>> aware of PCIe completion semantics so its the root ports > >>>> responsibility to say, gracefully translate a non-posted write that > >>>> doesn't have a completion for the interconnects its attached to, > >>>> rather than tripping something generic like a SLVERR. > >>>> > >>>> Anyway, for this I would poke around the pile of exception registers, > >>>> with your specific processors manual handy because a lot of them are > >>>> implementation defined. > >>> > >>> I should be able to get you an answer in the new few days whether > >>> configuration space requests also generate an error towards the ARM CPU, > >>> since memory space requests most definitively do. > >> > >> Did not get an answer from the design team, but going through our bug > >> tracker, there were evidences of configuration space accesses also > >> generating external aborts: > >> > >> [ 8.988237] Unhandled fault: synchronous external abort (0x96000210) at 0xffffff8009539004 > >> [ 9.026698] PC is at pci_generic_config_read32+0x30/0xb0 > > > > So this is error caused by reading from config space. > > > > Can you check if also writing to config space can trigger some crash? If > > yes, I would like to know if write would be also synchronous or rather > > asynchronous abort. > > Yes it does and AFAICT it always shows up as a system error interrupt, > here is an example: > > # setpci -d *:* latency_timer=40 > [ 25.909644] SError Interrupt on CPU2, code 0xbf000002 -- SError > [ 25.909652] pc : pci_user_write_config_byte+0x6c/0x78 > [ 25.909706] Kernel panic - not syncing: Asynchronous SError Interrupt Ok! So writing to config space cause asynchronous abort. Looking at the codes and 0x96000210 on all ARMv8 should be Data Abort. 0xbf...... on ARMv8 is SError interrupt and other bits are CPU core specific. What CPU core do you have on this machine? I have just decoder for A53 core and on this core value 0xbf000002 means "SLVERR on external access". But I guess that it would mean also SLVERR for your CPU core. Because Exactly same behavior I'm seeing with PCIe controller on A3720 SoC which has A53 core. It looks like that PCIe controller translates PCIe CA and UR responses to AXI SLVERR responses which are delivered to CPU and kernel just see these fatal error interrupts. And same issue is not only for config requests but also for memory read / write commands. In my case PCIe controller really receives response (timeout does not occur) from PCIe core (which probably timeouts as it cannot send message when link is down) but instead of translating them to SLVOK with fabricated 0xffffffff response it sends to CPU that fatal SLVERR. I was told that the fix for this kind of issue is to "reconfigure" PCIe controller to never send SLVERR to CPU. And instead fabricate 0xffffffff SLVOK response. It should be configurable in PCIe wrapper or PCIe glue IP which do connection between CPU / AXI and PCIe core. I do not know if there is any way how to "ignores" these SLVERR responses from PCIe controller sent to CPU. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-08-26 7:15 [PATCH v3 0/4] CM4 ACPI PCIe quirk Jeremy Linton 2021-08-26 7:15 ` [PATCH v3 1/4] PCI: brcmstb: Break register definitions into separate header Jeremy Linton 2021-08-26 7:15 ` [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk Jeremy Linton @ 2021-08-26 7:15 ` Jeremy Linton 2021-08-30 8:37 ` nicolas saenz julienne ` (3 more replies) 2021-08-26 7:15 ` [PATCH v3 4/4] MAINTAINERS: Widen brcmstb PCIe file scope Jeremy Linton 3 siblings, 4 replies; 28+ messages in thread From: Jeremy Linton @ 2021-08-26 7:15 UTC (permalink / raw) To: linux-pci Cc: lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel, Jeremy Linton Now that there is a bcm2711 quirk, it needs to be enabled when the MCFG is missing. Use an ACPI namespace _DSD property "linux-ecam-quirk-id" as an alternative to the MCFG OEM. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 53cab975f612..04c517418365 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { ALTRA_ECAM_QUIRK(1, 13), ALTRA_ECAM_QUIRK(1, 14), ALTRA_ECAM_QUIRK(1, 15), + + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, + DEFINE_RES_MEM(0xFD500000, 0xA000) }, }; static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, u16 segment = root->segment; struct resource *bus_range = &root->secondary; struct mcfg_fixup *f; + const char *soc; int i; + /* + * This may be a machine with a PCI/SMC conduit, which means it doesn't + * have an MCFG. Use an ACPI namespace definition instead. + */ + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), + "linux-ecam-quirk-id", &soc)) { + if (strlen(soc) != ACPI_OEM_ID_SIZE) + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", + ACPI_OEM_ID_SIZE); + else + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); + } + for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { if (pci_mcfg_quirk_matches(f, segment, bus_range)) { if (f->cfgres.start) -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-08-26 7:15 ` [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk Jeremy Linton @ 2021-08-30 8:37 ` nicolas saenz julienne 2021-09-13 16:12 ` Rafael J. Wysocki ` (2 subsequent siblings) 3 siblings, 0 replies; 28+ messages in thread From: nicolas saenz julienne @ 2021-08-30 8:37 UTC (permalink / raw) To: Jeremy Linton, linux-pci Cc: lorenzo.pieralisi, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote: > Now that there is a bcm2711 quirk, it needs to be enabled when the > MCFG is missing. Use an ACPI namespace _DSD property > "linux-ecam-quirk-id" as an alternative to the MCFG OEM. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- Acked-by: Nicolas Saenz Julienne <nsaenz@kernel.org> Regards, Nicolas ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-08-26 7:15 ` [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk Jeremy Linton 2021-08-30 8:37 ` nicolas saenz julienne @ 2021-09-13 16:12 ` Rafael J. Wysocki 2021-10-05 15:10 ` Bjorn Helgaas 2021-10-05 20:02 ` Pali Rohár 3 siblings, 0 replies; 28+ messages in thread From: Rafael J. Wysocki @ 2021-09-13 16:12 UTC (permalink / raw) To: Jeremy Linton Cc: Linux PCI, Lorenzo Pieralisi, nsaenz, Bjorn Helgaas, Rafael J. Wysocki, Len Brown, Rob Herring, Krzysztof Wilczyński, Florian Fainelli, bcm-kernel-feedback-list, ACPI Devel Maling List, Linux ARM, linux-rpi-kernel, Linux Kernel Mailing List On Thu, Aug 26, 2021 at 9:16 AM Jeremy Linton <jeremy.linton@arm.com> wrote: > > Now that there is a bcm2711 quirk, it needs to be enabled when the > MCFG is missing. Use an ACPI namespace _DSD property > "linux-ecam-quirk-id" as an alternative to the MCFG OEM. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > index 53cab975f612..04c517418365 100644 > --- a/drivers/acpi/pci_mcfg.c > +++ b/drivers/acpi/pci_mcfg.c > @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { > ALTRA_ECAM_QUIRK(1, 13), > ALTRA_ECAM_QUIRK(1, 14), > ALTRA_ECAM_QUIRK(1, 15), > + > + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, > + DEFINE_RES_MEM(0xFD500000, 0xA000) }, > }; > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, > u16 segment = root->segment; > struct resource *bus_range = &root->secondary; > struct mcfg_fixup *f; > + const char *soc; > int i; > > + /* > + * This may be a machine with a PCI/SMC conduit, which means it doesn't > + * have an MCFG. Use an ACPI namespace definition instead. > + */ > + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), > + "linux-ecam-quirk-id", &soc)) { > + if (strlen(soc) != ACPI_OEM_ID_SIZE) > + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", > + ACPI_OEM_ID_SIZE); > + else > + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); > + } > + > for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { > if (pci_mcfg_quirk_matches(f, segment, bus_range)) { > if (f->cfgres.start) > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-08-26 7:15 ` [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk Jeremy Linton 2021-08-30 8:37 ` nicolas saenz julienne 2021-09-13 16:12 ` Rafael J. Wysocki @ 2021-10-05 15:10 ` Bjorn Helgaas 2021-10-05 15:43 ` Jeremy Linton 2021-10-05 20:02 ` Pali Rohár 3 siblings, 1 reply; 28+ messages in thread From: Bjorn Helgaas @ 2021-10-05 15:10 UTC (permalink / raw) To: Jeremy Linton Cc: linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote: > Now that there is a bcm2711 quirk, it needs to be enabled when the > MCFG is missing. Use an ACPI namespace _DSD property > "linux-ecam-quirk-id" as an alternative to the MCFG OEM. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > index 53cab975f612..04c517418365 100644 > --- a/drivers/acpi/pci_mcfg.c > +++ b/drivers/acpi/pci_mcfg.c > @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { > ALTRA_ECAM_QUIRK(1, 13), > ALTRA_ECAM_QUIRK(1, 14), > ALTRA_ECAM_QUIRK(1, 15), > + > + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, > + DEFINE_RES_MEM(0xFD500000, 0xA000) }, > }; > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, > u16 segment = root->segment; > struct resource *bus_range = &root->secondary; > struct mcfg_fixup *f; > + const char *soc; > int i; > > + /* > + * This may be a machine with a PCI/SMC conduit, which means it doesn't > + * have an MCFG. Use an ACPI namespace definition instead. > + */ > + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), > + "linux-ecam-quirk-id", &soc)) { > + if (strlen(soc) != ACPI_OEM_ID_SIZE) From a reviewing perspective, it's not obvious why soc must be exactly ACPI_OEM_ID_SIZE. Does that imply space-padding in the DT string or something? Is there any documentation for this DT property? Also not obvious why strlen() is safe here. I mean, I looked a couple levels deep in fwnode_property_read_string(), but whatever guarantees null termination is buried pretty deep. It seems a little weird to use an MCFG quirk mechanism when there's no MCFG at all on this platform. > + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", > + ACPI_OEM_ID_SIZE); > + else > + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); > + } > + > for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { > if (pci_mcfg_quirk_matches(f, segment, bus_range)) { > if (f->cfgres.start) > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-10-05 15:10 ` Bjorn Helgaas @ 2021-10-05 15:43 ` Jeremy Linton 2021-10-05 22:31 ` Bjorn Helgaas 0 siblings, 1 reply; 28+ messages in thread From: Jeremy Linton @ 2021-10-05 15:43 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hi, Thanks for looking at this again. On 10/5/21 10:10 AM, Bjorn Helgaas wrote: > On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote: >> Now that there is a bcm2711 quirk, it needs to be enabled when the >> MCFG is missing. Use an ACPI namespace _DSD property >> "linux-ecam-quirk-id" as an alternative to the MCFG OEM. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> Acked-by: Florian Fainelli <f.fainelli@gmail.com> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >> --- >> drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >> index 53cab975f612..04c517418365 100644 >> --- a/drivers/acpi/pci_mcfg.c >> +++ b/drivers/acpi/pci_mcfg.c >> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { >> ALTRA_ECAM_QUIRK(1, 13), >> ALTRA_ECAM_QUIRK(1, 14), >> ALTRA_ECAM_QUIRK(1, 15), >> + >> + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, >> + DEFINE_RES_MEM(0xFD500000, 0xA000) }, >> }; >> >> static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; >> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, >> u16 segment = root->segment; >> struct resource *bus_range = &root->secondary; >> struct mcfg_fixup *f; >> + const char *soc; >> int i; >> >> + /* >> + * This may be a machine with a PCI/SMC conduit, which means it doesn't >> + * have an MCFG. Use an ACPI namespace definition instead. >> + */ >> + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), >> + "linux-ecam-quirk-id", &soc)) { >> + if (strlen(soc) != ACPI_OEM_ID_SIZE) > > From a reviewing perspective, it's not obvious why soc must be exactly > ACPI_OEM_ID_SIZE. Does that imply space-padding in the DT string or > something? This is at the moment an ACPI only DSD, and it must follow the MADT OEM_ID format for now because we are effectively just overriding that field. The rest of the code in this module is just treating it as a fixed 6 bytes. > > Is there any documentation for this DT property? Its not a DT property, and its unclear since its linux only if it belongs in previously linked ACPI registry. > > Also not obvious why strlen() is safe here. I mean, I looked a couple > levels deep in fwnode_property_read_string(), but whatever guarantees > null termination is buried pretty deep. I've not tracked down who, if anyone other than the AML compiler is guaranteeing a null. The spec says something to the effect "Most other string, however, are of variable-length and are automatically null terminated by the compiler". Not sure if that helps any. > > It seems a little weird to use an MCFG quirk mechanism when there's no > MCFG at all on this platform. Well its just a point to hook in a CFG space quirk, and since that is what most of the MCFG quirks are, it seemed reasonable to reuse it rather than recreate it. PS, had some offline convo about reposing with a simple rebase and the ACK's applied, will do that if it helps any, but when I checked a couple weeks back this applied to 5.15 automatically. > >> + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", >> + ACPI_OEM_ID_SIZE); >> + else >> + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); >> + } >> + >> for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { >> if (pci_mcfg_quirk_matches(f, segment, bus_range)) { >> if (f->cfgres.start) >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-10-05 15:43 ` Jeremy Linton @ 2021-10-05 22:31 ` Bjorn Helgaas 2021-10-05 23:32 ` Jeremy Linton 0 siblings, 1 reply; 28+ messages in thread From: Bjorn Helgaas @ 2021-10-05 22:31 UTC (permalink / raw) To: Jeremy Linton Cc: linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Tue, Oct 05, 2021 at 10:43:32AM -0500, Jeremy Linton wrote: > On 10/5/21 10:10 AM, Bjorn Helgaas wrote: > > On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote: > > > Now that there is a bcm2711 quirk, it needs to be enabled when the > > > MCFG is missing. Use an ACPI namespace _DSD property > > > "linux-ecam-quirk-id" as an alternative to the MCFG OEM. > > > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > --- > > > drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ > > > 1 file changed, 17 insertions(+) > > > > > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > > > index 53cab975f612..04c517418365 100644 > > > --- a/drivers/acpi/pci_mcfg.c > > > +++ b/drivers/acpi/pci_mcfg.c > > > @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { > > > ALTRA_ECAM_QUIRK(1, 13), > > > ALTRA_ECAM_QUIRK(1, 14), > > > ALTRA_ECAM_QUIRK(1, 15), > > > + > > > + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, > > > + DEFINE_RES_MEM(0xFD500000, 0xA000) }, > > > }; > > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > > > @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, > > > u16 segment = root->segment; > > > struct resource *bus_range = &root->secondary; > > > struct mcfg_fixup *f; > > > + const char *soc; > > > int i; > > > + /* > > > + * This may be a machine with a PCI/SMC conduit, which means it doesn't > > > + * have an MCFG. Use an ACPI namespace definition instead. > > > + */ > > > + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), > > > + "linux-ecam-quirk-id", &soc)) { > > > + if (strlen(soc) != ACPI_OEM_ID_SIZE) > > > > From a reviewing perspective, it's not obvious why soc must be exactly > > ACPI_OEM_ID_SIZE. Does that imply space-padding in the DT string or > > something? > > This is at the moment an ACPI only DSD, and it must follow the MADT OEM_ID > format for now because we are effectively just overriding that field. The > rest of the code in this module is just treating it as a fixed 6 bytes. > > > Is there any documentation for this DT property? > > Its not a DT property, and its unclear since its linux only if it > belongs in previously linked ACPI registry. Oh, right, it comes from a _DSD. > > Also not obvious why strlen() is safe here. I mean, I looked a couple > > levels deep in fwnode_property_read_string(), but whatever guarantees > > null termination is buried pretty deep. > > I've not tracked down who, if anyone other than the AML compiler is > guaranteeing a null. The spec says something to the effect "Most other > string, however, are of variable-length and are automatically null > terminated by the compiler". Not sure if that helps any. Doesn't help for me. The PCI core shouldn't go in the weeds no matter what junk we might get from an AML compiler. Maybe fwnode_property_read_string() guarantees null termination, but it's not documented and not easy to verify. I think a strncpy() here might be better. Not sure it's worthwhile to emit a specific error message for the wrong length. > > It seems a little weird to use an MCFG quirk mechanism when there's no > > MCFG at all on this platform. > > Well its just a point to hook in a CFG space quirk, and since that > is what most of the MCFG quirks are, it seemed reasonable to reuse > it rather than recreate it. Yeah, it's ugly no matter how we slice it. The pci_no_msi() especially has nothing to do with ECAM at all. But I don't know how to identify this thing for a quirk. PNP0A08 devices really rely on ECAM or a system firmware config accessor. > > > + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", > > > + ACPI_OEM_ID_SIZE); > > > + else > > > + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); > > > + } > > > + > > > for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { > > > if (pci_mcfg_quirk_matches(f, segment, bus_range)) { > > > if (f->cfgres.start) > > > -- > > > 2.31.1 > > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-10-05 22:31 ` Bjorn Helgaas @ 2021-10-05 23:32 ` Jeremy Linton 0 siblings, 0 replies; 28+ messages in thread From: Jeremy Linton @ 2021-10-05 23:32 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hi, On 10/5/21 5:31 PM, Bjorn Helgaas wrote: > On Tue, Oct 05, 2021 at 10:43:32AM -0500, Jeremy Linton wrote: >> On 10/5/21 10:10 AM, Bjorn Helgaas wrote: >>> On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote: >>>> Now that there is a bcm2711 quirk, it needs to be enabled when the >>>> MCFG is missing. Use an ACPI namespace _DSD property >>>> "linux-ecam-quirk-id" as an alternative to the MCFG OEM. >>>> >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>> Acked-by: Florian Fainelli <f.fainelli@gmail.com> >>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >>>> --- >>>> drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ >>>> 1 file changed, 17 insertions(+) >>>> >>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >>>> index 53cab975f612..04c517418365 100644 >>>> --- a/drivers/acpi/pci_mcfg.c >>>> +++ b/drivers/acpi/pci_mcfg.c >>>> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { >>>> ALTRA_ECAM_QUIRK(1, 13), >>>> ALTRA_ECAM_QUIRK(1, 14), >>>> ALTRA_ECAM_QUIRK(1, 15), >>>> + >>>> + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, >>>> + DEFINE_RES_MEM(0xFD500000, 0xA000) }, >>>> }; >>>> static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; >>>> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, >>>> u16 segment = root->segment; >>>> struct resource *bus_range = &root->secondary; >>>> struct mcfg_fixup *f; >>>> + const char *soc; >>>> int i; >>>> + /* >>>> + * This may be a machine with a PCI/SMC conduit, which means it doesn't >>>> + * have an MCFG. Use an ACPI namespace definition instead. >>>> + */ >>>> + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), >>>> + "linux-ecam-quirk-id", &soc)) { >>>> + if (strlen(soc) != ACPI_OEM_ID_SIZE) >>> >>> From a reviewing perspective, it's not obvious why soc must be exactly >>> ACPI_OEM_ID_SIZE. Does that imply space-padding in the DT string or >>> something? >> >> This is at the moment an ACPI only DSD, and it must follow the MADT OEM_ID >> format for now because we are effectively just overriding that field. The >> rest of the code in this module is just treating it as a fixed 6 bytes. >> >>> Is there any documentation for this DT property? >> >> Its not a DT property, and its unclear since its linux only if it >> belongs in previously linked ACPI registry. > > Oh, right, it comes from a _DSD. > >>> Also not obvious why strlen() is safe here. I mean, I looked a couple >>> levels deep in fwnode_property_read_string(), but whatever guarantees >>> null termination is buried pretty deep. >> >> I've not tracked down who, if anyone other than the AML compiler is >> guaranteeing a null. The spec says something to the effect "Most other >> string, however, are of variable-length and are automatically null >> terminated by the compiler". Not sure if that helps any. > > Doesn't help for me. The PCI core shouldn't go in the weeds no matter > what junk we might get from an AML compiler. Maybe > fwnode_property_read_string() guarantees null termination, but it's > not documented and not easy to verify. > > I think a strncpy() here might be better. Not sure it's worthwhile to > emit a specific error message for the wrong length. I think we went around about this a bit, but yes strncpy() is exactly what we want because the rest of the code assumes 6 non-null terminated characters and strncpy won't terminate it if its longer. OTOH, i'm not sure we really want shorter strings padded with nulls either. strncpy() is easy though, so sure. <shrug> > >>> It seems a little weird to use an MCFG quirk mechanism when there's no >>> MCFG at all on this platform. >> >> Well its just a point to hook in a CFG space quirk, and since that >> is what most of the MCFG quirks are, it seemed reasonable to reuse >> it rather than recreate it. > > Yeah, it's ugly no matter how we slice it. The pci_no_msi() > especially has nothing to do with ECAM at all. But I don't know how > to identify this thing for a quirk. PNP0A08 devices really rely on > ECAM or a system firmware config accessor. > >>>> + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", >>>> + ACPI_OEM_ID_SIZE); >>>> + else >>>> + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); >>>> + } >>>> + >>>> for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { >>>> if (pci_mcfg_quirk_matches(f, segment, bus_range)) { >>>> if (f->cfgres.start) >>>> -- >>>> 2.31.1 >>>> >> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-08-26 7:15 ` [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk Jeremy Linton ` (2 preceding siblings ...) 2021-10-05 15:10 ` Bjorn Helgaas @ 2021-10-05 20:02 ` Pali Rohár 2021-10-05 22:44 ` Jeremy Linton 3 siblings, 1 reply; 28+ messages in thread From: Pali Rohár @ 2021-10-05 20:02 UTC (permalink / raw) To: bhelgaas, Jeremy Linton Cc: linux-pci, lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Thursday 26 August 2021 02:15:56 Jeremy Linton wrote: > Now that there is a bcm2711 quirk, it needs to be enabled when the > MCFG is missing. Use an ACPI namespace _DSD property > "linux-ecam-quirk-id" as an alternative to the MCFG OEM. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > index 53cab975f612..04c517418365 100644 > --- a/drivers/acpi/pci_mcfg.c > +++ b/drivers/acpi/pci_mcfg.c > @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { > ALTRA_ECAM_QUIRK(1, 13), > ALTRA_ECAM_QUIRK(1, 14), > ALTRA_ECAM_QUIRK(1, 15), > + > + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, > + DEFINE_RES_MEM(0xFD500000, 0xA000) }, Hello! According to discussion described in email [1], adding a new MCFG quirk (like above) into kernel requires adding some errata entry for documenting buggy HW. But this patch series does not introduce any new errata entry. Bjorn, could you look at how to properly document these "hw bugs"? I guess there would be lot of more requests for adding MCFG quirks as now according to [1], doors are open for them. And it is possible that after more years nobody would be able to maintain these quirks if would not be properly documented. [1] - https://lore.kernel.org/linux-pci/20210325131231.GA18590@e121166-lin.cambridge.arm.com/ > }; > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, > u16 segment = root->segment; > struct resource *bus_range = &root->secondary; > struct mcfg_fixup *f; > + const char *soc; > int i; > > + /* > + * This may be a machine with a PCI/SMC conduit, which means it doesn't > + * have an MCFG. Use an ACPI namespace definition instead. > + */ > + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), > + "linux-ecam-quirk-id", &soc)) { > + if (strlen(soc) != ACPI_OEM_ID_SIZE) > + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", > + ACPI_OEM_ID_SIZE); > + else > + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); > + } > + > for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { > if (pci_mcfg_quirk_matches(f, segment, bus_range)) { > if (f->cfgres.start) > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk 2021-10-05 20:02 ` Pali Rohár @ 2021-10-05 22:44 ` Jeremy Linton 0 siblings, 0 replies; 28+ messages in thread From: Jeremy Linton @ 2021-10-05 22:44 UTC (permalink / raw) To: Pali Rohár, bhelgaas Cc: linux-pci, lorenzo.pieralisi, nsaenz, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel Hi, On 10/5/21 3:02 PM, Pali Rohár wrote: > On Thursday 26 August 2021 02:15:56 Jeremy Linton wrote: >> Now that there is a bcm2711 quirk, it needs to be enabled when the >> MCFG is missing. Use an ACPI namespace _DSD property >> "linux-ecam-quirk-id" as an alternative to the MCFG OEM. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> Acked-by: Florian Fainelli <f.fainelli@gmail.com> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >> --- >> drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >> index 53cab975f612..04c517418365 100644 >> --- a/drivers/acpi/pci_mcfg.c >> +++ b/drivers/acpi/pci_mcfg.c >> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = { >> ALTRA_ECAM_QUIRK(1, 13), >> ALTRA_ECAM_QUIRK(1, 14), >> ALTRA_ECAM_QUIRK(1, 15), >> + >> + { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops, >> + DEFINE_RES_MEM(0xFD500000, 0xA000) }, > > Hello! > > According to discussion described in email [1], adding a new MCFG quirk > (like above) into kernel requires adding some errata entry for > documenting buggy HW. > > But this patch series does not introduce any new errata entry. Nor did the quirk following that email, which was merged. Presumably because I'm not sure anyone can predict the future, and some of the points in that posting were made simply to fill gaps in the arguments around why the SMC is/was a better plan. The final point about requiring a MCFG is also flies in the face of the fact that there is an Arm/Pci Standardized way to deal with these problems which is supported by other OS's running on these platforms and it requires that MFCG is missing. The idea that MCFG will magically appear for only Linux and contain relevant info (like the non even power of two cfg region on this SoC) is a bit wishful. I'm certainly in no place to claim future PCIe compliance for Broadcom's SoC's, nor its lifecycle. I'm betting few people submitting Linux patches are, particularly for mostly community supported projects like the RPi4/CM4/ACPI port. I'm more than happy to hack/tweak future problems with this code, and possibly even extend it if needed. Thats about the extent of what I can do. Thanks, > > Bjorn, could you look at how to properly document these "hw bugs"? > I guess there would be lot of more requests for adding MCFG quirks as > now according to [1], doors are open for them. And it is possible that > after more years nobody would be able to maintain these quirks if would > not be properly documented. > > [1] - https://lore.kernel.org/linux-pci/20210325131231.GA18590@e121166-lin.cambridge.arm.com/ > >> }; >> >> static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; >> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, >> u16 segment = root->segment; >> struct resource *bus_range = &root->secondary; >> struct mcfg_fixup *f; >> + const char *soc; >> int i; >> >> + /* >> + * This may be a machine with a PCI/SMC conduit, which means it doesn't >> + * have an MCFG. Use an ACPI namespace definition instead. >> + */ >> + if (!fwnode_property_read_string(acpi_fwnode_handle(root->device), >> + "linux-ecam-quirk-id", &soc)) { >> + if (strlen(soc) != ACPI_OEM_ID_SIZE) >> + dev_err(&root->device->dev, "ECAM quirk should be %d characters\n", >> + ACPI_OEM_ID_SIZE); >> + else >> + memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE); >> + } >> + >> for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { >> if (pci_mcfg_quirk_matches(f, segment, bus_range)) { >> if (f->cfgres.start) >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 4/4] MAINTAINERS: Widen brcmstb PCIe file scope 2021-08-26 7:15 [PATCH v3 0/4] CM4 ACPI PCIe quirk Jeremy Linton ` (2 preceding siblings ...) 2021-08-26 7:15 ` [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk Jeremy Linton @ 2021-08-26 7:15 ` Jeremy Linton 2021-08-30 8:38 ` nicolas saenz julienne 3 siblings, 1 reply; 28+ messages in thread From: Jeremy Linton @ 2021-08-26 7:15 UTC (permalink / raw) To: linux-pci Cc: lorenzo.pieralisi, nsaenz, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel, Jeremy Linton The brcmstb PCI hardware is now split across multiple files. Include them in the maintainers block. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> --- MAINTAINERS | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index eee4c926003b..ec2c7a294495 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3514,7 +3514,7 @@ L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/nsaenz/linux-rpi.git F: Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml -F: drivers/pci/controller/pcie-brcmstb.c +F: drivers/pci/controller/pcie-brcmstb* F: drivers/staging/vc04_services N: bcm2711 N: bcm283* @@ -3595,7 +3595,7 @@ F: arch/arm/include/asm/hardware/cache-b15-rac.h F: arch/arm/mach-bcm/*brcmstb* F: arch/arm/mm/cache-b15-rac.c F: drivers/bus/brcmstb_gisb.c -F: drivers/pci/controller/pcie-brcmstb.c +F: drivers/pci/controller/pcie-brcmstb* N: brcmstb BROADCOM BDC DRIVER @@ -3888,7 +3888,7 @@ M: bcm-kernel-feedback-list@broadcom.com L: linux-pci@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml -F: drivers/pci/controller/pcie-brcmstb.c +F: drivers/pci/controller/pcie-brcmstb* BROADCOM SYSTEMPORT ETHERNET DRIVER M: Florian Fainelli <f.fainelli@gmail.com> -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/4] MAINTAINERS: Widen brcmstb PCIe file scope 2021-08-26 7:15 ` [PATCH v3 4/4] MAINTAINERS: Widen brcmstb PCIe file scope Jeremy Linton @ 2021-08-30 8:38 ` nicolas saenz julienne 0 siblings, 0 replies; 28+ messages in thread From: nicolas saenz julienne @ 2021-08-30 8:38 UTC (permalink / raw) To: Jeremy Linton, linux-pci Cc: lorenzo.pieralisi, bhelgaas, rjw, lenb, robh, kw, f.fainelli, bcm-kernel-feedback-list, linux-acpi, linux-arm-kernel, linux-rpi-kernel, linux-kernel On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote: > The brcmstb PCI hardware is now split across multiple files. Include > them in the maintainers block. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- Acked-by: Nicolas Saenz Julienne <nsaenz@kernel.org> Regards, Nicolas ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-10-22 17:57 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-26 7:15 [PATCH v3 0/4] CM4 ACPI PCIe quirk Jeremy Linton 2021-08-26 7:15 ` [PATCH v3 1/4] PCI: brcmstb: Break register definitions into separate header Jeremy Linton 2021-08-30 8:37 ` nicolas saenz julienne 2021-08-26 7:15 ` [PATCH v3 2/4] PCI: brcmstb: Add ACPI config space quirk Jeremy Linton 2021-08-30 8:36 ` nicolas saenz julienne 2021-08-30 16:23 ` Jeremy Linton 2021-08-30 16:27 ` Florian Fainelli 2021-08-30 17:17 ` nicolas saenz julienne 2021-10-05 15:32 ` Bjorn Helgaas 2021-10-05 15:57 ` Jeremy Linton 2021-10-05 19:43 ` Pali Rohár 2021-10-05 22:25 ` Jeremy Linton 2021-10-06 2:07 ` Florian Fainelli 2021-10-22 17:04 ` Florian Fainelli 2021-10-22 17:17 ` Pali Rohár 2021-10-22 17:29 ` Florian Fainelli 2021-10-22 17:57 ` Pali Rohár 2021-08-26 7:15 ` [PATCH v3 3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk Jeremy Linton 2021-08-30 8:37 ` nicolas saenz julienne 2021-09-13 16:12 ` Rafael J. Wysocki 2021-10-05 15:10 ` Bjorn Helgaas 2021-10-05 15:43 ` Jeremy Linton 2021-10-05 22:31 ` Bjorn Helgaas 2021-10-05 23:32 ` Jeremy Linton 2021-10-05 20:02 ` Pali Rohár 2021-10-05 22:44 ` Jeremy Linton 2021-08-26 7:15 ` [PATCH v3 4/4] MAINTAINERS: Widen brcmstb PCIe file scope Jeremy Linton 2021-08-30 8:38 ` nicolas saenz julienne
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).