Received-SPF: pass (google.com: domain of linux-kernel+bounces-166491-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240502123312.31083-1-quic_ugoswami@quicinc.com>
In-Reply-To: <20240502123312.31083-1-quic_ugoswami@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Thu, 2 May 2024 16:12:20 +0300
Message-ID: <CAA8EJppeQTadmny=hcs4xCQDXHwXEBHXjeecvZCUVcSXmwBTgg@mail.gmail.com>
Subject: Re: [PATCH] phy: qcom-snps-femto-v2: Add load and voltage setting for
 LDO's used
To: Udipto Goswami <quic_ugoswami@quicinc.com>
Cc: Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>, linux-arm-msm@vger.kernel.org, 
	linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Thu, 2 May 2024 at 15:33, Udipto Goswami <quic_ugoswami@quicinc.com> wrote:
>
> The Femto phy depends on 0.88/ 1.8/ 3.3V regulators for its operation.
> If one of the regulators is not voted to the required minimum voltage
> for phy to operate, then High speed mode of operation will fail.
>
> On certain targets like (qcm6490_rb3gen2) where the minimum voltage
> of the regulator is lower than the operating voltage of the phy.
> If not voted properly, the phy supply would be limited to the min value
> of the LDO thereby rendering the phy non-operational.
>
> The current implementation of the regulators in the Femto PHY is not
> setting the load and voltage for each LDO. The appropriate voltages and
> loads required for the PHY to operate should be set.

Please move min/max voltages to the DTS. There is no need to set them
from the driver.

Also, is there any reason why you can't use `regulator-initial-mode =
<RPMH_REGULATOR_MODE_HPM>;` like other boards do?

>
> Implement a bulk operation api to set load & voltages before doing bulk
> enable. This will ensure that the PHY remains operational under all
> conditions.
>
> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 121 +++++++++++++++++-
>  1 file changed, 114 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index eb0b0f61d98e..cbe9cdaa6849 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> @@ -78,11 +78,39 @@
>
>  #define LS_FS_OUTPUT_IMPEDANCE_MASK            GENMASK(3, 0)
>
> -static const char * const qcom_snps_hsphy_vreg_names[] = {
> -       "vdda-pll", "vdda33", "vdda18",
> +
> +struct qcom_snps_hsphy_regulator_data {
> +       const char *name;
> +       unsigned int enable_load;
> +       unsigned int min_voltage;
> +       unsigned int max_voltage;
> +};
> +
> +static const struct qcom_snps_hsphy_regulator_data hsphy_vreg_l[] = {
> +       {
> +               .name = "vdda-pll",
> +               .enable_load = 30000,
> +               .min_voltage = 825000,
> +               .max_voltage = 925000,
> +       },
> +
> +       {
> +               .name = "vdda18",
> +               .enable_load = 19000,
> +               .min_voltage = 1704000,
> +               .max_voltage = 1800000
> +       },
> +
> +       {
> +               .name = "vdda33",
> +               .enable_load = 16000,
> +               .min_voltage = 3050000,
> +               .max_voltage = 3300000
> +       },
> +
>  };
>
> -#define SNPS_HS_NUM_VREGS              ARRAY_SIZE(qcom_snps_hsphy_vreg_names)
> +#define SNPS_HS_NUM_VREGS              ARRAY_SIZE(hsphy_vreg_l)
>
>  struct override_param {
>         s32     value;
> @@ -132,12 +160,90 @@ struct qcom_snps_hsphy {
>         struct clk_bulk_data *clks;
>         struct reset_control *phy_reset;
>         struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS];
> +       const struct qcom_snps_hsphy_regulator_data *vreg_list;
>
>         bool phy_initialized;
>         enum phy_mode mode;
>         struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
>  };
>
> +static int __vdda_phy_bulk_enable_disable(struct qcom_snps_hsphy *hsphy, bool on)

Separate functions, please.

> +{
> +       int ret = 0;
> +       int i;
> +
> +       if (!on)
> +               goto disable_vdd;
> +
> +       for (i = 0; i < SNPS_HS_NUM_VREGS; i++) {
> +
> +               ret = regulator_set_load(hsphy->vregs[i].consumer,
> +                                        hsphy->vreg_list[i].enable_load);
> +
> +               if (ret < 0) {
> +                       dev_err(hsphy->dev, "unable to set HPM of %s %d\n",
> +                                               hsphy->vregs[i].supply, ret);
> +                       goto err_vdd;
> +               }
> +       }
> +
> +       for (i = 0; i < SNPS_HS_NUM_VREGS; i++) {
> +               ret = regulator_set_voltage(hsphy->vregs[i].consumer,
> +                                           hsphy->vreg_list[i].min_voltage,
> +                                           hsphy->vreg_list[i].max_voltage);
> +               if (ret) {
> +                       dev_err(hsphy->dev,
> +                               "unable to set voltage of regulator %s %d\n",
> +                                               hsphy->vregs[i].supply, ret);
> +                       goto put_vdd_lpm;
> +               }
> +       }
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> +       if (ret)
> +               goto unconfig_vdd;
> +
> +       return ret;
> +
> +disable_vdd:
> +       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> +
> +unconfig_vdd:
> +       for (i = 0; i < SNPS_HS_NUM_VREGS; i++) {
> +               ret = regulator_set_voltage(hsphy->vregs[i].consumer, 0,
> +                                           hsphy->vreg_list[i].max_voltage);
> +               if (ret) {
> +                       dev_err(hsphy->dev,
> +                       "unable to set voltage of regulator %s %d\n",
> +                                       hsphy->vregs[i].supply, ret);
> +               }
> +       }
> +
> +put_vdd_lpm:
> +       for (i = 0; i < SNPS_HS_NUM_VREGS; i++) {
> +               ret = regulator_set_load(hsphy->vregs[i].consumer, 0);
> +
> +               if (ret < 0) {
> +                       dev_err(hsphy->dev, "unable to set LPM of %s %d\n",
> +                                               hsphy->vregs[i].supply, ret);
> +               }
> +       }
> +
> +err_vdd:
> +       return ret;
> +
> +}
> +
> +static int vdda_phy_bulk_enable(struct qcom_snps_hsphy *hsphy)
> +{
> +       return __vdda_phy_bulk_enable_disable(hsphy, true);
> +}
> +
> +static int vdda_phy_bulk_disable(struct qcom_snps_hsphy *hsphy)
> +{
> +       return __vdda_phy_bulk_enable_disable(hsphy, false);
> +}
> +
>  static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
>  {
>         struct device *dev = hsphy->dev;
> @@ -390,7 +496,7 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>
>         dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
>
> -       ret = regulator_bulk_enable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> +       ret = vdda_phy_bulk_enable(hsphy);
>         if (ret)
>                 return ret;
>
> @@ -471,7 +577,7 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>  disable_clks:
>         clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>  poweroff_phy:
> -       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> +       ret = vdda_phy_bulk_disable(hsphy);
>
>         return ret;
>  }
> @@ -482,7 +588,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy)
>
>         reset_control_assert(hsphy->phy_reset);
>         clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
> -       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> +       vdda_phy_bulk_disable(hsphy);
>         hsphy->phy_initialized = false;
>
>         return 0;
> @@ -592,8 +698,9 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>
>         num = ARRAY_SIZE(hsphy->vregs);
>         for (i = 0; i < num; i++)
> -               hsphy->vregs[i].supply = qcom_snps_hsphy_vreg_names[i];
> +               hsphy->vregs[i].supply = hsphy_vreg_l[i].name;
>
> +       hsphy->vreg_list  = hsphy_vreg_l;
>         ret = devm_regulator_bulk_get(dev, num, hsphy->vregs);
>         if (ret)
>                 return dev_err_probe(dev, ret,
> --
> 2.17.1
>
>


-- 
With best wishes
Dmitry