All of lore.kernel.org
 help / color / mirror / Atom feed
From: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
To: Rob Herring <robh@kernel.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	"Andy Gross" <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	"Konrad Dybcio" <konrad.dybcio@linaro.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Felipe Balbi <balbi@kernel.org>, <linux-usb@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <quic_pkondeti@quicinc.com>,
	<quic_ppratap@quicinc.com>, <quic_wcheng@quicinc.com>,
	<quic_jackp@quicinc.com>, <quic_harshq@quicinc.com>
Subject: Re: [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties
Date: Tue, 17 Jan 2023 14:31:38 +0530	[thread overview]
Message-ID: <4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com> (raw)
In-Reply-To: <20230116163401.GA2371990-robh@kernel.org>



On 1/16/2023 10:04 PM, Rob Herring wrote:
> On Sun, Jan 15, 2023 at 05:11:42PM +0530, Krishna Kurapati wrote:
>> Add bindings to indicate properties required to support multiport
>> on Snps Dwc3 controller.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   .../devicetree/bindings/usb/snps,dwc3.yaml    | 53 ++++++++++++++++---
>>   1 file changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index 6d78048c4613..3ea051beb2f8 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -81,15 +81,26 @@ properties:
>>   
>>     phys:
>>       minItems: 1
>> -    maxItems: 2
>> +    maxItems: 8
>>   
>>     phy-names:
>>       minItems: 1
>> -    maxItems: 2
>> -    items:
>> -      enum:
>> -        - usb2-phy
>> -        - usb3-phy
>> +    maxItems: 8
>> +    oneOf:
>> +    - items:
>> +        enum:
>> +          - usb2-phy
>> +          - usb3-phy
>> +    - items:
>> +        enum:
>> +          - usb2-phy_port0
>> +          - usb2-phy_port1
>> +          - usb2-phy_port2
>> +          - usb2-phy_port3
>> +          - usb3-phy_port0
>> +          - usb3-phy_port1
>> +          - usb3-phy_port2
>> +          - usb3-phy_port3
> 
> usbN-portM
> 
>>   
>>     resets:
>>       minItems: 1
>> @@ -360,6 +371,22 @@ properties:
>>       description:
>>         Enable USB remote wakeup.
>>   
>> +  num-ports:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      This property indicates the number of ports present on the target that
>> +      are to be serviced by the DWC3 controller.
>> +    minimum: 1
>> +    maximum: 4
>> +
>> +  num-ss-ports:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      This property indicates the number of SS capable ports present on the
>> +      target that are to be serviced by the DWC3 controller.
>> +    minimum: 1
>> +    maximum: 4
> 
> This information is redundant. 'phy-names' tells you how many ports of
> each.
> 
Hi Rob,

  Thanks for the review. The reason I wanted to introduce two more 
variables is to get info on number of ports  and ss-capable ports 
present on hardware whether or not the user provides them in DTSI file.

In the code there are two types of per port / per phy operations:
a) Modifying GUSB2PFYCFG and GUSB3PIPECTL registers per phy.
b) Generic Phy operations - per phy.

In today's code, if someone doesn't mention the SSPHY in DTSI, 
dwc->usb3_generic_phy will be NULL and any call to phy operations will 
just bail out. And irrespective of whether we provide SS Phy in DTSI or 
not, we still configure GUSB3PIPECTL register.

Consider the following cases:

1. There are 3 ports and 2 of them are SS capable and all phy's are 
mentioned in DTSI.

phy-names= "usb2-port0", "usb3-port0", "usb2-port1", "usb3-port1", 
"usb2-port2"

When we count them in the driver, we get num ports as 3 (presuming 
num-ports = num of hs ports) and num-ss-ports = 2.

Since there is no ambiguity in which all ports to configure, we can 
modify GUSB2PHYCFG registers for all 3 HS Phy's and GUSB3PIPECTL for 
both SS Phy's.
This is a proper scenario.

2. If the user skips providing SS Phy on Port-0, then:

phy-names= "usb2-port0", "usb2-port1", "usb3-port1", "usb2-port2"

If we count the phys, we end up getting num-ports=3 and num-ss-ports=1.

Since in the driver code, we are not keeping track of which ports are SS 
capable and which ones are not, we end up configuring
GUSB2PIPECTL(port-0) instead of port-1  as the num-ss-ports is "1" which 
is incorrect.

3. If the user skips providing one complete port, in this case port-1 is 
skipped, then:

phy-names= "usb2-port0", "usb3-port0", "usb2-port2"

If we count the phys, we end up getting num-ports=2 and num-ss-ports=1.

Since in the driver code, we are not keeping track of which ports are SS 
capable and which ones are not, we end up configuring 
GUSB2PHYCFG(port-0) and GUSB2PHYCFG(port-1) instead of port-2 which is 
incorrect.

To avoid these scenarios, if we can get the exact number of SS Ports and 
Ports in total present on the HW, we can configure all the registers 
whether the phy's are provided in DTSI or not. (This is of no harm I 
believe as it still works in today's code)

Incase the 2nd and 3rd scenarios are not allowed and user *MUST* declare 
all the phy's in the DTSI, then I can go ahead and remove these 
properties and count them in the driver code.

Thanks,
Krishna,

>> +
>>   unevaluatedProperties: false
>>   
>>   required:
>> @@ -388,4 +415,18 @@ examples:
>>         snps,dis_u2_susphy_quirk;
>>         snps,dis_enblslpm_quirk;
>>       };
>> +  - |
>> +    usb@4a000000 {
>> +      compatible = "snps,dwc3";
>> +      reg = <0x4a000000 0xcfff>;
>> +      interrupts = <0 92 4>;
>> +      clocks = <&clk 1>, <&clk 2>, <&clk 3>;
>> +      clock-names = "bus_early", "ref", "suspend";
>> +      num-ports = <2>;
>> +      num-ss-ports = <1>;
>> +      phys = <&usb2_phy0>, <&usb3_phy0>, <&usb2_phy1>;
>> +      phy-names = "usb2-phy_port0", "usb3-phy_port0", "usb2-phy_port1";
>> +      snps,dis_u2_susphy_quirk;
>> +      snps,dis_enblslpm_quirk;
>> +    };
> 
> Does a different number of phys really need its own example?
> 
> Rob

  reply	other threads:[~2023-01-17  9:07 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-15 11:41 [RFC v4 0/5] Add multiport support for DWC3 controllers Krishna Kurapati
2023-01-15 11:41 ` [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties Krishna Kurapati
2023-01-15 15:11   ` Rob Herring
2023-01-16 16:34   ` Rob Herring
2023-01-17  9:01     ` Krishna Kurapati PSSNV [this message]
2023-01-17 11:02       ` Krzysztof Kozlowski
2023-01-17 14:01         ` Krishna Kurapati PSSNV
2023-01-18 18:20   ` Bjorn Andersson
2023-01-15 11:41 ` [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller Krishna Kurapati
2023-01-19  0:36   ` Thinh Nguyen
2023-01-19  3:01     ` Krishna Kurapati PSSNV
2023-01-20  1:02       ` Thinh Nguyen
2023-01-20  1:46         ` Krishna Kurapati PSSNV
2023-01-20 22:44           ` Thinh Nguyen
2023-01-21  2:09             ` Krishna Kurapati PSSNV
2023-01-25 10:07             ` Krishna Kurapati PSSNV
2023-01-25 19:08               ` Thinh Nguyen
2023-01-25 20:49                 ` Jack Pham
2023-01-25 22:27                   ` Thinh Nguyen
2023-01-20 22:57     ` Thinh Nguyen
2023-01-21  2:06       ` Krishna Kurapati PSSNV
2023-01-21  2:19         ` Thinh Nguyen
2023-01-21  2:24           ` Krishna Kurapati PSSNV
2023-01-21  2:55             ` Thinh Nguyen
2023-01-19 22:09   ` Andrew Halaney
2023-01-20  1:55     ` Krishna Kurapati PSSNV
2023-01-20 14:37       ` Andrew Halaney
2023-01-20 15:13         ` Krishna Kurapati PSSNV
2023-01-20 15:18           ` Krishna Kurapati PSSNV
2023-01-24  8:21             ` Shazad Hussain
2023-01-15 11:41 ` [RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for host only controllers Krishna Kurapati
2023-01-19  0:38   ` Thinh Nguyen
2023-01-19  1:57     ` Jack Pham
2023-01-19  2:32       ` Thinh Nguyen
2023-01-15 11:41 ` [RFC v4 4/5] usb: dwc3: qcom: Add multiport controller support for qcom wrapper Krishna Kurapati
2023-01-15 11:41 ` [RFC v4 5/5] arm: dts: msm: Add multiport controller node for usb Krishna Kurapati
2023-01-18 18:28   ` Bjorn Andersson
2023-01-18 18:31     ` Krishna Kurapati PSSNV
2023-01-19  3:43 ` [RFC v4 0/5] Add multiport support for DWC3 controllers Bjorn Andersson
2023-01-19  5:17   ` Krishna Kurapati PSSNV

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com \
    --to=quic_kriskura@quicinc.com \
    --cc=Thinh.Nguyen@synopsys.com \
    --cc=agross@kernel.org \
    --cc=andersson@kernel.org \
    --cc=balbi@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=konrad.dybcio@linaro.org \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=quic_harshq@quicinc.com \
    --cc=quic_jackp@quicinc.com \
    --cc=quic_pkondeti@quicinc.com \
    --cc=quic_ppratap@quicinc.com \
    --cc=quic_wcheng@quicinc.com \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.