Re: [PATCH v3 3/8] scsi: ufs: Split ufshcd_probe_hba() based on its called flow

From: Alim Akhtar
Date: Sat Jan 18 2020 - 20:41:33 EST

Next message: Matthew Wilcox: "Re: [RFC] iomap: fix race between readahead and direct write"
Previous message: Alim Akhtar: "Re: [PATCH v3 2/8] scsi: ufs: Delete struct ufs_dev_desc"
In reply to: Bean Huo: "[PATCH v3 3/8] scsi: ufs: Split ufshcd_probe_hba() based on its called flow"
Next in thread: Bean Huo: "[PATCH v3 4/8] scsi: ufs: move ufshcd_get_max_pwr_mode() to ufs_init_params()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Jan 19, 2020 at 5:45 AM Bean Huo <huobean@xxxxxxxxx> wrote:
>
> From: Bean Huo <beanhuo@xxxxxxxxxx>
>
> This patch has two major non-functionality changes:
>
> 1. Take scanning host if-statement out from ufshcd_probe_hba(), and
> move into a new added function ufshcd_add_lus().
> In this new function ufshcd_add_lus(), the main functionalitis include:
> ICC initialization, add well-known LUs, devfreq initialization, UFS
> bsg probe and scsi host scan. The reason for this change is that these
> functionalities only being called during booting stage flow
> ufshcd_init()->ufshcd_async_scan(). In the processes of error handling
> and power management ufshcd_suspend(), ufshcd_resume(), ufshcd_probe_hba()
> being called, but these functionalitis above metioned are not hit.
>
> 2. Move context of initialization of parameters associated with the UFS
> device to a new added function ufshcd_init_params().

Since you are moving UFS device initialization related code, this
function can be renamed as
ufshcd_device_params_init() to make it more obvious about the function.
whats your thought on this?

> The reason of this change is that all of these parameters are used by
> driver, but only need to be initialized once when booting. Combine them
> into an integral function, make them easier maintain.
>
> Signed-off-by: Bean Huo <beanhuo@xxxxxxxxxx>
> ---
> drivers/scsi/ufs/ufshcd.c | 167 +++++++++++++++++++++++---------------
> 1 file changed, 101 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 58ef45b80cb0..54c127ef360b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -246,7 +246,7 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
> static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
> static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
> static void ufshcd_hba_exit(struct ufs_hba *hba);
> -static int ufshcd_probe_hba(struct ufs_hba *hba);
> +static int ufshcd_probe_hba(struct ufs_hba *hba, bool async);
> static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
> bool skip_ref_clk);
> static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
> @@ -6307,7 +6307,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
> goto out;
>
> /* Establish the link again and restore the device */
> - err = ufshcd_probe_hba(hba);
> + err = ufshcd_probe_hba(hba, false);
>
> if (!err && (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL))
> err = -EIO;
> @@ -6935,13 +6935,83 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
> return err;
> }
>
> +static int ufshcd_init_params(struct ufs_hba *hba)
> +{
> + bool flag;
> + int ret;
> +
> + /* Init check for device descriptor sizes */
> + ufshcd_init_desc_sizes(hba);
> +
> + /* Check and apply UFS device quirks */
> + ret = ufs_get_device_desc(hba);
> + if (ret) {
> + dev_err(hba->dev, "%s: Failed getting device info. err = %d\n",
> + __func__, ret);
> + goto out;
> + }
> +
> + ufs_fixup_device_setup(hba);
> +
> + /* Clear any previous UFS device information */
> + memset(&hba->dev_info, 0, sizeof(hba->dev_info));
> + if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> + QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> + hba->dev_info.f_power_on_wp_en = flag;
> +
> +out:
> + return ret;
> +}
> +
> +/**
> + * ufshcd_add_lus - probe and add UFS logical units
> + * @hba: per-adapter instance
> + */
> +static int ufshcd_add_lus(struct ufs_hba *hba)
> +{
> + int ret;
> +
> + if (!hba->is_init_prefetch)
> + ufshcd_init_icc_levels(hba);
> +
> + /* Add required well known logical units to scsi mid layer */
> + ret = ufshcd_scsi_add_wlus(hba);
> + if (ret)
> + goto out;
> +
> + /* Initialize devfreq after UFS device is detected */
> + if (ufshcd_is_clkscaling_supported(hba)) {
> + memcpy(&hba->clk_scaling.saved_pwr_info.info,
> + &hba->pwr_info,
> + sizeof(struct ufs_pa_layer_attr));
> + hba->clk_scaling.saved_pwr_info.is_valid = true;
> + if (!hba->devfreq) {
> + ret = ufshcd_devfreq_init(hba);
> + if (ret)
> + goto out;
> + }
> +
> + hba->clk_scaling.is_allowed = true;
> + }
> +
> + ufs_bsg_probe(hba);
> + scsi_scan_host(hba->host);
> + pm_runtime_put_sync(hba->dev);
> +
> + if (!hba->is_init_prefetch)
> + hba->is_init_prefetch = true;
> +out:
> + return ret;
> +}
> +
> /**
> * ufshcd_probe_hba - probe hba to detect device and initialize
> * @hba: per-adapter instance
> + * @async: asynchronous execution or not
> *
> * Execute link-startup and verify device initialization
> */
> -static int ufshcd_probe_hba(struct ufs_hba *hba)
> +static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
> {
> int ret;
> ktime_t start = ktime_get();
> @@ -6960,25 +7030,26 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> /* UniPro link is active now */
> ufshcd_set_link_active(hba);
>
> + /* Verify device initialization by sending NOP OUT UPIU */
> ret = ufshcd_verify_dev_init(hba);
> if (ret)
> goto out;
>
> + /* Initiate UFS initialization, and waiting until completion */
> ret = ufshcd_complete_dev_init(hba);
> if (ret)
> goto out;
>
> - /* Init check for device descriptor sizes */
> - ufshcd_init_desc_sizes(hba);
> -
> - ret = ufs_get_device_desc(hba);
> - if (ret) {
> - dev_err(hba->dev, "%s: Failed getting device info. err = %d\n",
> - __func__, ret);
> - goto out;
> + /*
> + * Initialize UFS device parameters used by driver, these
> + * parameters are associated with UFS descriptors.
> + */
> + if (async) {
> + ret = ufshcd_init_params(hba);
> + if (ret)
> + goto out;
> }
>
> - ufs_fixup_device_setup(hba);
> ufshcd_tune_unipro_params(hba);
>
> /* UFS device is also active now */
> @@ -7011,60 +7082,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> /* Enable Auto-Hibernate if configured */
> ufshcd_auto_hibern8_enable(hba);
>
> - /*
> - * If we are in error handling context or in power management callbacks
> - * context, no need to scan the host
> - */
> - if (!ufshcd_eh_in_progress(hba) && !hba->pm_op_in_progress) {
> - bool flag;
> -
> - /* clear any previous UFS device information */
> - memset(&hba->dev_info, 0, sizeof(hba->dev_info));
> - if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> - QUERY_FLAG_IDN_PWR_ON_WPE, &flag))
> - hba->dev_info.f_power_on_wp_en = flag;
> -
> - if (!hba->is_init_prefetch)
> - ufshcd_init_icc_levels(hba);
> -
> - /* Add required well known logical units to scsi mid layer */
> - ret = ufshcd_scsi_add_wlus(hba);
> - if (ret)
> - goto out;
> -
> - /* Initialize devfreq after UFS device is detected */
> - if (ufshcd_is_clkscaling_supported(hba)) {
> - memcpy(&hba->clk_scaling.saved_pwr_info.info,
> - &hba->pwr_info,
> - sizeof(struct ufs_pa_layer_attr));
> - hba->clk_scaling.saved_pwr_info.is_valid = true;
> - if (!hba->devfreq) {
> - ret = ufshcd_devfreq_init(hba);
> - if (ret)
> - goto out;
> - }
> - hba->clk_scaling.is_allowed = true;
> - }
> -
> - ufs_bsg_probe(hba);
> -
> - scsi_scan_host(hba->host);
> - pm_runtime_put_sync(hba->dev);
> - }
> -
> - if (!hba->is_init_prefetch)
> - hba->is_init_prefetch = true;
> -
> out:
> - /*
> - * If we failed to initialize the device or the device is not
> - * present, turn off the power/clocks etc.
> - */
> - if (ret && !ufshcd_eh_in_progress(hba) && !hba->pm_op_in_progress) {
> - pm_runtime_put_sync(hba->dev);
> - ufshcd_exit_clk_scaling(hba);
> - ufshcd_hba_exit(hba);
> - }
>
> trace_ufshcd_init(dev_name(hba->dev), ret,
> ktime_to_us(ktime_sub(ktime_get(), start)),
> @@ -7080,8 +7098,25 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> static void ufshcd_async_scan(void *data, async_cookie_t cookie)
> {
> struct ufs_hba *hba = (struct ufs_hba *)data;
> + int ret;
>
> - ufshcd_probe_hba(hba);
> + /* Initialize hba, detect and initialize UFS device */
> + ret = ufshcd_probe_hba(hba, true);
> + if (ret)
> + goto out;
> +
> + /* Probe and add UFS logical units */
> + ret = ufshcd_add_lus(hba);
> +out:
> + /*
> + * If we failed to initialize the device or the device is not
> + * present, turn off the power/clocks etc.
> + */
> + if (ret) {
> + pm_runtime_put_sync(hba->dev);
> + ufshcd_exit_clk_scaling(hba);
> + ufshcd_hba_exit(hba);
> + }
> }
>
> static const struct attribute_group *ufshcd_driver_groups[] = {
> --
> 2.17.1
>

--
Regards,
Alim

Next message: Matthew Wilcox: "Re: [RFC] iomap: fix race between readahead and direct write"
Previous message: Alim Akhtar: "Re: [PATCH v3 2/8] scsi: ufs: Delete struct ufs_dev_desc"
In reply to: Bean Huo: "[PATCH v3 3/8] scsi: ufs: Split ufshcd_probe_hba() based on its called flow"
Next in thread: Bean Huo: "[PATCH v3 4/8] scsi: ufs: move ufshcd_get_max_pwr_mode() to ufs_init_params()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]