| Message ID | 1499897779-12338-1-git-send-email-zjwu@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: > From: Hu Ziji <huziji@marvell.com> > > Add Xenon specific system-level suspend and resume support. > Especially during resume, re-configure Xenon specific registers > since registers setting will be lost in suspend if Xenon is power off. I recommend to start with deploying runtime PM support instead of system PM support. Then on top of such change, you should make use of the runtime PM centric path to get system sleep support for "free" (and thus all the nice benefits). Please have a look at sdhci-of-at91 for a good reference. Regarding sdhci-of-at91, there is currently a patch [1], which adds dealing with restoring context of registers after a system resume. Similar to what you need. Kind regards Uffe [1] https://patchwork.kernel.org/patch/9837951/ [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: > On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: > > From: Hu Ziji <huziji@marvell.com> > > > > Add Xenon specific system-level suspend and resume support. > > Especially during resume, re-configure Xenon specific registers > > since registers setting will be lost in suspend if Xenon is power off. > > I recommend to start with deploying runtime PM support instead of > system PM support. Then on top of such change, you should make use of > the runtime PM centric path to get system sleep support for "free" > (and thus all the nice benefits). I'm not sure whether runtime PM is useful for xenon case. The xenon HW support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, that's to say we even don't need to do anything but achieve the runtime PM gains. Thanks, Jisheng > > Please have a look at sdhci-of-at91 for a good reference. > Regarding sdhci-of-at91, there is currently a patch [1], which adds > dealing with restoring context of registers after a system resume. > Similar to what you need. > > Kind regards > Uffe > > [1] > https://patchwork.kernel.org/patch/9837951/ > > [...] > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: > Hi Ulf, > > On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: > >> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: >> > From: Hu Ziji <huziji@marvell.com> >> > >> > Add Xenon specific system-level suspend and resume support. >> > Especially during resume, re-configure Xenon specific registers >> > since registers setting will be lost in suspend if Xenon is power off. >> >> I recommend to start with deploying runtime PM support instead of >> system PM support. Then on top of such change, you should make use of >> the runtime PM centric path to get system sleep support for "free" >> (and thus all the nice benefits). > > I'm not sure whether runtime PM is useful for xenon case. The xenon HW > support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, that's > to say we even don't need to do anything but achieve the runtime PM gains. Yeah, but that's only internally managed by mmc controller. The clock will not be unprepared/disabled, from clock tree point of view. Isn't that also worth doing? Kind regards Uffe > > Thanks, > Jisheng > >> >> Please have a look at sdhci-of-at91 for a good reference. >> Regarding sdhci-of-at91, there is currently a patch [1], which adds >> dealing with restoring context of registers after a system resume. >> Similar to what you need. >> >> Kind regards >> Uffe >> >> [1] >> https://patchwork.kernel.org/patch/9837951/ >> >> [...] >> >> Kind regards >> Uffe > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote: > On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: > > Hi Ulf, > > > > On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: > > > >> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: > >> > From: Hu Ziji <huziji@marvell.com> > >> > > >> > Add Xenon specific system-level suspend and resume support. > >> > Especially during resume, re-configure Xenon specific registers > >> > since registers setting will be lost in suspend if Xenon is power off. > >> > >> I recommend to start with deploying runtime PM support instead of > >> system PM support. Then on top of such change, you should make use of > >> the runtime PM centric path to get system sleep support for "free" > >> (and thus all the nice benefits). > > > > I'm not sure whether runtime PM is useful for xenon case. The xenon HW > > support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, that's > > to say we even don't need to do anything but achieve the runtime PM gains. > > Yeah, but that's only internally managed by mmc controller. The clock > will not be unprepared/disabled, from clock tree point of view. Isn't > that also worth doing? > The HW is clock gated, the difference is clock itself. From power saving point of view, the gain is nearly zero. From latency point of view, could runtime PM introduce extra latency? I.E the time spent on sdhci_runtime_suspend_host and sdhci_runtime_resume_host. Thanks, Jisheng -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2017 at 12:13, Jisheng Zhang <jszhang@marvell.com> wrote: > On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote: > >> On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: >> > Hi Ulf, >> > >> > On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: >> > >> >> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: >> >> > From: Hu Ziji <huziji@marvell.com> >> >> > >> >> > Add Xenon specific system-level suspend and resume support. >> >> > Especially during resume, re-configure Xenon specific registers >> >> > since registers setting will be lost in suspend if Xenon is power off. >> >> >> >> I recommend to start with deploying runtime PM support instead of >> >> system PM support. Then on top of such change, you should make use of >> >> the runtime PM centric path to get system sleep support for "free" >> >> (and thus all the nice benefits). >> > >> > I'm not sure whether runtime PM is useful for xenon case. The xenon HW >> > support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, that's >> > to say we even don't need to do anything but achieve the runtime PM gains. >> >> Yeah, but that's only internally managed by mmc controller. The clock >> will not be unprepared/disabled, from clock tree point of view. Isn't >> that also worth doing? >> > > The HW is clock gated, the difference is clock itself. From power saving > point of view, the gain is nearly zero. From latency point of view, could I assume the clock you are talking about is the "core" clock? I then assumes that clock is used as the interface clock for the card? That makes me wonder, don't you have other device clocks to manage as well? Clocks that is provided to the controller to make it functional? > runtime PM introduce extra latency? I.E the time spent on > sdhci_runtime_suspend_host and sdhci_runtime_resume_host. There may be some latency, but exactly how much I don't know - and perhaps you could do some optimizations in that path. Moreover, you can make use of the runtime PM autosuspend feature, which I guess you know the benefits of. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2017 at 12:48, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 13 July 2017 at 12:13, Jisheng Zhang <jszhang@marvell.com> wrote: >> On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote: >> >>> On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: >>> > Hi Ulf, >>> > >>> > On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: >>> > >>> >> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: >>> >> > From: Hu Ziji <huziji@marvell.com> >>> >> > >>> >> > Add Xenon specific system-level suspend and resume support. >>> >> > Especially during resume, re-configure Xenon specific registers >>> >> > since registers setting will be lost in suspend if Xenon is power off. >>> >> >>> >> I recommend to start with deploying runtime PM support instead of >>> >> system PM support. Then on top of such change, you should make use of >>> >> the runtime PM centric path to get system sleep support for "free" >>> >> (and thus all the nice benefits). >>> > >>> > I'm not sure whether runtime PM is useful for xenon case. The xenon HW >>> > support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, that's >>> > to say we even don't need to do anything but achieve the runtime PM gains. >>> >>> Yeah, but that's only internally managed by mmc controller. The clock >>> will not be unprepared/disabled, from clock tree point of view. Isn't >>> that also worth doing? >>> >> >> The HW is clock gated, the difference is clock itself. From power saving >> point of view, the gain is nearly zero. From latency point of view, could > > I assume the clock you are talking about is the "core" clock? I then > assumes that clock is used as the interface clock for the card? > > That makes me wonder, don't you have other device clocks to manage as > well? Clocks that is provided to the controller to make it functional? Besides the clocks, you have the xenon mmc phy. Can't that also be put that in some low power mode at request in-activity? [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/13/2017 04:03 AM, Ulf Hansson wrote: > On 13 July 2017 at 12:48, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 13 July 2017 at 12:13, Jisheng Zhang <jszhang@marvell.com> wrote: >>> On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote: >>> >>>> On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: >>>>> Hi Ulf, >>>>> >>>>> On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: >>>>> >>>>>> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: >>>>>>> From: Hu Ziji <huziji@marvell.com> >>>>>>> >>>>>>> Add Xenon specific system-level suspend and resume support. >>>>>>> Especially during resume, re-configure Xenon specific registers >>>>>>> since registers setting will be lost in suspend if Xenon is power off. >>>>>> I recommend to start with deploying runtime PM support instead of >>>>>> system PM support. Then on top of such change, you should make use of >>>>>> the runtime PM centric path to get system sleep support for "free" >>>>>> (and thus all the nice benefits). >>>>> I'm not sure whether runtime PM is useful for xenon case. The xenon HW >>>>> support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, that's >>>>> to say we even don't need to do anything but achieve the runtime PM gains. >>>> Yeah, but that's only internally managed by mmc controller. The clock >>>> will not be unprepared/disabled, from clock tree point of view. Isn't >>>> that also worth doing? >>>> >>> The HW is clock gated, the difference is clock itself. From power saving >>> point of view, the gain is nearly zero. From latency point of view, could >> I assume the clock you are talking about is the "core" clock? I then >> assumes that clock is used as the interface clock for the card? >> >> That makes me wonder, don't you have other device clocks to manage as >> well? Clocks that is provided to the controller to make it functional? At first, really appreciate your quick and valuable feedback. The core clock in this driver is the clock provided by SOC to sdh controller, and there is a divider inside the controller to generate sdclk which provides to sd/emmc card. Actually there are two runtime power saving features inside the controller per my understanding. sdclk_idle_enable will cut the clock to sd/emmc card if sd bus idle for some time. auto_clkgate_enable means HW will auto gate the clock to sdh controller core logic. With SW runtime pm mechanism, compares with HW auto clock gating, the only difference is SW cut the source of sdh clock tree, external clock gating vs internal clock gating, there will be some benefits, but limited. Previously we enabled the runtime pm mechanism in our mobile products, which were using the same IP(some old version, including 3 sdh slots) with auto clock gating feature(the driver is sdhci-pxav3.c). The saving of power was about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No more than 1mA/1sdh slot. I read sdhci-of-at91 driver and your recommended patch, I got your point is using a light way for system sleep based on runtime pm feature. From SW perspective, kill two birds with one stone, it is good. But considering about the benefits, it is not that urgent to take runtime pm feature as a must, it is a better to have feature. System standby is a must feature, without this patch, the system can't work well after resume. Do you think it is reasonable to add complete standby support at first, then take runtime pm as a next step? > Besides the clocks, you have the xenon mmc phy. Can't that also be put > that in some low power mode at request in-activity? For the phy behavior, currently I don't see any SW operation for the lpm, I will check with HW guys about the behaviour. > > [...] > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2017 at 23:45, Zhoujie Wu <zjwu@marvell.com> wrote: > > > On 07/13/2017 04:03 AM, Ulf Hansson wrote: >> >> On 13 July 2017 at 12:48, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> >>> On 13 July 2017 at 12:13, Jisheng Zhang <jszhang@marvell.com> wrote: >>>> >>>> On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote: >>>> >>>>> On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: >>>>>> >>>>>> Hi Ulf, >>>>>> >>>>>> On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: >>>>>> >>>>>>> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: >>>>>>>> >>>>>>>> From: Hu Ziji <huziji@marvell.com> >>>>>>>> >>>>>>>> Add Xenon specific system-level suspend and resume support. >>>>>>>> Especially during resume, re-configure Xenon specific registers >>>>>>>> since registers setting will be lost in suspend if Xenon is power >>>>>>>> off. >>>>>>> >>>>>>> I recommend to start with deploying runtime PM support instead of >>>>>>> system PM support. Then on top of such change, you should make use of >>>>>>> the runtime PM centric path to get system sleep support for "free" >>>>>>> (and thus all the nice benefits). >>>>>> >>>>>> I'm not sure whether runtime PM is useful for xenon case. The xenon HW >>>>>> support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, >>>>>> that's >>>>>> to say we even don't need to do anything but achieve the runtime PM >>>>>> gains. >>>>> >>>>> Yeah, but that's only internally managed by mmc controller. The clock >>>>> will not be unprepared/disabled, from clock tree point of view. Isn't >>>>> that also worth doing? >>>>> >>>> The HW is clock gated, the difference is clock itself. From power saving >>>> point of view, the gain is nearly zero. From latency point of view, >>>> could >>> >>> I assume the clock you are talking about is the "core" clock? I then >>> assumes that clock is used as the interface clock for the card? >>> >>> That makes me wonder, don't you have other device clocks to manage as >>> well? Clocks that is provided to the controller to make it functional? > > At first, really appreciate your quick and valuable feedback. > The core clock in this driver is the clock provided by SOC to sdh > controller, and there is a divider inside the controller to generate sdclk > which provides to sd/emmc card. > Actually there are two runtime power saving features inside the controller > per my understanding. > sdclk_idle_enable will cut the clock to sd/emmc card if sd bus idle for some > time. auto_clkgate_enable means HW will auto gate the clock to sdh > controller core logic. I am not sure I get the second part here. The clock to shd is enabled via a call to clk_prepare_enable(). Unless you explicitly call clk_disable_unprepare() for it, no? How can any outer logic know when it can be gated? > With SW runtime pm mechanism, compares with HW auto clock gating, the only > difference is SW cut the source of sdh clock tree, external clock gating vs > internal clock gating, there will be some benefits, but limited. Right. > Previously we enabled the runtime pm mechanism in our mobile products, which > were using the same IP(some old version, including 3 sdh slots) with auto > clock gating feature(the driver is sdhci-pxav3.c). The saving of power was > about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No more > than 1mA/1sdh slot. 1 mA/sdh slot is a great reason to deploy runtime PM support. For a battery driven device that would be a significant improvement. Back in the days when I worked at ST-Ericssion, we were chasing uA when optimizing for power-save. :-) > I read sdhci-of-at91 driver and your recommended patch, I got your point is > using a light way for system sleep based on runtime pm feature. From SW > perspective, kill two birds with one stone, it is good. Right. > But considering about the benefits, it is not that urgent to take runtime pm > feature as a must, it is a better to have feature. System standby is a must > feature, without this patch, the system can't work well after resume. > Do you think it is reasonable to add complete standby support at first, then > take runtime pm as a next step? You can do that, but why? And will then the "next step" ever happen? Do you really want to spend efforts in getting something working for system suspend only, while you instead easily could deploy both runtime PM and system PM support at the same time? > >> Besides the clocks, you have the xenon mmc phy. Can't that also be put >> that in some low power mode at request in-activity? > > For the phy behavior, currently I don't see any SW operation for the lpm, I > will check with HW guys about the behaviour. Great, that would really be interesting from a runtime PM point of view. Perhaps then also ask if re-configuring the phy via xenon_phy_adj(), makes sense when powering off the card? Because currently you seems to keep the latest configuration, even if the mmc core decides to power off the card during system sleep. Unless I am reading the code wrong from the ->set_ios() ops. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-07-14 17:09 GMT+08:00 Ulf Hansson <ulf.hansson@linaro.org>: > On 13 July 2017 at 23:45, Zhoujie Wu <zjwu@marvell.com> wrote: >> >> >> On 07/13/2017 04:03 AM, Ulf Hansson wrote: >>> >>> On 13 July 2017 at 12:48, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> >>>> On 13 July 2017 at 12:13, Jisheng Zhang <jszhang@marvell.com> wrote: >>>>> >>>>> On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote: >>>>> >>>>>> On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: >>>>>>> >>>>>>> Hi Ulf, >>>>>>> >>>>>>> On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: >>>>>>> >>>>>>>> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: >>>>>>>>> >>>>>>>>> From: Hu Ziji <huziji@marvell.com> >>>>>>>>> >>>>>>>>> Add Xenon specific system-level suspend and resume support. >>>>>>>>> Especially during resume, re-configure Xenon specific registers >>>>>>>>> since registers setting will be lost in suspend if Xenon is power >>>>>>>>> off. >>>>>>>> >>>>>>>> I recommend to start with deploying runtime PM support instead of >>>>>>>> system PM support. Then on top of such change, you should make use of >>>>>>>> the runtime PM centric path to get system sleep support for "free" >>>>>>>> (and thus all the nice benefits). >>>>>>> >>>>>>> I'm not sure whether runtime PM is useful for xenon case. The xenon HW >>>>>>> support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, >>>>>>> that's >>>>>>> to say we even don't need to do anything but achieve the runtime PM >>>>>>> gains. >>>>>> >>>>>> Yeah, but that's only internally managed by mmc controller. The clock >>>>>> will not be unprepared/disabled, from clock tree point of view. Isn't >>>>>> that also worth doing? >>>>>> >>>>> The HW is clock gated, the difference is clock itself. From power saving >>>>> point of view, the gain is nearly zero. From latency point of view, >>>>> could >>>> >>>> I assume the clock you are talking about is the "core" clock? I then >>>> assumes that clock is used as the interface clock for the card? >>>> >>>> That makes me wonder, don't you have other device clocks to manage as >>>> well? Clocks that is provided to the controller to make it functional? >> >> At first, really appreciate your quick and valuable feedback. >> The core clock in this driver is the clock provided by SOC to sdh >> controller, and there is a divider inside the controller to generate sdclk >> which provides to sd/emmc card. >> Actually there are two runtime power saving features inside the controller >> per my understanding. >> sdclk_idle_enable will cut the clock to sd/emmc card if sd bus idle for some >> time. auto_clkgate_enable means HW will auto gate the clock to sdh >> controller core logic. > > I am not sure I get the second part here. The clock to shd is enabled > via a call to clk_prepare_enable(). Unless you explicitly call > clk_disable_unprepare() for it, no? How can any outer logic know when > it can be gated? > >> With SW runtime pm mechanism, compares with HW auto clock gating, the only >> difference is SW cut the source of sdh clock tree, external clock gating vs >> internal clock gating, there will be some benefits, but limited. > > Right. > >> Previously we enabled the runtime pm mechanism in our mobile products, which >> were using the same IP(some old version, including 3 sdh slots) with auto >> clock gating feature(the driver is sdhci-pxav3.c). The saving of power was >> about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No more >> than 1mA/1sdh slot. > > 1 mA/sdh slot is a great reason to deploy runtime PM support. For a > battery driven device that would be a significant improvement. > > Back in the days when I worked at ST-Ericssion, we were chasing uA > when optimizing for power-save. :-) > >> I read sdhci-of-at91 driver and your recommended patch, I got your point is >> using a light way for system sleep based on runtime pm feature. From SW >> perspective, kill two birds with one stone, it is good. > > Right. > >> But considering about the benefits, it is not that urgent to take runtime pm >> feature as a must, it is a better to have feature. System standby is a must >> feature, without this patch, the system can't work well after resume. >> Do you think it is reasonable to add complete standby support at first, then >> take runtime pm as a next step? > > You can do that, but why? And will then the "next step" ever happen? > > Do you really want to spend efforts in getting something working for > system suspend only, while you instead easily could deploy both > runtime PM and system PM support at the same time? > As the author of this patch, I'd like to argue for myself here, although it is not my task any more. :p The reason of implementing system PM firstly, is definitely NOT that runtime PM is unnecessary. Instead, I was just not able to. Internal customers in Marvell and external customers previously just required simple system PM. Thus I decided to meet customers requirement at first. Otherwise, I didn't have any platform to verify runtime PM even if runtime PM is completed. Those SoCs implementation vary a lot. Thus I was actually not sure what a generic Xenon runtime PM should look like for all those Marvell products. I previously planed to implement runtime PM when I got enough resources (boards/BSP/supports) from customers. >> >>> Besides the clocks, you have the xenon mmc phy. Can't that also be put >>> that in some low power mode at request in-activity? >> >> For the phy behavior, currently I don't see any SW operation for the lpm, I >> will check with HW guys about the behaviour. > > Great, that would really be interesting from a runtime PM point of view. > > Perhaps then also ask if re-configuring the phy via xenon_phy_adj(), > makes sense when powering off the card? Because currently you seems to > keep the latest configuration, even if the mmc core decides to power > off the card during system sleep. Unless I am reading the code wrong > from the ->set_ios() ops. > As I know, PHY HW will also automatically enter standby mode (power saving mode) when ACG/SDCLK-off-while-idle are triggered. Actually there is no SW interface to "enable" or "disable" PHY. It requires confirm from HW bros. xenon_phy_adj() will set PHY based on current timing. For example, when coming back from resume, xenon_phy_adj() will re-configure PHY for legacy timing. Thus it is unnecessary to clean PHY setting before sleep. > Kind regards > Uffe > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/14/2017 02:09 AM, Ulf Hansson wrote: > On 13 July 2017 at 23:45, Zhoujie Wu <zjwu@marvell.com> wrote: >> >> On 07/13/2017 04:03 AM, Ulf Hansson wrote: >>> On 13 July 2017 at 12:48, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> On 13 July 2017 at 12:13, Jisheng Zhang <jszhang@marvell.com> wrote: >>>>> On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote: >>>>> >>>>>> On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@marvell.com> wrote: >>>>>>> Hi Ulf, >>>>>>> >>>>>>> On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote: >>>>>>> >>>>>>>> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: >>>>>>>>> From: Hu Ziji <huziji@marvell.com> >>>>>>>>> >>>>>>>>> Add Xenon specific system-level suspend and resume support. >>>>>>>>> Especially during resume, re-configure Xenon specific registers >>>>>>>>> since registers setting will be lost in suspend if Xenon is power >>>>>>>>> off. >>>>>>>> I recommend to start with deploying runtime PM support instead of >>>>>>>> system PM support. Then on top of such change, you should make use of >>>>>>>> the runtime PM centric path to get system sleep support for "free" >>>>>>>> (and thus all the nice benefits). >>>>>>> I'm not sure whether runtime PM is useful for xenon case. The xenon HW >>>>>>> support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features, >>>>>>> that's >>>>>>> to say we even don't need to do anything but achieve the runtime PM >>>>>>> gains. >>>>>> Yeah, but that's only internally managed by mmc controller. The clock >>>>>> will not be unprepared/disabled, from clock tree point of view. Isn't >>>>>> that also worth doing? >>>>>> >>>>> The HW is clock gated, the difference is clock itself. From power saving >>>>> point of view, the gain is nearly zero. From latency point of view, >>>>> could >>>> I assume the clock you are talking about is the "core" clock? I then >>>> assumes that clock is used as the interface clock for the card? >>>> >>>> That makes me wonder, don't you have other device clocks to manage as >>>> well? Clocks that is provided to the controller to make it functional? >> At first, really appreciate your quick and valuable feedback. >> The core clock in this driver is the clock provided by SOC to sdh >> controller, and there is a divider inside the controller to generate sdclk >> which provides to sd/emmc card. >> Actually there are two runtime power saving features inside the controller >> per my understanding. >> sdclk_idle_enable will cut the clock to sd/emmc card if sd bus idle for some >> time. auto_clkgate_enable means HW will auto gate the clock to sdh >> controller core logic. > I am not sure I get the second part here. The clock to shd is enabled > via a call to clk_prepare_enable(). Unless you explicitly call > clk_disable_unprepare() for it, no? How can any outer logic know when > it can be gated? This is my understanding. Hope it can make you clear. The clock tree is like below. SOC --> [ SDH_CLK_GEN --> SDH_CONTROLLER ] --> SD/EMMC CARD There is one clock generator inside sdh slot IP, SOC provides the clock to the sdh slot IP. This clock is enabled/disabled by SW when calling clk_prepare_enable/clk_disable_unprepare. The auto clock gating is not any outer logic, it is inside sdh slot IP, when sdh controller has no activity, the IP will gate the clock from sdh_clk_gen to sdh_controller. sdh_clk_gen logic itself still has clock from SOC. With or without runtime pm, the only difference is if sdh_clk_gen has clock or not. So the power benefit is limited. > >> With SW runtime pm mechanism, compares with HW auto clock gating, the only >> difference is SW cut the source of sdh clock tree, external clock gating vs >> internal clock gating, there will be some benefits, but limited. > Right. > >> Previously we enabled the runtime pm mechanism in our mobile products, which >> were using the same IP(some old version, including 3 sdh slots) with auto >> clock gating feature(the driver is sdhci-pxav3.c). The saving of power was >> about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No more >> than 1mA/1sdh slot. > 1 mA/sdh slot is a great reason to deploy runtime PM support. For a > battery driven device that would be a significant improvement. > > Back in the days when I worked at ST-Ericssion, we were chasing uA > when optimizing for power-save. :-) Definitely for mobile products, but now I didn't see urgent requirement for our networking products. > >> I read sdhci-of-at91 driver and your recommended patch, I got your point is >> using a light way for system sleep based on runtime pm feature. From SW >> perspective, kill two birds with one stone, it is good. > Right. > >> But considering about the benefits, it is not that urgent to take runtime pm >> feature as a must, it is a better to have feature. System standby is a must >> feature, without this patch, the system can't work well after resume. >> Do you think it is reasonable to add complete standby support at first, then >> take runtime pm as a next step? > You can do that, but why? And will then the "next step" ever happen? > > Do you really want to spend efforts in getting something working for > system suspend only, while you instead easily could deploy both > runtime PM and system PM support at the same time? As Ziji said in another mail, it takes time for next step. The runtime pm need to be verified completely on all supported boards. I understand from SW perspective, we'd better have both. But I need input from internal customers to see if they only request system sleep or they want both, and what's their priority. > >>> Besides the clocks, you have the xenon mmc phy. Can't that also be put >>> that in some low power mode at request in-activity? >> For the phy behavior, currently I don't see any SW operation for the lpm, I >> will check with HW guys about the behaviour. > Great, that would really be interesting from a runtime PM point of view. > > Perhaps then also ask if re-configuring the phy via xenon_phy_adj(), > makes sense when powering off the card? Because currently you seems to > keep the latest configuration, even if the mmc core decides to power > off the card during system sleep. Unless I am reading the code wrong > from the ->set_ios() ops. > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >>> But considering about the benefits, it is not that urgent to take runtime pm >>> feature as a must, it is a better to have feature. System standby is a must >>> feature, without this patch, the system can't work well after resume. >>> Do you think it is reasonable to add complete standby support at first, then >>> take runtime pm as a next step? >> >> You can do that, but why? And will then the "next step" ever happen? >> >> Do you really want to spend efforts in getting something working for >> system suspend only, while you instead easily could deploy both >> runtime PM and system PM support at the same time? >> > > As the author of this patch, I'd like to argue for myself here, although it is > not my task any more. :p > The reason of implementing system PM firstly, is definitely NOT that > runtime PM is > unnecessary. Instead, I was just not able to. > > Internal customers in Marvell and external customers previously just required > simple system PM. Thus I decided to meet customers requirement at first. > Otherwise, I didn't have any platform to verify runtime PM even if runtime PM > is completed. > > Those SoCs implementation vary a lot. Thus I was actually not sure > what a generic Xenon runtime PM should look like for all those Marvell products. > I previously planed to implement runtime PM when I got enough resources > (boards/BSP/supports) from customers. If saving power during system sleep makes sense, then it seems odd that one don't want to care about saving power in runtime as well. Anyway, I rest my case and I won't prevent you from adding only system sleep support. > >>> >>>> Besides the clocks, you have the xenon mmc phy. Can't that also be put >>>> that in some low power mode at request in-activity? >>> >>> For the phy behavior, currently I don't see any SW operation for the lpm, I >>> will check with HW guys about the behaviour. >> >> Great, that would really be interesting from a runtime PM point of view. >> >> Perhaps then also ask if re-configuring the phy via xenon_phy_adj(), >> makes sense when powering off the card? Because currently you seems to >> keep the latest configuration, even if the mmc core decides to power >> off the card during system sleep. Unless I am reading the code wrong >> from the ->set_ios() ops. >> > > As I know, PHY HW will also automatically enter standby mode > (power saving mode) > when ACG/SDCLK-off-while-idle are triggered. Actually there is no SW interface > to "enable" or "disable" PHY. It requires confirm from HW bros. > > xenon_phy_adj() will set PHY based on current timing. For example, > when coming back > from resume, xenon_phy_adj() will re-configure PHY for legacy timing. > Thus it is unnecessary to clean PHY setting before sleep. Okay, thanks for clarifying. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> >> I am not sure I get the second part here. The clock to shd is enabled >> via a call to clk_prepare_enable(). Unless you explicitly call >> clk_disable_unprepare() for it, no? How can any outer logic know when >> it can be gated? > > > This is my understanding. Hope it can make you clear. > The clock tree is like below. > SOC --> [ SDH_CLK_GEN --> SDH_CONTROLLER ] --> SD/EMMC CARD > > There is one clock generator inside sdh slot IP, SOC provides the clock to > the sdh slot IP. This clock is enabled/disabled by SW when calling > clk_prepare_enable/clk_disable_unprepare. > The auto clock gating is not any outer logic, it is inside sdh slot IP, when > sdh controller has no activity, the IP will gate the clock from sdh_clk_gen > to sdh_controller. sdh_clk_gen logic itself still has clock from SOC. > With or without runtime pm, the only difference is if sdh_clk_gen has clock > or not. So the power benefit is limited. Thanks for clarifying! > >> >>> With SW runtime pm mechanism, compares with HW auto clock gating, the >>> only >>> difference is SW cut the source of sdh clock tree, external clock gating >>> vs >>> internal clock gating, there will be some benefits, but limited. >> >> Right. >> >>> Previously we enabled the runtime pm mechanism in our mobile products, >>> which >>> were using the same IP(some old version, including 3 sdh slots) with auto >>> clock gating feature(the driver is sdhci-pxav3.c). The saving of power >>> was >>> about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No >>> more >>> than 1mA/1sdh slot. >> >> 1 mA/sdh slot is a great reason to deploy runtime PM support. For a >> battery driven device that would be a significant improvement. >> >> Back in the days when I worked at ST-Ericssion, we were chasing uA >> when optimizing for power-save. :-) > > > Definitely for mobile products, but now I didn't see urgent requirement for > our networking products. I see. I think what puzzles me is that you do care about saving power in system sleep, but not during runtime. >> >> >>> I read sdhci-of-at91 driver and your recommended patch, I got your point >>> is >>> using a light way for system sleep based on runtime pm feature. From SW >>> perspective, kill two birds with one stone, it is good. >> >> Right. >> >>> But considering about the benefits, it is not that urgent to take runtime >>> pm >>> feature as a must, it is a better to have feature. System standby is a >>> must >>> feature, without this patch, the system can't work well after resume. >>> Do you think it is reasonable to add complete standby support at first, >>> then >>> take runtime pm as a next step? >> >> You can do that, but why? And will then the "next step" ever happen? >> >> Do you really want to spend efforts in getting something working for >> system suspend only, while you instead easily could deploy both >> runtime PM and system PM support at the same time? > > > As Ziji said in another mail, it takes time for next step. The runtime pm > need to be verified completely on all supported boards. > I understand from SW perspective, we'd better have both. But I need input > from internal customers to see if they only request system sleep or they > want both, and what's their priority. Okay. As I just responded in the other email, I rest my case. :-) [...] However, I need an ack from Adrian before I can apply this. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf, On 08/07/2017 02:23 AM, Ulf Hansson wrote: > [...] > >>> I am not sure I get the second part here. The clock to shd is enabled >>> via a call to clk_prepare_enable(). Unless you explicitly call >>> clk_disable_unprepare() for it, no? How can any outer logic know when >>> it can be gated? >> >> This is my understanding. Hope it can make you clear. >> The clock tree is like below. >> SOC --> [ SDH_CLK_GEN --> SDH_CONTROLLER ] --> SD/EMMC CARD >> >> There is one clock generator inside sdh slot IP, SOC provides the clock to >> the sdh slot IP. This clock is enabled/disabled by SW when calling >> clk_prepare_enable/clk_disable_unprepare. >> The auto clock gating is not any outer logic, it is inside sdh slot IP, when >> sdh controller has no activity, the IP will gate the clock from sdh_clk_gen >> to sdh_controller. sdh_clk_gen logic itself still has clock from SOC. >> With or without runtime pm, the only difference is if sdh_clk_gen has clock >> or not. So the power benefit is limited. > Thanks for clarifying! > >>>> With SW runtime pm mechanism, compares with HW auto clock gating, the >>>> only >>>> difference is SW cut the source of sdh clock tree, external clock gating >>>> vs >>>> internal clock gating, there will be some benefits, but limited. >>> Right. >>> >>>> Previously we enabled the runtime pm mechanism in our mobile products, >>>> which >>>> were using the same IP(some old version, including 3 sdh slots) with auto >>>> clock gating feature(the driver is sdhci-pxav3.c). The saving of power >>>> was >>>> about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No >>>> more >>>> than 1mA/1sdh slot. >>> 1 mA/sdh slot is a great reason to deploy runtime PM support. For a >>> battery driven device that would be a significant improvement. >>> >>> Back in the days when I worked at ST-Ericssion, we were chasing uA >>> when optimizing for power-save. :-) >> >> Definitely for mobile products, but now I didn't see urgent requirement for >> our networking products. > I see. > > I think what puzzles me is that you do care about saving power in > system sleep, but not during runtime. > >>> >>>> I read sdhci-of-at91 driver and your recommended patch, I got your point >>>> is >>>> using a light way for system sleep based on runtime pm feature. From SW >>>> perspective, kill two birds with one stone, it is good. >>> Right. >>> >>>> But considering about the benefits, it is not that urgent to take runtime >>>> pm >>>> feature as a must, it is a better to have feature. System standby is a >>>> must >>>> feature, without this patch, the system can't work well after resume. >>>> Do you think it is reasonable to add complete standby support at first, >>>> then >>>> take runtime pm as a next step? >>> You can do that, but why? And will then the "next step" ever happen? >>> >>> Do you really want to spend efforts in getting something working for >>> system suspend only, while you instead easily could deploy both >>> runtime PM and system PM support at the same time? >> >> As Ziji said in another mail, it takes time for next step. The runtime pm >> need to be verified completely on all supported boards. >> I understand from SW perspective, we'd better have both. But I need input >> from internal customers to see if they only request system sleep or they >> want both, and what's their priority. > Okay. As I just responded in the other email, I rest my case. :-) > > [...] > > However, I need an ack from Adrian before I can apply this. Thanks for your feedback. System level standby is mandatory requirement from our customer. It's nice you can merge it at first. For runtime PM, it's nice to have. Actually in the past two weeks I've already implemented and verified the basic function on our platform. But it took time for them for the full verification, I will submit runtime pm patch after hear feedback from them. > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/07/17 01:16, Zhoujie Wu wrote: > From: Hu Ziji <huziji@marvell.com> > > Add Xenon specific system-level suspend and resume support. > Especially during resume, re-configure Xenon specific registers > since registers setting will be lost in suspend if Xenon is power off. > > Signed-off-by: Hu Ziji <huziji@marvell.com> > Signed-off-by: Zhoujie Wu <zjwu@marvell.com> > Tested-by: Jisheng Zhang <jszhang@marvell.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > Hu Ziji has left marvell, submit the patch for him. > I will submit a patch to change xenon driver maintainer information and take over the ownership. > v2: > Add Signed-off-by Hu Ziji <huziji@marvell.com>. > v3: > Since Hu Ziji wrote the patch, correct the author information. > drivers/mmc/host/sdhci-xenon.c | 42 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c > index bc1781b..9f399c4 100644 > --- a/drivers/mmc/host/sdhci-xenon.c > +++ b/drivers/mmc/host/sdhci-xenon.c > @@ -519,6 +519,46 @@ static int xenon_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > +static int xenon_suspend(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + int ret; > + > + ret = sdhci_suspend_host(host); > + if (ret) > + return ret; > + > + clk_disable_unprepare(pltfm_host->clk); > + return ret; > +} > + > +static int xenon_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + int ret; > + > + ret = clk_prepare_enable(pltfm_host->clk); > + if (ret) > + return ret; > + > + /* > + * If SoCs power off the entire Xenon, registers setting will > + * be lost. > + * Re-configure Xenon specific register to enable current SDHC > + */ > + ret = xenon_sdhc_prepare(host); > + if (ret) > + return ret; > + > + return sdhci_resume_host(host); > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(xenon_pmops, xenon_suspend, xenon_resume); > + > static const struct of_device_id sdhci_xenon_dt_ids[] = { > { .compatible = "marvell,armada-ap806-sdhci",}, > { .compatible = "marvell,armada-cp110-sdhci",}, > @@ -531,7 +571,7 @@ static int xenon_remove(struct platform_device *pdev) > .driver = { > .name = "xenon-sdhci", > .of_match_table = sdhci_xenon_dt_ids, > - .pm = &sdhci_pltfm_pmops, > + .pm = &xenon_pmops, > }, > .probe = xenon_probe, > .remove = xenon_remove, > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@marvell.com> wrote: > > From: Hu Ziji <huziji@marvell.com> > > Add Xenon specific system-level suspend and resume support. > Especially during resume, re-configure Xenon specific registers > since registers setting will be lost in suspend if Xenon is power off. > > Signed-off-by: Hu Ziji <huziji@marvell.com> > Signed-off-by: Zhoujie Wu <zjwu@marvell.com> > Tested-by: Jisheng Zhang <jszhang@marvell.com> Thanks, applied for next! Kind regards Uffe > > --- > Hu Ziji has left marvell, submit the patch for him. > I will submit a patch to change xenon driver maintainer information and take over the ownership. > v2: > Add Signed-off-by Hu Ziji <huziji@marvell.com>. > v3: > Since Hu Ziji wrote the patch, correct the author information. > drivers/mmc/host/sdhci-xenon.c | 42 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c > index bc1781b..9f399c4 100644 > --- a/drivers/mmc/host/sdhci-xenon.c > +++ b/drivers/mmc/host/sdhci-xenon.c > @@ -519,6 +519,46 @@ static int xenon_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > +static int xenon_suspend(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + int ret; > + > + ret = sdhci_suspend_host(host); > + if (ret) > + return ret; > + > + clk_disable_unprepare(pltfm_host->clk); > + return ret; > +} > + > +static int xenon_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + int ret; > + > + ret = clk_prepare_enable(pltfm_host->clk); > + if (ret) > + return ret; > + > + /* > + * If SoCs power off the entire Xenon, registers setting will > + * be lost. > + * Re-configure Xenon specific register to enable current SDHC > + */ > + ret = xenon_sdhc_prepare(host); > + if (ret) > + return ret; > + > + return sdhci_resume_host(host); > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(xenon_pmops, xenon_suspend, xenon_resume); > + > static const struct of_device_id sdhci_xenon_dt_ids[] = { > { .compatible = "marvell,armada-ap806-sdhci",}, > { .compatible = "marvell,armada-cp110-sdhci",}, > @@ -531,7 +571,7 @@ static int xenon_remove(struct platform_device *pdev) > .driver = { > .name = "xenon-sdhci", > .of_match_table = sdhci_xenon_dt_ids, > - .pm = &sdhci_pltfm_pmops, > + .pm = &xenon_pmops, > }, > .probe = xenon_probe, > .remove = xenon_remove, > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c index bc1781b..9f399c4 100644 --- a/drivers/mmc/host/sdhci-xenon.c +++ b/drivers/mmc/host/sdhci-xenon.c @@ -519,6 +519,46 @@ static int xenon_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM_SLEEP +static int xenon_suspend(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + int ret; + + ret = sdhci_suspend_host(host); + if (ret) + return ret; + + clk_disable_unprepare(pltfm_host->clk); + return ret; +} + +static int xenon_resume(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + int ret; + + ret = clk_prepare_enable(pltfm_host->clk); + if (ret) + return ret; + + /* + * If SoCs power off the entire Xenon, registers setting will + * be lost. + * Re-configure Xenon specific register to enable current SDHC + */ + ret = xenon_sdhc_prepare(host); + if (ret) + return ret; + + return sdhci_resume_host(host); +} +#endif + +static SIMPLE_DEV_PM_OPS(xenon_pmops, xenon_suspend, xenon_resume); + static const struct of_device_id sdhci_xenon_dt_ids[] = { { .compatible = "marvell,armada-ap806-sdhci",}, { .compatible = "marvell,armada-cp110-sdhci",}, @@ -531,7 +571,7 @@ static int xenon_remove(struct platform_device *pdev) .driver = { .name = "xenon-sdhci", .of_match_table = sdhci_xenon_dt_ids, - .pm = &sdhci_pltfm_pmops, + .pm = &xenon_pmops, }, .probe = xenon_probe, .remove = xenon_remove,