usb: dwc3: second part of hibernation support
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): 9c4ee1f156d28640bc43dee647e3470b9e1b9f37.1328582753.git.paulz@synopsys.com
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): F6C4814A9B336243ABC3D1FB44E08AE801D5677E3A@US01WXMBX1.internal.synopsys.com
Hi,
Quoted 11 lines · Paul Zimmerman
Quoted 11 lines
-----Original Message-----
From: Felipe Balbi [mailto:[email protected]]
Sent: Monday, February 06, 2012 11:03 PMQuoted 2 lines · Paul Zimmerman
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 ?
Quoted 6 lines
Quoted 2 lines
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.
Quoted 7 lines
< snip >
Quoted 2 lines
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 ?
Quoted 31 lines
< snip >
Quoted 20 lines
Quoted 20 lines
(quoted diff) +20
-0+/* + * 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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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); ??
Quoted 2 lines
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.
Quoted 59 lines
Quoted 21 lines
Quoted 21 lines
(quoted diff) +21
-0+ /* + * 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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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
Quoted 18 lines
Quoted 18 lines
(quoted diff) +18
-0+ + /* + * 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); + } +}Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ +/* + * 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?
Quoted 3 lines
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.
Quoted 11 lines
< snip >
Quoted 6 lines
this is unnecessary. You should be using
pm_runtimeAPIs 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_runtimecalls 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
Quoted 1 line
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.
Quoted 4 lines
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 ;-)
Signature · 1 line
balbi
Quoted 21 lines
-----Original Message-----
From: Felipe Balbi [mailto:[email protected]]
Sent: Tuesday, February 07, 2012 12:18 PMHi,
Quoted 14 lines · Paul Zimmerman
Quoted 13 lines
-----Original Message-----
From: Felipe Balbi [mailto:[email protected]]
Sent: Monday, February 06, 2012 11:03 PMQuoted 2 lines · Paul Zimmerman
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 ?
Quoted 7 lines
Quoted 3 lines
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.
I'm worried more about an imperfect hardware implementation. If one of those gets out into the field, then the user will be screwed unless they have a way to disable this.
Quoted 11 lines
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.
Quoted 5 lines
< snip >
Quoted 2 lines
hmm, so this is all haps-specific ?? Isn't PCIe supposed to be standardized, how come you have vendor-specific registers ??
Quoted 6 lines
Quoted 2 lines
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 ?
ASCII art diagram follows, hopefully Outlook won't screw it up.
PCIe Bus
| |
+--------------+
| DWC PCIe |
| Controller |
+--------------+
| |
+------------------+
| SNPS Custom Glue |<-------+
| Logic ("Gasket") |<--+ |
+------------------+ | |
| | | |
+------------+ IRQ | |
| DWC USB3 |------+ |
| Controller | |
+------------ |
| | |
+--------+ Wake Signal |
| SNPS |-------------+
| Phy |
+--------+
| |USB Bus
All of the blocks shown, except for the Phy, are implemented in the FPGA. The power control registers, the wake interrupt generation, and logic to simulate having the power removed without actualy turning off the power, are all contained in the custom gasket. The wake signal is combined with the normal IRQ, so to the processor it appears to be coming from the USB3 controller, except that it appears in one of the gasket registers instead of the USB3 controller registers. The gasket registers are in the same BAR address space as the USB3 controller.
< snip >
Quoted 12 lines
Quoted 2 lines
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_opswhich 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.
< snip >
Quoted 27 lines
Quoted 2 lines
Can you point me to an existing kernel driver that uses the
pm_runtimecalls to perform what we need here? It must guarantee that after somegit grep -e "
pm_runtime_"drivers/will have show you many examples. There's also lots of information underDocumentation/power/runtime_pm.txtQuoted 1 line
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.Quoted 4 lines
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, andpm_runtime_put()when the device isn't needed anymore.
Quoted 5 lines
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 ;-)
OK, I will poke around and see if I can find an existing driver to use as an example.
Signature · 1 line
Paul
Quoted 6 lines
-----Original Message-----
From: Paul Zimmerman
Sent: Tuesday, February 07, 2012 1:16 PMOK, I will poke around and see if I can find an existing driver to use as an example.
Hi Felipe,
Can you explain a little about how the runtime_pm stuff will work in the DWC3 driver, disregarding the HAPS platform for now.
For example, if the device is a PCI device, won't the PM ops need to be part of the struct pci_driver in the dwc3-pci module?
If so, how will they be able to communicate with the routines in the dwc3 module? Or if not, how will that work exactly?
Thanks.
Signature · 1 line
Paul
Hi.
Quoted 9 lines · Paul Zimmerman
Quoted 6 lines
-----Original Message-----
From: Paul Zimmerman
Sent: Tuesday, February 07, 2012 1:16 PMOK, I will poke around and see if I can find an existing driver to use as an example.
Hi Felipe,
Quoted 8 lines
Can you explain a little about how the
runtime_pmstuff will work in the DWC3 driver, disregarding the HAPS platform for now.For example, if the device is a PCI device, won't the PM ops need to be part of the struct
pci_driverin the dwc3-pci module?If so, how will they be able to communicate with the routines in the dwc3 module? Or if not, how will that work exactly?
Ok, the way it works is that the e.g. PCI device is a parent device to the DWC3. So, in fact, you will end up with two pm ops: one that only knows about DWC3 and one that only knows about PCI.
The pm core guarantees that parent's e.g. runtime_suspend(), will only be called after all children (in this case, only one) are suspended. Likewise for resume.
So, what we need to do is to make one pm ops that knows everything the power management features on DWC3 and another which knows about PCI, then hook them up properly and whenever pm usage counter reaches zero, dwc3's runtime_suspend() is called, then after that dwc3-pci's runtime_suspend() will be called.
Just keep in mind that glue layer also needs to keep track of its own usage counters, what I mean by that, is that on dwc3-pci probe you should probably call pm_runtime_get_sync() and right before returning from probe, you call pm_runtime_put(). This will make sure that the PCIe device is powered up when you need and can be autosuspended when you don't need it anymore.
Signature · 1 line
balbi
Quoted 6 lines · Felipe Balbi
Just keep in mind that glue layer also needs to keep track of its own usage counters, what I mean by that, is that on dwc3-pci probe you should probably call
pm_runtime_get_sync()and right before returning from probe, you callpm_runtime_put(). This will make sure that the PCIe device is powered up when you need and can be autosuspended when you don't need it anymore.
Runtime PM for PCI devices uses a slightly different scheme. The PCI core increments the usage counter when a PCI device is first probed, and doesn't decrement the counter until the device is removed. This helps drivers without runtime PM support; the device will never be autosuspended.
As a result, if a PCI driver does support autosuspend, it doesn't need to make any special calls in its probe and remove routines. It just has to call pm_runtime_put[_sync] when not using the device and pm_runtime_get[_sync] when ready to start using it again.
Alan Stern
Quoted 27 lines
From: Felipe Balbi [mailto:[email protected]]
Sent: Tuesday, February 07, 2012 10:15 PMQuoted 6 lines · Paul Zimmerman
Quoted 3 lines
OK, I will poke around and see if I can find an existing driver to use as an example.
Hi Felipe,
Quoted 8 lines
Can you explain a little about how the
runtime_pmstuff will work in the DWC3 driver, disregarding the HAPS platform for now.For example, if the device is a PCI device, won't the PM ops need to be part of the struct
pci_driverin the dwc3-pci module?If so, how will they be able to communicate with the routines in the dwc3 module? Or if not, how will that work exactly?
Ok, the way it works is that the e.g. PCI device is a parent device to the DWC3. So, in fact, you will end up with two pm ops: one that only knows about DWC3 and one that only knows about PCI.
The pm core guarantees that parent's e.g.
runtime_suspend(), will only be called after all children (in this case, only one) are suspended. Likewise for resume.
Quoted 5 lines
So, what we need to do is to make one pm ops that knows everything the power management features on DWC3 and another which knows about PCI, then hook them up properly and whenever pm usage counter reaches zero, dwc3's
runtime_suspend()is called, then after that dwc3-pci'sruntime_suspend()will be called.
Thank you! That was the piece of the puzzle that was missing. OK, I'll take a crack at implementing the pm ops for dwc3 and dwc3-pci.
Quoted 6 lines
Just keep in mind that glue layer also needs to keep track of its own usage counters, what I mean by that, is that on dwc3-pci probe you should probably call
pm_runtime_get_sync()and right before returning from probe, you callpm_runtime_put(). This will make sure that the PCIe device is powered up when you need and can be autosuspended when you don't need it anymore.
I see that Alan had a correction to this part in a later email.
Signature · 1 line
Paul
Quoted 27 lines · Paul Zimmerman
Quoted 27 lines
From: Felipe Balbi [mailto:[email protected]]
Sent: Tuesday, February 07, 2012 10:15 PMQuoted 6 lines · Paul Zimmerman
Quoted 3 lines
OK, I will poke around and see if I can find an existing driver to use as an example.
Hi Felipe,
Quoted 8 lines
Can you explain a little about how the
runtime_pmstuff will work in the DWC3 driver, disregarding the HAPS platform for now.For example, if the device is a PCI device, won't the PM ops need to be part of the struct
pci_driverin the dwc3-pci module?If so, how will they be able to communicate with the routines in the dwc3 module? Or if not, how will that work exactly?
Ok, the way it works is that the e.g. PCI device is a parent device to the DWC3. So, in fact, you will end up with two pm ops: one that only knows about DWC3 and one that only knows about PCI.
The pm core guarantees that parent's e.g.
runtime_suspend(), will only be called after all children (in this case, only one) are suspended. Likewise for resume.
Quoted 8 lines
Quoted 5 lines
So, what we need to do is to make one pm ops that knows everything the power management features on DWC3 and another which knows about PCI, then hook them up properly and whenever pm usage counter reaches zero, dwc3's
runtime_suspend()is called, then after that dwc3-pci'sruntime_suspend()will be called.Thank you! That was the piece of the puzzle that was missing. OK, I'll take a crack at implementing the pm ops for dwc3 and dwc3-pci.
no problem ;-) There's very good documentation under Documentation/power/ and the folks at linux-pm@vger are very helpful too ;-)
Quoted 8 lines
Quoted 6 lines
Just keep in mind that glue layer also needs to keep track of its own usage counters, what I mean by that, is that on dwc3-pci probe you should probably call
pm_runtime_get_sync()and right before returning from probe, you callpm_runtime_put(). This will make sure that the PCIe device is powered up when you need and can be autosuspended when you don't need it anymore.I see that Alan had a correction to this part in a later email.
Indeed, I was not aware of the PCI differences :-) I will take care of that on the dwc3-omap.c though ;-)
thanks
Signature · 1 line
balbi