Re: [PATCH 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver

[Jorge Ramirez <jorge.ramirez-ortiz@linaro.org>]

On 12/20/18 21:29, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2018-12-07 01:55:58)
>> From: Shawn Guo <shawn.guo@linaro.org>
>>
>> Driver to control the Synopsys SS PHY 1.0.0 implemeneted in QCS404
>>
>> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original code.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> 
> chain should be swapped?

ok.

Shawn asked me to remove him from the authors list so will remove.

> 
>> Reviewed-by: Vinod Koul <vkoul@kernel.org>

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);
>> +}
>