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@xxxxxxxxxx>
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
--
மணிவண்ணன் சதாசிவம்
