On 12/20/2024 5:45 AM, Dmitry Baryshkov wrote:
> On Thu, Dec 19, 2024 at 06:36:38PM +0800, Xiangxu Yin wrote:
>>
>>
>> On 12/5/2024 7:40 PM, Dmitry Baryshkov wrote:
>>> On Thu, 5 Dec 2024 at 13:28, Xiangxu Yin <quic_xiang...@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/2/2024 6:46 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, Dec 02, 2024 at 04:40:05PM +0800, Xiangxu Yin wrote:
>>>>>>
>>>>>>
>>>>>> On 11/29/2024 9:50 PM, Dmitry Baryshkov wrote:
>>>>>>> On Fri, 29 Nov 2024 at 09:59, Xiangxu Yin <quic_xiang...@quicinc.com> 
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Add the ability to configure lane mapping for the DP controller. This 
>>>>>>>> is
>>>>>>>> required when the platform's lane mapping does not follow the default
>>>>>>>> order (0, 1, 2, 3). The mapping rules are now configurable via the
>>>>>>>> `data-lane` property in the devicetree. This property defines the
>>>>>>>> logical-to-physical lane mapping sequence, ensuring correct lane
>>>>>>>> assignment for non-default configurations.
>>>>>>>>
>>>>>>>> Signed-off-by: Xiangxu Yin <quic_xiang...@quicinc.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/msm/dp/dp_catalog.c | 11 +++++------
>>>>>>>>  drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
>>>>>>>>  drivers/gpu/drm/msm/dp/dp_ctrl.c    |  2 +-
>>>>>>>>  drivers/gpu/drm/msm/dp/dp_panel.c   | 13 ++++++++++---
>>>>>>>>  drivers/gpu/drm/msm/dp/dp_panel.h   |  3 +++
>>>>>>>>  5 files changed, 20 insertions(+), 11 deletions(-)
>>>>>>>>
>>>>>
>>>>>>>> @@ -461,6 +460,7 @@ static int msm_dp_panel_parse_dt(struct 
>>>>>>>> msm_dp_panel *msm_dp_panel)
>>>>>>>>         struct msm_dp_panel_private *panel;
>>>>>>>>         struct device_node *of_node;
>>>>>>>>         int cnt;
>>>>>>>> +       u32 lane_map[DP_MAX_NUM_DP_LANES] = {0, 1, 2, 3};
>>>>>>>>
>>>>>>>>         panel = container_of(msm_dp_panel, struct 
>>>>>>>> msm_dp_panel_private, msm_dp_panel);
>>>>>>>>         of_node = panel->dev->of_node;
>>>>>>>> @@ -474,10 +474,17 @@ static int msm_dp_panel_parse_dt(struct 
>>>>>>>> msm_dp_panel *msm_dp_panel)
>>>>>>>>                 cnt = drm_of_get_data_lanes_count(of_node, 1, 
>>>>>>>> DP_MAX_NUM_DP_LANES);
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -       if (cnt > 0)
>>>>>>>> +       if (cnt > 0) {
>>>>>>>> +               struct device_node *endpoint;
>>>>>>>> +
>>>>>>>>                 msm_dp_panel->max_dp_lanes = cnt;
>>>>>>>> -       else
>>>>>>>> +               endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 
>>>>>>>> -1);
>>>>>>>> +               of_property_read_u32_array(endpoint, "data-lanes", 
>>>>>>>> lane_map, cnt);
>>>>>>>> +       } else {
>>>>>>>>                 msm_dp_panel->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 
>>>>>>>> lanes */
>>>>>>>> +       }
>>>>>>>
>>>>>>> Why? This sounds more like dp_catalog or (after the refactoring at
>>>>>>> [1]) dp_ctrl. But not the dp_panel.
>>>>>>>
>>>>>>> [1] 
>>>>>>> https://patchwork.freedesktop.org/project/freedreno/series/?ordering=-last_updated
>>>>>>>
>>>>>> We are used the same prop 'data-lanes = <3 2 0 1>' in mdss_dp_out to 
>>>>>> keep similar behaviour with dsi_host_parse_lane_data.
>>>>>> From the modules used, catalog seems more appropriate, but since the 
>>>>>> max_dp_lanes is parsed at dp_panel, it has been placed here.
>>>>>> Should lane_map parsing in msm_dp_catalog_get, and keep max_dp_lanes 
>>>>>> parsing at the dp_panel?
>>>>>
>>>>> msm_dp_catalog_get() is going to be removed. Since the functions that
>>>>> are going to use it are in dp_ctrl module, I thought that dp_ctrl.c is
>>>>> the best place. A better option might be to move max_dp_lanes and
>>>>> max_dp_link_rate to dp_link.c as those are link params. Then
>>>>> lane_mapping also logically becomes a part of dp_link module.
>>>>>
>>>>> But now I have a more important question (triggered by Krishna's email
>>>>> about SAR2130P's USB): if the lanes are swapped, does USB 3 work on that
>>>>> platform? Or is it being demoted to USB 2 with nobody noticing that?
>>>>>
>>>>> If lanes 0/1 and 2/3 are swapped, shouldn't it be handled in the QMP
>>>>> PHY, where we handle lanes and orientation switching?
>>>>>
>>>> I have checked the DP hardware programming guide and also discussed it 
>>>> with Krishna.
>>>>
>>>> According to the HPG section '3.4.2 PN and Lane Swap: PHY supports PN swap 
>>>> for mainlink and AUX, but it doesn't support lane swap feature.'
>>>>
>>>> The lane swap mainly refers to the logical to physical mapping between the 
>>>> DP controller and the DP PHY. The PHY handles polarity inversion, and the 
>>>> lane map does not affect USB behavior.
>>>>
>>>> On the QCS615 platform, we have also tested when DP works with lane swap, 
>>>> other USB 3.0 ports can works normally at super speed.
>>>
>>> "Other USB 3.0 ports"? What does that mean? Please correct me if I'm
>>> wrong, you should have a USB+DP combo port that is being managed with
>>> combo PHY. Does USB 3 work on that port?
>>>
>>> In other words, where the order of lanes is actually inverted? Between
>>> DP and combo PHY? Within combo PHY? Between the PHY and the pinout?
>>> Granted that SM6150 was supported in msm-4.14 could you possibly point
>>> out a corresponding commit or a set of commits from that kernel?
>>>
>> For "Other USB 3.0 ports", as replied in USBC driver, USB3 primary phy works 
>> for other four USB type-A port.
> 
> So if that's the USB3 primary, then why do you mention here at all? We
> are taling about the secondary USB3 + DP.
> 
OK, sorry for confusing you.
>> The REG_DP_LOGICAL2PHYSICAL_LANE_MAPPING mapping determines how logical 
>> lanes (0, 1, 2, 3) map to physical lanes sent to the PHY.
>> This ensures alignment with hardware requirements.
>> The PHY’s polarity inversion only adjusts signal polarity and doesn’t affect 
>> lane mapping.
>> Both DP ctrl and PHY lane related config will not affect USB phy.
> 
> Probably we misundersand each other. The DP PHY should have orientation
> switch register, which controls whether 2-lane DP uses lanes 0/1 or 2/3.
> Can you use that register?
> 
Yes, DP PHY have orientation register as below.
DP_PHY_DP_PHY_CFG_1(0x88e9014) bit(7) SW_PORTSELECT
> Also, could you _please_ answer the question that I have asked? Is the
> order of lanes inverted between the DP controller and DP PHY? Or between
> DP PHY and the DP connector? If one uses USB3 signals coming from this
> port (yes, on the other board, not on the Ride), would they also need to
> switch the order of USB3 lanes? If one uses a DP-over-USB-C, are DP
> lanes are swapped?
> 
It's inverted between the DP controller and DP PHY.
If other use USB3 on the other board, will not need switch order of USB3 lanes,
If one use DP-over-USB-C, then need DP lanes swap.
>> Without extra Type-C mapping, the DP controller’s mapping indirectly decides 
>> how signals are transmitted through Type-C.
>> Mapping ensures proper data transmission and compatibility across interfaces.
>>
>> We only found sm6150 need this lane mapping config, 
>> For msm 4.14, please refer these links,
>> https://android.googlesource.com/kernel/msm/+/af03eef7d4c3cbd1fe26c67d4f1915b05d0c1488/arch/arm64/boot/dts/qcom/sm6150-sde.dtsi
>>  (qcom,logical2physical-lane-map)
>> https://android.googlesource.com/kernel/msm/+/af03eef7d4c3cbd1fe26c67d4f1915b05d0c1488/drivers/gpu/drm/msm/dp/dp_parser.c
>>  (dp_parser_misc)
>> https://android.googlesource.com/kernel/msm/+/af03eef7d4c3cbd1fe26c67d4f1915b05d0c1488/drivers/gpu/drm/msm/dp/dp_catalog_v200.c
>>  (dp_catalog_ctrl_lane_mapping_v200)
>>
>> If need process orientation info like dp_catalog_ctrl_lane_mapping_v200, 
>> then 
>> if implement in DP phy, then we need config dp_link register in PHY,
>> if implement in DP link, then we need pass orientation info to DP driver, 
>> perhaps we could add a new attribute to the phy_configure_opts_dp structure 
>> to pass this.
>> Do you have any suggestions?
> 
> Does SW_PORTSEL_VAL affect the DP lanes on this platform?
> 
SW_PORTSEL_VAL for USB3PHY_PCS_MISC_TYPEC_CTRL will not affect DP lanes in this 
DP or USB3 chip series.
USB3 will use USB3PHY_PCS_MISC_TYPEC_CTRL(SW_PORTSEL_VAL BIT_0) and DP will use 
DP_PHY_DP_PHY_CFG_1(SW_PORTSELECT BIT_7)
>>
>>>>
>>>> Additionally, if it were placed on the PHY side, the PHY would need access 
>>>> to dp_link’s domain which can access REG_DP_LOGICAL2PHYSICAL_LANE_MAPPING.
>>>
>>> I was thinking about inverting the SW_PORTSEL_VAL bit.
>>>
>>>> Therefore, we believe that the  max_dp_link_rate,max_dp_lanes and lane_map 
>>>> move to dp_link side is better.
>>>>
>>>>>>>> +
>>>>>>>> +       memcpy(msm_dp_panel->lane_map, lane_map, 
>>>>>>>> msm_dp_panel->max_dp_lanes * sizeof(u32));
>>>>>>>>
>>>>>>>>         msm_dp_panel->max_dp_link_rate = 
>>>>>>>> msm_dp_panel_link_frequencies(of_node);
>>>>>>>>         if (!msm_dp_panel->max_dp_link_rate)
>>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h 
>>>>>>>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>>>>>> index 
>>>>>>>> 0e944db3adf2f187f313664fe80cf540ec7a19f2..7603b92c32902bd3d4485539bd6308537ff75a2c
>>>>>>>>  100644
>>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>>>>>> @@ -11,6 +11,8 @@
>>>>>>>>  #include "dp_aux.h"
>>>>>>>>  #include "dp_link.h"
>>>>>>>>
>>>>>>>> +#define DP_MAX_NUM_DP_LANES    4
>>>>>>>> +
>>>>>>>>  struct edid;
>>>>>>>>
>>>>>>>>  struct msm_dp_display_mode {
>>>>>>>> @@ -46,6 +48,7 @@ struct msm_dp_panel {
>>>>>>>>         bool video_test;
>>>>>>>>         bool vsc_sdp_supported;
>>>>>>>>
>>>>>>>> +       u32 lane_map[DP_MAX_NUM_DP_LANES];
>>>>>>>>         u32 max_dp_lanes;
>>>>>>>>         u32 max_dp_link_rate;
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> linux-phy mailing list
>>>>>> linux-...@lists.infradead.org
>>>>>> https://lists.infradead.org/mailman/listinfo/linux-phy
>>>>>
>>>>
>>>
>>>
>>
> 

Reply via email to