[0/2] USB SS PHY for Qualcomm's QCS404
mbox series

Message ID 1544176558-7946-1-git-send-email-jorge.ramirez-ortiz@linaro.org
Headers show
Series
  • USB SS PHY for Qualcomm's QCS404
Related show

Message

Jorge Ramirez Dec. 7, 2018, 9:55 a.m. UTC
This set adds USB SS PHY support to Qualcomm's QCS404 SoC
The PHY is implemented using Synopsys SS PHY 1.0.0 IP

The code is based on Sriharsha Allenki's <sallenki@codeaurora.org>
original implementation.

Jorge Ramirez-Ortiz (1):
  dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

Shawn Guo (1):
  phy: qualcomm: usb: Add Super-Speed PHY driver

 .../devicetree/bindings/usb/qcom,usb-ssphy.txt     |  78 +++++
 drivers/phy/qualcomm/Kconfig                       |  11 +
 drivers/phy/qualcomm/Makefile                      |   1 +
 drivers/phy/qualcomm/phy-qcom-usb-ss.c             | 340 +++++++++++++++++++++
 4 files changed, 430 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
 create mode 100644 drivers/phy/qualcomm/phy-qcom-usb-ss.c

Comments

Stephen Boyd Dec. 20, 2018, 8:29 p.m. UTC | #1
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?

> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> ---
> 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?

> +       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?

> +}
> +
> +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?

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

> +       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?

> +       regulator_set_load(priv->vdda1p8, 0);
> +}
Jorge Ramirez Dec. 26, 2018, 5:53 p.m. UTC | #2
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);
>> +}
>
Stephen Boyd Jan. 3, 2019, 11:30 p.m. UTC | #3
Quoting Jorge Ramirez (2018-12-26 09:53:08)
> On 12/20/18 21:29, Stephen Boyd wrote:
> > Quoting Jorge Ramirez-Ortiz (2018-12-07 01:55:58)
> > 
> >> +       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>;

Is this also the CX or MX voltage for the SoC? There would be a pin like
VDDCX or VDDMX on the SoC part that is connected to this regulator if
that's the case. Because you have "LEVEL" in the code it makes it sounds
like it's voltage corners here so I suspect this is mapping into the
voltage corner stuff that qcom has.

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

Yes they have different load requirements, but in the end I would guess
they always push the regulator into high power mode when the device is
active and drop the load requirement when it's inactive. If it matches
the enable state of the regulator then there isn't much need for setting
a load explicitly besides stating that when the regulator is on it
should be in high power mode and when it's off it should be in low power
mode and off.