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

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

[Damien Le Moal]

The PCI specifications mandate that PERST be asserted for at least
100ms. Make sure that is done in rockchip_pcie_host_init_port() by
adding a 100ms sleep before 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>
---
 drivers/pci/controller/pcie-rockchip-host.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 300b9dc85ecc..d526b9d26c18 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,6 +323,11 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
+	/*
+	 * PCIe specifications mandate that PERST be asserted for at
+	 * least 100ms.
+	 */
+	msleep(100);
 	gpiod_set_value_cansleep(rockchip->ep_gpio, 1);
 
 	/* 500ms timeout value should be enough for Gen1/2 training */
-- 
2.44.0

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

[Bjorn Helgaas]

Observing the timing is the important thing here, if you can wedge
that into the subject somehow.

On Fri, Mar 29, 2024 at 05:44:07PM +0900, Damien Le Moal wrote:
> The PCI specifications mandate that PERST be asserted for at least
> 100ms. Make sure that is done in rockchip_pcie_host_init_port() by
> adding a 100ms sleep before bringing back PESRT signal to high using the
> ep_gpio GPIO. Comments are also added to clarify this behavior.

"PERST#" in spec, and to make the "assert" and "set value to 0" all
match up.

s/PESRT/PERST#/

> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 300b9dc85ecc..d526b9d26c18 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,6 +323,11 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
>  
> +	/*
> +	 * PCIe specifications mandate that PERST be asserted for at
> +	 * least 100ms.
> +	 */
> +	msleep(100);

Specific rev, section citation?  And a name for the parameter if there
is one (T_PVPERL, etc).  And hopefully a macro along the lines of
PCIE_T_PVPERL_MS since this isn't rockchip-specific.

>  	gpiod_set_value_cansleep(rockchip->ep_gpio, 1);
>  
>  	/* 500ms timeout value should be enough for Gen1/2 training */
> -- 
> 2.44.0
>