Hi,
> From: Sean Anderson <sean...@gmail.com>
> Sent: mercredi 16 septembre 2020 15:44
>
> On 9/16/20 9:30 AM, Patrick DELAUNAY wrote:
> > Hi Sean,
> >
> >> From: Sean Anderson <sean...@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 <sean...@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(®s->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(®s->ghwcfg2); @@ -306,7 +309,7 @@ static
> >> void dwc_otg_core_host_init(struct udevice *dev,
> >> ret = wait_for_bit_le32(®s->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, ®s->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(®s->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, ®s->gusbcfg);
> >>
> >> /* Reset after setting the PHY parameters */
> >> - dwc_otg_core_reset(regs);
> >> + dwc_otg_core_reset(dev, regs);
> >> #endif
> >>
> >> usbcfg = readl(®s->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(®s->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.delau...@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
Yes exactly.
Because I assume that dev_get_priv() is just a macro to access dev element,
so my proposal don't increase the code size / the execution time.
But I check it yesterday, because I start to migrate all stm32 driver to log
API and it is not the case...
drivers/core/device.c:545
void *dev_get_priv(const struct udevice *dev)
{
if (!dev) {
dm_warn("%s: null device\n", __func__);
return NULL;
}
return dev->priv;
}
So now, I don't sure that duplicate the call to dev_get_priv() is a good
solution.
And I think your first solution is correct.
Sorry for disturbance
Patrick
