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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Damien Le Moal <dlemoal@kernel.org>
Cc: "Shawn Lin" <shawn.lin@rock-chips.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	linux-pci@vger.kernel.org, linux-rockchip@lists.infradead.org
Subject: Re: [PATCH] PCI: rockchip-host: Fix rockchip_pcie_host_init_port() PERST handling
Date: Fri, 29 Mar 2024 07:46:16 -0500	[thread overview]
Message-ID: <20240329124616.GA1639231@bhelgaas> (raw)
In-Reply-To: <20240329084407.1050307-1-dlemoal@kernel.org>

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
> 