usb: dwc3: Addition of "dr_mode" dt property.
Subsystems (3)
PATCH 1/1 usb: dwc3: Addition of "dr_mode" dt property.
This patch adds the possibility of the "mode" being specified in a device tree. This allows the scenario when there maybe multiple USB subsystems operating in different modes.
--- Documentation/devicetree/bindings/usb/dwc3.txt | 3 ++- drivers/usb/dwc3/core.c | 22 ++++++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/usb/dwc3.txt +2 -1
@@ -10,7 +10,8 @@ Required properties: Optional properties: - tx-fifo-resize: determines if the FIFO *has* to be reallocated. - + - dr_mode: determines the mode of core. This could be "gadget", "host", + "drd". This is usually a subnode to DWC3 glue to which it is connected. dwc3@4a030000 {
drivers/usb/dwc3/core.c +18 -4
@@ -378,7 +378,7 @@ static int dwc3_probe(struct platform_device *pdev)void *mem;u8 mode; - +char *dr_mode; mem =devm_kzalloc (dev,sizeof (*dwc) +DWC3_ALIGN_MASK ,GFP_KERNEL );if (!mem) {dev_err (dev,"not enough memory\n" );
@@ -520,9 +520,23 @@ static int dwc3_probe(struct platform_device *pdev) mode =DWC3_MODE_HOST ; else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) mode =DWC3_MODE_DEVICE ; - else - mode =DWC3_MODE_DRD ; - + else { +if (of_property_read_string (node,"dr_mode" , &dr_mode)) { +dev_warn (dev,"Missing dr_mode so assuming DWC3_MODE_DRD\n" ); + mode =DWC3_MODE_DRD ; + }else { +if (strcmp (dr_mode,"host" ) ==0 ) + mode =DWC3_MODE_HOST ; +else if (strcmp (dr_mode,"gadget" ) ==0 ) + mode =DWC3_MODE_DEVICE ; +else if (strcmp (dr_mode,"drd" ) ==0 ) + mode =DWC3_MODE_DRD ; +else { +dev_err (dev,"invalid dr_mode property value\n" ); + goto err0; + } + } + }switch (mode) {case DWC3_MODE_DEVICE :dwc3_set_mode (dwc,DWC3_GCTL_PRTCAP_DEVICE );
Signature · 1 line
1.7.5.4
Quoted 3 lines · Ruchika Kharwar
This patch adds the possibility of the "mode" being specified in a device tree. This allows the scenario when there maybe multiple USB subsystems operating in different modes.
Nitpick. Maybes and possibilities can we get more concrete statements?
Quoted 12 lines
Signed-off-by Ruchika Kharwar <[email protected]>--- Documentation/devicetree/bindings/usb/dwc3.txt | 3 ++- drivers/usb/dwc3/core.c | 22 ++++++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-)Documentation/devicetree/bindings/usb/dwc3.txt +0
-0
Documentation/devicetree/bindings/usb/dwc3.txt Required properties: · line 14
Quoted 6 lines
Documentation/devicetree/bindings/usb/dwc3.txt +2
-1@@ -10,7 +10,8 @@ Required properties: Optional properties: - tx-fifo-resize: determines if the FIFO *has* to be reallocated. - + - dr_mode: determines the mode of core. This could be "gadget", "host", + "drd".
change to a more concrete statement.
dr_mode: determines the mode of the DWC core. Modes supported are "gadget", "host", "drd"
Quoted 17 lines
Documentation/devicetree/bindings/usb/dwc3.txt +0
-0This is usually a subnode to DWC3 glue to which it is connected. dwc3@4a030000 {drivers/usb/dwc3/core.c +1
-1@@ -378,7 +378,7 @@ static int dwc3_probe(struct platform_device *pdev)void *mem;u8 mode; - +char *dr_mode; mem =devm_kzalloc (dev,sizeof (*dwc) +DWC3_ALIGN_MASK ,GFP_KERNEL );if (!mem) {dev_err (dev,"not enough memory\n" );
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 524
Quoted 8 lines
drivers/usb/dwc3/core.c +2
-3@@ -520,9 +520,23 @@ static int dwc3_probe(struct platform_device *pdev) mode =DWC3_MODE_HOST ; else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) mode =DWC3_MODE_DEVICE ; - else - mode =DWC3_MODE_DRD ; - + else { +if (of_property_read_string (node,"dr_mode" , &dr_mode)) {
This will not execute if the either CONFIG options are set and then the DT property is not even honored Did you test this with multiple CONFIG options?
There seems to be a conflict between CONFIGs and runtime operation.
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 525
Quoted 1 line
drivers/usb/dwc3/core.c +1
-0+dev_warn (dev,"Missing dr_mode so assuming DWC3_MODE_DRD\n" );
If dr_mode is an optional parameter why would the dev_warn say it is missing? Do we even want to warn here?
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 529
Quoted 4 lines
drivers/usb/dwc3/core.c +4
-0+ mode =DWC3_MODE_DRD ; + }else { +if (strcmp (dr_mode,"host" ) ==0 ) + mode =DWC3_MODE_HOST ;
What if CONFIG_USB_DWC3_HOST is not enabled?
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 531
Quoted 2 lines
drivers/usb/dwc3/core.c +2
-0+else if (strcmp (dr_mode,"gadget" ) ==0 ) + mode =DWC3_MODE_DEVICE ;
What if CONFIG_USB_DWC3_GADGET is not enabled?
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 536
Quoted 5 lines
drivers/usb/dwc3/core.c +5
-0+else if (strcmp (dr_mode,"drd" ) ==0 ) + mode =DWC3_MODE_DRD ; +else { +dev_err (dev,"invalid dr_mode property value\n" ); + goto err0;
This should be err1 since core init is called prior to this
Quoted 6 lines
drivers/usb/dwc3/core.c +3
-0+ } + } + }switch (mode) {case DWC3_MODE_DEVICE :dwc3_set_mode (dwc,DWC3_GCTL_PRTCAP_DEVICE );
Signature · 2 lines
------------------ Dan Murphy
Hi,
Quoted 5 lines · Dan Murphy
Quoted 5 lines
drivers/usb/dwc3/core.c +0
-1@@ -520,9 +520,23 @@ static int dwc3_probe(struct platform_device *pdev) mode =DWC3_MODE_HOST ; else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) mode =DWC3_MODE_DEVICE ; - else
Quoted 8 lines
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 524
Quoted 4 lines
drivers/usb/dwc3/core.c +2
-2- mode =DWC3_MODE_DRD ; - +else { +if (of_property_read_string (node,"dr_mode" , &dr_mode)) {This will not execute if the either CONFIG options are set and then the DT property is not even honored
Did you test this with multiple CONFIG options?
There seems to be a conflict between CONFIGs and runtime operation.
this is alright. We still want to honor the users who chose to compile the driver for gadget-only. In that case, there is no choice to be made.
Now, if you build the driver in its entirety (meaning, DRD), you can still choose in runtime if you want the driver to behave as host-only or gadget-only.
Picture a situation where you have a single SoC with multiple instances of this IP and you want to make sure that e.g. ports 1-3 are host-only, port 4 is peripheral-only and port 5 is DRD.
Quoted 3 lines
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 525
Quoted 1 line
drivers/usb/dwc3/core.c +1
-0+dev_warn (dev,"Missing dr_mode so assuming DWC3_MODE_DRD\n" );If
dr_modeis an optional parameter why would thedev_warnsay it is missing? Do we even want to warn here?
Yes. Definitely yes. That would mean a less than optimal DTS file. Still, for the sake of sensible defaults, we can still choose to work on DRD mode, assuming full capabilities in case user didn't write proper DTS, still user should be notified about it.
Quoted 5 lines
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 529
Quoted 4 lines
drivers/usb/dwc3/core.c +4
-0+ mode =DWC3_MODE_DRD ; + }else { +if (strcmp (dr_mode,"host" ) ==0 ) + mode =DWC3_MODE_HOST ;What if
CONFIG_USB_DWC3_HOSTis not enabled?
No issues, this will only execute if DRD is enabled, which means both Host and Device are built in the final binary.
Quoted 3 lines
drivers/usb/dwc3/core.c static int dwc3_probe(struct platform_device *pdev) · line 531
Quoted 2 lines
drivers/usb/dwc3/core.c +2
-0+else if (strcmp (dr_mode,"gadget" ) ==0 ) + mode =DWC3_MODE_DEVICE ;What if
CONFIG_USB_DWC3_GADGETis not enabled?
see above.
Signature · 1 line
balbi