Re: [PATCH RFC/WIP 1/4] arm64: dts: qcom: sm8750: Add display (MDSS) with Display CC

[Abhinav Kumar]

On 5/3/2025 2:01 PM, Dmitry Baryshkov wrote:
> On Sat, 3 May 2025 at 22:59, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > On 5/2/2025 10:51 PM, Dmitry Baryshkov wrote:
> > > On Tue, Apr 29, 2025 at 04:07:24PM -0700, Abhinav Kumar wrote:
> > > > On 4/28/2025 2:31 PM, Konrad Dybcio wrote:
> > > > > On 4/24/25 3:04 PM, Krzysztof Kozlowski wrote:
> > > > > > Add device nodes for entire display: MDSS, DPU, DSI, DSI PHYs,
> > > > > > DisplayPort and Display Clock Controller.
> > > > > >
> > > > > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > > > > >
> > > > > > ---
> > > > >
> > > > > [...]
> > > > >
> > > > > > +                          mdp_opp_table: opp-table {
> > > > > > +                                  compatible = "operating-points-v2";
> > > > > > +
> > > > >
> > > > > The computer tells me there's also a 156 MHz rate @ SVS_D1
> > > > >
> > > > > Maybe Abhinav could chime in whether we should add it or not
> > > >
> > > > Yes I also see a 156Mhz for LOW_SVS_D1 but we had a similar entry even for
> > > > sm8650 and did not publish it in the dt.
> > > >
> > > > It was present till sm8450.dtsi but dropped in sm8550/sm8650 even though
> > > > LOW_SVS_D1 is present even on those.
> > > >
> > > > I think the reason could be that the displays being used on the reference
> > > > boards will need a pixel clock of atleast >= low_svs and the MDP clock
> > > > usually depends on the value of the DSI pixel clock (which has a fixed
> > > > relationship to the byte clock) to maintain the data rate. So as a result
> > > > perhaps even if we add it, for most displays this level will be unused.
> > > >
> > > > If we end up using displays which are so small that the pixel clock
> > > > requirement will be even lower than low_svs, we can add those.
> > > >
> > > > OR as an alternative, we can leave this patch as it is and add the
> > > > low_svs_d1 for all chipsets which support it together in another series that
> > > > way it will have the full context of why we are adding it otherwise it will
> > > > look odd again of why sm8550/sm8650 was left out but added in sm8750.
> > >
> > > I think it's better to describe hardware accurately, even if the
> > > particular entry ends up being unused. I'd vote for this option.
> > >
> > > > > [...]
> > > > >
> > > > > > +                          mdss_dsi_opp_table: opp-table {
> > > > > > +                                  compatible = "operating-points-v2";
> > > > > > +
> > > > >
> > > > > Similarly there's a 140.63 MHz rate at SVS_D1, but it seems odd
> > > > > with the decimals
> > > >
> > > > For this one, yes its true that LOW_SVS_D1 is 140.63Mhz for sm8750 but this
> > > > voltage corner was somehow never used for DSI byte clock again I am thinking
> > > > this is because for the display resolutions we use, we will always be >=
> > > > low_svs so the low_svs_d1 will never hit even if we add it.
> > >
> > > Please add all voltage/frequency corners. Think about low-res DP or
> > > low-res, low-rate WB.
> >
> > Sounds good, lets go ahead and add all the voltage/freq corners.
> >
> > Like I noted, even for sm8550/sm8650 the low_svs_d1 was missed out, so
> > if we are adding it for sm8750 now in this series, a follow up patch
> > should also be sent to add them for sm8550/sm8650 as well. That way we
> > will fix them all up together and this does not come across as a
> > discrepancy.
>
> Abhinav, if you know a missing piece, please send a patch, fixing it.

Sure, I will send something next week to fix up the sm8550/sm8650 dtsi 
to add the missing opp tables unless someone beats me to it.