On 12/9/2015 8:45 PM, Doug Anderson wrote:
> John,
>
> On Wed, Dec 2, 2015 at 11:15 AM, John Youn <John.Youn@xxxxxxxxxxxx> 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 <johnyoun@xxxxxxxxxxxx>
>> ---
>> 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, 50000);
>> }
>>
>> +/*
>> + * 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 = &hsotg->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 = &hsotg->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 = &hsotg->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, 50000);
>> - }
>> -
>> - 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, 50000);
>> - }
>> -
>
> 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 / etc could happen in
> the same relative place that they happened before.
I'll do this also.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
