[PATCH 3/8] pci: pcie_dw_rockchip: Speed up link probe

[Jonas Karlman]

Hi Kever,

On 2023-05-09 14:08, Kever Yang wrote:
> Hi Jonas,
> 
> On 2023/4/23 02:19, Jonas Karlman wrote:
>> Use a similar pattern and delay values as the linux mainline driver to
>> speed up failing when nothing is connected.
>>
>> Reduce fail speed from around 5+ seconds down to around one second on a
>> Radxa ROCK 3 Model A, where pcie2x1 is probed before pcie3x2 M2 slot.
>>
>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>> ---
>>   drivers/pci/pcie_dw_rockchip.c | 68 ++++++++++++++++++----------------
>>   1 file changed, 37 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c
>> index a5b900f95981..fd3da47272b3 100644
>> --- a/drivers/pci/pcie_dw_rockchip.c
>> +++ b/drivers/pci/pcie_dw_rockchip.c
>> @@ -61,9 +61,6 @@ struct rk_pcie {
>>   #define PCIE_CLIENT_DBG_TRANSITION_DATA	0xffff0000
>>   #define PCIE_CLIENT_DBF_EN		0xffff0003
>>   
>> -/* Parameters for the waiting for #perst signal */
>> -#define MACRO_US			1000
>> -
>>   static int rk_pcie_read(void __iomem *addr, int size, u32 *val)
>>   {
>>   	if ((uintptr_t)addr & (size - 1)) {
>> @@ -242,43 +239,46 @@ static int rk_pcie_link_up(struct rk_pcie *priv, u32 cap_speed)
>>   	/* DW pre link configurations */
>>   	rk_pcie_configure(priv, cap_speed);
>>   
>> -	/* Rest the device */
>> -	if (dm_gpio_is_valid(&priv->rst_gpio)) {
>> -		dm_gpio_set_value(&priv->rst_gpio, 0);
>> -		/*
>> -		 * Minimal is 100ms from spec but we see
>> -		 * some wired devices need much more, such as 600ms.
>> -		 * Add a enough delay to cover all cases.
>> -		 */
>> -		udelay(MACRO_US * 1000);
>
> This delay has been changed.
>
>> -		dm_gpio_set_value(&priv->rst_gpio, 1);
>> -	}
>> -
>>   	rk_pcie_disable_ltssm(priv);
>>   	rk_pcie_link_status_clear(priv);
>>   	rk_pcie_enable_debug(priv);
>>   
>> +	/* Reset the device */
>> +	if (dm_gpio_is_valid(&priv->rst_gpio))
>> +		dm_gpio_set_value(&priv->rst_gpio, 0);
>> +
>>   	/* Enable LTSSM */
>>   	rk_pcie_enable_ltssm(priv);
>>   
>> -	for (retries = 0; retries < 5; retries++) {
>> -		if (is_link_up(priv)) {
>> -			dev_info(priv->dw.dev, "PCIe Link up, LTSSM is 0x%x\n",
>> -				 rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
>> -			rk_pcie_debug_dump(priv);
>> -			return 0;
>> -		}
>> -
>> -		dev_info(priv->dw.dev, "PCIe Linking... LTSSM is 0x%x\n",
>> -			 rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
>> -		rk_pcie_debug_dump(priv);
>> -		udelay(MACRO_US * 1000);
>> +	/*
>> +	 * PCIe requires the refclk to be stable for 100ms prior to releasing
>> +	 * PERST. See table 2-4 in section 2.6.2 AC Specifications of the PCI
>> +	 * Express Card Electromechanical Specification, 1.1. However, we don't
>> +	 * know if the refclk is coming from RC's PHY or external OSC. If it's
>> +	 * from RC, so enabling LTSSM is the just right place to release #PERST.
>> +	 */
>> +	mdelay(100);
> 
> There is a common about this delay, the spec is 100ms, but some device 
> on the public market
> 
> may not work with this delay, it's better to keep previous delay value.

The old delays are very excessive and do not seem to be necessary,
they have also been changed in vendor u-boot in e.g. following commit:

https://github.com/rockchip-linux/u-boot/commit/92d3587863d327e3b48e0eaf5c37db8e7f614b36

Instead, I copied the config/reset order and same delay values used by
the linux driver and it seem to work much better after these changes.

Does anybody know what devices was having issues that the old comment
was referring to? The devices I was able to test did not see any issue.

Regards,
Jonas

> 
> 
> Thanks,
> 
> - Kever
> 
>> +	if (dm_gpio_is_valid(&priv->rst_gpio))
>> +		dm_gpio_set_value(&priv->rst_gpio, 1);
>> +
>> +	/* Check if the link is up or not */
>> +	for (retries = 0; retries < 10; retries++) {
>> +		if (is_link_up(priv))
>> +			break;
>> +
>> +		mdelay(100);
>> +	}
>> +
>> +	if (retries >= 10) {
>> +		dev_err(priv->dw.dev, "PCIe-%d Link Fail\n",
>> +			dev_seq(priv->dw.dev));
>> +		return -EIO;
>>   	}
>>   
>> -	dev_err(priv->dw.dev, "PCIe-%d Link Fail\n", dev_seq(priv->dw.dev));
>> -	/* Link maybe in Gen switch recovery but we need to wait more 1s */
>> -	udelay(MACRO_US * 1000);
>> -	return -EIO;
>> +	dev_info(priv->dw.dev, "PCIe Link up, LTSSM is 0x%x\n",
>> +		 rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
>> +	rk_pcie_debug_dump(priv);
>> +	return 0;
>>   }
>>   
>>   static int rockchip_pcie_init_port(struct udevice *dev)
>> @@ -287,6 +287,12 @@ static int rockchip_pcie_init_port(struct udevice *dev)
>>   	u32 val;
>>   	struct rk_pcie *priv = dev_get_priv(dev);
>>   
>> +	ret = reset_assert_bulk(&priv->rsts);
>> +	if (ret) {
>> +		dev_err(dev, "failed to assert resets (ret=%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>>   	/* Set power and maybe external ref clk input */
>>   	ret = regulator_set_enable_if_allowed(priv->vpcie3v3, true);
>>   	if (ret && ret != -ENOSYS) {