Message ID | 20230413113438.1577658-1-bryan.odonoghue@linaro.org |
---|---|
Headers | show |
Series | Add Qualcomm PMIC TPCM support | expand |
On Thu Apr 13, 2023 at 5:08 PM CEST, Bryan O'Donoghue wrote: > On 13/04/2023 15:19, Luca Weiss wrote: > > Hi Bryan, > > > > On Thu Apr 13, 2023 at 1:34 PM CEST, Bryan O'Donoghue wrote: > >> V5: > >> - Amagamates into once device, Heikki, Rob > >> > >> - Takes feedback on usage form Luka and Jianhua on VSafeV state transition detection > >> dev_err() -> dev_warn() > >> > >> - Orientation graph example and general expected bindings > >> I discussed offline with Bjorn the conclusions of the glink/sbu model. > >> The expected orientation-switch path is > >> connector/port@0 <-> phy/port@X <-> dp/port@0 > >> This can then be expanded to > >> connector/port@0 <-> redriver/port@0 <-> phy/port@X <-> dp/port@0 > >> > >> - Rob, Bjorn, Krzysztof > >> > >> - Data role > >> The data-role path is > >> connector/port@0 <-> dwc3/port@Y > > > > I believe I have adjusted my dts correctly for v5 compared to v4 but now > > the usb doesn't seem to work anymore in most cases. > > > > Only when having the phone already plugged in during boot in one > > orientation does USB come up, but also disappears once you replug the > > cable. I still see the same (or at least visually similar) messages when > > plugging in the USB cable or the USB stick but nothing more than that > > happens. > > > > Not that v4 worked perfectly on pm7250b+sm7225(/sm6350) but at least it > > worked in most cases as described in the emails there. Since the driver > > structure changed quite a bit, git diff isn't helpful here > > unfortunately. > > > > Don't think it matters but worth mentioning that sm6350 uses the new > > qmpphy bindings as described in qcom,sc8280xp-qmp-usb43dp-phy.yaml (this > > was also the case when testing v4 of this). > > > > Any idea? > > Can you confirm the output of /sys/class/typec/port0/orientation in host > mode with the USB key / peripheral in both orientations ? I see "reverse" and "normal" depending on the direction the USB stick is plugged in. When unplugged but also when plugged into my PC it stays at "unknown". > > If that's still OK, then perhaps we can figure out the gap in the PHY > code for v3 > > @caleb is working on this code for sdm845 which is a v3 PHY Regards Luca > > --- > bod
On 2023-04-13 12:34:29, Bryan O'Donoghue wrote: > Add a YAML binding for the Type-C silicon interface inside Qualcomm's > pm8150b hardware block. > > The Type-C driver operates with a pdphy driver inside of a high level > single TCPM device. > > Based on original work by Wesley. > > Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../bindings/usb/qcom,pmic-typec.yaml | 169 ++++++++++++++++++ > 1 file changed, 169 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > > diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > new file mode 100644 > index 0000000000000..6d0f5d00305cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > @@ -0,0 +1,169 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/usb/qcom,pmic-typec.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: Qualcomm PMIC based USB Type-C block > + > +maintainers: > + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> > + > +description: | > + Qualcomm PMIC Type-C block > + > +properties: > + compatible: > + enum: > + - qcom,pm8150b-typec > + > + connector: > + type: object > + $ref: /schemas/connector/usb-connector.yaml# > + unevaluatedProperties: false > + > + reg: > + description: Type-C port and pdphy SPMI register base offsets > + minItems: 2 > + maxItems: 2 > + > + interrupts: > + items: > + - description: Bitmask of CC attach, VBUS error, tCCDebounce done and more > + - description: VCONN Powered Detection > + - description: CC state change > + - description: VCONN over-current condition > + - description: VBUS state change > + - description: Attach Deteach notification Deteach -> Detach > + - description: Legacy cable detect > + - description: Try.Src Try.Snk state change > + - description: Sig TX - transmitted reset signal > + - description: Sig RX - received reset signal > + - description: TX completion > + - description: RX completion > + - description: TX fail > + - description: TX discgard > + - description: RX discgard discgard (2x) -> discard > + - description: Fast Role Swap event None of these descriptions follow a similar pattern and are very hard to read and understand. Can you rewrite them? For starters, think about using the same wording and capitalization. > + > + interrupt-names: > + items: > + - const: or-rid-detect-change > + - const: vpd-detect > + - const: cc-state-change > + - const: vconn-oc > + - const: vbus-change > + - const: attach-detach > + - const: legacy-cable-detect > + - const: try-snk-src-detect > + - const: sig-tx > + - const: sig-rx > + - const: msg-tx > + - const: msg-rx > + - const: msg-tx-failed > + - const: msg-tx-discarded > + - const: msg-rx-discarded > + - const: fr-swap > + > + vdd-vbus-supply: > + description: VBUS power supply. > + > + vdd-pdphy-supply: > + description: VDD regulator supply to the PDPHY. > + > + port: > + $ref: /schemas/graph.yaml#/properties/port > + description: > + Contains a port which produces data-role switching messages. > + > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - vdd-vbus-supply > + - vdd-pdphy-supply > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/usb/pd.h> > + > + pm8150b { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pm8150b_typec: typec@1500 { > + compatible = "qcom,pm8150b-typec"; > + reg = <0x1500>, > + <0x1700>; > + > + interrupts = <0x2 0x15 0x00 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x15 0x01 IRQ_TYPE_EDGE_BOTH>, > + <0x2 0x15 0x02 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x15 0x03 IRQ_TYPE_EDGE_BOTH>, > + <0x2 0x15 0x04 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x15 0x05 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x15 0x06 IRQ_TYPE_EDGE_BOTH>, > + <0x2 0x15 0x07 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x00 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x01 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x02 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x03 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x04 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x05 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x06 IRQ_TYPE_EDGE_RISING>, > + <0x2 0x17 0x07 IRQ_TYPE_EDGE_RISING>; > + > + interrupt-names = "or-rid-detect-change", > + "vpd-detect", > + "cc-state-change", > + "vconn-oc", > + "vbus-change", > + "attach-detach", > + "legacy-cable-detect", > + "try-snk-src-detect", > + "sig-tx", > + "sig-rx", > + "msg-tx", > + "msg-rx", > + "msg-tx-failed", > + "msg-tx-discarded", > + "msg-rx-discarded", > + "fr-swap"; > + > + vdd-vbus-supply = <&pm8150b_vbus>; > + vdd-pdphy-supply = <&vreg_l2a_3p1>; > + connector { > + compatible = "usb-c-connector"; > + > + power-role = "source"; > + data-role = "dual"; > + self-powered; > + > + source-pdos = <PDO_FIXED(5000, 3000, PDO_FIXED_DUAL_ROLE | > + PDO_FIXED_USB_COMM | PDO_FIXED_DATA_SWAP)>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + pm8150b_typec_mux_out: endpoint { > + remote-endpoint = <&qmpphy_typec_mux_in>; > + }; > + }; Also a newline? - Marijn > + port@1 { > + reg = <1>; > + pm8150b_typec_role_switch_out: endpoint { > + remote-endpoint = <&dwc3_role_switch_in>; > + }; > + }; > + }; > + }; > + }; > + }; > +... > -- > 2.39.2 >
On 13/04/2023 13:34, Bryan O'Donoghue wrote: > The regulator code needs to know the location of the register to write to > to switch on/off. Right now we have a driver that does this, a yaml that > partially describes it and no dts that uses it. > > Switching on the VBUS for sm8250 shows that we haven't documented reg as a > required property, do so now. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml | 1 + > 1 file changed, 1 insertion(+) Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 13/04/2023 13:34, Bryan O'Donoghue wrote: > Add a YAML binding for the Type-C silicon interface inside Qualcomm's > pm8150b hardware block. > > The Type-C driver operates with a pdphy driver inside of a high level > single TCPM device. Subject: drop second/last, redundant "YAML schema". The "dt-bindings" prefix is already stating that these are bindings (and their format). > > Based on original work by Wesley. > > Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../bindings/usb/qcom,pmic-typec.yaml | 169 ++++++++++++++++++ > 1 file changed, 169 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > > diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > new file mode 100644 > index 0000000000000..6d0f5d00305cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > @@ -0,0 +1,169 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/usb/qcom,pmic-typec.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" Drop quotes from both. > + > +title: Qualcomm PMIC based USB Type-C block > + > +maintainers: > + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> > + > +description: | Do not need '|' unless you need to preserve formatting. > + Qualcomm PMIC Type-C block > + > +properties: > + compatible: > + enum: > + - qcom,pm8150b-typec > + > + connector: > + type: object > + $ref: /schemas/connector/usb-connector.yaml# > + unevaluatedProperties: false > + > + reg: > + description: Type-C port and pdphy SPMI register base offsets > + minItems: 2 Drop minItems. > + maxItems: 2 > + > + interrupts: > + items: > + - description: Bitmask of CC attach, VBUS error, tCCDebounce done and more > + - description: VCONN Powered Detection > + - description: CC state change > + - description: VCONN over-current condition > + - description: VBUS state change > + - description: Attach Deteach notification > + - description: Legacy cable detect > + - description: Try.Src Try.Snk state change > + - description: Sig TX - transmitted reset signal > + - description: Sig RX - received reset signal > + - description: TX completion > + - description: RX completion > + - description: TX fail > + - description: TX discgard > + - description: RX discgard > + - description: Fast Role Swap event > + > + interrupt-names: > + items: > + - const: or-rid-detect-change > + - const: vpd-detect > + - const: cc-state-change > + - const: vconn-oc > + - const: vbus-change > + - const: attach-detach > + - const: legacy-cable-detect > + - const: try-snk-src-detect > + - const: sig-tx > + - const: sig-rx > + - const: msg-tx > + - const: msg-rx > + - const: msg-tx-failed > + - const: msg-tx-discarded > + - const: msg-rx-discarded > + - const: fr-swap > + > + vdd-vbus-supply: > + description: VBUS power supply. > + > + vdd-pdphy-supply: > + description: VDD regulator supply to the PDPHY. > + > + port: > + $ref: /schemas/graph.yaml#/properties/port > + description: > + Contains a port which produces data-role switching messages. I think Rob asked for example for this... > + > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - vdd-vbus-supply > + - vdd-pdphy-supply > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/usb/pd.h> > + > + pm8150b { pmic > + #address-cells = <1>; > + #size-cells = <0>; > + Best regards, Krzysztof
On 14/04/2023 07:51, Luca Weiss wrote: > I see "reverse" and "normal" depending on the direction the USB stick is > plugged in. When unplugged but also when plugged into my PC it stays at > "unknown". Right so, this is down to bad behavior on the PHY patch, which is resolved for me on sm8250 with the below. Basically when you unplug a device you would transition back to "TYPEC_ORIENTATION_NONE" but that would turn off the PHY, which is obs not very useful if you want to subsequently be a gadget. Anyway thanks for testing this - I'd missed the host->device->host->device ping-pong breakage. diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index b9a30c087423d..edb788a71edeb 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -3372,12 +3372,13 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, qmp->orientation = orientation; - if (orientation == TYPEC_ORIENTATION_NONE) { - if (qmp->init_count) - ret = qmp_combo_dp_power_off(dp_phy); - } else { - if (!qmp->init_count) - ret = qmp_combo_dp_power_on(dp_phy); + if (orientation != TYPEC_ORIENTATION_NONE) { + ret = qmp_combo_dp_power_off(dp_phy); + if (ret) + return ret; + ret = qmp_combo_dp_power_on(dp_phy); + if (ret) + return ret; } --- bod
On Mon Apr 17, 2023 at 2:30 AM CEST, Bryan O'Donoghue wrote: > On 14/04/2023 07:51, Luca Weiss wrote: > > I see "reverse" and "normal" depending on the direction the USB stick is > > plugged in. When unplugged but also when plugged into my PC it stays at > > "unknown". > > Right so, this is down to bad behavior on the PHY patch, which is > resolved for me on sm8250 with the below. > > Basically when you unplug a device you would transition back to > "TYPEC_ORIENTATION_NONE" but that would turn off the PHY, which is obs > not very useful if you want to subsequently be a gadget. > > Anyway thanks for testing this - I'd missed the > host->device->host->device ping-pong breakage. Hm, unfortunately no improvement with this on my side.. No USB connection pops up on the host, or USB messages regarding the USB stick on the device. Do you have an idea in which part of the code to start debugging this? Since orientation detection is working is it maybe in the phy code and not in the tcpm driver? Or does that also touch crucial stuff for USB apart from telling phy which direction to use? Regards Luca > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > index b9a30c087423d..edb788a71edeb 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > @@ -3372,12 +3372,13 @@ static int qmp_combo_typec_switch_set(struct > typec_switch_dev *sw, > > qmp->orientation = orientation; > > - if (orientation == TYPEC_ORIENTATION_NONE) { > - if (qmp->init_count) > - ret = qmp_combo_dp_power_off(dp_phy); > - } else { > - if (!qmp->init_count) > - ret = qmp_combo_dp_power_on(dp_phy); > + if (orientation != TYPEC_ORIENTATION_NONE) { > + ret = qmp_combo_dp_power_off(dp_phy); > + if (ret) > + return ret; > + ret = qmp_combo_dp_power_on(dp_phy); > + if (ret) > + return ret; > } > > --- > bod
On 17/04/2023 08:35, Luca Weiss wrote: > Do you have an idea in which part of the code to start debugging this? > Since orientation detection is working is it maybe in the phy code and > not in the tcpm driver? Or does that also touch crucial stuff for USB > apart from telling phy which direction to use? PHY - I'd almost just do the following diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index edb788a71edeb..bbac82bd093f8 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -3369,7 +3369,7 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, dev_dbg(qmp->dev, "Toggling orientation current %d requested %d\n", qmp->orientation, orientation); - +return 0; In that case the PHY should "just work" for host or device in one orientation. The other possibility is that the data role message is not hitting dwc3 drd on your platform. If you take the last commit on this branch - plus the updated PHY commit Commit: 171d7f507511 ("usb: dwc3: drd: Enable user-space triggered role-switching") Commit: eb0daa19f3ad ("phy: qcom-qmp: Register as a typec switch for orientation detection") https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-04-17-pm8150b-tcpm-qcom-wrapper-typec-mux cat /sys/class/usb_role/a600000.usb-role-switch/role On SM8250 it looks like this - Attach TypeC accessory with USB key plugged in [1] Mount USB key, read/write some data Unmount USB key cat /sys/class/usb_role/a600000.usb-role-switch/role host - Attach TypeC accessory in opposite orientation Run same test - Connect to PC via TypeC cable Run usb-ecm-up.sh [2] cat /sys/class/usb_role/a600000.usb-role-switch/role device PC : ifconfig enp49s0f3u2u4 192.168.8.1 SM8250 : ifconfig usb0 192.168.8.2 Then PC : iperf -s SM8250 : iperf -c 192.168.8.1 -t 10 [ 1] 0.0000-10.0706 sec 307 MBytes 256 Mbits/sec - Unplug from PC - replug TypeC accessory Rerun test in both orientations - Replug target to PC In this case we only have to reset the IP address PC : ifconfig enp49s0f3u2u4 192.168.8.1 SM8250 : ifconfig usb0 192.168.8.2 Yep its worth checking out that the data-role switch is working, we might be looking at the wrong thing for you on the PHY. [1] https://www.amazon.com/CableCreation-Multiport-Adapter-Gigabit-Ethernet/dp/B08FWMWGTD [2] usb-ecm-up.sh root@linaro-gnome:~# cat usb-ecm-up.sh #!/usr/bin/env bash # load libcomposite module modprobe libcomposite # ensure function is loaded modprobe usb_f_ecm modprobe usb_f_ncm mount -t configfs none /sys/kernel/config/ # create a gadget mkdir /sys/kernel/config/usb_gadget/g0 # cd to its configfs node cd /sys/kernel/config/usb_gadget/g0 # configure it (vid/pid can be anything if USB Class is used for driver compat) echo 0x0525 > idVendor echo 0xa4a4 > idProduct # configure its serial/mfg/product mkdir strings/0x409 echo 0xCAFEBABE > strings/0x409/serialnumber echo Linaro > strings/0x409/manufacturer echo qrb5165-rb5 > strings/0x409/product # create configs mkdir configs/c.1 mkdir configs/c.1/strings/0x409 # create the function (name must match a usb_f_<name> module such as 'acm') mkdir functions/ncm.0 echo "CDC ECM" > configs/c.1/strings/0x409/configuration # associate function with config ln -s functions/ncm.0 configs/c.1 # Set USB version 3.1 echo 0x0310 > bcdUSB echo "super-speed-plus" > max_speed # enable gadget by binding it to a UDC from /sys/class/udc #echo a600000.dwc3 > UDC echo a600000.usb > UDC # to unbind it: echo "" > UDC; sleep 1; rm -rf /sys/kernel/config/usb_gadget/g0 sleep 1 ifconfig usb0 192.168.8.2
On 17/04/2023 11:04, Bryan O'Donoghue wrote: > Unmount USB key > > cat /sys/class/usb_role/a600000.usb-role-switch/role > host Sorry obviously confirm the data role before detaching the accessory :)
On 21/04/2023 11:26, Luca Weiss wrote: > With the "user-space triggered role-switching" patch I can see that > whatever scenario the USB-C port is in, the role is stuck on "device". Hmm. Could you share a branch ? --- bod