Hi, On Thu, Feb 21, 2013 at 03:34:56AM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:[email protected]] > > Sent: Wednesday, February 20, 2013 12:50 AM > > > > On Tue, Feb 19, 2013 at 06:50:07PM -0800, Paul Zimmerman wrote: > > > This file contains the PCI bus interface "glue" for the DWC2 > > > driver > > > > > > Signed-off-by: Paul Zimmerman <[email protected]> > > > > before anything, out of curiosity, do you have DWC2 PCI configured with > > OTG support ? I remember that on HAPS6x configuring DWC3 PCI OTG would > > take too much space on the FPGA and that would cause difficulties on > > timing closure, but perhaps dwc2 is smaller ?!? > > Yes, our standard HSOTG FPGA image is configured with OTG support. I'm > not sure how difficult it is to synthesize, that work is done by a team on > another continent ;)
cool, guess USB2 is far smaller IP than USB3 :-)
> > > +static void dwc2_driver_remove(struct pci_dev *dev)
> > > +{
> > > + struct dwc2_device *otg_dev = pci_get_drvdata(dev);
> > > +
> > > + dev_dbg(&dev->dev, "%s(%p)\n", __func__, dev);
> > > +
> > > + if (!otg_dev) {
> > > + dev_dbg(&dev->dev, "%s: otg_dev NULL!\n", __func__);
> > > + return;
> > > + }
> >
> > this check can be removed, drivers core guarantees that pci_dev *dev
> > will always be true and, since you clearly and unconditionally call
> > pci_set_drvdata(), otg_dev will always be valid.
>
> This function is called as part of the failure path from dwc2_driver_probe(),
> and it's possible that pci_set_drvdata() has not been called yet.
that means you're calling it at the wrong time ;-)
> > > + dev_dbg(&dev->dev, "otg_dev->hsotg = %p\n", otg_dev->hsotg);
> > > + if (otg_dev->hsotg)
> > > + dwc2_hcd_remove(&dev->dev, otg_dev);
> > > + else
> > > + dev_dbg(&dev->dev, "%s: otg_dev->hsotg NULL!\n", __func__);
> >
> > this check can be removed I guess. You shouldn't be able to remove what
> > wasn't registered.
>
> Ditto.
same as above. If both checks fail, this function does nothing. Seems
like you're calling it at the wrong location.
> > > + /* Map the DWC_otg Core memory into virtual address space */
> > > + dev->current_state = PCI_D0;
> > > + dev->dev.power.power_state = PMSG_ON;
> >
> > no no no... you shouldn't access these directly.
> >
> > Plese use pci_set_state(dev, PCI_D0) and
> > dev_pm_qos_constraints_init(&dev->dev);
>
> I will switch to pci_set_power_state(dev, PCI_D0), I guess that's what
> you meant? I can't get dev_pm_qos_constraints_init() to compile, so
> I think I'll leave that out for now. I don't have any PM support in the
> driver yet, anyway.
cool, then leave out PMSG_ON too ;-)
> For the rest of your comments I will implement the changes that
> you suggested. Thanks.
thanks
--
balbi
signature.asc
Description: Digital signature
