On Tue, Feb 11, 2020 at 8:05 PM Jeffrey Hugo <jeffrey.l.hugo@xxxxxxxxx> wrote: > > On Tue, Feb 11, 2020 at 5:28 PM Rob Clark <robdclark@xxxxxxxxx> wrote: > > > > On Tue, Feb 11, 2020 at 7:59 AM Jeffrey Hugo <jeffrey.l.hugo@xxxxxxxxx> wrote: > > > > > > On Tue, Feb 11, 2020 at 8:44 AM Rob Clark <robdclark@xxxxxxxxx> wrote: > > > > > > > > On Mon, Feb 10, 2020 at 9:58 PM <harigovi@xxxxxxxxxxxxxx> wrote: > > > > > > > > > > On 2020-02-07 19:40, Jeffrey Hugo wrote: > > > > > > On Fri, Feb 7, 2020 at 5:38 AM <harigovi@xxxxxxxxxxxxxx> wrote: > > > > > >> > > > > > >> On 2020-02-06 20:29, Jeffrey Hugo wrote: > > > > > >> > On Thu, Feb 6, 2020 at 2:13 AM Harigovindan P <harigovi@xxxxxxxxxxxxxx> > > > > > >> > 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@xxxxxxxxx> 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
