On 03/09/2020 23:43, Jonathan Marek wrote:
> On 9/2/20 7:02 PM, Stephen Boyd wrote:
>> Add support for the USB3 + DisplayPort (DP) "combo" phy to the qmp phy
>> driver. We already have support for the USB3 part of the combo phy, so
>> most additions are for the DP phy.
>>
>> Split up the qcom_qmp_phy{enable,disable}() functions into the phy init,
>> power on, power off, and exit functions that the common phy framework
>> expects so that the DP phy can add even more phy ops like
>> phy_calibrate() and phy_configure(). This allows us to initialize the DP
>> PHY and configure the AUX channel before powering on the PHY at the link
>> rate that was negotiated during link training.
>>
>> The general design is as follows:
>>
>>    1) DP controller calls phy_init() to initialize the PHY and configure
>>    the dp_com register region.
>>
>>    2) DP controller calls phy_configure() to tune the link rate and
>>    voltage swing and pre-emphasis settings.
>>
>>    3) DP controller calls phy_power_on() to enable the PLL and power on
>>    the phy.
>>
>>    4) DP controller calls phy_configure() again to tune the voltage swing
>>    and pre-emphasis settings determind during link training.
>>
>>    5) DP controller calls phy_calibrate() some number of times to change
>>    the aux settings if the aux channel times out during link training.
>>
>>    6) DP controller calls phy_power_off() if the link rate is to be
>>    changed and goes back to step 2 to try again at a different link rate.
>>
>>    5) DP controller calls phy_power_off() and then phy_exit() to power
>>    down the PHY when it is done.
>>
>> The DP PHY contains a PLL that is different from the one used for the
>> USB3 PHY. Instead of a pipe clk there is a link clk and a pixel clk
>> output from the DP PLL after going through various dividers. Introduce
>> clk ops for these two clks that just tell the child clks what the
>> frequency of the pixel and link are. When the phy link rate is
>> configured we call clk_set_rate() to update the child clks in the
>> display clk controller on what rate is in use. The clk frequencies
>> always differ based on the link rate (i.e. 1.6Gb/s 2.7Gb/s, 5.4Gb/s, or
>> 8.1Gb/s corresponding to various transmission modes like HBR1, HBR2 or
>> HBR3) so we simply store the link rate and use that to calculate the clk
>> frequencies.
>>
>> The PLL enable sequence is a little different from other QMP phy PLLs so
>> we power on the PLL in qcom_qmp_phy_configure_dp_phy() that gets called
>> from phy_power_on(). This should probably be split out better so that
>> each phy has a way to run the final PLL/PHY enable sequence.
>>
>> This code is based on a submission of this phy and PLL in the drm
>> subsystem.
> 
> I updated my upstream-based sm8150/sm8250 displayport stack [1] to use 
> these patches.

I have tried your branch on my RB5 with two different dongles. Both 
dongles provide the same behaviour:
  - on first plug I see VDM Tx errors,
  - after I unplug and replug the dongle, PD phy seems to be stuck on 
sending capabilities.

See attached logs.

Also I had to add typec_unregister_port(port->typec_port); to 
IS_ERR(alt) in your tcpm.c hack.

I'm currently finishing the driver for the mux/redriver, will retry 
testing afterwards.

> This commit [2] might interest you, so that you can consider what needs 
> to change between v3 and v4 PHYs. Note some of the V4 registers have the 
> same address as V3, so the diff could be smaller.
> 
> Do you have any plan for dealing with the SS PHY and DP PHY conflicting 
> with each other? For example, PHY_MODE_CTRL needs to be "DP_MODE" for 
> 4-lane DP, "DP_MODE | USB3_MODE" for 2-lane DP + USB3, and (AFAIK) 
> "USB3_MODE" for superspeedplus usb (and it seems this gates some clocks, 
> so you can't read/write dp tx2 registers in 2-lane DP mode for example). 
> From your cover letter it sounds like this isn't relevant to your 
> hardware, but it looks like both PHYs are writing to the dp_com region 
> which is still problematic. (in the branch I linked, I disabled the SS 
> PHY to test the DP PHY)
> 
> Also some issues I noticed:
> - used QSERDES_COM_RESETSM_CNTRL instead of 
> QSERDES_V3_COM_RESETSM_CNTRL2, which has different value
> - in sc7180_dpphy_cfg, .regs is NULL, which results in NULL references
> 
> [1] https://github.com/flto/linux/commits/sm8x50-hdk-display
> [2] 
> https://github.com/flto/linux/commit/ccf56912bd5e652b4daebec1300961a9d51342b1 
> 


-- 
With best wishes
Dmitry