[v4,3/4] dt-bindings: Add Qualcomm USB SuperSpeed PHY bindings

Commit Message

Jorge Ramirez-Ortiz Feb. 7, 2019, 11:17 a.m. UTC

Binding description for Qualcomm's Synopsys 1.0.0 SuperSpeed phy
controller embedded in QCS404.

Based on Sriharsha Allenki's <[e-mail]> original
definitions.

Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
---
 .../bindings/phy/qcom,snps-usb-ssphy.txt      | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt

Comments

Rob Herring (Arm) Feb. 13, 2019, 9:43 p.m. UTC | #1

On Thu, Feb 07, 2019 at 12:17:33PM +0100, Jorge Ramirez-Ortiz wrote:
> Binding description for Qualcomm's Synopsys 1.0.0 SuperSpeed phy
> controller embedded in QCS404.
> 
> Based on Sriharsha Allenki's <[e-mail]> original
> definitions.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
> ---
>  .../bindings/phy/qcom,snps-usb-ssphy.txt      | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
> new file mode 100644
> index 000000000000..354e6f9cef62
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
> @@ -0,0 +1,79 @@
> +Qualcomm Synopsys 1.0.0 SS phy controller
> +===========================================
> +
> +Qualcomm 1.0.0 SS phy controller supports SuperSpeed USB connectivity on
> +some Qualcomm platforms.
> +
> +Required properties:
> +
> +- compatible:
> +    Value type: <string>
> +    Definition: Should contain "qcom,snps-usb-ssphy".

Still not specific. Put the SoC name(s) in it.

> +
> +- 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.
> +
> +Optional properties:
> +
> +- resets:
> +    Value type: <prop-encoded-array>
> +    Definition: See reset.txt section "consumers". Specifiers for COM and
> +		 PHY resets.
> +
> +- reset-names:
> +    Value type: <string>
> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
> +		 property. Must contain "com" and "phy" if the property is
> +		 specified.
> +
> +Required child nodes:
> +
> +- usb connector node as defined in bindings/connector/usb-connector.txt
> +  containing the property vbus-supply.
> +
> +Example:
> +
> +usb3_phy: usb3-phy@78000 {
> +	compatible = "qcom,snps-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>;
> +	usb3_c_connector: usb3-c-connector {
> +		compatible = "usb-c-connector";
> +		label = "USB-C";
> +		type = "micro";

'micro' is certainly not right for USB-C. Probably should be omitted?

> +		vbus-supply = <&usb3_vbus_reg>;
> +	};
> +};
> -- 
> 2.20.1
>

Bjorn Andersson Feb. 23, 2019, 4:52 p.m. UTC | #2

On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:

> Binding description for Qualcomm's Synopsys 1.0.0 SuperSpeed phy
> controller embedded in QCS404.
> 
> Based on Sriharsha Allenki's <[e-mail]> original
> definitions.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
> ---
>  .../bindings/phy/qcom,snps-usb-ssphy.txt      | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
> new file mode 100644
> index 000000000000..354e6f9cef62
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
> @@ -0,0 +1,79 @@
> +Qualcomm Synopsys 1.0.0 SS phy controller
> +===========================================
> +
> +Qualcomm 1.0.0 SS phy controller supports SuperSpeed USB connectivity on
> +some Qualcomm platforms.
> +
> +Required properties:
> +
> +- compatible:
> +    Value type: <string>
> +    Definition: Should contain "qcom,snps-usb-ssphy".

Per Rob's request make this:

Should contain "qcom,qcs404-snps-usb-ssphy" and "qcom,snps-usb-ssphy"

You can then leave the driver matching on qcom,snps-usb-ssphy for now
and if we ever find this to be incompatible with other platforms we can
make the driver match on the platform-specific compatible.

> +
> +- 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.
> +
> +Optional properties:
> +
> +- resets:
> +    Value type: <prop-encoded-array>
> +    Definition: See reset.txt section "consumers". Specifiers for COM and
> +		 PHY resets.
> +
> +- reset-names:
> +    Value type: <string>
> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
> +		 property. Must contain "com" and "phy" if the property is
> +		 specified.
> +
> +Required child nodes:
> +
> +- usb connector node as defined in bindings/connector/usb-connector.txt
> +  containing the property vbus-supply.
> +
> +Example:
> +
> +usb3_phy: usb3-phy@78000 {
> +	compatible = "qcom,snps-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>;
> +	usb3_c_connector: usb3-c-connector {

The USB-C connector is attached both to the HS and SS PHYs, so I think
you should represent this external to this node and use of_graph to
query it.

So the connector should look similar to example 2 in
connector/usb-connector.txt.

Regards,
Bjorn

> +		compatible = "usb-c-connector";
> +		label = "USB-C";
> +		type = "micro";
> +		vbus-supply = <&usb3_vbus_reg>;
> +	};
> +};
> -- 
> 2.20.1
>

Jorge Ramirez-Ortiz Aug. 29, 2019, 7:03 a.m. UTC | #3

On 2/23/19 17:52, Bjorn Andersson wrote:
> On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
> 
>> Binding description for Qualcomm's Synopsys 1.0.0 SuperSpeed phy
>> controller embedded in QCS404.
>>
>> Based on Sriharsha Allenki's <[e-mail]> original
>> definitions.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <[e-mail]>
>> ---
>>  .../bindings/phy/qcom,snps-usb-ssphy.txt      | 79 +++++++++++++++++++
>>  1 file changed, 79 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
>> new file mode 100644
>> index 000000000000..354e6f9cef62
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
>> @@ -0,0 +1,79 @@
>> +Qualcomm Synopsys 1.0.0 SS phy controller
>> +===========================================
>> +
>> +Qualcomm 1.0.0 SS phy controller supports SuperSpeed USB connectivity on
>> +some Qualcomm platforms.
>> +
>> +Required properties:
>> +
>> +- compatible:
>> +    Value type: <string>
>> +    Definition: Should contain "qcom,snps-usb-ssphy".
> 
> Per Rob's request make this:
> 
> Should contain "qcom,qcs404-snps-usb-ssphy" and "qcom,snps-usb-ssphy"

ok

> 
> You can then leave the driver matching on qcom,snps-usb-ssphy for now
> and if we ever find this to be incompatible with other platforms we can
> make the driver match on the platform-specific compatible.

ok

> 
>> +
>> +- 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.
>> +
>> +Optional properties:
>> +
>> +- resets:
>> +    Value type: <prop-encoded-array>
>> +    Definition: See reset.txt section "consumers". Specifiers for COM and
>> +		 PHY resets.
>> +
>> +- reset-names:
>> +    Value type: <string>
>> +    Definition: Names of the resets in 1-1 correspondence with the "resets"
>> +		 property. Must contain "com" and "phy" if the property is
>> +		 specified.
>> +
>> +Required child nodes:
>> +
>> +- usb connector node as defined in bindings/connector/usb-connector.txt
>> +  containing the property vbus-supply.
>> +
>> +Example:
>> +
>> +usb3_phy: usb3-phy@78000 {
>> +	compatible = "qcom,snps-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>;
>> +	usb3_c_connector: usb3-c-connector {
> 
> The USB-C connector is attached both to the HS and SS PHYs, so I think
> you should represent this external to this node and use of_graph to
> query it.

but AFAICS we wont be able to retrieve the vbux-supply from an external
node (that interface does not exist).

rob, do you have a suggestion?

> 
> So the connector should look similar to example 2 in
> connector/usb-connector.txt.
> 
> Regards,
> Bjorn
> 
>> +		compatible = "usb-c-connector";
>> +		label = "USB-C";
>> +		type = "micro";
>> +		vbus-supply = <&usb3_vbus_reg>;
>> +	};
>> +};
>> -- 
>> 2.20.1
>>
>

Stephen Boyd Aug. 30, 2019, 4:01 p.m. UTC | #4

Quoting Jorge Ramirez (2019-08-29 00:03:48)
> On 2/23/19 17:52, Bjorn Andersson wrote:
> > On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
> >> +
> >> +Required child nodes:
> >> +
> >> +- usb connector node as defined in bindings/connector/usb-connector.txt
> >> +  containing the property vbus-supply.
> >> +
> >> +Example:
> >> +
> >> +usb3_phy: usb3-phy@78000 {
> >> +    compatible = "qcom,snps-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>;
> >> +    usb3_c_connector: usb3-c-connector {

Node name should be 'connector', not usb3-c-connector.

> > 
> > The USB-C connector is attached both to the HS and SS PHYs, so I think
> > you should represent this external to this node and use of_graph to
> > query it.
> 
> but AFAICS we wont be able to retrieve the vbux-supply from an external
> node (that interface does not exist).
> 
> rob, do you have a suggestion?

Shouldn't the vbus supply be in the phy? Or is this a situation where
the phy itself doesn't have the vbus supply going to it because the PMIC
gets in the way and handles the vbus for the connector by having the SoC
communicate with the PMIC about when to turn the vbus on and off, etc?

> 
> > 
> > So the connector should look similar to example 2 in
> > connector/usb-connector.txt.
> > 
> > Regards,
> > Bjorn
> > 
> >> +            compatible = "usb-c-connector";
> >> +            label = "USB-C";
> >> +            type = "micro";
> >> +            vbus-supply = <&usb3_vbus_reg>;
> >> +    };
> >> +};

Bjorn Andersson Aug. 30, 2019, 4:45 p.m. UTC | #5

On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:

> Quoting Jorge Ramirez (2019-08-29 00:03:48)
> > On 2/23/19 17:52, Bjorn Andersson wrote:
> > > On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
> > >> +
> > >> +Required child nodes:
> > >> +
> > >> +- usb connector node as defined in bindings/connector/usb-connector.txt
> > >> +  containing the property vbus-supply.
> > >> +
> > >> +Example:
> > >> +
> > >> +usb3_phy: usb3-phy@78000 {
> > >> +    compatible = "qcom,snps-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>;
> > >> +    usb3_c_connector: usb3-c-connector {
> 
> Node name should be 'connector', not usb3-c-connector.
> 

It probably has to be usb-c-connector, because we have a
micro-usb-connector on the same board.

> > > 
> > > The USB-C connector is attached both to the HS and SS PHYs, so I think
> > > you should represent this external to this node and use of_graph to
> > > query it.
> > 
> > but AFAICS we wont be able to retrieve the vbux-supply from an external
> > node (that interface does not exist).
> > 
> > rob, do you have a suggestion?
> 
> Shouldn't the vbus supply be in the phy? Or is this a situation where
> the phy itself doesn't have the vbus supply going to it because the PMIC
> gets in the way and handles the vbus for the connector by having the SoC
> communicate with the PMIC about when to turn the vbus on and off, etc?
> 

That's correct, the VBUS comes out of the PMIC and goes directly to the
connector.

The additional complicating factor here is that the connector is wired
to a USB2 phy as well, so we need to wire up detection and vbus control
to both of them - but I think this will be fine, if we can only figure
out a sane way of getting hold of the vbus-supply.

Regards,
Bjorn

Stephen Boyd Aug. 30, 2019, 6:28 p.m. UTC | #6

Quoting Bjorn Andersson (2019-08-30 09:45:20)
> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
> 
> > Quoting Jorge Ramirez (2019-08-29 00:03:48)
> > > On 2/23/19 17:52, Bjorn Andersson wrote:
> > > > On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
> > > >> +
> > > >> +Required child nodes:
> > > >> +
> > > >> +- usb connector node as defined in bindings/connector/usb-connector.txt
> > > >> +  containing the property vbus-supply.
> > > >> +
> > > >> +Example:
> > > >> +
> > > >> +usb3_phy: usb3-phy@78000 {
> > > >> +    compatible = "qcom,snps-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>;
> > > >> +    usb3_c_connector: usb3-c-connector {
> > 
> > Node name should be 'connector', not usb3-c-connector.
> > 
> 
> It probably has to be usb-c-connector, because we have a
> micro-usb-connector on the same board.

Ok. Or connector@1 and connector@2? Our toplevel node container story is
sort of sad because we have to play tricks with node names. But in the
example, just connector I presume? 

> 
> > > > 
> > > > The USB-C connector is attached both to the HS and SS PHYs, so I think
> > > > you should represent this external to this node and use of_graph to
> > > > query it.
> > > 
> > > but AFAICS we wont be able to retrieve the vbux-supply from an external
> > > node (that interface does not exist).
> > > 
> > > rob, do you have a suggestion?
> > 
> > Shouldn't the vbus supply be in the phy? Or is this a situation where
> > the phy itself doesn't have the vbus supply going to it because the PMIC
> > gets in the way and handles the vbus for the connector by having the SoC
> > communicate with the PMIC about when to turn the vbus on and off, etc?
> > 
> 
> That's correct, the VBUS comes out of the PMIC and goes directly to the
> connector.
> 
> The additional complicating factor here is that the connector is wired
> to a USB2 phy as well, so we need to wire up detection and vbus control
> to both of them - but I think this will be fine, if we can only figure
> out a sane way of getting hold of the vbus-supply.
> 

Does it really matter to describe this situation though? Maybe it's
simpler to throw the vbus supply into the phy and control it from the
phy driver, even if it never really goes there. Or put it into the
toplevel usb controller?

Jorge Ramirez-Ortiz Sept. 2, 2019, 6:23 a.m. UTC | #7

On 8/30/19 20:28, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2019-08-30 09:45:20)
>> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
>>
>>> Quoting Jorge Ramirez (2019-08-29 00:03:48)
>>>> On 2/23/19 17:52, Bjorn Andersson wrote:
>>>>> On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
>>>>>> +
>>>>>> +Required child nodes:
>>>>>> +
>>>>>> +- usb connector node as defined in bindings/connector/usb-connector.txt
>>>>>> +  containing the property vbus-supply.
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +usb3_phy: usb3-phy@78000 {
>>>>>> +    compatible = "qcom,snps-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>;
>>>>>> +    usb3_c_connector: usb3-c-connector {
>>>
>>> Node name should be 'connector', not usb3-c-connector.
>>>
>>
>> It probably has to be usb-c-connector, because we have a
>> micro-usb-connector on the same board.
> 
> Ok. Or connector@1 and connector@2? Our toplevel node container story is
> sort of sad because we have to play tricks with node names. But in the
> example, just connector I presume? 
> 
>>
>>>>>
>>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
>>>>> you should represent this external to this node and use of_graph to
>>>>> query it.
>>>>
>>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
>>>> node (that interface does not exist).
>>>>
>>>> rob, do you have a suggestion?
>>>
>>> Shouldn't the vbus supply be in the phy? Or is this a situation where
>>> the phy itself doesn't have the vbus supply going to it because the PMIC
>>> gets in the way and handles the vbus for the connector by having the SoC
>>> communicate with the PMIC about when to turn the vbus on and off, etc?
>>>
>>
>> That's correct, the VBUS comes out of the PMIC and goes directly to the
>> connector.
>>
>> The additional complicating factor here is that the connector is wired
>> to a USB2 phy as well, so we need to wire up detection and vbus control
>> to both of them - but I think this will be fine, if we can only figure
>> out a sane way of getting hold of the vbus-supply.
>>
> 
> Does it really matter to describe this situation though? Maybe it's
> simpler to throw the vbus supply into the phy and control it from the
> phy driver, even if it never really goes there. Or put it into the
> toplevel usb controller?
> 
that would work for me - the connector definition seemed a better way to
explain the connectivity but since we cant retrieve the supply from the
external node is not of much functional use.

but please let me know how to proceed. shall I add the supply back to
the phy?

Jack Pham Sept. 3, 2019, 5:39 p.m. UTC | #8

On Mon, Sep 02, 2019 at 08:23:04AM +0200, Jorge Ramirez wrote:
> On 8/30/19 20:28, Stephen Boyd wrote:
> > Quoting Bjorn Andersson (2019-08-30 09:45:20)
> >> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
> >>
> >>> Quoting Jorge Ramirez (2019-08-29 00:03:48)
> >>>> On 2/23/19 17:52, Bjorn Andersson wrote:
> >>>>> On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
> >>>>>> +
> >>>>>> +Required child nodes:
> >>>>>> +
> >>>>>> +- usb connector node as defined in bindings/connector/usb-connector.txt
> >>>>>> +  containing the property vbus-supply.
> >>>>>> +
> >>>>>> +Example:
> >>>>>> +
> >>>>>> +usb3_phy: usb3-phy@78000 {
> >>>>>> +    compatible = "qcom,snps-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>;
> >>>>>> +    usb3_c_connector: usb3-c-connector {
> >>>
> >>> Node name should be 'connector', not usb3-c-connector.
> >>>
> >>
> >> It probably has to be usb-c-connector, because we have a
> >> micro-usb-connector on the same board.
> > 
> > Ok. Or connector@1 and connector@2? Our toplevel node container story is
> > sort of sad because we have to play tricks with node names. But in the
> > example, just connector I presume? 
> > 
> >>
> >>>>>
> >>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
> >>>>> you should represent this external to this node and use of_graph to
> >>>>> query it.
> >>>>
> >>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
> >>>> node (that interface does not exist).
> >>>>
> >>>> rob, do you have a suggestion?
> >>>
> >>> Shouldn't the vbus supply be in the phy? Or is this a situation where
> >>> the phy itself doesn't have the vbus supply going to it because the PMIC
> >>> gets in the way and handles the vbus for the connector by having the SoC
> >>> communicate with the PMIC about when to turn the vbus on and off, etc?
> >>>
> >>
> >> That's correct, the VBUS comes out of the PMIC and goes directly to the
> >> connector.
> >>
> >> The additional complicating factor here is that the connector is wired
> >> to a USB2 phy as well, so we need to wire up detection and vbus control
> >> to both of them - but I think this will be fine, if we can only figure
> >> out a sane way of getting hold of the vbus-supply.
> >>
> > 
> > Does it really matter to describe this situation though? Maybe it's
> > simpler to throw the vbus supply into the phy and control it from the
> > phy driver, even if it never really goes there. Or put it into the
> > toplevel usb controller?
> > 
> that would work for me - the connector definition seemed a better way to
> explain the connectivity but since we cant retrieve the supply from the
> external node is not of much functional use.
> 
> but please let me know how to proceed. shall I add the supply back to
> the phy?

Putting it in the toplevel usb node makes sense to me, since that's
usually the driver that knows when it's switching into host mode and
needs to turn on VBUS. The dwc3-qcom driver & bindings currently don't 
do this but there's precedent in a couple of the other dwc3 "glues"--see
Documentation/devicetree/bindings/usb/{amlogic\,dwc3,omap-usb}.txt

One exception is if the PMIC is also USB-PD capable and can do power
role swap, in which case the VBUS control needs to be done by the TCPM,
so that'd be a case where having vbus-supply in the connector node might
make more sense.

Jack

Stephen Boyd Sept. 3, 2019, 9:45 p.m. UTC | #9

Quoting Jack Pham (2019-09-03 10:39:24)
> On Mon, Sep 02, 2019 at 08:23:04AM +0200, Jorge Ramirez wrote:
> > On 8/30/19 20:28, Stephen Boyd wrote:
> > > Quoting Bjorn Andersson (2019-08-30 09:45:20)
> > >> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
> > >>
> > >>>>>
> > >>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
> > >>>>> you should represent this external to this node and use of_graph to
> > >>>>> query it.
> > >>>>
> > >>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
> > >>>> node (that interface does not exist).
> > >>>>
> > >>>> rob, do you have a suggestion?
> > >>>
> > >>> Shouldn't the vbus supply be in the phy? Or is this a situation where
> > >>> the phy itself doesn't have the vbus supply going to it because the PMIC
> > >>> gets in the way and handles the vbus for the connector by having the SoC
> > >>> communicate with the PMIC about when to turn the vbus on and off, etc?
> > >>>
> > >>
> > >> That's correct, the VBUS comes out of the PMIC and goes directly to the
> > >> connector.
> > >>
> > >> The additional complicating factor here is that the connector is wired
> > >> to a USB2 phy as well, so we need to wire up detection and vbus control
> > >> to both of them - but I think this will be fine, if we can only figure
> > >> out a sane way of getting hold of the vbus-supply.
> > >>
> > > 
> > > Does it really matter to describe this situation though? Maybe it's
> > > simpler to throw the vbus supply into the phy and control it from the
> > > phy driver, even if it never really goes there. Or put it into the
> > > toplevel usb controller?
> > > 
> > that would work for me - the connector definition seemed a better way to
> > explain the connectivity but since we cant retrieve the supply from the
> > external node is not of much functional use.
> > 
> > but please let me know how to proceed. shall I add the supply back to
> > the phy?

So does the vbus actually go to the phy? I thought it never went there
and the power for the phy was different (and possibly lower in voltage).

> 
> Putting it in the toplevel usb node makes sense to me, since that's
> usually the driver that knows when it's switching into host mode and
> needs to turn on VBUS. The dwc3-qcom driver & bindings currently don't 
> do this but there's precedent in a couple of the other dwc3 "glues"--see
> Documentation/devicetree/bindings/usb/{amlogic\,dwc3,omap-usb}.txt
> 
> One exception is if the PMIC is also USB-PD capable and can do power
> role swap, in which case the VBUS control needs to be done by the TCPM,
> so that'd be a case where having vbus-supply in the connector node might
> make more sense.
> 

The other way is to implement the code to get the vbus supply out of a
connector. Then any driver can do the work if it knows it needs to and
we don't have to care that the vbus isn't going somewhere. I suppose
that would need an of_regulator_get() sort of API that can get the
regulator out of there? Or to make the connector into a struct device
that can get the regulator out per some generic connector driver and
then pass it through to the USB controller when it asks for it. Maybe
try to prototype that out?

Bjorn Andersson Sept. 3, 2019, 11:34 p.m. UTC | #10

On Tue 03 Sep 14:45 PDT 2019, Stephen Boyd wrote:

> Quoting Jack Pham (2019-09-03 10:39:24)
> > On Mon, Sep 02, 2019 at 08:23:04AM +0200, Jorge Ramirez wrote:
> > > On 8/30/19 20:28, Stephen Boyd wrote:
> > > > Quoting Bjorn Andersson (2019-08-30 09:45:20)
> > > >> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
> > > >>
> > > >>>>>
> > > >>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
> > > >>>>> you should represent this external to this node and use of_graph to
> > > >>>>> query it.
> > > >>>>
> > > >>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
> > > >>>> node (that interface does not exist).
> > > >>>>
> > > >>>> rob, do you have a suggestion?
> > > >>>
> > > >>> Shouldn't the vbus supply be in the phy? Or is this a situation where
> > > >>> the phy itself doesn't have the vbus supply going to it because the PMIC
> > > >>> gets in the way and handles the vbus for the connector by having the SoC
> > > >>> communicate with the PMIC about when to turn the vbus on and off, etc?
> > > >>>
> > > >>
> > > >> That's correct, the VBUS comes out of the PMIC and goes directly to the
> > > >> connector.
> > > >>
> > > >> The additional complicating factor here is that the connector is wired
> > > >> to a USB2 phy as well, so we need to wire up detection and vbus control
> > > >> to both of them - but I think this will be fine, if we can only figure
> > > >> out a sane way of getting hold of the vbus-supply.
> > > >>
> > > > 
> > > > Does it really matter to describe this situation though? Maybe it's
> > > > simpler to throw the vbus supply into the phy and control it from the
> > > > phy driver, even if it never really goes there. Or put it into the
> > > > toplevel usb controller?
> > > > 
> > > that would work for me - the connector definition seemed a better way to
> > > explain the connectivity but since we cant retrieve the supply from the
> > > external node is not of much functional use.
> > > 
> > > but please let me know how to proceed. shall I add the supply back to
> > > the phy?
> 
> So does the vbus actually go to the phy? I thought it never went there
> and the power for the phy was different (and possibly lower in voltage).
> 

No, the PHYs use different - lower voltage - supplies to operate. VBUS
is coming from a 5V supply straight to the connector and plug-detect
logic (which is passive in this design).

> > 
> > Putting it in the toplevel usb node makes sense to me, since that's
> > usually the driver that knows when it's switching into host mode and
> > needs to turn on VBUS. The dwc3-qcom driver & bindings currently don't 
> > do this but there's precedent in a couple of the other dwc3 "glues"--see
> > Documentation/devicetree/bindings/usb/{amlogic\,dwc3,omap-usb}.txt
> > 
> > One exception is if the PMIC is also USB-PD capable and can do power
> > role swap, in which case the VBUS control needs to be done by the TCPM,
> > so that'd be a case where having vbus-supply in the connector node might
> > make more sense.
> > 
> 
> The other way is to implement the code to get the vbus supply out of a
> connector. Then any driver can do the work if it knows it needs to and
> we don't have to care that the vbus isn't going somewhere. I suppose
> that would need an of_regulator_get() sort of API that can get the
> regulator out of there? Or to make the connector into a struct device
> that can get the regulator out per some generic connector driver and
> then pass it through to the USB controller when it asks for it. Maybe
> try to prototype that out?
> 

The examples given in the DT bindings describes the connector as a child
of a PMIC, with of_graph somehow tying it to the various inputs. But in
these examples vbus is handled by implicitly inside the MFD, where
extcon is informed about the plug event they toggle vbus as well.

In our case we have a extcon-usb-gpio to detect mode, which per Jorge's
proposal will trickle down to the PHY and become a regulator calls on
either some external regulator or more typically one of the chargers in
the system.


So if we come up with a struct device for the connector and some API for
toggling the vbus we're going to have to fairly abstract entities
representing pretty much the same thing - and in a design with a mux we
would have a different setup.

Regards,
Bjorn

Jorge Ramirez-Ortiz Sept. 5, 2019, 7:18 a.m. UTC | #11

On 9/4/19 01:34, Bjorn Andersson wrote:
> On Tue 03 Sep 14:45 PDT 2019, Stephen Boyd wrote:
> 
>> Quoting Jack Pham (2019-09-03 10:39:24)
>>> On Mon, Sep 02, 2019 at 08:23:04AM +0200, Jorge Ramirez wrote:
>>>> On 8/30/19 20:28, Stephen Boyd wrote:
>>>>> Quoting Bjorn Andersson (2019-08-30 09:45:20)
>>>>>> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
>>>>>>
>>>>>>>>>
>>>>>>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
>>>>>>>>> you should represent this external to this node and use of_graph to
>>>>>>>>> query it.
>>>>>>>>
>>>>>>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
>>>>>>>> node (that interface does not exist).
>>>>>>>>
>>>>>>>> rob, do you have a suggestion?
>>>>>>>
>>>>>>> Shouldn't the vbus supply be in the phy? Or is this a situation where
>>>>>>> the phy itself doesn't have the vbus supply going to it because the PMIC
>>>>>>> gets in the way and handles the vbus for the connector by having the SoC
>>>>>>> communicate with the PMIC about when to turn the vbus on and off, etc?
>>>>>>>
>>>>>>
>>>>>> That's correct, the VBUS comes out of the PMIC and goes directly to the
>>>>>> connector.
>>>>>>
>>>>>> The additional complicating factor here is that the connector is wired
>>>>>> to a USB2 phy as well, so we need to wire up detection and vbus control
>>>>>> to both of them - but I think this will be fine, if we can only figure
>>>>>> out a sane way of getting hold of the vbus-supply.
>>>>>>
>>>>>
>>>>> Does it really matter to describe this situation though? Maybe it's
>>>>> simpler to throw the vbus supply into the phy and control it from the
>>>>> phy driver, even if it never really goes there. Or put it into the
>>>>> toplevel usb controller?
>>>>>
>>>> that would work for me - the connector definition seemed a better way to
>>>> explain the connectivity but since we cant retrieve the supply from the
>>>> external node is not of much functional use.
>>>>
>>>> but please let me know how to proceed. shall I add the supply back to
>>>> the phy?
>>
>> So does the vbus actually go to the phy? I thought it never went there
>> and the power for the phy was different (and possibly lower in voltage).
>>
> 
> No, the PHYs use different - lower voltage - supplies to operate. VBUS
> is coming from a 5V supply straight to the connector and plug-detect
> logic (which is passive in this design).
> 
>>>
>>> Putting it in the toplevel usb node makes sense to me, since that's
>>> usually the driver that knows when it's switching into host mode and
>>> needs to turn on VBUS. The dwc3-qcom driver & bindings currently don't 
>>> do this but there's precedent in a couple of the other dwc3 "glues"--see
>>> Documentation/devicetree/bindings/usb/{amlogic\,dwc3,omap-usb}.txt
>>>
>>> One exception is if the PMIC is also USB-PD capable and can do power
>>> role swap, in which case the VBUS control needs to be done by the TCPM,
>>> so that'd be a case where having vbus-supply in the connector node might
>>> make more sense.
>>>
>>
>> The other way is to implement the code to get the vbus supply out of a
>> connector. Then any driver can do the work if it knows it needs to and
>> we don't have to care that the vbus isn't going somewhere. I suppose
>> that would need an of_regulator_get() sort of API that can get the
>> regulator out of there? Or to make the connector into a struct device
>> that can get the regulator out per some generic connector driver and
>> then pass it through to the USB controller when it asks for it. Maybe
>> try to prototype that out?
>>
> 
> The examples given in the DT bindings describes the connector as a child
> of a PMIC, with of_graph somehow tying it to the various inputs. But in
> these examples vbus is handled by implicitly inside the MFD, where
> extcon is informed about the plug event they toggle vbus as well.
> 
> In our case we have a extcon-usb-gpio to detect mode, which per Jorge's
> proposal will trickle down to the PHY and become a regulator calls on
> either some external regulator or more typically one of the chargers in
> the system.
> 
> 
> So if we come up with a struct device for the connector and some API for
> toggling the vbus we're going to have to fairly abstract entities
> representing pretty much the same thing - and in a design with a mux we
> would have a different setup.

I am a bit unclear - not sure if we have gone full circle on this
subject. what is then the direction to get this merged?

I did have look last week and the level of effort to support regulators
on external nodes is not neglectable meaning that I might not have the
time to deliver that feature (perhaps someone else wishes to take over?)

> 
> Regards,
> Bjorn
>

Jack Pham Sept. 5, 2019, 5:58 p.m. UTC | #12

Hi Jorge, Bjorn,

On Thu, Sep 05, 2019 at 09:18:57AM +0200, Jorge Ramirez wrote:
> On 9/4/19 01:34, Bjorn Andersson wrote:
> > On Tue 03 Sep 14:45 PDT 2019, Stephen Boyd wrote:
> > 
> >> Quoting Jack Pham (2019-09-03 10:39:24)
> >>> On Mon, Sep 02, 2019 at 08:23:04AM +0200, Jorge Ramirez wrote:
> >>>> On 8/30/19 20:28, Stephen Boyd wrote:
> >>>>> Quoting Bjorn Andersson (2019-08-30 09:45:20)
> >>>>>> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
> >>>>>>
> >>>>>>>>>
> >>>>>>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
> >>>>>>>>> you should represent this external to this node and use of_graph to
> >>>>>>>>> query it.
> >>>>>>>>
> >>>>>>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
> >>>>>>>> node (that interface does not exist).
> >>>>>>>>
> >>>>>>>> rob, do you have a suggestion?
> >>>>>>>
> >>>>>>> Shouldn't the vbus supply be in the phy? Or is this a situation where
> >>>>>>> the phy itself doesn't have the vbus supply going to it because the PMIC
> >>>>>>> gets in the way and handles the vbus for the connector by having the SoC
> >>>>>>> communicate with the PMIC about when to turn the vbus on and off, etc?
> >>>>>>>
> >>>>>>
> >>>>>> That's correct, the VBUS comes out of the PMIC and goes directly to the
> >>>>>> connector.
> >>>>>>
> >>>>>> The additional complicating factor here is that the connector is wired
> >>>>>> to a USB2 phy as well, so we need to wire up detection and vbus control
> >>>>>> to both of them - but I think this will be fine, if we can only figure
> >>>>>> out a sane way of getting hold of the vbus-supply.
> >>>>>>
> >>>>>
> >>>>> Does it really matter to describe this situation though? Maybe it's
> >>>>> simpler to throw the vbus supply into the phy and control it from the
> >>>>> phy driver, even if it never really goes there. Or put it into the
> >>>>> toplevel usb controller?
> >>>>>
> >>>> that would work for me - the connector definition seemed a better way to
> >>>> explain the connectivity but since we cant retrieve the supply from the
> >>>> external node is not of much functional use.
> >>>>
> >>>> but please let me know how to proceed. shall I add the supply back to
> >>>> the phy?
> >>
> >> So does the vbus actually go to the phy? I thought it never went there
> >> and the power for the phy was different (and possibly lower in voltage).
> >>
> > 
> > No, the PHYs use different - lower voltage - supplies to operate. VBUS
> > is coming from a 5V supply straight to the connector and plug-detect
> > logic (which is passive in this design).
> > 
> >>>
> >>> Putting it in the toplevel usb node makes sense to me, since that's
> >>> usually the driver that knows when it's switching into host mode and
> >>> needs to turn on VBUS. The dwc3-qcom driver & bindings currently don't 
> >>> do this but there's precedent in a couple of the other dwc3 "glues"--see
> >>> Documentation/devicetree/bindings/usb/{amlogic\,dwc3,omap-usb}.txt
> >>>
> >>> One exception is if the PMIC is also USB-PD capable and can do power
> >>> role swap, in which case the VBUS control needs to be done by the TCPM,
> >>> so that'd be a case where having vbus-supply in the connector node might
> >>> make more sense.
> >>>
> >>
> >> The other way is to implement the code to get the vbus supply out of a
> >> connector. Then any driver can do the work if it knows it needs to and
> >> we don't have to care that the vbus isn't going somewhere. I suppose
> >> that would need an of_regulator_get() sort of API that can get the
> >> regulator out of there? Or to make the connector into a struct device
> >> that can get the regulator out per some generic connector driver and
> >> then pass it through to the USB controller when it asks for it. Maybe
> >> try to prototype that out?
> >>
> > 
> > The examples given in the DT bindings describes the connector as a child
> > of a PMIC, with of_graph somehow tying it to the various inputs. But in
> > these examples vbus is handled by implicitly inside the MFD, where
> > extcon is informed about the plug event they toggle vbus as well.
> > 
> > In our case we have a extcon-usb-gpio to detect mode, which per Jorge's
> > proposal will trickle down to the PHY and become a regulator calls on
> > either some external regulator or more typically one of the chargers in
> > the system.

Interesting you mention extcon-usb-gpio. I thought extcon at least from
bindings perspective is passé now. Maybe this is what you need (just
landed in usb-next):

usb: common: add USB GPIO based connection detection driver
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=4602f3bff2669012c1147eecfe74c121765f5c56

dt-bindings: usb: add binding for USB GPIO based connection detection driver
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=f651c73e71f53f65e9846677d79d8e120452b59f

Fortunately this new driver might check the right boxes for you:
- usb connector binding
- ID detect GPIO
- vbus-supply regulator

With that, I think you can also keep the connector subnode out of the
SSPHY node well, and similarly get rid of the vbus toggle handling from
the PHY driver.

The big thing missing now is that this driver replaces extcon
completely, so we'll need handling in dwc3/dwc3-qcom to retrieve the
role switch state to know when host mode is entered. I saw this a while
back but don't think it got picked up:

https://patchwork.kernel.org/patch/10909981/

> > So if we come up with a struct device for the connector and some API for
> > toggling the vbus we're going to have to fairly abstract entities
> > representing pretty much the same thing - and in a design with a mux we
> > would have a different setup.
> 
> I am a bit unclear - not sure if we have gone full circle on this
> subject. what is then the direction to get this merged?
> 
> I did have look last week and the level of effort to support regulators
> on external nodes is not neglectable meaning that I might not have the
> time to deliver that feature (perhaps someone else wishes to take over?)
> 
> > 
> > Regards,
> > Bjorn
> > 
> 

Jack

Stephen Boyd Sept. 6, 2019, 5:26 a.m. UTC | #13

Quoting Jack Pham (2019-09-05 10:58:02)
> Hi Jorge, Bjorn,
> 
> On Thu, Sep 05, 2019 at 09:18:57AM +0200, Jorge Ramirez wrote:
> > On 9/4/19 01:34, Bjorn Andersson wrote:
> > > On Tue 03 Sep 14:45 PDT 2019, Stephen Boyd wrote:
> > >> that would need an of_regulator_get() sort of API that can get the
> > >> regulator out of there? Or to make the connector into a struct device
> > >> that can get the regulator out per some generic connector driver and
> > >> then pass it through to the USB controller when it asks for it. Maybe
> > >> try to prototype that out?
> > >>
> > > 
> > > The examples given in the DT bindings describes the connector as a child
> > > of a PMIC, with of_graph somehow tying it to the various inputs. But in
> > > these examples vbus is handled by implicitly inside the MFD, where
> > > extcon is informed about the plug event they toggle vbus as well.
> > > 
> > > In our case we have a extcon-usb-gpio to detect mode, which per Jorge's
> > > proposal will trickle down to the PHY and become a regulator calls on
> > > either some external regulator or more typically one of the chargers in
> > > the system.
> 
> Interesting you mention extcon-usb-gpio. I thought extcon at least from
> bindings perspective is passé now. Maybe this is what you need (just
> landed in usb-next):
> 
> usb: common: add USB GPIO based connection detection driver
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=4602f3bff2669012c1147eecfe74c121765f5c56
> 
> dt-bindings: usb: add binding for USB GPIO based connection detection driver
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=f651c73e71f53f65e9846677d79d8e120452b59f
> 
> Fortunately this new driver might check the right boxes for you:
> - usb connector binding
> - ID detect GPIO
> - vbus-supply regulator
> 
> With that, I think you can also keep the connector subnode out of the
> SSPHY node well, and similarly get rid of the vbus toggle handling from
> the PHY driver.
> 
> The big thing missing now is that this driver replaces extcon
> completely, so we'll need handling in dwc3/dwc3-qcom to retrieve the
> role switch state to know when host mode is entered. I saw this a while
> back but don't think it got picked up:
> 
> https://patchwork.kernel.org/patch/10909981/
> 

Yes this looks like the approach that should be taken. One question
though, is this a micro-b connector or a type-c connector on the board?
I thought it was a type-c, so then this USB gpio based connection driver
isn't an exact fit?

Bjorn Andersson Sept. 6, 2019, 6:25 p.m. UTC | #14

On Thu 05 Sep 22:26 PDT 2019, Stephen Boyd wrote:

> Quoting Jack Pham (2019-09-05 10:58:02)
> > Hi Jorge, Bjorn,
> > 
> > On Thu, Sep 05, 2019 at 09:18:57AM +0200, Jorge Ramirez wrote:
> > > On 9/4/19 01:34, Bjorn Andersson wrote:
> > > > On Tue 03 Sep 14:45 PDT 2019, Stephen Boyd wrote:
> > > >> that would need an of_regulator_get() sort of API that can get the
> > > >> regulator out of there? Or to make the connector into a struct device
> > > >> that can get the regulator out per some generic connector driver and
> > > >> then pass it through to the USB controller when it asks for it. Maybe
> > > >> try to prototype that out?
> > > >>
> > > > 
> > > > The examples given in the DT bindings describes the connector as a child
> > > > of a PMIC, with of_graph somehow tying it to the various inputs. But in
> > > > these examples vbus is handled by implicitly inside the MFD, where
> > > > extcon is informed about the plug event they toggle vbus as well.
> > > > 
> > > > In our case we have a extcon-usb-gpio to detect mode, which per Jorge's
> > > > proposal will trickle down to the PHY and become a regulator calls on
> > > > either some external regulator or more typically one of the chargers in
> > > > the system.
> > 
> > Interesting you mention extcon-usb-gpio. I thought extcon at least from
> > bindings perspective is passé now. Maybe this is what you need (just
> > landed in usb-next):
> > 
> > usb: common: add USB GPIO based connection detection driver
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=4602f3bff2669012c1147eecfe74c121765f5c56
> > 
> > dt-bindings: usb: add binding for USB GPIO based connection detection driver
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=f651c73e71f53f65e9846677d79d8e120452b59f
> > 
> > Fortunately this new driver might check the right boxes for you:
> > - usb connector binding
> > - ID detect GPIO
> > - vbus-supply regulator
> > 
> > With that, I think you can also keep the connector subnode out of the
> > SSPHY node well, and similarly get rid of the vbus toggle handling from
> > the PHY driver.
> > 
> > The big thing missing now is that this driver replaces extcon
> > completely, so we'll need handling in dwc3/dwc3-qcom to retrieve the
> > role switch state to know when host mode is entered. I saw this a while
> > back but don't think it got picked up:
> > 
> > https://patchwork.kernel.org/patch/10909981/
> > 
> 
> Yes this looks like the approach that should be taken. One question
> though, is this a micro-b connector or a type-c connector on the board?
> I thought it was a type-c, so then this USB gpio based connection driver
> isn't an exact fit?
> 

For this particular case it's a type c connector, but the port
controller is operated completely passively (and there's no PD or DP
involved), so the GPIO based approach seems like a good fit.

Regards,
Bjorn

Stephen Boyd Sept. 6, 2019, 8:40 p.m. UTC | #15

Quoting Bjorn Andersson (2019-09-06 11:25:30)
> On Thu 05 Sep 22:26 PDT 2019, Stephen Boyd wrote:
> 
> > 
> > Yes this looks like the approach that should be taken. One question
> > though, is this a micro-b connector or a type-c connector on the board?
> > I thought it was a type-c, so then this USB gpio based connection driver
> > isn't an exact fit?
> > 
> 
> For this particular case it's a type c connector, but the port
> controller is operated completely passively (and there's no PD or DP
> involved), so the GPIO based approach seems like a good fit.
> 

OK. Perhaps the binding needs an update then to have another compatible
string indicating type-c connector that's not able to support PD or DP?

Patch

diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
new file mode 100644
index 000000000000..354e6f9cef62
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
@@ -0,0 +1,79 @@ 
+Qualcomm Synopsys 1.0.0 SS phy controller
+===========================================
+
+Qualcomm 1.0.0 SS phy controller supports SuperSpeed USB connectivity on
+some Qualcomm platforms.
+
+Required properties:
+
+- compatible:
+    Value type: <string>
+    Definition: Should contain "qcom,snps-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.
+
+Optional properties:
+
+- resets:
+    Value type: <prop-encoded-array>
+    Definition: See reset.txt section "consumers". Specifiers for COM and
+		 PHY resets.
+
+- reset-names:
+    Value type: <string>
+    Definition: Names of the resets in 1-1 correspondence with the "resets"
+		 property. Must contain "com" and "phy" if the property is
+		 specified.
+
+Required child nodes:
+
+- usb connector node as defined in bindings/connector/usb-connector.txt
+  containing the property vbus-supply.
+
+Example:
+
+usb3_phy: usb3-phy@78000 {
+	compatible = "qcom,snps-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>;
+	usb3_c_connector: usb3-c-connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+		type = "micro";
+		vbus-supply = <&usb3_vbus_reg>;
+	};
+};