| Message ID | 20190523194409.17718-1-niklas.cassel@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PCI: qcom: Ensure that PERST is asserted for at least 100 ms | expand |
On 23/05/2019 21:44, Niklas Cassel wrote: > Currently, there is only a 1 ms sleep after asserting PERST. > > Reading the datasheets for different endpoints, some require PERST to be > asserted for 10 ms in order for the endpoint to perform a reset, others > require it to be asserted for 50 ms. > > Several SoCs using this driver uses PCIe Mini Card, where we don't know > what endpoint will be plugged in. > > The PCI Express Card Electromechanical Specification specifies: > "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from > the power rails achieving specified operating limits." > > Add a sleep of 100 ms before deasserting PERST, in order to ensure that > we are compliant with the spec. > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 0ed235d560e3..cae24376237c 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp) > if (IS_ENABLED(CONFIG_PCI_MSI)) > dw_pcie_msi_init(pp); > > + /* Ensure that PERST has been asserted for at least 100 ms */ > + msleep(100); > qcom_ep_reset_deassert(pcie); > > ret = qcom_pcie_establish_link(pcie); Currently, qcom_ep_reset_assert() and qcom_ep_reset_deassert() both include a call to usleep_range() of 1.0 to 1.5 ms Can we git rid of both if we sleep 100 ms before qcom_ep_reset_deassert? Should the msleep() call be included in one of the two wrappers? Regards.
On Thu 23 May 12:44 PDT 2019, Niklas Cassel wrote: > Currently, there is only a 1 ms sleep after asserting PERST. > > Reading the datasheets for different endpoints, some require PERST to be > asserted for 10 ms in order for the endpoint to perform a reset, others > require it to be asserted for 50 ms. > > Several SoCs using this driver uses PCIe Mini Card, where we don't know > what endpoint will be plugged in. > > The PCI Express Card Electromechanical Specification specifies: > "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from > the power rails achieving specified operating limits." > > Add a sleep of 100 ms before deasserting PERST, in order to ensure that > we are compliant with the spec. > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 0ed235d560e3..cae24376237c 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp) > if (IS_ENABLED(CONFIG_PCI_MSI)) > dw_pcie_msi_init(pp); > > + /* Ensure that PERST has been asserted for at least 100 ms */ > + msleep(100); > qcom_ep_reset_deassert(pcie); > > ret = qcom_pcie_establish_link(pcie); > -- > 2.21.0 >
On Fri, May 24, 2019 at 02:43:00PM +0200, Marc Gonzalez wrote: > On 23/05/2019 21:44, Niklas Cassel wrote: > > > Currently, there is only a 1 ms sleep after asserting PERST. > > > > Reading the datasheets for different endpoints, some require PERST to be > > asserted for 10 ms in order for the endpoint to perform a reset, others > > require it to be asserted for 50 ms. > > > > Several SoCs using this driver uses PCIe Mini Card, where we don't know > > what endpoint will be plugged in. > > > > The PCI Express Card Electromechanical Specification specifies: > > "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from > > the power rails achieving specified operating limits." > > > > Add a sleep of 100 ms before deasserting PERST, in order to ensure that > > we are compliant with the spec. > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-qcom.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > index 0ed235d560e3..cae24376237c 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp) > > if (IS_ENABLED(CONFIG_PCI_MSI)) > > dw_pcie_msi_init(pp); > > > > + /* Ensure that PERST has been asserted for at least 100 ms */ > > + msleep(100); > > qcom_ep_reset_deassert(pcie); > > > > ret = qcom_pcie_establish_link(pcie); > > Currently, qcom_ep_reset_assert() and qcom_ep_reset_deassert() both include > a call to usleep_range() of 1.0 to 1.5 ms > > Can we git rid of both if we sleep 100 ms before qcom_ep_reset_deassert? These two sleeps after asserting/deasserting reset in qcom_ep_reset_assert()/ qcom_ep_reset_deassert() matches the sleeps in: https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1942 and https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1949 I would rather not remove these since that might affect existing devices. This new sleep matches matches the sleep in: https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n3926 > > Should the msleep() call be included in one of the two wrappers? This new sleep could be moved into qcom_ep_reset_deassert(), added before the gpiod_set_value_cansleep(pcie->reset, 0) call, if Stanimir prefers it to be placed there instead. Kind regards, Niklas
Hi Niklas, On 27.05.19 г. 20:15 ч., Niklas Cassel wrote: > On Fri, May 24, 2019 at 02:43:00PM +0200, Marc Gonzalez wrote: >> On 23/05/2019 21:44, Niklas Cassel wrote: >> >>> Currently, there is only a 1 ms sleep after asserting PERST. >>> >>> Reading the datasheets for different endpoints, some require PERST to be >>> asserted for 10 ms in order for the endpoint to perform a reset, others >>> require it to be asserted for 50 ms. >>> >>> Several SoCs using this driver uses PCIe Mini Card, where we don't know >>> what endpoint will be plugged in. >>> >>> The PCI Express Card Electromechanical Specification specifies: >>> "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from >>> the power rails achieving specified operating limits." >>> >>> Add a sleep of 100 ms before deasserting PERST, in order to ensure that >>> we are compliant with the spec. >>> >>> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> >>> --- >>> drivers/pci/controller/dwc/pcie-qcom.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >>> index 0ed235d560e3..cae24376237c 100644 >>> --- a/drivers/pci/controller/dwc/pcie-qcom.c >>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >>> @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp) >>> if (IS_ENABLED(CONFIG_PCI_MSI)) >>> dw_pcie_msi_init(pp); >>> >>> + /* Ensure that PERST has been asserted for at least 100 ms */ >>> + msleep(100); >>> qcom_ep_reset_deassert(pcie); >>> >>> ret = qcom_pcie_establish_link(pcie); >> >> Currently, qcom_ep_reset_assert() and qcom_ep_reset_deassert() both include >> a call to usleep_range() of 1.0 to 1.5 ms >> >> Can we git rid of both if we sleep 100 ms before qcom_ep_reset_deassert? > > These two sleeps after asserting/deasserting reset in qcom_ep_reset_assert()/ > qcom_ep_reset_deassert() matches the sleeps in: > https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1942 > > and > > https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n1949 > > I would rather not remove these since that might affect existing devices. > > > This new sleep matches matches the sleep in: > https://source.codeaurora.org/quic/la/kernel/msm-4.14/tree/drivers/pci/host/pci-msm.c?h=LA.UM.7.1.r1-14000-sm8150.0#n3926 > >> >> Should the msleep() call be included in one of the two wrappers? > > This new sleep could be moved into qcom_ep_reset_deassert(), > added before the gpiod_set_value_cansleep(pcie->reset, 0) call, > if Stanimir prefers it to be placed there instead. yes, please move the sleep in qcom_ep_reset_deassert() with that: Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com> regards, Stan
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 0ed235d560e3..cae24376237c 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -1110,6 +1110,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp) if (IS_ENABLED(CONFIG_PCI_MSI)) dw_pcie_msi_init(pp); + /* Ensure that PERST has been asserted for at least 100 ms */ + msleep(100); qcom_ep_reset_deassert(pcie); ret = qcom_pcie_establish_link(pcie);
Currently, there is only a 1 ms sleep after asserting PERST. Reading the datasheets for different endpoints, some require PERST to be asserted for 10 ms in order for the endpoint to perform a reset, others require it to be asserted for 50 ms. Several SoCs using this driver uses PCIe Mini Card, where we don't know what endpoint will be plugged in. The PCI Express Card Electromechanical Specification specifies: "On power up, the deassertion of PERST# is delayed 100 ms (TPVPERL) from the power rails achieving specified operating limits." Add a sleep of 100 ms before deasserting PERST, in order to ensure that we are compliant with the spec. Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 2 ++ 1 file changed, 2 insertions(+)