[PATCH v2] PCI: rockchip-host: Fix rockchip_pcie_host_init_port() PERST# handling

[Damien Le Moal]

On 4/1/24 04:34, Dragan Simic wrote:
> Hello Damien,
> 
> Please see my comments below.
> 
> On 2024-03-30 04:50, Damien Le Moal wrote:
>> The PCIe specifications (PCI Express Electromechanical Specification 
>> rev
>> 2.0, section 2.6.2) mandate that the PERST# signal must remain asserted
>> for at least 100 usec (Tperst-clk) after the PCIe reference clock
>> becomes stable (if a reference clock is supplied), for at least 100 
>> msec
>> after the power is stable (Tpvperl).
>>
>> In addition, the PCI Express Base SPecification Rev 2.0, section 6.6.1
>> state that the host should wait for at least 100 msec from the end of a
>> conventional reset (PERST# is de-asserted) before accessing the
>> configuration space of the attached device.
>>
>> Modify rockchip_pcie_host_init_port() by adding two 100ms sleep, one
>> before and after bringing back PESRT signal to high using the ep_gpio
>> GPIO. Comments are also added to clarify this behavior.
>>
>> Signed-off-by: Damien Le Moal <dlemoal at kernel.org>
>> ---
>>
>> Changes from v1:
>>  - Add more specification details to the commit message.
>>  - Add missing msleep(100) after PERST# is deasserted.
>>
>>  drivers/pci/controller/pcie-rockchip-host.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
>> b/drivers/pci/controller/pcie-rockchip-host.c
>> index 300b9dc85ecc..ff2fa27bd883 100644
>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>> @@ -294,6 +294,7 @@ static int rockchip_pcie_host_init_port(struct
>> rockchip_pcie *rockchip)
>>  	int err, i = MAX_LANE_NUM;
>>  	u32 status;
>>
>> +	/* Assert PERST */
>>  	gpiod_set_value_cansleep(rockchip->ep_gpio, 0);
>>
>>  	err = rockchip_pcie_init_port(rockchip);
>> @@ -322,8 +323,19 @@ static int rockchip_pcie_host_init_port(struct
>> rockchip_pcie *rockchip)
>>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>>  			    PCIE_CLIENT_CONFIG);
>>
>> +	/*
>> +	 * PCIe CME specifications mandate that PERST be asserted for at
>> +	 * least 100ms after power is stable.
>> +	 */
>> +	msleep(100);
> 
> Perhaps it would be slightly better to use usleep_range()
> instead of msleep().

I can do that, but I fail to see the advantage. Why do you say that it may be
better ?

> 
>>  	gpiod_set_value_cansleep(rockchip->ep_gpio, 1);
>>
>> +	/*
>> +	 * PCIe base specifications rev 2.0 mandate that the host wait for
>> +	 * 100ms after completion of a conventional reset.
>> +	 */
>> +	msleep(100);
> 
> Obviously, the same comment as above applies here.
> 
>> +
>>  	/* 500ms timeout value should be enough for Gen1/2 training */
>>  	err = readl_poll_timeout(rockchip->apb_base + 
>> PCIE_CLIENT_BASIC_STATUS1,
>>  				 status, PCIE_LINK_UP(status), 20,

-- 
Damien Le Moal
Western Digital Research