Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 3/3] PCI: uniphier-ep: Add EPC restart management
 support
To:     Kunihiko Hayashi <hayashi.kunihiko@socionext.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>
CC:     <linux-pci@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>,
        Masami Hiramatsu <masami.hiramatsu@linaro.org>,
        Jassi Brar <jaswinder.singh@linaro.org>
References: <1611500977-24816-1-git-send-email-hayashi.kunihiko@socionext.com>
 <1611500977-24816-4-git-send-email-hayashi.kunihiko@socionext.com>
From:   Kishon Vijay Abraham I <kishon@ti.com>
Message-ID: <c5e89789-2dd3-3247-ec85-d54652987e2a@ti.com>
Date:   Thu, 28 Jan 2021 19:59:17 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <1611500977-24816-4-git-send-email-hayashi.kunihiko@socionext.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Kunihiko,

On 24/01/21 8:39 pm, Kunihiko Hayashi wrote:
> Set the polling function and call the init function to enable EPC restart
> management. The polling function detects that the bus-reset signal is a
> rising edge.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/controller/dwc/Kconfig            |  1 +
>  drivers/pci/controller/dwc/pcie-uniphier-ep.c | 44 ++++++++++++++++++++++++++-
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 22c5529..90d400a 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -302,6 +302,7 @@ config PCIE_UNIPHIER_EP
>  	depends on OF && HAS_IOMEM
>  	depends on PCI_ENDPOINT
>  	select PCIE_DW_EP
> +	select PCI_ENDPOINT_RESTART
>  	help
>  	  Say Y here if you want PCIe endpoint controller support on
>  	  UniPhier SoCs. This driver supports Pro5 SoC.
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier-ep.c b/drivers/pci/controller/dwc/pcie-uniphier-ep.c
> index 69810c6..9d83850 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier-ep.c
> @@ -26,6 +26,7 @@
>  #define PCL_RSTCTRL_PIPE3		BIT(0)
>  
>  #define PCL_RSTCTRL1			0x0020
> +#define PCL_RSTCTRL_PERST_MON		BIT(16)
>  #define PCL_RSTCTRL_PERST		BIT(0)
>  
>  #define PCL_RSTCTRL2			0x0024
> @@ -60,6 +61,7 @@ struct uniphier_pcie_ep_priv {
>  	struct clk *clk, *clk_gio;
>  	struct reset_control *rst, *rst_gio;
>  	struct phy *phy;
> +	bool bus_reset_status;
>  	const struct pci_epc_features *features;
>  };
>  
> @@ -212,6 +214,41 @@ uniphier_pcie_get_features(struct dw_pcie_ep *ep)
>  	return priv->features;
>  }
>  
> +static bool uniphier_pcie_ep_poll_reset(void *data)
> +{
> +	struct uniphier_pcie_ep_priv *priv = data;
> +	bool ret, status;
> +
> +	if (!priv)
> +		return false;
> +
> +	status = !(readl(priv->base + PCL_RSTCTRL1) & PCL_RSTCTRL_PERST_MON);
> +
> +	/* return true if the rising edge of bus reset is detected */
> +	ret = !!(status == false && priv->bus_reset_status == true);
> +	priv->bus_reset_status = status;

I'm still not convinced about having a separate library for restart
management but shouldn't we reset the function driver on falling edge?
After the rising edge the host expects the endpoint to be ready.

Why not use the CORE_INIT (core_init_notifier) infrastructure?

Thanks
Kishon

> +
> +	return ret;
> +}
> +
> +static int uniphier_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
> +	int ret;
> +
> +	/* Set up epc-restart thread */
> +	pci_epc_restart_register_poll_func(ep->epc,
> +					    uniphier_pcie_ep_poll_reset, priv);
> +	/* With call of poll_reset() directly, initialize internal state */
> +	uniphier_pcie_ep_poll_reset(priv);
> +	ret = pci_epc_restart_init(ep->epc);
> +	if (ret)
> +		dev_err(pci->dev, "Failed to initialize epc-restart (%d)\n", ret);
> +
> +	return ret;
> +}
> +
>  static const struct dw_pcie_ep_ops uniphier_pcie_ep_ops = {
>  	.ep_init = uniphier_pcie_ep_init,
>  	.raise_irq = uniphier_pcie_ep_raise_irq,
> @@ -318,7 +355,12 @@ static int uniphier_pcie_ep_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	priv->pci.ep.ops = &uniphier_pcie_ep_ops;
> -	return dw_pcie_ep_init(&priv->pci.ep);
> +
> +	ret = dw_pcie_ep_init(&priv->pci.ep);
> +	if (ret)
> +		return ret;
> +
> +	return uniphier_pcie_ep_init_complete(&priv->pci.ep);
>  }
>  
>  static const struct pci_epc_features uniphier_pro5_data = {
>