[PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Elson Roy Serrao]

The Embedded USB Debugger (EUD) is a mini High-Speed USB on-chip hub to
support the USB-based debug and trace capabilities on Qualcomm devices.
The current implementation lacks in below aspects that are needed for
proper EUD functionality.

1.) HS-Phy control: EUD being a HS hub needs HS-Phy support for it's
    operation. Hence EUD module should enable/disable HS-phy
    accordingly.
	
2.) Proper routing of USB role switch notifications: EUD hub is physically
    present in between the USB connector and the USB controller. So the
    usb role switch notifications originating from the connector should
    route through EUD. EUD also relies on role switch notifications to
    communicate with the USB, regarding EUD attach/detach events.

This series aims at implementing the above aspects to enable EUD on
Qualcomm sm8450 SoC.

Elson Roy Serrao (8):
  dt-bindings: soc: qcom: eud: Add phy related bindings
  dt-bindings: soc: qcom: eud: Add usb role switch property
  dt-bindings: soc: qcom: eud: Add compatible for sm8450
  arm64: dts: qcom: sm8450: Add EUD node
  arm64: dts: qcom: Enable EUD on sm8450 hdk
  usb: misc: eud: Add High-Speed Phy control for EUD operations
  usb: misc: eud: Handle usb role switch notifications
  usb: misc: eud: Add compatible for sm8450

 .../bindings/soc/qcom/qcom,eud.yaml           |  17 +++
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts       |  15 ++-
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |  29 ++++
 drivers/usb/misc/qcom_eud.c                   | 125 +++++++++++++++---
 4 files changed, 164 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH 1/8] dt-bindings: soc: qcom: eud: Add phy related bindings

[Elson Roy Serrao]

Embedded USB Debugger(EUD) being a High-Speed USB  hub needs
HS-Phy support for it's operation. Hence document phy bindings
to support this.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 .../devicetree/bindings/soc/qcom/qcom,eud.yaml       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
index f2c5ec7e6437..fca5b608ec63 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
@@ -29,6 +29,14 @@ properties:
     description: EUD interrupt
     maxItems: 1
 
+  phys:
+    items:
+      - description: USB2/HS PHY needed for EUD functionality
+
+  phy-names:
+    items:
+      - const: usb2-phy
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
     description:
@@ -48,6 +56,8 @@ properties:
 required:
   - compatible
   - reg
+  - phys
+  - phy-names
   - ports
 
 additionalProperties: false
@@ -58,6 +68,8 @@ examples:
            compatible = "qcom,sc7280-eud", "qcom,eud";
            reg = <0x88e0000 0x2000>,
                  <0x88e2000 0x1000>;
+           phys = <&usb_2_hsphy>;
+           phy-names = "usb2-phy";
 
            ports {
                    #address-cells = <1>;
-- 
2.17.1

Re: [PATCH 1/8] dt-bindings: soc: qcom: eud: Add phy related bindings

[Krzysztof Kozlowski]

On 31/07/2024 00:24, Elson Roy Serrao wrote:
> Embedded USB Debugger(EUD) being a High-Speed USB  hub needs
> HS-Phy support for it's operation. Hence document phy bindings
> to support this.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> ---
>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> index f2c5ec7e6437..fca5b608ec63 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> @@ -29,6 +29,14 @@ properties:
>      description: EUD interrupt
>      maxItems: 1
>  
> +  phys:
> +    items:
> +      - description: USB2/HS PHY needed for EUD functionality
> +
> +  phy-names:
> +    items:
> +      - const: usb2-phy
> +
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>      description:
> @@ -48,6 +56,8 @@ properties:
>  required:
>    - compatible
>    - reg
> +  - phys
> +  - phy-names

That's an ABI break and nothing in commit msg justified it.

Best regards,
Krzysztof

Re: [PATCH 1/8] dt-bindings: soc: qcom: eud: Add phy related bindings

[Elson Serrao]

On 7/30/2024 10:33 PM, Krzysztof Kozlowski wrote:
> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>> Embedded USB Debugger(EUD) being a High-Speed USB  hub needs
>> HS-Phy support for it's operation. Hence document phy bindings
>> to support this.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
Ack
>> ---
>>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml       | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>> index f2c5ec7e6437..fca5b608ec63 100644
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>> @@ -29,6 +29,14 @@ properties:
>>      description: EUD interrupt
>>      maxItems: 1
>>  
>> +  phys:
>> +    items:
>> +      - description: USB2/HS PHY needed for EUD functionality
>> +
>> +  phy-names:
>> +    items:
>> +      - const: usb2-phy
>> +
>>    ports:
>>      $ref: /schemas/graph.yaml#/properties/ports
>>      description:
>> @@ -48,6 +56,8 @@ properties:
>>  required:
>>    - compatible
>>    - reg
>> +  - phys
>> +  - phy-names
> 
> That's an ABI break and nothing in commit msg justified it.
> 

Hi Krzysztof

Thank you for the review.
I see that the only user for EUD as of now is QC sc7280 SoC where phy property
is missing and EUD node is disabled. As described in my cover letter, HS phy
support is needed for EUD functionality and this is applicable to all SoCs
where EUD is to be enabled. Hence phy would be a required property.
Given that the changes in this series are directly applicable to sc7280 as well,
I will re-enable/rectify EUD feature on sc7280 SoC first, by adhering it to this binding
requirement. That would address the ABI break. 
Once the base framework is set I shall extend it to sm8450 SoC.

Regards
Elson 

Re: [PATCH 1/8] dt-bindings: soc: qcom: eud: Add phy related bindings

[Krzysztof Kozlowski]

On 01/08/2024 00:23, Elson Serrao wrote:
> 
> 
> On 7/30/2024 10:33 PM, Krzysztof Kozlowski wrote:
>> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>>> Embedded USB Debugger(EUD) being a High-Speed USB  hub needs
>>> HS-Phy support for it's operation. Hence document phy bindings
>>> to support this.
>>>
>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
> Ack
>>> ---
>>>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml       | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> index f2c5ec7e6437..fca5b608ec63 100644
>>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> @@ -29,6 +29,14 @@ properties:
>>>      description: EUD interrupt
>>>      maxItems: 1
>>>  
>>> +  phys:
>>> +    items:
>>> +      - description: USB2/HS PHY needed for EUD functionality
>>> +
>>> +  phy-names:
>>> +    items:
>>> +      - const: usb2-phy
>>> +
>>>    ports:
>>>      $ref: /schemas/graph.yaml#/properties/ports
>>>      description:
>>> @@ -48,6 +56,8 @@ properties:
>>>  required:
>>>    - compatible
>>>    - reg
>>> +  - phys
>>> +  - phy-names
>>
>> That's an ABI break and nothing in commit msg justified it.
>>
> 
> Hi Krzysztof
> 
> Thank you for the review.
> I see that the only user for EUD as of now is QC sc7280 SoC where phy property

Did you ask all customers and all users of Linux kernel?

> is missing and EUD node is disabled. As described in my cover letter, HS phy
> support is needed for EUD functionality and this is applicable to all SoCs
> where EUD is to be enabled. Hence phy would be a required property.

Nothing in commit msg explained that, but I have a bit hard time to
believe that this never worked. If that's the case, say it explicitly in
commit msg - this was always broken.

> Given that the changes in this series are directly applicable to sc7280 as well,
> I will re-enable/rectify EUD feature on sc7280 SoC first, by adhering it to this binding
> requirement. That would address the ABI break. 

I don't understand what you are saying here.

> Once the base framework is set I shall extend it to sm8450 SoC.


Best regards,
Krzysztof

Re: [PATCH 1/8] dt-bindings: soc: qcom: eud: Add phy related bindings

[Konrad Dybcio]

On 1.08.2024 9:45 AM, Krzysztof Kozlowski wrote:
> On 01/08/2024 00:23, Elson Serrao wrote:
>>
>>
>> On 7/30/2024 10:33 PM, Krzysztof Kozlowski wrote:
>>> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>>>> Embedded USB Debugger(EUD) being a High-Speed USB  hub needs
>>>> HS-Phy support for it's operation. Hence document phy bindings
>>>> to support this.
>>>>
>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>
>>> A nit, subject: drop second/last, redundant "bindings". The
>>> "dt-bindings" prefix is already stating that these are bindings.
>>> See also:
>>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>>
>> Ack
>>>> ---
>>>>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml       | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>> index f2c5ec7e6437..fca5b608ec63 100644
>>>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>> @@ -29,6 +29,14 @@ properties:
>>>>      description: EUD interrupt
>>>>      maxItems: 1
>>>>  
>>>> +  phys:
>>>> +    items:
>>>> +      - description: USB2/HS PHY needed for EUD functionality
>>>> +
>>>> +  phy-names:
>>>> +    items:
>>>> +      - const: usb2-phy
>>>> +
>>>>    ports:
>>>>      $ref: /schemas/graph.yaml#/properties/ports
>>>>      description:
>>>> @@ -48,6 +56,8 @@ properties:
>>>>  required:
>>>>    - compatible
>>>>    - reg
>>>> +  - phys
>>>> +  - phy-names
>>>
>>> That's an ABI break and nothing in commit msg justified it.
>>>
>>
>> Hi Krzysztof
>>
>> Thank you for the review.
>> I see that the only user for EUD as of now is QC sc7280 SoC where phy property
> 
> Did you ask all customers and all users of Linux kernel?

Unfortunately, the PDF agrees - the current description is inherently incomplete
and the driver seems to have been upstreamed in a rather "i need this specific
part of it for my usecase" manner..

The driver must be aware of all USB state changes (as EUD is essentially a mux+hub
sitting between the PHYs and the USB controllers).

Additionally, AFAICU, all device-mode-capable USB ports may potentially be used
for debug purposes (one at a time), so it's not just a matter of a single
controller here. Plug events / their suspend state must be monitored to program
the EUD (which again, sits in the middle of all this) in a specific manner.

EUD is present on all non-ancient SoCs and by default it's on in bypass mode, so
you can ignore its existence. That is, unless you want to use the features it
provides, which we absolutely do.

>> is missing and EUD node is disabled. As described in my cover letter, HS phy
>> support is needed for EUD functionality and this is applicable to all SoCs
>> where EUD is to be enabled. Hence phy would be a required property.
> 
> Nothing in commit msg explained that, but I have a bit hard time to
> believe that this never worked. If that's the case, say it explicitly in
> commit msg - this was always broken.

Even if it does work, it does so on a specific class of boards, relying on
specific setup from a previous stage bootloader.

>> Given that the changes in this series are directly applicable to sc7280 as well,
>> I will re-enable/rectify EUD feature on sc7280 SoC first, by adhering it to this binding
>> requirement. That would address the ABI break. 
> 
> I don't understand what you are saying here.
> 
>> Once the base framework is set I shall extend it to sm8450 SoC.

tldr, we should fix both the bindings and the 7280 dt for it

Konrad

Re: [PATCH 1/8] dt-bindings: soc: qcom: eud: Add phy related bindings

[Konrad Dybcio]

On 27.01.2025 3:40 PM, Konrad Dybcio wrote:
> On 1.08.2024 9:45 AM, Krzysztof Kozlowski wrote:
>> On 01/08/2024 00:23, Elson Serrao wrote:
>>>
>>>
>>> On 7/30/2024 10:33 PM, Krzysztof Kozlowski wrote:
>>>> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>>>>> Embedded USB Debugger(EUD) being a High-Speed USB  hub needs
>>>>> HS-Phy support for it's operation. Hence document phy bindings
>>>>> to support this.
>>>>>
>>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>>
>>>> A nit, subject: drop second/last, redundant "bindings". The
>>>> "dt-bindings" prefix is already stating that these are bindings.
>>>> See also:
>>>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>>>
>>> Ack
>>>>> ---
>>>>>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml       | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>>> index f2c5ec7e6437..fca5b608ec63 100644
>>>>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>>> @@ -29,6 +29,14 @@ properties:
>>>>>      description: EUD interrupt
>>>>>      maxItems: 1
>>>>>  
>>>>> +  phys:
>>>>> +    items:
>>>>> +      - description: USB2/HS PHY needed for EUD functionality
>>>>> +
>>>>> +  phy-names:
>>>>> +    items:
>>>>> +      - const: usb2-phy
>>>>> +
>>>>>    ports:
>>>>>      $ref: /schemas/graph.yaml#/properties/ports
>>>>>      description:
>>>>> @@ -48,6 +56,8 @@ properties:
>>>>>  required:
>>>>>    - compatible
>>>>>    - reg
>>>>> +  - phys
>>>>> +  - phy-names
>>>>
>>>> That's an ABI break and nothing in commit msg justified it.
>>>>
>>>
>>> Hi Krzysztof
>>>
>>> Thank you for the review.
>>> I see that the only user for EUD as of now is QC sc7280 SoC where phy property
>>
>> Did you ask all customers and all users of Linux kernel?
> 
> Unfortunately, the PDF agrees - the current description is inherently incomplete
> and the driver seems to have been upstreamed in a rather "i need this specific
> part of it for my usecase" manner..
> 
> The driver must be aware of all USB state changes (as EUD is essentially a mux+hub
> sitting between the PHYs and the USB controllers).
> 
> Additionally, AFAICU, all device-mode-capable USB ports may potentially be used
> for debug purposes (one at a time), so it's not just a matter of a single
> controller here. Plug events / their suspend state must be monitored to program
> the EUD (which again, sits in the middle of all this) in a specific manner.
> 
> EUD is present on all non-ancient SoCs and by default it's on in bypass mode, so
> you can ignore its existence. That is, unless you want to use the features it
> provides, which we absolutely do.
> 
>>> is missing and EUD node is disabled. As described in my cover letter, HS phy
>>> support is needed for EUD functionality and this is applicable to all SoCs
>>> where EUD is to be enabled. Hence phy would be a required property.
>>
>> Nothing in commit msg explained that, but I have a bit hard time to
>> believe that this never worked. If that's the case, say it explicitly in
>> commit msg - this was always broken.
> 
> Even if it does work, it does so on a specific class of boards, relying on
> specific setup from a previous stage bootloader.
> 
>>> Given that the changes in this series are directly applicable to sc7280 as well,
>>> I will re-enable/rectify EUD feature on sc7280 SoC first, by adhering it to this binding
>>> requirement. That would address the ABI break. 
>>
>> I don't understand what you are saying here.
>>
>>> Once the base framework is set I shall extend it to sm8450 SoC.
> 
> tldr, we should fix both the bindings and the 7280 dt for it

On top of these comments, reg should be split up into three regions too,
if we want to be docs-accurate..

Konrad

[PATCH 2/8] dt-bindings: soc: qcom: eud: Add usb role switch property

[Elson Roy Serrao]

EUD hub is physically present in between the USB connector and the
USB controller. So the role switch notifications originating from
the connector should route through EUD. Hence to interpret the usb
role assigned by the connector, role switch property is needed.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
index fca5b608ec63..0fa4608568d0 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
@@ -37,6 +37,10 @@ properties:
     items:
       - const: usb2-phy
 
+  usb-role-switch:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: Support role switch.
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
     description:
-- 
2.17.1

Re: [PATCH 2/8] dt-bindings: soc: qcom: eud: Add usb role switch property

[Krzysztof Kozlowski]

On 31/07/2024 00:24, Elson Roy Serrao wrote:
> EUD hub is physically present in between the USB connector and the
> USB controller. So the role switch notifications originating from
> the connector should route through EUD. Hence to interpret the usb
> role assigned by the connector, role switch property is needed.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> index fca5b608ec63..0fa4608568d0 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> @@ -37,6 +37,10 @@ properties:
>      items:
>        - const: usb2-phy
>  
> +  usb-role-switch:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: Support role switch.

So both EUD and DWC3 controller (as this binding states) are role switching?

Best regards,
Krzysztof

Re: [PATCH 2/8] dt-bindings: soc: qcom: eud: Add usb role switch property

[Elson Serrao]

On 7/30/2024 10:36 PM, Krzysztof Kozlowski wrote:
> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>> EUD hub is physically present in between the USB connector and the
>> USB controller. So the role switch notifications originating from
>> the connector should route through EUD. Hence to interpret the usb
>> role assigned by the connector, role switch property is needed.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>> index fca5b608ec63..0fa4608568d0 100644
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>> @@ -37,6 +37,10 @@ properties:
>>      items:
>>        - const: usb2-phy
>>  
>> +  usb-role-switch:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: Support role switch.
> 
> So both EUD and DWC3 controller (as this binding states) are role switching?
> 

Yes. EUD would receive roles from the connector and relay it to the DWC3 controller. In addition to these roles, the DWC3 controller
would also receive roles from EUD itself (related to USB attach/detach events).

Thanks
Elson 

Re: [PATCH 2/8] dt-bindings: soc: qcom: eud: Add usb role switch property

[Krzysztof Kozlowski]

On 01/08/2024 02:16, Elson Serrao wrote:
> 
> 
> On 7/30/2024 10:36 PM, Krzysztof Kozlowski wrote:
>> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>>> EUD hub is physically present in between the USB connector and the
>>> USB controller. So the role switch notifications originating from
>>> the connector should route through EUD. Hence to interpret the usb
>>> role assigned by the connector, role switch property is needed.
>>>
>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> index fca5b608ec63..0fa4608568d0 100644
>>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> @@ -37,6 +37,10 @@ properties:
>>>      items:
>>>        - const: usb2-phy
>>>  
>>> +  usb-role-switch:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description: Support role switch.
>>
>> So both EUD and DWC3 controller (as this binding states) are role switching?
>>
> 
> Yes. EUD would receive roles from the connector and relay it to the DWC3 controller. In addition to these roles, the DWC3 controller
> would also receive roles from EUD itself (related to USB attach/detach events).

Does not look right. Seems like you add something because it is easier
to code in drivers.

Best regards,
Krzysztof

Re: [PATCH 2/8] dt-bindings: soc: qcom: eud: Add usb role switch property

[Konrad Dybcio]

On 1.08.2024 9:46 AM, Krzysztof Kozlowski wrote:
> On 01/08/2024 02:16, Elson Serrao wrote:
>>
>>
>> On 7/30/2024 10:36 PM, Krzysztof Kozlowski wrote:
>>> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>>>> EUD hub is physically present in between the USB connector and the
>>>> USB controller. So the role switch notifications originating from
>>>> the connector should route through EUD. Hence to interpret the usb
>>>> role assigned by the connector, role switch property is needed.
>>>>
>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>> index fca5b608ec63..0fa4608568d0 100644
>>>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>>> @@ -37,6 +37,10 @@ properties:
>>>>      items:
>>>>        - const: usb2-phy
>>>>  
>>>> +  usb-role-switch:
>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>> +    description: Support role switch.
>>>
>>> So both EUD and DWC3 controller (as this binding states) are role switching?
>>>
>>
>> Yes. EUD would receive roles from the connector and relay it to the DWC3 controller. In addition to these roles, the DWC3 controller
>> would also receive roles from EUD itself (related to USB attach/detach events).
> 
> Does not look right. Seems like you add something because it is easier
> to code in drivers.

Perhaps that's semantics.. EUD can be thought of as something approximating
USB-C (very loosely). If you program it right, it exposes a USB hub full of
"""altmodes""" (debug components visible as separate USB peripherals, really)

We need it to know when the device is connected in USB device mode (vs host),
so that we don't accidentally undermine the rest of the USB hardware by
de-muxing the usb controller from a direct connection to the PHY.

Konrad

[PATCH 3/8] dt-bindings: soc: qcom: eud: Add compatible for sm8450

[Elson Roy Serrao]

Document the EUD compatible for sm8450 SoC.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
index 0fa4608568d0..d7a913bd5edb 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
@@ -18,6 +18,7 @@ properties:
     items:
       - enum:
           - qcom,sc7280-eud
+          - qcom,sm8450-eud
       - const: qcom,eud
 
   reg:
-- 
2.17.1

Re: [PATCH 3/8] dt-bindings: soc: qcom: eud: Add compatible for sm8450

[Krzysztof Kozlowski]

On 31/07/2024 00:24, Elson Roy Serrao wrote:
> Document the EUD compatible for sm8450 SoC.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml | 1 +

This should be squashed with the previous patches or the previous
patches should explain why the properties are missing in existing
binding, but everything was fine.

You add new device with all its new quirks or properties. Adding
"compatible" alone is not a change itself.

Best regards,
Krzysztof

[PATCH 4/8] arm64: dts: qcom: sm8450: Add EUD node

[Elson Roy Serrao]

Add a DT node to describe  Embedded USB Debugger(EUD) module
on sm8450 SoC.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 29 ++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 9bafb3b350ff..bcdf61223ff3 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -4693,6 +4693,35 @@
 			};
 		};
 
+		eud: eud@88e0000 {
+			compatible = "qcom,sm8450-eud", "qcom,eud";
+			reg = <0 0x88e0000 0 0x2000>,
+			      <0 0x88e2000 0 0x1000>;
+			interrupts-extended = <&pdc 11 IRQ_TYPE_LEVEL_HIGH>;
+
+			phys = <&usb_1_hsphy>;
+			phy-names = "usb2-phy";
+
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					eud_ep: endpoint {
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+					eud_con: endpoint {
+					};
+				};
+			};
+		};
+
 		nsp_noc: interconnect@320c0000 {
 			compatible = "qcom,sm8450-nsp-noc";
 			reg = <0 0x320c0000 0 0x10000>;
-- 
2.17.1

[PATCH 5/8] arm64: dts: qcom: Enable EUD on sm8450 hdk

[Elson Roy Serrao]

Enable EUD on sm8450 hdk and route the role switch endpoints
accordingly.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
index a754b8fe9167..21a63ad81aac 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
@@ -111,7 +111,7 @@
 					reg = <0>;
 
 					pmic_glink_hs_in: endpoint {
-						remote-endpoint = <&usb_1_dwc3_hs>;
+						remote-endpoint = <&eud_con>;
 					};
 				};
 
@@ -1102,9 +1102,22 @@
 };
 
 &usb_1_dwc3_hs {
+	remote-endpoint = <&eud_ep>;
+};
+
+&eud {
+	status = "okay";
+	usb-role-switch;
+};
+
+&eud_con {
 	remote-endpoint = <&pmic_glink_hs_in>;
 };
 
+&eud_ep {
+	remote-endpoint = <&usb_1_dwc3_hs>;
+};
+
 &usb_1_hsphy {
 	status = "okay";
 
-- 
2.17.1

[PATCH 6/8] usb: misc: eud: Add High-Speed Phy control for EUD operations

[Elson Roy Serrao]

The Embedded USB Debugger(EUD) is a HS-USB on-chip hub to support the
debug and trace capabilities on Qualcomm devices. It is physically
present in between the usb connector and the usb controller. Being a
HS USB hub, it relies on HS Phy for its functionality. Add HS phy
support in the eud driver and control the phy during eud enable/disable
operations.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/misc/qcom_eud.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 26e9b8749d8a..3de7d465912c 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -33,6 +34,7 @@
 struct eud_chip {
 	struct device			*dev;
 	struct usb_role_switch		*role_sw;
+	struct phy			*usb2_phy;
 	void __iomem			*base;
 	void __iomem			*mode_mgr;
 	unsigned int			int_status;
@@ -41,8 +43,35 @@ struct eud_chip {
 	bool				usb_attached;
 };
 
+static int eud_phy_enable(struct eud_chip *chip)
+{
+	int ret;
+
+	ret = phy_init(chip->usb2_phy);
+	if (ret)
+		return ret;
+
+	ret = phy_power_on(chip->usb2_phy);
+	if (ret)
+		phy_exit(chip->usb2_phy);
+
+	return ret;
+}
+
+static void eud_phy_disable(struct eud_chip *chip)
+{
+	phy_power_off(chip->usb2_phy);
+	phy_exit(chip->usb2_phy);
+}
+
 static int enable_eud(struct eud_chip *priv)
 {
+	int ret;
+
+	ret = eud_phy_enable(priv);
+	if (ret)
+		return ret;
+
 	writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
 	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
 			priv->base + EUD_REG_INT1_EN_MASK);
@@ -55,6 +84,7 @@ static void disable_eud(struct eud_chip *priv)
 {
 	writel(0, priv->base + EUD_REG_CSR_EUD_EN);
 	writel(0, priv->mode_mgr + EUD_REG_EUD_EN2);
+	eud_phy_disable(priv);
 }
 
 static ssize_t enable_show(struct device *dev,
@@ -186,6 +216,11 @@ static int eud_probe(struct platform_device *pdev)
 
 	chip->dev = &pdev->dev;
 
+	chip->usb2_phy = devm_phy_get(chip->dev, "usb2-phy");
+	if (IS_ERR(chip->usb2_phy))
+		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
+				     "no usb2 phy configured\n");
+
 	chip->role_sw = usb_role_switch_get(&pdev->dev);
 	if (IS_ERR(chip->role_sw))
 		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
-- 
2.17.1

Re: [PATCH 6/8] usb: misc: eud: Add High-Speed Phy control for EUD operations

[Krzysztof Kozlowski]

On 31/07/2024 00:24, Elson Roy Serrao wrote:
> The Embedded USB Debugger(EUD) is a HS-USB on-chip hub to support the
> debug and trace capabilities on Qualcomm devices. It is physically
> present in between the usb connector and the usb controller. Being a
> HS USB hub, it relies on HS Phy for its functionality. Add HS phy
> support in the eud driver and control the phy during eud enable/disable
> operations.
> 

...
>  static ssize_t enable_show(struct device *dev,
> @@ -186,6 +216,11 @@ static int eud_probe(struct platform_device *pdev)
>  
>  	chip->dev = &pdev->dev;
>  
> +	chip->usb2_phy = devm_phy_get(chip->dev, "usb2-phy");
> +	if (IS_ERR(chip->usb2_phy))
> +		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
> +				     "no usb2 phy configured\n");

This nicely breaks all users.

NAK

Best regards,
Krzysztof

Re: [PATCH 6/8] usb: misc: eud: Add High-Speed Phy control for EUD operations

[Elson Serrao]

On 7/30/2024 10:39 PM, Krzysztof Kozlowski wrote:
> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>> The Embedded USB Debugger(EUD) is a HS-USB on-chip hub to support the
>> debug and trace capabilities on Qualcomm devices. It is physically
>> present in between the usb connector and the usb controller. Being a
>> HS USB hub, it relies on HS Phy for its functionality. Add HS phy
>> support in the eud driver and control the phy during eud enable/disable
>> operations.
>>
> 
> ...
>>  static ssize_t enable_show(struct device *dev,
>> @@ -186,6 +216,11 @@ static int eud_probe(struct platform_device *pdev)
>>  
>>  	chip->dev = &pdev->dev;
>>  
>> +	chip->usb2_phy = devm_phy_get(chip->dev, "usb2-phy");
>> +	if (IS_ERR(chip->usb2_phy))
>> +		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
>> +				     "no usb2 phy configured\n");
> 
> This nicely breaks all users.
> 
> NAK
> 

As per my comment in [patch 1/8], phy would be a required property and hence I will first modify
and enable EUD on the existing user (sc7280 SoC) and then extend this to other users.

Thanks
Elson

Re: [PATCH 6/8] usb: misc: eud: Add High-Speed Phy control for EUD operations

[Krzysztof Kozlowski]

On 01/08/2024 00:38, Elson Serrao wrote:
> 
> 
> On 7/30/2024 10:39 PM, Krzysztof Kozlowski wrote:
>> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>>> The Embedded USB Debugger(EUD) is a HS-USB on-chip hub to support the
>>> debug and trace capabilities on Qualcomm devices. It is physically
>>> present in between the usb connector and the usb controller. Being a
>>> HS USB hub, it relies on HS Phy for its functionality. Add HS phy
>>> support in the eud driver and control the phy during eud enable/disable
>>> operations.
>>>
>>
>> ...
>>>  static ssize_t enable_show(struct device *dev,
>>> @@ -186,6 +216,11 @@ static int eud_probe(struct platform_device *pdev)
>>>  
>>>  	chip->dev = &pdev->dev;
>>>  
>>> +	chip->usb2_phy = devm_phy_get(chip->dev, "usb2-phy");
>>> +	if (IS_ERR(chip->usb2_phy))
>>> +		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
>>> +				     "no usb2 phy configured\n");
>>
>> This nicely breaks all users.
>>
>> NAK
>>
> 
> As per my comment in [patch 1/8], phy would be a required property and hence I will first modify
> and enable EUD on the existing user (sc7280 SoC) and then extend this to other users.

NAK, you break existing users without clear reason.

Best regards,
Krzysztof

[PATCH 7/8] usb: misc: eud: Handle usb role switch notifications

[Elson Roy Serrao]

Since EUD is physically present between the USB connector and
the USB controller, it should relay the usb role notifications
from the connector. Hence register a role switch handler to
process and relay these roles to the USB controller. This results
in a common framework to send both connector related events
and eud attach/detach events to the USB controller.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
 1 file changed, 69 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 3de7d465912c..9a49c934e8cf 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -10,6 +10,7 @@
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -35,12 +36,16 @@ struct eud_chip {
 	struct device			*dev;
 	struct usb_role_switch		*role_sw;
 	struct phy			*usb2_phy;
+
+	/* mode lock */
+	struct mutex			mutex;
 	void __iomem			*base;
 	void __iomem			*mode_mgr;
 	unsigned int			int_status;
 	int				irq;
 	bool				enabled;
 	bool				usb_attached;
+	enum usb_role			current_role;
 };
 
 static int eud_phy_enable(struct eud_chip *chip)
@@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
 	phy_exit(chip->usb2_phy);
 }
 
+static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
+{
+	struct usb_role_switch *sw;
+	int ret = 0;
+
+	mutex_lock(&chip->mutex);
+
+	/* Avoid duplicate role handling */
+	if (role == chip->current_role)
+		goto err;
+
+	sw = usb_role_switch_get(chip->dev);
+	if (IS_ERR_OR_NULL(sw)) {
+		dev_err(chip->dev, "failed to get usb switch\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = usb_role_switch_set_role(sw, role);
+	usb_role_switch_put(sw);
+
+	if (ret) {
+		dev_err(chip->dev, "failed to set role\n");
+		goto err;
+	}
+	chip->current_role = role;
+err:
+	mutex_unlock(&chip->mutex);
+
+	return ret;
+}
+
 static int enable_eud(struct eud_chip *priv)
 {
 	int ret;
@@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv)
 			priv->base + EUD_REG_INT1_EN_MASK);
 	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
 
-	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
+	return ret;
 }
 
 static void disable_eud(struct eud_chip *priv)
@@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev,
 	if (kstrtobool(buf, &enable))
 		return -EINVAL;
 
+	/* EUD enable is applicable only in DEVICE mode */
+	if (enable && chip->current_role != USB_ROLE_DEVICE)
+		return -EINVAL;
+
 	if (enable) {
 		ret = enable_eud(chip);
-		if (!ret)
-			chip->enabled = enable;
-		else
-			disable_eud(chip);
+		if (ret) {
+			dev_err(chip->dev, "failed to enable eud\n");
+			return count;
+		}
 	} else {
 		disable_eud(chip);
 	}
+	chip->enabled = enable;
 
 	return count;
 }
@@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
 	int ret;
 
 	if (chip->usb_attached)
-		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
+		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
 	else
-		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
-	if (ret)
-		dev_err(chip->dev, "failed to set role switch\n");
+		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
 
 	/* set and clear vbus_int_clr[0] to clear interrupt */
 	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
@@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void eud_role_switch_release(void *data)
+static int eud_usb_role_switch_set(struct usb_role_switch *sw,
+				   enum usb_role role)
 {
-	struct eud_chip *chip = data;
+	struct eud_chip *chip = usb_role_switch_get_drvdata(sw);
 
-	usb_role_switch_put(chip->role_sw);
+	return eud_usb_role_set(chip, role);
 }
 
 static int eud_probe(struct platform_device *pdev)
 {
 	struct eud_chip *chip;
+	struct usb_role_switch_desc eud_role_switch = {NULL};
 	int ret;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
@@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev)
 		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
 				     "no usb2 phy configured\n");
 
-	chip->role_sw = usb_role_switch_get(&pdev->dev);
-	if (IS_ERR(chip->role_sw))
-		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
-					"failed to get role switch\n");
-
-	ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip);
-	if (ret)
-		return dev_err_probe(chip->dev, ret,
-				"failed to add role switch release action\n");
-
 	chip->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(chip->base))
 		return PTR_ERR(chip->base);
@@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
 
+	eud_role_switch.fwnode = dev_fwnode(chip->dev);
+	eud_role_switch.set = eud_usb_role_switch_set;
+	eud_role_switch.get = NULL;
+	eud_role_switch.driver_data = chip;
+	chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch);
+
+	if (IS_ERR(chip->role_sw))
+		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
+				"failed to register role switch\n");
+
+	mutex_init(&chip->mutex);
+
 	enable_irq_wake(chip->irq);
 
 	platform_set_drvdata(pdev, chip);
@@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev)
 	if (chip->enabled)
 		disable_eud(chip);
 
+	if (chip->role_sw)
+		usb_role_switch_unregister(chip->role_sw);
+
 	device_init_wakeup(&pdev->dev, false);
 	disable_irq_wake(chip->irq);
 }
-- 
2.17.1

Re: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications

[Dmitry Baryshkov]

On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote:
> Since EUD is physically present between the USB connector and
> the USB controller, it should relay the usb role notifications
> from the connector. Hence register a role switch handler to
> process and relay these roles to the USB controller. This results
> in a common framework to send both connector related events
> and eud attach/detach events to the USB controller.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
>  1 file changed, 69 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 3de7d465912c..9a49c934e8cf 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -10,6 +10,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> @@ -35,12 +36,16 @@ struct eud_chip {
>  	struct device			*dev;
>  	struct usb_role_switch		*role_sw;
>  	struct phy			*usb2_phy;
> +
> +	/* mode lock */
> +	struct mutex			mutex;
>  	void __iomem			*base;
>  	void __iomem			*mode_mgr;
>  	unsigned int			int_status;
>  	int				irq;
>  	bool				enabled;
>  	bool				usb_attached;
> +	enum usb_role			current_role;
>  };
>  
>  static int eud_phy_enable(struct eud_chip *chip)
> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
>  	phy_exit(chip->usb2_phy);
>  }
>  
> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
> +{
> +	struct usb_role_switch *sw;
> +	int ret = 0;
> +
> +	mutex_lock(&chip->mutex);
> +
> +	/* Avoid duplicate role handling */
> +	if (role == chip->current_role)
> +		goto err;
> +
> +	sw = usb_role_switch_get(chip->dev);

Why isn't chip->role_sw good enough? Why do you need to get it each
time?

> +	if (IS_ERR_OR_NULL(sw)) {
> +		dev_err(chip->dev, "failed to get usb switch\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	ret = usb_role_switch_set_role(sw, role);
> +	usb_role_switch_put(sw);
> +
> +	if (ret) {
> +		dev_err(chip->dev, "failed to set role\n");
> +		goto err;
> +	}
> +	chip->current_role = role;
> +err:
> +	mutex_unlock(&chip->mutex);
> +
> +	return ret;
> +}
> +
>  static int enable_eud(struct eud_chip *priv)
>  {
>  	int ret;
> @@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv)
>  			priv->base + EUD_REG_INT1_EN_MASK);
>  	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
>  
> -	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
> +	return ret;
>  }
>  
>  static void disable_eud(struct eud_chip *priv)
> @@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev,
>  	if (kstrtobool(buf, &enable))
>  		return -EINVAL;
>  
> +	/* EUD enable is applicable only in DEVICE mode */
> +	if (enable && chip->current_role != USB_ROLE_DEVICE)
> +		return -EINVAL;
> +
>  	if (enable) {
>  		ret = enable_eud(chip);
> -		if (!ret)
> -			chip->enabled = enable;
> -		else
> -			disable_eud(chip);
> +		if (ret) {
> +			dev_err(chip->dev, "failed to enable eud\n");
> +			return count;
> +		}
>  	} else {
>  		disable_eud(chip);
>  	}
> +	chip->enabled = enable;
>  
>  	return count;
>  }
> @@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  	int ret;
>  
>  	if (chip->usb_attached)
> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
> +		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
>  	else
> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
> -	if (ret)
> -		dev_err(chip->dev, "failed to set role switch\n");
> +		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
>  
>  	/* set and clear vbus_int_clr[0] to clear interrupt */
>  	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
> @@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void eud_role_switch_release(void *data)
> +static int eud_usb_role_switch_set(struct usb_role_switch *sw,
> +				   enum usb_role role)
>  {
> -	struct eud_chip *chip = data;
> +	struct eud_chip *chip = usb_role_switch_get_drvdata(sw);
>  
> -	usb_role_switch_put(chip->role_sw);
> +	return eud_usb_role_set(chip, role);
>  }
>  
>  static int eud_probe(struct platform_device *pdev)
>  {
>  	struct eud_chip *chip;
> +	struct usb_role_switch_desc eud_role_switch = {NULL};
>  	int ret;
>  
>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> @@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev)
>  		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
>  				     "no usb2 phy configured\n");
>  
> -	chip->role_sw = usb_role_switch_get(&pdev->dev);
> -	if (IS_ERR(chip->role_sw))
> -		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
> -					"failed to get role switch\n");
> -
> -	ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip);
> -	if (ret)
> -		return dev_err_probe(chip->dev, ret,
> -				"failed to add role switch release action\n");
> -
>  	chip->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(chip->base))
>  		return PTR_ERR(chip->base);
> @@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
>  
> +	eud_role_switch.fwnode = dev_fwnode(chip->dev);
> +	eud_role_switch.set = eud_usb_role_switch_set;
> +	eud_role_switch.get = NULL;
> +	eud_role_switch.driver_data = chip;
> +	chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch);
> +
> +	if (IS_ERR(chip->role_sw))
> +		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
> +				"failed to register role switch\n");
> +
> +	mutex_init(&chip->mutex);

please move mutex_init earlier.

> +
>  	enable_irq_wake(chip->irq);
>  
>  	platform_set_drvdata(pdev, chip);
> @@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev)
>  	if (chip->enabled)
>  		disable_eud(chip);
>  
> +	if (chip->role_sw)
> +		usb_role_switch_unregister(chip->role_sw);
> +
>  	device_init_wakeup(&pdev->dev, false);
>  	disable_irq_wake(chip->irq);
>  }
> -- 
> 2.17.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications

[Elson Serrao]

On 7/31/2024 6:06 AM, Dmitry Baryshkov wrote:
> On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote:
>> Since EUD is physically present between the USB connector and
>> the USB controller, it should relay the usb role notifications
>> from the connector. Hence register a role switch handler to
>> process and relay these roles to the USB controller. This results
>> in a common framework to send both connector related events
>> and eud attach/detach events to the USB controller.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>  drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
>>  1 file changed, 69 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
>> index 3de7d465912c..9a49c934e8cf 100644
>> --- a/drivers/usb/misc/qcom_eud.c
>> +++ b/drivers/usb/misc/qcom_eud.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/iopoll.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>> +#include <linux/mutex.h>
>>  #include <linux/of.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_device.h>
>> @@ -35,12 +36,16 @@ struct eud_chip {
>>  	struct device			*dev;
>>  	struct usb_role_switch		*role_sw;
>>  	struct phy			*usb2_phy;
>> +
>> +	/* mode lock */
>> +	struct mutex			mutex;
>>  	void __iomem			*base;
>>  	void __iomem			*mode_mgr;
>>  	unsigned int			int_status;
>>  	int				irq;
>>  	bool				enabled;
>>  	bool				usb_attached;
>> +	enum usb_role			current_role;
>>  };
>>  
>>  static int eud_phy_enable(struct eud_chip *chip)
>> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
>>  	phy_exit(chip->usb2_phy);
>>  }
>>  
>> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
>> +{
>> +	struct usb_role_switch *sw;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&chip->mutex);
>> +
>> +	/* Avoid duplicate role handling */
>> +	if (role == chip->current_role)
>> +		goto err;
>> +
>> +	sw = usb_role_switch_get(chip->dev);
> 
> Why isn't chip->role_sw good enough? Why do you need to get it each
> time?
>

Hi Dmitry

chip->role_sw is the eud role switch handler to receive role switch notifications from the
USB connector. The 'sw' I am getting above is the role switch handler of the USB controller.
As per this design, EUD receives role switch notification from the connector 
(via chip->role_sw) and then relays it to the 'sw' switch handler of the USB controller.


Thanks
Elson
 
>> +	if (IS_ERR_OR_NULL(sw)) {
>> +		dev_err(chip->dev, "failed to get usb switch\n");
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	ret = usb_role_switch_set_role(sw, role);
>> +	usb_role_switch_put(sw);
>> +
>> +	if (ret) {
>> +		dev_err(chip->dev, "failed to set role\n");
>> +		goto err;
>> +	}
>> +	chip->current_role = role;
>> +err:
>> +	mutex_unlock(&chip->mutex);
>> +
>> +	return ret;
>> +}
>> +
>>  static int enable_eud(struct eud_chip *priv)
>>  {
>>  	int ret;
>> @@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv)
>>  			priv->base + EUD_REG_INT1_EN_MASK);
>>  	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
>>  
>> -	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
>> +	return ret;
>>  }
>>  
>>  static void disable_eud(struct eud_chip *priv)
>> @@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev,
>>  	if (kstrtobool(buf, &enable))
>>  		return -EINVAL;
>>  
>> +	/* EUD enable is applicable only in DEVICE mode */
>> +	if (enable && chip->current_role != USB_ROLE_DEVICE)
>> +		return -EINVAL;
>> +
>>  	if (enable) {
>>  		ret = enable_eud(chip);
>> -		if (!ret)
>> -			chip->enabled = enable;
>> -		else
>> -			disable_eud(chip);
>> +		if (ret) {
>> +			dev_err(chip->dev, "failed to enable eud\n");
>> +			return count;
>> +		}
>>  	} else {
>>  		disable_eud(chip);
>>  	}
>> +	chip->enabled = enable;
>>  
>>  	return count;
>>  }
>> @@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>>  	int ret;
>>  
>>  	if (chip->usb_attached)
>> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
>> +		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
>>  	else
>> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
>> -	if (ret)
>> -		dev_err(chip->dev, "failed to set role switch\n");
>> +		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
>>  
>>  	/* set and clear vbus_int_clr[0] to clear interrupt */
>>  	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
>> @@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> -static void eud_role_switch_release(void *data)
>> +static int eud_usb_role_switch_set(struct usb_role_switch *sw,
>> +				   enum usb_role role)
>>  {
>> -	struct eud_chip *chip = data;
>> +	struct eud_chip *chip = usb_role_switch_get_drvdata(sw);
>>  
>> -	usb_role_switch_put(chip->role_sw);
>> +	return eud_usb_role_set(chip, role);
>>  }
>>  
>>  static int eud_probe(struct platform_device *pdev)
>>  {
>>  	struct eud_chip *chip;
>> +	struct usb_role_switch_desc eud_role_switch = {NULL};
>>  	int ret;
>>  
>>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> @@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev)
>>  		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
>>  				     "no usb2 phy configured\n");
>>  
>> -	chip->role_sw = usb_role_switch_get(&pdev->dev);
>> -	if (IS_ERR(chip->role_sw))
>> -		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
>> -					"failed to get role switch\n");
>> -
>> -	ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip);
>> -	if (ret)
>> -		return dev_err_probe(chip->dev, ret,
>> -				"failed to add role switch release action\n");
>> -
>>  	chip->base = devm_platform_ioremap_resource(pdev, 0);
>>  	if (IS_ERR(chip->base))
>>  		return PTR_ERR(chip->base);
>> @@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
>>  
>> +	eud_role_switch.fwnode = dev_fwnode(chip->dev);
>> +	eud_role_switch.set = eud_usb_role_switch_set;
>> +	eud_role_switch.get = NULL;
>> +	eud_role_switch.driver_data = chip;
>> +	chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch);
>> +
>> +	if (IS_ERR(chip->role_sw))
>> +		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
>> +				"failed to register role switch\n");
>> +
>> +	mutex_init(&chip->mutex);
> 
> please move mutex_init earlier.
> 

Ack
>> +
>>  	enable_irq_wake(chip->irq);
>>  
>>  	platform_set_drvdata(pdev, chip);
>> @@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev)
>>  	if (chip->enabled)
>>  		disable_eud(chip);
>>  
>> +	if (chip->role_sw)
>> +		usb_role_switch_unregister(chip->role_sw);
>> +
>>  	device_init_wakeup(&pdev->dev, false);
>>  	disable_irq_wake(chip->irq);
>>  }
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications

[Dmitry Baryshkov]

On Wed, Jul 31, 2024 at 05:51:17PM GMT, Elson Serrao wrote:
> 
> 
> On 7/31/2024 6:06 AM, Dmitry Baryshkov wrote:
> > On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote:
> >> Since EUD is physically present between the USB connector and
> >> the USB controller, it should relay the usb role notifications
> >> from the connector. Hence register a role switch handler to
> >> process and relay these roles to the USB controller. This results
> >> in a common framework to send both connector related events
> >> and eud attach/detach events to the USB controller.
> >>
> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> >> ---
> >>  drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
> >>  1 file changed, 69 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> >> index 3de7d465912c..9a49c934e8cf 100644
> >> --- a/drivers/usb/misc/qcom_eud.c
> >> +++ b/drivers/usb/misc/qcom_eud.c
> >> @@ -10,6 +10,7 @@
> >>  #include <linux/iopoll.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/module.h>
> >> +#include <linux/mutex.h>
> >>  #include <linux/of.h>
> >>  #include <linux/phy/phy.h>
> >>  #include <linux/platform_device.h>
> >> @@ -35,12 +36,16 @@ struct eud_chip {
> >>  	struct device			*dev;
> >>  	struct usb_role_switch		*role_sw;
> >>  	struct phy			*usb2_phy;
> >> +
> >> +	/* mode lock */
> >> +	struct mutex			mutex;
> >>  	void __iomem			*base;
> >>  	void __iomem			*mode_mgr;
> >>  	unsigned int			int_status;
> >>  	int				irq;
> >>  	bool				enabled;
> >>  	bool				usb_attached;
> >> +	enum usb_role			current_role;
> >>  };
> >>  
> >>  static int eud_phy_enable(struct eud_chip *chip)
> >> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
> >>  	phy_exit(chip->usb2_phy);
> >>  }
> >>  
> >> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
> >> +{
> >> +	struct usb_role_switch *sw;
> >> +	int ret = 0;
> >> +
> >> +	mutex_lock(&chip->mutex);
> >> +
> >> +	/* Avoid duplicate role handling */
> >> +	if (role == chip->current_role)
> >> +		goto err;
> >> +
> >> +	sw = usb_role_switch_get(chip->dev);
> > 
> > Why isn't chip->role_sw good enough? Why do you need to get it each
> > time?
> >
> 
> Hi Dmitry
> 
> chip->role_sw is the eud role switch handler to receive role switch notifications from the
> USB connector. The 'sw' I am getting above is the role switch handler of the USB controller.
> As per this design, EUD receives role switch notification from the connector 
> (via chip->role_sw) and then relays it to the 'sw' switch handler of the USB controller.

The fact that you have repurposed existing structure field is not a
waiver for the inefficiency.

So what about keeping chip->role_sw as is and adding _new_ field for the
self-provided role switch?


-- 
With best wishes
Dmitry

Re: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications

[kernel test robot]

Hi Elson,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11-rc1 next-20240801]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Elson-Roy-Serrao/dt-bindings-soc-qcom-eud-Add-phy-related-bindings/20240801-210521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240730222439.3469-8-quic_eserrao%40quicinc.com
patch subject: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240802/202408020600.vU0uKLa7-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240802/202408020600.vU0uKLa7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408020600.vU0uKLa7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/usb/misc/qcom_eud.c: In function 'handle_eud_irq_thread':
>> drivers/usb/misc/qcom_eud.c:227:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
     227 |         int ret;
         |             ^~~


vim +/ret +227 drivers/usb/misc/qcom_eud.c

9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  223  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  224  static irqreturn_t handle_eud_irq_thread(int irq, void *data)
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  225  {
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  226  	struct eud_chip *chip = data;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 @227  	int ret;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  228  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  229  	if (chip->usb_attached)
c007e96bfd0471 Elson Roy Serrao    2024-07-30  230  		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  231  	else
c007e96bfd0471 Elson Roy Serrao    2024-07-30  232  		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  233  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  234  	/* set and clear vbus_int_clr[0] to clear interrupt */
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  235  	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  236  	writel(0, chip->base + EUD_REG_VBUS_INT_CLR);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  237  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  238  	return IRQ_HANDLED;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  239  }
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  240  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 8/8] usb: misc: eud: Add compatible for sm8450

[Elson Roy Serrao]

Add compatible string to enable EUD on sm8450 SoC.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/misc/qcom_eud.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 9a49c934e8cf..465d57c05c3c 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -315,6 +315,7 @@ static void eud_remove(struct platform_device *pdev)
 
 static const struct of_device_id eud_dt_match[] = {
 	{ .compatible = "qcom,sc7280-eud" },
+	{ .compatible = "qcom,sm8450-eud" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, eud_dt_match);
-- 
2.17.1

Re: [PATCH 8/8] usb: misc: eud: Add compatible for sm8450

[Krzysztof Kozlowski]

On 31/07/2024 00:24, Elson Roy Serrao wrote:
> Add compatible string to enable EUD on sm8450 SoC.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/misc/qcom_eud.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 9a49c934e8cf..465d57c05c3c 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -315,6 +315,7 @@ static void eud_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id eud_dt_match[] = {
>  	{ .compatible = "qcom,sc7280-eud" },
> +	{ .compatible = "qcom,sm8450-eud" },

No, let's don't.

I'll fix the existing file.

Best regards,
Krzysztof

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Caleb Connolly]

Hi,

On 31/07/2024 00:24, Elson Roy Serrao wrote:
> The Embedded USB Debugger (EUD) is a mini High-Speed USB on-chip hub to
> support the USB-based debug and trace capabilities on Qualcomm devices.
> The current implementation lacks in below aspects that are needed for
> proper EUD functionality.
> 
> 1.) HS-Phy control: EUD being a HS hub needs HS-Phy support for it's
>      operation. Hence EUD module should enable/disable HS-phy
>      accordingly.
> 	
> 2.) Proper routing of USB role switch notifications: EUD hub is physically
>      present in between the USB connector and the USB controller. So the
>      usb role switch notifications originating from the connector should
>      route through EUD. EUD also relies on role switch notifications to
>      communicate with the USB, regarding EUD attach/detach events.
> 
> This series aims at implementing the above aspects to enable EUD on
> Qualcomm sm8450 SoC.

Are there any plans to make this feature available for folks outside of 
Qualcomm / an NDA?

There is an openOCD fork on CodeLinaro but it still requires some 
proprietary library which is only available to folks with a quicinc 
email as I understand it.

Kind regards,

~ someone eager for JTAG access
> 
> Elson Roy Serrao (8):
>    dt-bindings: soc: qcom: eud: Add phy related bindings
>    dt-bindings: soc: qcom: eud: Add usb role switch property
>    dt-bindings: soc: qcom: eud: Add compatible for sm8450
>    arm64: dts: qcom: sm8450: Add EUD node
>    arm64: dts: qcom: Enable EUD on sm8450 hdk
>    usb: misc: eud: Add High-Speed Phy control for EUD operations
>    usb: misc: eud: Handle usb role switch notifications
>    usb: misc: eud: Add compatible for sm8450
> 
>   .../bindings/soc/qcom/qcom,eud.yaml           |  17 +++
>   arch/arm64/boot/dts/qcom/sm8450-hdk.dts       |  15 ++-
>   arch/arm64/boot/dts/qcom/sm8450.dtsi          |  29 ++++
>   drivers/usb/misc/qcom_eud.c                   | 125 +++++++++++++++---
>   4 files changed, 164 insertions(+), 22 deletions(-)
> 

-- 
// Caleb (they/them)

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Trilok Soni]

On 7/31/2024 4:13 AM, Caleb Connolly wrote:
>>     
>> 2.) Proper routing of USB role switch notifications: EUD hub is physically
>>      present in between the USB connector and the USB controller. So the
>>      usb role switch notifications originating from the connector should
>>      route through EUD. EUD also relies on role switch notifications to
>>      communicate with the USB, regarding EUD attach/detach events.
>>
>> This series aims at implementing the above aspects to enable EUD on
>> Qualcomm sm8450 SoC.
> 
> Are there any plans to make this feature available for folks outside of Qualcomm / an NDA?
> 
> There is an openOCD fork on CodeLinaro but it still requires some proprietary library which is only available to folks with a quicinc email as I understand it.
> 

Which codelinaro link are you referring here? 


-- 
---Trilok Soni

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Caleb Connolly]

Hi Trilok,

On 31/07/2024 21:58, Trilok Soni wrote:
> On 7/31/2024 4:13 AM, Caleb Connolly wrote:
>>>      
>>> 2.) Proper routing of USB role switch notifications: EUD hub is physically
>>>       present in between the USB connector and the USB controller. So the
>>>       usb role switch notifications originating from the connector should
>>>       route through EUD. EUD also relies on role switch notifications to
>>>       communicate with the USB, regarding EUD attach/detach events.
>>>
>>> This series aims at implementing the above aspects to enable EUD on
>>> Qualcomm sm8450 SoC.
>>
>> Are there any plans to make this feature available for folks outside of Qualcomm / an NDA?
>>
>> There is an openOCD fork on CodeLinaro but it still requires some proprietary library which is only available to folks with a quicinc email as I understand it.
>>
> 
> Which codelinaro link are you referring here?

That would be 
https://git.codelinaro.org/clo/la/openocd-org/openocd/-/blob/qcom_changes/README_QCOM?ref_type=heads

Which says:

Qualcomm specific tools:
- Login to qpm.qualcomm.com
- QUTS: 1.64.1.39 (version & above)
- Qualcomm Host USB Product Suite - QUD QC only : 1.00.63 (supported 
version)
- EUD QC : 2.1.1 (supported version)

I believe the specific versions of QUD and EUD are only available to 
Qualcomm engineers and not even to OEMs, though I might be mistaken.

Kind regards,
> 
> 

-- 
// Caleb (they/them)

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Trilok Soni]

On 8/1/2024 3:52 AM, Caleb Connolly wrote:
> Hi Trilok,
> 
> On 31/07/2024 21:58, Trilok Soni wrote:
>> On 7/31/2024 4:13 AM, Caleb Connolly wrote:
>>>>      2.) Proper routing of USB role switch notifications: EUD hub is physically
>>>>       present in between the USB connector and the USB controller. So the
>>>>       usb role switch notifications originating from the connector should
>>>>       route through EUD. EUD also relies on role switch notifications to
>>>>       communicate with the USB, regarding EUD attach/detach events.
>>>>
>>>> This series aims at implementing the above aspects to enable EUD on
>>>> Qualcomm sm8450 SoC.
>>>
>>> Are there any plans to make this feature available for folks outside of Qualcomm / an NDA?
>>>
>>> There is an openOCD fork on CodeLinaro but it still requires some proprietary library which is only available to folks with a quicinc email as I understand it.
>>>
>>
>> Which codelinaro link are you referring here?
> 
> That would be https://git.codelinaro.org/clo/la/openocd-org/openocd/-/blob/qcom_changes/README_QCOM?ref_type=heads
> 
> Which says:
> 
> Qualcomm specific tools:
> - Login to qpm.qualcomm.com
> - QUTS: 1.64.1.39 (version & above)
> - Qualcomm Host USB Product Suite - QUD QC only : 1.00.63 (supported version)
> - EUD QC : 2.1.1 (supported version)
> 
> I believe the specific versions of QUD and EUD are only available to Qualcomm engineers and not even to OEMs, though I might be mistaken.

Thanks. So are we okay w/ one of the following option? (trying to understand the need here properly before I relay it internally). 

Options:

(1) Provide EUD library and tools - proprietary w/o any login requirement. 
(2) Provide open-source EUD library and tools w/o any login requirement. 

Is Option (1) fine to begin with or option 2 is must? 


-- 
---Trilok Soni

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Dmitry Baryshkov]

On Tue, Aug 06, 2024 at 11:58:02AM GMT, Trilok Soni wrote:
> On 8/1/2024 3:52 AM, Caleb Connolly wrote:
> > Hi Trilok,
> > 
> > On 31/07/2024 21:58, Trilok Soni wrote:
> >> On 7/31/2024 4:13 AM, Caleb Connolly wrote:
> >>>>      2.) Proper routing of USB role switch notifications: EUD hub is physically
> >>>>       present in between the USB connector and the USB controller. So the
> >>>>       usb role switch notifications originating from the connector should
> >>>>       route through EUD. EUD also relies on role switch notifications to
> >>>>       communicate with the USB, regarding EUD attach/detach events.
> >>>>
> >>>> This series aims at implementing the above aspects to enable EUD on
> >>>> Qualcomm sm8450 SoC.
> >>>
> >>> Are there any plans to make this feature available for folks outside of Qualcomm / an NDA?
> >>>
> >>> There is an openOCD fork on CodeLinaro but it still requires some proprietary library which is only available to folks with a quicinc email as I understand it.
> >>>
> >>
> >> Which codelinaro link are you referring here?
> > 
> > That would be https://git.codelinaro.org/clo/la/openocd-org/openocd/-/blob/qcom_changes/README_QCOM?ref_type=heads
> > 
> > Which says:
> > 
> > Qualcomm specific tools:
> > - Login to qpm.qualcomm.com
> > - QUTS: 1.64.1.39 (version & above)
> > - Qualcomm Host USB Product Suite - QUD QC only : 1.00.63 (supported version)
> > - EUD QC : 2.1.1 (supported version)
> > 
> > I believe the specific versions of QUD and EUD are only available to Qualcomm engineers and not even to OEMs, though I might be mistaken.
> 
> Thanks. So are we okay w/ one of the following option? (trying to understand the need here properly before I relay it internally). 
> 
> Options:
> 
> (1) Provide EUD library and tools - proprietary w/o any login requirement. 
> (2) Provide open-source EUD library and tools w/o any login requirement. 
> 
> Is Option (1) fine to begin with or option 2 is must? 

The usual problem of (1) is future compatibility guarantees. What
system libraries will it depend upon? When the open-source world and
openocd update to the next libusb ABI, will it break the EUD library?

Next, which interfaces are going to be used and/or provided by the lib
and tools? In other words, will it be really useful?

Last, if is prorietary, then under which licence? Will it allow reverse
engineering or not? Will it allow redistributing? Also note that OpenOCD
is licenced under GPL-2.0-or-later, so while one can link it with a
proprietary software, they can not further distribute the resulting
binaries. Also there might be different questions on whether the lib
itself is a derivative work (and as such it must be covered by the GPL).

-- 
With best wishes
Dmitry

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Krzysztof Kozlowski]

On 31/07/2024 13:13, Caleb Connolly wrote:
> Hi,
> 
> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>> The Embedded USB Debugger (EUD) is a mini High-Speed USB on-chip hub to
>> support the USB-based debug and trace capabilities on Qualcomm devices.
>> The current implementation lacks in below aspects that are needed for
>> proper EUD functionality.
>>
>> 1.) HS-Phy control: EUD being a HS hub needs HS-Phy support for it's
>>      operation. Hence EUD module should enable/disable HS-phy
>>      accordingly.
>> 	
>> 2.) Proper routing of USB role switch notifications: EUD hub is physically
>>      present in between the USB connector and the USB controller. So the
>>      usb role switch notifications originating from the connector should
>>      route through EUD. EUD also relies on role switch notifications to
>>      communicate with the USB, regarding EUD attach/detach events.
>>
>> This series aims at implementing the above aspects to enable EUD on
>> Qualcomm sm8450 SoC.
> 
> Are there any plans to make this feature available for folks outside of 
> Qualcomm / an NDA?
> 
> There is an openOCD fork on CodeLinaro but it still requires some 
> proprietary library which is only available to folks with a quicinc 
> email as I understand it.

Are you saying that there is no fully open and available user-space
which is necessary to use EUD?

Best regards,
Krzysztof

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Caleb Connolly]

On 01/08/2024 09:55, Krzysztof Kozlowski wrote:
> On 31/07/2024 13:13, Caleb Connolly wrote:
>> Hi,
>>
>> On 31/07/2024 00:24, Elson Roy Serrao wrote:
>>> The Embedded USB Debugger (EUD) is a mini High-Speed USB on-chip hub to
>>> support the USB-based debug and trace capabilities on Qualcomm devices.
>>> The current implementation lacks in below aspects that are needed for
>>> proper EUD functionality.
>>>
>>> 1.) HS-Phy control: EUD being a HS hub needs HS-Phy support for it's
>>>       operation. Hence EUD module should enable/disable HS-phy
>>>       accordingly.
>>> 	
>>> 2.) Proper routing of USB role switch notifications: EUD hub is physically
>>>       present in between the USB connector and the USB controller. So the
>>>       usb role switch notifications originating from the connector should
>>>       route through EUD. EUD also relies on role switch notifications to
>>>       communicate with the USB, regarding EUD attach/detach events.
>>>
>>> This series aims at implementing the above aspects to enable EUD on
>>> Qualcomm sm8450 SoC.
>>
>> Are there any plans to make this feature available for folks outside of
>> Qualcomm / an NDA?
>>
>> There is an openOCD fork on CodeLinaro but it still requires some
>> proprietary library which is only available to folks with a quicinc
>> email as I understand it.
> 
> Are you saying that there is no fully open and available user-space
> which is necessary to use EUD?

Yes, exactly. Tools must be obtained via the Qualcomm Package Manager 
which are not available by default after making an account and signing 
and NDA.

Kind regards,
> 
> Best regards,
> Krzysztof
> 

-- 
// Caleb (they/them)

Re: [PATCH 0/8] Enable EUD on Qualcomm sm8450 SoC

[Manivannan Sadhasivam]

On Tue, Jul 30, 2024 at 03:24:31PM -0700, Elson Roy Serrao wrote:
> The Embedded USB Debugger (EUD) is a mini High-Speed USB on-chip hub to
> support the USB-based debug and trace capabilities on Qualcomm devices.
> The current implementation lacks in below aspects that are needed for
> proper EUD functionality.
> 
> 1.) HS-Phy control: EUD being a HS hub needs HS-Phy support for it's
>     operation. Hence EUD module should enable/disable HS-phy
>     accordingly.
> 	
> 2.) Proper routing of USB role switch notifications: EUD hub is physically
>     present in between the USB connector and the USB controller. So the
>     usb role switch notifications originating from the connector should
>     route through EUD. EUD also relies on role switch notifications to
>     communicate with the USB, regarding EUD attach/detach events.
> 
> This series aims at implementing the above aspects to enable EUD on
> Qualcomm sm8450 SoC.
> 

For the open source community, EUD enablement means they will only get EUD ports
enumerated on the host and nothing else. There is no public info on how to use
EUD nor are there any tools to make use of it. So what is the purpose of
upstreaming it in the first place?

If the goal is to use EUD only by Qcom employees or customers who have signed
NDA, then you can just supply them the out-of-tree EUD patches and tools to work
with. There is absolutely no need to upstream the driver support.

So for this series and any other future EUD patches,

Nacked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

I'm really tempted to send a patch to remove the EUD driver altogether, but I'll
just wait for the response before doing so.

- Mani

-- 
மணிவண்ணன் சதாசிவம்