Re: [PATCH v2] arm64/dts/qcom/sc7180: Add Display Port dt node

From: Stephen Boyd <swboyd@chromium.org>
To: Bjorn Andersson <bjorn.andersson@linaro.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
Date: Tue, 8 Jun 2021 15:44:18 -0700	[thread overview]
Message-ID: <CAE-0n51GM65rZVJgXuHy6FerJorHeHKf2W31GijG8sDEhaX_KQ@mail.gmail.com> (raw)
In-Reply-To: <YL/wWdRs6e/eECiC@yoga>

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.

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?