Received-SPF: pass (google.com: domain of linux-kernel+bounces-31530-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 19 Jan 2024 14:07:15 -0600
From: Andrew Halaney <ahalaney@redhat.com>
To: Eric Chanudet <echanude@redhat.com>
Cc: Bjorn Andersson <andersson@kernel.org>, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, 
	"James E.J. Bottomley" <jejb@linux.ibm.com>, "Martin K. Petersen" <martin.petersen@oracle.com>, 
	linux-arm-msm@vger.kernel.org, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scsi: ufs: qcom: avoid re-init quirk when gears match
Message-ID: <3xnedre2d32rkad6n2ln4rrah7sgg6epxnzsdm54uab3zrutnz@fww7wb5mvykj>
References: <20240119185537.3091366-11-echanude@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240119185537.3091366-11-echanude@redhat.com>

On Fri, Jan 19, 2024 at 01:55:47PM -0500, Eric Chanudet wrote:
> On sa8775p-ride, probing the hba will go through the
> UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH path although the power info
> are same during the second init.
> 
> If the host is at least v4, ufs_qcom_get_hs_gear() picked the highest
> supported gear when setting the host_params. After the negotiation, if
> the host and device are on the same gear, it is the highest gear
> supported between the two. Skip the re-init to save some time.
> 
> Signed-off-by: Eric Chanudet <echanude@redhat.com>
> ---
> 
> "trace_event=ufs:ufshcd_init" reports the time spent where the re-init
> quirk is performed. On sa8775p-ride:
> Baseline:
>   0.355879: ufshcd_init: 1d84000.ufs: took 103377 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0
> With this patch:
>   0.297676: ufshcd_init: 1d84000.ufs: took 43553 usecs, dev_state: UFS_ACTIVE_PWR_MODE, link_state: UIC_LINK_ACTIVE_STATE, err 0
> 
>  drivers/ufs/host/ufs-qcom.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 39eef470f8fa..f9f161340e78 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -738,8 +738,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>  		 * the second init can program the optimal PHY settings. This allows one to start
>  		 * the first init with either the minimum or the maximum support gear.
>  		 */
> -		if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> +		if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
> +			if (host->hw_ver.major >= 0x4 &&

Is this check really necessary?

The initial phy_gear state is something like this (my phrasing of
ufs_qcom_set_phy_gear()):

    if hw_ver < 4:
        # Comments about powering up with minimum gear (with no
        # reasoning in the comment afaict), and mentions switching
        # to higher gear in reinit quirk. This is opposite of the later
        # versions which start at the max and scale down
        phy_gear = UFS_HS_G2

    else if hw_ver == 4:
        phy_gear = hs_tx_gear # (so afaict always UFS_HS_G4)

    else if hw_ver >= 5:
        phy_gear = hs_tx_gear # (What ever the max is for this version)

        if dev_major:
            # Clears the reinit quirk in ufs_qcom_set_phy_gear() if the
            # device version is provided by bootloader / controller
            # because we already found it out and can init directly
            # to the ideal gear
            quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH

            if dev_major < 4:
                # Sets gear to UFS_HS_G4 to save power for UFS 3.1 and
                # older devices
                phy_gear = UFS_HS_G4

I guess what I'm saying is that I'm not totally seeing how this check
is dependent on the controller version. To me, if we're in the ideal
gear already and we know it, we should *not* reinit, no matter what the
controller version is. That's assuming there's not some other reasoning
for the quirk, but as far as I understand it the quirk exists because we
have to start with *some* phy gear value so we can talk to the device,
and once we discover what the device is capable of it makes sense to
scale down (or up for older controllers) to the ideal gear setting for
the attached device. Unfortunately to do the change we have to
reprogram the phy which I guess is only acceptable if we reprogram
everything (hence the reinit).

Does that make sense or do you think I'm missing something? I think say
even for an older controller this makes sense, if its attached to a
UFS_HS_G2 capable device there is no reason to reinit since we started
up in the ideal configuration.

Thanks,
Andrew

> +			    host_params->hs_tx_gear == dev_req_params->gear_tx)
> +				hba->quirks &= ~UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
>  			host->phy_gear = dev_req_params->gear_tx;
> +		}
>  
>  		/* enable the device ref clock before changing to HS mode */
>  		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
> -- 
> 2.43.0
> 
>