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

[Dragan Simic]

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().

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