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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 01 Feb 2019 10:43:41 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andy Gross <andy.gross@...aro.org>,
        Evan Green <evgreen@...omium.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Rob Herring <robh+dt@...nel.org>
Cc:     Can Guo <cang@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        Asutosh Das <asutoshd@...eaurora.org>,
        Vivek Gautam <vivek.gautam@...eaurora.org>,
        Evan Green <evgreen@...omium.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Marc Gonzalez <marc.w.gonzalez@...e.fr>,
        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

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.

> "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.

>  #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.