Hi, On Fri, Jan 11, 2013 at 10:22:08PM +0000, Paul Zimmerman wrote: > > 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? Actually no since it's all part of the same driver anyway. I just need to force myself into going over it all :-) > 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 that was very clear from the looks of the driver :-) I imagined that was the case. > 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. fair enough :-) > > 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. that's fair, but I wonder if it wouldn't be better to save the extra commits later ? > 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 hehe, I didn't mean to put it all in one file, I meant that whatever has to be exposed to other source files, shouldn't get close to the amount of functions implemented. Current implementation exposes almost every single function and I think that's overkill. The "small set of APIs" are the ones which really have to be exposed, but implementation details can be hidde. I mean, instead of exposing (hypothetical situation) event_buff_alloc(), event_buff_setup(), enable_irq_events(), udc_setup(), etc etc, you can expose a single "dwc3_initialize()" and that is already calling the others properly. > individual APIs for each of the files. I disagree that there are any > external "users" like you mention above, it is all one driver. right right, see above. > 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. Yeah, but some of those aren't adding any real information to a debug session. Those are the ones I complain about, besides you can very easily add them while debugging if you really want to be that verbose. > > 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? fair enough. > > > +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. haven't you heard ? Style is all that matters. Seriously though, even though they are static, when you get to things like function tracer, it's good to make it easy to trace e.g all functions matching dwc2_*. > > > + 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. Copy the definitions to your own driver and fix them up. The whole "once it's integrated I'll fix" I can already see that we will go through another ancient civilization doomsday nonsense and the same camelcase will still be there. > > > + /* > > > + * 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. you _do_ have access to the IP engineers right ? Also, you can very easily test different msleep() arguments to see if the comment is actually sound. > > > + /* > > > + * This programming sequence needs to happen in FS mode before any other > > > + * programming occurs > > > + */ > > > > why ? > > Ditto. ditto > > > + 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. cool > > 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 very difficult to argument against this statement when you give no reasoning behind it. What has made you come to this conclusion ? I have see no limitations to dwc3.ko + dwc3-$platform.ko design, specially when you understand how driver core works. > we have a second bus interface to connect. same comment as above related to ancient civilizations doomsday prophecies. > I will look at refactoring the function, though. thanks > > > +/** > > > + * 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. right. > > > + 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. great, gives the same information IMO :-p > Your right about fixing the parameters at runtime, I plan to do that in > the next revision. thanks. And I agree with you the dev_err() is probably still necessary, but your wording is far better. > > > +/** > > > + * 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. right, debugfs wouldn't help in your case, but current implementation will cause quite some extra delay in the driver, no ? I would keep this out of tree and implemented with trace_printk(), but that's just me. > > > +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. hehe, that was more of a general comment :-) > > > +#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. right, if you had your own read/write wrappers you just put dev_vdbg() or add tracepoints to them ;-) > > > +/* > > > + * 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. Thank you > > > + > > > +#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. Then it can probably be removed until then :-p > > > +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? it is, but in this case I prefer to duplicate the register definition. Note that headers (unless local to the driver) should all live under include/, if it's not there then adding a 'fake' -I drivers/usb/gadget is an even bigger no-no :-) cheers -- balbi
Attachment:
signature.asc
Description: Digital signature
