Re: [PATCH v5 2/7] scsi: ufs-qcom: Add reset control support for host controller

[cang]

On 2019-12-17 03:12, Jeffrey Hugo wrote:
> On Mon, Dec 16, 2019 at 12:05 PM Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> > Hi Can,
> >
> > On 14-11-19, 22:09, Can Guo wrote:
> > > Add reset control for host controller so that host controller can be reset
> > > as required in its power up sequence.
> >
> > I am seeing a regression on UFS on SM8150-mtp with this patch. I think
> > Jeff is seeing same one lenove laptop on 8998.
>
> Confirmed.
>
> > 845 does not seem to have this issue and only thing I can see is that 
> > on
> > sm8150 and 8998 we define reset as:
> >
> >                         resets = <&gcc GCC_UFS_BCR>;
> >                         reset-names = "rst";
> >
> > Thanks
> >
> > >
> > > Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/scsi/ufs/ufs-qcom.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/scsi/ufs/ufs-qcom.h |  3 +++
> > >  2 files changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > > index a5b7148..c69c29a1c 100644
> > > --- a/drivers/scsi/ufs/ufs-qcom.c
> > > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > > @@ -246,6 +246,44 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
> > >       mb();
> > >  }
> > >
> > > +/**
> > > + * ufs_qcom_host_reset - reset host controller and PHY
> > > + */
> > > +static int ufs_qcom_host_reset(struct ufs_hba *hba)
> > > +{
> > > +     int ret = 0;
> > > +     struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > > +
> > > +     if (!host->core_reset) {
> > > +             dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> > > +             goto out;
> > > +     }
> > > +
> > > +     ret = reset_control_assert(host->core_reset);
> > > +     if (ret) {
> > > +             dev_err(hba->dev, "%s: core_reset assert failed, err = %d\n",
> > > +                              __func__, ret);
> > > +             goto out;
> > > +     }
> > > +
> > > +     /*
> > > +      * The hardware requirement for delay between assert/deassert
> > > +      * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
> > > +      * ~125us (4/32768). To be on the safe side add 200us delay.
> > > +      */
> > > +     usleep_range(200, 210);
> > > +
> > > +     ret = reset_control_deassert(host->core_reset);
> > > +     if (ret)
> > > +             dev_err(hba->dev, "%s: core_reset deassert failed, err = %d\n",
> > > +                              __func__, ret);
> > > +
> > > +     usleep_range(1000, 1100);
> > > +
> > > +out:
> > > +     return ret;
> > > +}
> > > +
> > >  static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> > >  {
> > >       struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > > @@ -254,6 +292,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> > >       bool is_rate_B = (UFS_QCOM_LIMIT_HS_RATE == PA_HS_MODE_B)
> > >                                                       ? true : false;
> > >
> > > +     /* Reset UFS Host Controller and PHY */
> > > +     ret = ufs_qcom_host_reset(hba);
> > > +     if (ret)
> > > +             dev_warn(hba->dev, "%s: host reset returned %d\n",
> > > +                               __func__, ret);
> > > +
> > >       if (is_rate_B)
> > >               phy_set_mode(phy, PHY_MODE_UFS_HS_B);
> > >
> > > @@ -1101,6 +1145,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
> > >       host->hba = hba;
> > >       ufshcd_set_variant(hba, host);
> > >
> > > +     /* Setup the reset control of HCI */
> > > +     host->core_reset = devm_reset_control_get(hba->dev, "rst");
> > > +     if (IS_ERR(host->core_reset)) {
> > > +             err = PTR_ERR(host->core_reset);
> > > +             dev_warn(dev, "Failed to get reset control %d\n", err);
> > > +             host->core_reset = NULL;
> > > +             err = 0;
> > > +     }
> > > +
> > >       /* Fire up the reset controller. Failure here is non-fatal. */
> > >       host->rcdev.of_node = dev->of_node;
> > >       host->rcdev.ops = &ufs_qcom_reset_ops;
> > > diff --git a/drivers/scsi/ufs/ufs-qcom.h b/drivers/scsi/ufs/ufs-qcom.h
> > > index d401f17..2d95e7c 100644
> > > --- a/drivers/scsi/ufs/ufs-qcom.h
> > > +++ b/drivers/scsi/ufs/ufs-qcom.h
> > > @@ -6,6 +6,7 @@
> > >  #define UFS_QCOM_H_
> > >
> > >  #include <linux/reset-controller.h>
> > > +#include <linux/reset.h>
> > >
> > >  #define MAX_UFS_QCOM_HOSTS   1
> > >  #define MAX_U32                 (~(u32)0)
> > > @@ -233,6 +234,8 @@ struct ufs_qcom_host {
> > >       u32 dbg_print_en;
> > >       struct ufs_qcom_testbus testbus;
> > >
> > > +     /* Reset control of HCI */
> > > +     struct reset_control *core_reset;
> > >       struct reset_controller_dev rcdev;
> > >
> > >       struct gpio_desc *device_reset;
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> >
> > --
> > ~Vinod

Hi Jeffrey and Vinod,

Thanks for reporting this. May I know what kind of regression do you see 
on
8150 and 8998?
BTW, do you have reset control for UFS PHY in your DT?
See 71278b058a9f8752e51030e363b7a7306938f64e.

FYI, we use reset control on all of our platforms and it is
a must during our power up sequence.

Thanks,
Can Guo.