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
References: <20200119001327.29155-1-huobean@gmail.com> <20200119001327.29155-4-huobean@gmail.com>
In-Reply-To: <20200119001327.29155-4-huobean@gmail.com>
From:   Alim Akhtar <alim.akhtar@gmail.com>
Date:   Sun, 19 Jan 2020 07:10:05 +0530
Message-ID: <CAGOxZ524szfnoYTCsgShaGoChHub4kHgvTgdAdsCmgFo53x8aw@mail.gmail.com>
Subject: Re: [PATCH v3 3/8] scsi: ufs: Split ufshcd_probe_hba() based on its
 called flow
To:     Bean Huo <huobean@gmail.com>
Cc:     Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>, asutoshd@codeaurora.org,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        "Bean Huo (beanhuo)" <beanhuo@micron.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Tomas Winkler <tomas.winkler@intel.com>,
        Can Guo <cang@codeaurora.org>, linux-scsi@vger.kernel.org,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jan 19, 2020 at 5:45 AM Bean Huo <huobean@gmail.com> wrote:
>
> From: Bean Huo <beanhuo@micron.com>
>
> 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@micron.com>
> ---
>  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