| Message ID | 20220422132140.313390-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Qcom UFS driver updates | expand |
On Fri, Apr 22, 2022 at 06:51:36PM +0530, Manivannan Sadhasivam wrote: > On Qcom UFS platforms, the reset control line seems to be optional > (for SoCs like MSM8996 and probably for others too). The current logic > tries to mimic the `devm_reset_control_get_optional()` API but it also > continues the probe if there is an error with the declared reset line in > DT/ACPI. > > In an ideal case, if the reset line is not declared in DT/ACPI, the probe > should continue. But if there is problem in acquiring the declared reset > line (like EPROBE_DEFER) it should fail and return the appropriate error > code. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/scsi/ufs/ufs-qcom.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 0d2e950d0865..5db0fd922062 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -1002,13 +1002,13 @@ static int ufs_qcom_init(struct ufs_hba *hba) > host->hba = hba; > ufshcd_set_variant(hba, host); > > - /* Setup the reset control of HCI */ > - host->core_reset = devm_reset_control_get(hba->dev, "rst"); > + /* Setup the optional reset control of HCI */ > + host->core_reset = devm_reset_control_get_optional(hba->dev, "rst"); > if (IS_ERR(host->core_reset)) { > err = PTR_ERR(host->core_reset); > - dev_warn(dev, "Failed to get reset control %d\n", err); > - host->core_reset = NULL; > - err = 0; > + if (err != -EPROBE_DEFER) > + dev_err(dev, "Failed to get reset control %d\n", err); Could we use dev_err_probe() here? Otherwise, looks good to me. > + goto out_variant_clear; > } > > /* Fire up the reset controller. Failure here is non-fatal. */ > -- > 2.25.1 >
On 4/22/22 06:21, Manivannan Sadhasivam wrote: > In ufs_qcom_dev_ref_clk_ctrl(), it was noted that the ref_clk needs to be > stable for atleast 1us. Eventhough there is wmb() to make sure the write ^ ^ Some spaces are missing. > gets "completed", there is no guarantee that the write actually reached > the UFS device. There is a good chance that the write could be stored in > a Write Buffer (WB). In that case, eventhough the CPU waits for 1us, the ^ missing space---------------------------- > ref_clk might not be stable for that period. > > So lets do a readl() to make sure that the previous write has reached the > UFS device before udelay(). > > Cc: stable@vger.kernel.org > Fixes: f06fcc7155dc ("scsi: ufs-qcom: add QUniPro hardware support and power optimizations") > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/scsi/ufs/ufs-qcom.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 5f0a8f646eb5..5b9986c63eed 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -690,6 +690,12 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable) > /* ensure that ref_clk is enabled/disabled before we return */ > wmb(); > > + /* > + * Make sure the write to ref_clk reaches the destination and > + * not stored in a Write Buffer (WB). > + */ > + readl(host->dev_ref_clk_ctrl_mmio); > + > /* > * If we call hibern8 exit after this, we need to make sure that > * device ref_clk is stable for at least 1us before the hibern8 The comment above the wmb() call looks wrong to me. How about removing that wmb() call? Thanks, Bart.
On Fri, Apr 22, 2022 at 10:40:10AM -0500, Andrew Halaney wrote: > On Fri, Apr 22, 2022 at 06:51:36PM +0530, Manivannan Sadhasivam wrote: > > On Qcom UFS platforms, the reset control line seems to be optional > > (for SoCs like MSM8996 and probably for others too). The current logic > > tries to mimic the `devm_reset_control_get_optional()` API but it also > > continues the probe if there is an error with the declared reset line in > > DT/ACPI. > > > > In an ideal case, if the reset line is not declared in DT/ACPI, the probe > > should continue. But if there is problem in acquiring the declared reset > > line (like EPROBE_DEFER) it should fail and return the appropriate error > > code. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/scsi/ufs/ufs-qcom.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > > index 0d2e950d0865..5db0fd922062 100644 > > --- a/drivers/scsi/ufs/ufs-qcom.c > > +++ b/drivers/scsi/ufs/ufs-qcom.c > > @@ -1002,13 +1002,13 @@ static int ufs_qcom_init(struct ufs_hba *hba) > > host->hba = hba; > > ufshcd_set_variant(hba, host); > > > > - /* Setup the reset control of HCI */ > > - host->core_reset = devm_reset_control_get(hba->dev, "rst"); > > + /* Setup the optional reset control of HCI */ > > + host->core_reset = devm_reset_control_get_optional(hba->dev, "rst"); > > if (IS_ERR(host->core_reset)) { > > err = PTR_ERR(host->core_reset); > > - dev_warn(dev, "Failed to get reset control %d\n", err); > > - host->core_reset = NULL; > > - err = 0; > > + if (err != -EPROBE_DEFER) > > + dev_err(dev, "Failed to get reset control %d\n", err); > > Could we use dev_err_probe() here? > Yes. Will do the same for patch 2/5 as well. Thanks, Mani > Otherwise, looks good to me. > > > + goto out_variant_clear; > > } > > > > /* Fire up the reset controller. Failure here is non-fatal. */ > > -- > > 2.25.1 > > >
On Fri, Apr 22, 2022 at 10:03:22PM -0700, Bart Van Assche wrote: > On 4/22/22 06:21, Manivannan Sadhasivam wrote: > > In ufs_qcom_dev_ref_clk_ctrl(), it was noted that the ref_clk needs to be > > stable for atleast 1us. Eventhough there is wmb() to make sure the write > ^ ^ > Some spaces are missing. > > > gets "completed", there is no guarantee that the write actually reached > > the UFS device. There is a good chance that the write could be stored in > > a Write Buffer (WB). In that case, eventhough the CPU waits for 1us, the > ^ > missing space---------------------------- > Kind of used to it ;) Will fix it. > > ref_clk might not be stable for that period. > > > > So lets do a readl() to make sure that the previous write has reached the > > UFS device before udelay(). > > > > Cc: stable@vger.kernel.org > > Fixes: f06fcc7155dc ("scsi: ufs-qcom: add QUniPro hardware support and power optimizations") > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/scsi/ufs/ufs-qcom.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > > index 5f0a8f646eb5..5b9986c63eed 100644 > > --- a/drivers/scsi/ufs/ufs-qcom.c > > +++ b/drivers/scsi/ufs/ufs-qcom.c > > @@ -690,6 +690,12 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable) > > /* ensure that ref_clk is enabled/disabled before we return */ > > wmb(); > > + /* > > + * Make sure the write to ref_clk reaches the destination and > > + * not stored in a Write Buffer (WB). > > + */ > > + readl(host->dev_ref_clk_ctrl_mmio); > > + > > /* > > * If we call hibern8 exit after this, we need to make sure that > > * device ref_clk is stable for at least 1us before the hibern8 > > The comment above the wmb() call looks wrong to me. How about removing that > wmb() call? > Hmm, yes it could be removed as well. readl() on weakly ordered architectures include a control dependency [1] so there is no way the instructions after it can be speculated. Thanks, Mani [1] https://www.spinics.net/lists/arm-kernel/msg689858.html > Thanks, > > Bart.