Messages in this thread | | | From | Evan Green <> | Date | Tue, 5 Feb 2019 10:00:26 -0800 | Subject | Re: [PATCH v2 9/9] phy: qcom-ufs: Refactor all init steps into phy_poweron |
| |
On Fri, Feb 1, 2019 at 10:43 AM Stephen Boyd <swboyd@chromium.org> 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@chromium.org> > > > 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.
| |