| Message ID | 246c9d24da27b6ce91d5f1e536fa96ac5656a0b2.1633343547.git.schowdhu@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Embedded USB Debugger (EUD) driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Mon, Oct 04, 2021 at 04:46:19PM +0530, Souradeep Chowdhury wrote: > Added the property for EUD(Embedded USB Debug) connector.Added > the "reg" and "interrupts" property which is needed for EUD. You are going to need a better explanation of this h/w. > > Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org> > --- > .../devicetree/bindings/connector/usb-connector.yaml | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml > index 7eb8659..908129f 100644 > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml > @@ -30,6 +30,21 @@ properties: > - const: samsung,usb-connector-11pin > - const: usb-b-connector > > + - items: > + - enum: > + - qcom,sc7280-usb-connector-eud > + - const: qcom,usb-connector-eud > + - const: usb-c-connector > + > + reg: > + items: > + - description: EUD Base Register Region > + - description: EUD Mode Manager Region A connector node represents the physical connector on a board. That can't really be an MMIO peripheral. Maybe you need a node for EUD and then it should have a connector child node? Don't really know without understanding this h/w. > + > + interrupts: > + description: > + EUD interrupt > + > label: > description: Symbolic name for the connector. > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > >
On 2021-10-04 22:07, Rob Herring wrote: > On Mon, Oct 04, 2021 at 04:46:19PM +0530, Souradeep Chowdhury wrote: >> Added the property for EUD(Embedded USB Debug) connector.Added >> the "reg" and "interrupts" property which is needed for EUD. > > You are going to need a better explanation of this h/w. Ack. Will update this with the detailed hardware description in the next version. > >> >> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org> >> --- >> .../devicetree/bindings/connector/usb-connector.yaml | 15 >> +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git >> a/Documentation/devicetree/bindings/connector/usb-connector.yaml >> b/Documentation/devicetree/bindings/connector/usb-connector.yaml >> index 7eb8659..908129f 100644 >> --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml >> +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml >> @@ -30,6 +30,21 @@ properties: >> - const: samsung,usb-connector-11pin >> - const: usb-b-connector >> >> + - items: >> + - enum: >> + - qcom,sc7280-usb-connector-eud >> + - const: qcom,usb-connector-eud >> + - const: usb-c-connector >> + >> + reg: >> + items: >> + - description: EUD Base Register Region >> + - description: EUD Mode Manager Region > > A connector node represents the physical connector on a board. That > can't really be an MMIO peripheral. Maybe you need a node for EUD and > then it should have a connector child node? Don't really know without > understanding this h/w. As per the previous discussion on the EUD, it was agreed upon to map EUD as a type C connector and use Role-Switch to change the USB role instead of extcon interface that was being used previously. The link for the same is as follows:- https://lore.kernel.org/lkml/5db1a666-62ec-c850-6626-ad33d337b452@codeaurora.org/ > >> + >> + interrupts: >> + description: >> + EUD interrupt >> + >> label: >> description: Symbolic name for the connector. >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a >> member >> of Code Aurora Forum, hosted by The Linux Foundation >> >>
On Tue 05 Oct 06:11 PDT 2021, schowdhu@codeaurora.org wrote: > On 2021-10-04 22:07, Rob Herring wrote: > > On Mon, Oct 04, 2021 at 04:46:19PM +0530, Souradeep Chowdhury wrote: > > > Added the property for EUD(Embedded USB Debug) connector.Added > > > the "reg" and "interrupts" property which is needed for EUD. > > > > You are going to need a better explanation of this h/w. > > Ack. Will update this with the detailed hardware description > in the next version. > > > > > > > > > Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org> > > > --- > > > .../devicetree/bindings/connector/usb-connector.yaml | 15 > > > +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > b/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > index 7eb8659..908129f 100644 > > > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > @@ -30,6 +30,21 @@ properties: > > > - const: samsung,usb-connector-11pin > > > - const: usb-b-connector > > > > > > + - items: > > > + - enum: > > > + - qcom,sc7280-usb-connector-eud > > > + - const: qcom,usb-connector-eud > > > + - const: usb-c-connector > > > + > > > + reg: > > > + items: > > > + - description: EUD Base Register Region > > > + - description: EUD Mode Manager Region > > > > A connector node represents the physical connector on a board. That > > can't really be an MMIO peripheral. Maybe you need a node for EUD and > > then it should have a connector child node? Don't really know without > > understanding this h/w. > > As per the previous discussion on the EUD, it was agreed upon to map EUD > as a type C connector and use Role-Switch to change the USB role instead > of extcon interface that was being used previously. The link for the same > is as follows:- > > https://lore.kernel.org/lkml/5db1a666-62ec-c850-6626-ad33d337b452@codeaurora.org/ > Not using extcon is the right thing, but perhaps we should make the EUD a role_switch provider and client, so that we can describe how it sits inbetween the connector and the controller. That way it has the power to pass through or override requests from the upstream role-switcher, based on the status of EUD. That said, I'm still curious to what happens if I renegotiate the roles dynamically in a Type-C environment, while enabling EUD. How would the device on the other end of the cable know that it's supposed to be a host? Or there's simply a reset of the link when this happens? Thanks, Bjorn > > > > > + > > > + interrupts: > > > + description: > > > + EUD interrupt > > > + > > > label: > > > description: Symbolic name for the connector. > > > > > > -- > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a > > > member > > > of Code Aurora Forum, hosted by The Linux Foundation > > > > > >
On 2021-10-05 22:07, Bjorn Andersson wrote: > On Tue 05 Oct 06:11 PDT 2021, schowdhu@codeaurora.org wrote: > >> On 2021-10-04 22:07, Rob Herring wrote: >> > On Mon, Oct 04, 2021 at 04:46:19PM +0530, Souradeep Chowdhury wrote: >> > > Added the property for EUD(Embedded USB Debug) connector.Added >> > > the "reg" and "interrupts" property which is needed for EUD. >> > >> > You are going to need a better explanation of this h/w. >> >> Ack. Will update this with the detailed hardware description >> in the next version. >> >> > >> > > >> > > Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org> >> > > --- >> > > .../devicetree/bindings/connector/usb-connector.yaml | 15 >> > > +++++++++++++++ >> > > 1 file changed, 15 insertions(+) >> > > >> > > diff --git >> > > a/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > b/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > index 7eb8659..908129f 100644 >> > > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > @@ -30,6 +30,21 @@ properties: >> > > - const: samsung,usb-connector-11pin >> > > - const: usb-b-connector >> > > >> > > + - items: >> > > + - enum: >> > > + - qcom,sc7280-usb-connector-eud >> > > + - const: qcom,usb-connector-eud >> > > + - const: usb-c-connector >> > > + >> > > + reg: >> > > + items: >> > > + - description: EUD Base Register Region >> > > + - description: EUD Mode Manager Region >> > >> > A connector node represents the physical connector on a board. That >> > can't really be an MMIO peripheral. Maybe you need a node for EUD and >> > then it should have a connector child node? Don't really know without >> > understanding this h/w. >> >> As per the previous discussion on the EUD, it was agreed upon to map >> EUD >> as a type C connector and use Role-Switch to change the USB role >> instead >> of extcon interface that was being used previously. The link for the >> same >> is as follows:- >> >> https://lore.kernel.org/lkml/5db1a666-62ec-c850-6626-ad33d337b452@codeaurora.org/ >> > > Not using extcon is the right thing, but perhaps we should make the EUD > a role_switch provider and client, so that we can describe how it sits > inbetween the connector and the controller. > > That way it has the power to pass through or override requests from the > upstream role-switcher, based on the status of EUD. > > > That said, I'm still curious to what happens if I renegotiate the roles > dynamically in a Type-C environment, while enabling EUD. How would the > device on the other end of the cable know that it's supposed to be a > host? Or there's simply a reset of the link when this happens? > > Thanks, > Bjorn Hi Bjorn, By making EUD Role-Switch provider and client do you mean that we should have a EUD node which will have a connector node as child and this connector node will have a port that points towards the drd role-switch? So that my device tree node will look like the following in that case eud@88e0000 { compatible = "qcom,usb-connector-eud"; reg = <0 0x88e0000 0 0x2000>, <0 0x88e2000 0 0x1000>; interrupt-parent = <&pdc>; interrupts = <11 IRQ_TYPE_LEVEL_HIGH>; usb_con: connector { compatible = "usb-c-connector"; label = "USB-C"; port { eud_usb_output: endpoint { remote-endpoint = <&eud_usb3_input>; }; }; }; @usb2 { dwc3 { usb-role-switch; port { eud_usb3_input: endpoint { remote-endpoint = <&eud_usb_output>; }; }; }; Also EUD functions only in device mode, so when the role-switch is done by the controller to set the device mode, the PC on the other end becomes the host. Thanks, Souradeep > >> > >> > > + >> > > + interrupts: >> > > + description: >> > > + EUD interrupt >> > > + >> > > label: >> > > description: Symbolic name for the connector. >> > > >> > > -- >> > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a >> > > member >> > > of Code Aurora Forum, hosted by The Linux Foundation >> > > >> > >
On Thu 07 Oct 04:25 CDT 2021, schowdhu@codeaurora.org wrote: > On 2021-10-05 22:07, Bjorn Andersson wrote: > > On Tue 05 Oct 06:11 PDT 2021, schowdhu@codeaurora.org wrote: > > > > > On 2021-10-04 22:07, Rob Herring wrote: > > > > On Mon, Oct 04, 2021 at 04:46:19PM +0530, Souradeep Chowdhury wrote: > > > > > Added the property for EUD(Embedded USB Debug) connector.Added > > > > > the "reg" and "interrupts" property which is needed for EUD. > > > > > > > > You are going to need a better explanation of this h/w. > > > > > > Ack. Will update this with the detailed hardware description > > > in the next version. > > > > > > > > > > > > > > > > > Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org> > > > > > --- > > > > > .../devicetree/bindings/connector/usb-connector.yaml | 15 > > > > > +++++++++++++++ > > > > > 1 file changed, 15 insertions(+) > > > > > > > > > > diff --git > > > > > a/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > > > b/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > > > index 7eb8659..908129f 100644 > > > > > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml > > > > > @@ -30,6 +30,21 @@ properties: > > > > > - const: samsung,usb-connector-11pin > > > > > - const: usb-b-connector > > > > > > > > > > + - items: > > > > > + - enum: > > > > > + - qcom,sc7280-usb-connector-eud > > > > > + - const: qcom,usb-connector-eud > > > > > + - const: usb-c-connector > > > > > + > > > > > + reg: > > > > > + items: > > > > > + - description: EUD Base Register Region > > > > > + - description: EUD Mode Manager Region > > > > > > > > A connector node represents the physical connector on a board. That > > > > can't really be an MMIO peripheral. Maybe you need a node for EUD and > > > > then it should have a connector child node? Don't really know without > > > > understanding this h/w. > > > > > > As per the previous discussion on the EUD, it was agreed upon to map > > > EUD > > > as a type C connector and use Role-Switch to change the USB role > > > instead > > > of extcon interface that was being used previously. The link for the > > > same > > > is as follows:- > > > > > > https://lore.kernel.org/lkml/5db1a666-62ec-c850-6626-ad33d337b452@codeaurora.org/ > > > > > > > Not using extcon is the right thing, but perhaps we should make the EUD > > a role_switch provider and client, so that we can describe how it sits > > inbetween the connector and the controller. > > > > That way it has the power to pass through or override requests from the > > upstream role-switcher, based on the status of EUD. > > > > > > That said, I'm still curious to what happens if I renegotiate the roles > > dynamically in a Type-C environment, while enabling EUD. How would the > > device on the other end of the cable know that it's supposed to be a > > host? Or there's simply a reset of the link when this happens? > > > > Thanks, > > Bjorn > > Hi Bjorn, > Hi Souradeep > By making EUD Role-Switch provider and client do you mean that > we should have a EUD node which will have a connector node as > child and this connector node will have a port that points towards > the drd role-switch? > > So that my device tree node will look like the following in that case > > eud@88e0000 { > compatible = "qcom,usb-connector-eud"; > reg = <0 0x88e0000 0 0x2000>, > <0 0x88e2000 0 0x1000>; > interrupt-parent = <&pdc>; > interrupts = <11 IRQ_TYPE_LEVEL_HIGH>; > usb_con: connector { > compatible = "usb-c-connector"; > label = "USB-C"; > port { > eud_usb_output: endpoint { > remote-endpoint = <&eud_usb3_input>; > }; > }; > > }; > > > @usb2 { > dwc3 { > usb-role-switch; > port { > eud_usb3_input: endpoint { > remote-endpoint = <&eud_usb_output>; > }; > }; > }; While your "output" and "input" matches the direction of the role switching, I think the connection should be describe in the other direction. Also my suggestion was that EUD is both connector for the dwc3 and has a reference to the connector described in the TypeC controller - to properly describe the relationship: DWC -> EUD -> connector With the role switching request going from the connector (pmic_glink driver) to DWC through the EUD, which can override the vote. That said, this is just my suggestion. You need to ensure that Rob understands the hardware design well enough to approve your proposed binding. E.g. The connector in the EUD isn't a usb-c-connector, it's some type of internal connection, the next step in that chain is the actual usb-c-connector. Regards, Bjorn > > Also EUD functions only in device mode, so when the role-switch is done by > the controller > to set the device mode, the PC on the other end becomes the host. > > Thanks, > Souradeep > > > > > > > > > > > > + > > > > > + interrupts: > > > > > + description: > > > > > + EUD interrupt > > > > > + > > > > > label: > > > > > description: Symbolic name for the connector. > > > > > > > > > > -- > > > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a > > > > > member > > > > > of Code Aurora Forum, hosted by The Linux Foundation > > > > > > > > > >
On 2021-10-12 08:52, Bjorn Andersson wrote: > On Thu 07 Oct 04:25 CDT 2021, schowdhu@codeaurora.org wrote: > >> On 2021-10-05 22:07, Bjorn Andersson wrote: >> > On Tue 05 Oct 06:11 PDT 2021, schowdhu@codeaurora.org wrote: >> > >> > > On 2021-10-04 22:07, Rob Herring wrote: >> > > > On Mon, Oct 04, 2021 at 04:46:19PM +0530, Souradeep Chowdhury wrote: >> > > > > Added the property for EUD(Embedded USB Debug) connector.Added >> > > > > the "reg" and "interrupts" property which is needed for EUD. >> > > > >> > > > You are going to need a better explanation of this h/w. >> > > >> > > Ack. Will update this with the detailed hardware description >> > > in the next version. >> > > >> > > > >> > > > > >> > > > > Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org> >> > > > > --- >> > > > > .../devicetree/bindings/connector/usb-connector.yaml | 15 >> > > > > +++++++++++++++ >> > > > > 1 file changed, 15 insertions(+) >> > > > > >> > > > > diff --git >> > > > > a/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > > > b/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > > > index 7eb8659..908129f 100644 >> > > > > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml >> > > > > @@ -30,6 +30,21 @@ properties: >> > > > > - const: samsung,usb-connector-11pin >> > > > > - const: usb-b-connector >> > > > > >> > > > > + - items: >> > > > > + - enum: >> > > > > + - qcom,sc7280-usb-connector-eud >> > > > > + - const: qcom,usb-connector-eud >> > > > > + - const: usb-c-connector >> > > > > + >> > > > > + reg: >> > > > > + items: >> > > > > + - description: EUD Base Register Region >> > > > > + - description: EUD Mode Manager Region >> > > > >> > > > A connector node represents the physical connector on a board. That >> > > > can't really be an MMIO peripheral. Maybe you need a node for EUD and >> > > > then it should have a connector child node? Don't really know without >> > > > understanding this h/w. >> > > >> > > As per the previous discussion on the EUD, it was agreed upon to map >> > > EUD >> > > as a type C connector and use Role-Switch to change the USB role >> > > instead >> > > of extcon interface that was being used previously. The link for the >> > > same >> > > is as follows:- >> > > >> > > https://lore.kernel.org/lkml/5db1a666-62ec-c850-6626-ad33d337b452@codeaurora.org/ >> > > >> > >> > Not using extcon is the right thing, but perhaps we should make the EUD >> > a role_switch provider and client, so that we can describe how it sits >> > inbetween the connector and the controller. >> > >> > That way it has the power to pass through or override requests from the >> > upstream role-switcher, based on the status of EUD. >> > >> > >> > That said, I'm still curious to what happens if I renegotiate the roles >> > dynamically in a Type-C environment, while enabling EUD. How would the >> > device on the other end of the cable know that it's supposed to be a >> > host? Or there's simply a reset of the link when this happens? >> > >> > Thanks, >> > Bjorn >> >> Hi Bjorn, >> > > Hi Souradeep > >> By making EUD Role-Switch provider and client do you mean that >> we should have a EUD node which will have a connector node as >> child and this connector node will have a port that points towards >> the drd role-switch? >> >> So that my device tree node will look like the following in that case >> >> eud@88e0000 { >> compatible = "qcom,usb-connector-eud"; >> reg = <0 0x88e0000 0 0x2000>, >> <0 0x88e2000 0 0x1000>; >> interrupt-parent = <&pdc>; >> interrupts = <11 IRQ_TYPE_LEVEL_HIGH>; >> usb_con: connector { >> compatible = "usb-c-connector"; >> label = "USB-C"; >> port { >> eud_usb_output: endpoint { >> remote-endpoint = <&eud_usb3_input>; >> }; >> }; >> >> }; >> >> >> @usb2 { >> dwc3 { >> usb-role-switch; >> port { >> eud_usb3_input: endpoint { >> remote-endpoint = <&eud_usb_output>; >> }; >> }; >> }; > > While your "output" and "input" matches the direction of the role > switching, I think the connection should be describe in the other > direction. > > Also my suggestion was that EUD is both connector for the dwc3 and has > a > reference to the connector described in the TypeC controller - to > properly describe the relationship: > > DWC -> EUD -> connector > > With the role switching request going from the connector (pmic_glink > driver) to DWC through the EUD, which can override the vote. > > > That said, this is just my suggestion. You need to ensure that Rob > understands the hardware design well enough to approve your proposed > binding. > > > E.g. The connector in the EUD isn't a usb-c-connector, it's some > type of internal connection, the next step in that chain is the actual > usb-c-connector. > > Regards, > Bjorn > >> >> Also EUD functions only in device mode, so when the role-switch is >> done by >> the controller >> to set the device mode, the PC on the other end becomes the host. >> >> Thanks, >> Souradeep Hi Bjorn/Robb, Following is how my EUD design looks like My device tree node will look like as follows eud@88e0000 { compatible = "qcom,usb-connector-eud"; reg = <0 0x88e0000 0 0x2000>, <0 0x88e2000 0 0x1000>; interrupt-parent = <&pdc>; interrupts = <11 IRQ_TYPE_LEVEL_HIGH>; ports { #address-cells = <2>; #size-cells = <2>; port@0 { reg = <0>; usb2_eud: endpoint { remote-endpoint = <&eud_ep>; }; }; }; }; @usb2 { dwc3 { usb-role-switch; usb_con: eud_usb_connector { compatible = “usb-connector-eud", "usb-c-connector"; ports { #address-cells = <2>; #size-cells = <2>; port@0 { reg = <0>; eud_ep: endpoint { remote-endpoint = <&usb2_eud>; }; }; }; }; So, I am putting a connector node as a child of dwc3 node which can be connected to the EUD node via ports. I am keeping EUD as a separate device tree node. I can get the device equivalent to the connector by doing the following in EUD driver dev = usb_of_get_companion_dev(&pdev->dev); Once I get the device, I can then do role_sw = usb_role_switch_get(dev); This will give me the role switch reference which can be used to set device or host role. The advantage here is that -> I am keeping the existing yamls untouched which won't be the case if I map EUD as child of dwc3. ->This is also consistent with EUD hardware since EUD is not a connector in itself but it needs a connector to function because it can work only in device mode so the role-switch needs to be done. Let me know your thoughts regarding this. Thanks, Souradeep >> >> > >> > > > >> > > > > + >> > > > > + interrupts: >> > > > > + description: >> > > > > + EUD interrupt >> > > > > + >> > > > > label: >> > > > > description: Symbolic name for the connector. >> > > > > >> > > > > -- >> > > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a >> > > > > member >> > > > > of Code Aurora Forum, hosted by The Linux Foundation >> > > > > >> > > > >
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml index 7eb8659..908129f 100644 --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml @@ -30,6 +30,21 @@ properties: - const: samsung,usb-connector-11pin - const: usb-b-connector + - items: + - enum: + - qcom,sc7280-usb-connector-eud + - const: qcom,usb-connector-eud + - const: usb-c-connector + + reg: + items: + - description: EUD Base Register Region + - description: EUD Mode Manager Region + + interrupts: + description: + EUD interrupt + label: description: Symbolic name for the connector.
Added the property for EUD(Embedded USB Debug) connector.Added the "reg" and "interrupts" property which is needed for EUD. Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org> --- .../devicetree/bindings/connector/usb-connector.yaml | 15 +++++++++++++++ 1 file changed, 15 insertions(+)