[PATCH v2 39/46] usb: dwc2: Fix not calling dev_xxx with a device

Sean Anderson
2020-09-16T13:43:51Z

On 9/16/20 9:30 AM, Patrick DELAUNAY wrote:
> Hi Sean,
>
>> From: Sean Anderson <seanga2@gmail.com>
>> Sent: mardi 15 septembre 2020 16:45
>>
>> This adds a dev argument to some functions so dev_xxx always has a device to
>> log with. In one instance we must use use a different log function when we are
>> compiled without DM_USB.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> Changes in v2:
>> - New
>>
>> drivers/usb/host/dwc2.c | 39 +++++++++++++++++++++++----------------
>> 1 file changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index
>> cefe9d83b1..f1d13b1c1d 100644
>> --- a/drivers/usb/host/dwc2.c
>> +++ b/drivers/usb/host/dwc2.c
>> @@ -114,7 +114,8 @@ static void init_fslspclksel(struct dwc2_core_regs *regs)
>> * @param regs Programming view of DWC_otg controller.
>> * @param num Tx FIFO to flush.
>> */
>> -static void dwc_otg_flush_tx_fifo(struct dwc2_core_regs *regs, const int num)
>> +static void dwc_otg_flush_tx_fifo(struct udevice *dev,
>> + struct dwc2_core_regs *regs, const int num)
>> {
>> int ret;
>>
>> @@ -134,7 +135,8 @@ static void dwc_otg_flush_tx_fifo(struct dwc2_core_regs
>> *regs, const int num)
>> *
>> * @param regs Programming view of DWC_otg controller.
>> */
>> -static void dwc_otg_flush_rx_fifo(struct dwc2_core_regs *regs)
>> +static void dwc_otg_flush_rx_fifo(struct udevice *dev,
>> + struct dwc2_core_regs *regs)
>> {
>> int ret;
>>
>> @@ -152,7 +154,8 @@ static void dwc_otg_flush_rx_fifo(struct dwc2_core_regs
>> *regs)
>> * Do core a soft reset of the core. Be careful with this because it
>> * resets all the internal state machines of the core.
>> */
>> -static void dwc_otg_core_reset(struct dwc2_core_regs *regs)
>> +static void dwc_otg_core_reset(struct udevice *dev,
>> + struct dwc2_core_regs *regs)
>> {
>> int ret;
>>
>> @@ -284,8 +287,8 @@ static void dwc_otg_core_host_init(struct udevice *dev,
>> clrbits_le32(&regs->gotgctl, DWC2_GOTGCTL_HSTSETHNPEN);
>>
>> /* Make sure the FIFOs are flushed. */
>> - dwc_otg_flush_tx_fifo(regs, 0x10); /* All Tx FIFOs */
>> - dwc_otg_flush_rx_fifo(regs);
>> + dwc_otg_flush_tx_fifo(dev, regs, 0x10); /* All Tx FIFOs */
>> + dwc_otg_flush_rx_fifo(dev, regs);
>>
>> /* Flush out any leftover queued requests. */
>> num_channels = readl(&regs->ghwcfg2);
>> @@ -306,7 +309,7 @@ static void dwc_otg_core_host_init(struct udevice *dev,
>> ret = wait_for_bit_le32(&regs->hc_regs[i].hcchar,
>> DWC2_HCCHAR_CHEN, false, 1000,
>> false);
>> if (ret)
>> - dev_info("%s: Timeout!\n", __func__);
>> + dev_info(dev, "%s: Timeout!\n", __func__);
>> }
>>
>> /* Turn on the vbus power. */
>> @@ -330,8 +333,9 @@ static void dwc_otg_core_host_init(struct udevice *dev,
>> *
>> * @param regs Programming view of the DWC_otg controller
>> */
>> -static void dwc_otg_core_init(struct dwc2_priv *priv)
>> +static void dwc_otg_core_init(struct udevice *dev)
>> {
>> + struct dwc2_priv *priv = dev_get_priv(dev);
>> struct dwc2_core_regs *regs = priv->regs;
>> uint32_t ahbcfg = 0;
>> uint32_t usbcfg = 0;
>> @@ -360,7 +364,7 @@ static void dwc_otg_core_init(struct dwc2_priv *priv)
>> writel(usbcfg, &regs->gusbcfg);
>>
>> /* Reset the Controller */
>> - dwc_otg_core_reset(regs);
>> + dwc_otg_core_reset(dev, regs);
>>
>> /*
>> * This programming sequence needs to happen in FS mode before @@ -
>> 372,7 +376,7 @@ static void dwc_otg_core_init(struct dwc2_priv *priv)
>> setbits_le32(&regs->gusbcfg, DWC2_GUSBCFG_PHYSEL);
>>
>> /* Reset after a PHY select */
>> - dwc_otg_core_reset(regs);
>> + dwc_otg_core_reset(dev, regs);
>>
>> /*
>> * Program DCFG.DevSpd or HCFG.FSLSPclkSel to 48Mhz in FS.
>> @@ -419,7 +423,7 @@ static void dwc_otg_core_init(struct dwc2_priv *priv)
>> writel(usbcfg, &regs->gusbcfg);
>>
>> /* Reset after setting the PHY parameters */
>> - dwc_otg_core_reset(regs);
>> + dwc_otg_core_reset(dev, regs);
>> #endif
>>
>> usbcfg = readl(&regs->gusbcfg);
>> @@ -1128,7 +1132,12 @@ int _submit_int_msg(struct dwc2_priv *priv, struct
>> usb_device *dev,
>> timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
>> for (;;) {
>> if (get_timer(0) > timeout) {
>> - dev_err(dev, "Timeout poll on interrupt endpoint\n");
>> +#if CONFIG_IS_ENABLED(DM_USB)
>> + dev_err(dev->dev,
>> + "Timeout poll on interrupt endpoint\n"); #else
>> + log_err("Timeout poll on interrupt endpoint\n"); #endif
>> return -ETIMEDOUT;
>> }
>> ret = _submit_bulk_msg(priv, dev, pipe, buffer, len); @@ -1194,7
>> +1203,7 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv
>> *priv)
>> priv->ext_vbus = 0;
>> #endif
>>
>> - dwc_otg_core_init(priv);
>> + dwc_otg_core_init(dev);
>> dwc_otg_core_host_init(dev, regs);
>>
>> clrsetbits_le32(&regs->hprt0, DWC2_HPRT0_PRTENA | @@ -1320,12
>> +1329,10 @@ static int dwc2_submit_int_msg(struct udevice *dev, struct
>> usb_device *udev, static int dwc2_usb_ofdata_to_platdata(struct udevice *dev) {
>> struct dwc2_priv *priv = dev_get_priv(dev);
>> - fdt_addr_t addr;
>>
>> - addr = dev_read_addr(dev);
>> - if (addr == FDT_ADDR_T_NONE)
>> + priv->regs = dev_read_addr_ptr(dev);
>> + if (!priv->regs)
>> return -EINVAL;
>> - priv->regs = (struct dwc2_core_regs *)addr;
>>
>> priv->oc_disable = dev_read_bool(dev, "disable-over-current");
>> priv->hnp_srp_disable = dev_read_bool(dev, "hnp-srp-disable");
>> --
>> 2.28.0
>
>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>
>
> I had just a concern about the addition of dev parameter when regs exist in
> dwc_otg_flush_rx_fifo, dwc_otg_flush_rx_fifo and dwc_otg_core_reset
>
> struct udevice *dev,
> struct dwc2_core_regs *regs,
>
> as regs can be found in dev, I think that duplicate the parameter without need....
>
> struct dwc2_priv *priv = dev_get_priv(dev);
> struct dwc2_core_regs *regs = priv->regs;
>
> but it is also the case in existing function, for example :
>
> dwc_otg_core_host_init(dev, regs);

So if I understand correctly, you would you prefer something like

-static void dwc_otg_flush_rx_fifo(struct dwc2_core_regs *regs)
+static void dwc_otg_flush_rx_fifo(struct udevice *dev)
{
int ret;
+ struct dwc2_priv *priv = dev_get_priv(dev);
+ struct dwc2_core_regs *regs = priv->regs;

--Sean