[PATCH 0/5] usb: dwc2: fix parameter handling

[PATCH 0/5] usb: dwc2: fix parameter handling

[Stefan Wahren]

This patch series fixes several parameter handling issues
found on bcm2835 in gadget mode. It's based on Felipe's USB next.

Stefan Wahren (5):
  usb: dwc2: Do not set host parameter in peripheral mode
  usb: dwc2: fix dwc2_get_device_property for u8 and u16
  usb: dwc2: fix default value for DMA support
  usb: dwc2: gadget: fix default value for gadget-dma-desc
  usb: dwc2: fix kernel-doc for dwc2_set_param

 drivers/usb/dwc2/params.c |   32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] usb: dwc2: Do not set host parameter in peripheral mode

[Stefan Wahren]

Since commit "usb: dwc2: Improve handling of host and device hwparams" the
host mode specific hardware parameter aren't initialized in peripheral mode
from the register settings anymore. So we better do not set them in this
case which avoids the following warnings on bcm2835:

  256 invalid for host_nperio_tx_fifo_size. Check HW configuration.
  512 invalid for host_perio_tx_fifo_size. Check HW configuration.

Fixes: 55e1040e424b ("usb: dwc2: Improve handling of host and device hwparams")
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/usb/dwc2/params.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index a786256..fd5f7f8 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -1132,6 +1132,12 @@ static void dwc2_set_parameters(struct dwc2_hsotg *hsotg,
 				    false, "host-dma",
 				    true, false,
 				    dma_capable);
+		dwc2_set_param_host_rx_fifo_size(hsotg,
+				params->host_rx_fifo_size);
+		dwc2_set_param_host_nperio_tx_fifo_size(hsotg,
+				params->host_nperio_tx_fifo_size);
+		dwc2_set_param_host_perio_tx_fifo_size(hsotg,
+				params->host_perio_tx_fifo_size);
 	}
 	dwc2_set_param_dma_desc_enable(hsotg, params->dma_desc_enable);
 	dwc2_set_param_dma_desc_fs_enable(hsotg, params->dma_desc_fs_enable);
@@ -1140,12 +1146,6 @@ static void dwc2_set_parameters(struct dwc2_hsotg *hsotg,
 			params->host_support_fs_ls_low_power);
 	dwc2_set_param_enable_dynamic_fifo(hsotg,
 			params->enable_dynamic_fifo);
-	dwc2_set_param_host_rx_fifo_size(hsotg,
-			params->host_rx_fifo_size);
-	dwc2_set_param_host_nperio_tx_fifo_size(hsotg,
-			params->host_nperio_tx_fifo_size);
-	dwc2_set_param_host_perio_tx_fifo_size(hsotg,
-			params->host_perio_tx_fifo_size);
 	dwc2_set_param_max_transfer_size(hsotg,
 			params->max_transfer_size);
 	dwc2_set_param_max_packet_count(hsotg,
-- 
1.7.9.5

[PATCH 2/5] usb: dwc2: fix dwc2_get_device_property for u8 and u16

[Stefan Wahren]

According to the Devicetree ePAPR [1] the datatypes u8 and u16 are
not defined. So using device_property_read_u16() would result in
a partial read of a 32-bit big-endian integer which is not intended.
So we better read the complete 32-bit value. This fixes a regression
on bcm2835 where the values for g-rx-fifo-size and g-np-tx-fifo-size
always read as zero:

  Invalid value 0 for param g-rx-fifo-size
  Invalid value 0 for param g-np-tx-fifo-size

[1] - http://elinux.org/images/c/cf/Power_ePAPR_APPROVED_v1.1.pdf

Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/usb/dwc2/params.c |   12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index fd5f7f8..2c7b624 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -247,8 +247,6 @@
 static void dwc2_get_device_property(struct dwc2_hsotg *hsotg,
 				     char *property, u8 size, u64 *value)
 {
-	u8 val8;
-	u16 val16;
 	u32 val32;
 
 	switch (size) {
@@ -256,17 +254,7 @@ static void dwc2_get_device_property(struct dwc2_hsotg *hsotg,
 		*value = device_property_read_bool(hsotg->dev, property);
 		break;
 	case 1:
-		if (device_property_read_u8(hsotg->dev, property, &val8))
-			return;
-
-		*value = val8;
-		break;
 	case 2:
-		if (device_property_read_u16(hsotg->dev, property, &val16))
-			return;
-
-		*value = val16;
-		break;
 	case 4:
 		if (device_property_read_u32(hsotg->dev, property, &val32))
 			return;
-- 
1.7.9.5

[PATCH 3/5] usb: dwc2: fix default value for DMA support

[Stefan Wahren]

The current defaults for DMA results on a non-DMA platform in a unnecessary
error message:

  Invalid value 0 for param gadget-dma

So fix this by using dma_capable as default value.

Fixes: 9962b62f1be ("usb: dwc2: Deprecate g-use-dma binding")
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/usb/dwc2/params.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 2c7b624..30b954e 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -1088,7 +1088,7 @@ static void dwc2_set_gadget_dma(struct dwc2_hsotg *hsotg)
 	/* Buffer DMA */
 	dwc2_set_param_bool(hsotg, &p->g_dma,
 			    false, "gadget-dma",
-			    true, false,
+			    dma_capable, false,
 			    dma_capable);
 
 	/* DMA Descriptor */
@@ -1118,7 +1118,7 @@ static void dwc2_set_parameters(struct dwc2_hsotg *hsotg,
 
 		dwc2_set_param_bool(hsotg, &p->host_dma,
 				    false, "host-dma",
-				    true, false,
+				    dma_capable, false,
 				    dma_capable);
 		dwc2_set_param_host_rx_fifo_size(hsotg,
 				params->host_rx_fifo_size);
-- 
1.7.9.5

[PATCH 4/5] usb: dwc2: gadget: fix default value for gadget-dma-desc

[Stefan Wahren]

The current default for gadget DMA descriptor results on bcm2835 in a
unnecessary error message:

  Invalid value 1 for param gadget-dma-desc

So fix this by using hw->dma_desc_enable as default value.

Fixes: dec4b55677e ("usb: dwc2: gadget: Add descriptor DMA parameter")
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/usb/dwc2/params.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 30b954e..11fe68a 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -1094,7 +1094,7 @@ static void dwc2_set_gadget_dma(struct dwc2_hsotg *hsotg)
 	/* DMA Descriptor */
 	dwc2_set_param_bool(hsotg, &p->g_dma_desc, false,
 			    "gadget-dma-desc",
-			    p->g_dma, false,
+			    !!hw->dma_desc_enable, false,
 			    !!hw->dma_desc_enable);
 }
 
-- 
1.7.9.5

[PATCH 5/5] usb: dwc2: fix kernel-doc for dwc2_set_param

[Stefan Wahren]

Since there is no parameter @value replace it with @legacy.

Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/usb/dwc2/params.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 11fe68a..10407cb 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -320,7 +320,7 @@ static void dwc2_set_core_param(void *param, u8 size, u64 value)
  * @size: The size of the core parameter in bytes, or 0 for bool.
  *
  * This function looks up @property and sets the @param to that value.
- * If the property doesn't exist it uses the passed-in @value. It will
+ * If the property doesn't exist it uses the passed-in @legacy value. It will
  * verify that the value falls between @min and @max. If it doesn't,
  * it will output an error and set the parameter to either @def or,
  * failing that, to @min.
-- 
1.7.9.5

Re: [PATCH 0/5] usb: dwc2: fix parameter handling

[John Youn]

On 11/20/2016 1:26 PM, Stefan Wahren wrote:
> This patch series fixes several parameter handling issues
> found on bcm2835 in gadget mode. It's based on Felipe's USB next.
> 
> Stefan Wahren (5):
>   usb: dwc2: Do not set host parameter in peripheral mode
>   usb: dwc2: fix dwc2_get_device_property for u8 and u16
>   usb: dwc2: fix default value for DMA support
>   usb: dwc2: gadget: fix default value for gadget-dma-desc
>   usb: dwc2: fix kernel-doc for dwc2_set_param
> 
>  drivers/usb/dwc2/params.c |   32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 

For this series:

Acked-by: John Youn <johnyoun@synopsys.com>


Felipe,

This is too late for 4.10-rc1 right?

Can you queue for 4.10 fixes. I can remind you after 4.10-rc1 if it's
too early for that.

Regards,
John

Re: [PATCH 0/5] usb: dwc2: fix parameter handling

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 11/20/2016 1:26 PM, Stefan Wahren wrote:
>> This patch series fixes several parameter handling issues
>> found on bcm2835 in gadget mode. It's based on Felipe's USB next.
>> 
>> Stefan Wahren (5):
>>   usb: dwc2: Do not set host parameter in peripheral mode
>>   usb: dwc2: fix dwc2_get_device_property for u8 and u16
>>   usb: dwc2: fix default value for DMA support
>>   usb: dwc2: gadget: fix default value for gadget-dma-desc
>>   usb: dwc2: fix kernel-doc for dwc2_set_param
>> 
>>  drivers/usb/dwc2/params.c |   32 ++++++++++----------------------
>>  1 file changed, 10 insertions(+), 22 deletions(-)
>> 
>
> For this series:
>
> Acked-by: John Youn <johnyoun@synopsys.com>
>
>
> Felipe,
>
> This is too late for 4.10-rc1 right?
>
> Can you queue for 4.10 fixes. I can remind you after 4.10-rc1 if it's
> too early for that.

I can keep it in testing/fixes rebased on 'the next of the day' until
v4.10-rc1 is tagged. No problems.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 5/5] usb: dwc2: fix kernel-doc for dwc2_set_param

[Felipe Balbi]

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


Hi,

Stefan Wahren <stefan.wahren@i2se.com> writes:
> Since there is no parameter @value replace it with @legacy.
>
> Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/usb/dwc2/params.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 11fe68a..10407cb 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -320,7 +320,7 @@ static void dwc2_set_core_param(void *param, u8 size, u64 value)
>   * @size: The size of the core parameter in bytes, or 0 for bool.
>   *
>   * This function looks up @property and sets the @param to that value.
> - * If the property doesn't exist it uses the passed-in @value. It will
> + * If the property doesn't exist it uses the passed-in @legacy value. It will

This doesn't fix any bugs. Also, is @legacy a parameter?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 5/5] usb: dwc2: fix kernel-doc for dwc2_set_param

[Stefan Wahren]

Hi Felipe,

Am 22.11.2016 um 13:23 schrieb Felipe Balbi:
> Hi,
>
> Stefan Wahren <stefan.wahren@i2se.com> writes:
>> Since there is no parameter @value replace it with @legacy.
>>
>> Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  drivers/usb/dwc2/params.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>> index 11fe68a..10407cb 100644
>> --- a/drivers/usb/dwc2/params.c
>> +++ b/drivers/usb/dwc2/params.c
>> @@ -320,7 +320,7 @@ static void dwc2_set_core_param(void *param, u8 size, u64 value)
>>   * @size: The size of the core parameter in bytes, or 0 for bool.
>>   *
>>   * This function looks up @property and sets the @param to that value.
>> - * If the property doesn't exist it uses the passed-in @value. It will
>> + * If the property doesn't exist it uses the passed-in @legacy value. It will
> This doesn't fix any bugs. 

you are right. I found this documentation bug while fixing the issues
before.

Since this is the last patch of the series, you could ignore it. And i
resend it without fixes tag after the merge window.

> Also, is @legacy a parameter?
>

|/** * dwc2_set_param() - Set a core parameter * * @hsotg: Programming
view of the DWC_otg controller * @param: Pointer to the parameter to set
* @lookup: True if the property should be looked up * @property: The
device property to read * @legacy: The param value to set if @property
is not available. This * will typically be the legacy value set in the
static * params structure. * @def: The default value * @min: The minimum
value * @max: The maximum value * @size: The size of the core parameter
in bytes, or 0 for bool. * * This function looks up @property and sets
the @param to that value. * If the property doesn't exist it uses the
passed-in @value. It will * verify that the value falls between @min and
@max. If it doesn't, * it will output an error and set the parameter to
either @def or, * failing that, to @min. * * The @size is used to write
to @param and to query the device * properties so that this same
function can be used with different * types of parameters. */ static
void dwc2_set_param(struct dwc2_hsotg *hsotg, void *param, bool lookup,
char *property, u64 legacy, u64 def, u64 min, u64 max, u8 size)|

Re: [PATCH 5/5] usb: dwc2: fix kernel-doc for dwc2_set_param

[Felipe Balbi]

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


Hi,

Stefan Wahren <stefan.wahren@i2se.com> writes:
> Hi Felipe,
>
> Am 22.11.2016 um 13:23 schrieb Felipe Balbi:
>> Hi,
>>
>> Stefan Wahren <stefan.wahren@i2se.com> writes:
>>> Since there is no parameter @value replace it with @legacy.
>>>
>>> Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>>  drivers/usb/dwc2/params.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>>> index 11fe68a..10407cb 100644
>>> --- a/drivers/usb/dwc2/params.c
>>> +++ b/drivers/usb/dwc2/params.c
>>> @@ -320,7 +320,7 @@ static void dwc2_set_core_param(void *param, u8 size, u64 value)
>>>   * @size: The size of the core parameter in bytes, or 0 for bool.
>>>   *
>>>   * This function looks up @property and sets the @param to that value.
>>> - * If the property doesn't exist it uses the passed-in @value. It will
>>> + * If the property doesn't exist it uses the passed-in @legacy value. It will
>> This doesn't fix any bugs. 
>
> you are right. I found this documentation bug while fixing the issues
> before.
>
> Since this is the last patch of the series, you could ignore it. And i
> resend it without fixes tag after the merge window.

fine by me :-)

>> Also, is @legacy a parameter?
>>
>
> |/** * dwc2_set_param() - Set a core parameter * * @hsotg: Programming
> view of the DWC_otg controller * @param: Pointer to the parameter to set
> * @lookup: True if the property should be looked up * @property: The
> device property to read * @legacy: The param value to set if @property
> is not available. This * will typically be the legacy value set in the
> static * params structure. * @def: The default value * @min: The minimum
> value * @max: The maximum value * @size: The size of the core parameter
> in bytes, or 0 for bool. * * This function looks up @property and sets
> the @param to that value. * If the property doesn't exist it uses the
> passed-in @value. It will * verify that the value falls between @min and
> @max. If it doesn't, * it will output an error and set the parameter to
> either @def or, * failing that, to @min. * * The @size is used to write
> to @param and to query the device * properties so that this same
> function can be used with different * types of parameters. */ static
> void dwc2_set_param(struct dwc2_hsotg *hsotg, void *param, bool lookup,
> char *property, u64 legacy, u64 def, u64 min, u64 max, u8 size)|

heh, a little difficult to read, but got your point ;-) thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]