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

[Kunihiko Hayashi <hayashi.kunihiko@socionext.com>]

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

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