| Message ID | 20231208065902.11006-6-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: qcom: Code cleanups | expand |
On 12/8/2023 6:59 PM, Nitin Rawat wrote: > > > On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote: >> On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote: >>> >>> >>> On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote: >>>> core_reset is optional, so there is no need to warn the user if it >>>> is not >>>> available. >>>> >>>> Reviewed-by: Andrew Halaney <ahalaney@redhat.com> >>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >>>> --- >>>> drivers/ufs/host/ufs-qcom.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >>>> index dc93b1c5ca74..d474de0739e4 100644 >>>> --- a/drivers/ufs/host/ufs-qcom.c >>>> +++ b/drivers/ufs/host/ufs-qcom.c >>>> @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba >>>> *hba) >>>> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >>>> bool reenable_intr; >>>> - if (!host->core_reset) { >>>> - dev_warn(hba->dev, "%s: reset control not set\n", __func__); >>>> + if (!host->core_reset) >>>> return 0; >>>> - } >>>> reenable_intr = hba->is_irq_enabled; >>>> disable_irq(hba->irq); >>> >>> >>> Hi Mani, >>> >>> I think core reset is not frequent. It happen during only probe ,error >>> handler. >>> >>> core reset is needed in kernel to cleanup UFS phy and controller >>> configuration before UFS HLOS operation starts as per HPG. >>> >> >> This sounds like core reset is not an optional property but a required >> one. I >> just checked the upstream DT files for all SoCs, and looks like pretty >> much all >> of them support core reset. >> >> Only MSM8996 doesn't have the reset property, but the reset is >> available in GCC. >> So we should be able to use it in dtsi. >> >> I also skimmed through the HPG and looks like core reset is not >> optional. Please >> confirm. >> >> - Mani > > > Hi Mani, > > Yes Core_reset is part of HPG sequence and is needed. > > Regards, > Nitin Hi Mani, I see this patch series is merged . So planning to keep the warn message based on above discussion. Regards, Nitin > > >> >>> Having existing warn print can be used to to debug or atleast know >>> core_reset is missed in device tree to give indication complete reset >>> hasn't >>> been done and we could still be operating in bootloader configuration. >>> >>> >>> Regards, >>> Nitin >>> >> >
On Thu, Dec 14, 2023 at 12:43:02PM +0530, Nitin Rawat wrote: > > > On 12/8/2023 6:59 PM, Nitin Rawat wrote: > > > > > > On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote: > > > On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote: > > > > > > > > > > > > On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote: > > > > > core_reset is optional, so there is no need to warn the user > > > > > if it is not > > > > > available. > > > > > > > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > --- > > > > > drivers/ufs/host/ufs-qcom.c | 4 +--- > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > > > > index dc93b1c5ca74..d474de0739e4 100644 > > > > > --- a/drivers/ufs/host/ufs-qcom.c > > > > > +++ b/drivers/ufs/host/ufs-qcom.c > > > > > @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct > > > > > ufs_hba *hba) > > > > > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > > > > > bool reenable_intr; > > > > > - if (!host->core_reset) { > > > > > - dev_warn(hba->dev, "%s: reset control not set\n", __func__); > > > > > + if (!host->core_reset) > > > > > return 0; > > > > > - } > > > > > reenable_intr = hba->is_irq_enabled; > > > > > disable_irq(hba->irq); > > > > > > > > > > > > Hi Mani, > > > > > > > > I think core reset is not frequent. It happen during only probe ,error > > > > handler. > > > > > > > > core reset is needed in kernel to cleanup UFS phy and controller > > > > configuration before UFS HLOS operation starts as per HPG. > > > > > > > > > > This sounds like core reset is not an optional property but a > > > required one. I > > > just checked the upstream DT files for all SoCs, and looks like > > > pretty much all > > > of them support core reset. > > > > > > Only MSM8996 doesn't have the reset property, but the reset is > > > available in GCC. > > > So we should be able to use it in dtsi. > > > > > > I also skimmed through the HPG and looks like core reset is not > > > optional. Please > > > confirm. > > > > > > - Mani > > > > > > Hi Mani, > > > > Yes Core_reset is part of HPG sequence and is needed. > > > > Regards, > > Nitin > > > Hi Mani, > > I see this patch series is merged . So planning to keep the warn message > based on above discussion. > No, you should not add that warning back. As per our discussion, we concluded that core_reset is not optional, so devm_reset_control_get_exclusive() should be used to acquire the reset. If the reset property is not present in DT, then ufs_qcom_init() will fail. This also means that we should fix the binding and DT of SoCs missing the reset property. - Mani > Regards, > Nitin > > > > > > > > > > > Having existing warn print can be used to to debug or atleast know > > > > core_reset is missed in device tree to give indication complete > > > > reset hasn't > > > > been done and we could still be operating in bootloader configuration. > > > > > > > > > > > > Regards, > > > > Nitin > > > > > > > > >
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index dc93b1c5ca74..d474de0739e4 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba) struct ufs_qcom_host *host = ufshcd_get_variant(hba); bool reenable_intr; - if (!host->core_reset) { - dev_warn(hba->dev, "%s: reset control not set\n", __func__); + if (!host->core_reset) return 0; - } reenable_intr = hba->is_irq_enabled; disable_irq(hba->irq);