| Message ID | 20230501121111.1058190-13-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Qualcomm PMIC TPCM support | expand |
On Mon, May 01, 2023 at 01:11:10PM +0100, Bryan O'Donoghue wrote: > Switch on usb-role-switching for usb_1 via TCPM. We need to declare > usb-role-switch in &usb_1 and associate with the remote-endpoint in TCPM > which provides the necessary signal. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 19 ++++++++++++++++++- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index 1e0b6fd59abc9..b5cc45358a474 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -1273,7 +1273,13 @@ &usb_1 { > }; > > &usb_1_dwc3 { > - dr_mode = "peripheral"; > + dr_mode = "otg"; > + usb-role-switch; > + port { > + dwc3_role_switch_in: endpoint { > + remote-endpoint = <&pm8150b_role_switch_out>; > + }; > + }; > }; > > &usb_1_hsphy { > @@ -1359,5 +1365,16 @@ connector { > PDO_FIXED_DUAL_ROLE | > PDO_FIXED_USB_COMM | > PDO_FIXED_DATA_SWAP)>; > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + pm8150b_role_switch_out: endpoint { > + remote-endpoint = <&dwc3_role_switch_in>; > + }; > + }; This port node should be moved out to the block of pm8150b_typec rather than usb-c-connector. Otherwise, [ 10.998897] OF: graph: no port node found in /soc@0/spmi@c440000/pmic@2/typec@1500 > + }; > }; > }; > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index af16d3ba76b8e..af988328db6b9 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -3740,6 +3740,10 @@ usb_1_dwc3: usb@a600000 { > snps,dis_enblslpm_quirk; > phys = <&usb_1_hsphy>, <&usb_1_ssphy>; > phy-names = "usb2-phy", "usb3-phy"; > + > + port { > + dwc3_role_switch_in: endpoint {}; > + }; > }; > }; > > -- > 2.39.2 > >
On Mon, May 01, 2023 at 01:11:10PM +0100, Bryan O'Donoghue wrote: > Switch on usb-role-switching for usb_1 via TCPM. We need to declare > usb-role-switch in &usb_1 and associate with the remote-endpoint in TCPM > which provides the necessary signal. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 19 ++++++++++++++++++- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index 1e0b6fd59abc9..b5cc45358a474 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -1273,7 +1273,13 @@ &usb_1 { > }; > > &usb_1_dwc3 { > - dr_mode = "peripheral"; > + dr_mode = "otg"; > + usb-role-switch; > + port { Add new line before subndoe > + dwc3_role_switch_in: endpoint { > + remote-endpoint = <&pm8150b_role_switch_out>; > + }; > + }; > }; > > &usb_1_hsphy { > @@ -1359,5 +1365,16 @@ connector { > PDO_FIXED_DUAL_ROLE | > PDO_FIXED_USB_COMM | > PDO_FIXED_DATA_SWAP)>; > + ports { Same > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + pm8150b_role_switch_out: endpoint { > + remote-endpoint = <&dwc3_role_switch_in>; > + }; > + }; > + }; > }; > }; > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index af16d3ba76b8e..af988328db6b9 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -3740,6 +3740,10 @@ usb_1_dwc3: usb@a600000 { > snps,dis_enblslpm_quirk; > phys = <&usb_1_hsphy>, <&usb_1_ssphy>; > phy-names = "usb2-phy", "usb3-phy"; > + > + port { > + dwc3_role_switch_in: endpoint {}; > + }; > }; > }; > > -- > 2.39.2 > >
On 01/05/2023 16:21, Jianhua Lu wrote: > This port node should be moved out to the block of pm8150b_typec rather than > usb-c-connector. Otherwise, > > [ 10.998897] OF: graph: no port node found in /soc@0/spmi@c440000/pmic@2/typec@1500 Ports/endpoints should go in the connector. Documentation/devicetree/bindings/connector/usb-connector.yaml grep usb-c-connector arch/arm* -r This dtsi should be updated arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi != Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml --- bod
On Mon, May 01, 2023 at 04:53:22PM +0100, Bryan O'Donoghue wrote: > On 01/05/2023 16:21, Jianhua Lu wrote: > > This port node should be moved out to the block of pm8150b_typec rather than > > usb-c-connector. Otherwise, > > > > [ 10.998897] OF: graph: no port node found in /soc@0/spmi@c440000/pmic@2/typec@1500 > uh.... This is runtime error, it doesn't matter bindings. I haven't much experience with typc-c, so I can't address code for this. > > Ports/endpoints should go in the connector. > > Documentation/devicetree/bindings/connector/usb-connector.yaml > > grep usb-c-connector arch/arm* -r > > This dtsi should be updated > > arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi != > Documentation/devicetree/bindings/usb/nxp,ptn5110.yaml > > --- > bod
On 1.05.2023 14:11, Bryan O'Donoghue wrote: > Switch on usb-role-switching for usb_1 via TCPM. We need to declare > usb-role-switch in &usb_1 and associate with the remote-endpoint in TCPM > which provides the necessary signal. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 19 ++++++++++++++++++- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index 1e0b6fd59abc9..b5cc45358a474 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -1273,7 +1273,13 @@ &usb_1 { > }; > > &usb_1_dwc3 { > - dr_mode = "peripheral"; > + dr_mode = "otg"; > + usb-role-switch; > + port { > + dwc3_role_switch_in: endpoint { Any reason you're redefining it? You can just do &dwc3_role_switch_in { remote-endpoint = <&pm8150b_role_switch_out>; }; > + remote-endpoint = <&pm8150b_role_switch_out>; > + }; > + }; > }; > > &usb_1_hsphy { > @@ -1359,5 +1365,16 @@ connector { > PDO_FIXED_DUAL_ROLE | > PDO_FIXED_USB_COMM | > PDO_FIXED_DATA_SWAP)>; > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + pm8150b_role_switch_out: endpoint { Similarly to the QMPPHY, the port definition can be moved to the common node in the SoC DTSI Konrad > + remote-endpoint = <&dwc3_role_switch_in>; > + }; > + }; > + }; > }; > }; > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index af16d3ba76b8e..af988328db6b9 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -3740,6 +3740,10 @@ usb_1_dwc3: usb@a600000 { > snps,dis_enblslpm_quirk; > phys = <&usb_1_hsphy>, <&usb_1_ssphy>; > phy-names = "usb2-phy", "usb3-phy"; > + > + port { > + dwc3_role_switch_in: endpoint {}; > + }; > }; > }; >
On 02/05/2023 12:00, Konrad Dybcio wrote: >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + pm8150b_role_switch_out: endpoint { > Similarly to the QMPPHY, the port definition can be moved to > the common node in the SoC DTSI But then the connector would have to be defined in the SoC dtsi and not all derivatives of SoC can be assumed to have a usb-c-connector. grep "usb-c-connector" arch/arm64/boot/dts/qcom/* --- bod
On 2.05.2023 13:03, Bryan O'Donoghue wrote: > On 02/05/2023 12:00, Konrad Dybcio wrote: >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + port@0 { >>> + reg = <0>; >>> + pm8150b_role_switch_out: endpoint { >> Similarly to the QMPPHY, the port definition can be moved to >> the common node in the SoC DTSI > > But then the connector would have to be defined in the SoC dtsi and not all derivatives of SoC can be assumed to have a usb-c-connector. Not quite! You can define an empty endpoint (like we do with DSI<->panel ones) and fill it in on the device side. Konrad > > grep "usb-c-connector" arch/arm64/boot/dts/qcom/* > > --- > bod
On 02/05/2023 12:13, Konrad Dybcio wrote: > > > On 2.05.2023 13:03, Bryan O'Donoghue wrote: >> On 02/05/2023 12:00, Konrad Dybcio wrote: >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + port@0 { >>>> + reg = <0>; >>>> + pm8150b_role_switch_out: endpoint { >>> Similarly to the QMPPHY, the port definition can be moved to >>> the common node in the SoC DTSI >> >> But then the connector would have to be defined in the SoC dtsi and not all derivatives of SoC can be assumed to have a usb-c-connector. > Not quite! > > You can define an empty endpoint (like we do with DSI<->panel ones) and > fill it in on the device side. Sorry you're saying to define as an example the port here in the dtsi &usb_1_dwc3 { dr_mode = "otg"; usb-role-switch; port { dwc3_role_switch_in: endpoint { remote-endpoint = <&pm8150b_role_switch_out>; }; }; }; and to leave the reciprocal definition in the connector to the dts ? &pm8150b_typec { connector { compatible = "usb-c-connector"; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; pm8150b_role_switch_out: endpoint { remote-endpoint = <&dwc3_role_switch_in>; }; };
On 2.05.2023 13:16, Bryan O'Donoghue wrote: > On 02/05/2023 12:13, Konrad Dybcio wrote: >> >> >> On 2.05.2023 13:03, Bryan O'Donoghue wrote: >>> On 02/05/2023 12:00, Konrad Dybcio wrote: >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + port@0 { >>>>> + reg = <0>; >>>>> + pm8150b_role_switch_out: endpoint { >>>> Similarly to the QMPPHY, the port definition can be moved to >>>> the common node in the SoC DTSI >>> >>> But then the connector would have to be defined in the SoC dtsi and not all derivatives of SoC can be assumed to have a usb-c-connector. >> Not quite! >> >> You can define an empty endpoint (like we do with DSI<->panel ones) and >> fill it in on the device side. > > Sorry you're saying to define as an example the port here in the dtsi > > &usb_1_dwc3 { > dr_mode = "otg"; > usb-role-switch; === > port { > dwc3_role_switch_in: endpoint { > remote-endpoint = <&pm8150b_role_switch_out>; > }; > }; === this part (minus remote-endpoint) would go to SoC dtsi remote-endpoint would be assigned on a per-device basis > }; > > and to leave the reciprocal definition in the connector to the dts ? > > &pm8150b_typec { > ==== > connector { > compatible = "usb-c-connector"; > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > pm8150b_role_switch_out: endpoint { > remote-endpoint = <&dwc3_role_switch_in>; > }; > }; ==== this part (minus remote-endpoint) would go to pm8150b.dtsi remote-endpoint would be assigned (or left empty) on a per-device basis Konrad >
On 02/05/2023 12:47, Konrad Dybcio wrote: > > this part (minus remote-endpoint) would go to pm8150b.dtsi > > remote-endpoint would be assigned (or left empty) on a per-device basis Hmm. Yes, actually I think I can locate the connector {} defintion in the pm8150b.dtsi.. you would need to have one tcpm {typec, pdphy} block per type-c port so.. yes done --- bod
On 02/05/2023 12:47, Konrad Dybcio wrote: >> connector { >> compatible = "usb-c-connector"; >> ports { >> #address-cells = <1>; >> #size-cells = <0>; >> >> port@0 { >> reg = <0>; >> pm8150b_role_switch_out: endpoint { >> remote-endpoint = <&dwc3_role_switch_in>; >> }; >> }; > ==== Hmm, I'm actually recoiling from doing this now that I look at it. - Every other implementation defines the connector in the platfrom dts not the core dtsi - The connector would have to be named something like pm8150b_type_connector which is not consistent with the rest of the Type-C connector namespace Yes it can be done that the connector with the ports can be defined in the pm8150b dtsi but TBH I think it is more confusing that way and not consistent with other implementations. I'm citing prior art here I just think this is nicer/easier to follow - arch/arm64/boot/dts/qcom/sc8280xp.dtsi - arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts So for now https://imgflip.com/i/7kw6ck --- bod
diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts index 1e0b6fd59abc9..b5cc45358a474 100644 --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts @@ -1273,7 +1273,13 @@ &usb_1 { }; &usb_1_dwc3 { - dr_mode = "peripheral"; + dr_mode = "otg"; + usb-role-switch; + port { + dwc3_role_switch_in: endpoint { + remote-endpoint = <&pm8150b_role_switch_out>; + }; + }; }; &usb_1_hsphy { @@ -1359,5 +1365,16 @@ connector { PDO_FIXED_DUAL_ROLE | PDO_FIXED_USB_COMM | PDO_FIXED_DATA_SWAP)>; + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + pm8150b_role_switch_out: endpoint { + remote-endpoint = <&dwc3_role_switch_in>; + }; + }; + }; }; }; diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi index af16d3ba76b8e..af988328db6b9 100644 --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi @@ -3740,6 +3740,10 @@ usb_1_dwc3: usb@a600000 { snps,dis_enblslpm_quirk; phys = <&usb_1_hsphy>, <&usb_1_ssphy>; phy-names = "usb2-phy", "usb3-phy"; + + port { + dwc3_role_switch_in: endpoint {}; + }; }; };
Switch on usb-role-switching for usb_1 via TCPM. We need to declare usb-role-switch in &usb_1 and associate with the remote-endpoint in TCPM which provides the necessary signal. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 19 ++++++++++++++++++- arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++ 2 files changed, 22 insertions(+), 1 deletion(-)