Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 8 Jun 2021 18:10:14 -0500
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     khsieh@codeaurora.org, robdclark@gmail.com, sean@poorly.run,
        vkoul@kernel.org, agross@kernel.org, robh+dt@kernel.org,
        devicetree@vger.kernel.org, abhinavk@codeaurora.org,
        aravindh@codeaurora.org, freedreno@lists.freedesktop.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node
Message-ID: <YL/41hWz8xB+jSeO@yoga>
References: <YLkI/6ItCz+SbbuJ@yoga>
 <ac326ec8689c0babb08b2311e19d52cc@codeaurora.org>
 <YLxX/YtegtbLmkri@builder.lan>
 <ef1879fa7ecfefaf0c70c7a4782240a9@codeaurora.org>
 <YL6sY/1E5wLzMiP/@yoga>
 <CAE-0n50-X03sMyJdsw7s=Ue0dWXBo=iHOc0HxDQm5yh2J-uS3A@mail.gmail.com>
 <YL/uj+t+BFkII1Fh@yoga>
 <CAE-0n50WP25kRQkWMVdDZGsZWBXwfbVSTFKyBLF7f8Mp3x2Wfg@mail.gmail.com>
 <YL/wWdRs6e/eECiC@yoga>
 <CAE-0n51GM65rZVJgXuHy6FerJorHeHKf2W31GijG8sDEhaX_KQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAE-0n51GM65rZVJgXuHy6FerJorHeHKf2W31GijG8sDEhaX_KQ@mail.gmail.com>
Precedence: bulk

On Tue 08 Jun 17:44 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-08 15:34:01)
> > On Tue 08 Jun 17:29 CDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-06-08 15:26:23)
> > > > On Tue 08 Jun 17:15 CDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-06-07 16:31:47)
> > > > > > On Mon 07 Jun 12:48 CDT 2021, khsieh@codeaurora.org wrote:
> > > > > >
> > > > > > > Sorry about the confusion. What I meant is that even though DP controller is
> > > > > > > in the MDSS_GDSC
> > > > > > > power domain, DP PHY/PLL sources out of CX. The DP link clocks have a direct
> > > > > > > impact
> > > > > > > on the CX voltage corners. Therefore, we need to mention the CX power domain
> > > > > > > here. And, since
> > > > > > > we can associate only one OPP table with one device, we picked the DP link
> > > > > > > clock over other
> > > > > > > clocks.
> > > > > >
> > > > > > Thank you, that's a much more useful answer.
> > > > > >
> > > > > > Naturally I would think it would make more sense for the PHY/PLL driver
> > > > > > to ensure that CX is appropriately voted for then, but I think that
> > > > > > would result in it being the clock driver performing such vote and I'm
> > > > > > unsure how the opp table for that would look.
> > > > > >
> > > > > > @Stephen, what do you say?
> > > > > >
> > > > >
> > > > > Wouldn't the PHY be the one that sets some vote? So it wouldn't be the
> > > > > clk driver, and probably not from the clk ops, but instead come from the
> > > > > phy ops via phy_enable() and phy_configure().
> > > > >
> > > >
> > > > If I understand the logic correctly *_configure_dp_phy() will both
> > > > configure the vco clock and "request" the clock framework to change the
> > > > rate.
> > > >
> > > > So I presume what you're suggesting is that that would be the place to
> > > > cast the CX corner vote?
> > >
> > > Yes that would be a place to make the CX vote. The problem is then I
> > > don't know where to drop the vote. Is that when the phy is disabled?
> >
> > We do pass qcom_qmp_phy_power_off() and power down the DP part as DP
> > output is being disabled. So that sounds like a reasonable place to drop
> > the vote for the lowest performance state.
> >
> 
> So then will the corner vote be in place when the PHY isn't actually
> powered up? That will be bad for power. The phy configure code will need
> to know if the phy is enabled and then only put in the vote when the phy
> is enabled, otherwise wait for enable to make the corner vote.
> 

If we vote for a corner based on the link rate in *_configure_dp_phy()
and put the vote for lowest corner we'd get the corner part sorted out
afaict.

We'd still have to make sure that the PHY doesn't hang on to the cx vote
beyond that though - and implicitly in the non-DP cases...

> Honestly I suspect the DP PHY is _not_ in the CX domain as CX is for
> digital logic. Probably the PLL is the hardware that has some minimum CX
> requirement, and that flows down into the various display clks like the
> link clk that actually clock the DP controller hardware. The mdss_gdsc
> probably gates CX for the display subsystem (mdss) so if we had proper
> corner aggregation logic we could indicate that mdss_gdsc is a child of
> the CX domain and then make requests from the DP driver for particular
> link frequencies on the mdss_gdsc and then have that bubble up to CX
> appropriately. I don't think any of that sort of code is in place
> though, right?

I haven't checked sc7180, but I'm guessing that it's following the other
modern platforms, where all the MDSS related pieces (including e.g.
dispcc) lives in the MMCX domain, which is separate from CX.

So the parent of MDSS_GDSC should be MMCX, while Kuogee's answer (and
the dp-opp-table) tells us that the PLL lives in the CX domain.


PS. While this goes for the QMPs the DSI and eDP/DP PHYs (and PLLs)
seems to live in MMCX.

Regards,
Bjorn