Hi, On Tue, Feb 12, 2013 at 12:01:28AM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:[email protected]] > > Sent: Monday, February 11, 2013 5:43 AM > > Hi Felipe, > > I'm only responding to the things I want to clarify here, the stuff that > I don't respond to you can consider as "OK, I'll do that". > > > On Sat, Feb 09, 2013 at 07:37:48PM -0800, Paul Zimmerman wrote: > > > +static void dwc2_core_reset(struct dwc2_hcd *hcd) > > > > not sure if I would call this an HCD. Surely it can _be_ an HCD, but > > it's actually a DRD device (not all configurations, but still). > > > > If you call this HCD, it will be a little misleading when peripheral > > mode is merged here IMO. How about: > > > > sed -i 's/dwc2_hcd\s\*\(\w\+\)/dwc2 *dwc/g' ? > > That sed expression doesn't seem to do anything here. Mind > explaining what you meant in English?
rename struct dwc2_hcd to struct dwc and *hcd to *dwc.
> I see s3c-hsotg uses 'struct s3c_hsotg'. How about if I use
> 'struct dwc2_hsotg'? Then when we move that driver here we
sure that would be alright, makes sense.
> > > +static void dwc2_phy_init(struct dwc2_hcd *hcd)
> > > +{
> > > + u32 usbcfg, i2cctl, hs_phy_type, fs_phy_type;
> > > +
> > > + if (hcd->core_params->speed == DWC2_SPEED_PARAM_FULL &&
> > > + hcd->core_params->phy_type == DWC2_PHY_TYPE_PARAM_FS) {
> > > + /* If FS mode with FS PHY */
> > > + /*
> > > + * core_init() is now called on every switch so only call the
> > > + * following for the first time through
> > > + */
> > > + if (!hcd->phy_init_done) {
> > > + hcd->phy_init_done = 1;
> >
> > I wonder if this flag is really necessary. Seems like you can easily
> > make sure that this doesn't get called twice.
>
> Well, according to the comment, it gets called on every role switch,
> so I don't see how.
right, but does it have to ? Wouldn't it be enough to initialize PHYs
during probe() ?
> > > + /* Reset after setting the PHY parameters */
> > > + dwc2_core_reset(hcd);
> >
> > since this is called on both branches, I guess you could factor it out
> > of the branches, no ?
>
> Not really. In one case things are done after the reset, in the other
> not.
I see, my bad.
> > not really needed right now, but you should start considering dma engine
> > support.
>
> I thought that was only for generic DMA engines? This one is only
> used in the HSOTG controller.
right, but you can also have external DMAs. But, as I said, we can
tackle that headache later ;-)
> > > +static void dwc2_gusbcfg_init(struct dwc2_hcd *hcd)
> > > +{
> > > + u32 usbcfg;
> > > +
> > > + usbcfg = readl(hcd->regs + GUSBCFG);
> > > + usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP);
> > > +
> > > + switch (hcd->hwcfg2 & GHWCFG2_OP_MODE_MASK) {
> > > + case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE:
> > > + if (hcd->core_params->otg_cap == DWC2_CAP_PARAM_HNP_SRP_CAPABLE)
> >
> > I'm assuming core_params is passed via module parameter, if that's the
> > case, then I'm not sure you need a module parameter. You could be using
> > DT/platform_data since this is board/platform-specific and won't change
> > in runtime.
> >
> > If you expose to a module parameter, then a not-so-careful user might
> > start reporting bugs which don't exist just because he didn't know how
> > to pass the proper module parameter.
>
> See my response to 0/5 about the FPGA development board. For a
> production platform this would be set via DT or platform_data.
you can't set those parameters via DT, but I see what you mean.
> > > +/**
> > > + * dwc2_core_init() - Initializes the DWC_otg controller registers and
> > > + * prepares the core for device mode or host mode operation
> > > + *
> > > + * @hcd: Programming view of the DWC_otg controller
> > > + */
> > > +int dwc2_core_init(struct dwc2_hcd *hcd)
> >
> > is this still being called by the PCI layer ? On your cover letter you
> > said they're now two separate drivers, if that's the case, this should
> > be static and called on this driver's probe().
>
> The core is a separate module, not a separate driver. It doesn't have a
> probe() function.
>
> > > + /* Clear the SRP success bit for FS-I2c */
> > > + hcd->srp_success = 0;
> >
> > wasn't 'hcd' allocated with kzalloc ? (btw, would be nice if you were
> > using devm_* API all over this driver, where possible).
>
> This function is called on every role switch, not just at init time. So
> kzalloc is not enough.
fair enough.
> I am using devm_* in the bus glue code. I don't think I can use it
> here, because this is library code and not a driver?
right.
> > Also, this function seems to be dealing with PIO only, so how about
> > appending the name with _pio to make that clear ?
>
> No, if you read the header comments you will see this is used in both
> slave mode and buffer DMA mode. Descriptor DMA mode is handled
> by a different function (dwc2_hc_start_transfer_ddma).
>
> I thnk dwc2_hc_start_transfer_slave_or_bufdma() is a little too
> wordy.
fair enough :-)
> > > + /* Wait for at least 3 PHY Clocks */
> > > + udelay(1);
> >
> > could you use usleep_range() here ?
>
> This is a udelay, not a usleep, because of interrupt context. I don't
> think there is a udelay_range(), is there?
no there isn't. Should've followed the function calls better.
> > > + /*
> > > + * Need to schedule a work, as there are possible DELAY function calls.
> > > + * Release lock before scheduling workq as it holds spinlock during
> > > + * scheduling.
> > > + */
> > > + spin_unlock(&hcd->lock);
> > > + queue_work(hcd->wq_otg, &hcd->wf_otg);
> >
> > please don't, convert to devm_request_threaded_irq() and run your IRQ
> > handler on a thread.
> >
> > Ideally, you should use hardirq solely to check if this device generated
> > the IRQ (yes, I know dwc3 isn't doing that, I have plans to fix it but
> > have other more important stuff to do first ;-)
>
> I would rather not do that at this point if you don't mind. It seems like
> a pretty large change, and I would like to get a working driver into the
> kernel fairly soon.
your call, I guess...
--
balbi
signature.asc
Description: Digital signature
