| Message ID | 20240215-topic-pci_sleep-v2-1-79334884546b@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v2] PCI: dwc: Use the correct sleep function in wait_for_link | expand |
On Wed, Mar 27, 2024 at 07:24:49PM +0100, Konrad Dybcio wrote: > According to [1], msleep should be used for large sleeps, such as the > 100-ish ms one in this function. Comply with the guide and use it. > > [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > Tested on Qualcomm SC8280XP CRD > --- > Changes in v2: > - Rename the define > - Sleep for 90ms (the lower boundary) instead of 100 > - Link to v1: https://lore.kernel.org/r/20240215-topic-pci_sleep-v1-1-7ac79ac9739a@linaro.org Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
On Wed, Mar 27, 2024 at 07:24:49PM +0100, Konrad Dybcio wrote: > According to [1], msleep should be used for large sleeps, such as the > 100-ish ms one in this function. Comply with the guide and use it. > > [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> Thanks for fixing this up! No need to repost, but whoever applies this, please update the subject to be more specific: s/the correct sleep function/msleep()/ s/wait_for_link/dw_pcie_wait_for_link()/ Also update the doc link to something like this since timers-howto.txt no longer exists, and even timers-howto might be renamed or moved in the future: https://docs.kernel.org/6.8/timers/timers-howto.html > --- > Tested on Qualcomm SC8280XP CRD > --- > Changes in v2: > - Rename the define > - Sleep for 90ms (the lower boundary) instead of 100 > - Link to v1: https://lore.kernel.org/r/20240215-topic-pci_sleep-v1-1-7ac79ac9739a@linaro.org > --- > drivers/pci/controller/dwc/pcie-designware.c | 2 +- > drivers/pci/controller/dwc/pcie-designware.h | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 250cf7f40b85..62915e4b2ebd 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -655,7 +655,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) > if (dw_pcie_link_up(pci)) > break; > > - usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > + msleep(LINK_WAIT_SLEEP_MS); > } > > if (retries >= LINK_WAIT_MAX_RETRIES) { > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 26dae4837462..b17e8ff54f55 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -63,8 +63,7 @@ > > /* Parameters for the waiting for link up routine */ > #define LINK_WAIT_MAX_RETRIES 10 > -#define LINK_WAIT_USLEEP_MIN 90000 > -#define LINK_WAIT_USLEEP_MAX 100000 > +#define LINK_WAIT_SLEEP_MS 90 > > /* Parameters for the waiting for iATU enabled routine */ > #define LINK_WAIT_MAX_IATU_RETRIES 5 > > --- > base-commit: 26074e1be23143b2388cacb36166766c235feb7c > change-id: 20240215-topic-pci_sleep-368108a1fb6f > > Best regards, > -- > Konrad Dybcio <konrad.dybcio@linaro.org> >
On Wed, Mar 27, 2024 at 07:24:49PM +0100, Konrad Dybcio wrote: > According to [1], msleep should be used for large sleeps, such as the > 100-ish ms one in this function. Comply with the guide and use it. > > [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > Tested on Qualcomm SC8280XP CRD > --- > Changes in v2: > - Rename the define > - Sleep for 90ms (the lower boundary) instead of 100 > - Link to v1: https://lore.kernel.org/r/20240215-topic-pci_sleep-v1-1-7ac79ac9739a@linaro.org > --- > drivers/pci/controller/dwc/pcie-designware.c | 2 +- > drivers/pci/controller/dwc/pcie-designware.h | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 250cf7f40b85..62915e4b2ebd 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -655,7 +655,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) > if (dw_pcie_link_up(pci)) > break; > > - usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > + msleep(LINK_WAIT_SLEEP_MS); > } > > if (retries >= LINK_WAIT_MAX_RETRIES) { > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 26dae4837462..b17e8ff54f55 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -63,8 +63,7 @@ > > /* Parameters for the waiting for link up routine */ > #define LINK_WAIT_MAX_RETRIES 10 > -#define LINK_WAIT_USLEEP_MIN 90000 > -#define LINK_WAIT_USLEEP_MAX 100000 > +#define LINK_WAIT_SLEEP_MS 90 > > /* Parameters for the waiting for iATU enabled routine */ > #define LINK_WAIT_MAX_IATU_RETRIES 5 > > --- > base-commit: 26074e1be23143b2388cacb36166766c235feb7c > change-id: 20240215-topic-pci_sleep-368108a1fb6f > > Best regards, > -- > Konrad Dybcio <konrad.dybcio@linaro.org> >
Hello, > According to [1], msleep should be used for large sleeps, such as the > 100-ish ms one in this function. Comply with the guide and use it. > > [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt Applied to controller/dwc, thank you! [1/1] PCI: dwc: Use msleep() in dw_pcie_wait_for_link() https://git.kernel.org/pci/pci/c/0189ae3b7f87 Krzysztof
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 250cf7f40b85..62915e4b2ebd 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -655,7 +655,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) if (dw_pcie_link_up(pci)) break; - usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); + msleep(LINK_WAIT_SLEEP_MS); } if (retries >= LINK_WAIT_MAX_RETRIES) { diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 26dae4837462..b17e8ff54f55 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -63,8 +63,7 @@ /* Parameters for the waiting for link up routine */ #define LINK_WAIT_MAX_RETRIES 10 -#define LINK_WAIT_USLEEP_MIN 90000 -#define LINK_WAIT_USLEEP_MAX 100000 +#define LINK_WAIT_SLEEP_MS 90 /* Parameters for the waiting for iATU enabled routine */ #define LINK_WAIT_MAX_IATU_RETRIES 5
According to [1], msleep should be used for large sleeps, such as the 100-ish ms one in this function. Comply with the guide and use it. [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Tested on Qualcomm SC8280XP CRD --- Changes in v2: - Rename the define - Sleep for 90ms (the lower boundary) instead of 100 - Link to v1: https://lore.kernel.org/r/20240215-topic-pci_sleep-v1-1-7ac79ac9739a@linaro.org --- drivers/pci/controller/dwc/pcie-designware.c | 2 +- drivers/pci/controller/dwc/pcie-designware.h | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) --- base-commit: 26074e1be23143b2388cacb36166766c235feb7c change-id: 20240215-topic-pci_sleep-368108a1fb6f Best regards,