> -----Original Message----- > From: Felipe Balbi [mailto:balbi@xxxxxx] > Sent: Monday, February 06, 2012 11:03 PM > > On Mon, Feb 06, 2012 at 07:20:16PM -0800, Paul Zimmerman wrote: > > This patch adds the actual hibernation code, and allows it to be > > enabled in the Kconfig. > > Actually, is there any real reason someone would not use such a feature > if it's available in HW ? > > I mean, the guy chose to enable support for it on your core > configuration tool, right ? It's a dangerous feature if it's not implemented 100% correctly. It can cause the system to freeze solid. So I think it's best to make it an optional feature, at least for the time being. < snip > > hmm, so this is all haps-specific ?? Isn't PCIe supposed to be > standardized, how come you have vendor-specific registers ?? The HAPS implementation is non-standard, sorry. There's nothing I can do about that. < snip > > > +/* > > + * Platform-specific power control routine, for HAPS board > > + */ > > +void dwc3_haps_power_ctl(struct dwc3 *dwc, int on) > > +{ > > + u32 reg; > > + int cnt = 0; > > + > > + if (on) { > > + /* > > + * Enable core well power > > + */ > > + > > + /* This is "faked" by the FPGA */ > > + > > + /* > > + * Communicate with power controller to set power state to D0 > > + */ > > + > > + reg = dwc3_readl(dwc->regs, DWC3_HAPS_GASKET_POWER_CTL); > > + dev_vdbg(dwc->dev, "R1=0x%08x before write\n", reg); > > + reg = (reg & ~DWC3_HAPS_GASKET_POWER_DSTATE_MASK) | > > + DWC3_HAPS_GASKET_POWER_DSTATE_D0; > > + dwc3_writel(dwc->regs, DWC3_HAPS_GASKET_POWER_CTL, reg); > > + reg = dwc3_readl(dwc->regs, DWC3_HAPS_GASKET_POWER_CTL); > > + dev_vdbg(dwc->dev, "R1=0x%08x after write\n", reg); > > isn't this the same as pci_set_power_state(pci, PCI_D0); ?? Since the HAPS has a non-standard implementation, not part of the PCI interface, the PCI power control functions won't work, AFAIK. > > + /* > > + * Wait until both PMUs confirm that they have entered D0 > > + */ > > + > > + dev_vdbg(dwc->dev, "Asked for D0 state, awaiting response\n"); > > + > > + do { > > + udelay(1); > > + reg = dwc3_readl(dwc->regs, DWC3_HAPS_GASKET_DEBUG); > > + if (++cnt > 10000000) { > > + cnt = 0; > > + dev_vdbg(dwc->dev, "0x%08x\n", reg); > > + } > > + } while ((reg & DWC3_HAPS_GASKET_DEBUG_DSTATE_MASK) != > > + DWC3_HAPS_GASKET_DEBUG_DSTATE_D0); > > + } else { > > + /* > > + * Communicate with power controller to set power state to D3 > > + */ > > + > > + reg = dwc3_readl(dwc->regs, DWC3_HAPS_GASKET_POWER_CTL); > > + dev_vdbg(dwc->dev, "R1=0x%08x before write\n", reg); > > + reg = (reg & ~DWC3_HAPS_GASKET_POWER_DSTATE_MASK) | > > + DWC3_HAPS_GASKET_POWER_DSTATE_D3; > > + dwc3_writel(dwc->regs, DWC3_HAPS_GASKET_POWER_CTL, reg); > > + reg = dwc3_readl(dwc->regs, DWC3_HAPS_GASKET_POWER_CTL); > > + dev_vdbg(dwc->dev, "R1=0x%08x after write\n", reg); > > likewise pci_set_poer_state(pci, PCI_D3hot); or D3cold, I don't know ?? ditto > > + > > + /* > > + * Wait until both PMUs confirm that they have entered D3 > > + */ > > + > > + dev_vdbg(dwc->dev, "Asked for D3 state, awaiting response\n"); > > + > > + do { > > + udelay(1); > > + reg = dwc3_readl(dwc->regs, DWC3_HAPS_GASKET_DEBUG); > > + if (++cnt >= 10000000) { > > + cnt = 0; > > + dev_vdbg(dwc->dev, "0x%08x\n", reg); > > + } > > + } while ((reg & DWC3_HAPS_GASKET_DEBUG_DSTATE_MASK) != > > + DWC3_HAPS_GASKET_DEBUG_DSTATE_D3); > > + } > > +} > > + > > +/* > > + * Platform-specific PME interrupt handler, for HAPS board > > + */ > > +int dwc3_haps_pme_intr(struct dwc3 *dwc) > > if it's an IRQ handler, why aren't you requesting the IRQ and why ins't > this returning irqreturn_t ? It's not a normal interrupt handler, it's part of the non-standard HAPS implementation of power control. In a normal platform there would be a separate interrupt for this. < snip > > this is unnecessary. You should be using pm_runtime APIs for this ;-) > Please don't duplicate effort, we have our pm runtime layer for this > sort of situation. > > If you use pm_runtime, you will enter hibernation whenever your pm usage > counter reaches zero, and we can be as aggressive as we want. Can you point me to an existing kernel driver that uses the pm_runtime calls to perform what we need here? It must guarantee that after some function (pm_runtime_get?) is called, nothing will attempt to touch any of the controller registers except the ones that are guaranteed to be alive, namely the two HAPS-specific functions above. In particular, it must prevent calls from the upper layers into the gadget API from touching the controller registers. Thanks. -- 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
