* [v1] drm/msm/dsi/pll: call vco set rate explicitly @ 2020-02-06 9:12 Harigovindan P 2020-02-06 14:59 ` [Freedreno] " Jeffrey Hugo 0 siblings, 1 reply; 11+ messages in thread From: Harigovindan P @ 2020-02-06 9:12 UTC (permalink / raw) To: dri-devel, linux-arm-msm, freedreno, devicetree Cc: Harigovindan P, linux-kernel, robdclark, seanpaul, hoegsberg, kalyan_t, nganji For a given byte clock, if VCO recalc value is exactly same as vco set rate value, vco_set_rate does not get called assuming VCO is already set to required value. But Due to GDSC toggle, VCO values are erased in the HW. To make sure VCO is programmed correctly, we forcefully call set_rate from vco_prepare. Signed-off-by: Harigovindan P <harigovi@codeaurora.org> --- Changes in v1: - Calling set rate from vco_prepare drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c index 1c89454..6ac04fc 100644 --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c @@ -411,6 +411,12 @@ static int dsi_pll_10nm_vco_prepare(struct clk_hw *hw) if (pll_10nm->slave) dsi_pll_enable_pll_bias(pll_10nm->slave); + rc = dsi_pll_10nm_vco_set_rate(hw,pll_10nm->vco_current_rate, 0); + if (rc) { + pr_err("vco_set_rate failed, rc=%d\n", rc); + return rc; + } + /* Start PLL */ pll_write(pll_10nm->phy_cmn_mmio + REG_DSI_10nm_PHY_CMN_PLL_CNTRL, 0x01); -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-06 9:12 [v1] drm/msm/dsi/pll: call vco set rate explicitly Harigovindan P @ 2020-02-06 14:59 ` Jeffrey Hugo 2020-02-07 12:38 ` harigovi 0 siblings, 1 reply; 11+ messages in thread From: Jeffrey Hugo @ 2020-02-06 14:59 UTC (permalink / raw) To: Harigovindan P Cc: open list:DRM PANEL DRIVERS, MSM, freedreno, DTML, lkml, Rob Clark, nganji, Sean Paul, kalyan_t, Kristian H. Kristensen On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> wrote: > > For a given byte clock, if VCO recalc value is exactly same as > vco set rate value, vco_set_rate does not get called assuming > VCO is already set to required value. But Due to GDSC toggle, > VCO values are erased in the HW. To make sure VCO is programmed > correctly, we forcefully call set_rate from vco_prepare. Is this specific to certain SoCs? I don't think I've observed this. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-06 14:59 ` [Freedreno] " Jeffrey Hugo @ 2020-02-07 12:38 ` harigovi 2020-02-07 14:10 ` Jeffrey Hugo 0 siblings, 1 reply; 11+ messages in thread From: harigovi @ 2020-02-07 12:38 UTC (permalink / raw) To: Jeffrey Hugo Cc: open list:DRM PANEL DRIVERS, MSM, freedreno, DTML, lkml, Rob Clark, nganji, Sean Paul, kalyan_t, Kristian H. Kristensen On 2020-02-06 20:29, Jeffrey Hugo wrote: > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> > wrote: >> >> For a given byte clock, if VCO recalc value is exactly same as >> vco set rate value, vco_set_rate does not get called assuming >> VCO is already set to required value. But Due to GDSC toggle, >> VCO values are erased in the HW. To make sure VCO is programmed >> correctly, we forcefully call set_rate from vco_prepare. > > Is this specific to certain SoCs? I don't think I've observed this. As far as Qualcomm SOCs are concerned, since pll is analog and the value is directly read from hardware if we get recalc value same as set rate value, the vco_set_rate will not be invoked. We checked in our idp device which has the same SOC but it works there since the rates are different. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-07 12:38 ` harigovi @ 2020-02-07 14:10 ` Jeffrey Hugo 2020-02-11 5:58 ` harigovi 0 siblings, 1 reply; 11+ messages in thread From: Jeffrey Hugo @ 2020-02-07 14:10 UTC (permalink / raw) To: Harigovindan P Cc: open list:DRM PANEL DRIVERS, MSM, freedreno, DTML, lkml, Rob Clark, nganji, Sean Paul, kalyan_t, Kristian H. Kristensen On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: > > On 2020-02-06 20:29, Jeffrey Hugo wrote: > > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> > > wrote: > >> > >> For a given byte clock, if VCO recalc value is exactly same as > >> vco set rate value, vco_set_rate does not get called assuming > >> VCO is already set to required value. But Due to GDSC toggle, > >> VCO values are erased in the HW. To make sure VCO is programmed > >> correctly, we forcefully call set_rate from vco_prepare. > > > > Is this specific to certain SoCs? I don't think I've observed this. > > As far as Qualcomm SOCs are concerned, since pll is analog and the value > is directly read from hardware if we get recalc value same as set rate > value, the vco_set_rate will not be invoked. We checked in our idp > device which has the same SOC but it works there since the rates are > different. This doesn't seem to be an answer to my question. What Qualcomm SoCs does this issue apply to? Everything implementing the 10nm pll? One specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, so I'm interested to know what is the actual impact here. I don't see an "IDP" SoC in the IP catalog, so I really have no idea what you are referring to. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-07 14:10 ` Jeffrey Hugo @ 2020-02-11 5:58 ` harigovi 2020-02-11 15:44 ` Rob Clark 0 siblings, 1 reply; 11+ messages in thread From: harigovi @ 2020-02-11 5:58 UTC (permalink / raw) To: Jeffrey Hugo Cc: open list:DRM PANEL DRIVERS, MSM, freedreno, DTML, lkml, Rob Clark, nganji, Sean Paul, kalyan_t, Kristian H. Kristensen On 2020-02-07 19:40, Jeffrey Hugo wrote: > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: >> >> On 2020-02-06 20:29, Jeffrey Hugo wrote: >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> >> > wrote: >> >> >> >> For a given byte clock, if VCO recalc value is exactly same as >> >> vco set rate value, vco_set_rate does not get called assuming >> >> VCO is already set to required value. But Due to GDSC toggle, >> >> VCO values are erased in the HW. To make sure VCO is programmed >> >> correctly, we forcefully call set_rate from vco_prepare. >> > >> > Is this specific to certain SoCs? I don't think I've observed this. >> >> As far as Qualcomm SOCs are concerned, since pll is analog and the >> value >> is directly read from hardware if we get recalc value same as set rate >> value, the vco_set_rate will not be invoked. We checked in our idp >> device which has the same SOC but it works there since the rates are >> different. > > This doesn't seem to be an answer to my question. What Qualcomm SoCs > does this issue apply to? Everything implementing the 10nm pll? One > specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, > so I'm interested to know what is the actual impact here. I don't see > an "IDP" SoC in the IP catalog, so I really have no idea what you are > referring to. This is not 10nm specific. It is applicable for other nms also. Its specific to the frequency being set. If vco_recalc returns the same value as being set by vco_set_rate, vco_set_rate will not be invoked second time onwards. For example: Lets take below devices: Cheza is based on SDM845 which is 10nm only. Clk frequency:206016 dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000 dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947 Trogdor is based on sc7180 which is also 10nm. Clk frequency:69300 dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663200000 dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663200000 In same trogdor device, we slightly changed the clock frequency and the values actually differ which will not cause any issue. Clk frequency:69310 dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663440000 dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-11 5:58 ` harigovi @ 2020-02-11 15:44 ` Rob Clark 2020-02-11 15:59 ` Jeffrey Hugo 0 siblings, 1 reply; 11+ messages in thread From: Rob Clark @ 2020-02-11 15:44 UTC (permalink / raw) To: Harigovindan P Cc: Jeffrey Hugo, DTML, MSM, lkml, open list:DRM PANEL DRIVERS, nganji, Sean Paul, Kalyan Thota, Kristian H. Kristensen, freedreno On Mon, Feb 10, 2020 at 9:58 PM <harigovi@codeaurora.org> wrote: > > On 2020-02-07 19:40, Jeffrey Hugo wrote: > > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: > >> > >> On 2020-02-06 20:29, Jeffrey Hugo wrote: > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> > >> > wrote: > >> >> > >> >> For a given byte clock, if VCO recalc value is exactly same as > >> >> vco set rate value, vco_set_rate does not get called assuming > >> >> VCO is already set to required value. But Due to GDSC toggle, > >> >> VCO values are erased in the HW. To make sure VCO is programmed > >> >> correctly, we forcefully call set_rate from vco_prepare. > >> > > >> > Is this specific to certain SoCs? I don't think I've observed this. > >> > >> As far as Qualcomm SOCs are concerned, since pll is analog and the > >> value > >> is directly read from hardware if we get recalc value same as set rate > >> value, the vco_set_rate will not be invoked. We checked in our idp > >> device which has the same SOC but it works there since the rates are > >> different. > > > > This doesn't seem to be an answer to my question. What Qualcomm SoCs > > does this issue apply to? Everything implementing the 10nm pll? One > > specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, > > so I'm interested to know what is the actual impact here. I don't see > > an "IDP" SoC in the IP catalog, so I really have no idea what you are > > referring to. > > > This is not 10nm specific. It is applicable for other nms also. > Its specific to the frequency being set. If vco_recalc returns the same > value as being set by vco_set_rate, > vco_set_rate will not be invoked second time onwards. > > For example: Lets take below devices: > > Cheza is based on SDM845 which is 10nm only. > Clk frequency:206016 > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000 > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947 > > Trogdor is based on sc7180 which is also 10nm. > Clk frequency:69300 > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663200000 > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663200000 > > In same trogdor device, we slightly changed the clock frequency and the > values actually differ which will not cause any issue. > Clk frequency:69310 > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663440000 > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941 tbh, loosing state when power is off is kind of the behavior that I'd expect. It kinda makes me wonder if things are not getting powered off all the way on some SoCs? jhugo, are you worried that this patch will cause problems on other users of the 10nm pll? BR, -R ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-11 15:44 ` Rob Clark @ 2020-02-11 15:59 ` Jeffrey Hugo 2020-02-12 0:28 ` Rob Clark 0 siblings, 1 reply; 11+ messages in thread From: Jeffrey Hugo @ 2020-02-11 15:59 UTC (permalink / raw) To: Rob Clark Cc: Harigovindan P, DTML, MSM, lkml, open list:DRM PANEL DRIVERS, nganji, Sean Paul, Kalyan Thota, Kristian H. Kristensen, freedreno On Tue, Feb 11, 2020 at 8:44 AM Rob Clark <robdclark@gmail.com> wrote: > > On Mon, Feb 10, 2020 at 9:58 PM <harigovi@codeaurora.org> wrote: > > > > On 2020-02-07 19:40, Jeffrey Hugo wrote: > > > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: > > >> > > >> On 2020-02-06 20:29, Jeffrey Hugo wrote: > > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> > > >> > wrote: > > >> >> > > >> >> For a given byte clock, if VCO recalc value is exactly same as > > >> >> vco set rate value, vco_set_rate does not get called assuming > > >> >> VCO is already set to required value. But Due to GDSC toggle, > > >> >> VCO values are erased in the HW. To make sure VCO is programmed > > >> >> correctly, we forcefully call set_rate from vco_prepare. > > >> > > > >> > Is this specific to certain SoCs? I don't think I've observed this. > > >> > > >> As far as Qualcomm SOCs are concerned, since pll is analog and the > > >> value > > >> is directly read from hardware if we get recalc value same as set rate > > >> value, the vco_set_rate will not be invoked. We checked in our idp > > >> device which has the same SOC but it works there since the rates are > > >> different. > > > > > > This doesn't seem to be an answer to my question. What Qualcomm SoCs > > > does this issue apply to? Everything implementing the 10nm pll? One > > > specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, > > > so I'm interested to know what is the actual impact here. I don't see > > > an "IDP" SoC in the IP catalog, so I really have no idea what you are > > > referring to. > > > > > > This is not 10nm specific. It is applicable for other nms also. > > Its specific to the frequency being set. If vco_recalc returns the same > > value as being set by vco_set_rate, > > vco_set_rate will not be invoked second time onwards. > > > > For example: Lets take below devices: > > > > Cheza is based on SDM845 which is 10nm only. > > Clk frequency:206016 > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000 > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947 > > > > Trogdor is based on sc7180 which is also 10nm. > > Clk frequency:69300 > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663200000 > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663200000 > > > > In same trogdor device, we slightly changed the clock frequency and the > > values actually differ which will not cause any issue. > > Clk frequency:69310 > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663440000 > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941 > > > tbh, loosing state when power is off is kind of the behavior that I'd > expect. It kinda makes me wonder if things are not getting powered > off all the way on some SoCs? > > jhugo, are you worried that this patch will cause problems on other > users of the 10nm pll? Essentially yes. Conceptually it doesn't seem like this change should cause any harm, however - This sounds like we are trying to work around the clk framework, which seems wrong. It feels like we should be able to set a clk flag for this and make the framework deal with it. Also, this fix is 10nm specific, yet this issue affects all implementations? Seems like this should perhaps be in common code so that we don't need to play whack-a-mole by fixing every implementation piecemeal. Finally, the PLLs are notorious for not taking a configuration unless they are running. I admit, I haven't looked at this patch in detail to determine if that is the case here, but there doesn't seem to be any indication from the commit test or a comment that doing so is actually valid in all cases. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-11 15:59 ` Jeffrey Hugo @ 2020-02-12 0:28 ` Rob Clark 2020-02-12 4:05 ` Jeffrey Hugo 0 siblings, 1 reply; 11+ messages in thread From: Rob Clark @ 2020-02-12 0:28 UTC (permalink / raw) To: Jeffrey Hugo Cc: DTML, MSM, Harigovindan P, open list:DRM PANEL DRIVERS, lkml, nganji, Sean Paul, Kalyan Thota, Kristian H. Kristensen, freedreno On Tue, Feb 11, 2020 at 7:59 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > On Tue, Feb 11, 2020 at 8:44 AM Rob Clark <robdclark@gmail.com> wrote: > > > > On Mon, Feb 10, 2020 at 9:58 PM <harigovi@codeaurora.org> wrote: > > > > > > On 2020-02-07 19:40, Jeffrey Hugo wrote: > > > > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: > > > >> > > > >> On 2020-02-06 20:29, Jeffrey Hugo wrote: > > > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> > > > >> > wrote: > > > >> >> > > > >> >> For a given byte clock, if VCO recalc value is exactly same as > > > >> >> vco set rate value, vco_set_rate does not get called assuming > > > >> >> VCO is already set to required value. But Due to GDSC toggle, > > > >> >> VCO values are erased in the HW. To make sure VCO is programmed > > > >> >> correctly, we forcefully call set_rate from vco_prepare. > > > >> > > > > >> > Is this specific to certain SoCs? I don't think I've observed this. > > > >> > > > >> As far as Qualcomm SOCs are concerned, since pll is analog and the > > > >> value > > > >> is directly read from hardware if we get recalc value same as set rate > > > >> value, the vco_set_rate will not be invoked. We checked in our idp > > > >> device which has the same SOC but it works there since the rates are > > > >> different. > > > > > > > > This doesn't seem to be an answer to my question. What Qualcomm SoCs > > > > does this issue apply to? Everything implementing the 10nm pll? One > > > > specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, > > > > so I'm interested to know what is the actual impact here. I don't see > > > > an "IDP" SoC in the IP catalog, so I really have no idea what you are > > > > referring to. > > > > > > > > > This is not 10nm specific. It is applicable for other nms also. > > > Its specific to the frequency being set. If vco_recalc returns the same > > > value as being set by vco_set_rate, > > > vco_set_rate will not be invoked second time onwards. > > > > > > For example: Lets take below devices: > > > > > > Cheza is based on SDM845 which is 10nm only. > > > Clk frequency:206016 > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000 > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947 > > > > > > Trogdor is based on sc7180 which is also 10nm. > > > Clk frequency:69300 > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663200000 > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663200000 > > > > > > In same trogdor device, we slightly changed the clock frequency and the > > > values actually differ which will not cause any issue. > > > Clk frequency:69310 > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663440000 > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941 > > > > > > tbh, loosing state when power is off is kind of the behavior that I'd > > expect. It kinda makes me wonder if things are not getting powered > > off all the way on some SoCs? > > > > jhugo, are you worried that this patch will cause problems on other > > users of the 10nm pll? > > Essentially yes. Conceptually it doesn't seem like this change should > cause any harm, however - > > This sounds like we are trying to work around the clk framework, which > seems wrong. It feels like we should be able to set a clk flag for > this and make the framework deal with it. > > Also, this fix is 10nm specific, yet this issue affects all > implementations? Seems like this should perhaps be in common code so > that we don't need to play whack-a-mole by fixing every implementation > piecemeal. > > Finally, the PLLs are notorious for not taking a configuration unless > they are running. I admit, I haven't looked at this patch in detail > to determine if that is the case here, but there doesn't seem to be > any indication from the commit test or a comment that doing so is > actually valid in all cases. I'm not obviously seeing a clk-provider flag for this.. although I won't claim to be a clk expert so maybe I'm looking for the wrong thing.. On a more practical level, I'd kinda like to get some sort of fix for v5.6, as currently suspend/resume doesn't work (or at least the display does not survive) on trogdor, which is a bit annoying. It sounds a bit like cheza was just getting lucky (because of rate rounding?) I'm not sure if it is the same situation on other sdm850 devices (yoga c630) or sdm835 devices (are they using the 10mm pll as well?). I will confess to not really testing s/r on the yoga c630, although maybe someone else has (Bjorn?). Possibly this should be pushed up to the clk framework, although not sure if it has a good way to realize the clk provider has lost power? But that sounds like a better thing for v5.7 than v5.6-rc fixes.. ofc if there is a better way that I'm not seeing, I'm all ears. BR, -R ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-12 0:28 ` Rob Clark @ 2020-02-12 4:05 ` Jeffrey Hugo 2020-02-12 5:10 ` Rob Clark 0 siblings, 1 reply; 11+ messages in thread From: Jeffrey Hugo @ 2020-02-12 4:05 UTC (permalink / raw) To: Rob Clark Cc: DTML, MSM, Harigovindan P, open list:DRM PANEL DRIVERS, lkml, nganji, Sean Paul, Kalyan Thota, Kristian H. Kristensen, freedreno On Tue, Feb 11, 2020 at 5:28 PM Rob Clark <robdclark@gmail.com> wrote: > > On Tue, Feb 11, 2020 at 7:59 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > > > On Tue, Feb 11, 2020 at 8:44 AM Rob Clark <robdclark@gmail.com> wrote: > > > > > > On Mon, Feb 10, 2020 at 9:58 PM <harigovi@codeaurora.org> wrote: > > > > > > > > On 2020-02-07 19:40, Jeffrey Hugo wrote: > > > > > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: > > > > >> > > > > >> On 2020-02-06 20:29, Jeffrey Hugo wrote: > > > > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> > > > > >> > wrote: > > > > >> >> > > > > >> >> For a given byte clock, if VCO recalc value is exactly same as > > > > >> >> vco set rate value, vco_set_rate does not get called assuming > > > > >> >> VCO is already set to required value. But Due to GDSC toggle, > > > > >> >> VCO values are erased in the HW. To make sure VCO is programmed > > > > >> >> correctly, we forcefully call set_rate from vco_prepare. > > > > >> > > > > > >> > Is this specific to certain SoCs? I don't think I've observed this. > > > > >> > > > > >> As far as Qualcomm SOCs are concerned, since pll is analog and the > > > > >> value > > > > >> is directly read from hardware if we get recalc value same as set rate > > > > >> value, the vco_set_rate will not be invoked. We checked in our idp > > > > >> device which has the same SOC but it works there since the rates are > > > > >> different. > > > > > > > > > > This doesn't seem to be an answer to my question. What Qualcomm SoCs > > > > > does this issue apply to? Everything implementing the 10nm pll? One > > > > > specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, > > > > > so I'm interested to know what is the actual impact here. I don't see > > > > > an "IDP" SoC in the IP catalog, so I really have no idea what you are > > > > > referring to. > > > > > > > > > > > > This is not 10nm specific. It is applicable for other nms also. > > > > Its specific to the frequency being set. If vco_recalc returns the same > > > > value as being set by vco_set_rate, > > > > vco_set_rate will not be invoked second time onwards. > > > > > > > > For example: Lets take below devices: > > > > > > > > Cheza is based on SDM845 which is 10nm only. > > > > Clk frequency:206016 > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000 > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947 > > > > > > > > Trogdor is based on sc7180 which is also 10nm. > > > > Clk frequency:69300 > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663200000 > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663200000 > > > > > > > > In same trogdor device, we slightly changed the clock frequency and the > > > > values actually differ which will not cause any issue. > > > > Clk frequency:69310 > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663440000 > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941 > > > > > > > > > tbh, loosing state when power is off is kind of the behavior that I'd > > > expect. It kinda makes me wonder if things are not getting powered > > > off all the way on some SoCs? > > > > > > jhugo, are you worried that this patch will cause problems on other > > > users of the 10nm pll? > > > > Essentially yes. Conceptually it doesn't seem like this change should > > cause any harm, however - > > > > This sounds like we are trying to work around the clk framework, which > > seems wrong. It feels like we should be able to set a clk flag for > > this and make the framework deal with it. > > > > Also, this fix is 10nm specific, yet this issue affects all > > implementations? Seems like this should perhaps be in common code so > > that we don't need to play whack-a-mole by fixing every implementation > > piecemeal. > > > > Finally, the PLLs are notorious for not taking a configuration unless > > they are running. I admit, I haven't looked at this patch in detail > > to determine if that is the case here, but there doesn't seem to be > > any indication from the commit test or a comment that doing so is > > actually valid in all cases. > > I'm not obviously seeing a clk-provider flag for this.. although I > won't claim to be a clk expert so maybe I'm looking for the wrong > thing.. > > On a more practical level, I'd kinda like to get some sort of fix for > v5.6, as currently suspend/resume doesn't work (or at least the > display does not survive) on trogdor, which is a bit annoying. It > sounds a bit like cheza was just getting lucky (because of rate > rounding?) I'm not sure if it is the same situation on other sdm850 > devices (yoga c630) or sdm835 devices (are they using the 10mm pll as > well?). sdm835 is the first implementation of the 10nm PLL. Pretty much everything after (including sdm845/850) also uses the 10nm PLL. > I will confess to not really testing s/r on the yoga c630, > although maybe someone else has (Bjorn?). > > Possibly this should be pushed up to the clk framework, although not > sure if it has a good way to realize the clk provider has lost power? > But that sounds like a better thing for v5.7 than v5.6-rc fixes.. ofc > if there is a better way that I'm not seeing, I'm all ears. There is a suspend/resume sequence in the HPG where VCO isn't lost, but that assumes the GDSC isn't turned off. If GDSC is turned off, then we need to go through the entire power-up sequence again. Feels like this should be plumbed into runtime PM based on the suspend/resume usecase, but that's probably more complicated then this change. Looking at the HPG for the power up sequence, it seems like we should be setting the bias in the middle of the dsi_pll_commit(), so the order of operations is slight off, however I somewhat doubt that will have a meaningful impact and it does seem like this change is in line with the spirit of the HPG. It wasn't clear to me from the commit message what usecase triggered this. You've made it clear that its suspend/resume (it would be good if that was mentioned) and that its impacting an actual target. To me, the current description seemed more theoretical and didn't describe the impact that was being addressed. Overall, it really didn't answer the "why should I care if I have this change" question. Right now, I think my concerns are cosmetic, therefore I don't have reservations about it being picked up. If you like: Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-12 4:05 ` Jeffrey Hugo @ 2020-02-12 5:10 ` Rob Clark 2020-02-17 10:42 ` harigovi 0 siblings, 1 reply; 11+ messages in thread From: Rob Clark @ 2020-02-12 5:10 UTC (permalink / raw) To: Jeffrey Hugo Cc: DTML, MSM, Harigovindan P, open list:DRM PANEL DRIVERS, lkml, nganji, Sean Paul, Kalyan Thota, Kristian H. Kristensen, freedreno On Tue, Feb 11, 2020 at 8:05 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > On Tue, Feb 11, 2020 at 5:28 PM Rob Clark <robdclark@gmail.com> wrote: > > > > On Tue, Feb 11, 2020 at 7:59 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > > > > > On Tue, Feb 11, 2020 at 8:44 AM Rob Clark <robdclark@gmail.com> wrote: > > > > > > > > On Mon, Feb 10, 2020 at 9:58 PM <harigovi@codeaurora.org> wrote: > > > > > > > > > > On 2020-02-07 19:40, Jeffrey Hugo wrote: > > > > > > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: > > > > > >> > > > > > >> On 2020-02-06 20:29, Jeffrey Hugo wrote: > > > > > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> > > > > > >> > wrote: > > > > > >> >> > > > > > >> >> For a given byte clock, if VCO recalc value is exactly same as > > > > > >> >> vco set rate value, vco_set_rate does not get called assuming > > > > > >> >> VCO is already set to required value. But Due to GDSC toggle, > > > > > >> >> VCO values are erased in the HW. To make sure VCO is programmed > > > > > >> >> correctly, we forcefully call set_rate from vco_prepare. > > > > > >> > > > > > > >> > Is this specific to certain SoCs? I don't think I've observed this. > > > > > >> > > > > > >> As far as Qualcomm SOCs are concerned, since pll is analog and the > > > > > >> value > > > > > >> is directly read from hardware if we get recalc value same as set rate > > > > > >> value, the vco_set_rate will not be invoked. We checked in our idp > > > > > >> device which has the same SOC but it works there since the rates are > > > > > >> different. > > > > > > > > > > > > This doesn't seem to be an answer to my question. What Qualcomm SoCs > > > > > > does this issue apply to? Everything implementing the 10nm pll? One > > > > > > specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, > > > > > > so I'm interested to know what is the actual impact here. I don't see > > > > > > an "IDP" SoC in the IP catalog, so I really have no idea what you are > > > > > > referring to. > > > > > > > > > > > > > > > This is not 10nm specific. It is applicable for other nms also. > > > > > Its specific to the frequency being set. If vco_recalc returns the same > > > > > value as being set by vco_set_rate, > > > > > vco_set_rate will not be invoked second time onwards. > > > > > > > > > > For example: Lets take below devices: > > > > > > > > > > Cheza is based on SDM845 which is 10nm only. > > > > > Clk frequency:206016 > > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000 > > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947 > > > > > > > > > > Trogdor is based on sc7180 which is also 10nm. > > > > > Clk frequency:69300 > > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663200000 > > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663200000 > > > > > > > > > > In same trogdor device, we slightly changed the clock frequency and the > > > > > values actually differ which will not cause any issue. > > > > > Clk frequency:69310 > > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663440000 > > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941 > > > > > > > > > > > > tbh, loosing state when power is off is kind of the behavior that I'd > > > > expect. It kinda makes me wonder if things are not getting powered > > > > off all the way on some SoCs? > > > > > > > > jhugo, are you worried that this patch will cause problems on other > > > > users of the 10nm pll? > > > > > > Essentially yes. Conceptually it doesn't seem like this change should > > > cause any harm, however - > > > > > > This sounds like we are trying to work around the clk framework, which > > > seems wrong. It feels like we should be able to set a clk flag for > > > this and make the framework deal with it. > > > > > > Also, this fix is 10nm specific, yet this issue affects all > > > implementations? Seems like this should perhaps be in common code so > > > that we don't need to play whack-a-mole by fixing every implementation > > > piecemeal. > > > > > > Finally, the PLLs are notorious for not taking a configuration unless > > > they are running. I admit, I haven't looked at this patch in detail > > > to determine if that is the case here, but there doesn't seem to be > > > any indication from the commit test or a comment that doing so is > > > actually valid in all cases. > > > > I'm not obviously seeing a clk-provider flag for this.. although I > > won't claim to be a clk expert so maybe I'm looking for the wrong > > thing.. > > > > On a more practical level, I'd kinda like to get some sort of fix for > > v5.6, as currently suspend/resume doesn't work (or at least the > > display does not survive) on trogdor, which is a bit annoying. It > > sounds a bit like cheza was just getting lucky (because of rate > > rounding?) I'm not sure if it is the same situation on other sdm850 > > devices (yoga c630) or sdm835 devices (are they using the 10mm pll as > > well?). > > sdm835 is the first implementation of the 10nm PLL. Pretty much > everything after (including sdm845/850) also uses the 10nm PLL. > > > I will confess to not really testing s/r on the yoga c630, > > although maybe someone else has (Bjorn?). > > > > Possibly this should be pushed up to the clk framework, although not > > sure if it has a good way to realize the clk provider has lost power? > > But that sounds like a better thing for v5.7 than v5.6-rc fixes.. ofc > > if there is a better way that I'm not seeing, I'm all ears. > > There is a suspend/resume sequence in the HPG where VCO isn't lost, > but that assumes the GDSC isn't turned off. If GDSC is turned off, > then we need to go through the entire power-up sequence again. Feels > like this should be plumbed into runtime PM based on the > suspend/resume usecase, but that's probably more complicated then this > change. since gdsc is modelled as genpd, that seems to (afaict) happen all outside the scope of what the driver knows about.. (but I may be overlooking something) > Looking at the HPG for the power up sequence, it seems like we should > be setting the bias in the middle of the dsi_pll_commit(), so the > order of operations is slight off, however I somewhat doubt that will > have a meaningful impact and it does seem like this change is in line > with the spirit of the HPG. > > It wasn't clear to me from the commit message what usecase triggered > this. You've made it clear that its suspend/resume (it would be good > if that was mentioned) and that its impacting an actual target. To > me, the current description seemed more theoretical and didn't > describe the impact that was being addressed. Overall, it really > didn't answer the "why should I care if I have this change" question. > > Right now, I think my concerns are cosmetic, therefore I don't have > reservations about it being picked up. If you like: > > Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> hmm, yeah, I guess the commit msg didn't really make that clear.. at any rate, I want to see a clean solution pursued in the long run, but in the short term I also want to get things working (at least if it doesn't break any other users). So I don't want to land this patch at the expense of follow-up for a cleaner solution.. but like I said, I would like to get s/r working for now. So I guess I'd like to see some commitment from the display team to follow-up to improve this in the next cycle. And suggestions welcome about how the clk framework could make this easier. BR, -R ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Freedreno] [v1] drm/msm/dsi/pll: call vco set rate explicitly 2020-02-12 5:10 ` Rob Clark @ 2020-02-17 10:42 ` harigovi 0 siblings, 0 replies; 11+ messages in thread From: harigovi @ 2020-02-17 10:42 UTC (permalink / raw) To: Rob Clark Cc: Jeffrey Hugo, DTML, MSM, open list:DRM PANEL DRIVERS, lkml, nganji, Sean Paul, Kalyan Thota, Kristian H. Kristensen, freedreno On 2020-02-12 10:40, Rob Clark wrote: > On Tue, Feb 11, 2020 at 8:05 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > wrote: >> >> On Tue, Feb 11, 2020 at 5:28 PM Rob Clark <robdclark@gmail.com> wrote: >> > >> > On Tue, Feb 11, 2020 at 7:59 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: >> > > >> > > On Tue, Feb 11, 2020 at 8:44 AM Rob Clark <robdclark@gmail.com> wrote: >> > > > >> > > > On Mon, Feb 10, 2020 at 9:58 PM <harigovi@codeaurora.org> wrote: >> > > > > >> > > > > On 2020-02-07 19:40, Jeffrey Hugo wrote: >> > > > > > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@codeaurora.org> wrote: >> > > > > >> >> > > > > >> On 2020-02-06 20:29, Jeffrey Hugo wrote: >> > > > > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@codeaurora.org> >> > > > > >> > wrote: >> > > > > >> >> >> > > > > >> >> For a given byte clock, if VCO recalc value is exactly same as >> > > > > >> >> vco set rate value, vco_set_rate does not get called assuming >> > > > > >> >> VCO is already set to required value. But Due to GDSC toggle, >> > > > > >> >> VCO values are erased in the HW. To make sure VCO is programmed >> > > > > >> >> correctly, we forcefully call set_rate from vco_prepare. >> > > > > >> > >> > > > > >> > Is this specific to certain SoCs? I don't think I've observed this. >> > > > > >> >> > > > > >> As far as Qualcomm SOCs are concerned, since pll is analog and the >> > > > > >> value >> > > > > >> is directly read from hardware if we get recalc value same as set rate >> > > > > >> value, the vco_set_rate will not be invoked. We checked in our idp >> > > > > >> device which has the same SOC but it works there since the rates are >> > > > > >> different. >> > > > > > >> > > > > > This doesn't seem to be an answer to my question. What Qualcomm SoCs >> > > > > > does this issue apply to? Everything implementing the 10nm pll? One >> > > > > > specific SoC? I don't believe I've seen this on MSM8998, nor SDM845, >> > > > > > so I'm interested to know what is the actual impact here. I don't see >> > > > > > an "IDP" SoC in the IP catalog, so I really have no idea what you are >> > > > > > referring to. >> > > > > >> > > > > >> > > > > This is not 10nm specific. It is applicable for other nms also. >> > > > > Its specific to the frequency being set. If vco_recalc returns the same >> > > > > value as being set by vco_set_rate, >> > > > > vco_set_rate will not be invoked second time onwards. >> > > > > >> > > > > For example: Lets take below devices: >> > > > > >> > > > > Cheza is based on SDM845 which is 10nm only. >> > > > > Clk frequency:206016 >> > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1236096000 >> > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1236095947 >> > > > > >> > > > > Trogdor is based on sc7180 which is also 10nm. >> > > > > Clk frequency:69300 >> > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663200000 >> > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663200000 >> > > > > >> > > > > In same trogdor device, we slightly changed the clock frequency and the >> > > > > values actually differ which will not cause any issue. >> > > > > Clk frequency:69310 >> > > > > dsi_pll_10nm_vco_set_rate - DSI PLL0 rate=1663440000 >> > > > > dsi_pll_10nm_vco_recalc_rate - DSI PLL0 returning vco rate = 1663439941 >> > > > >> > > > >> > > > tbh, loosing state when power is off is kind of the behavior that I'd >> > > > expect. It kinda makes me wonder if things are not getting powered >> > > > off all the way on some SoCs? >> > > > >> > > > jhugo, are you worried that this patch will cause problems on other >> > > > users of the 10nm pll? >> > > >> > > Essentially yes. Conceptually it doesn't seem like this change should >> > > cause any harm, however - >> > > >> > > This sounds like we are trying to work around the clk framework, which >> > > seems wrong. It feels like we should be able to set a clk flag for >> > > this and make the framework deal with it. >> > > >> > > Also, this fix is 10nm specific, yet this issue affects all >> > > implementations? Seems like this should perhaps be in common code so >> > > that we don't need to play whack-a-mole by fixing every implementation >> > > piecemeal. >> > > >> > > Finally, the PLLs are notorious for not taking a configuration unless >> > > they are running. I admit, I haven't looked at this patch in detail >> > > to determine if that is the case here, but there doesn't seem to be >> > > any indication from the commit test or a comment that doing so is >> > > actually valid in all cases. >> > >> > I'm not obviously seeing a clk-provider flag for this.. although I >> > won't claim to be a clk expert so maybe I'm looking for the wrong >> > thing.. >> > >> > On a more practical level, I'd kinda like to get some sort of fix for >> > v5.6, as currently suspend/resume doesn't work (or at least the >> > display does not survive) on trogdor, which is a bit annoying. It >> > sounds a bit like cheza was just getting lucky (because of rate >> > rounding?) I'm not sure if it is the same situation on other sdm850 >> > devices (yoga c630) or sdm835 devices (are they using the 10mm pll as >> > well?). >> >> sdm835 is the first implementation of the 10nm PLL. Pretty much >> everything after (including sdm845/850) also uses the 10nm PLL. >> >> > I will confess to not really testing s/r on the yoga c630, >> > although maybe someone else has (Bjorn?). >> > >> > Possibly this should be pushed up to the clk framework, although not >> > sure if it has a good way to realize the clk provider has lost power? >> > But that sounds like a better thing for v5.7 than v5.6-rc fixes.. ofc >> > if there is a better way that I'm not seeing, I'm all ears. >> >> There is a suspend/resume sequence in the HPG where VCO isn't lost, >> but that assumes the GDSC isn't turned off. If GDSC is turned off, >> then we need to go through the entire power-up sequence again. Feels >> like this should be plumbed into runtime PM based on the >> suspend/resume usecase, but that's probably more complicated then this >> change. > > since gdsc is modelled as genpd, that seems to (afaict) happen all > outside the scope of what the driver knows about.. (but I may be > overlooking something) > >> Looking at the HPG for the power up sequence, it seems like we should >> be setting the bias in the middle of the dsi_pll_commit(), so the >> order of operations is slight off, however I somewhat doubt that will >> have a meaningful impact and it does seem like this change is in line >> with the spirit of the HPG. >> >> It wasn't clear to me from the commit message what usecase triggered >> this. You've made it clear that its suspend/resume (it would be good >> if that was mentioned) and that its impacting an actual target. To >> me, the current description seemed more theoretical and didn't >> describe the impact that was being addressed. Overall, it really >> didn't answer the "why should I care if I have this change" question. >> >> Right now, I think my concerns are cosmetic, therefore I don't have >> reservations about it being picked up. If you like: >> >> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > > hmm, yeah, I guess the commit msg didn't really make that clear.. at > any rate, I want to see a clean solution pursued in the long run, but > in the short term I also want to get things working (at least if it > doesn't break any other users). So I don't want to land this patch at > the expense of follow-up for a cleaner solution.. but like I said, I > would like to get s/r working for now. So I guess I'd like to see > some commitment from the display team to follow-up to improve this in > the next cycle. And suggestions welcome about how the clk framework > could make this easier. > > BR, > -R we had an initial discussion with the clock team, currently there is no ready made clock provider flag to help in the above scenario. however we are planning to have further discussions for a cleaner fix, can we open a new issue to track the discussions and design and close the current one ? ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-02-17 10:42 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-06 9:12 [v1] drm/msm/dsi/pll: call vco set rate explicitly Harigovindan P 2020-02-06 14:59 ` [Freedreno] " Jeffrey Hugo 2020-02-07 12:38 ` harigovi 2020-02-07 14:10 ` Jeffrey Hugo 2020-02-11 5:58 ` harigovi 2020-02-11 15:44 ` Rob Clark 2020-02-11 15:59 ` Jeffrey Hugo 2020-02-12 0:28 ` Rob Clark 2020-02-12 4:05 ` Jeffrey Hugo 2020-02-12 5:10 ` Rob Clark 2020-02-17 10:42 ` harigovi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).