On Tue, Feb 07, 2012 at 12:27:44PM -0800, Paul Zimmerman wrote: > > -----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. most likely they will and that's exactly what frightens me :-) This will just grow larger and larger and everybody will try to ifdef out the other platform and then boom, we're back to the ice ages ;-) Maybe, if you manage to find a way to use pm_runtime, this will all be hidden. Like I said before, let's see what the PCI folks say about your HAPS power control :-) > > @@ -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. You could use driver_data, which is a field on the pci ID table structure. > > > @@ -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? Maybe, but you're right. We're not using it on current driver. If someone needs later, they can add I guess. good point > > > -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. so how about: dwc3_gadget_usb2_phy_suspend() ? When you concatenate too much, it soon becomes difficult to keep track of all function names. And, btw, function renames, also separate from everything else ;-) > > > +#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. indeed. -- balbi
Attachment:
signature.asc
Description: Digital signature
