* [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-05-07 23:48 ` Douglas Anderson 0 siblings, 0 replies; 23+ messages in thread From: Douglas Anderson @ 2019-05-07 23:48 UTC (permalink / raw) To: Kishon Vijay Abraham I, Heiko Stuebner Cc: hl, linux-rockchip, dbasehore, mka, ryandcase, groeck, Elaine Zhang, wxt, Douglas Anderson, linux-kernel, linux-arm-kernel While testing a newer kernel on rk3288-based Chromebooks I found that the power draw in suspend was higher on newer kernels compared to the downstream Chrome OS 3.14 kernel. Specifically the power of an rk3288-veyron-jerry board that I tested (as measured by the smart battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. I tracked the regression down to the fact that the "DP PHY" driver didn't exist in our downstream 3.14. We relied on the eDP driver to turn on the clock and relied on the fact that the power for the PHY was default turned on. Specifically the thing that caused the power regression was turning the eDP PHY _off_. Presumably there is some sort of power leak in the system and when we turn the PHY off something is leaching power from something else and causing excessive power draw. Doing a search through device trees shows that this PHY is only ever used on rk3288. Presumably this power leak is present on all rk3288-SoCs running upstream Linux so let's just whack the driver to make sure we never turn off power. We'll still leave the parts that turn _on_ the power and grab the clock, though. NOTES: A) If someone can identify what this power leak is and fix it in some other way we can revert this patch. B) If someone can show that their particular board doesn't have this power leak (maybe they have rails hooked up differently?) we can perhaps add a device tree property indicating that for some boards it's OK to turn this rail off. I don't want to add this property until I know of a board that needs it. Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- As far as I know Yakir (the original author) is no longer at Rockchip. I've added a few other Rockchip people and hopefully one of them can help direct even if they're not directly responsible. drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c b/drivers/phy/rockchip/phy-rockchip-dp.c index 8b267a746576..10bbcd69d6f5 100644 --- a/drivers/phy/rockchip/phy-rockchip-dp.c +++ b/drivers/phy/rockchip/phy-rockchip-dp.c @@ -35,7 +35,7 @@ struct rockchip_dp_phy { static int rockchip_set_phy_state(struct phy *phy, bool enable) { struct rockchip_dp_phy *dp = phy_get_drvdata(phy); - int ret; + int ret = 0; if (enable) { ret = regmap_write(dp->grf, GRF_SOC_CON12, @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool enable) } else { clk_disable_unprepare(dp->phy_24m); - ret = regmap_write(dp->grf, GRF_SOC_CON12, - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | - GRF_EDP_PHY_SIDDQ_OFF); + /* + * Intentionally don't turn SIDDQ off when disabling + * the PHY. There is a power leak on rk3288 and + * suspend power _increases_ by 5 mA if you turn this + * off. + */ } return ret; -- 2.21.0.1020.gf2820cf01a-goog ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-05-07 23:48 ` Douglas Anderson 0 siblings, 0 replies; 23+ messages in thread From: Douglas Anderson @ 2019-05-07 23:48 UTC (permalink / raw) To: Kishon Vijay Abraham I, Heiko Stuebner Cc: Elaine Zhang, hl, dbasehore, Douglas Anderson, linux-kernel, linux-rockchip, mka, ryandcase, groeck, linux-arm-kernel, wxt While testing a newer kernel on rk3288-based Chromebooks I found that the power draw in suspend was higher on newer kernels compared to the downstream Chrome OS 3.14 kernel. Specifically the power of an rk3288-veyron-jerry board that I tested (as measured by the smart battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. I tracked the regression down to the fact that the "DP PHY" driver didn't exist in our downstream 3.14. We relied on the eDP driver to turn on the clock and relied on the fact that the power for the PHY was default turned on. Specifically the thing that caused the power regression was turning the eDP PHY _off_. Presumably there is some sort of power leak in the system and when we turn the PHY off something is leaching power from something else and causing excessive power draw. Doing a search through device trees shows that this PHY is only ever used on rk3288. Presumably this power leak is present on all rk3288-SoCs running upstream Linux so let's just whack the driver to make sure we never turn off power. We'll still leave the parts that turn _on_ the power and grab the clock, though. NOTES: A) If someone can identify what this power leak is and fix it in some other way we can revert this patch. B) If someone can show that their particular board doesn't have this power leak (maybe they have rails hooked up differently?) we can perhaps add a device tree property indicating that for some boards it's OK to turn this rail off. I don't want to add this property until I know of a board that needs it. Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- As far as I know Yakir (the original author) is no longer at Rockchip. I've added a few other Rockchip people and hopefully one of them can help direct even if they're not directly responsible. drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c b/drivers/phy/rockchip/phy-rockchip-dp.c index 8b267a746576..10bbcd69d6f5 100644 --- a/drivers/phy/rockchip/phy-rockchip-dp.c +++ b/drivers/phy/rockchip/phy-rockchip-dp.c @@ -35,7 +35,7 @@ struct rockchip_dp_phy { static int rockchip_set_phy_state(struct phy *phy, bool enable) { struct rockchip_dp_phy *dp = phy_get_drvdata(phy); - int ret; + int ret = 0; if (enable) { ret = regmap_write(dp->grf, GRF_SOC_CON12, @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool enable) } else { clk_disable_unprepare(dp->phy_24m); - ret = regmap_write(dp->grf, GRF_SOC_CON12, - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | - GRF_EDP_PHY_SIDDQ_OFF); + /* + * Intentionally don't turn SIDDQ off when disabling + * the PHY. There is a power leak on rk3288 and + * suspend power _increases_ by 5 mA if you turn this + * off. + */ } return ret; -- 2.21.0.1020.gf2820cf01a-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on 2019-05-07 23:48 ` Douglas Anderson @ 2019-05-17 23:57 ` Doug Anderson -1 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-05-17 23:57 UTC (permalink / raw) To: Elaine Zhang, Caesar Cc: Lin Huang, open list:ARM/Rockchip SoC..., Derek Basehore, Matthias Kaehlcke, Ryan Case, Guenter Roeck, LKML, Linux ARM, Kishon Vijay Abraham I, Heiko Stuebner Elaine and Caesar, On Tue, May 7, 2019 at 4:50 PM Douglas Anderson <dianders@chromium.org> wrote: > > While testing a newer kernel on rk3288-based Chromebooks I found that > the power draw in suspend was higher on newer kernels compared to the > downstream Chrome OS 3.14 kernel. Specifically the power of an > rk3288-veyron-jerry board that I tested (as measured by the smart > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > I tracked the regression down to the fact that the "DP PHY" driver > didn't exist in our downstream 3.14. We relied on the eDP driver to > turn on the clock and relied on the fact that the power for the PHY > was default turned on. > > Specifically the thing that caused the power regression was turning > the eDP PHY _off_. Presumably there is some sort of power leak in the > system and when we turn the PHY off something is leaching power from > something else and causing excessive power draw. > > Doing a search through device trees shows that this PHY is only ever > used on rk3288. Presumably this power leak is present on all > rk3288-SoCs running upstream Linux so let's just whack the driver to > make sure we never turn off power. We'll still leave the parts that > turn _on_ the power and grab the clock, though. > > NOTES: > A) If someone can identify what this power leak is and fix it in some > other way we can revert this patch. > B) If someone can show that their particular board doesn't have this > power leak (maybe they have rails hooked up differently?) we can > perhaps add a device tree property indicating that for some boards > it's OK to turn this rail off. I don't want to add this property > until I know of a board that needs it. > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > As far as I know Yakir (the original author) is no longer at Rockchip. > I've added a few other Rockchip people and hopefully one of them can > help direct even if they're not directly responsible. > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) Can you help direct this to the right person? ...or should we just land it and assume it's fine? -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-05-17 23:57 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-05-17 23:57 UTC (permalink / raw) To: Elaine Zhang, Caesar Cc: Lin Huang, Derek Basehore, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, Guenter Roeck, Linux ARM, Heiko Stuebner Elaine and Caesar, On Tue, May 7, 2019 at 4:50 PM Douglas Anderson <dianders@chromium.org> wrote: > > While testing a newer kernel on rk3288-based Chromebooks I found that > the power draw in suspend was higher on newer kernels compared to the > downstream Chrome OS 3.14 kernel. Specifically the power of an > rk3288-veyron-jerry board that I tested (as measured by the smart > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > I tracked the regression down to the fact that the "DP PHY" driver > didn't exist in our downstream 3.14. We relied on the eDP driver to > turn on the clock and relied on the fact that the power for the PHY > was default turned on. > > Specifically the thing that caused the power regression was turning > the eDP PHY _off_. Presumably there is some sort of power leak in the > system and when we turn the PHY off something is leaching power from > something else and causing excessive power draw. > > Doing a search through device trees shows that this PHY is only ever > used on rk3288. Presumably this power leak is present on all > rk3288-SoCs running upstream Linux so let's just whack the driver to > make sure we never turn off power. We'll still leave the parts that > turn _on_ the power and grab the clock, though. > > NOTES: > A) If someone can identify what this power leak is and fix it in some > other way we can revert this patch. > B) If someone can show that their particular board doesn't have this > power leak (maybe they have rails hooked up differently?) we can > perhaps add a device tree property indicating that for some boards > it's OK to turn this rail off. I don't want to add this property > until I know of a board that needs it. > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > As far as I know Yakir (the original author) is no longer at Rockchip. > I've added a few other Rockchip people and hopefully one of them can > help direct even if they're not directly responsible. > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) Can you help direct this to the right person? ...or should we just land it and assume it's fine? -Doug _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on 2019-05-17 23:57 ` Doug Anderson @ 2019-05-18 7:51 ` Heiko Stuebner -1 siblings, 0 replies; 23+ messages in thread From: Heiko Stuebner @ 2019-05-18 7:51 UTC (permalink / raw) To: Doug Anderson, Guenter Roeck, LKML, Linux ARM, Kishon Vijay Abraham I Cc: Elaine Zhang, Caesar, Lin Huang, open list:ARM/Rockchip SoC..., Derek Basehore, Matthias Kaehlcke, Ryan Case Hi, Am Samstag, 18. Mai 2019, 01:57:47 CEST schrieb Doug Anderson: > Elaine and Caesar, > > On Tue, May 7, 2019 at 4:50 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > While testing a newer kernel on rk3288-based Chromebooks I found that > > the power draw in suspend was higher on newer kernels compared to the > > downstream Chrome OS 3.14 kernel. Specifically the power of an > > rk3288-veyron-jerry board that I tested (as measured by the smart > > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > > > I tracked the regression down to the fact that the "DP PHY" driver > > didn't exist in our downstream 3.14. We relied on the eDP driver to > > turn on the clock and relied on the fact that the power for the PHY > > was default turned on. > > > > Specifically the thing that caused the power regression was turning > > the eDP PHY _off_. Presumably there is some sort of power leak in the > > system and when we turn the PHY off something is leaching power from > > something else and causing excessive power draw. > > > > Doing a search through device trees shows that this PHY is only ever > > used on rk3288. Presumably this power leak is present on all > > rk3288-SoCs running upstream Linux so let's just whack the driver to > > make sure we never turn off power. We'll still leave the parts that > > turn _on_ the power and grab the clock, though. > > > > NOTES: > > A) If someone can identify what this power leak is and fix it in some > > other way we can revert this patch. > > B) If someone can show that their particular board doesn't have this > > power leak (maybe they have rails hooked up differently?) we can > > perhaps add a device tree property indicating that for some boards > > it's OK to turn this rail off. I don't want to add this property > > until I know of a board that needs it. > > > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > As far as I know Yakir (the original author) is no longer at Rockchip. > > I've added a few other Rockchip people and hopefully one of them can > > help direct even if they're not directly responsible. > > > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > Can you help direct this to the right person? ...or should we just > land it and assume it's fine? I tink Kishon as phy-maintainer is the correct person to take on this patch, but maybe he's waiting for the merge-window to be over. Heiko ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-05-18 7:51 ` Heiko Stuebner 0 siblings, 0 replies; 23+ messages in thread From: Heiko Stuebner @ 2019-05-18 7:51 UTC (permalink / raw) To: Doug Anderson, Guenter Roeck, LKML, Linux ARM, Kishon Vijay Abraham I Cc: Elaine Zhang, Lin Huang, Derek Basehore, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, Caesar Hi, Am Samstag, 18. Mai 2019, 01:57:47 CEST schrieb Doug Anderson: > Elaine and Caesar, > > On Tue, May 7, 2019 at 4:50 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > While testing a newer kernel on rk3288-based Chromebooks I found that > > the power draw in suspend was higher on newer kernels compared to the > > downstream Chrome OS 3.14 kernel. Specifically the power of an > > rk3288-veyron-jerry board that I tested (as measured by the smart > > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > > > I tracked the regression down to the fact that the "DP PHY" driver > > didn't exist in our downstream 3.14. We relied on the eDP driver to > > turn on the clock and relied on the fact that the power for the PHY > > was default turned on. > > > > Specifically the thing that caused the power regression was turning > > the eDP PHY _off_. Presumably there is some sort of power leak in the > > system and when we turn the PHY off something is leaching power from > > something else and causing excessive power draw. > > > > Doing a search through device trees shows that this PHY is only ever > > used on rk3288. Presumably this power leak is present on all > > rk3288-SoCs running upstream Linux so let's just whack the driver to > > make sure we never turn off power. We'll still leave the parts that > > turn _on_ the power and grab the clock, though. > > > > NOTES: > > A) If someone can identify what this power leak is and fix it in some > > other way we can revert this patch. > > B) If someone can show that their particular board doesn't have this > > power leak (maybe they have rails hooked up differently?) we can > > perhaps add a device tree property indicating that for some boards > > it's OK to turn this rail off. I don't want to add this property > > until I know of a board that needs it. > > > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > As far as I know Yakir (the original author) is no longer at Rockchip. > > I've added a few other Rockchip people and hopefully one of them can > > help direct even if they're not directly responsible. > > > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > Can you help direct this to the right person? ...or should we just > land it and assume it's fine? I tink Kishon as phy-maintainer is the correct person to take on this patch, but maybe he's waiting for the merge-window to be over. Heiko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on 2019-05-18 7:51 ` Heiko Stuebner @ 2019-05-18 13:42 ` Doug Anderson -1 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-05-18 13:42 UTC (permalink / raw) To: Heiko Stuebner Cc: Guenter Roeck, LKML, Linux ARM, Kishon Vijay Abraham I, Elaine Zhang, Caesar, Lin Huang, open list:ARM/Rockchip SoC..., Derek Basehore, Matthias Kaehlcke, Ryan Case Hi, On Sat, May 18, 2019 at 12:51 AM Heiko Stuebner <heiko@sntech.de> wrote: > > Hi, > > Am Samstag, 18. Mai 2019, 01:57:47 CEST schrieb Doug Anderson: > > Elaine and Caesar, > > > > On Tue, May 7, 2019 at 4:50 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > While testing a newer kernel on rk3288-based Chromebooks I found that > > > the power draw in suspend was higher on newer kernels compared to the > > > downstream Chrome OS 3.14 kernel. Specifically the power of an > > > rk3288-veyron-jerry board that I tested (as measured by the smart > > > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > > > > > I tracked the regression down to the fact that the "DP PHY" driver > > > didn't exist in our downstream 3.14. We relied on the eDP driver to > > > turn on the clock and relied on the fact that the power for the PHY > > > was default turned on. > > > > > > Specifically the thing that caused the power regression was turning > > > the eDP PHY _off_. Presumably there is some sort of power leak in the > > > system and when we turn the PHY off something is leaching power from > > > something else and causing excessive power draw. > > > > > > Doing a search through device trees shows that this PHY is only ever > > > used on rk3288. Presumably this power leak is present on all > > > rk3288-SoCs running upstream Linux so let's just whack the driver to > > > make sure we never turn off power. We'll still leave the parts that > > > turn _on_ the power and grab the clock, though. > > > > > > NOTES: > > > A) If someone can identify what this power leak is and fix it in some > > > other way we can revert this patch. > > > B) If someone can show that their particular board doesn't have this > > > power leak (maybe they have rails hooked up differently?) we can > > > perhaps add a device tree property indicating that for some boards > > > it's OK to turn this rail off. I don't want to add this property > > > until I know of a board that needs it. > > > > > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > As far as I know Yakir (the original author) is no longer at Rockchip. > > > I've added a few other Rockchip people and hopefully one of them can > > > help direct even if they're not directly responsible. > > > > > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > Can you help direct this to the right person? ...or should we just > > land it and assume it's fine? > > I tink Kishon as phy-maintainer is the correct person to take on this > patch, but maybe he's waiting for the merge-window to be over. Yeah, definitely Kishon should be the one to land. I was kinda hoping to get confirmation from the Rockchip guys that this was a good idea across the board for rk3288 since I can only really test rk3288-veyron. They'd have access to the SoC design and could tell more about what this bit actually does in the SoC. ...in any case, if they don't respond then presumably we'd be good to land once the merge window is over and Kishon is landing patches again. -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-05-18 13:42 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-05-18 13:42 UTC (permalink / raw) To: Heiko Stuebner Cc: Derek Basehore, Lin Huang, Elaine Zhang, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Matthias Kaehlcke, Ryan Case, Guenter Roeck, Linux ARM, Caesar Hi, On Sat, May 18, 2019 at 12:51 AM Heiko Stuebner <heiko@sntech.de> wrote: > > Hi, > > Am Samstag, 18. Mai 2019, 01:57:47 CEST schrieb Doug Anderson: > > Elaine and Caesar, > > > > On Tue, May 7, 2019 at 4:50 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > While testing a newer kernel on rk3288-based Chromebooks I found that > > > the power draw in suspend was higher on newer kernels compared to the > > > downstream Chrome OS 3.14 kernel. Specifically the power of an > > > rk3288-veyron-jerry board that I tested (as measured by the smart > > > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > > > > > I tracked the regression down to the fact that the "DP PHY" driver > > > didn't exist in our downstream 3.14. We relied on the eDP driver to > > > turn on the clock and relied on the fact that the power for the PHY > > > was default turned on. > > > > > > Specifically the thing that caused the power regression was turning > > > the eDP PHY _off_. Presumably there is some sort of power leak in the > > > system and when we turn the PHY off something is leaching power from > > > something else and causing excessive power draw. > > > > > > Doing a search through device trees shows that this PHY is only ever > > > used on rk3288. Presumably this power leak is present on all > > > rk3288-SoCs running upstream Linux so let's just whack the driver to > > > make sure we never turn off power. We'll still leave the parts that > > > turn _on_ the power and grab the clock, though. > > > > > > NOTES: > > > A) If someone can identify what this power leak is and fix it in some > > > other way we can revert this patch. > > > B) If someone can show that their particular board doesn't have this > > > power leak (maybe they have rails hooked up differently?) we can > > > perhaps add a device tree property indicating that for some boards > > > it's OK to turn this rail off. I don't want to add this property > > > until I know of a board that needs it. > > > > > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > As far as I know Yakir (the original author) is no longer at Rockchip. > > > I've added a few other Rockchip people and hopefully one of them can > > > help direct even if they're not directly responsible. > > > > > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > Can you help direct this to the right person? ...or should we just > > land it and assume it's fine? > > I tink Kishon as phy-maintainer is the correct person to take on this > patch, but maybe he's waiting for the merge-window to be over. Yeah, definitely Kishon should be the one to land. I was kinda hoping to get confirmation from the Rockchip guys that this was a good idea across the board for rk3288 since I can only really test rk3288-veyron. They'd have access to the SoC design and could tell more about what this bit actually does in the SoC. ...in any case, if they don't respond then presumably we'd be good to land once the merge window is over and Kishon is landing patches again. -Doug _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on 2019-05-07 23:48 ` Douglas Anderson (?) @ 2019-05-20 8:04 ` Caesar Wang -1 siblings, 0 replies; 23+ messages in thread From: Caesar Wang @ 2019-05-20 8:04 UTC (permalink / raw) To: Douglas Anderson, Kishon Vijay Abraham I, Heiko Stuebner Cc: hl, linux-rockchip, dbasehore, mka, ryandcase, groeck, Elaine Zhang, linux-kernel, linux-arm-kernel, nickey.yang (nickey.yang@rock-chips.com), wzz, Huang Jiachai Hi Doug, For now, nobody of rockchip is responsible for this driver. Cc: Nickey, Zain, Hjc On 5/8/19 7:48 AM, Douglas Anderson wrote: > While testing a newer kernel on rk3288-based Chromebooks I found that > the power draw in suspend was higher on newer kernels compared to the > downstream Chrome OS 3.14 kernel. Specifically the power of an > rk3288-veyron-jerry board that I tested (as measured by the smart > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > I tracked the regression down to the fact that the "DP PHY" driver > didn't exist in our downstream 3.14. We relied on the eDP driver to > turn on the clock and relied on the fact that the power for the PHY > was default turned on. > > Specifically the thing that caused the power regression was turning > the eDP PHY _off_. Presumably there is some sort of power leak in the > system and when we turn the PHY off something is leaching power from > something else and causing excessive power draw. > > Doing a search through device trees shows that this PHY is only ever > used on rk3288. Presumably this power leak is present on all > rk3288-SoCs running upstream Linux so let's just whack the driver to > make sure we never turn off power. We'll still leave the parts that > turn _on_ the power and grab the clock, though. > > NOTES: > A) If someone can identify what this power leak is and fix it in some > other way we can revert this patch. > B) If someone can show that their particular board doesn't have this > power leak (maybe they have rails hooked up differently?) we can > perhaps add a device tree property indicating that for some boards > it's OK to turn this rail off. I don't want to add this property > until I know of a board that needs it. > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Caesar Wang <wxt@rock-chips.com> > --- > As far as I know Yakir (the original author) is no longer at Rockchip. > I've added a few other Rockchip people and hopefully one of them can > help direct even if they're not directly responsible. > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c b/drivers/phy/rockchip/phy-rockchip-dp.c > index 8b267a746576..10bbcd69d6f5 100644 > --- a/drivers/phy/rockchip/phy-rockchip-dp.c > +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > static int rockchip_set_phy_state(struct phy *phy, bool enable) > { > struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > - int ret; > + int ret = 0; > > if (enable) { > ret = regmap_write(dp->grf, GRF_SOC_CON12, > @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool enable) > } else { > clk_disable_unprepare(dp->phy_24m); > > - ret = regmap_write(dp->grf, GRF_SOC_CON12, > - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > - GRF_EDP_PHY_SIDDQ_OFF); > + /* > + * Intentionally don't turn SIDDQ off when disabling > + * the PHY. There is a power leak on rk3288 and > + * suspend power _increases_ by 5 mA if you turn this > + * off. > + */ As described by TRM, The “GRF_EDP_PHY_SIDDQ_OFF” that all circuits are power down, all IO are high-Z. That should make sure the PD_VIO[0] was disabled first, no active. But the rk3288 can't turn pd_vio off at the moment. [0] PD_VIO Which clock are device clocks: * clocks devices * *_IEP IEP:Image Enhancement Processor * *_ISP ISP:Image Signal Processing * *_VIP VIP:Video Input Processor * *_VOP* VOP:Visual Output Processor * *_RGA RGA * *_EDP* EDP * *_LVDS_* LVDS * *_HDMI HDMI * *_MIPI_* MIPI Thanks, -Caesar > } > > return ret; ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-05-20 8:04 ` Caesar Wang 0 siblings, 0 replies; 23+ messages in thread From: Caesar Wang @ 2019-05-20 8:04 UTC (permalink / raw) To: Douglas Anderson, Kishon Vijay Abraham I, Heiko Stuebner Cc: Elaine Zhang, hl, dbasehore, linux-kernel, Huang Jiachai, linux-rockchip, nickey.yang (nickey.yang@rock-chips.com), mka, ryandcase, groeck, wzz, linux-arm-kernel Hi Doug, For now, nobody of rockchip is responsible for this driver. Cc: Nickey, Zain, Hjc On 5/8/19 7:48 AM, Douglas Anderson wrote: > While testing a newer kernel on rk3288-based Chromebooks I found that > the power draw in suspend was higher on newer kernels compared to the > downstream Chrome OS 3.14 kernel. Specifically the power of an > rk3288-veyron-jerry board that I tested (as measured by the smart > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > I tracked the regression down to the fact that the "DP PHY" driver > didn't exist in our downstream 3.14. We relied on the eDP driver to > turn on the clock and relied on the fact that the power for the PHY > was default turned on. > > Specifically the thing that caused the power regression was turning > the eDP PHY _off_. Presumably there is some sort of power leak in the > system and when we turn the PHY off something is leaching power from > something else and causing excessive power draw. > > Doing a search through device trees shows that this PHY is only ever > used on rk3288. Presumably this power leak is present on all > rk3288-SoCs running upstream Linux so let's just whack the driver to > make sure we never turn off power. We'll still leave the parts that > turn _on_ the power and grab the clock, though. > > NOTES: > A) If someone can identify what this power leak is and fix it in some > other way we can revert this patch. > B) If someone can show that their particular board doesn't have this > power leak (maybe they have rails hooked up differently?) we can > perhaps add a device tree property indicating that for some boards > it's OK to turn this rail off. I don't want to add this property > until I know of a board that needs it. > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Caesar Wang <wxt@rock-chips.com> > --- > As far as I know Yakir (the original author) is no longer at Rockchip. > I've added a few other Rockchip people and hopefully one of them can > help direct even if they're not directly responsible. > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c b/drivers/phy/rockchip/phy-rockchip-dp.c > index 8b267a746576..10bbcd69d6f5 100644 > --- a/drivers/phy/rockchip/phy-rockchip-dp.c > +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > static int rockchip_set_phy_state(struct phy *phy, bool enable) > { > struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > - int ret; > + int ret = 0; > > if (enable) { > ret = regmap_write(dp->grf, GRF_SOC_CON12, > @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool enable) > } else { > clk_disable_unprepare(dp->phy_24m); > > - ret = regmap_write(dp->grf, GRF_SOC_CON12, > - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > - GRF_EDP_PHY_SIDDQ_OFF); > + /* > + * Intentionally don't turn SIDDQ off when disabling > + * the PHY. There is a power leak on rk3288 and > + * suspend power _increases_ by 5 mA if you turn this > + * off. > + */ As described by TRM, The “GRF_EDP_PHY_SIDDQ_OFF” that all circuits are power down, all IO are high-Z. That should make sure the PD_VIO[0] was disabled first, no active. But the rk3288 can't turn pd_vio off at the moment. [0] PD_VIO Which clock are device clocks: * clocks devices * *_IEP IEP:Image Enhancement Processor * *_ISP ISP:Image Signal Processing * *_VIP VIP:Video Input Processor * *_VOP* VOP:Visual Output Processor * *_RGA RGA * *_EDP* EDP * *_LVDS_* LVDS * *_HDMI HDMI * *_MIPI_* MIPI Thanks, -Caesar > } > > return ret; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-05-20 8:04 ` Caesar Wang 0 siblings, 0 replies; 23+ messages in thread From: Caesar Wang @ 2019-05-20 8:04 UTC (permalink / raw) To: Douglas Anderson, Kishon Vijay Abraham I, Heiko Stuebner Cc: Elaine Zhang, hl, dbasehore, linux-kernel, Huang Jiachai, linux-rockchip, nickey.yang (nickey.yang@rock-chips.com), mka, ryandcase, groeck, wzz, linux-arm-kernel Hi Doug, For now, nobody of rockchip is responsible for this driver. Cc: Nickey, Zain, Hjc On 5/8/19 7:48 AM, Douglas Anderson wrote: > While testing a newer kernel on rk3288-based Chromebooks I found that > the power draw in suspend was higher on newer kernels compared to the > downstream Chrome OS 3.14 kernel. Specifically the power of an > rk3288-veyron-jerry board that I tested (as measured by the smart > battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > I tracked the regression down to the fact that the "DP PHY" driver > didn't exist in our downstream 3.14. We relied on the eDP driver to > turn on the clock and relied on the fact that the power for the PHY > was default turned on. > > Specifically the thing that caused the power regression was turning > the eDP PHY _off_. Presumably there is some sort of power leak in the > system and when we turn the PHY off something is leaching power from > something else and causing excessive power draw. > > Doing a search through device trees shows that this PHY is only ever > used on rk3288. Presumably this power leak is present on all > rk3288-SoCs running upstream Linux so let's just whack the driver to > make sure we never turn off power. We'll still leave the parts that > turn _on_ the power and grab the clock, though. > > NOTES: > A) If someone can identify what this power leak is and fix it in some > other way we can revert this patch. > B) If someone can show that their particular board doesn't have this > power leak (maybe they have rails hooked up differently?) we can > perhaps add a device tree property indicating that for some boards > it's OK to turn this rail off. I don't want to add this property > until I know of a board that needs it. > > Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Caesar Wang <wxt@rock-chips.com> > --- > As far as I know Yakir (the original author) is no longer at Rockchip. > I've added a few other Rockchip people and hopefully one of them can > help direct even if they're not directly responsible. > > drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c b/drivers/phy/rockchip/phy-rockchip-dp.c > index 8b267a746576..10bbcd69d6f5 100644 > --- a/drivers/phy/rockchip/phy-rockchip-dp.c > +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > static int rockchip_set_phy_state(struct phy *phy, bool enable) > { > struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > - int ret; > + int ret = 0; > > if (enable) { > ret = regmap_write(dp->grf, GRF_SOC_CON12, > @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool enable) > } else { > clk_disable_unprepare(dp->phy_24m); > > - ret = regmap_write(dp->grf, GRF_SOC_CON12, > - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > - GRF_EDP_PHY_SIDDQ_OFF); > + /* > + * Intentionally don't turn SIDDQ off when disabling > + * the PHY. There is a power leak on rk3288 and > + * suspend power _increases_ by 5 mA if you turn this > + * off. > + */ As described by TRM, The “GRF_EDP_PHY_SIDDQ_OFF” that all circuits are power down, all IO are high-Z. That should make sure the PD_VIO[0] was disabled first, no active. But the rk3288 can't turn pd_vio off at the moment. [0] PD_VIO Which clock are device clocks: * clocks devices * *_IEP IEP:Image Enhancement Processor * *_ISP ISP:Image Signal Processing * *_VIP VIP:Video Input Processor * *_VOP* VOP:Visual Output Processor * *_RGA RGA * *_EDP* EDP * *_LVDS_* LVDS * *_HDMI HDMI * *_MIPI_* MIPI Thanks, -Caesar > } > > return ret; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-06-03 11:20 ` Kishon Vijay Abraham I 0 siblings, 0 replies; 23+ messages in thread From: Kishon Vijay Abraham I @ 2019-06-03 11:20 UTC (permalink / raw) To: Caesar Wang, Douglas Anderson, Heiko Stuebner Cc: hl, linux-rockchip, dbasehore, mka, ryandcase, groeck, Elaine Zhang, linux-kernel, linux-arm-kernel, nickey.yang (nickey.yang@rock-chips.com), wzz, Huang Jiachai Hi, On 20/05/19 1:34 PM, Caesar Wang wrote: > Hi Doug, > > For now, nobody of rockchip is responsible for this driver. > Cc: Nickey, Zain, Hjc > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: >> While testing a newer kernel on rk3288-based Chromebooks I found that >> the power draw in suspend was higher on newer kernels compared to the >> downstream Chrome OS 3.14 kernel. Specifically the power of an >> rk3288-veyron-jerry board that I tested (as measured by the smart >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. >> >> I tracked the regression down to the fact that the "DP PHY" driver >> didn't exist in our downstream 3.14. We relied on the eDP driver to >> turn on the clock and relied on the fact that the power for the PHY >> was default turned on. >> >> Specifically the thing that caused the power regression was turning >> the eDP PHY _off_. Presumably there is some sort of power leak in the >> system and when we turn the PHY off something is leaching power from >> something else and causing excessive power draw. >> >> Doing a search through device trees shows that this PHY is only ever >> used on rk3288. Presumably this power leak is present on all >> rk3288-SoCs running upstream Linux so let's just whack the driver to >> make sure we never turn off power. We'll still leave the parts that >> turn _on_ the power and grab the clock, though. >> >> NOTES: >> A) If someone can identify what this power leak is and fix it in some >> other way we can revert this patch. >> B) If someone can show that their particular board doesn't have this >> power leak (maybe they have rails hooked up differently?) we can >> perhaps add a device tree property indicating that for some boards >> it's OK to turn this rail off. I don't want to add this property >> until I know of a board that needs it. >> >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > >> --- >> As far as I know Yakir (the original author) is no longer at Rockchip. >> I've added a few other Rockchip people and hopefully one of them can >> help direct even if they're not directly responsible. >> >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c >> b/drivers/phy/rockchip/phy-rockchip-dp.c >> index 8b267a746576..10bbcd69d6f5 100644 >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { >> static int rockchip_set_phy_state(struct phy *phy, bool enable) >> { >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); >> - int ret; >> + int ret = 0; >> if (enable) { >> ret = regmap_write(dp->grf, GRF_SOC_CON12, >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool >> enable) >> } else { >> clk_disable_unprepare(dp->phy_24m); >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | >> - GRF_EDP_PHY_SIDDQ_OFF); >> + /* >> + * Intentionally don't turn SIDDQ off when disabling >> + * the PHY. There is a power leak on rk3288 and >> + * suspend power _increases_ by 5 mA if you turn this >> + * off. >> + */ Can someone in Rockchip try to find the root-cause of the issue? Keeping the PHY off shouldn't increase power draw. Thanks Kishon ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-06-03 11:20 ` Kishon Vijay Abraham I 0 siblings, 0 replies; 23+ messages in thread From: Kishon Vijay Abraham I @ 2019-06-03 11:20 UTC (permalink / raw) To: Caesar Wang, Douglas Anderson, Heiko Stuebner Cc: Elaine Zhang, hl, dbasehore, linux-kernel, Huang Jiachai, linux-rockchip, nickey.yang (nickey.yang@rock-chips.com), mka, ryandcase, groeck, wzz, linux-arm-kernel Hi, On 20/05/19 1:34 PM, Caesar Wang wrote: > Hi Doug, > > For now, nobody of rockchip is responsible for this driver. > Cc: Nickey, Zain, Hjc > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: >> While testing a newer kernel on rk3288-based Chromebooks I found that >> the power draw in suspend was higher on newer kernels compared to the >> downstream Chrome OS 3.14 kernel. Specifically the power of an >> rk3288-veyron-jerry board that I tested (as measured by the smart >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. >> >> I tracked the regression down to the fact that the "DP PHY" driver >> didn't exist in our downstream 3.14. We relied on the eDP driver to >> turn on the clock and relied on the fact that the power for the PHY >> was default turned on. >> >> Specifically the thing that caused the power regression was turning >> the eDP PHY _off_. Presumably there is some sort of power leak in the >> system and when we turn the PHY off something is leaching power from >> something else and causing excessive power draw. >> >> Doing a search through device trees shows that this PHY is only ever >> used on rk3288. Presumably this power leak is present on all >> rk3288-SoCs running upstream Linux so let's just whack the driver to >> make sure we never turn off power. We'll still leave the parts that >> turn _on_ the power and grab the clock, though. >> >> NOTES: >> A) If someone can identify what this power leak is and fix it in some >> other way we can revert this patch. >> B) If someone can show that their particular board doesn't have this >> power leak (maybe they have rails hooked up differently?) we can >> perhaps add a device tree property indicating that for some boards >> it's OK to turn this rail off. I don't want to add this property >> until I know of a board that needs it. >> >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > >> --- >> As far as I know Yakir (the original author) is no longer at Rockchip. >> I've added a few other Rockchip people and hopefully one of them can >> help direct even if they're not directly responsible. >> >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c >> b/drivers/phy/rockchip/phy-rockchip-dp.c >> index 8b267a746576..10bbcd69d6f5 100644 >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { >> static int rockchip_set_phy_state(struct phy *phy, bool enable) >> { >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); >> - int ret; >> + int ret = 0; >> if (enable) { >> ret = regmap_write(dp->grf, GRF_SOC_CON12, >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool >> enable) >> } else { >> clk_disable_unprepare(dp->phy_24m); >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | >> - GRF_EDP_PHY_SIDDQ_OFF); >> + /* >> + * Intentionally don't turn SIDDQ off when disabling >> + * the PHY. There is a power leak on rk3288 and >> + * suspend power _increases_ by 5 mA if you turn this >> + * off. >> + */ Can someone in Rockchip try to find the root-cause of the issue? Keeping the PHY off shouldn't increase power draw. Thanks Kishon _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-06-03 11:20 ` Kishon Vijay Abraham I 0 siblings, 0 replies; 23+ messages in thread From: Kishon Vijay Abraham I @ 2019-06-03 11:20 UTC (permalink / raw) To: Caesar Wang, Douglas Anderson, Heiko Stuebner Cc: Elaine Zhang, hl-TNX95d0MmH7DzftRWevZcw, dbasehore-F7+t8E8rja9g9hUCZPvPmw, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Huang Jiachai, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, nickey.yang (nickey.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org), mka-F7+t8E8rja9g9hUCZPvPmw, ryandcase-F7+t8E8rja9g9hUCZPvPmw, groeck-F7+t8E8rja9g9hUCZPvPmw, wzz, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Hi, On 20/05/19 1:34 PM, Caesar Wang wrote: > Hi Doug, > > For now, nobody of rockchip is responsible for this driver. > Cc: Nickey, Zain, Hjc > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: >> While testing a newer kernel on rk3288-based Chromebooks I found that >> the power draw in suspend was higher on newer kernels compared to the >> downstream Chrome OS 3.14 kernel. Specifically the power of an >> rk3288-veyron-jerry board that I tested (as measured by the smart >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. >> >> I tracked the regression down to the fact that the "DP PHY" driver >> didn't exist in our downstream 3.14. We relied on the eDP driver to >> turn on the clock and relied on the fact that the power for the PHY >> was default turned on. >> >> Specifically the thing that caused the power regression was turning >> the eDP PHY _off_. Presumably there is some sort of power leak in the >> system and when we turn the PHY off something is leaching power from >> something else and causing excessive power draw. >> >> Doing a search through device trees shows that this PHY is only ever >> used on rk3288. Presumably this power leak is present on all >> rk3288-SoCs running upstream Linux so let's just whack the driver to >> make sure we never turn off power. We'll still leave the parts that >> turn _on_ the power and grab the clock, though. >> >> NOTES: >> A) If someone can identify what this power leak is and fix it in some >> other way we can revert this patch. >> B) If someone can show that their particular board doesn't have this >> power leak (maybe they have rails hooked up differently?) we can >> perhaps add a device tree property indicating that for some boards >> it's OK to turn this rail off. I don't want to add this property >> until I know of a board that needs it. >> >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > >> --- >> As far as I know Yakir (the original author) is no longer at Rockchip. >> I've added a few other Rockchip people and hopefully one of them can >> help direct even if they're not directly responsible. >> >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c >> b/drivers/phy/rockchip/phy-rockchip-dp.c >> index 8b267a746576..10bbcd69d6f5 100644 >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { >> static int rockchip_set_phy_state(struct phy *phy, bool enable) >> { >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); >> - int ret; >> + int ret = 0; >> if (enable) { >> ret = regmap_write(dp->grf, GRF_SOC_CON12, >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool >> enable) >> } else { >> clk_disable_unprepare(dp->phy_24m); >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | >> - GRF_EDP_PHY_SIDDQ_OFF); >> + /* >> + * Intentionally don't turn SIDDQ off when disabling >> + * the PHY. There is a power leak on rk3288 and >> + * suspend power _increases_ by 5 mA if you turn this >> + * off. >> + */ Can someone in Rockchip try to find the root-cause of the issue? Keeping the PHY off shouldn't increase power draw. Thanks Kishon _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on 2019-06-03 11:20 ` Kishon Vijay Abraham I (?) @ 2019-06-03 15:22 ` Doug Anderson -1 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-06-03 15:22 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Caesar Wang, Heiko Stuebner, Lin Huang, open list:ARM/Rockchip SoC..., Derek Basehore, Matthias Kaehlcke, Ryan Case, Guenter Roeck, Elaine Zhang, LKML, Linux ARM, nickey.yang (nickey.yang@rock-chips.com), wzz, Huang Jiachai Kishon, On Mon, Jun 3, 2019 at 4:22 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi, > > On 20/05/19 1:34 PM, Caesar Wang wrote: > > Hi Doug, > > > > For now, nobody of rockchip is responsible for this driver. > > Cc: Nickey, Zain, Hjc > > > > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: > >> While testing a newer kernel on rk3288-based Chromebooks I found that > >> the power draw in suspend was higher on newer kernels compared to the > >> downstream Chrome OS 3.14 kernel. Specifically the power of an > >> rk3288-veyron-jerry board that I tested (as measured by the smart > >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > >> > >> I tracked the regression down to the fact that the "DP PHY" driver > >> didn't exist in our downstream 3.14. We relied on the eDP driver to > >> turn on the clock and relied on the fact that the power for the PHY > >> was default turned on. > >> > >> Specifically the thing that caused the power regression was turning > >> the eDP PHY _off_. Presumably there is some sort of power leak in the > >> system and when we turn the PHY off something is leaching power from > >> something else and causing excessive power draw. > >> > >> Doing a search through device trees shows that this PHY is only ever > >> used on rk3288. Presumably this power leak is present on all > >> rk3288-SoCs running upstream Linux so let's just whack the driver to > >> make sure we never turn off power. We'll still leave the parts that > >> turn _on_ the power and grab the clock, though. > >> > >> NOTES: > >> A) If someone can identify what this power leak is and fix it in some > >> other way we can revert this patch. > >> B) If someone can show that their particular board doesn't have this > >> power leak (maybe they have rails hooked up differently?) we can > >> perhaps add a device tree property indicating that for some boards > >> it's OK to turn this rail off. I don't want to add this property > >> until I know of a board that needs it. > >> > >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > > > >> --- > >> As far as I know Yakir (the original author) is no longer at Rockchip. > >> I've added a few other Rockchip people and hopefully one of them can > >> help direct even if they're not directly responsible. > >> > >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > >> 1 file changed, 7 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c > >> b/drivers/phy/rockchip/phy-rockchip-dp.c > >> index 8b267a746576..10bbcd69d6f5 100644 > >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c > >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > >> static int rockchip_set_phy_state(struct phy *phy, bool enable) > >> { > >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > >> - int ret; > >> + int ret = 0; > >> if (enable) { > >> ret = regmap_write(dp->grf, GRF_SOC_CON12, > >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool > >> enable) > >> } else { > >> clk_disable_unprepare(dp->phy_24m); > >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, > >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > >> - GRF_EDP_PHY_SIDDQ_OFF); > >> + /* > >> + * Intentionally don't turn SIDDQ off when disabling > >> + * the PHY. There is a power leak on rk3288 and > >> + * suspend power _increases_ by 5 mA if you turn this > >> + * off. > >> + */ > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > PHY off shouldn't increase power draw. It sounded like Chris already answered this, though? Basically things aren't hooked up in a way that this line can be turned safely turned off in rk3288 with the current state of the world. Chris says that there's an ordering problem where we've got to turn off PD_VIO _before_ we turn off SIDDQ. ...but PD_VIO is a power domain that contains much more than just eDP. So if we truly wanted to try to solve this we'd need to come up with a way to make sure PD_VIO got all the way off and then turn this off only afterwards. ...and right now on rk3288 it looks like we never actually turn off PD_VIO while the system is running. -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-06-03 15:22 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-06-03 15:22 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Elaine Zhang, Lin Huang, Derek Basehore, LKML, Huang Jiachai, open list:ARM/Rockchip SoC..., nickey.yang (nickey.yang@rock-chips.com), Matthias Kaehlcke, Ryan Case, wzz, Guenter Roeck, Caesar Wang, Linux ARM, Heiko Stuebner Kishon, On Mon, Jun 3, 2019 at 4:22 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi, > > On 20/05/19 1:34 PM, Caesar Wang wrote: > > Hi Doug, > > > > For now, nobody of rockchip is responsible for this driver. > > Cc: Nickey, Zain, Hjc > > > > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: > >> While testing a newer kernel on rk3288-based Chromebooks I found that > >> the power draw in suspend was higher on newer kernels compared to the > >> downstream Chrome OS 3.14 kernel. Specifically the power of an > >> rk3288-veyron-jerry board that I tested (as measured by the smart > >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > >> > >> I tracked the regression down to the fact that the "DP PHY" driver > >> didn't exist in our downstream 3.14. We relied on the eDP driver to > >> turn on the clock and relied on the fact that the power for the PHY > >> was default turned on. > >> > >> Specifically the thing that caused the power regression was turning > >> the eDP PHY _off_. Presumably there is some sort of power leak in the > >> system and when we turn the PHY off something is leaching power from > >> something else and causing excessive power draw. > >> > >> Doing a search through device trees shows that this PHY is only ever > >> used on rk3288. Presumably this power leak is present on all > >> rk3288-SoCs running upstream Linux so let's just whack the driver to > >> make sure we never turn off power. We'll still leave the parts that > >> turn _on_ the power and grab the clock, though. > >> > >> NOTES: > >> A) If someone can identify what this power leak is and fix it in some > >> other way we can revert this patch. > >> B) If someone can show that their particular board doesn't have this > >> power leak (maybe they have rails hooked up differently?) we can > >> perhaps add a device tree property indicating that for some boards > >> it's OK to turn this rail off. I don't want to add this property > >> until I know of a board that needs it. > >> > >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > > > >> --- > >> As far as I know Yakir (the original author) is no longer at Rockchip. > >> I've added a few other Rockchip people and hopefully one of them can > >> help direct even if they're not directly responsible. > >> > >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > >> 1 file changed, 7 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c > >> b/drivers/phy/rockchip/phy-rockchip-dp.c > >> index 8b267a746576..10bbcd69d6f5 100644 > >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c > >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > >> static int rockchip_set_phy_state(struct phy *phy, bool enable) > >> { > >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > >> - int ret; > >> + int ret = 0; > >> if (enable) { > >> ret = regmap_write(dp->grf, GRF_SOC_CON12, > >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool > >> enable) > >> } else { > >> clk_disable_unprepare(dp->phy_24m); > >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, > >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > >> - GRF_EDP_PHY_SIDDQ_OFF); > >> + /* > >> + * Intentionally don't turn SIDDQ off when disabling > >> + * the PHY. There is a power leak on rk3288 and > >> + * suspend power _increases_ by 5 mA if you turn this > >> + * off. > >> + */ > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > PHY off shouldn't increase power draw. It sounded like Chris already answered this, though? Basically things aren't hooked up in a way that this line can be turned safely turned off in rk3288 with the current state of the world. Chris says that there's an ordering problem where we've got to turn off PD_VIO _before_ we turn off SIDDQ. ...but PD_VIO is a power domain that contains much more than just eDP. So if we truly wanted to try to solve this we'd need to come up with a way to make sure PD_VIO got all the way off and then turn this off only afterwards. ...and right now on rk3288 it looks like we never actually turn off PD_VIO while the system is running. -Doug _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-06-03 15:22 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-06-03 15:22 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Elaine Zhang, Lin Huang, Derek Basehore, LKML, Huang Jiachai, open list:ARM/Rockchip SoC..., nickey.yang (nickey.yang@rock-chips.com), Matthias Kaehlcke, Ryan Case, wzz, Guenter Roeck, Caesar Wang, Linux ARM, Heiko Stuebner Kishon, On Mon, Jun 3, 2019 at 4:22 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi, > > On 20/05/19 1:34 PM, Caesar Wang wrote: > > Hi Doug, > > > > For now, nobody of rockchip is responsible for this driver. > > Cc: Nickey, Zain, Hjc > > > > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: > >> While testing a newer kernel on rk3288-based Chromebooks I found that > >> the power draw in suspend was higher on newer kernels compared to the > >> downstream Chrome OS 3.14 kernel. Specifically the power of an > >> rk3288-veyron-jerry board that I tested (as measured by the smart > >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > >> > >> I tracked the regression down to the fact that the "DP PHY" driver > >> didn't exist in our downstream 3.14. We relied on the eDP driver to > >> turn on the clock and relied on the fact that the power for the PHY > >> was default turned on. > >> > >> Specifically the thing that caused the power regression was turning > >> the eDP PHY _off_. Presumably there is some sort of power leak in the > >> system and when we turn the PHY off something is leaching power from > >> something else and causing excessive power draw. > >> > >> Doing a search through device trees shows that this PHY is only ever > >> used on rk3288. Presumably this power leak is present on all > >> rk3288-SoCs running upstream Linux so let's just whack the driver to > >> make sure we never turn off power. We'll still leave the parts that > >> turn _on_ the power and grab the clock, though. > >> > >> NOTES: > >> A) If someone can identify what this power leak is and fix it in some > >> other way we can revert this patch. > >> B) If someone can show that their particular board doesn't have this > >> power leak (maybe they have rails hooked up differently?) we can > >> perhaps add a device tree property indicating that for some boards > >> it's OK to turn this rail off. I don't want to add this property > >> until I know of a board that needs it. > >> > >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > > > >> --- > >> As far as I know Yakir (the original author) is no longer at Rockchip. > >> I've added a few other Rockchip people and hopefully one of them can > >> help direct even if they're not directly responsible. > >> > >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > >> 1 file changed, 7 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c > >> b/drivers/phy/rockchip/phy-rockchip-dp.c > >> index 8b267a746576..10bbcd69d6f5 100644 > >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c > >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > >> static int rockchip_set_phy_state(struct phy *phy, bool enable) > >> { > >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > >> - int ret; > >> + int ret = 0; > >> if (enable) { > >> ret = regmap_write(dp->grf, GRF_SOC_CON12, > >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool > >> enable) > >> } else { > >> clk_disable_unprepare(dp->phy_24m); > >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, > >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > >> - GRF_EDP_PHY_SIDDQ_OFF); > >> + /* > >> + * Intentionally don't turn SIDDQ off when disabling > >> + * the PHY. There is a power leak on rk3288 and > >> + * suspend power _increases_ by 5 mA if you turn this > >> + * off. > >> + */ > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > PHY off shouldn't increase power draw. It sounded like Chris already answered this, though? Basically things aren't hooked up in a way that this line can be turned safely turned off in rk3288 with the current state of the world. Chris says that there's an ordering problem where we've got to turn off PD_VIO _before_ we turn off SIDDQ. ...but PD_VIO is a power domain that contains much more than just eDP. So if we truly wanted to try to solve this we'd need to come up with a way to make sure PD_VIO got all the way off and then turn this off only afterwards. ...and right now on rk3288 it looks like we never actually turn off PD_VIO while the system is running. -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on 2019-06-03 15:22 ` Doug Anderson (?) @ 2019-06-03 15:23 ` Doug Anderson -1 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-06-03 15:23 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Caesar Wang, Heiko Stuebner, Lin Huang, open list:ARM/Rockchip SoC..., Derek Basehore, Matthias Kaehlcke, Ryan Case, Guenter Roeck, Elaine Zhang, LKML, Linux ARM, nickey.yang (nickey.yang@rock-chips.com), wzz, Huang Jiachai Hi, On Mon, Jun 3, 2019 at 8:22 AM Doug Anderson <dianders@chromium.org> wrote: > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > > PHY off shouldn't increase power draw. > > It sounded like Chris already answered this, though? Basically things Doh! Don't know why I said Chris when it was clearly Caesar that answered. Sorry Caesar! -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-06-03 15:23 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-06-03 15:23 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Elaine Zhang, Lin Huang, Derek Basehore, LKML, Huang Jiachai, open list:ARM/Rockchip SoC..., nickey.yang (nickey.yang@rock-chips.com), Matthias Kaehlcke, Ryan Case, wzz, Guenter Roeck, Caesar Wang, Linux ARM, Heiko Stuebner Hi, On Mon, Jun 3, 2019 at 8:22 AM Doug Anderson <dianders@chromium.org> wrote: > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > > PHY off shouldn't increase power draw. > > It sounded like Chris already answered this, though? Basically things Doh! Don't know why I said Chris when it was clearly Caesar that answered. Sorry Caesar! -Doug _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-06-03 15:23 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-06-03 15:23 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Caesar Wang, Heiko Stuebner, Lin Huang, open list:ARM/Rockchip SoC..., Derek Basehore, Matthias Kaehlcke, Ryan Case, Guenter Roeck, Elaine Zhang, LKML, Linux ARM, nickey.yang (nickey.yang@rock-chips.com), wzz, Huang Jiachai Hi, On Mon, Jun 3, 2019 at 8:22 AM Doug Anderson <dianders@chromium.org> wrote: > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > > PHY off shouldn't increase power draw. > > It sounded like Chris already answered this, though? Basically things Doh! Don't know why I said Chris when it was clearly Caesar that answered. Sorry Caesar! -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on 2019-06-03 15:22 ` Doug Anderson (?) @ 2019-07-22 21:52 ` Doug Anderson -1 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-07-22 21:52 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Caesar Wang, Heiko Stuebner, Lin Huang, open list:ARM/Rockchip SoC..., Derek Basehore, Matthias Kaehlcke, Ryan Case, Guenter Roeck, Elaine Zhang, LKML, Linux ARM, nickey.yang (nickey.yang@rock-chips.com), wzz, Huang Jiachai Kishon, On Mon, Jun 3, 2019 at 8:22 AM Doug Anderson <dianders@chromium.org> wrote: > > Kishon, > > On Mon, Jun 3, 2019 at 4:22 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > > > Hi, > > > > On 20/05/19 1:34 PM, Caesar Wang wrote: > > > Hi Doug, > > > > > > For now, nobody of rockchip is responsible for this driver. > > > Cc: Nickey, Zain, Hjc > > > > > > > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: > > >> While testing a newer kernel on rk3288-based Chromebooks I found that > > >> the power draw in suspend was higher on newer kernels compared to the > > >> downstream Chrome OS 3.14 kernel. Specifically the power of an > > >> rk3288-veyron-jerry board that I tested (as measured by the smart > > >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > >> > > >> I tracked the regression down to the fact that the "DP PHY" driver > > >> didn't exist in our downstream 3.14. We relied on the eDP driver to > > >> turn on the clock and relied on the fact that the power for the PHY > > >> was default turned on. > > >> > > >> Specifically the thing that caused the power regression was turning > > >> the eDP PHY _off_. Presumably there is some sort of power leak in the > > >> system and when we turn the PHY off something is leaching power from > > >> something else and causing excessive power draw. > > >> > > >> Doing a search through device trees shows that this PHY is only ever > > >> used on rk3288. Presumably this power leak is present on all > > >> rk3288-SoCs running upstream Linux so let's just whack the driver to > > >> make sure we never turn off power. We'll still leave the parts that > > >> turn _on_ the power and grab the clock, though. > > >> > > >> NOTES: > > >> A) If someone can identify what this power leak is and fix it in some > > >> other way we can revert this patch. > > >> B) If someone can show that their particular board doesn't have this > > >> power leak (maybe they have rails hooked up differently?) we can > > >> perhaps add a device tree property indicating that for some boards > > >> it's OK to turn this rail off. I don't want to add this property > > >> until I know of a board that needs it. > > >> > > >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > > > > > >> --- > > >> As far as I know Yakir (the original author) is no longer at Rockchip. > > >> I've added a few other Rockchip people and hopefully one of them can > > >> help direct even if they're not directly responsible. > > >> > > >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > > >> 1 file changed, 7 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c > > >> b/drivers/phy/rockchip/phy-rockchip-dp.c > > >> index 8b267a746576..10bbcd69d6f5 100644 > > >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c > > >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > > >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > > >> static int rockchip_set_phy_state(struct phy *phy, bool enable) > > >> { > > >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > > >> - int ret; > > >> + int ret = 0; > > >> if (enable) { > > >> ret = regmap_write(dp->grf, GRF_SOC_CON12, > > >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool > > >> enable) > > >> } else { > > >> clk_disable_unprepare(dp->phy_24m); > > >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, > > >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > > >> - GRF_EDP_PHY_SIDDQ_OFF); > > >> + /* > > >> + * Intentionally don't turn SIDDQ off when disabling > > >> + * the PHY. There is a power leak on rk3288 and > > >> + * suspend power _increases_ by 5 mA if you turn this > > >> + * off. > > >> + */ > > > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > > PHY off shouldn't increase power draw. > > It sounded like Caesar already answered this, though? Basically things > aren't hooked up in a way that this line can be turned safely turned > off in rk3288 with the current state of the world. Chris says that > there's an ordering problem where we've got to turn off PD_VIO > _before_ we turn off SIDDQ. ...but PD_VIO is a power domain that > contains much more than just eDP. So if we truly wanted to try to > solve this we'd need to come up with a way to make sure PD_VIO got all > the way off and then turn this off only afterwards. > > ...and right now on rk3288 it looks like we never actually turn off > PD_VIO while the system is running. Is now a good time to land this patch since 5.3-rc1 is out? Do you need me to re-send? Hopefully your concerns are all addressed? -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-07-22 21:52 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-07-22 21:52 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Elaine Zhang, Lin Huang, Derek Basehore, LKML, Huang Jiachai, open list:ARM/Rockchip SoC..., nickey.yang (nickey.yang@rock-chips.com), Matthias Kaehlcke, Ryan Case, wzz, Guenter Roeck, Caesar Wang, Linux ARM, Heiko Stuebner Kishon, On Mon, Jun 3, 2019 at 8:22 AM Doug Anderson <dianders@chromium.org> wrote: > > Kishon, > > On Mon, Jun 3, 2019 at 4:22 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > > > Hi, > > > > On 20/05/19 1:34 PM, Caesar Wang wrote: > > > Hi Doug, > > > > > > For now, nobody of rockchip is responsible for this driver. > > > Cc: Nickey, Zain, Hjc > > > > > > > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: > > >> While testing a newer kernel on rk3288-based Chromebooks I found that > > >> the power draw in suspend was higher on newer kernels compared to the > > >> downstream Chrome OS 3.14 kernel. Specifically the power of an > > >> rk3288-veyron-jerry board that I tested (as measured by the smart > > >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > >> > > >> I tracked the regression down to the fact that the "DP PHY" driver > > >> didn't exist in our downstream 3.14. We relied on the eDP driver to > > >> turn on the clock and relied on the fact that the power for the PHY > > >> was default turned on. > > >> > > >> Specifically the thing that caused the power regression was turning > > >> the eDP PHY _off_. Presumably there is some sort of power leak in the > > >> system and when we turn the PHY off something is leaching power from > > >> something else and causing excessive power draw. > > >> > > >> Doing a search through device trees shows that this PHY is only ever > > >> used on rk3288. Presumably this power leak is present on all > > >> rk3288-SoCs running upstream Linux so let's just whack the driver to > > >> make sure we never turn off power. We'll still leave the parts that > > >> turn _on_ the power and grab the clock, though. > > >> > > >> NOTES: > > >> A) If someone can identify what this power leak is and fix it in some > > >> other way we can revert this patch. > > >> B) If someone can show that their particular board doesn't have this > > >> power leak (maybe they have rails hooked up differently?) we can > > >> perhaps add a device tree property indicating that for some boards > > >> it's OK to turn this rail off. I don't want to add this property > > >> until I know of a board that needs it. > > >> > > >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > > > > > >> --- > > >> As far as I know Yakir (the original author) is no longer at Rockchip. > > >> I've added a few other Rockchip people and hopefully one of them can > > >> help direct even if they're not directly responsible. > > >> > > >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > > >> 1 file changed, 7 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c > > >> b/drivers/phy/rockchip/phy-rockchip-dp.c > > >> index 8b267a746576..10bbcd69d6f5 100644 > > >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c > > >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > > >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > > >> static int rockchip_set_phy_state(struct phy *phy, bool enable) > > >> { > > >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > > >> - int ret; > > >> + int ret = 0; > > >> if (enable) { > > >> ret = regmap_write(dp->grf, GRF_SOC_CON12, > > >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool > > >> enable) > > >> } else { > > >> clk_disable_unprepare(dp->phy_24m); > > >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, > > >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > > >> - GRF_EDP_PHY_SIDDQ_OFF); > > >> + /* > > >> + * Intentionally don't turn SIDDQ off when disabling > > >> + * the PHY. There is a power leak on rk3288 and > > >> + * suspend power _increases_ by 5 mA if you turn this > > >> + * off. > > >> + */ > > > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > > PHY off shouldn't increase power draw. > > It sounded like Caesar already answered this, though? Basically things > aren't hooked up in a way that this line can be turned safely turned > off in rk3288 with the current state of the world. Chris says that > there's an ordering problem where we've got to turn off PD_VIO > _before_ we turn off SIDDQ. ...but PD_VIO is a power domain that > contains much more than just eDP. So if we truly wanted to try to > solve this we'd need to come up with a way to make sure PD_VIO got all > the way off and then turn this off only afterwards. > > ...and right now on rk3288 it looks like we never actually turn off > PD_VIO while the system is running. Is now a good time to land this patch since 5.3-rc1 is out? Do you need me to re-send? Hopefully your concerns are all addressed? -Doug _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on @ 2019-07-22 21:52 ` Doug Anderson 0 siblings, 0 replies; 23+ messages in thread From: Doug Anderson @ 2019-07-22 21:52 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Elaine Zhang, Lin Huang, Derek Basehore, LKML, Huang Jiachai, open list:ARM/Rockchip SoC..., nickey.yang (nickey.yang@rock-chips.com), Matthias Kaehlcke, Ryan Case, wzz, Guenter Roeck, Caesar Wang, Linux ARM, Heiko Stuebner Kishon, On Mon, Jun 3, 2019 at 8:22 AM Doug Anderson <dianders@chromium.org> wrote: > > Kishon, > > On Mon, Jun 3, 2019 at 4:22 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > > > Hi, > > > > On 20/05/19 1:34 PM, Caesar Wang wrote: > > > Hi Doug, > > > > > > For now, nobody of rockchip is responsible for this driver. > > > Cc: Nickey, Zain, Hjc > > > > > > > > > On 5/8/19 7:48 AM, Douglas Anderson wrote: > > >> While testing a newer kernel on rk3288-based Chromebooks I found that > > >> the power draw in suspend was higher on newer kernels compared to the > > >> downstream Chrome OS 3.14 kernel. Specifically the power of an > > >> rk3288-veyron-jerry board that I tested (as measured by the smart > > >> battery) was ~16 mA on Chrome OS 3.14 and ~21 mA on a newer kernel. > > >> > > >> I tracked the regression down to the fact that the "DP PHY" driver > > >> didn't exist in our downstream 3.14. We relied on the eDP driver to > > >> turn on the clock and relied on the fact that the power for the PHY > > >> was default turned on. > > >> > > >> Specifically the thing that caused the power regression was turning > > >> the eDP PHY _off_. Presumably there is some sort of power leak in the > > >> system and when we turn the PHY off something is leaching power from > > >> something else and causing excessive power draw. > > >> > > >> Doing a search through device trees shows that this PHY is only ever > > >> used on rk3288. Presumably this power leak is present on all > > >> rk3288-SoCs running upstream Linux so let's just whack the driver to > > >> make sure we never turn off power. We'll still leave the parts that > > >> turn _on_ the power and grab the clock, though. > > >> > > >> NOTES: > > >> A) If someone can identify what this power leak is and fix it in some > > >> other way we can revert this patch. > > >> B) If someone can show that their particular board doesn't have this > > >> power leak (maybe they have rails hooked up differently?) we can > > >> perhaps add a device tree property indicating that for some boards > > >> it's OK to turn this rail off. I don't want to add this property > > >> until I know of a board that needs it. > > >> > > >> Fixes: fd968973de95 ("phy: Add driver for rockchip Display Port PHY") > > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > > > > Reviewed-by: Caesar Wang <wxt@rock-chips.com> > > > > > >> --- > > >> As far as I know Yakir (the original author) is no longer at Rockchip. > > >> I've added a few other Rockchip people and hopefully one of them can > > >> help direct even if they're not directly responsible. > > >> > > >> drivers/phy/rockchip/phy-rockchip-dp.c | 11 +++++++---- > > >> 1 file changed, 7 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/drivers/phy/rockchip/phy-rockchip-dp.c > > >> b/drivers/phy/rockchip/phy-rockchip-dp.c > > >> index 8b267a746576..10bbcd69d6f5 100644 > > >> --- a/drivers/phy/rockchip/phy-rockchip-dp.c > > >> +++ b/drivers/phy/rockchip/phy-rockchip-dp.c > > >> @@ -35,7 +35,7 @@ struct rockchip_dp_phy { > > >> static int rockchip_set_phy_state(struct phy *phy, bool enable) > > >> { > > >> struct rockchip_dp_phy *dp = phy_get_drvdata(phy); > > >> - int ret; > > >> + int ret = 0; > > >> if (enable) { > > >> ret = regmap_write(dp->grf, GRF_SOC_CON12, > > >> @@ -50,9 +50,12 @@ static int rockchip_set_phy_state(struct phy *phy, bool > > >> enable) > > >> } else { > > >> clk_disable_unprepare(dp->phy_24m); > > >> - ret = regmap_write(dp->grf, GRF_SOC_CON12, > > >> - GRF_EDP_PHY_SIDDQ_HIWORD_MASK | > > >> - GRF_EDP_PHY_SIDDQ_OFF); > > >> + /* > > >> + * Intentionally don't turn SIDDQ off when disabling > > >> + * the PHY. There is a power leak on rk3288 and > > >> + * suspend power _increases_ by 5 mA if you turn this > > >> + * off. > > >> + */ > > > > Can someone in Rockchip try to find the root-cause of the issue? Keeping the > > PHY off shouldn't increase power draw. > > It sounded like Caesar already answered this, though? Basically things > aren't hooked up in a way that this line can be turned safely turned > off in rk3288 with the current state of the world. Chris says that > there's an ordering problem where we've got to turn off PD_VIO > _before_ we turn off SIDDQ. ...but PD_VIO is a power domain that > contains much more than just eDP. So if we truly wanted to try to > solve this we'd need to come up with a way to make sure PD_VIO got all > the way off and then turn this off only afterwards. > > ...and right now on rk3288 it looks like we never actually turn off > PD_VIO while the system is running. Is now a good time to land this patch since 5.3-rc1 is out? Do you need me to re-send? Hopefully your concerns are all addressed? -Doug ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-07-22 21:59 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-07 23:48 [PATCH] phy: rockchip-dp: Avoid power leak by leaving the PHY power on Douglas Anderson 2019-05-07 23:48 ` Douglas Anderson 2019-05-17 23:57 ` Doug Anderson 2019-05-17 23:57 ` Doug Anderson 2019-05-18 7:51 ` Heiko Stuebner 2019-05-18 7:51 ` Heiko Stuebner 2019-05-18 13:42 ` Doug Anderson 2019-05-18 13:42 ` Doug Anderson 2019-05-20 8:04 ` Caesar Wang 2019-05-20 8:04 ` Caesar Wang 2019-05-20 8:04 ` Caesar Wang 2019-06-03 11:20 ` Kishon Vijay Abraham I 2019-06-03 11:20 ` Kishon Vijay Abraham I 2019-06-03 11:20 ` Kishon Vijay Abraham I 2019-06-03 15:22 ` Doug Anderson 2019-06-03 15:22 ` Doug Anderson 2019-06-03 15:22 ` Doug Anderson 2019-06-03 15:23 ` Doug Anderson 2019-06-03 15:23 ` Doug Anderson 2019-06-03 15:23 ` Doug Anderson 2019-07-22 21:52 ` Doug Anderson 2019-07-22 21:52 ` Doug Anderson 2019-07-22 21:52 ` Doug Anderson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.