Hi, On Tue, Feb 07, 2012 at 11:50:54AM -0800, Paul Zimmerman wrote: > > -----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. Could be, on the other hand, we want those failures to appear ASAP, so we fix them promptly. If we keep it disabled, we will most likely loose users, I'm afraid. Another argument is that we can keep this for testing for a loooong period of time before merging it to mainline. That's why we have linux-next and that's why we both have devices with DWC3 controller. Well there are a few more companies who might pitch in soon, so we might just get more people to help testing patches. > < 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. Can you talk more about what you're doing here ? Are you talking to HAPS' PCIe controller ? I mean, something on the FPGA, or something on the PCB ? I'm trying to understand whose registers are these ? > < 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. I'm not a PCI expert, but you ought to find a way to hide it under some abstraction. Describing HAPS better would help other guys pitch in with ideas. I'm adding linux-pci@vger to the Cc list in order to ask for some help. There is anyway a set of PCI pm platform_ops which you might be able to use in order to hide this, I don't know. Like I said, I'm not very confortable with the PCI subsystem. > > > + /* > > > + * 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. I see. Another of HAPS' details. It would be very nice to uncover all of this platform-specific details and find proper places for those. > < 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 git grep -e "pm_runtime_" drivers/ will have show you many examples. There's also lots of information under Documentation/power/runtime_pm.txt > function (pm_runtime_get?) is called, nothing will attempt to touch any pm_runtime_get() will, well, do a get. In short, it will enable the platform for access. In case of e.g. OMAP, we will be enabling the correct clock nodes in order to be able to use the correct interconnect to access device's register. > 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. that's something which is very driver specific ;-) So we must guarantee in the driver that this will not happen. It's not very complex to achieve that, though. Actually, what most people do, is that they call pm_runtime_get_sync() before an access to the device is needed, and pm_runtime_put() when the device isn't needed anymore. There's no way a generic pm layer can achieve that without help from the driver. See that with your ad-hoc PM implementation, you have a bunch of busy loops waiting for the device to come out of hibernation, those loops will be handled by the generic PM layer, but the driver has to help ;-) -- balbi
Attachment:
signature.asc
Description: Digital signature
