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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-04-01 08:59, Damien Le Moal wrote:
> On 4/1/24 04:34, Dragan Simic wrote:
>> 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@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 ?

Actually, I was wrong.  When sleeping for 100 msec, msleep()
is actually the preferred variant. [1]

[1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

>>>  	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,