diff mbox series

scsi: ufs: qcom: Clarify comments about the initial phy_gear

Message ID 20240123-ufs-reinit-comments-v1-1-ff2b3532d7fe@redhat.com
State New
Headers show
Series scsi: ufs: qcom: Clarify comments about the initial phy_gear | expand

Commit Message

Andrew Halaney Jan. 23, 2024, 7:13 p.m. UTC
The comments that currently are within the hw_ver < 4 conditional
are misleading. They really apply to various branches of the
conditionals there and incorrectly state that the phy_gear value
can increase.

Right now the logic is to:

    * Default to max supported gear for phy_gear
    * Set phy_gear to minimum value if version < 4 since those versions
      only support one PHY init sequence (and therefore don't need reinit)
    * Set phy_gear to the optimal value if the device version is already
      populated in the controller registers on boot

Let's move some of the comment to outside the if statement and clean up
the bit left about switching to a higher gear on reinit. This way the
comment more accurately reflects the logic.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
This is a minor comment cleanup inspired by my mistaken understanding of
the flow over at [0]

[0] https://lore.kernel.org/linux-arm-msm/20240123143615.GD19029@thinkpad/
---
 drivers/ufs/host/ufs-qcom.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)


---
base-commit: 319fbd8fc6d339e0a1c7b067eed870c518a13a02
change-id: 20240123-ufs-reinit-comments-17c44af62651

Best regards,

Comments

Manivannan Sadhasivam Jan. 24, 2024, 8:58 a.m. UTC | #1
On Tue, Jan 23, 2024 at 01:13:36PM -0600, Andrew Halaney wrote:
> The comments that currently are within the hw_ver < 4 conditional
> are misleading. They really apply to various branches of the
> conditionals there and incorrectly state that the phy_gear value
> can increase.
> 
> Right now the logic is to:
> 
>     * Default to max supported gear for phy_gear
>     * Set phy_gear to minimum value if version < 4 since those versions
>       only support one PHY init sequence (and therefore don't need reinit)
>     * Set phy_gear to the optimal value if the device version is already
>       populated in the controller registers on boot
> 
> Let's move some of the comment to outside the if statement and clean up
> the bit left about switching to a higher gear on reinit. This way the
> comment more accurately reflects the logic.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> This is a minor comment cleanup inspired by my mistaken understanding of
> the flow over at [0]
> 
> [0] https://lore.kernel.org/linux-arm-msm/20240123143615.GD19029@thinkpad/
> ---
>  drivers/ufs/host/ufs-qcom.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 39eef470f8fa..d9ec2dfbbda4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -843,15 +843,20 @@ static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host)
>  	struct ufs_host_params *host_params = &host->host_params;
>  	u32 val, dev_major;
>  
> +	/*
> +	 * Default to powering up the PHY to the max gear possible, which is
> +	 * backwards compatible with lower gears but not optimal from
> +	 * a power usage point of view. After device negotiation, if the
> +	 * gear is lower a reinit will be performed to program the PHY
> +	 * to the ideal gear for this combo of controller and device.
> +	 */
>  	host->phy_gear = host_params->hs_tx_gear;
>  
>  	if (host->hw_ver.major < 0x4) {
>  		/*
> -		 * For controllers whose major HW version is < 4, power up the
> -		 * PHY using minimum supported gear (UFS_HS_G2). Switching to
> -		 * max gear will be performed during reinit if supported.
> -		 * For newer controllers, whose major HW version is >= 4, power
> -		 * up the PHY using max supported gear.
> +		 * These controllers only have one PHY init sequence,
> +		 * let's power up the PHY using that (the minimum supported
> +		 * gear, UFS_HS_G2).
>  		 */
>  		host->phy_gear = UFS_HS_G2;
>  	} else if (host->hw_ver.major >= 0x5) {
> 
> ---
> base-commit: 319fbd8fc6d339e0a1c7b067eed870c518a13a02
> change-id: 20240123-ufs-reinit-comments-17c44af62651
> 
> Best regards,
> -- 
> Andrew Halaney <ahalaney@redhat.com>
>
Martin K. Petersen Jan. 25, 2024, 2:34 a.m. UTC | #2
Andrew,

> The comments that currently are within the hw_ver < 4 conditional are
> misleading. They really apply to various branches of the conditionals
> there and incorrectly state that the phy_gear value can increase.

Applied to 6.9/scsi-staging, thanks!
Martin K. Petersen Jan. 30, 2024, 2:26 a.m. UTC | #3
On Tue, 23 Jan 2024 13:13:36 -0600, Andrew Halaney wrote:

> The comments that currently are within the hw_ver < 4 conditional
> are misleading. They really apply to various branches of the
> conditionals there and incorrectly state that the phy_gear value
> can increase.
> 
> Right now the logic is to:
> 
> [...]

Applied to 6.9/scsi-queue, thanks!

[1/1] scsi: ufs: qcom: Clarify comments about the initial phy_gear
      https://git.kernel.org/mkp/scsi/c/883a8b451cf4
diff mbox series

Patch

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 39eef470f8fa..d9ec2dfbbda4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -843,15 +843,20 @@  static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host)
 	struct ufs_host_params *host_params = &host->host_params;
 	u32 val, dev_major;
 
+	/*
+	 * Default to powering up the PHY to the max gear possible, which is
+	 * backwards compatible with lower gears but not optimal from
+	 * a power usage point of view. After device negotiation, if the
+	 * gear is lower a reinit will be performed to program the PHY
+	 * to the ideal gear for this combo of controller and device.
+	 */
 	host->phy_gear = host_params->hs_tx_gear;
 
 	if (host->hw_ver.major < 0x4) {
 		/*
-		 * For controllers whose major HW version is < 4, power up the
-		 * PHY using minimum supported gear (UFS_HS_G2). Switching to
-		 * max gear will be performed during reinit if supported.
-		 * For newer controllers, whose major HW version is >= 4, power
-		 * up the PHY using max supported gear.
+		 * These controllers only have one PHY init sequence,
+		 * let's power up the PHY using that (the minimum supported
+		 * gear, UFS_HS_G2).
 		 */
 		host->phy_gear = UFS_HS_G2;
 	} else if (host->hw_ver.major >= 0x5) {