Re: [PATCH v1 6/9] phy: qcom-qmp: Add support for DP in USB3+DP combo phy

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-09-03 05:37:02)
> On 02/09/2020 04:01, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2020-09-01 06:36:34)
> >> With these functions I'm struggling between introducing
> >> PHY_TYPE_DP_V3/V4 and introducing callbacks into qmp_phy_cfg. What would
> >> you prefer?
> >>
> >> What about the following struct?
> >>
> >> struct qmp_phy_dp_opts {
> >>          void (*dp_aux_init)(struct qmp_phy *qphy);
> >>          void (*dp_configure_tx)(struct qmp_phy *qphy);
> >>          void (*dp_configure_lanes)(struct qmp_phy *qphy);
> >> };
> >>
> >> I'm not sure about dp_calibrate().
> >>
> > 
> > Is there v4 code somewhere that I can see? Another level of indirection
> > is always a solution, so it is probably fine. This driver is currently
> > written with many conditionals instead of function tables so I'm not
> > sure it fits in with the style of how things are done though. The
> > alternative is to use an enum and call different functions?
> 
> Downstream DP driver sources can be found here:
> https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/msm/dp/dp_catalog_v420.c?h=LA.UM.8.12.r1-13900-sm8250.0
> 
> https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/pll/dp_pll_7nm_util.c?h=LA.UM.8.12.r1-13900-sm8250.0
> 

Awesome thanks for the pointer.

> > 
> > The calibrate call is there to "turn the crank" on the aux settings.  I
> > need to cycle through the different values for that aux register so that
> > aux can be tuned properly. The AUX channel really has another phy that
> > needs tuning so we're sort of combining the aux and DP link phy together
> > here by letting the calibrate call tune the AUX phy and the configure
> > call tune the DP phy. I don't see any sort of concept of an AUX phy
> > though so this seemed ok. Does v4 need to tune more registers?
> 
> 
> It looks like four values are written to AUX_CFG1:
> 0x20, 0x13, 0x23, 0x1d
> 

Ok, so still just AUX_CFG1 but now some different values. Maybe it
should come from DT if it really differs based on board design. I don't
know if it does though. If it does differ it would be nice to know what
the settings are and if it doesn't just make sense to iterate through
all 256 of them instead of targeting specific ones.