Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: CHB/a6m0uEvFeflEVKq6ttsgV/QaqBQ61BQNXWJ/rp9EM6WUkIsWJB+gmndivwdLpn8f73VyjY
 3Ht2VfMadbR4ZuMHN3KtBth/yBZrdIAUpUOU3/ZMH9JE3Yx5vpUC/6xVCQWn6TK30sgrjumK+O
 cQyHT569SY4XVUT/yM1y/R7+KP1Fozl4fUs6bbXE6refOH7BAb7RoWqzWJ8kGmH+mCZRGsji/y
 9qClmC8RG8xVuQhWedqyHHyuYYYPb7ZLlavNgvOGcNT+va4gkmQDViV4Q4dBf6XWAIfiqgNl4T
 uLo=
From:   Avri Altman <Avri.Altman@wdc.com>
To:     Ziqi Chen <ziqichen@codeaurora.org>,
        "asutoshd@codeaurora.org" <asutoshd@codeaurora.org>,
        "nguyenb@codeaurora.org" <nguyenb@codeaurora.org>,
        "cang@codeaurora.org" <cang@codeaurora.org>,
        "hongwus@codeaurora.org" <hongwus@codeaurora.org>,
        "rnayak@codeaurora.org" <rnayak@codeaurora.org>,
        "vinholikatti@gmail.com" <vinholikatti@gmail.com>,
        "jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "kernel-team@android.com" <kernel-team@android.com>,
        "saravanak@google.com" <saravanak@google.com>,
        "salyzyn@google.com" <salyzyn@google.com>,
        "kwmad.kim@samsung.com" <kwmad.kim@samsung.com>,
        "stanley.chu@mediatek.com" <stanley.chu@mediatek.com>
CC:     Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Bean Huo <beanhuo@micron.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Satya Tangirala <satyat@google.com>,
        "moderated list:UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER..." 
        <linux-mediatek@lists.infradead.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@vger.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH RFC v4 1/1] scsi: ufs: Fix ufs power down/on specs
 violation
Thread-Topic: [PATCH RFC v4 1/1] scsi: ufs: Fix ufs power down/on specs
 violation
Thread-Index: AQHW2Gllg/EWmEMBPEKOS0xlcLh5tqoFIwog
Date:   Wed, 23 Dec 2020 20:45:22 +0000
Message-ID: <DM6PR04MB65751FC6D1ED61E569AC095EFCDE0@DM6PR04MB6575.namprd04.prod.outlook.com>
References: <1608644981-46267-1-git-send-email-ziqichen@codeaurora.org>
In-Reply-To: <1608644981-46267-1-git-send-email-ziqichen@codeaurora.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 69ce4840-7535-4a82-36b6-08d8a783ab06
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Dec 2020 20:45:22.3108
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /C7ke7H/I9VY+Rlv80w/QFNd5D4XcXLKzp8raDYyi2y+oYWeyO24c2uVoBg8Zc4JAsSCdrCw7YaEJkresWCVAw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5673
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,
>=20
> As per specs, e.g, JESD220E chapter 7.2, while powering
> off/on the ufs device, RST_N signal and REF_CLK signal
> should be between VSS(Ground) and VCCQ/VCCQ2.
>=20
> To flexibly control device reset line, refactor the function
> ufschd_vops_device_reset(sturct ufs_hba *hba) to ufshcd_
> vops_device_reset(sturct ufs_hba *hba, bool asserted). The
> new parameter "bool asserted" is used to separate device reset
> line pulling down from pulling up.
Sorry for my late response.
Please allow few more days to consult internally about this.=20

>=20
> Cc: Kiwoong Kim <kwmad.kim@samsung.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Signed-off-by: Ziqi Chen <ziqichen@codeaurora.org>


> -static int ufs_qcom_device_reset(struct ufs_hba *hba)
> +static int ufs_qcom_device_reset(struct ufs_hba *hba, bool asserted)
>  {
>         struct ufs_qcom_host *host =3D ufshcd_get_variant(hba);
>=20
> @@ -1417,15 +1418,20 @@ static int ufs_qcom_device_reset(struct ufs_hba
> *hba)
>         if (!host->device_reset)
>                 return -EOPNOTSUPP;
>=20
> -       /*
> -        * The UFS device shall detect reset pulses of 1us, sleep for 10u=
s to
> -        * be on the safe side.
> -        */
> -       gpiod_set_value_cansleep(host->device_reset, 1);
> -       usleep_range(10, 15);
> +       if (asserted) {
> +               gpiod_set_value_cansleep(host->device_reset, 1);
>=20
> -       gpiod_set_value_cansleep(host->device_reset, 0);
> -       usleep_range(10, 15);
> +               /*
> +                * The UFS device shall detect reset pulses of 1us, sleep=
 for 10us to
> +                * be on the safe side.
> +                */
> +               usleep_range(10, 15);
> +       } else {
> +               gpiod_set_value_cansleep(host->device_reset, 0);
> +
> +                /* Some devices may need time to respond to rst_n */
> +               usleep_range(10, 15);
Since sleep the same on assert/de-assert can move it outside the if-else cl=
ause?=20

> +       }
>=20
>         return 0;
>  }

All the below changes, in suspend/resume, deserves some references in your =
commit log,
And probably a separate patch.

Thanks,
Avri

> @@ -8686,8 +8696,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>         if (ret)
>                 goto set_dev_active;
>=20
> -       ufshcd_vreg_set_lpm(hba);
> -
>  disable_clks:
>         /*
>          * Call vendor specific suspend callback. As these callbacks may =
access
> @@ -8703,6 +8711,9 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>          */
>         ufshcd_disable_irq(hba);
>=20
> +       if (ufshcd_is_link_off(hba))
> +               ufshcd_vops_device_reset(hba, true);
> +
>         ufshcd_setup_clocks(hba, false);
>=20
>         if (ufshcd_is_clkgating_allowed(hba)) {
> @@ -8711,6 +8722,8 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>                                         hba->clk_gating.state);
>         }
>=20
> +       ufshcd_vreg_set_lpm(hba);
> +
>         /* Put the host controller in low power mode if possible */
>         ufshcd_hba_vreg_set_lpm(hba);
>         goto out;
> @@ -8778,18 +8791,19 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>         old_link_state =3D hba->uic_link_state;
>=20
>         ufshcd_hba_vreg_set_hpm(hba);
> +
> +       ret =3D ufshcd_vreg_set_hpm(hba);
> +       if (ret)
> +               goto out;
> +
>         /* Make sure clocks are enabled before accessing controller */
>         ret =3D ufshcd_setup_clocks(hba, true);
>         if (ret)
> -               goto out;
> +               goto disable_vreg;
>=20
>         /* enable the host irq as host controller would be active soon */
>         ufshcd_enable_irq(hba);
>=20
> -       ret =3D ufshcd_vreg_set_hpm(hba);
> -       if (ret)
> -               goto disable_irq_and_vops_clks;
> -
>         /*
>          * Call vendor specific resume callback. As these callbacks may a=
ccess
>          * vendor specific host controller register space call them when =
the
> @@ -8797,7 +8811,7 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>          */
>         ret =3D ufshcd_vops_resume(hba, pm_op);
>         if (ret)
> -               goto disable_vreg;
> +               goto disable_irq_and_vops_clks;
>=20
>         /* For DeepSleep, the only supported option is to have the link o=
ff */
>         WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) &&
> !ufshcd_is_link_off(hba));
> @@ -8864,8 +8878,6 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>         ufshcd_link_state_transition(hba, old_link_state, 0);
>  vendor_suspend:
>         ufshcd_vops_suspend(hba, pm_op);
> -disable_vreg:
> -       ufshcd_vreg_set_lpm(hba);
>  disable_irq_and_vops_clks:
>         ufshcd_disable_irq(hba);
>         if (hba->clk_scaling.is_allowed)
> @@ -8876,6 +8888,8 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>                 trace_ufshcd_clk_gating(dev_name(hba->dev),
>                                         hba->clk_gating.state);
>         }
> +disable_vreg:
> +       ufshcd_vreg_set_lpm(hba);
>  out:
>         hba->pm_op_in_progress =3D 0;
>         if (ret)
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 9bb5f0e..d5fbaba 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -319,7 +319,7 @@ struct ufs_pwr_mode_info {
>   * @resume: called during host controller PM callback
>   * @dbg_register_dump: used to dump controller debug information
>   * @phy_initialization: used to initialize phys
> - * @device_reset: called to issue a reset pulse on the UFS device
> + * @device_reset: called to assert or deassert device reset line
>   * @program_key: program or evict an inline encryption key
>   * @event_notify: called to notify important events
>   */
> @@ -350,7 +350,7 @@ struct ufs_hba_variant_ops {
>         int     (*resume)(struct ufs_hba *, enum ufs_pm_op);
>         void    (*dbg_register_dump)(struct ufs_hba *hba);
>         int     (*phy_initialization)(struct ufs_hba *);
> -       int     (*device_reset)(struct ufs_hba *hba);
> +       int     (*device_reset)(struct ufs_hba *hba, bool asserted);
>         void    (*config_scaling_param)(struct ufs_hba *hba,
>                                         struct devfreq_dev_profile *profi=
le,
>                                         void *data);
> @@ -1216,10 +1216,10 @@ static inline void
> ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
>                 hba->vops->dbg_register_dump(hba);
>  }
>=20
> -static inline int ufshcd_vops_device_reset(struct ufs_hba *hba)
> +static inline int ufshcd_vops_device_reset(struct ufs_hba *hba, bool
> asserted)
>  {
>         if (hba->vops && hba->vops->device_reset)
> -               return hba->vops->device_reset(hba);
> +               return hba->vops->device_reset(hba, asserted);
>=20
>         return -EOPNOTSUPP;
>  }
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> a Linux Foundation Collaborative Project