Re: [PATCH 15/18] usb: dwc2: Improve handling of host and device hwparams

[John Youn]

On 12/16/2015 8:08 AM, Felipe Balbi wrote:
> 
> Hi
> 
> John Youn  writes:
>> On 12/10/2015 2:55 PM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> John Youn  writes:
> Interesting.  You're getting a new parameter that's never been needed
> in the code before and (as far as I can tell) isn't used anywhere in
> your series.  I presume this is in prep for a future patch that uses
> this?
>
> At the moment you're burning a decent delay to read this unused
> parameter (potentially up to 100ms), so hopefully there's a good use
> for it eventually...

 I'll remove this from this series. It will be used by the gadget but
 those changes aren't ready to be submitted yet.
>>>
>>> so you're sending this series again ? Seems like I should drop all
>>> patches from my testing/next ?
>>>
>>
>> Yes I'll resend. You can drop them from your testing/next.
> 
> all right, dropped the entire series.
> 
> thanks
> 

Thanks. I'm still tracking down some issues with this. Hopefully
resend soon.

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/18] usb: dwc2: Improve handling of host and device hwparams

[Felipe Balbi]

Hi

John Youn  writes:
> On 12/10/2015 2:55 PM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> John Youn  writes:
 Interesting.  You're getting a new parameter that's never been needed
 in the code before and (as far as I can tell) isn't used anywhere in
 your series.  I presume this is in prep for a future patch that uses
 this?

 At the moment you're burning a decent delay to read this unused
 parameter (potentially up to 100ms), so hopefully there's a good use
 for it eventually...
>>>
>>> I'll remove this from this series. It will be used by the gadget but
>>> those changes aren't ready to be submitted yet.
>> 
>> so you're sending this series again ? Seems like I should drop all
>> patches from my testing/next ?
>> 
>
> Yes I'll resend. You can drop them from your testing/next.

all right, dropped the entire series.

thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 15/18] usb: dwc2: Improve handling of host and device hwparams

[John Youn]

On 12/9/2015 8:45 PM, Doug Anderson wrote:
> John,
> 
> On Wed, Dec 2, 2015 at 11:15 AM, John Youn  wrote:
>> This commit adds separate functions for getting the host and device
>> specific hardware params. These functions check whether the parameters
>> need to be read at all, depending on dr_mode, and they force the mode
>> only if necessary. This saves some delays during probe.
>>
>> Signed-off-by: John Youn 
>> ---
>>  drivers/usb/dwc2/core.c | 93 
>> +
>>  drivers/usb/dwc2/core.h |  1 +
>>  2 files changed, 71 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
>> index caaff42..91d63a8 100644
>> --- a/drivers/usb/dwc2/core.c
>> +++ b/drivers/usb/dwc2/core.c
>> @@ -3159,17 +3159,76 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg 
>> *hsotg)
>> usleep_range(25000, 5);
>>  }
>>
>> +/*
>> + * Gets host hardware parameters. Forces host mode if not currently in
>> + * host mode. Should be called immediately after a core soft reset in
>> + * order to get the reset values.
>> + */
>> +static void dwc2_get_host_hwparams(struct dwc2_hsotg *hsotg)
>> +{
>> +   struct dwc2_hw_params *hw = >hw_params;
>> +   u32 gnptxfsiz;
>> +   u32 hptxfsiz;
>> +   bool forced;
>> +
>> +   if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
>> +   return;
>> +
>> +   forced = dwc2_force_host_if_needed(hsotg);
>> +
>> +   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
>> +   hptxfsiz = dwc2_readl(hsotg->regs + HPTXFSIZ);
>> +   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
>> +   dev_dbg(hsotg->dev, "hptxfsiz=%08x\n", hptxfsiz);
>> +
>> +   if (forced)
>> +   dwc2_clear_force_mode(hsotg);
>> +
>> +   hw->host_nperio_tx_fifo_size = (gnptxfsiz & FIFOSIZE_DEPTH_MASK) >>
>> +  FIFOSIZE_DEPTH_SHIFT;
>> +   hw->host_perio_tx_fifo_size = (hptxfsiz & FIFOSIZE_DEPTH_MASK) >>
>> + FIFOSIZE_DEPTH_SHIFT;
>> +}
>> +
>> +/*
>> + * Gets device hardware parameters. Forces device mode if not
>> + * currently in device mode. Should be called immediately after a core
>> + * soft reset in order to get the reset values.
>> + */
>> +static void dwc2_get_dev_hwparams(struct dwc2_hsotg *hsotg)
>> +{
>> +   struct dwc2_hw_params *hw = >hw_params;
>> +   bool forced;
>> +   u32 gnptxfsiz;
>> +
>> +   if (hsotg->dr_mode == USB_DR_MODE_HOST)
>> +   return;
>> +
>> +   forced = dwc2_force_device_if_needed(hsotg);
> 
> Interesting.  You're getting a new parameter that's never been needed
> in the code before and (as far as I can tell) isn't used anywhere in
> your series.  I presume this is in prep for a future patch that uses
> this?
> 
> At the moment you're burning a decent delay to read this unused
> parameter (potentially up to 100ms), so hopefully there's a good use
> for it eventually...

I'll remove this from this series. It will be used by the gadget but
those changes aren't ready to be submitted yet.


> 
> 
>> +
>> +   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
>> +   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
>> +
>> +   if (forced)
>> +   dwc2_clear_force_mode(hsotg);
>> +
>> +   hw->dev_nperio_tx_fifo_size = (gnptxfsiz & FIFOSIZE_DEPTH_MASK) >>
>> +  FIFOSIZE_DEPTH_SHIFT;
>> +}
>> +
>>  /**
>>   * During device initialization, read various hardware configuration
>>   * registers and interpret the contents.
>> + *
>> + * This should be called during driver probe. It will perform a core
>> + * soft reset in order to get the reset values of the parameters.
>>   */
>>  int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>>  {
>> struct dwc2_hw_params *hw = >hw_params;
>> unsigned width;
>> u32 hwcfg1, hwcfg2, hwcfg3, hwcfg4;
>> -   u32 hptxfsiz, grxfsiz, gnptxfsiz;
>> -   u32 gusbcfg = 0;
>> +   u32 grxfsiz;
>> int retval;
>>
>> /*
>> @@ -3206,23 +3265,6 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>> dev_dbg(hsotg->dev, "hwcfg4=%08x\n", hwcfg4);
>> dev_dbg(hsotg->dev, "grxfsiz=%08x\n", grxfsiz);
>>
>> -   /* Force host mode to get HPTXFSIZ / GNPTXFSIZ exact power on value 
>> */
>> -   if (hsotg->dr_mode != USB_DR_MODE_HOST) {
>> -   gusbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
>> -   dwc2_writel(gusbcfg | GUSBCFG_FORCEHOSTMODE,
>> -   hsotg->regs + GUSBCFG);
>> -   usleep_range(25000, 5);
>> -   }
>> -
>> -   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
>> -   hptxfsiz = dwc2_readl(hsotg->regs + HPTXFSIZ);
>> -   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
>> -   dev_dbg(hsotg->dev, "hptxfsiz=%08x\n", hptxfsiz);
>> -   if (hsotg->dr_mode != 

Re: [PATCH 15/18] usb: dwc2: Improve handling of host and device hwparams

[John Youn]

On 12/10/2015 2:55 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>>> Interesting.  You're getting a new parameter that's never been needed
>>> in the code before and (as far as I can tell) isn't used anywhere in
>>> your series.  I presume this is in prep for a future patch that uses
>>> this?
>>>
>>> At the moment you're burning a decent delay to read this unused
>>> parameter (potentially up to 100ms), so hopefully there's a good use
>>> for it eventually...
>>
>> I'll remove this from this series. It will be used by the gadget but
>> those changes aren't ready to be submitted yet.
> 
> so you're sending this series again ? Seems like I should drop all
> patches from my testing/next ?
> 

Yes I'll resend. You can drop them from your testing/next.

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/18] usb: dwc2: Improve handling of host and device hwparams

[Felipe Balbi]

Hi,

John Youn  writes:
>> Interesting.  You're getting a new parameter that's never been needed
>> in the code before and (as far as I can tell) isn't used anywhere in
>> your series.  I presume this is in prep for a future patch that uses
>> this?
>> 
>> At the moment you're burning a decent delay to read this unused
>> parameter (potentially up to 100ms), so hopefully there's a good use
>> for it eventually...
>
> I'll remove this from this series. It will be used by the gadget but
> those changes aren't ready to be submitted yet.

so you're sending this series again ? Seems like I should drop all
patches from my testing/next ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 15/18] usb: dwc2: Improve handling of host and device hwparams

[Doug Anderson]

John,

On Wed, Dec 2, 2015 at 11:15 AM, John Youn  wrote:
> This commit adds separate functions for getting the host and device
> specific hardware params. These functions check whether the parameters
> need to be read at all, depending on dr_mode, and they force the mode
> only if necessary. This saves some delays during probe.
>
> Signed-off-by: John Youn 
> ---
>  drivers/usb/dwc2/core.c | 93 
> +
>  drivers/usb/dwc2/core.h |  1 +
>  2 files changed, 71 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index caaff42..91d63a8 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -3159,17 +3159,76 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg 
> *hsotg)
> usleep_range(25000, 5);
>  }
>
> +/*
> + * Gets host hardware parameters. Forces host mode if not currently in
> + * host mode. Should be called immediately after a core soft reset in
> + * order to get the reset values.
> + */
> +static void dwc2_get_host_hwparams(struct dwc2_hsotg *hsotg)
> +{
> +   struct dwc2_hw_params *hw = >hw_params;
> +   u32 gnptxfsiz;
> +   u32 hptxfsiz;
> +   bool forced;
> +
> +   if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
> +   return;
> +
> +   forced = dwc2_force_host_if_needed(hsotg);
> +
> +   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
> +   hptxfsiz = dwc2_readl(hsotg->regs + HPTXFSIZ);
> +   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
> +   dev_dbg(hsotg->dev, "hptxfsiz=%08x\n", hptxfsiz);
> +
> +   if (forced)
> +   dwc2_clear_force_mode(hsotg);
> +
> +   hw->host_nperio_tx_fifo_size = (gnptxfsiz & FIFOSIZE_DEPTH_MASK) >>
> +  FIFOSIZE_DEPTH_SHIFT;
> +   hw->host_perio_tx_fifo_size = (hptxfsiz & FIFOSIZE_DEPTH_MASK) >>
> + FIFOSIZE_DEPTH_SHIFT;
> +}
> +
> +/*
> + * Gets device hardware parameters. Forces device mode if not
> + * currently in device mode. Should be called immediately after a core
> + * soft reset in order to get the reset values.
> + */
> +static void dwc2_get_dev_hwparams(struct dwc2_hsotg *hsotg)
> +{
> +   struct dwc2_hw_params *hw = >hw_params;
> +   bool forced;
> +   u32 gnptxfsiz;
> +
> +   if (hsotg->dr_mode == USB_DR_MODE_HOST)
> +   return;
> +
> +   forced = dwc2_force_device_if_needed(hsotg);

Interesting.  You're getting a new parameter that's never been needed
in the code before and (as far as I can tell) isn't used anywhere in
your series.  I presume this is in prep for a future patch that uses
this?

At the moment you're burning a decent delay to read this unused
parameter (potentially up to 100ms), so hopefully there's a good use
for it eventually...


> +
> +   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
> +   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
> +
> +   if (forced)
> +   dwc2_clear_force_mode(hsotg);
> +
> +   hw->dev_nperio_tx_fifo_size = (gnptxfsiz & FIFOSIZE_DEPTH_MASK) >>
> +  FIFOSIZE_DEPTH_SHIFT;
> +}
> +
>  /**
>   * During device initialization, read various hardware configuration
>   * registers and interpret the contents.
> + *
> + * This should be called during driver probe. It will perform a core
> + * soft reset in order to get the reset values of the parameters.
>   */
>  int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>  {
> struct dwc2_hw_params *hw = >hw_params;
> unsigned width;
> u32 hwcfg1, hwcfg2, hwcfg3, hwcfg4;
> -   u32 hptxfsiz, grxfsiz, gnptxfsiz;
> -   u32 gusbcfg = 0;
> +   u32 grxfsiz;
> int retval;
>
> /*
> @@ -3206,23 +3265,6 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
> dev_dbg(hsotg->dev, "hwcfg4=%08x\n", hwcfg4);
> dev_dbg(hsotg->dev, "grxfsiz=%08x\n", grxfsiz);
>
> -   /* Force host mode to get HPTXFSIZ / GNPTXFSIZ exact power on value */
> -   if (hsotg->dr_mode != USB_DR_MODE_HOST) {
> -   gusbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
> -   dwc2_writel(gusbcfg | GUSBCFG_FORCEHOSTMODE,
> -   hsotg->regs + GUSBCFG);
> -   usleep_range(25000, 5);
> -   }
> -
> -   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
> -   hptxfsiz = dwc2_readl(hsotg->regs + HPTXFSIZ);
> -   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
> -   dev_dbg(hsotg->dev, "hptxfsiz=%08x\n", hptxfsiz);
> -   if (hsotg->dr_mode != USB_DR_MODE_HOST) {
> -   dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
> -   usleep_range(25000, 5);
> -   }
> -

optional nit: To make the function change less, it seems like
dwc2_get_host_hwparams() and dwc2_get_dev_hwparams() could be here
instead of below.  Then your forcing of modes / 

[PATCH 15/18] usb: dwc2: Improve handling of host and device hwparams

[John Youn]

This commit adds separate functions for getting the host and device
specific hardware params. These functions check whether the parameters
need to be read at all, depending on dr_mode, and they force the mode
only if necessary. This saves some delays during probe.

Signed-off-by: John Youn 
---
 drivers/usb/dwc2/core.c | 93 +
 drivers/usb/dwc2/core.h |  1 +
 2 files changed, 71 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index caaff42..91d63a8 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -3159,17 +3159,76 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg 
*hsotg)
usleep_range(25000, 5);
 }
 
+/*
+ * Gets host hardware parameters. Forces host mode if not currently in
+ * host mode. Should be called immediately after a core soft reset in
+ * order to get the reset values.
+ */
+static void dwc2_get_host_hwparams(struct dwc2_hsotg *hsotg)
+{
+   struct dwc2_hw_params *hw = >hw_params;
+   u32 gnptxfsiz;
+   u32 hptxfsiz;
+   bool forced;
+
+   if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
+   return;
+
+   forced = dwc2_force_host_if_needed(hsotg);
+
+   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
+   hptxfsiz = dwc2_readl(hsotg->regs + HPTXFSIZ);
+   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
+   dev_dbg(hsotg->dev, "hptxfsiz=%08x\n", hptxfsiz);
+
+   if (forced)
+   dwc2_clear_force_mode(hsotg);
+
+   hw->host_nperio_tx_fifo_size = (gnptxfsiz & FIFOSIZE_DEPTH_MASK) >>
+  FIFOSIZE_DEPTH_SHIFT;
+   hw->host_perio_tx_fifo_size = (hptxfsiz & FIFOSIZE_DEPTH_MASK) >>
+ FIFOSIZE_DEPTH_SHIFT;
+}
+
+/*
+ * Gets device hardware parameters. Forces device mode if not
+ * currently in device mode. Should be called immediately after a core
+ * soft reset in order to get the reset values.
+ */
+static void dwc2_get_dev_hwparams(struct dwc2_hsotg *hsotg)
+{
+   struct dwc2_hw_params *hw = >hw_params;
+   bool forced;
+   u32 gnptxfsiz;
+
+   if (hsotg->dr_mode == USB_DR_MODE_HOST)
+   return;
+
+   forced = dwc2_force_device_if_needed(hsotg);
+
+   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
+   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
+
+   if (forced)
+   dwc2_clear_force_mode(hsotg);
+
+   hw->dev_nperio_tx_fifo_size = (gnptxfsiz & FIFOSIZE_DEPTH_MASK) >>
+  FIFOSIZE_DEPTH_SHIFT;
+}
+
 /**
  * During device initialization, read various hardware configuration
  * registers and interpret the contents.
+ *
+ * This should be called during driver probe. It will perform a core
+ * soft reset in order to get the reset values of the parameters.
  */
 int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
 {
struct dwc2_hw_params *hw = >hw_params;
unsigned width;
u32 hwcfg1, hwcfg2, hwcfg3, hwcfg4;
-   u32 hptxfsiz, grxfsiz, gnptxfsiz;
-   u32 gusbcfg = 0;
+   u32 grxfsiz;
int retval;
 
/*
@@ -3206,23 +3265,6 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
dev_dbg(hsotg->dev, "hwcfg4=%08x\n", hwcfg4);
dev_dbg(hsotg->dev, "grxfsiz=%08x\n", grxfsiz);
 
-   /* Force host mode to get HPTXFSIZ / GNPTXFSIZ exact power on value */
-   if (hsotg->dr_mode != USB_DR_MODE_HOST) {
-   gusbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
-   dwc2_writel(gusbcfg | GUSBCFG_FORCEHOSTMODE,
-   hsotg->regs + GUSBCFG);
-   usleep_range(25000, 5);
-   }
-
-   gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
-   hptxfsiz = dwc2_readl(hsotg->regs + HPTXFSIZ);
-   dev_dbg(hsotg->dev, "gnptxfsiz=%08x\n", gnptxfsiz);
-   dev_dbg(hsotg->dev, "hptxfsiz=%08x\n", hptxfsiz);
-   if (hsotg->dr_mode != USB_DR_MODE_HOST) {
-   dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
-   usleep_range(25000, 5);
-   }
-
/* hwcfg2 */
hw->op_mode = (hwcfg2 & GHWCFG2_OP_MODE_MASK) >>
  GHWCFG2_OP_MODE_SHIFT;
@@ -3270,10 +3312,15 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
/* fifo sizes */
hw->host_rx_fifo_size = (grxfsiz & GRXFSIZ_DEPTH_MASK) >>
GRXFSIZ_DEPTH_SHIFT;
-   hw->host_nperio_tx_fifo_size = (gnptxfsiz & FIFOSIZE_DEPTH_MASK) >>
-  FIFOSIZE_DEPTH_SHIFT;
-   hw->host_perio_tx_fifo_size = (hptxfsiz & FIFOSIZE_DEPTH_MASK) >>
- FIFOSIZE_DEPTH_SHIFT;
+
+   /*
+* Host/device specific hardware parameters. Reading these
+* parameters requires the controller to be in either host or
+* device mode. The mode will be forced, if necessary, to read
+* these values.
+