Message-ID: <AD287CCE-9FFE-49BC-B9CA-B5CED4F05AF1@linaro.org>
Date: Wed, 13 Nov 2024 17:15:25 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Bjorn Helgaas <helgaas@...nel.org>,
 Jenishkumar Maheshbhai Patel <jpatel2@...vell.com>
CC: lpieralisi@...nel.org, thomas.petazzoni@...tlin.com, kw@...ux.com,
 robh@...nel.org, bhelgaas@...gle.com, linux-pci@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 salee@...vell.com, dingwei@...vell.com
Subject: Re: [PATCH 1/1] PCI: armada8k: Add link-down handle



On November 13, 2024 3:02:55 AM GMT+05:30, Bjorn Helgaas <helgaas@...nel.org> 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.

>
>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

>
>> Signed-off-by: Jenishkumar Maheshbhai Patel <jpatel2@...vell.com>
>> ---
>>  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
>> 

மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists