Received-SPF: pass (google.com: domain of linux-kernel+bounces-36671-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 24 Jan 2024 14:28:39 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Andrew Halaney <ahalaney@redhat.com>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	echanude@redhat.com, linux-arm-msm@vger.kernel.org,
	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scsi: ufs: qcom: Clarify comments about the initial
 phy_gear
Message-ID: <20240124085839.GH4906@thinkpad>
References: <20240123-ufs-reinit-comments-v1-1-ff2b3532d7fe@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240123-ufs-reinit-comments-v1-1-ff2b3532d7fe@redhat.com>

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

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