[PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[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.

Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
 drivers/usb/dwc3/core.c                        |   22 ++++++++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 7a95c65..5c4db93 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -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 {
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c35d49d..cf211be 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -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);
-- 
1.7.5.4

Re: [PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Dan Murphy]

On 05/30/2013 12:56 PM, Ruchika Kharwar wrote:
> 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?
>
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
>  drivers/usb/dwc3/core.c                        |   22 ++++++++++++++++++----
>  2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 7a95c65..5c4db93 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -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"

 

>  This is usually a subnode to DWC3 glue to which it is connected.
>  
>  dwc3@4a030000 {
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c35d49d..cf211be 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -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)) {
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.
> +			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?
> +			mode = DWC3_MODE_DRD;
> +		} else {
> +			if (strcmp(dr_mode, "host") == 0)
> +				mode = DWC3_MODE_HOST;
What if CONFIG_USB_DWC3_HOST is not enabled?
> +			else if (strcmp(dr_mode, "gadget") == 0)
> +				mode = DWC3_MODE_DEVICE;
What if CONFIG_USB_DWC3_GADGET is not enabled?
> +			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
> +			}
> +		}
> +	}
>  	switch (mode) {
>  	case DWC3_MODE_DEVICE:
>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);


-- 
------------------
Dan Murphy

[PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Ruchika Kharwar]

This patch adds an optional parameter "dr_mode" to the dwc3 core device node.
In the case the compile flag for the DWC3 controller is set to
"USB_DWC3_DUAL_ROLE" a device tree could restrain to either functionality of
host or gadget. In the case the device tree does not use this optional flag or
specifies it superfluously to "drd" the functionality will be that
of a dual role device.

Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
 drivers/usb/dwc3/core.c                        |   21 +++++++++++++++++----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 7a95c65..2f5d584 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -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. Supported modes are "gadget", "host"
+   and "drd".
 This is usually a subnode to DWC3 glue to which it is connected.
 
 dwc3@4a030000 {
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c35d49d..e11660a 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -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,22 @@ 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))
+			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 err2;
+			}
+		}
+	}
 	switch (mode) {
 	case DWC3_MODE_DEVICE:
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
-- 
1.7.5.4

Re: [PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Sergei Shtylyov]

Hello.

On 05/31/2013 12:14 AM, Ruchika Kharwar wrote:

> This patch adds an optional parameter "dr_mode" to the dwc3 core device node.
> In the case the compile flag for the DWC3 controller is set to
> "USB_DWC3_DUAL_ROLE" a device tree could restrain to either functionality of
> host or gadget. In the case the device tree does not use this optional flag or
> specifies it superfluously to "drd" the functionality will be that
> of a dual role device.
>
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> ---
>   Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
>   drivers/usb/dwc3/core.c                        |   21 +++++++++++++++++----
>   2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 7a95c65..2f5d584 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -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. Supported modes are "gadget", "host"
> +   and "drd".
>   This is usually a subnode to DWC3 glue to which it is connected.
>   
>   dwc3@4a030000 {
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c35d49d..e11660a 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -378,7 +378,7 @@ static int dwc3_probe(struct platform_device *pdev)
>   	void			*mem;
>   
>   	u8			mode;
> -
> +	char			*dr_mode;

    Please leave empty line here, after the declarations.

>   	mem = devm_kzalloc(dev, sizeof(*dwc) + DWC3_ALIGN_MASK, GFP_KERNEL);
>   	if (!mem) {
>   		dev_err(dev, "not enough memory\n");
>

WBR, Sergei

[PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Ruchika Kharwar]

This patch adds an optional parameter "dr_mode" to the dwc3 core device node.
In the case the compile flag for the DWC3 controller is set to
"USB_DWC3_DUAL_ROLE" a device tree could restrain to either functionality of
host or gadget. In the case the device tree does not use this optional flag or
specifies it superfluously to "drd" the functionality will be that
of a dual role device.

Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
 drivers/usb/dwc3/core.c                        |   20 +++++++++++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 7a95c65..2f5d584 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -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. Supported modes are "gadget", "host"
+   and "drd".
 This is usually a subnode to DWC3 glue to which it is connected.
 
 dwc3@4a030000 {
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c35d49d..05c0c8b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -378,6 +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) {
@@ -520,9 +521,22 @@ 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))
+			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 err2;
+			}
+		}
+	}
 	switch (mode) {
 	case DWC3_MODE_DEVICE:
 		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
-- 
1.7.5.4

Re: [PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Dan Murphy]

Fix spelling in my own comments

On 05/30/2013 03:31 PM, Dan Murphy wrote:
> On 05/30/2013 03:14 PM, Ruchika Kharwar wrote:
>> This patch adds an optional parameter "dr_mode" to the dwc3 core device node.
>> In the case the compile flag for the DWC3 controller is set to
>> "USB_DWC3_DUAL_ROLE" a device tree could restrain to either functionality of
>> host or gadget. In the case the device tree does not use this optional flag or
>> specifies it superfluously to "drd" the functionality will be that
>> of a dual role device.
>>
>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>> ---
> Can you add patch history if this is truly a new patch to a previous submission
>>  Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
>>  drivers/usb/dwc3/core.c                        |   21 +++++++++++++++++----
>>  2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 7a95c65..2f5d584 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -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. Supported modes are "gadget", "host"
>> +   and "drd".
> My previous comments still stand for this section.  Nothing was addressed or commented back
>
> https://patchwork.kernel.org/patch/2638511/
>>  This is usually a subnode to DWC3 glue to which it is connected.
>>  
>>  dwc3@4a030000 {
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index c35d49d..e11660a 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -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,22 @@ 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))
>> +			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;
> My previous comments still stand for this section.  Nothing was addressed or commented back
>
> https://patchwork.kernel.org/patch/2638511/
>> +			else {
>> +				dev_err(dev, "invalid dr_mode property value\n");
>> +				goto err2;
> This is still wrong.  You have not initialized and of the device's
s/and/any
>> +			}
>> +		}
>> +	}
>>  	switch (mode) {
>>  	case DWC3_MODE_DEVICE:
>>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> Is this a new patch?
> Subject should say v2.
>


-- 
------------------
Dan Murphy

Re: [PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Ruchika Kharwar]

On 05/30/2013 03:35 PM, Dan Murphy wrote:
> Fix spelling in my own comments
>
> On 05/30/2013 03:31 PM, Dan Murphy wrote:
>> On 05/30/2013 03:14 PM, Ruchika Kharwar wrote:
>>> This patch adds an optional parameter "dr_mode" to the dwc3 core device node.
>>> In the case the compile flag for the DWC3 controller is set to
>>> "USB_DWC3_DUAL_ROLE" a device tree could restrain to either functionality of
>>> host or gadget. In the case the device tree does not use this optional flag or
>>> specifies it superfluously to "drd" the functionality will be that
>>> of a dual role device.
>>>
>>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>>> ---
>> Can you add patch history if this is truly a new patch to a previous submission
         Will do in the next pach submitted.
>>>   Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
>>>   drivers/usb/dwc3/core.c                        |   21 +++++++++++++++++----
>>>   2 files changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index 7a95c65..2f5d584 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -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. Supported modes are "gadget", "host"
>>> +   and "drd".
>> My previous comments still stand for this section.  Nothing was addressed or commented back
>>
>> https://patchwork.kernel.org/patch/2638511/
>>>   This is usually a subnode to DWC3 glue to which it is connected.
>>>   
>>>   dwc3@4a030000 {
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index c35d49d..e11660a 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -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,22 @@ 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))
>>> +			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;
>> My previous comments still stand for this section.  Nothing was addressed or commented back
>>
>> https://patchwork.kernel.org/patch/2638511/
         DRD mode is the super set mode for Host and Gadget. The compile 
time flag is primary. If DRD is set, the host and gadget code is 
compiled in.
         The dr_mode property can be used *if* specified in the device 
tree. A specific example may be a board could have constraints where the 
port could only be a host
         or gadget. In that case, the dr_mode property is specified 
overrides the compile flag.
>>> +			else {
>>> +				dev_err(dev, "invalid dr_mode property value\n");
>>> +				goto err2;
>> This is still wrong.  You have not initialized and of the device's
> s/and/any
        This is correct as per 3.10-rc3. If you're looking at an older 
tree you'd be right.
>>> +			}
>>> +		}
>>> +	}
>>>   	switch (mode) {
>>>   	case DWC3_MODE_DEVICE:
>>>   		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>> Is this a new patch?
>> Subject should say v2.
Yes, my bad.. learning.. next patch will have it.
>>
>

Re: [PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

Hi,

On Thu, May 30, 2013 at 02:53:10PM -0500, Dan Murphy wrote:
> > @@ -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.

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.

> > +			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?

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.

> > +			mode = DWC3_MODE_DRD;
> > +		} else {
> > +			if (strcmp(dr_mode, "host") == 0)
> > +				mode = DWC3_MODE_HOST;
> What if CONFIG_USB_DWC3_HOST is not enabled?

No issues, this will only execute if DRD is enabled, which means both
Host and Device are built in the final binary.

> > +			else if (strcmp(dr_mode, "gadget") == 0)
> > +				mode = DWC3_MODE_DEVICE;
> What if CONFIG_USB_DWC3_GADGET is not enabled?

see above.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Michael Grzeschik]

Hi,

On Thu, May 30, 2013 at 03:14:54PM -0500, Ruchika Kharwar wrote:
> This patch adds an optional parameter "dr_mode" to the dwc3 core device node.
> In the case the compile flag for the DWC3 controller is set to
> "USB_DWC3_DUAL_ROLE" a device tree could restrain to either functionality of
> host or gadget. In the case the device tree does not use this optional flag or
> specifies it superfluously to "drd" the functionality will be that
> of a dual role device.
> 
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
>  drivers/usb/dwc3/core.c                        |   21 +++++++++++++++++----
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 7a95c65..2f5d584 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -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. Supported modes are "gadget", "host"
> +   and "drd".
>  This is usually a subnode to DWC3 glue to which it is connected.
>  
>  dwc3@4a030000 {
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c35d49d..e11660a 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -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,22 @@ 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))
> +			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 err2;
> +			}
> +		}
> +	}
>  	switch (mode) {
>  	case DWC3_MODE_DEVICE:
>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> -- 

Why not help to make use of that code?

https://patchwork.kernel.org/patch/2193321/

We currently stuck in the discussion about all possible dr_mode
properties. There came up a device that is host and device capable,
but not otg.

What does the property drd stand for?

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] usb: dwc3: Addition of "dr_mode" dt property.

[Michael Grzeschik]

Hi Felipe,

On Thu, May 30, 2013 at 03:31:21PM -0500, Ruchika Kharwar wrote:
> This patch adds an optional parameter "dr_mode" to the dwc3 core device node.
> In the case the compile flag for the DWC3 controller is set to
> "USB_DWC3_DUAL_ROLE" a device tree could restrain to either functionality of
> host or gadget. In the case the device tree does not use this optional flag or
> specifies it superfluously to "drd" the functionality will be that
> of a dual role device.
> 
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt |    3 ++-
>  drivers/usb/dwc3/core.c                        |   20 +++++++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 7a95c65..2f5d584 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -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. Supported modes are "gadget", "host"
> +   and "drd".
>  This is usually a subnode to DWC3 glue to which it is connected.
>  
>  dwc3@4a030000 {
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c35d49d..05c0c8b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -378,6 +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) {
> @@ -520,9 +521,22 @@ 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))
> +			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 err2;
> +			}
> +		}
> +	}
>  	switch (mode) {
>  	case DWC3_MODE_DEVICE:
>  		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> -- 
> 1.7.5.4

this is more likely to be solved with a common property description for
the gadget layer. We have some prepared parts in the latest patch series:

[PATCH 1/7] USB: add devicetree helpers for determining dr_mode and phy_type
http://www.spinics.net/lists/linux-usb/msg86829.html

What do you think?

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |