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

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

[Jorge Ramirez-Ortiz]

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

-- 
2.7.4

[PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez-Ortiz]

Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
controller embedded in QCS404.

Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
definitions.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Reviewed-by: Vinod Koul <vkoul@kernel.org>
---
 .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 78 ++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt

diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
new file mode 100644
index 0000000..fcf4e01
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
@@ -0,0 +1,78 @@
+Qualcomm Synopsys 1.0.0 SS phy controller
+===========================================
+
+Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
+chipsets
+
+Required properties:
+
+- compatible:
+    Value type: <string>
+    Definition: Should contain "qcom,usb-ssphy".
+
+- reg:
+    Value type: <prop-encoded-array>
+    Definition: USB PHY base address and length of the register map.
+
+- #phy-cells:
+    Value type: <u32>
+    Definition: Should be 0. See phy/phy-bindings.txt for details.
+
+- clocks:
+    Value type: <prop-encoded-array>
+    Definition: See clock-bindings.txt section "consumers". List of
+		 three clock specifiers for reference, phy core and
+		 pipe clocks.
+
+- clock-names:
+    Value type: <string>
+    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
+		 property. Must contain "ref", "phy" and "pipe".
+
+- vdd-supply:
+    Value type: <phandle>
+    Definition: phandle to the regulator VDD supply node.
+
+- vdda1p8-supply:
+    Value type: <phandle>
+    Definition: phandle to the regulator 1.8V supply node.
+
+- qcom,vdd-voltage-level:
+    Value type: <prop-array>
+    Definition: This is a list of three integer values <no min max> where
+		 each value corresponding to voltage corner in uV.
+
+Optional child nodes:
+
+- vbus-supply:
+    Value type: <phandle>
+    Definition: phandle to the VBUS supply node.
+
+- resets:
+    Value type: <prop-encoded-array>
+    Definition: See reset.txt section "consumers". PHY reset specifiers
+		 for phy core and COR resets.
+
+- reset-names:
+    Value type: <string>
+    Definition: Names of the resets in 1-1 correspondence with the "resets"
+		 property. Must contain "com" and "phy".
+
+Example:
+
+usb3_phy: phy@78000 {
+	compatible = "qcom,usb-ssphy";
+	reg = <0x78000 0x400>;
+	#phy-cells = <0>;
+	clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
+		 <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
+		 <&gcc GCC_USB3_PHY_PIPE_CLK>;
+	clock-names = "ref", "phy", "pipe";
+	resets = <&gcc GCC_USB3_PHY_BCR>,
+		 <&gcc GCC_USB3PHY_PHY_BCR>;
+	reset-names = "com", "phy";
+	vdd-supply = <&vreg_l3_1p05>;
+	vdda1p8-supply = <&vreg_l5_1p8>;
+	vbus-supply = <&usb3_vbus_reg>;
+	qcom,vdd-voltage-level = <0 1050000 1050000>;
+};
-- 
2.7.4

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez-Ortiz]

On 07/12/18 10:55:57, Jorge Ramirez-Ortiz wrote:
> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> controller embedded in QCS404.
> 
> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> definitions.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>

any follow up on this patchset?

thanks

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Rob Herring]

On Thu, Dec 20, 2018 at 10:52:45AM +0100, Jorge Ramirez-Ortiz wrote:
> On 07/12/18 10:55:57, Jorge Ramirez-Ortiz wrote:
> > Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> > controller embedded in QCS404.
> > 
> > Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> > definitions.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> > Reviewed-by: Vinod Koul <vkoul@kernel.org>
> 
> any follow up on this patchset?

Check DT patchwork[1] if you want to know where you are in the queue. 
Just so happened to get to it now.

Rob

[1] https://patchwork.ozlabs.org/project/devicetree-bindings/list/

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez]

On 12/20/18 18:07, Rob Herring wrote:
> On Thu, Dec 20, 2018 at 10:52:45AM +0100, Jorge Ramirez-Ortiz wrote:
>> On 07/12/18 10:55:57, Jorge Ramirez-Ortiz wrote:
>>> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
>>> controller embedded in QCS404.
>>>
>>> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
>>> definitions.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>> Reviewed-by: Vinod Koul <vkoul@kernel.org>
>> any follow up on this patchset?
> Check DT patchwork[1] if you want to know where you are in the queue.
> Just so happened to get to it now.

thanks for the info Rob. I honestly didnt know it operated in order! my 
fault

>
> Rob
>
> [1] https://patchwork.ozlabs.org/project/devicetree-bindings/list/
>

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Rob Herring]

On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote:
> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> controller embedded in QCS404.
> 
> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> definitions.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 78 ++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> new file mode 100644
> index 0000000..fcf4e01
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> @@ -0,0 +1,78 @@
> +Qualcomm Synopsys 1.0.0 SS phy controller
> +===========================================
> +
> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
> +chipsets
> +
> +Required properties:
> +
> +- compatible:
> +    Value type: <string>
> +    Definition: Should contain "qcom,usb-ssphy".

What is "qcom,dwc3-ss-usb-phy" which already exists then?

> +
> +- reg:
> +    Value type: <prop-encoded-array>
> +    Definition: USB PHY base address and length of the register map.
> +
> +- #phy-cells:
> +    Value type: <u32>
> +    Definition: Should be 0. See phy/phy-bindings.txt for details.
> +
> +- clocks:
> +    Value type: <prop-encoded-array>
> +    Definition: See clock-bindings.txt section "consumers". List of
> +		 three clock specifiers for reference, phy core and
> +		 pipe clocks.
> +
> +- clock-names:
> +    Value type: <string>
> +    Definition: Names of the clocks in 1-1 correspondence with the "clocks"
> +		 property. Must contain "ref", "phy" and "pipe".
> +
> +- vdd-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the regulator VDD supply node.
> +
> +- vdda1p8-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the regulator 1.8V supply node.
> +
> +- qcom,vdd-voltage-level:
> +    Value type: <prop-array>
> +    Definition: This is a list of three integer values <no min max> where
> +		 each value corresponding to voltage corner in uV.
> +
> +Optional child nodes:
> +
> +- vbus-supply:
> +    Value type: <phandle>
> +    Definition: phandle to the VBUS supply node.
> +
> +- resets:
> +    Value type: <prop-encoded-array>
> +    Definition: See reset.txt section "consumers". PHY reset specifiers
> +		 for phy core and COR resets.
> +
> +- reset-names:
> +    Value type: <string>
> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
> +		 property. Must contain "com" and "phy".
> +
> +Example:
> +
> +usb3_phy: phy@78000 {
> +	compatible = "qcom,usb-ssphy";
> +	reg = <0x78000 0x400>;
> +	#phy-cells = <0>;
> +	clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
> +		 <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
> +		 <&gcc GCC_USB3_PHY_PIPE_CLK>;
> +	clock-names = "ref", "phy", "pipe";
> +	resets = <&gcc GCC_USB3_PHY_BCR>,
> +		 <&gcc GCC_USB3PHY_PHY_BCR>;
> +	reset-names = "com", "phy";
> +	vdd-supply = <&vreg_l3_1p05>;
> +	vdda1p8-supply = <&vreg_l5_1p8>;
> +	vbus-supply = <&usb3_vbus_reg>;
> +	qcom,vdd-voltage-level = <0 1050000 1050000>;
> +};
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jack Pham]

Hi Rob, Jorge,

On Thu, Dec 20, 2018 at 11:05:31AM -0600, Rob Herring wrote:
> On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote:
> > Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> > controller embedded in QCS404.
> > 
> > Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> > definitions.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> > Reviewed-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 78 ++++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> > new file mode 100644
> > index 0000000..fcf4e01
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> > @@ -0,0 +1,78 @@
> > +Qualcomm Synopsys 1.0.0 SS phy controller
> > +===========================================
> > +
> > +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
> > +chipsets
> > +
> > +Required properties:
> > +
> > +- compatible:
> > +    Value type: <string>
> > +    Definition: Should contain "qcom,usb-ssphy".
> 
> What is "qcom,dwc3-ss-usb-phy" which already exists then?

Uh, apparently only the bindings doc is there but the driver never
landed. I guess it fell through the cracks nearly 4 years ago.

https://lore.kernel.org/patchwork/patch/499502/

Jorge, does Andy's version of this driver at all resemble what can be
used for QCS404?

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez]

On 12/20/18 18:37, Jack Pham wrote:
> Hi Rob, Jorge,
>
> On Thu, Dec 20, 2018 at 11:05:31AM -0600, Rob Herring wrote:
>> On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote:
>>> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
>>> controller embedded in QCS404.
>>>
>>> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
>>> definitions.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>> Reviewed-by: Vinod Koul <vkoul@kernel.org>
>>> ---
>>>   .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 78 ++++++++++++++++++++++
>>>   1 file changed, 78 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>> new file mode 100644
>>> index 0000000..fcf4e01
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>> @@ -0,0 +1,78 @@
>>> +Qualcomm Synopsys 1.0.0 SS phy controller
>>> +===========================================
>>> +
>>> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
>>> +chipsets
>>> +
>>> +Required properties:
>>> +
>>> +- compatible:
>>> +    Value type: <string>
>>> +    Definition: Should contain "qcom,usb-ssphy".
>> What is "qcom,dwc3-ss-usb-phy" which already exists then?
> Uh, apparently only the bindings doc is there but the driver never
> landed. I guess it fell through the cracks nearly 4 years ago.
>
> https://lore.kernel.org/patchwork/patch/499502/
>
> Jorge, does Andy's version of this driver at all resemble what can be
> used for QCS404?

the drivers seem quite different but I'll give it a go as well and 
compare (it is my understanding that the controller is the same - I will 
have access to the hardware documentation soon to compare)

but I'll come back to you on this.

>
> Jack

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez]

On 12/20/18 18:37, Jack Pham wrote:
> Hi Rob, Jorge,
> 
> On Thu, Dec 20, 2018 at 11:05:31AM -0600, Rob Herring wrote:
>> On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote:
>>> Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
>>> controller embedded in QCS404.
>>>
>>> Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
>>> definitions.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>> Reviewed-by: Vinod Koul <vkoul@kernel.org>
>>> ---
>>>   .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 78 ++++++++++++++++++++++
>>>   1 file changed, 78 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>> new file mode 100644
>>> index 0000000..fcf4e01
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
>>> @@ -0,0 +1,78 @@
>>> +Qualcomm Synopsys 1.0.0 SS phy controller
>>> +===========================================
>>> +
>>> +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
>>> +chipsets
>>> +
>>> +Required properties:
>>> +
>>> +- compatible:
>>> +    Value type: <string>
>>> +    Definition: Should contain "qcom,usb-ssphy".
>>
>> What is "qcom,dwc3-ss-usb-phy" which already exists then?
> 
> Uh, apparently only the bindings doc is there but the driver never
> landed. I guess it fell through the cracks nearly 4 years ago.
> 
> https://lore.kernel.org/patchwork/patch/499502/
> 
> Jorge, does Andy's version of this driver at all resemble what can be
> used for QCS404?

on close inspection I cant see any similitudes between the drivers. 
Unfortunately I don't have access to documentation yet but the control 
register offsets and the control bits in the drivers do not match.

because of the above I'd like to go ahead with our separate drivers 
-already tested and validated- for HS (Shawn's) and SS (mine).

if that is acceptable, should we reuse the upstream bindings for  our 
implementation? or perhaps Shawn Guo will do for his HS version of the 
driver and I go ahead and create a new one? what would you suggest?





> 
> Jack
> 

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jack Pham]

Hi Jorge,

Sorry for the late reply as I was out during the holiday break.

On Fri, Dec 28, 2018 at 01:38:59PM +0100, Jorge Ramirez wrote:
> On 12/20/18 18:37, Jack Pham wrote:
> >Hi Rob, Jorge,
> >
> >On Thu, Dec 20, 2018 at 11:05:31AM -0600, Rob Herring wrote:
> >>On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote:
> >>>Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> >>>controller embedded in QCS404.
> >>>
> >>>Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> >>>definitions.
> >>>
> >>>Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >>>Reviewed-by: Vinod Koul <vkoul@kernel.org>
> >>>---
> >>>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 78 ++++++++++++++++++++++
> >>>  1 file changed, 78 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> >>>
> >>>diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> >>>new file mode 100644
> >>>index 0000000..fcf4e01
> >>>--- /dev/null
> >>>+++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> >>>@@ -0,0 +1,78 @@
> >>>+Qualcomm Synopsys 1.0.0 SS phy controller
> >>>+===========================================
> >>>+
> >>>+Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
> >>>+chipsets
> >>>+
> >>>+Required properties:
> >>>+
> >>>+- compatible:
> >>>+    Value type: <string>
> >>>+    Definition: Should contain "qcom,usb-ssphy".
> >>
> >>What is "qcom,dwc3-ss-usb-phy" which already exists then?
> >
> >Uh, apparently only the bindings doc is there but the driver never
> >landed. I guess it fell through the cracks nearly 4 years ago.
> >
> >https://lore.kernel.org/patchwork/patch/499502/
> >
> >Jorge, does Andy's version of this driver at all resemble what can be
> >used for QCS404?
> 
> on close inspection I cant see any similitudes between the drivers.
> Unfortunately I don't have access to documentation yet but the
> control register offsets and the control bits in the drivers do not
> match.
> 
> because of the above I'd like to go ahead with our separate drivers
> -already tested and validated- for HS (Shawn's) and SS (mine).
> 
> if that is acceptable, should we reuse the upstream bindings for
> our implementation? or perhaps Shawn Guo will do for his HS version
> of the driver and I go ahead and create a new one? what would you
> suggest?

I'm not really sure. My understanding of the driver Andy submitted
were for some of the older MSM and IPQ SoCs that implemented the PHY
controls as part of the DWC3 controller's "QScratch" registers, which is
why the bindings doc and the compatible string reference "dwc3" in both
the compatible and the docs filename. Is the SNPS PHY on QCS404
architected similarly in this regard? Either way, the existing bindings
doc for the non-existent driver looks incomplete for QCS404, so you'd
have to update it anyway. My feeling is that there should just be one
document describing all variants of SNPS PHYs on Qualcomm chips.

Maybe we should also just delete the "qcom,dwc3-ss-usb-phy" binding
unless there is a plan to resurrect Andy's driver.

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Andy Gross]

On Mon, 7 Jan 2019 at 14:26, Jack Pham <jackp@codeaurora.org> wrote:
>
> Hi Jorge,
>
> Sorry for the late reply as I was out during the holiday break.
>
> On Fri, Dec 28, 2018 at 01:38:59PM +0100, Jorge Ramirez wrote:
> > On 12/20/18 18:37, Jack Pham wrote:
> > >Hi Rob, Jorge,
> > >
> > >On Thu, Dec 20, 2018 at 11:05:31AM -0600, Rob Herring wrote:
> > >>On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote:
> > >>>Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY
> > >>>controller embedded in QCS404.
> > >>>
> > >>>Based on Sriharsha Allenki's <sallenki@codeaurora.org> original
> > >>>definitions.
> > >>>
> > >>>Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> > >>>Reviewed-by: Vinod Koul <vkoul@kernel.org>
> > >>>---
> > >>>  .../devicetree/bindings/usb/qcom,usb-ssphy.txt     | 78 ++++++++++++++++++++++
> > >>>  1 file changed, 78 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> > >>>
> > >>>diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> > >>>new file mode 100644
> > >>>index 0000000..fcf4e01
> > >>>--- /dev/null
> > >>>+++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt
> > >>>@@ -0,0 +1,78 @@
> > >>>+Qualcomm Synopsys 1.0.0 SS phy controller
> > >>>+===========================================
> > >>>+
> > >>>+Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm
> > >>>+chipsets
> > >>>+
> > >>>+Required properties:
> > >>>+
> > >>>+- compatible:
> > >>>+    Value type: <string>
> > >>>+    Definition: Should contain "qcom,usb-ssphy".
> > >>
> > >>What is "qcom,dwc3-ss-usb-phy" which already exists then?
> > >
> > >Uh, apparently only the bindings doc is there but the driver never
> > >landed. I guess it fell through the cracks nearly 4 years ago.
> > >
> > >https://lore.kernel.org/patchwork/patch/499502/
> > >
> > >Jorge, does Andy's version of this driver at all resemble what can be
> > >used for QCS404?
> >
> > on close inspection I cant see any similitudes between the drivers.
> > Unfortunately I don't have access to documentation yet but the
> > control register offsets and the control bits in the drivers do not
> > match.
> >
> > because of the above I'd like to go ahead with our separate drivers
> > -already tested and validated- for HS (Shawn's) and SS (mine).
> >
> > if that is acceptable, should we reuse the upstream bindings for
> > our implementation? or perhaps Shawn Guo will do for his HS version
> > of the driver and I go ahead and create a new one? what would you
> > suggest?
>
> I'm not really sure. My understanding of the driver Andy submitted
> were for some of the older MSM and IPQ SoCs that implemented the PHY
> controls as part of the DWC3 controller's "QScratch" registers, which is
> why the bindings doc and the compatible string reference "dwc3" in both
> the compatible and the docs filename. Is the SNPS PHY on QCS404
> architected similarly in this regard? Either way, the existing bindings
> doc for the non-existent driver looks incomplete for QCS404, so you'd
> have to update it anyway. My feeling is that there should just be one
> document describing all variants of SNPS PHYs on Qualcomm chips.

Yeah the original driver was specifically for the IPQ8064 phys.  The
actual phy driver changed over time
due to some comments from a few people, but it still used the qscratch
memory for the phy control regs.

Due to this never landing, you can change the phy binding to work for
both of them or just for yours.  If the control
regs are totally different for the QCS404 phy, it should use a
different compatible and driver.  That's my 2 cents.


> Maybe we should also just delete the "qcom,dwc3-ss-usb-phy" binding
> unless there is a plan to resurrect Andy's driver.

I have the hardware I just don't have the time in the short-mid term
to resurrect this.  Unless someone else
wants to pick this up, it'll be a while.  In the meantime, I'd suggest
just changing the binding to apply to the QCS404
if that makes sense (completely different IP / register layout).

Andy

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Stephen Boyd]

Quoting Jorge Ramirez-Ortiz (2018-12-07 01:55:57)
> +
> +- qcom,vdd-voltage-level:
> +    Value type: <prop-array>
> +    Definition: This is a list of three integer values <no min max> where
> +                each value corresponding to voltage corner in uV.

As far as I'm aware, this isn't how qcom's voltage corners are handled
or are going to be handled. Can't you hardcode it in the driver for now?

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez]

On 12/20/18 21:25, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2018-12-07 01:55:57)
>> +
>> +- qcom,vdd-voltage-level:
>> +    Value type: <prop-array>
>> +    Definition: This is a list of three integer values <no min max> where
>> +                each value corresponding to voltage corner in uV.
> As far as I'm aware, this isn't how qcom's voltage corners are handled
> or are going to be handled. Can't you hardcode it in the driver for now?
>
>

ok so you mean use a regulator to control the supply (with 
regulator-min-microvolt and max-microvolt properties instead?
I'll look into that

I guess we should pull back on this one as well?
https://lore.kernel.org/patchwork/patch/1026641/

Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings

[Jorge Ramirez]

On 12/20/18 21:25, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2018-12-07 01:55:57)
>> +
>> +- qcom,vdd-voltage-level:
>> +    Value type: <prop-array>
>> +    Definition: This is a list of three integer values <no min max> where
>> +                each value corresponding to voltage corner in uV.
> 
> As far as I'm aware, this isn't how qcom's voltage corners are handled
> or are going to be handled. Can't you hardcode it in the driver for now?
> 
> 

sure will do.

if there is any other driver in some tree/branch that I could be looking 
as a reference - to avoid having to hardcode the values in the driver- 
please let me know.

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

[Jorge Ramirez-Ortiz]

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>
Reviewed-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/phy/qualcomm/Kconfig           |  11 ++
 drivers/phy/qualcomm/Makefile          |   1 +
 drivers/phy/qualcomm/phy-qcom-usb-ss.c | 340 +++++++++++++++++++++++++++++++++
 3 files changed, 352 insertions(+)
 create mode 100644 drivers/phy/qualcomm/phy-qcom-usb-ss.c

diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index c7b5ee8..35a5a67 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -92,3 +92,14 @@ config PHY_QCOM_USB_HS_SNPS_28NM
 	  Enable this to support the Synopsys 28nm Femto USB PHY on Qualcomm
 	  chips. This driver supports the high-speed PHY which is usually
 	  paired with either the ChipIdea or Synopsys DWC3 USB IPs on MSM SOCs.
+
+config PHY_QCOM_USB_SS
+	tristate "Qualcomm USB SS PHY driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on EXTCON || !EXTCON # if EXTCON=m, this cannot be built-in
+	select GENERIC_PHY
+	help
+	  Enable this to support the Super-Speed USB transceiver on Qualcomm
+	  chips. This driver supports the PHY which uses the QSCRATCH-based
+	  register set for its control sequences, normally paired with newer
+	  DWC3-based Super-Speed controllers on Qualcomm SoCs.
diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index dc238d9..7149261 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_PHY_QCOM_UFS_20NM)		+= phy-qcom-ufs-qmp-20nm.o
 obj-$(CONFIG_PHY_QCOM_USB_HS) 		+= phy-qcom-usb-hs.o
 obj-$(CONFIG_PHY_QCOM_USB_HSIC) 	+= phy-qcom-usb-hsic.o
 obj-$(CONFIG_PHY_QCOM_USB_HS_SNPS_28NM)	+= phy-qcom-usb-hs-snsp-28nm.o
+obj-$(CONFIG_PHY_QCOM_USB_SS)		+= phy-qcom-usb-ss.o
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
@@ -0,0 +1,340 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, Linaro Limited
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#define PHY_CTRL0			0x6C
+#define PHY_CTRL1			0x70
+#define PHY_CTRL2			0x74
+#define PHY_CTRL4			0x7C
+
+/* PHY_CTRL bits */
+#define REF_PHY_EN			BIT(0)
+#define LANE0_PWR_ON			BIT(2)
+#define SWI_PCS_CLK_SEL			BIT(4)
+#define TST_PWR_DOWN			BIT(4)
+#define PHY_RESET			BIT(7)
+
+enum phy_vdd_level { LEVEL_NONE, LEVEL_MIN, LEVEL_MAX, LEVEL_NUM, };
+
+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;
+	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]);
+}
+
+static int qcom_ssphy_ldo_enable(struct ssphy_priv *priv)
+{
+	int ret;
+
+	ret = regulator_set_load(priv->vdda1p8, 23000);
+	if (ret < 0) {
+		dev_err(priv->dev, "Failed to set regulator1p8 load\n");
+		return ret;
+	}
+
+	ret = regulator_set_voltage(priv->vdda1p8, 1800000, 1800000);
+	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);
+	regulator_set_load(priv->vdda1p8, 0);
+}
+
+static int qcom_ssphy_power_on(struct phy *phy)
+{
+	struct ssphy_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	if (!priv->vbus)
+		goto config;
+
+	ret = regulator_enable(priv->vbus);
+	if (ret)
+		return ret;
+config:
+	ret = qcom_ssphy_config_vdd(priv, LEVEL_MIN);
+	if (ret) {
+		dev_err(priv->dev, "Failed to config vdd on\n");
+		goto err;
+	}
+
+	ret = qcom_ssphy_ldo_enable(priv);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable LDO\n");
+		goto err1;
+	}
+
+	ret = clk_prepare_enable(priv->clk_ref);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable the reference clock\n");
+		goto err1;
+	}
+
+	ret = clk_prepare_enable(priv->clk_phy);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable the phy clock\n");
+		goto err2;
+	}
+
+	ret = clk_prepare_enable(priv->clk_pipe);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable the pipe clock\n");
+		goto err3;
+	}
+
+	if (priv->reset_com && priv->reset_phy) {
+		ret = reset_control_assert(priv->reset_com);
+		if (ret) {
+			dev_err(priv->dev, "Failed to assert reset com\n");
+			goto err4;
+		}
+
+		ret = reset_control_assert(priv->reset_phy);
+		if (ret) {
+			dev_err(priv->dev, "Failed to assert reset phy\n");
+			goto err4;
+		}
+
+		usleep_range(10, 20);
+
+		ret = reset_control_deassert(priv->reset_com);
+		if (ret) {
+			dev_err(priv->dev, "Failed to deassert reset com\n");
+			goto err4;
+		}
+
+		ret = reset_control_deassert(priv->reset_phy);
+		if (ret) {
+			dev_err(priv->dev, "Failed to deassert reset phy\n");
+			goto err4;
+		}
+
+		goto power_on;
+	}
+
+	qcom_ssphy_updatel(priv->base + PHY_CTRL1, PHY_RESET, PHY_RESET);
+	usleep_range(10, 20);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL1, PHY_RESET, 0);
+
+power_on:
+	writeb(SWI_PCS_CLK_SEL, priv->base + PHY_CTRL0);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, LANE0_PWR_ON);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, REF_PHY_EN);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, 0);
+
+	return 0;
+
+err4:
+	clk_disable_unprepare(priv->clk_ref);
+err3:
+	clk_disable_unprepare(priv->clk_phy);
+err2:
+	clk_disable_unprepare(priv->clk_ref);
+err1:
+	qcom_ssphy_config_vdd(priv, LEVEL_NONE);
+err:
+	if (priv->vbus)
+		regulator_disable(priv->vbus);
+
+	return ret;
+}
+
+static int qcom_ssphy_power_off(struct phy *phy)
+{
+	struct ssphy_priv *priv = phy_get_drvdata(phy);
+
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, 0);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, 0);
+	qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, TST_PWR_DOWN);
+
+	clk_disable_unprepare(priv->clk_pipe);
+	clk_disable_unprepare(priv->clk_phy);
+	clk_disable_unprepare(priv->clk_ref);
+
+	qcom_ssphy_ldo_disable(priv);
+	qcom_ssphy_config_vdd(priv, LEVEL_NONE);
+
+	if (priv->vbus)
+		regulator_disable(priv->vbus);
+
+	return 0;
+}
+
+static const struct phy_ops qcom_ssphy_ops = {
+	.power_off = qcom_ssphy_power_off,
+	.power_on = qcom_ssphy_power_on,
+	.owner = THIS_MODULE,
+};
+
+static int qcom_ssphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy_provider *provider;
+	struct ssphy_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(struct ssphy_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk_ref = devm_clk_get(dev, "ref");
+	if (IS_ERR(priv->clk_ref)) {
+		dev_err(dev, "Failed to get the reference clock\n");
+		return PTR_ERR(priv->clk_ref);
+	}
+
+	priv->clk_phy = devm_clk_get(dev, "phy");
+	if (IS_ERR(priv->clk_phy)) {
+		dev_err(dev, "Failed to get the phy clock\n");
+		return PTR_ERR(priv->clk_phy);
+	}
+
+	priv->clk_pipe = devm_clk_get(dev, "pipe");
+	if (IS_ERR(priv->clk_pipe)) {
+		dev_err(dev, "Failed to get the pipe clock\n");
+		return PTR_ERR(priv->clk_pipe);
+	}
+
+	priv->reset_com = devm_reset_control_get_optional(dev, "com");
+	if (IS_ERR(priv->reset_com)) {
+		dev_err(dev, "Failed to get reset control com\n");
+		return PTR_ERR(priv->reset_com);
+	}
+
+	if (priv->reset_com) {
+		/* if reset_com is present, reset_phy is no longer optional */
+		priv->reset_phy = devm_reset_control_get(dev, "phy");
+		if (IS_ERR(priv->reset_phy)) {
+			dev_err(dev, "Failed to get reset control phy\n");
+			return PTR_ERR(priv->reset_phy);
+		}
+	}
+
+	priv->vdd = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(priv->vdd)) {
+		dev_err(dev, "Failed to get the vdd regulator\n");
+		return PTR_ERR(priv->vdd);
+	}
+
+	priv->vdda1p8 = devm_regulator_get(dev, "vdda1p8");
+	if (IS_ERR(priv->vdda1p8)) {
+		dev_err(dev, "Failed to get the vdda1p8 regulator\n");
+		return PTR_ERR(priv->vdda1p8);
+	}
+
+	ret = of_property_read_u32_array(dev->of_node,
+					 "qcom,vdd-voltage-level",
+					 priv->vdd_levels,
+					 ARRAY_SIZE(priv->vdd_levels));
+	if (ret) {
+		dev_err(dev, "Failed to read qcom,vdd-voltage-level\n");
+		return ret;
+	}
+
+	priv->vbus = devm_regulator_get_optional(dev, "vbus");
+	if (IS_ERR(priv->vbus)) {
+		if (PTR_ERR(priv->vbus) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		priv->vbus = NULL;
+	}
+
+	phy = devm_phy_create(dev, dev->of_node, &qcom_ssphy_ops);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "Failed to create the ss phy\n");
+		return PTR_ERR(phy);
+	}
+
+	phy_set_drvdata(phy, priv);
+
+	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(provider);
+}
+
+static const struct of_device_id qcom_ssphy_match[] = {
+	{ .compatible = "qcom,usb-ssphy", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_ssphy_match);
+
+static struct platform_driver qcom_ssphy_driver = {
+	.probe		= qcom_ssphy_probe,
+	.driver = {
+		.name	= "qcom_usb_ssphy",
+		.of_match_table = qcom_ssphy_match,
+	},
+};
+module_platform_driver(qcom_ssphy_driver);
+
+MODULE_DESCRIPTION("Qualcomm Super-Speed USB PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

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

[Stephen Boyd]

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

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

[Jorge Ramirez]

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

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

[Stephen Boyd]

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.