| Message ID | 20231208065902.11006-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | scsi: ufs: qcom: Code cleanups | expand |
On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote: > devm_kzalloc() will itself print the error message on failure. So let's get > rid of the redundant error message in ufs_qcom_init(). > > 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 b141dd2a9346..05a9a25bc34c 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1109,10 +1109,8 @@ static int ufs_qcom_init(struct ufs_hba *hba) > struct ufs_clk_info *clki; > > host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL); > - if (!host) { > - dev_err(dev, "%s: no memory for qcom ufs host\n", __func__); > + if (!host) > return -ENOMEM; > - } > > /* Make a two way bind between the qcom host and the hba */ > host->hba = hba; Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>
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 > >> 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 Fri, Dec 08, 2023 at 12:28:45PM +0530, Manivannan Sadhasivam wrote: > Hello, > > This series has code some cleanups to the Qcom UFS driver. No functional > change. In this version, I've removed code supporting legacy controllers > ver < 2.0, as the respective platforms were never supported in upstream. > > Tested on: RB5 development board based on Qcom SM8250 SoC. > > - Mani > > Changes in v2: > > * Collected review tags > * Fixed the comments from Andrew > * Added a few more patches, most notably one removing the code for old > controllers (ver < v2.0) > I took this for a spin on sa8775p-ride when developing another patch today with no issues. Certainly doesn't hit all the cases here, but: Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8775p-ride > Manivannan Sadhasivam (17): > scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks > scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key() > scsi: ufs: qcom: Fix the return value when > platform_get_resource_byname() fails > scsi: ufs: qcom: Remove superfluous variable assignments > scsi: ufs: qcom: Remove the warning message when core_reset is not > available > scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make > use of them > scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset > fails > scsi: ufs: qcom: Check the return value of > ufs_qcom_power_up_sequence() > scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() > failure > scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of > devm_gpiod_get_optional() > scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array > scsi: ufs: qcom: Sort includes alphabetically > scsi: ufs: qcom: Initialize cycles_in_1us variable in > ufs_qcom_set_core_clk_ctrl() > scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset > scsi: ufs: qcom: Remove support for host controllers older than v2.0 > scsi: ufs: qcom: Use ufshcd_rmwl() where applicable > scsi: ufs: qcom: Remove unused definitions > > drivers/ufs/core/ufshcd.c | 6 +- > drivers/ufs/host/ufs-qcom.c | 377 +++++------------------------------- > drivers/ufs/host/ufs-qcom.h | 52 +---- > include/ufs/ufshcd.h | 2 + > 4 files changed, 66 insertions(+), 371 deletions(-) > > -- > 2.25.1 >
On 8.12.2023 07:58, Manivannan Sadhasivam wrote: > Hello, > > This series has code some cleanups to the Qcom UFS driver. No functional > change. In this version, I've removed code supporting legacy controllers > ver < 2.0, as the respective platforms were never supported in upstream. > > Tested on: RB5 development board based on Qcom SM8250 SoC. > > - Mani > > Changes in v2: > > * Collected review tags > * Fixed the comments from Andrew > * Added a few more patches, most notably one removing the code for old > controllers (ver < v2.0) FWIW i found this snipped from a downstream commit from 2014: 8084 : 1.1.1 8994v1 : 1.2.0 8994v2 : 1.3.0 I'm yet to see any 8994 production device utilizing UFS (it wasn't very good or affordable in 2014/15 IIRC), so I think it's gtg. Konrad
On Sat, Dec 09, 2023 at 06:42:31PM +0100, Konrad Dybcio wrote: > On 8.12.2023 07:58, Manivannan Sadhasivam wrote: > > Hello, > > > > This series has code some cleanups to the Qcom UFS driver. No functional > > change. In this version, I've removed code supporting legacy controllers > > ver < 2.0, as the respective platforms were never supported in upstream. > > > > Tested on: RB5 development board based on Qcom SM8250 SoC. > > > > - Mani > > > > Changes in v2: > > > > * Collected review tags > > * Fixed the comments from Andrew > > * Added a few more patches, most notably one removing the code for old > > controllers (ver < v2.0) > FWIW i found this snipped from a downstream commit from 2014: > > 8084 : 1.1.1 > 8994v1 : 1.2.0 > 8994v2 : 1.3.0 > > I'm yet to see any 8994 production device utilizing UFS (it wasn't > very good or affordable in 2014/15 IIRC), so I think it's gtg. > Thanks for digging! I was told that SoCs based on UFS 1.x controllers were not widely used in production, though I don't know why. - Mani > Konrad
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 Fri, 08 Dec 2023 12:28:45 +0530, Manivannan Sadhasivam wrote: > This series has code some cleanups to the Qcom UFS driver. No functional > change. In this version, I've removed code supporting legacy controllers > ver < 2.0, as the respective platforms were never supported in upstream. > > Tested on: RB5 development board based on Qcom SM8250 SoC. > > - Mani > > [...] Applied to 6.8/scsi-queue, thanks! [01/17] scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks https://git.kernel.org/mkp/scsi/c/9caef8568831 [02/17] scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key() https://git.kernel.org/mkp/scsi/c/3bf7ab4ac30c [03/17] scsi: ufs: qcom: Fix the return value when platform_get_resource_byname() fails https://git.kernel.org/mkp/scsi/c/3a747c5cf9b6 [04/17] scsi: ufs: qcom: Remove superfluous variable assignments https://git.kernel.org/mkp/scsi/c/1f165c87ec3e [05/17] scsi: ufs: qcom: Remove the warning message when core_reset is not available https://git.kernel.org/mkp/scsi/c/d42d368647da [06/17] scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make use of them https://git.kernel.org/mkp/scsi/c/0ae7a02726bc [07/17] scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset fails https://git.kernel.org/mkp/scsi/c/d11954711499 [08/17] scsi: ufs: qcom: Check the return value of ufs_qcom_power_up_sequence() https://git.kernel.org/mkp/scsi/c/e430c0e08957 [09/17] scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() failure https://git.kernel.org/mkp/scsi/c/8291652ed8a2 [10/17] scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of devm_gpiod_get_optional() https://git.kernel.org/mkp/scsi/c/c7afadacc180 [11/17] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array https://git.kernel.org/mkp/scsi/c/e7458beab809 [12/17] scsi: ufs: qcom: Sort includes alphabetically https://git.kernel.org/mkp/scsi/c/be2e06c81a31 [13/17] scsi: ufs: qcom: Initialize cycles_in_1us variable in ufs_qcom_set_core_clk_ctrl() https://git.kernel.org/mkp/scsi/c/3b60f4564ff5 [14/17] scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset https://git.kernel.org/mkp/scsi/c/6b481af25ec0 [15/17] scsi: ufs: qcom: Remove support for host controllers older than v2.0 https://git.kernel.org/mkp/scsi/c/104cd58d9af8 [16/17] scsi: ufs: qcom: Use ufshcd_rmwl() where applicable https://git.kernel.org/mkp/scsi/c/0e9f4375db1c [17/17] scsi: ufs: qcom: Remove unused definitions https://git.kernel.org/mkp/scsi/c/cac50d04fffe
Hello, This series has code some cleanups to the Qcom UFS driver. No functional change. In this version, I've removed code supporting legacy controllers ver < 2.0, as the respective platforms were never supported in upstream. Tested on: RB5 development board based on Qcom SM8250 SoC. - Mani Changes in v2: * Collected review tags * Fixed the comments from Andrew * Added a few more patches, most notably one removing the code for old controllers (ver < v2.0) Manivannan Sadhasivam (17): scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key() scsi: ufs: qcom: Fix the return value when platform_get_resource_byname() fails scsi: ufs: qcom: Remove superfluous variable assignments scsi: ufs: qcom: Remove the warning message when core_reset is not available scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make use of them scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset fails scsi: ufs: qcom: Check the return value of ufs_qcom_power_up_sequence() scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() failure scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of devm_gpiod_get_optional() scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array scsi: ufs: qcom: Sort includes alphabetically scsi: ufs: qcom: Initialize cycles_in_1us variable in ufs_qcom_set_core_clk_ctrl() scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset scsi: ufs: qcom: Remove support for host controllers older than v2.0 scsi: ufs: qcom: Use ufshcd_rmwl() where applicable scsi: ufs: qcom: Remove unused definitions drivers/ufs/core/ufshcd.c | 6 +- drivers/ufs/host/ufs-qcom.c | 377 +++++------------------------------- drivers/ufs/host/ufs-qcom.h | 52 +---- include/ufs/ufshcd.h | 2 + 4 files changed, 66 insertions(+), 371 deletions(-)