mbox series

[v5,00/14] Add Qualcomm PMIC TPCM support

Message ID 20230413113438.1577658-1-bryan.odonoghue@linaro.org
Headers show
Series Add Qualcomm PMIC TPCM support | expand

Message

Bryan O'Donoghue April 13, 2023, 11:34 a.m. UTC
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 

Previous set:
https://lore.kernel.org/linux-arm-msm/20230318121828.739424-1-bryan.odonoghue@linaro.org/

Bootable:
https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-04-12-pm8150b-tcpm-qcom-wrapper-typec-mux
   
V4:
- Per Rob's input the pdphy and type-c appear as stadalone blocks
  inside of the PMIC declaration which is a 1:1 mapping of PMIC hardware.
  The TCPM virtual device is declared at the top-level.
  https://lore.kernel.org/all/YY7p7jviA3ZG05gL@robh.at.kernel.org/

- Squashes the removal of the old driver with the addition of the new. - Heikki, Gunter
  https://lore.kernel.org/all/YYVHcHC1Gm92VxEM@kuha.fi.intel.com/

- Reworked Dmitry's old patch for the QMP to account for file renames and
  very minimal code-drift in the interregnum.

- New yaml checks drive update of PMIC VBUS yaml

- Some housekeeping on the sc7180 yaml side. sc7180 is not supported yet.

- Expands and fixes the examples being added in the PMIC tcpm examples.

Previous set:
https://lore.kernel.org/all/20211105033558.1573552-1-bryan.odonoghue@linaro.org/

Bootable:
https://git.codelinaro.org/bryan.odonoghue/kernel/-/commits/linux-next-23-03-18-pm8150b-tcpm-qcom-wrapper-typec-mux

V3:
Rob Herrings review

- Drops use of remote-endpoint and ports to bind
  tcpm to pdphy and typec replacing with phandle

- Drops pmic-pdphy-* and pmic-typec-* from interrupt names
  as suggested

- Passes make dt_binding_check DT_CHECKER_FLAGS=-m

BOD
- Noticed qcom_pmic_tcpm_pdphy_enable() was missing a
  regulator_disable in case of an error, added.

- qcom_pmic_tcpm_pdphy_probe()
  devm_regulator_get() should come before regmap_get()
  as is the case in qcom_pmic_tcpm_typec_probe()

- Fixes compatible name in qcom,pmic-typec.yaml should
  have read qcom,pm8150b-typec not qcom,pm8150b-usb-typec

- Makes sure compat for core is "qcom,pm8150b-tcpm" in
  docs and driver

- Drops redundant return in void qcom_pmic_tcpm_pdphy_reset_off()

Kernel Robot
- Drops unused variable debounced in qcom_pmic_tcpm_typec_get_cc()

- Drops unsused variable orientation in qcom_pmic_tcpm_typec_set_cc()

Latest bootable series can be found here:
Link: https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=usb-next-04-11-21-pm8150b-tcpm-v3

git diff usb-next-27-10-21-pm8150b-tcpm-v2 -- drivers/usb/typec/tcpm/qcom/
git diff usb-next-27-10-21-pm8150b-tcpm-v2 -- Documentation/devicetree/bindings/usb/qcom,pmic*

Previous set:
Link: https://lore.kernel.org/linux-usb/20211028164941.831918-1-bryan.odonoghue@linaro.org/T/#t

V2 resend:
- Adding omitted devicetree mailing list

V2:

Guenter Roeck's review
- Converts suggested qcom_pmic_tcpm_core.c into one-liners

- Adds comment on how polarity is set in set_polarity()

- Removes optional set_current_limit()

- regmap_read/regmap_write
  Reviwing other pm8150b/spmi drivers I then added in checks for all
  reamap_read()/regmap_write() calls.

- Fixes (type == TCPC_TX_CABLE_RESET || TCPC_TX_HARD_RESET)
  thanks I definitely had the blinkers on there and didn't see that at all

- qcom_pmic_tcpm_pdphy_pd_transmit_payload()
  Treats regmap_read and read value as separate error paths

- qcom_pmic_tcpm_pdphy_set_pd_rx()
  Replaces boolean if/else with !on as suggested

- Returns -ENODEV not -EINVAL on dev_get_regmap() error

- qcom_pmic_tcpm_pdphy_pd_receive()
  Guenter asks: "No error return ?"
  bod: No we are inside an ISR here if we read data we pass that off to TCPM
       if somehow we don't read the data - it is "junk" there's no value IMO
       in pushing an error upwards back to the handler.

Heikki Krogerus' review
- Includes Makefile I missed adding to my git index

- Removes old Kconfig entry for remove driver

Randy Dunlap's review 
- Rewords drivers/usb/typec/tcpm/Kconfig

- Drops tautology "aggregates togther"

- Corrects spelling typos

BOD's own review
- Drops redundant include of regmap.h in qcom_pmic_tcpm_core.c

- Propogates qcom_pmic_tcpm_pdphy_disable() error upwards

- Propogates pmic_pdphy_reset() error upwards

- Drops error prints in qcom_pmic_tcpm_pdphy_pd_transmit_payload()
  I had these in-place during development and don't recall them being
  triggered even once, they are redundant, remove.
 
Differences between the two can be seen by
git diff usb-next-27-10-21-pm8150b-tcpm-v2..usb-next-25-10-21-pm8150b-tcpm -- drivers/usb/typec/tcpm

Latest bootable series can be found here:
Link: https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=usb-next-27-10-21-pm8150b-tcpm-v2

Previous set:
Link: https://lore.kernel.org/all/20211025150906.176686-1-bryan.odonoghue@linaro.org/T/#t

V1:
This series adds a set of yaml and a driver to bind together the type-c and
pdphy silicon in qcom's pm8150b block as a Linux type-c port manager.

As part of that we retire the existing qcom-pmic-typec driver and fully
replicate its functionality inside of the new block with the additional
pdphy stuff along with it.

An additional series will follow this one for the SoC and RB5 dtsi and dts
respectively.

A bootable series can be found here

Link: https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=usb-next-25-10-21-pm8150b-tcpm

Bryan O'Donoghue (13):
  dt-bindings: regulator: qcom,usb-vbus-regulator: Mark reg as required
  dt-bindings: regulator: qcom,usb-vbus-regulator: Mark
    regulator-*-microamp required
  dt-bindings: phy: qcom,sc7180-qmp-usb3-dp-phy: Add orientation-switch
    as optional
  dt-bindings: phy: qcom,sc7180-qmp-usb3-dp-phy: Add ports as an
    optional
  dt-bindings: usb: Add Qualcomm PMIC Type-C YAML schema
  dt-bindings: mfd: qcom,spmi-pmic: Add typec to SPMI device types
  arm64: dts: qcom: sm8250: Define ports for qmpphy
    orientation-switching
  arm64: dts: qcom: pm8150b: Add a TCPM description
  arm64: dts: qcom: qrb5165-rb5: Switch on Type-C VBUS boost
  arm64: dts: qcom: qrb5165-rb5: Switch on basic TCPM
  arm64: dts: qcom: qrb5165-rb5: Switch on TCPM usb-role-switching for
    usb_1
  arm64: dts: qcom: qrb5165-rb5: Switch on TCPM orientation-switch for
    usb_1_qmpphy
  usb: typec: qcom: Add Qualcomm PMIC TCPM support

Dmitry Baryshkov (1):
  phy: qcom-qmp: Register as a typec switch for orientation detection

 .../bindings/mfd/qcom,spmi-pmic.yaml          |   4 +
 .../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml      |  40 ++
 .../regulator/qcom,usb-vbus-regulator.yaml    |  10 +-
 .../bindings/usb/qcom,pmic-typec.yaml         | 169 ++++++
 MAINTAINERS                                   |  10 +
 arch/arm64/boot/dts/qcom/pm8150b.dtsi         |  40 ++
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      |  57 +-
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |  13 +
 drivers/phy/qualcomm/Kconfig                  |   8 +
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c     |  80 ++-
 drivers/usb/typec/Kconfig                     |  13 -
 drivers/usb/typec/Makefile                    |   1 -
 drivers/usb/typec/qcom-pmic-typec.c           | 261 --------
 drivers/usb/typec/tcpm/Kconfig                |  11 +
 drivers/usb/typec/tcpm/Makefile               |   1 +
 drivers/usb/typec/tcpm/qcom/Makefile          |   6 +
 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 362 +++++++++++
 .../typec/tcpm/qcom/qcom_pmic_typec_pdphy.c   | 528 +++++++++++++++++
 .../typec/tcpm/qcom/qcom_pmic_typec_pdphy.h   | 115 ++++
 .../typec/tcpm/qcom/qcom_pmic_typec_port.c    | 560 ++++++++++++++++++
 .../typec/tcpm/qcom/qcom_pmic_typec_port.h    | 194 ++++++
 21 files changed, 2202 insertions(+), 281 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
 delete mode 100644 drivers/usb/typec/qcom-pmic-typec.c
 create mode 100644 drivers/usb/typec/tcpm/qcom/Makefile
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.h
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_port.c
 create mode 100644 drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_port.h

Comments

Luca Weiss April 14, 2023, 6:51 a.m. UTC | #1
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
Marijn Suijten April 14, 2023, 7:27 a.m. UTC | #2
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
>
Krzysztof Kozlowski April 16, 2023, 5:43 p.m. UTC | #3
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
Krzysztof Kozlowski April 16, 2023, 5:49 p.m. UTC | #4
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
Bryan O'Donoghue April 17, 2023, 12:30 a.m. UTC | #5
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
Luca Weiss April 17, 2023, 7:35 a.m. UTC | #6
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
Bryan O'Donoghue April 17, 2023, 10:04 a.m. UTC | #7
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
Bryan O'Donoghue April 17, 2023, 10:11 a.m. UTC | #8
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 :)
Bryan O'Donoghue April 22, 2023, 10:16 p.m. UTC | #9
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