diff mbox series

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

Message ID 20240330035043.1546087-1-dlemoal@kernel.org
State New
Headers show
Series [v2] PCI: rockchip-host: Fix rockchip_pcie_host_init_port() PERST# handling | expand

Commit Message

Damien Le Moal March 30, 2024, 3:50 a.m. UTC 
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(+)

Comments

Dragan Simic March 31, 2024, 7:34 p.m. UTC | #1 
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@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,
Damien Le Moal April 1, 2024, 6:59 a.m. UTC | #2 
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@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,
Dragan Simic April 1, 2024, 5:28 p.m. UTC | #3 
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,
Bjorn Helgaas April 9, 2024, 4:15 p.m. UTC | #4 
On Sat, Mar 30, 2024 at 12:50:43PM +0900, 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).

Reference current spec, e.g., "PCIe CEM r5.1, sec 2.9.2" and a note
about why you mention two parameters here but the code change only
uses one of them.

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

Current spec, e.g., "PCIe r6.0, sec 6.6.1".

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

s/PESRT/PERST#/

This is two separate changes that really would be better as separate
patches.

> 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.
> +	 */

Remove comment completely (given use of PCIE_T_PVPERL_MS below).

> +	msleep(100);

s/100/PCIE_T_PVPERL_MS/

>  	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);

Please add a #define for this since it's generic across all PCIe, not
just Rockchip.

I don't think the PCIe spec actually names this parameter.  It's
similar to T_rhfa (Conventional PCI r3.0, sec 4.3.2), although I think
devices must be ready to accept config cycles after T_rhfa.

This delay is a little different because IIUC, a PCIe device doesn't
have to be "Configuration Ready"; after 100ms, it only has to be able
to respond with a "Request Retry Status" completion if it isn't
Configuration Ready yet.

So I think we should add something like

  #define PCIE_T_RRS_READY_MS   100

to drivers/pci/pci.h for this case.

>  	/* 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,
> -- 
> 2.44.0
>
diff mbox series

Patch

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);
 	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);
+
 	/* 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,