Message-ID: <CAA8EJpqpBfD4Do_y=BnvLq5BM97VYgkWm1Pd-P5yG+NVc7S7aQ@mail.gmail.com>
Date: Sun, 14 Apr 2024 03:47:20 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Luca Weiss <luca.weiss@...rphone.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, linux-usb@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2] dt-bindings: usb: add common Type-C USB Switch schema

On Sat, 13 Apr 2024 at 14:31, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 11/04/2024 09:35, Luca Weiss wrote:
> > On Thu Apr 11, 2024 at 9:25 AM CEST, Krzysztof Kozlowski wrote:
> >> On 11/04/2024 09:13, Luca Weiss wrote:
> >>> On Mon Jan 22, 2024 at 10:44 AM CET, Krzysztof Kozlowski wrote:
> >>>> Several bindings implement parts of Type-C USB orientation and mode
> >>>> switching, and retiming.  Keep definition of such properties in one
> >>>> place, new usb-switch schema, to avoid duplicate defines.
> >>>>
> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> >>>>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> 1. Fix language typos handle->handler (Luca)
> >>>> 2. Drop debugging left-over (Luca)
> >>>> ---
> >>>>  .../devicetree/bindings/usb/fcs,fsa4480.yaml  | 12 ++--
> >>>>  .../devicetree/bindings/usb/gpio-sbu-mux.yaml | 12 ++--
> >>>>  .../devicetree/bindings/usb/nxp,ptn36502.yaml | 12 ++--
> >>>>  .../bindings/usb/onnn,nb7vpq904m.yaml         | 13 ++--
> >>>>  .../bindings/usb/qcom,wcd939x-usbss.yaml      | 12 ++--
> >>>>  .../devicetree/bindings/usb/usb-switch.yaml   | 67 +++++++++++++++++++
> >>>>  6 files changed, 92 insertions(+), 36 deletions(-)
> >>>>  create mode 100644 Documentation/devicetree/bindings/usb/usb-switch.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> >>>> index f9410eb76a62..8b25b9a01ced 100644
> >>>> --- a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> >>>> +++ b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> >>>> @@ -27,13 +27,8 @@ properties:
> >>>>    vcc-supply:
> >>>>      description: power supply (2.7V-5.5V)
> >>>>
> >>>> -  mode-switch:
> >>>> -    description: Flag the port as possible handle of altmode switching
> >>>> -    type: boolean
> >>>> -
> >>>> -  orientation-switch:
> >>>> -    description: Flag the port as possible handler of orientation switching
> >>>> -    type: boolean
> >>>> +  mode-switch: true
> >>>> +  orientation-switch: true
> >>>>
> >>>>    port:
> >>>>      $ref: /schemas/graph.yaml#/$defs/port-base
> >>>> @@ -79,6 +74,9 @@ required:
> >>>>    - reg
> >>>>    - port
> >>>>
> >>>> +allOf:
> >>>> +  - $ref: usb-switch.yaml#
> >>>> +
> >>>>  additionalProperties: false
> >>>>
> >>>>  examples:
> >>>
> >>> Hi Krzysztof,
> >>>
> >>> This patch seems to break validation for fsa4480 if data-lanes is set in
> >>> the endpoint like the following
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> >>> index f9410eb76a62..3aa03fd65556 100644
> >>> --- a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> >>> @@ -102,6 +102,7 @@ examples:
> >>>            port {
> >>>              fsa4480_ept: endpoint {
> >>>                remote-endpoint = <&typec_controller>;
> >>> +              data-lanes = <0 1>;
> >>>              };
> >>>            };
> >>>          };
> >>>
> >>> Similar to how it's already used on qcom/qcm6490-fairphone-fp5.dts
> >>>
> >>> I'm guessing the 'port' definition in the common schema somehow
> >>> disallows the fsa4480 schema from describing it further?
> >>
> >> There is no such code in qcm6490-fairphone-fp5.dts. There was no such
> >> code in the example of fsa4480 when I was testing my changes (and
> >> examples should be complete), so this did not pop up.
> >
> > Right, I'm sorry this is just out-of-tree for now, I've forgotten this.
> > There's some dependency chain with some unsupported DSC configuration in
> > DPU for now that blocks upstreaming this.
> >
> > My tree with these patches is here if you want to take a look:
> > https://github.com/sc7280-mainline/linux/blob/sc7280-6.8.y/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts#L628
>
> I think fsa4480 schema is incomplete. I looked at HDK8450 board diagram
> and fsa4480 sits between and connects:

I'd second this

> 1. Type-c connector
> 2. USB phy or controller

USB SS PHY or controller

> 3. DisplayPort controller for altmode

Also:
4. Audio HP / Mic pins

>
> I think nxp,ptn36502 is done correctly. You need three ports and
> data-lanes would be on only one of them. usb-switch.yaml schema is ready
> for this and assumes data-lanes will be on (2) above.
>
> >
> >>
> >> You right, new schema does not allow extending the port. However the
> >> true question is, why muxing happens on the port to the SoC controller?
> >> The graph in commit msg fad89aa14 shows it happens on the side of the
> >> connector.
> >>
> >> Looks like fsa4480 mixes connector with the controller.
> >
> > Could be honestly.. I trust you with knowing better how the ports are
> > supposed to work.
> >
> > The property is for telling the fsa4480 driver that essentially the
> > hardware is wired up the reverse way. So with this info the driver can
> > handle the orientation switching correctly.
> >
> > There's another layer to this as explained in the patches there that the
> > OCP96011 essentially works reversed compared to FSA4480, that's why it's
> > all a bit of a mess.
>
> Maybe Bjorn, Dmitry or Neil have some more ideas how this should look
> like, but as of now to me it feels we should add "ports" property and
> move there to port@1 the data-lanes part of fsa schema.

It might be done if this benefits HW description. However I don't
think it is really possible to define the usb-switch schema that
covers all mux and retimer cases.
Let's cover major usecases:
- simple SBU mux, is connected only to the SBU / DP AUX lines.
  - gpio-sbu-mux misses the port connected to DP AUX

- SBU switch. It is connected to SBU lines and to two other ports
(e.g. DP AUX and USB4/PCIe SBU)
  - gpio-sbu-mux in some laptops, not supported yet

- Audio accessory mode MUX. Connects USB-C DP/DM and SBU lines either
to the USB 2.0 controller + DP AUX (or any other usecase) or to the
audio codec
  - FSA4480,
  - WCD939X

- Debug accessory mode MUX. Connects USB-C DP/DM, SS and SBU lines
either to the USB 2.0, USB+DP SS and DP AUX or to the JTAG port
  - Qualcomm EUD?

- USB/DP MUX: Connects USB-C SS and SBU lines either to the USB 3.0 or
to the DP and DP AUX
  - pi3usb30532, no bindings

- USB-C SS retimer. Is connected to USB-C SS lines and and to the SS
lines on the other side
  - no known examples

- USB-C SS + SBU retirmer. Is connected to USB-C SS + SBU lines and
and to the SS + SBU lines on the other side.
  - nb7vpq904m Second SBU port is missing in bindings
  - ptn36502 Second SBU port is missing in bindings

There might be some combinations of these mux/switch/retimers.

Hope this summary helps.

>
> Driver then should check whether there is port or ports and use
> ports->port@1 in the latter case.
>
>
> Best regards,
> Krzysztof
>


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists