> -----Original Message----- > From: Felipe Balbi [mailto:balbi@xxxxxx] > Sent: Monday, February 06, 2012 10:51 PM > > Hi, > > On Mon, Feb 06, 2012 at 07:20:15PM -0800, Paul Zimmerman wrote: > > This patch modifies the existing driver code to allow hibernation > > support to be added by the next patch in the series. This patch by > > itself should not change the existing functionality. > > > > This patch adds some new register definitions, exports a few > > previously static functions that will be needed by the hibernation > > code, adds a couple of new functions, refactors a few existing > > functions, and adds some hooks that will be needed. The hooks are > > currently all defined to no-ops in the new "hibernate.h". > > > > The second part will add the actual hibernation code, and allow it > > to be optionally enabled in the kernel config. > > > > NOTE: This patch renames dwc3-pci.c to dwc3_pci.c and dwc3-omap.c > > to dwc3_omap.c. This is necessary because the next patch adds a > > new platform-specific file that gets included into the bus glue > > module. But if a source file name is the same as the module name, > > then Kconfig doesn't allow adding other object files to the module. > > Other alternatives would be to change the module name, or to > > include the other source files directly into dwc3-pci.c or > > dwc3-omap.c. > > > > Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> > > One thing: I need your changes to be a bit more atomic. You shuffle a > bunch of stuff around then add new code all in the same patch. Please > make it easy for reviewers/maintainers and do all cleanups/shuffling > separated from new code ;-) OK. I will try to do that, and address most of the other things you mention, in the next submission. But I want to respond to a few of your points. > > +/* > > + * struct dwc3_scratchpad_array - hibernation scratchpad array > > + * (format defined by hw) > > + */ > > +struct dwc3_scratchpad_array { > > + __le64 dma_adr[DWC3_MAX_HIBER_SCRATCHBUFS]; > > make it dma_addr. BTW, couldn't this be a dma_addr_t instead of __le64 ? It's not a dma_addr, it's always little-endian and 64 bits, independent of the processor arch. > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3_pci.c > > similarity index 94% > > rename from drivers/usb/dwc3/dwc3-pci.c > > rename to drivers/usb/dwc3/dwc3_pci.c > > index 64e1f7c..d630fac 100644 > > --- a/drivers/usb/dwc3/dwc3-pci.c > > +++ b/drivers/usb/dwc3/dwc3_pci.c > > @@ -1,5 +1,5 @@ > > /** > > - * dwc3-pci.c - PCI Specific glue layer > > + * dwc3_pci.c - PCI Specific glue layer > > * > > * Copyright (C) 2010-2011 Texas Instruments Incorporated - http://www.ti.com > > * > > @@ -43,8 +43,8 @@ > > #include <linux/platform_device.h> > > > > #include "core.h" > > +#include "haps_pwrctl.h" > > I don't like this. It couples dwc3-pci with HAPS and we all know this > file will be (is ?) used with platforms other than HAPS. I'm open to suggestions :) I was thinking that when someone adds a new platform, they will add "#include "xxx_prwctl.h" here. > @@ -106,6 +106,12 @@ static int __devinit dwc3_pci_probe(struct pci_dev *pci, > goto err4; > } > > + if (pci->vendor == PCI_VENDOR_ID_SYNOPSYS && > + pci->device == PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3) { > + dwc3_power_ctl = dwc3_haps_power_ctl; > + dwc3_pme_interrupt = dwc3_haps_pme_intr; > + } > another way, which would be neater, would be to pass this as > driver_data. But I want to avoid needing this, I need to read patch 4/4 > first, though. Yeah, I tried to find another way to do this, but it stumped me. > > @@ -312,8 +365,8 @@ static const char *dwc3_gadget_ep_cmd_string(u8 cmd) > > return "Clear Stall"; > > case DWC3_DEPCMD_SETSTALL: > > return "Set Stall"; > > - case DWC3_DEPCMD_GETSEQNUMBER: > > - return "Get Data Sequence Number"; > > + case DWC3_DEPCMD_GETEPSTATE: > > + return "Get Endpoint State"; > > Now we're screwed :-) If we're running on an older core and we want to > send GETSEQNUMBER, debugging message will be wrong. You will need to add > a revision check here. So pass struct dwc3 as a parameter too, I guess > it's the only way. I don't think there would be any reason to use GETSEQNUMBER on older cores, and it is currently not used in this driver. But I could make it say "Get Data Seq Num / Get EP State", would that be OK? > > -static void dwc3_gadget_usb2_phy_power(struct dwc3 *dwc, int on) > > +static void dwc3_gadget_usb2_ena_phy_susp(struct dwc3 *dwc, int enable) > > hehe, why ? Please revert this rename. I would like to keep it, please. The description of the register bit in the databook is: Suspend USB3.0 SS PHY (Suspend_en) When set, and if Suspend conditions are valid, the USB 3.0 PHY enters Suspend mode. So I would like the function name to reflect that. > > -static void dwc3_gadget_disable_phy(struct dwc3 *dwc, u8 speed) > > +static void dwc3_gadget_suspend_phy(struct dwc3 *dwc, u8 speed) > > please revert the rename. ditto > > +static inline void dwc3_gadget_get_ctlr(struct dwc3 *dwc) > > +{ > > + dwc->usage_cnt++; > > +} > > + > > +static inline void dwc3_gadget_put_ctlr(struct dwc3 *dwc) > > +{ > > + dwc->usage_cnt--; > > +} > > why do you need this ? You can use pm_runtime_get()/pm_runtime_put() See my other email. > > +#define dwc3_wait_if_hibernating(_dwc, _flags) \ > > +{ \ > > + int _temp = (_dwc)->hibernate; \ > > + while (_temp >= DWC3_HIBER_SLEEPING && \ > > + _temp != DWC3_HIBER_SS_DIS_QUIRK) { \ > > + spin_unlock_irqrestore(&(_dwc)->lock, (_flags)); \ > > + msleep(1); \ > > + spin_lock_irqsave(&(_dwc)->lock, (_flags)); \ > > + _temp = (_dwc)->hibernate; \ > > + } \ > > +} > > if HW go boo-boo, this could become quite dangerous and freeze the whole > machine. Whenever you add a wait/sleep, you need to have a way out of a > possible infinite loop. Actually, no. Since we release the lock and enable interrupts, the worst that could happen is the gadget driver stops working. It shouldn't freeze the rest of the machine. But if this is changed to use the pm ops then this is moot anyway. -- 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
