Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 13 Feb 2020 10:53:13 +0800
From:   Can Guo <cang@codeaurora.org>
To:     Avri Altman <Avri.Altman@wdc.com>
Cc:     asutoshd@codeaurora.org, nguyenb@codeaurora.org,
        hongwus@codeaurora.org, rnayak@codeaurora.org,
        linux-scsi@vger.kernel.org, kernel-team@android.com,
        saravanak@google.com, salyzyn@google.com,
        Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Bean Huo <beanhuo@micron.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Venkat Gopalakrishnan <venkatg@codeaurora.org>,
        Tomas Winkler <tomas.winkler@intel.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 1/2] scsi: ufs: Use ufshcd_config_pwr_mode() when scale
 gear
In-Reply-To: <MN2PR04MB6991136AD340D28D930F27F3FC1B0@MN2PR04MB6991.namprd04.prod.outlook.com>
References: <1581485910-8307-1-git-send-email-cang@codeaurora.org>
 <1581485910-8307-2-git-send-email-cang@codeaurora.org>
 <MN2PR04MB6991136AD340D28D930F27F3FC1B0@MN2PR04MB6991.namprd04.prod.outlook.com>
Message-ID: <5a54ea8f5c9f24a5c14b022d1d087a6d@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-02-12 20:21, Avri Altman wrote:
> Hi,
> 
>> 
>> When scale gear, use ufshcd_config_pwr_mode() instead of
>> ufshcd_change_power_mode() so that
>> vops_pwr_change_notify(PRE_CHANGE)
>> can be utilized to allow vendors use customized settings before change
>> the power mode.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index adcce41..67bd4f2 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1059,8 +1059,7 @@ static int ufshcd_scale_gear(struct ufs_hba 
>> *hba,
>> bool scale_up)
>>         }
>> 
>>         /* check if the power mode needs to be changed or not? */
>> -       ret = ufshcd_change_power_mode(hba, &new_pwr_info);
>> -
>> +       ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
> 
> You might want to inform ufshcd_config_pwr_mode() of the caller,
> As now it will be called much more frequently, and you want/don't want
> To call your vops on probe but not on scale_gear?
> 
> Also, Alim exported ufshcd_config_pwr_mode a while ago,
> In commit 0d846e703dc8 "scsi: ufs: make ufshcd_config_pwr_mode of
> non-static func"),
> But nobody uses it outside ufshcd - so maybe revert this commit as
> part of this series?
> 
> 
> 

Hi Avri,

Thanks for your review and suggestion.

What I get from your suggestion is that add one more parameter to
ufshcd_config_pwr_mode(say: bool from_probe), and pass the param to
ufshcd_vops_pwr_change_notify() so that vendor drivers know where
is the call comes from? If I get it correctly, then yes, we can
do that.

However, in ufs-qcom.c, ufs_qcom_pwr_change_notify(PRE_CHANGE)
is fine being called every time whenever there is a power mode change,
we have no problem with that. And as we are the only user of clock 
scaling
in the latest code base, so this change does not impact any other 
vendors.
If later other vendors need to use clock scaling, they can do this
change as you suggested above.

As for the commit from Alim, I think I should leave it to Alim's
call on it, maybe he has changes that use this func outside ufshcd.c
but not uploaded yet.

Thanks,
Can Guo.

>>         if (ret)
>>                 dev_err(hba->dev, "%s: failed err %d, old gear: (tx %d 
>> rx %d), new
>> gear: (tx %d rx %d)",
>>                         __func__, ret,
>> @@ -4126,8 +4125,6 @@ int ufshcd_config_pwr_mode(struct ufs_hba *hba,
>>                 memcpy(&final_params, desired_pwr_mode, 
>> sizeof(final_params));
>> 
>>         ret = ufshcd_change_power_mode(hba, &final_params);
>> -       if (!ret)
>> -               ufshcd_print_pwr_info(hba);
>> 
>>         return ret;
>>  }
>> @@ -7157,6 +7154,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
>> bool async)
>>                                         __func__, ret);
>>                         goto out;
>>                 }
>> +               ufshcd_print_pwr_info(hba);
>>         }
>> 
>>         /* set the state as operational after switching to desired 
>> gear */
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project