From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jorge Ramirez Subject: Re: [PATCH 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver Date: Wed, 26 Dec 2018 18:53:08 +0100 Message-ID: <2f38642e-27a0-0fd7-928e-8e782d0bc7c6@linaro.org> References: <1544176558-7946-1-git-send-email-jorge.ramirez-ortiz@linaro.org> <1544176558-7946-3-git-send-email-jorge.ramirez-ortiz@linaro.org> <154533779333.79149.17234544366247143930@swboyd.mtv.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <154533779333.79149.17234544366247143930@swboyd.mtv.corp.google.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Stephen Boyd , gregkh@linuxfoundation.org, kishon@ti.com, mark.rutland@arm.com, robh+dt@kernel.org Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, shawn.guo@linaro.org, vkoul@kernel.org List-Id: devicetree@vger.kernel.org On 12/20/18 21:29, Stephen Boyd wrote: > Quoting Jorge Ramirez-Ortiz (2018-12-07 01:55:58) >> From: Shawn Guo >> >> Driver to control the Synopsys SS PHY 1.0.0 implemeneted in QCS404 >> >> Based on Sriharsha Allenki's original code. >> >> Signed-off-by: Jorge Ramirez-Ortiz >> Signed-off-by: Shawn Guo > > chain should be swapped? ok. Shawn asked me to remove him from the authors list so will remove. > >> Reviewed-by: Vinod Koul will remove the reviewed-by line as well. >> --- >> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c b/drivers/phy/qualcomm/phy-qcom-usb-ss.c >> new file mode 100644 >> index 0000000..7b6a55e >> --- /dev/null >> +++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c >> + >> +struct ssphy_priv { >> + void __iomem *base; >> + struct device *dev; >> + struct reset_control *reset_com; >> + struct reset_control *reset_phy; >> + struct clk *clk_ref; >> + struct clk *clk_phy; >> + struct clk *clk_pipe; > > Use bulk clk APIs? And just get and enable all the clks? yes. > >> + struct regulator *vdda1p8; >> + struct regulator *vbus; >> + struct regulator *vdd; >> + unsigned int vdd_levels[LEVEL_NUM]; >> +}; >> + >> +static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val) >> +{ >> + writel((readl(addr) & ~mask) | val, addr); >> +} >> + >> +static int qcom_ssphy_config_vdd(struct ssphy_priv *priv, >> + enum phy_vdd_level level) >> +{ >> + return regulator_set_voltage(priv->vdd, >> + priv->vdd_levels[level], >> + priv->vdd_levels[LEVEL_MAX]); > > regulator_set_voltage(reg, 0, max) is very suspicious. It's voltage > corners where the voltages are known? sorry I dont understand the question this regulator on the ss phy wold be vreg_l3_1p05: l3 { regulator-min-microvolt = <976000>; regulator-max-microvolt = <1160000>; }; > >> +} >> + >> +static int qcom_ssphy_ldo_enable(struct ssphy_priv *priv) >> +{ >> + int ret; >> + >> + ret = regulator_set_load(priv->vdda1p8, 23000); > > Do you need to do this? Why can't the regulator be in high power mode > all the time and then go low power when it's disabled? this regulator is shared with the usb hs phy and each (ss/hs) have different load requirements. why would it be wrong for the ss phy to announce its needs (which will differ from those of the hs phy)? > >> + if (ret < 0) { >> + dev_err(priv->dev, "Failed to set regulator1p8 load\n"); >> + return ret; >> + } >> + >> + ret = regulator_set_voltage(priv->vdda1p8, 1800000, 1800000); > > This looks unnecessary. The 1.8V regulator sounds like it better be 1.8V > so board constraints should enforce that. All that should be here is > enabling the regulator. ok > >> + if (ret) { >> + dev_err(priv->dev, "Failed to set regulator1p8 voltage\n"); >> + goto put_vdda1p8_lpm; >> + } >> + >> + ret = regulator_enable(priv->vdda1p8); >> + if (ret) { >> + dev_err(priv->dev, "Failed to enable regulator1p8\n"); >> + goto unset_vdda1p8; >> + } >> + >> + return ret; >> + >> + /* rollback regulator changes */ >> + >> +unset_vdda1p8: >> + regulator_set_voltage(priv->vdda1p8, 0, 1800000); >> + >> +put_vdda1p8_lpm: >> + regulator_set_load(priv->vdda1p8, 0); >> + >> + return ret; >> +} >> + >> +static void qcom_ssphy_ldo_disable(struct ssphy_priv *priv) >> +{ >> + regulator_disable(priv->vdda1p8); >> + regulator_set_voltage(priv->vdda1p8, 0, 1800000); > > Urgh why? since it is being shared with the hs phy I understand this is required to vote the transition to the lowest voltage state. > >> + regulator_set_load(priv->vdda1p8, 0); >> +} >