Re: [PATCH v2 3/3] PCI: uniphier-ep: Add EPC restart management support

[Kunihiko Hayashi]

Hi Kishon,
Thank you for your comment.

On 2021/01/28 23:29, Kishon Vijay Abraham I wrote:
> 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@xxxxxxxxxxxxx>
> > ---
> >   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?

I understand your opnion well.
There might not be enough way to give controller-specific features
to handle "restart" as a common function.

> After the rising edge the host expects the endpoint to be ready.

I see. I didn't consider that restart was completed just after
the rising edge.

> Why not use the CORE_INIT (core_init_notifier) infrastructure?

I don't follow the CORE_INIT yet, so I'll try to introduce it
into the driver. However, our current controller doesn't have
an interrupt that detects PERST like pcie-tegra194.
I think the driver needs a thread for polling PERST like patch 2/3.

Thank you,

---
Best Regards
Kunihiko Hayashi