Re: [PATCH v2 9/9] phy: qcom-ufs: Refactor all init steps into phy_poweron

[Evan Green]

On Fri, Feb 1, 2019 at 10:43 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Evan Green (2019-01-23 14:11:37)
> > The phy code was using implicit sequencing between the PHY driver
> > and the UFS driver to implement certain hardware requirements.
> > Specifically, the PHY reset register in the UFS controller needs
> > to be deasserted before serdes start occurs in the PHY.
> >
> > Before this change, the code was doing this by utilizing the two
> > phy callbacks, phy_init and phy_poweron, as "init step 1" and
>
> Nitpick: Can you please indicate functions with () and variables with
> ''? So write phy_init() and phy_poweron(), etc.

Sure.

>
> > "init step 2", where the UFS driver would deassert reset between
> > these two steps.
> >
> > This makes it challenging to power off the regulators in suspend,
> > as regulators are initialized in init, not in poweron, but only
> > poweroff is called during suspend, not exit.
> >
> > Consolidate the initialization code into phy_poweron, and utilize
> > the reset controller exported from the UFS driver to explicitly
> > perform all the steps needed to initialize the PHY.
>
> Also mention that a new callback is introduced, 'calibrate', that
> >
> > Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx>
>
> > diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-i.h b/drivers/phy/qualcomm/phy-qcom-ufs-i.h
> > index f798fb64de94e..109ddd67be829 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-ufs-i.h
> > +++ b/drivers/phy/qualcomm/phy-qcom-ufs-i.h
> > @@ -19,6 +19,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/phy/phy.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/reset.h>
>
> Just forward declare struct reset_control instead of including this.

It looks like convention in this driver is to include everything in
this header, phy-qcom-ufs.c includes nothing but this file, and
similarly for the 14nm and 20nm variants. So maybe I should stay
following suit rather than being that one dude who plops his include
in a different spot.

>
> >  #include <linux/slab.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/io.h>
> > @@ -96,11 +97,10 @@ struct ufs_qcom_phy {
> >         char name[UFS_QCOM_PHY_NAME_LEN];
> >         struct ufs_qcom_phy_calibration *cached_regs;
> >         int cached_regs_table_size;
> > -       bool is_powered_on;
> > -       bool is_started;
> >         struct ufs_qcom_phy_specific_ops *phy_spec_ops;
> >
> >         enum phy_mode mode;
> > +       struct reset_control *ufs_reset;
> >  };
> >
> >  /**
>
> For some reason I get the feeling that this patch should be combined
> with something else from the controller. Does this complete the
> conversion but the patches before this one sort of wreck the state of
> reset and init/poweron phases so that they can't stand on their own?
> Maybe if the reset was introduced, and then a patch to get the resets
> was put in place, and then a final patch to rewrite the phy and
> controller at the same time would make more sense to read.

Yeah, let me try this refactoring.