| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200609235740.GA154315@google.com>
Date: Tue, 9 Jun 2020 16:57:40 -0700
From: Prashant Malani <pmalani@...omium.org>
To: Rob Herring <robh@...nel.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Tim Wawrzynczak <twawrzynczak@...omium.org>,
Benson Leung <bleung@...omium.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props
Hi Rob,
Thanks again for the comments and feedback. Kindly see responses inline:
(Trimming unrelated text from thread):
On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:
> On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@...omium.org> wrote:
> >
> > Nodes truncated and unrelated fields omitted in the interest of brevity:
> >
> > // Chrome OS EC Type C Port Manager.
> > typec {
> > compatible = "google,cros-ec-typec";
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > connector@0 {
> > compatible = "usb-c-connector";
> > reg = <0>;
> > power-role = "dual";
> > data-role = "dual";
> > try-power-role = "source";
> > mode-switch = <&foo_mux>;
> > // Other switches can point to the same mux.
> > ....
>
> The connector is supposed to have 'ports' for USB2, USB3, and Aux
> unless the parent is the USB controller.
Understood; so, coupled with Heikki's explanation (see below for where
I've pasted it), would it be something like so? (adding inline to the connector
node definition):
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
usb_con_hs: endpoint {
remote-endpoint = <&foo_usb_hs_controller>;
};
};
port@1 {
reg = <1>;
usb_con0_ss: endpoint@0 {
remote-endpoint = <&mode_mux_in>;
};
};
port@2 {
reg = <2>;
usb_con_sbu: endpoint {
remote-endpoint = <&foo_dp_aux>;
};
};
};
>
> > };
> > };
> >
> > // Mux switch
> > // TODO: Can possibly embed this in the PHY controller node itself?
> > foo_mux {
> > compatible = "vendor,typec-mux";
> > mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > port@0 {
> > reg = <0>;
> > mux_dp_in: endpoint {
> > remote-endpoint = <&dp_phy_out>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > mux_usb_in: endpoint1 {
> > remote-endpoint = <&usb3_phy_out>;
> > };
> > };
>
> This all goes away if you have ports in the connector node. More below.
I think I my earlier example may have been a bit incorrect. Per Heikki's
explanation of who the consumer of the mux-control bindings is, the
foo_mux definition would now be like so:
foo_mux {
compatible = "typec-mux-consumer";
// This can be expanded to add more mux-controls for orientation,
// data role etc.
mux-controls = <&mode_mux_controller>;
mux-control-names = "mode";
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
mode_mux_in: endpoint {
remote-endpoint = <&usb_con0_ss>
};
};
port@1 {
reg = <1>;
mode_mux_out_usb3: endpoint {
remote-endpoint = <&usb3_0_ep>
};
};
port@2 {
reg = <2>;
mode_mux_out_dp: endpoint {
remote-endpoint = <&dp0_out_ep>
};
};
};
and we add the definition for the mux-controller, where the mux-gpio
control actually resides:
mode_mux_controller: mux-controller {
compatible = "vendor,typec-mode-mux";
mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
};
>
> > };
> > };
> >
> > // Type C PHY Controller.
> > tcphy0: phy@...c0000 {
> > compatible = "rockchip,rk3399-typec-phy";
> > reg = <0x0 0xff7c0000 0x0 0x40000>;
> > ...
> > tcphy0_dp: phy@...0000 {
> > compatible = "soc,dp-phy";
> > reg = <0xdc00000 0x1000>;
> > ports {
> > port@0 {
> > reg = <0>;
> > dp_phy_out: endpoint {
> > remote-endpoint = <&mux_dp_in>;
> > };
> > };
>
> This is wrong in that 'phys' are not part of the graph. It's the DP
> and USB controllers that should be part of the graph. Any phys are
> referenced with the phys binding and are not part of the graph.
Got it. So the new PHY definition would be just the same, but no
"ports".
>
> > };
> > };
> >
> > tcphy0_usb3: phy@...0000 {
> > compatible = "soc,usb3-phy";
> > reg = <0xdb00000 0x1000>;
> > ports {
> > port@0 {
> > reg = <0>;
> > usb3_phy_out: endpoint {
> > remote-endpoint = <&mux_usb3_in>;
> > };
> > };
> > };
> > };
> > };
> >
> >
> > // USB3 Host controller
> > usbdrd3_0: usb@...00000 {
> > compatible = "rockchip,rk3399-dwc3";
> > #address-cells = <2>;
> > #size-cells = <2>;
> > clocks = ...;
> > clock-names = ...;
> > status = "disabled";
> >
> > usbdrd_dwc3_0: usb@...00000 {
> > compatible = "snps,dwc3";
> > reg = <0x0 0xfe800000 0x0 0x100000>;
> > interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH 0>;
> > clocks = ...;
> > clock-names = ...;
> > dr_mode = "otg";
> > phys = <&tcphy0_usb3>;
> > phy-names = "usb3-phy";
> > phy_type = "utmi_wide";
> > power-domains = <&power RK3399_PD_USB3>;
> > status = "disabled";
This means a port definition here:
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
usb3_0_ep: endpoint {
remote-endpoint = <&mode_mux_out_usb3>;
};
};
};
> > };
> > };
> >
> > // DP controller
> > cdn_dp: dp@...00000 {
> > compatible = "rockchip,rk3399-cdn-dp";
> > reg = <0x0 0xfec00000 0x0 0x100000>;
> > interrupts = ...;
> > clocks = ...;
> > clock-names = ...;
> > phys = <&tcphy0_dp>;
> > ...
> > ports {
> > dp_in: port {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > dp_in_vopb: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&vopb_out_dp>;
> > };
> >
> > dp_in_vopl: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&vopl_out_dp>;
> > };
> > };
>
> This should have an output port and then that is connected to the USB
> connector. Given that DP is muxed with the USB SS signals, port@1
> (defined as USB SS) should then have 2 endpoints.
So, completing the example, another port here:
dp_out: port@1 {
reg = <1>;
dp0_out_ep: endpoint {
remote-endpoint = <&mode_mux_out_dp>;
};
};
>
> Then the only new thing here is how to represent the GPIO line
> controlling the mux. Normally, I'd expect this to be in the parent of
> the connector (the type-C controller), but since you have multiple
> connectors, that doesn't work so well. So you can put 'mux-gpios' in
> the port@1 node. (Or maybe it should be 'mux-controls' with a gpio mux
> defined elsewhere).
I think the updated example better helps delineate what we're trying to
achieve. Per Heikki's earlier explanation (re-quoting here) :
"
> > > > The mode-switch here would actually represent the "consumer" part in
> > > > the mux-control bindings. So the mux-controls would describe the
> > > > relationship between the "mode-switch" and the mux controller(s),
> > > > while the mode-switch property describes the relationship between
> > > > something like USB Type-C Port Manager (or this cros_ec function) and
> > > > the "mux consumer".
"
So the device that "mode-switch" points to (in this case, foo_mux) is the "consumer" of
the mux-control direct consumer). Heikki, does this example better
demonstrate what the Type C connector class expects?
Also, I think it might be better to move these *-switch properties into the
usb-connector.yaml binding in the event that we decide to take this route,
since the "Chrome EC Type C port driver" isn't expected to use them directly.
Rob, kindly LMK if you'd prefer that, and I can make the change in the
next patch version.
I can re-write this example in a non-inline form if that would be easier
to interpret. Kindly LMK, and also LMK if this graph connection scheme
example is acceptable.
Thanks again, and best regards,
-Prashant
>
> Rob
Powered by blists - more mailing lists