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@xxxxxxxxxx> >> --- >> >> 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, -- Damien Le Moal Western Digital Research
