mbox series

[0/8] Add DisplayPort support for QCS615 platform

Message ID 20241129-add-displayport-support-for-qcs615-platform-v1-0-09a4338d93ef@quicinc.com
Headers show
Series Add DisplayPort support for QCS615 platform | expand

Message

Xiangxu Yin Nov. 29, 2024, 7:57 a.m. UTC 
This series aims to extend the USB-C PHY to support DP mode and enable
DisplayPort on the Qualcomm QCS615 platform.

The devicetree modification for DisplayPort on QCS615 will be provided
in a future patch.

Signed-off-by: Xiangxu Yin <quic_xiangxuy@quicinc.com>
---
Xiangxu Yin (8):
      dt-bindings: display/msm: Document DP on QCS615
      dt-bindings: phy: qcom,msm8998-qmp-usb3-phy: Add DP support for QCS615
      phy: qcom: qmp-usbc: Add DP phy mode support on QCS615
      drm/msm/dp: Add DisplayPort support for QCS615
      drm/msm/dp: Add support for lane mapping configuration
      drm/msm/dp: Add maximum width limitation for modes
      drm/msm/dp: Retry Link Training 2 with lower pattern
      drm/msm/dp: Support external GPIO HPD with 3rd pinctrl chip

 .../bindings/display/msm/dp-controller.yaml        |   13 +
 .../bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml    |   21 +-
 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                   |   36 +-
 drivers/gpu/drm/msm/dp/dp_display.c                |   87 ++
 drivers/gpu/drm/msm/dp/dp_display.h                |    1 +
 drivers/gpu/drm/msm/dp/dp_panel.c                  |   26 +-
 drivers/gpu/drm/msm/dp/dp_panel.h                  |    4 +
 drivers/phy/qualcomm/phy-qcom-qmp-dp-phy.h         |    1 +
 drivers/phy/qualcomm/phy-qcom-qmp-usbc.c           | 1453 +++++++++++++++++---
 11 files changed, 1438 insertions(+), 217 deletions(-)
---
base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
change-id: 20241129-add-displayport-support-for-qcs615-platform-f31b6dc83919

Best regards,

Comments

Krzysztof Kozlowski Nov. 29, 2024, 8:14 a.m. UTC | #1 
On 29/11/2024 08:57, Xiangxu Yin wrote:
> Declare the DP QMP PHY present on the Qualcomm QCS615 platforms.
> 
> Signed-off-by: Xiangxu Yin <quic_xiangxuy@quicinc.com>
> ---
>  .../bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml     | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml
> index 1636285fbe535c430fdf792b33a5e9c523de323b..eb21cfe734526fce670c540212a607a016cedf2c 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml
> @@ -18,6 +18,7 @@ properties:
>      enum:
>        - qcom,msm8998-qmp-usb3-phy
>        - qcom,qcm2290-qmp-usb3-phy
> +      - qcom,qcs615-qmp-dp-phy
>        - qcom,qcs615-qmp-usb3-phy
>        - qcom,sdm660-qmp-usb3-phy
>        - qcom,sm6115-qmp-usb3-phy
> @@ -47,7 +48,7 @@ properties:
>      const: 0
>  
>    clock-output-names:
> -    maxItems: 1
> +    maxItems: 2


Why all devices now have two clocks? No, this needs lower constraints
and further customization per each variant.

>  
>    "#phy-cells":
>      const: 0
> @@ -62,7 +63,8 @@ properties:
>      items:
>        - items:
>            - description: phandle to TCSR hardware block
> -          - description: offset of the VLS CLAMP register
> +          - description: offset of the VLS CLAMP register in USB mode
> +                         and offset of the DP Phy mode register in DP mode

You change all existing devices, no.

>      description: Clamp register present in the TCSR
>  
>    ports:
> @@ -128,6 +130,21 @@ allOf:
>              - const: com_aux
>              - const: pipe
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,qcs615-qmp-dp-phy
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 2
> +        clock-names:
> +          items:
> +            - const: cfg_ahb
> +            - const: ref

Top level says you have minimum 4 clocks, not 2. You need to fix that,
if this devices stays in this schema. Anyway your changes suggest device
is quite different, so probably should not be here in the first place
but in different schema, maybe new one.



Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 29, 2024, 8:21 a.m. UTC | #2 
On 29/11/2024 08:57, Xiangxu Yin wrote:
>  
> +	if (of_find_property(pdev->dev.of_node, "dp-hpd-gpio", NULL)) {
> +		dp->ext_gpio = true;
> +		dp->gpio_num = of_get_named_gpio(pdev->dev.of_node, "dp-hpd-gpio", 0);
> +		if (dp->gpio_num < 0) {
> +			dev_err(&pdev->dev, "Failed to get gpio:%d\n", dp->gpio_num);
> +			return dp->gpio_num;
> +		}
> +
> +		if (!gpio_is_valid(dp->gpio_num)) {
> +			DRM_ERROR("gpio(%d) invalid\n", dp->gpio_num);
> +			return -EINVAL;
> +		}
> +
> +		rc = gpio_request(dp->gpio_num, "dp-hpd-gpio");
This is not how you request GPIOs. All this code is just wrong. See
Gpiolib API description/document. Or any other driver using GPIOs.

Best regards,
Krzysztof
Dmitry Baryshkov Nov. 29, 2024, 1:52 p.m. UTC | #3 
On Fri, 29 Nov 2024 at 09:59, Xiangxu Yin <quic_xiangxuy@quicinc.com> wrote:
>
> Introduce a maximum width constraint for modes during validation. This
> ensures that the modes are filtered based on hardware capabilities,
> specifically addressing the line buffer limitations of individual pipes.

This doesn't describe, why this is necessary. What does "buffer
limitations of individual pipes" mean?
If the platforms have hw capabilities like being unable to support 8k
or 10k, it should go to platform data

>
> Signed-off-by: Xiangxu Yin <quic_xiangxuy@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c |  3 +++
>  drivers/gpu/drm/msm/dp/dp_display.h |  1 +
>  drivers/gpu/drm/msm/dp/dp_panel.c   | 13 +++++++++++++
>  drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
>  4 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 4c83402fc7e0d41cb7621fa2efda043269d0a608..eb6fb76c68e505fafbec563440e9784f51e1894b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -944,6 +944,9 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
>         msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>         link_info = &msm_dp_display->panel->link_info;
>
> +       if (mode->hdisplay > msm_dp_display->panel->max_dp_width)
> +               return MODE_BAD;
> +
>         if (drm_mode_is_420_only(&dp->connector->display_info, mode) &&
>             msm_dp_display->panel->vsc_sdp_supported)
>                 mode_pclk_khz /= 2;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index ecbc2d92f546a346ee53adcf1b060933e4f54317..7a11f7eeb691976f06afc7aff67650397d7deb90 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -11,6 +11,7 @@
>  #include "disp/msm_disp_snapshot.h"
>
>  #define DP_MAX_PIXEL_CLK_KHZ   675000
> +#define DP_MAX_WIDTH   7680
>
>  struct msm_dp {
>         struct drm_device *drm_dev;
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 8654180aa259234bbd41f4f88c13c485f9791b1d..10501e301c5e073d8d34093b86a15d72e646a01f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -4,6 +4,7 @@
>   */
>
>  #include "dp_panel.h"
> +#include "dp_display.h"
>  #include "dp_utils.h"
>
>  #include <drm/drm_connector.h>
> @@ -455,6 +456,16 @@ static u32 msm_dp_panel_link_frequencies(struct device_node *of_node)
>         return frequency;
>  }
>
> +static u32 msm_dp_panel_max_width(struct device_node *of_node)
> +{
> +       u32 max_width = 0;
> +
> +       if (of_property_read_u32(of_node, "max-width", &max_width))
> +               max_width = DP_MAX_WIDTH;
> +
> +       return max_width;

msm_dp_panel->max_dp_width = DP_MAX_WIDTH;
of_property_read_u32(of_node, "max-width", &msm_dp_panel->max_dp_width);

> +}
> +
>  static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel)
>  {
>         struct msm_dp_panel_private *panel;
> @@ -490,6 +501,8 @@ static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel)
>         if (!msm_dp_panel->max_dp_link_rate)
>                 msm_dp_panel->max_dp_link_rate = DP_LINK_RATE_HBR2;
>
> +       msm_dp_panel->max_dp_width = msm_dp_panel_max_width(of_node);
> +
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 7603b92c32902bd3d4485539bd6308537ff75a2c..61513644161209c243bbb623ee4ded951b2a0597 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -51,6 +51,7 @@ struct msm_dp_panel {
>         u32 lane_map[DP_MAX_NUM_DP_LANES];
>         u32 max_dp_lanes;
>         u32 max_dp_link_rate;
> +       u32 max_dp_width;
>
>         u32 max_bw_code;
>  };
>
> --
> 2.25.1
>
Dmitry Baryshkov Nov. 29, 2024, 1:54 p.m. UTC | #4 
On Fri, 29 Nov 2024 at 09:59, Xiangxu Yin <quic_xiangxuy@quicinc.com> wrote:
>
> The Qualcomm QCS615 platform comes with a DisplayPort controller use the
> same base offset as sc7180. add support for this in DP driver.
>
> Signed-off-by: Xiangxu Yin <quic_xiangxuy@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Xiangxu Yin Dec. 2, 2024, 9:05 a.m. UTC | #5 
On 11/29/2024 9:52 PM, Dmitry Baryshkov wrote:
> On Fri, 29 Nov 2024 at 09:59, Xiangxu Yin <quic_xiangxuy@quicinc.com> wrote:
>>
>> Introduce a maximum width constraint for modes during validation. This
>> ensures that the modes are filtered based on hardware capabilities,
>> specifically addressing the line buffer limitations of individual pipes.
> 
> This doesn't describe, why this is necessary. What does "buffer
> limitations of individual pipes" mean?
> If the platforms have hw capabilities like being unable to support 8k
> or 10k, it should go to platform data
> 
It's SSPP line buffer limitation for this platform and only support to 2160 mode width.
Then, shall I add max_width config to struct msm_dp_desc in next patch? for other platform will set defualt value to ‘DP_MAX_WIDTH 7680'
>>
>> Signed-off-by: Xiangxu Yin <quic_xiangxuy@quicinc.com>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_display.c |  3 +++
>>  drivers/gpu/drm/msm/dp/dp_display.h |  1 +
>>  drivers/gpu/drm/msm/dp/dp_panel.c   | 13 +++++++++++++
>>  drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
>>  4 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 4c83402fc7e0d41cb7621fa2efda043269d0a608..eb6fb76c68e505fafbec563440e9784f51e1894b 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -944,6 +944,9 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
>>         msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>>         link_info = &msm_dp_display->panel->link_info;
>>
>> +       if (mode->hdisplay > msm_dp_display->panel->max_dp_width)
>> +               return MODE_BAD;
>> +
>>         if (drm_mode_is_420_only(&dp->connector->display_info, mode) &&
>>             msm_dp_display->panel->vsc_sdp_supported)
>>                 mode_pclk_khz /= 2;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index ecbc2d92f546a346ee53adcf1b060933e4f54317..7a11f7eeb691976f06afc7aff67650397d7deb90 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -11,6 +11,7 @@
>>  #include "disp/msm_disp_snapshot.h"
>>
>>  #define DP_MAX_PIXEL_CLK_KHZ   675000
>> +#define DP_MAX_WIDTH   7680
>>
>>  struct msm_dp {
>>         struct drm_device *drm_dev;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 8654180aa259234bbd41f4f88c13c485f9791b1d..10501e301c5e073d8d34093b86a15d72e646a01f 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -4,6 +4,7 @@
>>   */
>>
>>  #include "dp_panel.h"
>> +#include "dp_display.h"
>>  #include "dp_utils.h"
>>
>>  #include <drm/drm_connector.h>
>> @@ -455,6 +456,16 @@ static u32 msm_dp_panel_link_frequencies(struct device_node *of_node)
>>         return frequency;
>>  }
>>
>> +static u32 msm_dp_panel_max_width(struct device_node *of_node)
>> +{
>> +       u32 max_width = 0;
>> +
>> +       if (of_property_read_u32(of_node, "max-width", &max_width))
>> +               max_width = DP_MAX_WIDTH;
>> +
>> +       return max_width;
> 
> msm_dp_panel->max_dp_width = DP_MAX_WIDTH;
> of_property_read_u32(of_node, "max-width", &msm_dp_panel->max_dp_width);
> 
>> +}
>> +
>>  static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel)
>>  {
>>         struct msm_dp_panel_private *panel;
>> @@ -490,6 +501,8 @@ static int msm_dp_panel_parse_dt(struct msm_dp_panel *msm_dp_panel)
>>         if (!msm_dp_panel->max_dp_link_rate)
>>                 msm_dp_panel->max_dp_link_rate = DP_LINK_RATE_HBR2;
>>
>> +       msm_dp_panel->max_dp_width = msm_dp_panel_max_width(of_node);
>> +
>>         return 0;
>>  }
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 7603b92c32902bd3d4485539bd6308537ff75a2c..61513644161209c243bbb623ee4ded951b2a0597 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -51,6 +51,7 @@ struct msm_dp_panel {
>>         u32 lane_map[DP_MAX_NUM_DP_LANES];
>>         u32 max_dp_lanes;
>>         u32 max_dp_link_rate;
>> +       u32 max_dp_width;
>>
>>         u32 max_bw_code;
>>  };
>>
>> --
>> 2.25.1
>>
> 
>
Dmitry Baryshkov Dec. 2, 2024, 9:32 a.m. UTC | #6 
On Mon, 2 Dec 2024 at 11:05, Xiangxu Yin <quic_xiangxuy@quicinc.com> wrote:
>
>
>
> On 11/29/2024 9:52 PM, Dmitry Baryshkov wrote:
> > On Fri, 29 Nov 2024 at 09:59, Xiangxu Yin <quic_xiangxuy@quicinc.com> wrote:
> >>
> >> Introduce a maximum width constraint for modes during validation. This
> >> ensures that the modes are filtered based on hardware capabilities,
> >> specifically addressing the line buffer limitations of individual pipes.
> >
> > This doesn't describe, why this is necessary. What does "buffer
> > limitations of individual pipes" mean?
> > If the platforms have hw capabilities like being unable to support 8k
> > or 10k, it should go to platform data
> >
> It's SSPP line buffer limitation for this platform and only support to 2160 mode width.
> Then, shall I add max_width config to struct msm_dp_desc in next patch? for other platform will set defualt value to ‘DP_MAX_WIDTH 7680'

SSPP line buffer limitations are to be handled in the DPU driver. The
DP driver shouldn't care about those.

> >>
> >> Signed-off-by: Xiangxu Yin <quic_xiangxuy@quicinc.com>
> >> ---
> >>  drivers/gpu/drm/msm/dp/dp_display.c |  3 +++
> >>  drivers/gpu/drm/msm/dp/dp_display.h |  1 +
> >>  drivers/gpu/drm/msm/dp/dp_panel.c   | 13 +++++++++++++
> >>  drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
> >>  4 files changed, 18 insertions(+)