> From: Felipe Balbi [mailto:balbi@xxxxxx] > 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? I see s3c-hsotg uses 'struct s3c_hsotg'. How about if I use 'struct dwc2_hsotg'? Then when we move that driver here we can do 's/struct s3c_hsotg/struct dwc2_hsotg/' and combine the structure members. > > +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. > > + /* > > + * Program DCFG.DevSpd or HCFG.FSLSPclkSel to 48Mhz in FS. Also > > + * do this on HNP Dev/Host mode switches (done in dev_init and > > + * host_init). > > + */ > > + if (dwc2_is_host_mode(hcd)) > > + dwc2_init_fs_ls_pclk_sel(hcd); > > + > > + if (hcd->core_params->i2c_enable > 0) { > > seems like you could read this out of a register instead of using module > parameter, no ? Not sure, I will need to check. > > + /* 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. > 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. > > +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. > > +/** > > + * 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. 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? > > + /* Enable common interrupts */ > > + dwc2_enable_common_interrupts(hcd); > > before this call, there should be a request_irq() (preferrably a > devm_request_irq()). That is done by the bus glue code. > > +void dwc2_hc_start_transfer(struct dwc2_hcd *hcd, struct dwc2_hc *hc) > > dwc2_hc is a bit too short, IMO. I didn't know, at first, if HC meant > Host Controller or Host Channel (apparently it's Host Channel), so how > about: > > sed -i 's/dwc2_hc *hc/dwc2_host_channel *channel/g' * Good point, that confused me at first too ;) I'll change it. > 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. > > + /* 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? > Frankly, I'm still not a fan of the amount of exposed functions you have > on this driver, but this is your headache, so I won't bother anymore. I good ;) > have already tried to explain why I think following what we did on dwc3 > pays off in the long run... > > Anyway, what I want to say here is: > > can you at least use EXPORT_SYMBOL_GPL(), or does that taint this same > driver because it's Dual BSD/GPL ? I don't know, I will have have to check. > > +/* Macros defined for DWC OTG HW Release version */ > > +#define DWC2_CORE_REV_2_60a 0x4f54260a > > +#define DWC2_CORE_REV_2_71a 0x4f54271a > > +#define DWC2_CORE_REV_2_72a 0x4f54272a > > +#define DWC2_CORE_REV_2_80a 0x4f54280a > > +#define DWC2_CORE_REV_2_81a 0x4f54281a > > +#define DWC2_CORE_REV_2_90a 0x4f54290a > > +#define DWC2_CORE_REV_2_91a 0x4f54291a > > +#define DWC2_CORE_REV_2_92a 0x4f54292a > > +#define DWC2_CORE_REV_2_93a 0x4f54293a > > +#define DWC2_CORE_REV_2_94a 0x4f54294a > > +#define DWC2_CORE_REV_3_00a 0x4f54300a > > I guess it would be best to define these closer to where they're used. > Note that we have defined them inside struct dwc3 on the other driver. > > It makes it easier for the silly human reading the code to note that > these are the supported revisions. Yeah. Actually only 5 of these are used in the driver, so I will delete the rest. > > + /* > > + * 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. -- Paul -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
