| Message ID | 1320410207-14537-1-git-send-email-chenhui.zhao@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 11/04/2011 07:36 AM, Zhao Chenhui wrote: > From: Li Yang <leoli@freescale.com> > > Signed-off-by: Li Yang <leoli@freescale.com> > --- > .../devicetree/bindings/powerpc/fsl/pmc.txt | 63 +++++++++++-------- > 1 files changed, 36 insertions(+), 27 deletions(-) > > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt > index 07256b7..d84b4f8 100644 > --- a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt > +++ b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt > @@ -9,22 +9,27 @@ Properties: > > "fsl,mpc8548-pmc" should be listed for any chip whose PMC is > compatible. "fsl,mpc8536-pmc" should also be listed for any chip > - whose PMC is compatible, and implies deep-sleep capability. > + whose PMC is compatible, and implies deep-sleep capability and > + wake on user defined packet(wakeup on ARP). Why does the PMC care? This is an ethernet controller feature, the PMC is just keeping the wakeup-relevant parts of the ethernet controller alive (whatever they happen to be). Do we have any chips that have ethernet controller support for wake on user-defined packet, but a sleep mode that doesn't let it be used? BTW, please remove fsl,mpc8536-pmc from the p1023rds device tree -- it was wrong before (no deep sleep, though it does appear to have jog mode...), and is even more wrong with this provision (it has a different ethernet controller). > + "fsl,p1022-pmc" should be listed for any chip whose PMC is > + compatible, and implies lossless Ethernet capability during sleep. > > "fsl,mpc8641d-pmc" should be listed for any chip whose PMC is > compatible; all statements below that apply to "fsl,mpc8548-pmc" also > apply to "fsl,mpc8641d-pmc". > > Compatibility does not include bit assignments in SCCR/PMCDR/DEVDISR; these > - bit assignments are indicated via the sleep specifier in each device's > - sleep property. > + bit assignments are indicated via the clock nodes. Device which has a > + controllable clock source should have a "clk-handle" property pointing > + to the clock node. Do we have any code to use this? Normally that shouldn't matter, but we already an unused binding for this. :-) Please provide rationale for doing it this way. Ideally it should probably use whatever http://devicetree.org/ClockBindings ends up being. > - reg: For devices compatible with "fsl,mpc8349-pmc", the first resource > is the PMC block, and the second resource is the Clock Configuration > block. > > - For devices compatible with "fsl,mpc8548-pmc", the first resource > - is a 32-byte block beginning with DEVDISR. > + For devices compatible with "fsl,mpc8548-pmc", the second resource > + is a 32-byte block beginning with DEVDISR if supported. Huh? -Scott
On 11/04/2011 03:05 PM, Scott Wood wrote: > On 11/04/2011 07:36 AM, Zhao Chenhui wrote: >> + "fsl,p1022-pmc" should be listed for any chip whose PMC is >> + compatible, and implies lossless Ethernet capability during sleep. >> >> "fsl,mpc8641d-pmc" should be listed for any chip whose PMC is >> compatible; all statements below that apply to "fsl,mpc8548-pmc" also >> apply to "fsl,mpc8641d-pmc". >> >> Compatibility does not include bit assignments in SCCR/PMCDR/DEVDISR; these >> - bit assignments are indicated via the sleep specifier in each device's >> - sleep property. >> + bit assignments are indicated via the clock nodes. Device which has a >> + controllable clock source should have a "clk-handle" property pointing >> + to the clock node. > > Do we have any code to use this? > > Normally that shouldn't matter, but we already an unused binding for > this. :-) > > Please provide rationale for doing it this way. Ideally it should > probably use whatever http://devicetree.org/ClockBindings ends up being. OK, I see the code now. Still could use some explanation. -Scott
>-----Original Message----- >From: linuxppc-dev-bounces+leoli=freescale.com@lists.ozlabs.org >[mailto:linuxppc-dev-bounces+leoli=freescale.com@lists.ozlabs.org] On >Behalf Of Scott Wood >Sent: Saturday, November 05, 2011 4:05 AM >To: Zhao Chenhui-B35336 >Cc: linuxppc-dev@lists.ozlabs.org >Subject: Re: [PATCH 5/7] fsl_pmc: update device bindings > >On 11/04/2011 07:36 AM, Zhao Chenhui wrote: >> From: Li Yang <leoli@freescale.com> >> >> Signed-off-by: Li Yang <leoli@freescale.com> >> --- >> .../devicetree/bindings/powerpc/fsl/pmc.txt | 63 +++++++++++-- >------ >> 1 files changed, 36 insertions(+), 27 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >> index 07256b7..d84b4f8 100644 >> --- a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >> +++ b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >> @@ -9,22 +9,27 @@ Properties: >> >> "fsl,mpc8548-pmc" should be listed for any chip whose PMC is >> compatible. "fsl,mpc8536-pmc" should also be listed for any chip >> - whose PMC is compatible, and implies deep-sleep capability. >> + whose PMC is compatible, and implies deep-sleep capability and >> + wake on user defined packet(wakeup on ARP). > >Why does the PMC care? This is an ethernet controller feature, the PMC >is just keeping the wakeup-relevant parts of the ethernet controller >alive (whatever they happen to be). > >Do we have any chips that have ethernet controller support for wake on >user-defined packet, but a sleep mode that doesn't let it be used? I think it is more a PMC feature cause Ethernet controller on many SoCs have the filer feature, but only very limited SoCs can support using it as a wake-up source. The differences should be mostly in the PM controller block. Also the wake on user-defined packet feature was never mentioned in the Ethernet controller part of UM. > >BTW, please remove fsl,mpc8536-pmc from the p1023rds device tree -- it >was wrong before (no deep sleep, though it does appear to have jog >mode...), and is even more wrong with this provision (it has a different >ethernet controller). > >> + "fsl,p1022-pmc" should be listed for any chip whose PMC is >> + compatible, and implies lossless Ethernet capability during sleep. >> >> "fsl,mpc8641d-pmc" should be listed for any chip whose PMC is >> compatible; all statements below that apply to "fsl,mpc8548-pmc" also >> apply to "fsl,mpc8641d-pmc". >> >> Compatibility does not include bit assignments in SCCR/PMCDR/DEVDISR; >these >> - bit assignments are indicated via the sleep specifier in each >device's >> - sleep property. >> + bit assignments are indicated via the clock nodes. Device which has >a >> + controllable clock source should have a "clk-handle" property >pointing >> + to the clock node. > >Do we have any code to use this? Yes, in another patch in the series. > >Normally that shouldn't matter, but we already an unused binding for >this. :-) > >Please provide rationale for doing it this way. Ideally it should >probably use whatever http://devicetree.org/ClockBindings ends up being. I have looked into that binding. The binding was primarily defined for the Linux clock API which is not same as what we are doing here(set wakeup source). If in the future the clock API also covers wakeup related features, we can change to use it. - Leo
On 11/08/2011 04:56 AM, Li Yang-R58472 wrote: >>> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >> b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >>> index 07256b7..d84b4f8 100644 >>> --- a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >>> @@ -9,22 +9,27 @@ Properties: >>> >>> "fsl,mpc8548-pmc" should be listed for any chip whose PMC is >>> compatible. "fsl,mpc8536-pmc" should also be listed for any chip >>> - whose PMC is compatible, and implies deep-sleep capability. >>> + whose PMC is compatible, and implies deep-sleep capability and >>> + wake on user defined packet(wakeup on ARP). >> >> Why does the PMC care? This is an ethernet controller feature, the PMC >> is just keeping the wakeup-relevant parts of the ethernet controller >> alive (whatever they happen to be). >> >> Do we have any chips that have ethernet controller support for wake on >> user-defined packet, but a sleep mode that doesn't let it be used? > > I think it is more a PMC feature cause Ethernet controller on many > SoCs have the filer feature, but only very limited SoCs can support > using it as a wake-up source. The differences should be mostly in > the PM controller block. Have you tried waking from sleep with it on those other SoCs? > Also the wake on user-defined packet feature was never mentioned in the Ethernet controller part of UM. AFAICT all this "feature" is, is programming the Ethernet controller to filter out some packets that we don't want to wake us up, and then refraining from entering magic packet mode. What PMC registers are programmed any differently for this? It's in the PM part of the manual because that's where they generally describe issues related to PM. They describe magic packet there too. The set of devices which are still active during sleep is a different issue from the conditions on which a device will request a wake. I also don't trust our PM documentation very much. It's ambiguous just what gets shut down in ordinary sleep mode. E.g. the mpc8544 manual talks about "external interrupts", but in one place it looks like it means external to the SoC: > In sleep mode, all clocks internal to the e500 core are turned off, including the timer facilities clock. All > I/O interfaces in the device logic are also shut down. Only the clocks to the MPC8544E PIC are still > running so that an external interrupt can wake up the device. But the note immediately below that seems to imply they're talking about external to the core: > Only external interrupts can wake the MPC8544E from sleep mode. Internal > interrupt sources like the core interval timer or watchdog timer depend on > an active clock for their operation and these are disabled in sleep mode. >> Normally that shouldn't matter, but we already an unused binding for >> this. :-) >> >> Please provide rationale for doing it this way. Ideally it should >> probably use whatever http://devicetree.org/ClockBindings ends up being. > > I have looked into that binding. The binding was primarily defined for the Linux clock API which is not same as what we are doing here(set wakeup source). > If in the future the clock API also covers wakeup related features, we can change to use it. While Linux APIs can serve as an inspiration, they're not the basis of device tree bindings. The device tree is not Linux-specific, and should not change just because Linux changes, but rather should describe the hardware. We want to get this right the first time. What we are describing here is how a device's clock is connected to the PMC. -Scott
>-----Original Message----- >From: Wood Scott-B07421 >Sent: Wednesday, November 09, 2011 4:40 AM >To: Li Yang-R58472 >Cc: Wood Scott-B07421; Zhao Chenhui-B35336; linuxppc-dev@lists.ozlabs.org >Subject: Re: [PATCH 5/7] fsl_pmc: update device bindings > >On 11/08/2011 04:56 AM, Li Yang-R58472 wrote: >>>> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >>> b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >>>> index 07256b7..d84b4f8 100644 >>>> --- a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >>>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt >>>> @@ -9,22 +9,27 @@ Properties: >>>> >>>> "fsl,mpc8548-pmc" should be listed for any chip whose PMC is >>>> compatible. "fsl,mpc8536-pmc" should also be listed for any chip >>>> - whose PMC is compatible, and implies deep-sleep capability. >>>> + whose PMC is compatible, and implies deep-sleep capability and >>>> + wake on user defined packet(wakeup on ARP). >>> >>> Why does the PMC care? This is an ethernet controller feature, the >>> PMC is just keeping the wakeup-relevant parts of the ethernet >>> controller alive (whatever they happen to be). >>> >>> Do we have any chips that have ethernet controller support for wake >>> on user-defined packet, but a sleep mode that doesn't let it be used? >> >> I think it is more a PMC feature cause Ethernet controller on many >> SoCs have the filer feature, but only very limited SoCs can support >> using it as a wake-up source. The differences should be mostly in the >> PM controller block. > >Have you tried waking from sleep with it on those other SoCs? We have tried and it's not working. Although the filer is an eTSEC feature, waking up on it is really a complex system wide change including eTSEC, DDR, ECM, PIC, and etc. And currently it's tied up to deep sleep feature. So I would like it to be part of the SoC integration domain i.e. PMC. > >> Also the wake on user-defined packet feature was never mentioned in the >Ethernet controller part of UM. > >AFAICT all this "feature" is, is programming the Ethernet controller to >filter out some packets that we don't want to wake us up, and then >refraining from entering magic packet mode. What PMC registers are >programmed any differently for this? > >It's in the PM part of the manual because that's where they generally >describe issues related to PM. They describe magic packet there too. > >The set of devices which are still active during sleep is a different >issue from the conditions on which a device will request a wake. > >I also don't trust our PM documentation very much. It's ambiguous just >what gets shut down in ordinary sleep mode. E.g. the mpc8544 manual talks >about "external interrupts", but in one place it looks like it means >external to the SoC: > >> In sleep mode, all clocks internal to the e500 core are turned off, >> including the timer facilities clock. All I/O interfaces in the device >> logic are also shut down. Only the clocks to the MPC8544E PIC are still >running so that an external interrupt can wake up the device. > >But the note immediately below that seems to imply they're talking about >external to the core: > >> Only external interrupts can wake the MPC8544E from sleep mode. >> Internal interrupt sources like the core interval timer or watchdog >> timer depend on an active clock for their operation and these are >disabled in sleep mode. > > > >>> Normally that shouldn't matter, but we already an unused binding for >>> this. :-) >>> >>> Please provide rationale for doing it this way. Ideally it should >>> probably use whatever http://devicetree.org/ClockBindings ends up being. >> >> I have looked into that binding. The binding was primarily defined for >the Linux clock API which is not same as what we are doing here(set wakeup >source). >> If in the future the clock API also covers wakeup related features, we >can change to use it. > >While Linux APIs can serve as an inspiration, they're not the basis of >device tree bindings. The device tree is not Linux-specific, and should >not change just because Linux changes, but rather should describe the >hardware. We want to get this right the first time. > >What we are describing here is how a device's clock is connected to the >PMC. AFAIKT, the purpose of defining the clock binding is used to describe the topology of clocks in a system. And then used for runtime control of enabling/disabling clocks or getting/changing clock frequencies. But in this PM case, we just set the wakeup capability and provide little runtime control of the clocks for on-chip devices. And it doesn't get any benefit of using this binding. If your concern is the confusion with the already existing clock binding, we can remove the clk in the name of the PM binding. If we are to use the clock binding, I think adding the CSB clock, DDR clock, core clock, and etc with this binding is more appropriate. - Leo
On Wed, Nov 09, 2011 at 04:59:16AM -0600, Li Yang-R58472 wrote: > >>> Do we have any chips that have ethernet controller support for wake > >>> on user-defined packet, but a sleep mode that doesn't let it be used? > >> > >> I think it is more a PMC feature cause Ethernet controller on many > >> SoCs have the filer feature, but only very limited SoCs can support > >> using it as a wake-up source. The differences should be mostly in the > >> PM controller block. > > > >Have you tried waking from sleep with it on those other SoCs? > > We have tried and it's not working. OK. > AFAIKT, the purpose of defining the clock binding is used to describe > the topology of clocks in a system. And then used for runtime control > of enabling/disabling clocks or getting/changing clock frequencies. > > But in this PM case, we just set the wakeup capability Where "wakeup capability" means deciding whether to turn off the clock during a low-power state. The basic information you need from the device tree is the same -- a connection from here to there. > and provide little runtime control of the clocks for on-chip devices. My original intent for the binding you replaced was that it could be used for other clock management as well -- you could use it to describe DEVDISR or 83xx SCCR mappings as well. It was when I sent that binding out that I recall being asked to use the clock binding. That said, Grant has recently said he's unhappy with the current state of the clock binding, so I'm not sure what the right thing to do here is. > And it doesn't get any benefit of using this binding. If your concern > is the confusion with the already existing clock binding, we can remove > the clk in the name of the PM binding. My concern, besides unnecessary duplication of ways to express something, is a loss of generality. > If we are to use the clock binding, I think adding the CSB clock, DDR > clock, core clock, and etc with this binding is more appropriate. That would be another use of it, but one doesn't need to imply the other. -Scott
diff --git a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt index 07256b7..d84b4f8 100644 --- a/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt +++ b/Documentation/devicetree/bindings/powerpc/fsl/pmc.txt @@ -9,22 +9,27 @@ Properties: "fsl,mpc8548-pmc" should be listed for any chip whose PMC is compatible. "fsl,mpc8536-pmc" should also be listed for any chip - whose PMC is compatible, and implies deep-sleep capability. + whose PMC is compatible, and implies deep-sleep capability and + wake on user defined packet(wakeup on ARP). + + "fsl,p1022-pmc" should be listed for any chip whose PMC is + compatible, and implies lossless Ethernet capability during sleep. "fsl,mpc8641d-pmc" should be listed for any chip whose PMC is compatible; all statements below that apply to "fsl,mpc8548-pmc" also apply to "fsl,mpc8641d-pmc". Compatibility does not include bit assignments in SCCR/PMCDR/DEVDISR; these - bit assignments are indicated via the sleep specifier in each device's - sleep property. + bit assignments are indicated via the clock nodes. Device which has a + controllable clock source should have a "clk-handle" property pointing + to the clock node. - reg: For devices compatible with "fsl,mpc8349-pmc", the first resource is the PMC block, and the second resource is the Clock Configuration block. - For devices compatible with "fsl,mpc8548-pmc", the first resource - is a 32-byte block beginning with DEVDISR. + For devices compatible with "fsl,mpc8548-pmc", the second resource + is a 32-byte block beginning with DEVDISR if supported. - interrupts: For "fsl,mpc8349-pmc"-compatible devices, the first resource is the PMC block interrupt. @@ -33,31 +38,35 @@ Properties: this is a phandle to an "fsl,gtm" node on which timer 4 can be used as a wakeup source from deep sleep. -Sleep specifiers: +Clock nodes: +The clock nodes are to describe the masks in PM controller registers for each +soc clock. +- fsl,pmcdr-mask: For "fsl,mpc8548-pmc"-compatible devices, the mask will be + ORed into PMCDR before suspend if the device using this clock is the wake-up + source and need to be running during low power mode; clear the mask if + otherwise. - fsl,mpc8349-pmc: Sleep specifiers consist of one cell. For each bit - that is set in the cell, the corresponding bit in SCCR will be saved - and cleared on suspend, and restored on resume. This sleep controller - supports disabling and resuming devices at any time. +- fsl,sccr-mask: For "fsl,mpc8349-pmc"-compatible devices, the corresponding + bit specified by the mask in SCCR will be saved and cleared on suspend, and + restored on resume. - fsl,mpc8536-pmc: Sleep specifiers consist of three cells, the third of - which will be ORed into PMCDR upon suspend, and cleared from PMCDR - upon resume. The first two cells are as described for fsl,mpc8578-pmc. - This sleep controller only supports disabling devices during system - sleep, or permanently. - - fsl,mpc8548-pmc: Sleep specifiers consist of one or two cells, the - first of which will be ORed into DEVDISR (and the second into - DEVDISR2, if present -- this cell should be zero or absent if the - hardware does not have DEVDISR2) upon a request for permanent device - disabling. This sleep controller does not support configuring devices - to disable during system sleep (unless supported by another compatible - match), or dynamically. +- fsl,devdisr-mask: Contain one or two cells, depending on the availability of + DEVDISR2 register. For compatible devices, the mask will be ORed into DEVDISR + or DEVDISR2 when the clock should be permenently disabled. Example: - power@b00 { - compatible = "fsl,mpc8313-pmc", "fsl,mpc8349-pmc"; - reg = <0xb00 0x100 0xa00 0x100>; - interrupts = <80 8>; + power@e0070 { + compatible = "fsl,mpc8536-pmc", "fsl,mpc8548-pmc"; + reg = <0xe0070 0x20>; + + etsec1_clk: soc-clk@24 { + fsl,pmcdr-mask = <0x00000080>; + }; + etsec2_clk: soc-clk@25 { + fsl,pmcdr-mask = <0x00000040>; + }; + etsec3_clk: soc-clk@26 { + fsl,pmcdr-mask = <0x00000020>; + }; };