> From: Felipe Balbi [mailto:balbi@xxxxxx] > Sent: Friday, January 11, 2013 1:44 AM > > Hi, Hi Felipe, thanks for the review. > On Mon, Jan 07, 2013 at 11:51:36AM -0800, Paul Zimmerman wrote: > > > > Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> > > no commit log == no commit ;-) Will fix next time. > Also, you shouldn't have reset the version counter. 2 years ago this > same driver was already on v17, then someone else tried and did a few > extra versions... I meant to just remove the version number, will do that in the next revision. > ... There are a few comments below, probably should be > more but all your files are so big that I just got bored, sorry. Sorry. I know it's a freaking huge driver, but this core has a lot of optional features, three DMA modes, yadda yadda. I actually removed code for supporting several more recent features before submitting this! Would it help if I did one patch per file? By the way, I probably should have mentioned this originally. I did not really write this driver. I took our vendor driver that was written by another team, and modified it so it would at least have a chance of being acceptable for the kernel. So I replaced the proprietary license with BSD/GPL, removed the portability abstraction layer, and fixed up the coding and comment style. I did fix some logic problems along the way. And I did become somewhat familiar with how the driver works while fixing the bugs I introduced while modifying it. But I am far from being an expert on how it all works. > I would like it much better if you would follow what we did for dwc3 and > have core IP in a separate driver with pci/exynos/whatever glue in a > parent device driver. Still, if you really want to make core IP driver a > library, then at a minimum do it right, don't expose ALL functions, make > sure you have a small set of APIs for the glue to call. Encapsulate > implementation details from your users, they really don't need to call > each function separately. Ideally pci glue would call dwc2_initialize() > on probe() and that's it. Once we get to the point of integrating another interface, like for a platform driver, I plan to make the core code a separate module, so the code can be shared instead of being duplicated in each driver. But we're not quite there yet. But I disagree with the "small set of APIs" thing. It would be a HUGE amount of effort to rewrite the code like that. The reason the code is in separate files is to give it a somewhat logical arrangement, and to avoid having one gargantuan source file. There is no intention to have individual APIs for each of the files. I disagree that there are any external "users" like you mention above, it is all one driver. As a global response to your complaints about all the debug statements: I said in the first email in the series that I would like to keep those for the time being, until we have the s3c-hsotg driver integrated and there are no major issues remaining, and then strip them all out. I found them _very_ useful for catching my mistakes after doing the initial conversion from the vendor driver, and I am sure they will be needed once we start integrating the s3c-hsotg code. > Anyway, more comments below. > > > +void dwc2_enable_global_interrupts(struct dwc2_hcd *hcd) > > +{ > > + u32 ahbcfg = readl(hcd->regs + GAHBCFG); > > + > > + ahbcfg |= GAHBCFG_GlblIntrEn; > > + writel(ahbcfg, hcd->regs + GAHBCFG); > > personally, I would add private static inline wrappers around writel() > and readl(), but not a big deal if you want to use directly. I think I will leave it as is, unless you have a concrete reason for having a wrapper? > > +static void enable_common_interrupts(struct dwc2_hcd *hcd) > > even though it's a static function, I still like to see consistency on > function names, please prepend this one too with dwc2_. I would rather not. There are a heap of static functions in all of these files, and it seems pointless to rename them all for no good reason other than style. > > + intmsk |= GINTSTS_ConIDStsChng; > > + intmsk |= GINTSTS_WkUpInt; > > + intmsk |= GINTSTS_USBSusp; > > + intmsk |= GINTSTS_SessReqInt; > > no CaMeLcAsE in kernel code. Make it all upper case for the defines. We are reusing s3c-hsotg.h for a lot of the register definitions, so I can't change this without breaking that driver. Once this driver is integrated with the s3c-hsotg driver, we can make a cleanup patch and fix them all. > > +/* > > + * Initializes the FSLSPClkSel field of the HCFG register depending on the > > + * PHY type > > + */ > > +static void init_fslspclksel(struct dwc2_hcd *hcd) > > this name can be improved. Sure, no problem. > > + if (((hcd->hwcfg2 & GHWCFG2_HS_PHY_TYPE_MASK) == > > + GHWCFG2_HS_PHY_TYPE_ULPI && > > + (hcd->hwcfg2 & GHWCFG2_FS_PHY_TYPE_MASK) == > > + GHWCFG2_FS_PHY_TYPE_DEDICATED && > > + hcd->core_params->ulpi_fs_ls > 0) || > > + hcd->core_params->phy_type == DWC_PHY_TYPE_PARAM_FS) { > > + /* Full speed PHY */ > > + val = HCFG_FSLSPCLKSEL_48_MHZ; > > + } else { > > + /* High speed PHY running at full speed or high speed */ > > + val = HCFG_FSLSPCLKSEL_30_60_MHZ; > > + } > > how about: > > phy_type = hcd->hwcfg2 & GHWCFG2_HS_PHY_TYPE_MASK; > switch(phy_type) { > case GHWCFG2_HS_PHY_TYPE_ULPI: > case GHWCFG2_HS_PHY_TYPE_DEDICATED: > foo(); > break; > default: > bar(); > } OK. > + /* Wait for AHB master IDLE state */ > > + do { > > + msleep(20); > > + greset = readl(hcd->regs + GRSTCTL); > > + if (++count > 500) { > > hmm... 10 seconds waiting ? No no no... maybe you meant usleep above ? > If that's the case, use usleep_range(1000, 5000) or something similar. No, I meant msleep. I agree 10 seconds is pretty excessive, I will reduce it to something sane. > > + do { > > + msleep(20); > > + greset = readl(hcd->regs + GRSTCTL); > > + if (++count > 500) { > > 10 more seconds ?!? wow ditto. > > + /* > > + * NOTE: This long sleep is _very_ important, otherwise the core will > > + * not stay in host mode after a connector ID change! > > + */ > > + msleep(100); > > this comment needs further explanation, badly :-) I know, but I don't know the reason :) See "I didn't really write this driver" above. > > + /* > > + * This programming sequence needs to happen in FS mode before any other > > + * programming occurs > > + */ > > why ? Ditto. > > + if ((hcd->hwcfg2 & GHWCFG2_HS_PHY_TYPE_MASK) == > > + GHWCFG2_HS_PHY_TYPE_ULPI && > > + (hcd->hwcfg2 & GHWCFG2_FS_PHY_TYPE_MASK) == > > + GHWCFG2_FS_PHY_TYPE_DEDICATED && hcd->core_params->ulpi_fs_ls > 0) { > > switch ? Sure. > > + case GHWCFG2_INT_DMA_ARCH: > > + dev_dbg(hcd->dev, "Internal DMA Mode\n"); > > + /* > > + * Old value was GAHBCFG_HBstLen_Incr - done for > > + * Host mode ISOC in issue fix - vahrama > > + */ > > + ahbcfg &= ~GAHBCFG_HBstLen_MASK; > > + ahbcfg |= GAHBCFG_HBstLen_Incr4; > > + hcd->dma_enable = hcd->core_params->dma_enable > 0; > > + hcd->dma_desc_enable = hcd->core_params->dma_desc_enable > 0; > > + break; > > if you can have internal and external DMA support, you ought to use > dmaengine API. In case of internal DMA, make this driver register a dma > engine device and request it back. I will need to look into that, I'm not familiar with the dmaengine API. > > + /* Set OTG version supported */ > > + hcd->otg_ver = hcd->core_params->otg_ver > 0; > > comment is wrong, right hand side will evaluate to true or false, not > the OTG version supported. Will fix. > BTW, what a HUGE function... You can surely refactor parts of it to > other functions which are called by this one. Also, I don't think driver > initialization should be exposed, the only reason for that is that > you're not giving the core driver its own platform_device and, believe > me, you will be hurt by this choice. > > We have gone through that before, giving core driver its own > platform_device will make it a lot easier to "hide" platform details on > a parent platform_device, just like Sebastian and I did for dwc3. Sorry, after trying to add some features to the dwc3 driver, I became convinced that making the core code a separate platform device is not something I want to do. As I said, I will make it a separate module once we have a second bus interface to connect. I will look at refactoring the function, though. > > +/** > > + * dwc2_enable_host_interrupts() - Enables the Host mode interrupts > > + * > > + * @hcd: Programming view of DWC_otg controller > > + */ > > +void dwc2_enable_host_interrupts(struct dwc2_hcd *hcd) > > should be static. OK. There may be a lot of functions that could be static but are not. I converted some of them but haven't swept the whole driver yet. > > +/** > > + * dwc2_disable_host_interrupts() - Disables the Host Mode interrupts > > + * > > + * @hcd: Programming view of DWC_otg controller > > + */ > > +void dwc2_disable_host_interrupts(struct dwc2_hcd *hcd) > > should be static OK. > > + if (!((hcd->hwcfg4 & GHWCFG4_DESC_DMA) && > > + hcd->snpsid >= DWC2_CORE_REV_2_90a && > > + (op_mode == GHWCFG2_OP_MODE_HNP_SRP_CAPABLE || > > + op_mode == GHWCFG2_OP_MODE_SRP_ONLY_CAPABLE || > > + op_mode == GHWCFG2_OP_MODE_NO_HNP_SRP_CAPABLE || > > + op_mode == GHWCFG2_OP_MODE_SRP_CAPABLE_HOST || > > + op_mode == GHWCFG2_OP_MODE_NO_SRP_CAPABLE_HOST))) { > > + > > + dev_err(hcd->dev, > > + "Host can't operate in Descriptor DMA mode.\n" > > + "Either core version is below 2.90a or " > > + "GHWCFG2, GHWCFG4 register values do not " > > + "allow Descriptor DMA in host mode.\nTo " > > + "run the driver in Buffer DMA host mode set " > > + "dma_desc_enable module parameter to 0.\n"); > > + return; > > + } > > this big if just to print a dev_err() ? And what a dev_err()... wow. > Instead of erroring out, you could just fix the parameters yourself in > runtime. We want to inform the user that they did not get descriptor DMA mode although they asked for it. I agree it's a little verbose, something like "hardware does not support DDMA mode" would suffice. Your right about fixing the parameters at runtime, I plan to do that in the next revision. > > +void dwc2_hc_init(struct dwc2_hcd *hcd, struct dwc2_hc *hc) > > static OK > > +#ifdef DEBUG > > + dev_info(hcd->dev, > > + "*** %s: Channel %d, hc->halt_pending already set ***\n", > > + __func__, hc->hc_num); > > + > > +#endif > > this is called dev_dbg(). Right, will fix. > > > +/** > > + * dwc2_dump_spram() - Reads the SPRAM and prints its content > > + * > > + * @hcd: Programming view of DWC_otg controller > > + */ > > +void dwc2_dump_spram(struct dwc2_hcd *hcd) > > debugfs using regsets I don't think it's useful, I'll just remove it. > > +/** > > + * dwc2_dump_host_registers() - Reads the host registers and prints them > > + * > > + * @hcd: Programming view of DWC_otg controller > > + */ > > +void dwc2_dump_host_registers(struct dwc2_hcd *hcd) > > debugfs using regsets. Actually, I want to keep it this way for now. This makes it easy to just insert a call to dwc2_dump_host_registers() in a troublesome spot in the code while debugging, and get a snapshot of the registers in the dmesg. I don't think I can do that with regsets, can I? I any case, this will be removed once the driver is stable. > > +int dwc2_check_core_status(struct dwc2_hcd *hcd) > > +{ > > + if (readl(hcd->regs + GSNPSID) == 0xffffffff) > > + return -1; > > + else > > + return 0; > > +} > > the whole thing is exposed. I don't like that at all, you need to > encapsulate implementation details. In fact, core.c should be a > platform_driver in its own right. Your PCI layer should just be a parent > device to this one just like on dwc3. I'm not sure how that applies here. This is just a simple routine to check that the hardware has not gone away unexpectedly. > > +#if 0 > > we don't like commented code, remove it. > > > +static inline void do_write(u32 value, void *addr) > > +{ > > + writel(value, addr); > > + pr_info("INFO:: wrote %08x to %p\n", value, addr); > > +} > > + > > +#undef writel > > +#define writel(v, a) do_write(v, a) > > +#endif This is part of the debugging code I mentioned. By enabling this, I get a trace of every register write the driver does. This is indispensable for debugging. Again, it will be removed once the driver is stable. > > +/* > > + * Host channel descriptor. This structure represents the state of a single > > + * host channel when acting in host mode. It contains the data items needed to > > + * transfer packets to an endpoint via a host channel. > > + */ > > +struct dwc2_hc { > > + /* Host channel number used for register address lookup */ > > + u8 hc_num; > > + > > + /* Device to access */ > > + unsigned dev_addr:7; > > this is not how we document structures Right, kerneldoc. I will add that to all of the structure definitions. > > +static const char *op_state_str(struct dwc2_hcd *hcd) > > +{ > > +#ifdef DEBUG > > + return (hcd->op_state == A_HOST ? "a_host" : > > + (hcd->op_state == A_SUSPEND ? "a_suspend" : > > + (hcd->op_state == A_PERIPHERAL ? "a_peripheral" : > > + (hcd->op_state == B_PERIPHERAL ? "b_peripheral" : > > + (hcd->op_state == B_HOST ? "b_host" : "unknown"))))); > > switch ? OK. > > + > > +#ifndef DWC_HOST_ONLY > > + if (dwc2_is_device_mode(hcd)) { > > + dev_info(hcd->dev, "SRP: Device mode\n"); > > + } else { > > + u32 hprt0; > > + > > + dev_info(hcd->dev, "SRP: Host mode\n"); > > + > > + /* Turn on the port power bit */ > > + hprt0 = dwc2_read_hprt0(hcd); > > + hprt0 |= HPRT0_PWR; > > + writel(hprt0, hcd->regs + HPRT0); > > + > > + /* > > + * Start the Connection timer. So a message can be displayed if > > + * connect does not occur within 10 seconds. > > + */ > > + dwc2_hcd_session_start(hcd); > > + } > > +#endif > > this check should be done in runtime, no ? Right now we don't have a device-mode driver, so this is code is just a placeholder until then. > > +#ifdef DEBUG > > + /* If any common interrupts set */ > > + if (gintsts & gintmsk_common) > > + dev_dbg(hcd->dev, "gintsts=%08x gintmsk=%08x\n", > > + gintsts, gintmsk); > > +#endif > > dev_vdbg() or just remove the ifdef. dev_dbg() is only valid when > DEBUG=y OK > > +int dwc2_handle_common_intr(void *dev) > > even your interrupt handler is exposed ? wow > > > +#include "s3c-hsotg.h" > > we don't want to depend on that, how about just copying it here ? In > fact, this file isn't available... Yes, it is available, I added "-I drivers/usb/gadget" to the Makefile. I thought unneeded code duplication is a big no-no in the kernel, am I wrong? -- 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
