Re: [PATCH] scsi: ufs: qcom: Clarify comments about the initial phy_gear

Manivannan Sadhasivam
2024-01-24T08:58:50Z

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>
>

--
மணிவண்ணன் சதாசிவம்