> On November 13, 2024 3:02:55 AM GMT+05:30, Bjorn Helgaas > <mailto:helgaas@xxxxxxxxxx> wrote: > >In subject: > > > > PCI: armada8k: Add link-down handling > > > >On Mon, Nov 11, 2024 at 10:48:13PM -0800, Jenishkumar Maheshbhai > Patel wrote: > >> In PCIE ISR routine caused by RST_LINK_DOWN we schedule work to > >> handle the link-down procedure. > >> Link-down procedure will: > >> 1. Remove PCIe bus > >> 2. Reset the MAC > >> 3. Reconfigure link back up > >> 4. Rescan PCIe bus > > > >s/PCIE/PCIe/ > > > >Rewrap to fill 75 columns. > > > >I assume this basically removes a Root Port (and the hierarchy below > >it) if the link goes down, and then resets the MAC and tries to bring > >up the link and enumerate the hierarchy again. > > > >No other drivers do this, so why does armada8k need it? Is this to > >work around some unreliable link? > > Certainly Qcom IPs have this same feature and I was also looking to implement > it. But the link down should not be handled by this in the controller driver. > > Instead, it should be tied to bus reset in the core and the reset should be done > through a callback implemented in the controller drivers. This way, the reset > cannot happen in the back of PCI core and client drivers. > > That said, the Link down IRQ received by this driver should also be propagated > back to the PCI core and the core should then call the callback to reset the bus > that I mentioned above. > It's more than a work-around for the unreliable link. A few customers may have such application - independent power supply to the device with dedicated reset GPIO to #PRST. In this way, the power cycle and warm reset of RC and EP won't have impact on each other. However, it may lead into the PCI driver not aware of the link down when an unexpected power down or reset occurs on the device. We cannot assume the link will be recovered soon. The worse thing is the driver may continue access to the device, which may hang the bus. Since the device is no longer present on the bus, it's better to remove it. Besides, in order to bring up the link, the only way is to reset the MAC, which starts over the state machine of LTSSM. Well, we also noticed that there is no other driver that did this. I agree it is not necessary if the power cycle or warm reset of the device is done gracefully. The user can remove the device prior to the power cycle/reset. And do the rescan after the link is recovered. However, the unexpected power down is still possible. Please enlighten me if there is any better approach to handle such unexpected link down. In the meanwhile, I am quite interested the callback implementation suggested by Mani. But I am sure if we have such infrastructure right there. Mani, could you please elaborate a bit more, or is there any examples in the existing code and patches. > > > >I would think this would be reported via AER and possibly handled there > >already, but apparently not? > > No, these are not reported via AER (atleast on Qcom platforms). We have a > global_irq that fires when Link down happens. > > - Mani > No. Although AER is reported, it is consider as a recoverable error somehow. Like QCOM platform, the link down is a global irq. The difference is that Armada8k don't have an irq for link up. > > > >> Signed-off-by: Jenishkumar Maheshbhai Patel > >> <mailto:jpatel2@xxxxxxxxxxx> > >> --- > >> drivers/pci/controller/dwc/pcie-armada8k.c | 84 > >> ++++++++++++++++++++++ > >> 1 file changed, 84 insertions(+) > >> > >> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c > >> b/drivers/pci/controller/dwc/pcie-armada8k.c > >> index 07775539b321..b1b48c2016f7 100644 > >> --- a/drivers/pci/controller/dwc/pcie-armada8k.c > >> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c > >> @@ -21,6 +21,8 @@ > >> #include <linux/platform_device.h> > >> #include <linux/resource.h> > >> #include <linux/of_pci.h> > >> +#include <linux/mfd/syscon.h> > >> +#include <linux/regmap.h> > >> > >> #include "pcie-designware.h" > >> > >> @@ -32,6 +34,9 @@ struct armada8k_pcie { > >> struct clk *clk_reg; > >> struct phy *phy[ARMADA8K_PCIE_MAX_LANES]; > >> unsigned int phy_count; > >> + struct regmap *sysctrl_base; > >> + u32 mac_rest_bitmask; > >> + struct work_struct recover_link_work; > >> }; > >> > >> #define PCIE_VENDOR_REGS_OFFSET 0x8000 > >> @@ -72,6 +77,8 @@ struct armada8k_pcie { > >> #define AX_USER_DOMAIN_MASK 0x3 > >> #define AX_USER_DOMAIN_SHIFT 4 > >> > >> +#define UNIT_SOFT_RESET_CONFIG_REG 0x268 > >> + > >> #define to_armada8k_pcie(x) dev_get_drvdata((x)->dev) > >> > >> static void armada8k_pcie_disable_phys(struct armada8k_pcie *pcie) > >> @@ -216,6 +223,65 @@ static int armada8k_pcie_host_init(struct > dw_pcie_rp *pp) > >> return 0; > >> } > >> > >> +static void armada8k_pcie_recover_link(struct work_struct *ws) { > >> + struct armada8k_pcie *pcie = container_of(ws, struct armada8k_pcie, > recover_link_work); > >> + struct dw_pcie_rp *pp = &pcie->pci->pp; > >> + struct pci_bus *bus = pp->bridge->bus; > >> + struct pci_dev *root_port; > >> + int ret; > >> + > >> + root_port = pci_get_slot(bus, 0); > >> + if (!root_port) { > >> + dev_err(pcie->pci->dev, "failed to get root port\n"); > >> + return; > >> + } > >> + pci_lock_rescan_remove(); > >> + pci_stop_and_remove_bus_device(root_port); > > > >Add blank line. > > > >> + /* > >> + * Sleep needed to make sure all pcie transactions and access > >> + * are flushed before resetting the mac > >> + */ > >> + msleep(100); > > > >s/pcie/PCIe/ > >s/mac/MAC/ (also below) > > > >What PCIe spec parameter is the 100ms? If we don't already have a > >#define for it, add one in drivers/pci/pci.h with spec citation. > > > >> + /* Reset mac */ > >> + regmap_update_bits_base(pcie->sysctrl_base, > UNIT_SOFT_RESET_CONFIG_REG, > >> + pcie->mac_rest_bitmask, 0, NULL, false, true); > >> + udelay(1); > >> + regmap_update_bits_base(pcie->sysctrl_base, > UNIT_SOFT_RESET_CONFIG_REG, > >> + pcie->mac_rest_bitmask, pcie- > >mac_rest_bitmask, > >> + NULL, false, true); > >> + udelay(1); > >> + > >> + ret = dw_pcie_setup_rc(pp); > >> + if (ret) > >> + goto fail; > >> + > >> + ret = armada8k_pcie_host_init(pp); > >> + if (ret) { > >> + dev_err(pcie->pci->dev, "failed to initialize host: %d\n", ret); > >> + goto fail; > >> + } > >> + > >> + if (!dw_pcie_link_up(pcie->pci)) { > >> + ret = dw_pcie_start_link(pcie->pci); > >> + if (ret) > >> + goto fail; > >> + } > >> + > >> + /* Wait until the link becomes active again */ > >> + if (dw_pcie_wait_for_link(pcie->pci)) > >> + dev_err(pcie->pci->dev, "Link not up after reconfiguration\n"); > >> + > >> + bus = NULL; > >> + while ((bus = pci_find_next_bus(bus)) != NULL) > >> + pci_rescan_bus(bus); > >> + > >> +fail: > >> + pci_unlock_rescan_remove(); > >> + pci_dev_put(root_port); > >> +} > >> + > >> static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg) { > >> struct armada8k_pcie *pcie = arg; > >> @@ -253,6 +319,9 @@ static irqreturn_t armada8k_pcie_irq_handler(int > irq, void *arg) > >> * initiate a link retrain. If link retrains were > >> * possible, that is. > >> */ > >> + if (pcie->sysctrl_base && pcie->mac_rest_bitmask) > >> + schedule_work(&pcie->recover_link_work); > >> + > >> dev_dbg(pci->dev, "%s: link went down\n", __func__); > >> } > >> > >> @@ -322,6 +391,8 @@ static int armada8k_pcie_probe(struct > >> platform_device *pdev) > >> > >> pcie->pci = pci; > >> > >> + INIT_WORK(&pcie->recover_link_work, armada8k_pcie_recover_link); > >> + > >> pcie->clk = devm_clk_get(dev, NULL); > >> if (IS_ERR(pcie->clk)) > >> return PTR_ERR(pcie->clk); > >> @@ -349,6 +420,19 @@ static int armada8k_pcie_probe(struct > platform_device *pdev) > >> goto fail_clkreg; > >> } > >> > >> + pcie->sysctrl_base = syscon_regmap_lookup_by_phandle(pdev- > >dev.of_node, > >> + "marvell,system- > controller"); > >> + if (IS_ERR(pcie->sysctrl_base)) { > >> + dev_warn(dev, "failed to find marvell,system-controller\n"); > >> + pcie->sysctrl_base = 0x0; > >> + } > >> + > >> + ret = of_property_read_u32(pdev->dev.of_node, "marvell,mac-reset- > bit-mask", > >> + &pcie->mac_rest_bitmask); > >> + if (ret < 0) { > >> + dev_warn(dev, "couldn't find mac reset bit mask: %d\n", ret); > >> + pcie->mac_rest_bitmask = 0x0; > >> + } > >> ret = armada8k_pcie_setup_phys(pcie); > >> if (ret) > >> goto fail_clkreg; > >> -- > >> 2.25.1 > >> > > மணிவண்ணன் சதாசிவம்
