Re: [PATCH 1/3] usb: dwc2: Add support for STM32F429/439/469 USB OTG in FS mode with internal PHY

[John Youn <John.Youn@synopsys.com>]

On 6/29/2016 12:47 PM, Bruno Herrera wrote:
> On Mon, Jun 27, 2016 at 7:51 PM, John Youn <John.Youn@synopsys.com> wrote:
>> On 6/21/2016 7:26 PM, Bruno Herrera wrote:
>>> Signed-off-by: Bruno Herrera <bruherrera@gmail.com>
>>
>> Please add a commit message describing the purpose of your changes,
>> some information about the platform you're adding, and the special
>> handling of the GGPIO.
>>
> Ok, no problem.
> 
>>> ---
>>>  drivers/usb/dwc2/core.c     | 18 ++++++++++++++++++
>>>  drivers/usb/dwc2/core.h     |  5 +++++
>>>  drivers/usb/dwc2/hcd.c      | 12 +++++++++++-
>>>  drivers/usb/dwc2/hw.h       |  2 ++
>>>  drivers/usb/dwc2/platform.c | 37 +++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 73 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
>>> index 4135a5f..83fbed6 100644
>>> --- a/drivers/usb/dwc2/core.c
>>> +++ b/drivers/usb/dwc2/core.c
>>> @@ -1276,6 +1276,23 @@ static void dwc2_set_param_hibernation(struct dwc2_hsotg *hsotg,
>>>       hsotg->core_params->hibernation = val;
>>>  }
>>>
>>> +static void dwc2_set_param_stm32_powerdown(struct dwc2_hsotg *hsotg,
>>> +             int val)
>>> +{
>>> +     if (DWC2_OUT_OF_BOUNDS(val, 0, 1)) {
>>> +             if (val >= 0) {
>>> +                     dev_err(hsotg->dev,
>>> +                             "'%d' invalid for parameter power down\n",
>>> +                             val);
>>> +                     dev_err(hsotg->dev, "power down must be 0 or 1\n");
>>> +             }
>>> +             val = 0;
>>> +             dev_dbg(hsotg->dev, "Setting power down to %d\n", val);
>>> +     }
>>> +
>>> +     hsotg->core_params->stm32_powerdown = val;
>>> +}
>>> +
>>>  /*
>>>   * This function is called during module intialization to pass module parameters
>>>   * for the DWC_otg core.
>>> @@ -1323,6 +1340,7 @@ void dwc2_set_parameters(struct dwc2_hsotg *hsotg,
>>>       dwc2_set_param_uframe_sched(hsotg, params->uframe_sched);
>>>       dwc2_set_param_external_id_pin_ctl(hsotg, params->external_id_pin_ctl);
>>>       dwc2_set_param_hibernation(hsotg, params->hibernation);
>>> +     dwc2_set_param_stm32_powerdown(hsotg, params->stm32_powerdown);
>>>  }
>>>
>>>  /*
>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>> index 3c58d63..d3e4fcb 100644
>>> --- a/drivers/usb/dwc2/core.h
>>> +++ b/drivers/usb/dwc2/core.h
>>> @@ -386,6 +386,10 @@ enum dwc2_ep0_state {
>>>   *                   needed.
>>>   *                   0 - No (default)
>>>   *                   1 - Yes
>>> + * @stm32_powerdown: Enable STM32 specific USB FS transceiver power down
>>> + *                   control.
>>> + *                   0 = USB FS transceiver disabled (default)
>>> + *                   1 = USB FS transceiver enabled
>>>   *
>>>   * The following parameters may be specified when starting the module. These
>>>   * parameters define how the DWC_otg controller should be configured. A
>>> @@ -426,6 +430,7 @@ struct dwc2_core_params {
>>>       int uframe_sched;
>>>       int external_id_pin_ctl;
>>>       int hibernation;
>>> +     int stm32_powerdown;
>>>  };
>>>
>>>  /**
>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>> index 2df3d04..4f9bb93 100644
>>> --- a/drivers/usb/dwc2/hcd.c
>>> +++ b/drivers/usb/dwc2/hcd.c
>>> @@ -118,7 +118,7 @@ static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg)
>>>
>>>  static int dwc2_fs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy)
>>>  {
>>> -     u32 usbcfg, i2cctl;
>>> +     u32 usbcfg, usbgpio, i2cctl;
>>
>> The convention in this driver would be to just call 'usbgpio' -> 'ggpio'
> 
> OK.
>>
>>>       int retval = 0;
>>>
>>>       /*
>>> @@ -142,6 +142,16 @@ static int dwc2_fs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy)
>>>                               return retval;
>>>                       }
>>>               }
>>> +
>>> +             if (hsotg->core_params->stm32_powerdown > 0) {
>>> +                     dev_dbg(hsotg->dev, "STM32 FS PHY enabling transceiver\n");
>>> +                     /* STM32 uses the GGPIO register as general core
>>> +                      * configuration register.
>>> +                      */
>>> +                     usbgpio = dwc2_readl(hsotg->regs + GGPIO);
>>> +                     usbgpio |= STM32_OTG_GCCFG_PWRDWN;
>>> +                     dwc2_writel(usbgpio, hsotg->regs + GGPIO);
>>> +             }
>>>       }
>>>
>>>       /*
>>> diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
>>> index 281b57b..d5f9294 100644
>>> --- a/drivers/usb/dwc2/hw.h
>>> +++ b/drivers/usb/dwc2/hw.h
>>> @@ -224,6 +224,8 @@
>>>
>>>  #define GPVNDCTL                     HSOTG_REG(0x0034)
>>>  #define GGPIO                                HSOTG_REG(0x0038)
>>> +#define STM32_OTG_GCCFG_PWRDWN               (1 << 16)
>>> +
>>
>> Also, bitfields for GGPIO should be named: GGPIO_xxx
> 
> Could it be -> GGPIO_STM32_OTG_GCCFG_PWRDWN
> From my understand the GGPIO are platform/vendor dependent so in the
> future some other vendor could use the same bit for other purpose.
> Should we keep it defined here or only in the scope it is used?

Here is fine. We already have overlapping bitfield definitions for
some registers due to differences in host or device mode.

Regards,
John