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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAE=gft7-G1vSNGjvA9OhSyQefjWJFmHiP0uxJK1qMZ8K8Bk5mw@mail.gmail.com>
Date:   Tue, 5 Feb 2019 10:00:26 -0800
From:   Evan Green <evgreen@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Can Guo <cang@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        Asutosh Das <asutoshd@...eaurora.org>,
        Vivek Gautam <vivek.gautam@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Marc Gonzalez <marc.w.gonzalez@...e.fr>,
        LKML <linux-kernel@...r.kernel.org>,
        "Martin K. Petersen" <martin.petersen@...cle.com>
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@...omium.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@...omium.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.