Re: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Mon, Oct 07, 2024 at 01:12:18PM +0900, Damien Le Moal wrote:
> Currently, the Rockchip PCIe endpoint controller driver does not handle
> PERST# signal, which prevents detecting when link training should
> actually be started or if the host reset the device. This however can
> be supported using the controller ep_gpio, set as an input GPIO for
> endpoint mode.
> 
> Modify the endpoint rockchip driver to get the ep_gpio and its
> associated interrupt which is serviced using a threaded IRQ with the
> function rockchip_pcie_ep_perst_irq_thread() as handler.
> 
> This handler function notifies a link down event corresponding to the RC
> side asserting the PERST# signal using pci_epc_linkdown() when the gpio
> is high. Once the gpio value goes down, corresponding to the RC
> de-asserting the PERST# signal, link training is started. The polarity
> of the gpio interrupt trigger is changed from high to low after the RC
> asserted PERST#, and conversely changed from low to high after the RC
> de-asserts PERST#.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Just minor nits below. Overall LGTM.

> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 118 +++++++++++++++++++++-
>  drivers/pci/controller/pcie-rockchip.c    |  12 +--
>  2 files changed, 122 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index af50432525b4..c70a64c37a56 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -18,6 +18,7 @@
>  #include <linux/sizes.h>
>  #include <linux/workqueue.h>
>  #include <linux/iopoll.h>
> +#include <linux/gpio/consumer.h>

Please sort the includes.

>  
>  #include "pcie-rockchip.h"
>  
> @@ -50,6 +51,9 @@ struct rockchip_pcie_ep {
>  	u64			irq_pci_addr;
>  	u8			irq_pci_fn;
>  	u8			irq_pending;
> +	int			perst_irq;
> +	bool			perst_asserted;
> +	bool			link_up;
>  	struct delayed_work	link_training;
>  };
>  
> @@ -462,13 +466,17 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
>  
>  	rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG);
>  
> +	if (rockchip->ep_gpio)
> +		enable_irq(ep->perst_irq);
> +
>  	/* Enable configuration and start link training */
>  	rockchip_pcie_write(rockchip,
>  			    PCIE_CLIENT_LINK_TRAIN_ENABLE |
>  			    PCIE_CLIENT_CONF_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
>  
> -	schedule_delayed_work(&ep->link_training, 0);
> +	if (!rockchip->ep_gpio)
> +		schedule_delayed_work(&ep->link_training, 0);
>  
>  	return 0;
>  }
> @@ -478,6 +486,11 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc)
>  	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct rockchip_pcie *rockchip = &ep->rockchip;
>  
> +	if (rockchip->ep_gpio) {
> +		ep->perst_asserted = true;
> +		disable_irq(ep->perst_irq);
> +	}
> +
>  	cancel_delayed_work_sync(&ep->link_training);
>  
>  	/* Stop link training and disable configuration */
> @@ -540,6 +553,13 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)
>  	if (!rockchip_pcie_ep_link_up(rockchip))
>  		goto again;
>  
> +	/*
> +	 * If PERST was asserted while polling the link, do not notify
> +	 * the function.
> +	 */
> +	if (ep->perst_asserted)
> +		return;
> +
>  	val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS0);
>  	dev_info(dev,
>  		 "Link UP (Negociated speed: %sGT/s, width: x%lu)\n",
> @@ -549,6 +569,7 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)
>  
>  	/* Notify the function */
>  	pci_epc_linkup(ep->epc);
> +	ep->link_up = true;
>  
>  	return;
>  
> @@ -556,6 +577,94 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)
>  	schedule_delayed_work(&ep->link_training, msecs_to_jiffies(5));
>  }
>  
> +static void rockchip_pcie_ep_perst_assert(struct rockchip_pcie_ep *ep)
> +{
> +	struct rockchip_pcie *rockchip = &ep->rockchip;
> +	struct device *dev = rockchip->dev;
> +
> +	dev_dbg(dev, "PERST asserted, link down\n");
> +
> +	if (ep->perst_asserted)
> +		return;
> +
> +	ep->perst_asserted = true;
> +
> +	cancel_delayed_work_sync(&ep->link_training);
> +
> +	if (ep->link_up) {
> +		pci_epc_linkdown(ep->epc);
> +		ep->link_up = false;
> +	}
> +}
> +
> +static void rockchip_pcie_ep_perst_deassert(struct rockchip_pcie_ep *ep)
> +{
> +	struct rockchip_pcie *rockchip = &ep->rockchip;
> +	struct device *dev = rockchip->dev;
> +
> +	dev_dbg(dev, "PERST de-asserted, starting link training\n");
> +
> +	if (!ep->perst_asserted)
> +		return;
> +
> +	ep->perst_asserted = false;
> +
> +	/* Enable link re-training */
> +	rockchip_pcie_ep_retrain_link(rockchip);
> +

I hope that no registers are getting reset post PERST# assert.

> +	/* Start link training */
> +	schedule_delayed_work(&ep->link_training, 0);
> +}
> +
> +static irqreturn_t rockchip_pcie_ep_perst_irq_thread(int irq, void *data)
> +{
> +	struct pci_epc *epc = data;
> +	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
> +	struct rockchip_pcie *rockchip = &ep->rockchip;
> +	u32 perst = gpiod_get_value(rockchip->ep_gpio);
> +
> +	if (perst)
> +		rockchip_pcie_ep_perst_assert(ep);
> +	else
> +		rockchip_pcie_ep_perst_deassert(ep);
> +
> +	irq_set_irq_type(ep->perst_irq,
> +			 (perst ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc)
> +{
> +	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
> +	struct rockchip_pcie *rockchip = &ep->rockchip;
> +	struct device *dev = rockchip->dev;
> +	int ret;
> +
> +	if (!rockchip->ep_gpio)
> +		return 0;
> +
> +	/* PCIe reset interrupt */
> +	ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
> +	if (ep->perst_irq < 0) {
> +		dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
> +		return ep->perst_irq;
> +	}
> +
> +	ep->perst_asserted = true;

How come?

> +	irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
> +	ret = devm_request_threaded_irq(dev, ep->perst_irq, NULL,
> +					rockchip_pcie_ep_perst_irq_thread,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					"pcie-ep-perst", epc);
> +	if (ret) {
> +		dev_err(dev, "Request PERST GPIO IRQ failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct pci_epc_features rockchip_pcie_epc_features = {
>  	.linkup_notifier = true,
>  	.msi_capable = true,
> @@ -719,6 +828,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  	rockchip->is_rc = false;
>  	rockchip->dev = dev;
>  	INIT_DELAYED_WORK(&ep->link_training, rockchip_pcie_ep_link_training);
> +	ep->link_up = false;

'false' is the default state, isn't it?

- Mani

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