| Message ID | 20240715-pci-qcom-hotplug-v1-0-5f3765cc873a@linaro.org |
|---|---|
| Headers | show |
| Series | PCI: qcom: Simulate PCIe hotplug using 'global' interrupt | expand |
On 15.07.2024 7:33 PM, Manivannan Sadhasivam via B4 Relay wrote: > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Current error message just prints the contents of PARF_INT_ALL_STATUS > register as if like the IRQ event number. It could mislead the users. > Reword it to make it clear that the error message is actually showing the > interrupt status register to help debug spurious IRQ events. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 15.07.2024 7:33 PM, Manivannan Sadhasivam via B4 Relay wrote: > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Right now, PCI endpoint subsystem doesn't assign PCI domain number for the > PCI endpoint controllers. But this domain number could be useful to the EPC > drivers to uniquely identify each controller based on the hardware instance > when there are multiple ones present in an SoC (even multiple RC/EP). > > So let's make use of the existing pci_bus_find_domain_nr() API to allocate > domain numbers based on either Devicetree (linux,pci-domain) property or > dynamic domain number allocation scheme. > > It should be noted that the domain number allocated by this API will be > based on both RC and EP controllers in a SoC. If the 'linux,pci-domain' DT > property is present, then the domain number represents the actual hardware > instance of the PCI endpoint controller. If not, then the domain number > will be allocated based on the PCI EP/RC controller probe order. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- The PCI counterpart does some error checking and requires CONFIG_PCI_DOMAINS_GENERIC. Is that something that needs to be taken care of here as well? Konrad
On 15.07.2024 7:33 PM, Manivannan Sadhasivam via B4 Relay wrote: > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > 'linux,pci-domain' property provides the PCI domain number for the PCI > endpoint controllers in a SoC. If this property is not present, then an > unstable (across boots) unique number will be assigned. > > Use this property to specify the domain number based on the actual hardware > instance of the PCI endpoint controllers in SDX65 SoC. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 7/15/2024 11:03 PM, Manivannan Sadhasivam via B4 Relay wrote: > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Historically, Qcom PCIe RC controllers lack standard hotplug support. So > when an endpoint is attached to the SoC, users have to rescan the bus > manually to enumerate the device. But this can be avoided by simulating the > PCIe hotplug using Qcom specific way. > > Qcom PCIe RC controllers are capable of generating the 'global' SPI > interrupt to the host CPUs. The device driver can use this event to > identify events such as PCIe link specific events, safety events etc... > > One such event is the PCIe Link up event generated when an endpoint is > detected on the bus and the Link is 'up'. This event can be used to > simulate the PCIe hotplug in the Qcom SoCs. > > So add support for capturing the PCIe Link up event using the 'global' > interrupt in the driver. Once the Link up event is received, the bus > underneath the host bridge is scanned to enumerate PCIe endpoint devices, > thus simulating hotplug. > > All of the Qcom SoCs have only one rootport per controller instance. So > only a single 'Link up' event is generated for the PCIe controller. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 55 ++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 0180edf3310e..38ed411d2052 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -50,6 +50,9 @@ > #define PARF_AXI_MSTR_WR_ADDR_HALT_V2 0x1a8 > #define PARF_Q2A_FLUSH 0x1ac > #define PARF_LTSSM 0x1b0 > +#define PARF_INT_ALL_STATUS 0x224 > +#define PARF_INT_ALL_CLEAR 0x228 > +#define PARF_INT_ALL_MASK 0x22c > #define PARF_SID_OFFSET 0x234 > #define PARF_BDF_TRANSLATE_CFG 0x24c > #define PARF_SLV_ADDR_SPACE_SIZE 0x358 > @@ -121,6 +124,9 @@ > /* PARF_LTSSM register fields */ > #define LTSSM_EN BIT(8) > > +/* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */ > +#define PARF_INT_ALL_LINK_UP BIT(13) > + > /* PARF_NO_SNOOP_OVERIDE register fields */ > #define WR_NO_SNOOP_OVERIDE_EN BIT(1) > #define RD_NO_SNOOP_OVERIDE_EN BIT(3) > @@ -260,6 +266,7 @@ struct qcom_pcie { > struct icc_path *icc_cpu; > const struct qcom_pcie_cfg *cfg; > struct dentry *debugfs; > + int global_irq; > bool suspended; > }; > > @@ -1488,6 +1495,29 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie) > qcom_pcie_link_transition_count); > } > > +static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data) > +{ > + struct qcom_pcie *pcie = data; > + struct dw_pcie_rp *pp = &pcie->pci->pp; > + struct device *dev = pcie->pci->dev; > + u32 status = readl_relaxed(pcie->parf + PARF_INT_ALL_STATUS); > + > + writel_relaxed(status, pcie->parf + PARF_INT_ALL_CLEAR); > + > + if (FIELD_GET(PARF_INT_ALL_LINK_UP, status)) { > + dev_dbg(dev, "Received Link up event. Starting enumeration!\n"); > + /* Rescan the bus to enumerate endpoint devices */ > + pci_lock_rescan_remove(); > + pci_rescan_bus(pp->bridge->bus); There can be chances of getting link up interrupt before PCIe framework starts enumeration and at that time bridge-> bus is not created and cause NULL point access. Please have a check for this. - Krishna Chaitanya. > + pci_unlock_rescan_remove(); > + } else { > + dev_err(dev, "Received unknown event. INT_STATUS: 0x%08x\n", > + status); > + } > + > + return IRQ_HANDLED; > +} > + > static int qcom_pcie_probe(struct platform_device *pdev) > { > const struct qcom_pcie_cfg *pcie_cfg; > @@ -1498,6 +1528,7 @@ static int qcom_pcie_probe(struct platform_device *pdev) > struct dw_pcie_rp *pp; > struct resource *res; > struct dw_pcie *pci; > + char *name; > int ret; > > pcie_cfg = of_device_get_match_data(dev); > @@ -1617,6 +1648,28 @@ static int qcom_pcie_probe(struct platform_device *pdev) > goto err_phy_exit; > } > > + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_global_irq%d", > + pci_domain_nr(pp->bridge->bus)); > + if (!name) { > + ret = -ENOMEM; > + goto err_host_deinit; > + } > + > + pcie->global_irq = platform_get_irq_byname_optional(pdev, "global"); > + if (pcie->global_irq > 0) { > + ret = devm_request_threaded_irq(&pdev->dev, pcie->global_irq, > + NULL, > + qcom_pcie_global_irq_thread, > + IRQF_ONESHOT, name, pcie); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, > + "Failed to request Global IRQ\n"); > + goto err_host_deinit; > + } > + > + writel_relaxed(PARF_INT_ALL_LINK_UP, pcie->parf + PARF_INT_ALL_MASK); > + } > + > qcom_pcie_icc_opp_update(pcie); > > if (pcie->mhi) > @@ -1624,6 +1677,8 @@ static int qcom_pcie_probe(struct platform_device *pdev) > > return 0; > > +err_host_deinit: > + dw_pcie_host_deinit(pp); > err_phy_exit: > phy_exit(pcie->phy); > err_pm_runtime_put: >
On Tue, Jul 16, 2024 at 09:34:13AM +0530, Krishna Chaitanya Chundru wrote: > > > On 7/15/2024 11:03 PM, Manivannan Sadhasivam via B4 Relay wrote: > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > Historically, Qcom PCIe RC controllers lack standard hotplug support. So > > when an endpoint is attached to the SoC, users have to rescan the bus > > manually to enumerate the device. But this can be avoided by simulating the > > PCIe hotplug using Qcom specific way. > > > > Qcom PCIe RC controllers are capable of generating the 'global' SPI > > interrupt to the host CPUs. The device driver can use this event to > > identify events such as PCIe link specific events, safety events etc... > > > > One such event is the PCIe Link up event generated when an endpoint is > > detected on the bus and the Link is 'up'. This event can be used to > > simulate the PCIe hotplug in the Qcom SoCs. > > > > So add support for capturing the PCIe Link up event using the 'global' > > interrupt in the driver. Once the Link up event is received, the bus > > underneath the host bridge is scanned to enumerate PCIe endpoint devices, > > thus simulating hotplug. > > > > All of the Qcom SoCs have only one rootport per controller instance. So > > only a single 'Link up' event is generated for the PCIe controller. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-qcom.c | 55 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > index 0180edf3310e..38ed411d2052 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > @@ -50,6 +50,9 @@ > > #define PARF_AXI_MSTR_WR_ADDR_HALT_V2 0x1a8 > > #define PARF_Q2A_FLUSH 0x1ac > > #define PARF_LTSSM 0x1b0 > > +#define PARF_INT_ALL_STATUS 0x224 > > +#define PARF_INT_ALL_CLEAR 0x228 > > +#define PARF_INT_ALL_MASK 0x22c > > #define PARF_SID_OFFSET 0x234 > > #define PARF_BDF_TRANSLATE_CFG 0x24c > > #define PARF_SLV_ADDR_SPACE_SIZE 0x358 > > @@ -121,6 +124,9 @@ > > /* PARF_LTSSM register fields */ > > #define LTSSM_EN BIT(8) > > +/* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */ > > +#define PARF_INT_ALL_LINK_UP BIT(13) > > + > > /* PARF_NO_SNOOP_OVERIDE register fields */ > > #define WR_NO_SNOOP_OVERIDE_EN BIT(1) > > #define RD_NO_SNOOP_OVERIDE_EN BIT(3) > > @@ -260,6 +266,7 @@ struct qcom_pcie { > > struct icc_path *icc_cpu; > > const struct qcom_pcie_cfg *cfg; > > struct dentry *debugfs; > > + int global_irq; > > bool suspended; > > }; > > @@ -1488,6 +1495,29 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie) > > qcom_pcie_link_transition_count); > > } > > +static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data) > > +{ > > + struct qcom_pcie *pcie = data; > > + struct dw_pcie_rp *pp = &pcie->pci->pp; > + struct device *dev = pcie->pci->dev; > > + u32 status = readl_relaxed(pcie->parf + PARF_INT_ALL_STATUS); > > + > > + writel_relaxed(status, pcie->parf + PARF_INT_ALL_CLEAR); > > + > > + if (FIELD_GET(PARF_INT_ALL_LINK_UP, status)) { > > + dev_dbg(dev, "Received Link up event. Starting enumeration!\n"); > > + /* Rescan the bus to enumerate endpoint devices */ > > + pci_lock_rescan_remove(); > > + pci_rescan_bus(pp->bridge->bus); > There can be chances of getting link up interrupt before PCIe framework > starts enumeration and at that time bridge-> bus is not created and > cause NULL point access. > Please have a check for this. > Host bridge is enumerated during dw_pcie_host_init() and the IRQ handler is registered afterwards. So there is no way the 'pp->bridge' can be NULL. - Mani
On Tue, Jul 16, 2024 at 09:54:54AM +0530, Krishna Chaitanya Chundru wrote: > > > On 7/16/2024 9:48 AM, Manivannan Sadhasivam wrote: > > On Tue, Jul 16, 2024 at 09:34:13AM +0530, Krishna Chaitanya Chundru wrote: > > > > > > > > > On 7/15/2024 11:03 PM, Manivannan Sadhasivam via B4 Relay wrote: > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > > > Historically, Qcom PCIe RC controllers lack standard hotplug support. So > > > > when an endpoint is attached to the SoC, users have to rescan the bus > > > > manually to enumerate the device. But this can be avoided by simulating the > > > > PCIe hotplug using Qcom specific way. > > > > > > > > Qcom PCIe RC controllers are capable of generating the 'global' SPI > > > > interrupt to the host CPUs. The device driver can use this event to > > > > identify events such as PCIe link specific events, safety events etc... > > > > > > > > One such event is the PCIe Link up event generated when an endpoint is > > > > detected on the bus and the Link is 'up'. This event can be used to > > > > simulate the PCIe hotplug in the Qcom SoCs. > > > > > > > > So add support for capturing the PCIe Link up event using the 'global' > > > > interrupt in the driver. Once the Link up event is received, the bus > > > > underneath the host bridge is scanned to enumerate PCIe endpoint devices, > > > > thus simulating hotplug. > > > > > > > > All of the Qcom SoCs have only one rootport per controller instance. So > > > > only a single 'Link up' event is generated for the PCIe controller. > > > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > drivers/pci/controller/dwc/pcie-qcom.c | 55 ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 55 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > > index 0180edf3310e..38ed411d2052 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > > @@ -50,6 +50,9 @@ > > > > #define PARF_AXI_MSTR_WR_ADDR_HALT_V2 0x1a8 > > > > #define PARF_Q2A_FLUSH 0x1ac > > > > #define PARF_LTSSM 0x1b0 > > > > +#define PARF_INT_ALL_STATUS 0x224 > > > > +#define PARF_INT_ALL_CLEAR 0x228 > > > > +#define PARF_INT_ALL_MASK 0x22c > > > > #define PARF_SID_OFFSET 0x234 > > > > #define PARF_BDF_TRANSLATE_CFG 0x24c > > > > #define PARF_SLV_ADDR_SPACE_SIZE 0x358 > > > > @@ -121,6 +124,9 @@ > > > > /* PARF_LTSSM register fields */ > > > > #define LTSSM_EN BIT(8) > > > > +/* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */ > > > > +#define PARF_INT_ALL_LINK_UP BIT(13) > > > > + > > > > /* PARF_NO_SNOOP_OVERIDE register fields */ > > > > #define WR_NO_SNOOP_OVERIDE_EN BIT(1) > > > > #define RD_NO_SNOOP_OVERIDE_EN BIT(3) > > > > @@ -260,6 +266,7 @@ struct qcom_pcie { > > > > struct icc_path *icc_cpu; > > > > const struct qcom_pcie_cfg *cfg; > > > > struct dentry *debugfs; > > > > + int global_irq; > > > > bool suspended; > > > > }; > > > > @@ -1488,6 +1495,29 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie) > > > > qcom_pcie_link_transition_count); > > > > } > > > > +static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data) > > > > +{ > > > > + struct qcom_pcie *pcie = data; > > > > + struct dw_pcie_rp *pp = &pcie->pci->pp; > + struct device *dev = pcie->pci->dev; > > > > + u32 status = readl_relaxed(pcie->parf + PARF_INT_ALL_STATUS); > > > > + > > > > + writel_relaxed(status, pcie->parf + PARF_INT_ALL_CLEAR); > > > > + > > > > + if (FIELD_GET(PARF_INT_ALL_LINK_UP, status)) { > > > > + dev_dbg(dev, "Received Link up event. Starting enumeration!\n"); > > > > + /* Rescan the bus to enumerate endpoint devices */ > > > > + pci_lock_rescan_remove(); > > > > + pci_rescan_bus(pp->bridge->bus); > > > There can be chances of getting link up interrupt before PCIe framework > > > starts enumeration and at that time bridge-> bus is not created and > > > cause NULL point access. > > > Please have a check for this. > > > > > > > Host bridge is enumerated during dw_pcie_host_init() and the IRQ handler is > > registered afterwards. So there is no way the 'pp->bridge' can be NULL. > > > > - Mani > I leaved a gap between bridge-> & bus by mistake, I want to highlight > bridge->bus in above comment. The bus can be NULL and it can create NULL > point access. How can the bridge->bus be NULL? Only if the bridge itself is not enumerated, it will be NULL. And that cannot happen unless something wrong with the controller itself. In that case, how can Link up event be generated? - Mani
Hi, This series adds support to simulate PCIe hotplug using the Qcom specific 'global' IRQ. Historically, Qcom PCIe RC controllers lack standard hotplug support. So when an endpoint is attached to the SoC, users have to rescan the bus manually to enumerate the device. But this can be avoided by simulating the PCIe hotplug using Qcom specific way. Qcom PCIe RC controllers are capable of generating the 'global' SPI interrupt to the host CPUs. The device driver can use this event to identify events such as PCIe link specific events, safety events etc... One such event is the PCIe Link up event generated when an endpoint is detected on the bus and the Link is 'up'. This event can be used to simulate the PCIe hotplug in the Qcom SoCs. So add support for capturing the PCIe Link up event using the 'global' interrupt in the driver. Once the Link up event is received, the bus underneath the host bridge is scanned to enumerate PCIe endpoint devices, thus simulating hotplug. This series also has some cleanups to the Qcom PCIe EP controller driver for interrupt handling. Testing ======= This series is tested on Qcom SM8450 based development board that has 2 SoCs connected over PCIe. - Mani Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- Manivannan Sadhasivam (14): PCI: qcom-ep: Drop the redundant masking of global IRQ events PCI: qcom-ep: Reword the error message for receiving unknown global IRQ event dt-bindings: PCI: pci-ep: Update Maintainers dt-bindings: PCI: pci-ep: Document 'linux,pci-domain' property dt-bindings: PCI: qcom-ep: Document "linux,pci-domain" property PCI: endpoint: Assign PCI domain number for endpoint controllers PCI: qcom-ep: Modify 'global_irq' and 'perst_irq' IRQ device names ARM: dts: qcom: sdx55: Add 'linux,pci-domain' to PCIe EP controller node ARM: dts: qcom: sdx65: Add 'linux,pci-domain' to PCIe EP controller node arm64: dts: qcom: sa8775p: Add 'linux,pci-domain' to PCIe EP controller nodes dt-bindings: PCI: qcom: Add 'global' interrupt dt-bindings: PCI: qcom,pcie-sm8450: Add 'global' interrupt PCI: qcom: Simulate PCIe hotplug using 'global' interrupt arm64: dts: qcom: sm8450: Add 'global' interrupt to the PCIe RC node Documentation/devicetree/bindings/pci/pci-ep.yaml | 14 +++++- .../devicetree/bindings/pci/qcom,pcie-common.yaml | 4 +- .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 1 + .../devicetree/bindings/pci/qcom,pcie-sm8450.yaml | 10 ++-- arch/arm/boot/dts/qcom/qcom-sdx55.dtsi | 1 + arch/arm/boot/dts/qcom/qcom-sdx65.dtsi | 1 + arch/arm64/boot/dts/qcom/sa8775p.dtsi | 2 + arch/arm64/boot/dts/qcom/sm8450.dtsi | 12 +++-- drivers/pci/controller/dwc/pcie-qcom-ep.c | 21 +++++++-- drivers/pci/controller/dwc/pcie-qcom.c | 55 ++++++++++++++++++++++ drivers/pci/endpoint/pci-epc-core.c | 1 + include/linux/pci-epc.h | 2 + 12 files changed, 108 insertions(+), 16 deletions(-) --- base-commit: 91e3b24eb7d297d9d99030800ed96944b8652eaf change-id: 20240715-pci-qcom-hotplug-bcde1c13d91f Best regards,