| Message ID | 20190529094352.5961-1-niklas.cassel@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] PCI: qcom: Ensure that PERST is asserted for at least 100 ms | expand |
On Wed, May 29, 2019 at 11:43:52AM +0200, 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. I think this patch is absolutely the right thing to do. We don't know what might be plugged in, so we should support arbitrary endpoints. One could imagine some sort of DT property to identify closed environments where there is no need to support arbitrary endpoints and it might be desirable to reduce the delay. But that should wait until there's a need for it and if the need *does* arise, hopefully the definition of such a property could be generic across all SoCs. > 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." It'd be nice to include the complete citation, i.e., "PCI Express Card Electromechanical Specification r2.0, sec 2.2" > Add a sleep of 100 ms before deasserting PERST, in order to ensure that > we are compliant with the spec. > > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver") > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com> > Cc: stable@vger.kernel.org # 4.5+ > --- > Changes since v1: > Move the sleep into qcom_ep_reset_deassert() > > 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..5d1713069d14 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie) > > static void qcom_ep_reset_deassert(struct qcom_pcie *pcie) > { > + /* Ensure that PERST has been asserted for at least 100 ms */ > + msleep(100); > gpiod_set_value_cansleep(pcie->reset, 0); > usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500); > } > -- > 2.21.0 >
On Wed, May 29, 2019 at 07:52:32AM -0500, Bjorn Helgaas wrote: > On Wed, May 29, 2019 at 11:43:52AM +0200, 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. > > I think this patch is absolutely the right thing to do. We don't know > what might be plugged in, so we should support arbitrary endpoints. > > One could imagine some sort of DT property to identify closed > environments where there is no need to support arbitrary endpoints and > it might be desirable to reduce the delay. But that should wait > until there's a need for it and if the need *does* arise, hopefully > the definition of such a property could be generic across all SoCs. > > > 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." > > It'd be nice to include the complete citation, i.e., "PCI Express Card > Electromechanical Specification r2.0, sec 2.2" I will add it and merge the patch. Thanks, Lorenzo > > Add a sleep of 100 ms before deasserting PERST, in order to ensure that > > we are compliant with the spec. > > > > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver") > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com> > > Cc: stable@vger.kernel.org # 4.5+ > > --- > > Changes since v1: > > Move the sleep into qcom_ep_reset_deassert() > > > > 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..5d1713069d14 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > @@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie) > > > > static void qcom_ep_reset_deassert(struct qcom_pcie *pcie) > > { > > + /* Ensure that PERST has been asserted for at least 100 ms */ > > + msleep(100); > > gpiod_set_value_cansleep(pcie->reset, 0); > > usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500); > > } > > -- > > 2.21.0 > >
On Wed, May 29, 2019 at 11:43:52AM +0200, 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. > > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver") > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com> > Cc: stable@vger.kernel.org # 4.5+ > --- > Changes since v1: > Move the sleep into qcom_ep_reset_deassert() > > drivers/pci/controller/dwc/pcie-qcom.c | 2 ++ > 1 file changed, 2 insertions(+) Applied to pci/qcom for v5.3, thanks. Lorenzo > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 0ed235d560e3..5d1713069d14 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie) > > static void qcom_ep_reset_deassert(struct qcom_pcie *pcie) > { > + /* Ensure that PERST has been asserted for at least 100 ms */ > + msleep(100); > gpiod_set_value_cansleep(pcie->reset, 0); > usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500); > } > -- > 2.21.0 >
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 0ed235d560e3..5d1713069d14 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -178,6 +178,8 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie) static void qcom_ep_reset_deassert(struct qcom_pcie *pcie) { + /* Ensure that PERST has been asserted for at least 100 ms */ + msleep(100); gpiod_set_value_cansleep(pcie->reset, 0); usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500); }
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. Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver") Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com> Cc: stable@vger.kernel.org # 4.5+ --- Changes since v1: Move the sleep into qcom_ep_reset_deassert() drivers/pci/controller/dwc/pcie-qcom.c | 2 ++ 1 file changed, 2 insertions(+)