[PATCH v1 0/2] Add support for eDP on SC7280

[PATCH v1 0/2] Add support for eDP on SC7280

[Sankeerth Billakanti]

	This series will add eDP controller support for Qualcomm SC7280
platform. These patches are baseline changes with which we can enable
eDP display on sc7280. The sc7280 eDP controller can also support
additional features such as backlight control, PSR etc. which will be
enabled in subsequent patch series.

	This is based on Bjorn's changes in the below mentioned series
to support both eDP and DP programming through the same driver:
v1 of https://patchwork.freedesktop.org/series/92860/
v1 of https://patchwork.freedesktop.org/series/92992/

Summary of changes:
Add support for eDP on SC7280 platform.
Add the new compatible string to documentation.


Sankeerth Billakanti (2):
  drm/msm/dp: Add support for SC7280 eDP
  dt-bindings: Add SC7280 compatible string

 .../bindings/display/msm/dp-controller.yaml        |  3 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |  4 +--
 drivers/gpu/drm/msm/dp/dp_ctrl.c                   | 19 +++++++++++++
 drivers/gpu/drm/msm/dp/dp_display.c                | 32 ++++++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_parser.c                 | 31 +++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_parser.h                 |  5 ++++
 6 files changed, 90 insertions(+), 4 deletions(-)

-- 
The Qualcomm Innovatin Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP

[Sankeerth Billakanti]

The eDP controller on SC7280 is similar to the eDP/DP controllers
supported by the current driver implementation.

SC7280 supports one EDP and one DP controller which can operate
concurrently.

The following are some required changes for the sc7280 sink:
1. Additional gpio configuration for backlight and pwm via pmic.
2. ASSR support programming on the sink.
3. SSC support programming on the sink.

Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  4 ++--
 drivers/gpu/drm/msm/dp/dp_ctrl.c               | 19 +++++++++++++++
 drivers/gpu/drm/msm/dp/dp_display.c            | 32 ++++++++++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_parser.c             | 31 +++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_parser.h             |  5 ++++
 5 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index b131fd37..1096c44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -856,9 +856,9 @@ static const struct dpu_intf_cfg sm8150_intf[] = {
 };
 
 static const struct dpu_intf_cfg sc7280_intf[] = {
-	INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
+	INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 1, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
 	INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
-	INTF_BLK("intf_5", INTF_5, 0x39000, INTF_EDP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
+	INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
 };
 
 /*************************************************************
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index d2569da..06d5a2d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1244,7 +1244,9 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
 		struct dp_cr_status *cr, int *training_step)
 {
 	int ret = 0;
+	u8 *dpcd = ctrl->panel->dpcd;
 	u8 encoding = DP_SET_ANSI_8B10B;
+	u8 ssc = 0, assr = 0;
 	struct dp_link_info link_info = {0};
 
 	dp_ctrl_config_ctrl(ctrl);
@@ -1254,9 +1256,21 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
 	link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
 
 	dp_aux_link_configure(ctrl->aux, &link_info);
+
+	if (dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
+		ssc = DP_SPREAD_AMP_0_5;
+		drm_dp_dpcd_write(ctrl->aux, DP_DOWNSPREAD_CTRL, &ssc, 1);
+	}
+
 	drm_dp_dpcd_write(ctrl->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
 				&encoding, 1);
 
+	if (dpcd[DP_EDP_CONFIGURATION_CAP] & DP_ALTERNATE_SCRAMBLER_RESET_CAP) {
+		assr = DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+		drm_dp_dpcd_write(ctrl->aux, DP_EDP_CONFIGURATION_SET,
+				&assr, 1);
+	}
+
 	ret = dp_ctrl_link_train_1(ctrl, cr, training_step);
 	if (ret) {
 		DRM_ERROR("link training #1 failed. ret=%d\n", ret);
@@ -1328,9 +1342,11 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
 	struct dp_io *dp_io = &ctrl->parser->io;
 	struct phy *phy = dp_io->phy;
 	struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
+	u8 *dpcd = ctrl->panel->dpcd;
 
 	opts_dp->lanes = ctrl->link->link_params.num_lanes;
 	opts_dp->link_rate = ctrl->link->link_params.rate / 100;
+	opts_dp->ssc = dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5;
 	dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
 					ctrl->link->link_params.rate * 1000);
 
@@ -1760,6 +1776,9 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 	ctrl->link->link_params.num_lanes = ctrl->panel->link_info.num_lanes;
 	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 
+	if (ctrl->dp_ctrl.pixel_rate == 0)
+		return -EINVAL;
+
 	DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
 		ctrl->link->link_params.rate,
 		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index ee5bf64..a772290 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -117,8 +117,36 @@ struct dp_display_private {
 	struct dp_audio *audio;
 };
 
+struct msm_dp_config {
+	phys_addr_t io_start[3];
+	size_t num_dp;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+	.io_start = { 0x0ae90000 },
+	.num_dp = 1,
+};
+
+static const struct msm_dp_config sc8180x_dp_cfg = {
+	.io_start = { 0xae90000, 0xae98000, 0 },
+	.num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+	.io_start = { 0, 0, 0xae9a000 },
+	.num_dp = 3,
+};
+
+static const struct msm_dp_config sc7280_edp_cfg = {
+	.io_start = { 0xaea0000, 0 },
+	.num_dp = 2,
+};
+
 static const struct of_device_id dp_dt_match[] = {
-	{.compatible = "qcom,sc7180-dp"},
+	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
+	{ .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
+	{ .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
+	{ .compatible = "qcom,sc7280-edp", .data = &sc7280_edp_cfg },
 	{}
 };
 
@@ -1408,7 +1436,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
 
 	dp_hpd_event_setup(dp);
 
-	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1);
 }
 
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 0519dd3..c05fc0a 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -248,6 +248,33 @@ static int dp_parser_clock(struct dp_parser *parser)
 	return 0;
 }
 
+static int dp_parser_gpio(struct dp_parser *parser)
+{
+	struct device *dev = &parser->pdev->dev;
+	int ret;
+
+	parser->panel_bklt_gpio = devm_gpiod_get(dev, "panel-bklt",
+			GPIOD_OUT_HIGH);
+	if (IS_ERR(parser->panel_bklt_gpio)) {
+		ret = PTR_ERR(parser->panel_bklt_gpio);
+		parser->panel_bklt_gpio = NULL;
+		DRM_ERROR("%s: cannot get panel-bklt gpio, %d\n", __func__, ret);
+		goto fail;
+	}
+
+	parser->panel_pwm_gpio = devm_gpiod_get(dev, "panel-pwm", GPIOD_OUT_HIGH);
+	if (IS_ERR(parser->panel_pwm_gpio)) {
+		ret = PTR_ERR(parser->panel_pwm_gpio);
+		parser->panel_pwm_gpio = NULL;
+		DRM_ERROR("%s: cannot get panel-pwm gpio, %d\n", __func__, ret);
+		goto fail;
+	}
+
+	DRM_INFO("gpio on");
+fail:
+	return 0;
+}
+
 static int dp_parser_parse(struct dp_parser *parser)
 {
 	int rc = 0;
@@ -269,6 +296,10 @@ static int dp_parser_parse(struct dp_parser *parser)
 	if (rc)
 		return rc;
 
+	rc = dp_parser_gpio(parser);
+	if (rc)
+		return rc;
+
 	/* Map the corresponding regulator information according to
 	 * version. Currently, since we only have one supported platform,
 	 * mapping the regulator directly.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 34b4962..7e94bbf 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -9,6 +9,7 @@
 #include <linux/platform_device.h>
 #include <linux/phy/phy.h>
 #include <linux/phy/phy-dp.h>
+#include <linux/gpio/consumer.h>
 
 #include "dpu_io_util.h"
 #include "msm_drv.h"
@@ -112,6 +113,10 @@ struct dp_parser {
 	struct platform_device *pdev;
 	struct dss_module_power mp[DP_MAX_PM];
 	struct dp_pinctrl pinctrl;
+
+	struct gpio_desc *panel_bklt_gpio;
+	struct gpio_desc *panel_pwm_gpio;
+
 	struct dp_io io;
 	struct dp_display_data disp_data;
 	const struct dp_regulator_cfg *regulator_cfg;
-- 
The Qualcomm Innovatin Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP

[Stephen Boyd]

Quoting Sankeerth Billakanti (2021-08-11 17:08:01)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index b131fd37..1096c44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -856,9 +856,9 @@ static const struct dpu_intf_cfg sm8150_intf[] = {
>  };
>
>  static const struct dpu_intf_cfg sc7280_intf[] = {
> -       INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> +       INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 1, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>         INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -       INTF_BLK("intf_5", INTF_5, 0x39000, INTF_EDP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> +       INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>  };
>
>  /*************************************************************
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index d2569da..06d5a2d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1244,7 +1244,9 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>                 struct dp_cr_status *cr, int *training_step)
>  {
>         int ret = 0;
> +       u8 *dpcd = ctrl->panel->dpcd;
>         u8 encoding = DP_SET_ANSI_8B10B;
> +       u8 ssc = 0, assr = 0;

Please don't initialize to zero and then overwrite it before using it.
It hides usage before actual initialization bugs.

>         struct dp_link_info link_info = {0};
>
>         dp_ctrl_config_ctrl(ctrl);
> @@ -1254,9 +1256,21 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>         link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
>
>         dp_aux_link_configure(ctrl->aux, &link_info);
> +
> +       if (dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
> +               ssc = DP_SPREAD_AMP_0_5;
> +               drm_dp_dpcd_write(ctrl->aux, DP_DOWNSPREAD_CTRL, &ssc, 1);
> +       }
> +
>         drm_dp_dpcd_write(ctrl->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
>                                 &encoding, 1);
>
> +       if (dpcd[DP_EDP_CONFIGURATION_CAP] & DP_ALTERNATE_SCRAMBLER_RESET_CAP) {
> +               assr = DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
> +               drm_dp_dpcd_write(ctrl->aux, DP_EDP_CONFIGURATION_SET,
> +                               &assr, 1);
> +       }
> +
>         ret = dp_ctrl_link_train_1(ctrl, cr, training_step);
>         if (ret) {
>                 DRM_ERROR("link training #1 failed. ret=%d\n", ret);
> @@ -1328,9 +1342,11 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>         struct dp_io *dp_io = &ctrl->parser->io;
>         struct phy *phy = dp_io->phy;
>         struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
> +       u8 *dpcd = ctrl->panel->dpcd;

const?

>
>         opts_dp->lanes = ctrl->link->link_params.num_lanes;
>         opts_dp->link_rate = ctrl->link->link_params.rate / 100;
> +       opts_dp->ssc = dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5;
>         dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
>                                         ctrl->link->link_params.rate * 1000);
>
> @@ -1760,6 +1776,9 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>         ctrl->link->link_params.num_lanes = ctrl->panel->link_info.num_lanes;
>         ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>
> +       if (ctrl->dp_ctrl.pixel_rate == 0)
> +               return -EINVAL;
> +

Why are we enabling the stream with a zero pixel clk?

>         DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>                 ctrl->link->link_params.rate,
>                 ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ee5bf64..a772290 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -117,8 +117,36 @@ struct dp_display_private {
>         struct dp_audio *audio;
>  };
>
> +struct msm_dp_config {
> +       phys_addr_t io_start[3];
> +       size_t num_dp;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> +       .io_start = { 0x0ae90000 },
> +       .num_dp = 1,
> +};
> +
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> +       .io_start = { 0xae90000, 0xae98000, 0 },
> +       .num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc8180x_edp_cfg = {
> +       .io_start = { 0, 0, 0xae9a000 },
> +       .num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc7280_edp_cfg = {
> +       .io_start = { 0xaea0000, 0 },
> +       .num_dp = 2,
> +};

Are all of these supposed to be here?

> +
>  static const struct of_device_id dp_dt_match[] = {
> -       {.compatible = "qcom,sc7180-dp"},
> +       { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
> +       { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
> +       { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
> +       { .compatible = "qcom,sc7280-edp", .data = &sc7280_edp_cfg },

Please sort alphabetically on compatible string, it helps avoid
conflicts in the future.

>         {}
>  };
>
> @@ -1408,7 +1436,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>
>         dp_hpd_event_setup(dp);
>
> -       dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> +       dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1);
>  }
>
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 0519dd3..c05fc0a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -248,6 +248,33 @@ static int dp_parser_clock(struct dp_parser *parser)
>         return 0;
>  }
>
> +static int dp_parser_gpio(struct dp_parser *parser)
> +{
> +       struct device *dev = &parser->pdev->dev;
> +       int ret;
> +
> +       parser->panel_bklt_gpio = devm_gpiod_get(dev, "panel-bklt",
> +                       GPIOD_OUT_HIGH);
> +       if (IS_ERR(parser->panel_bklt_gpio)) {
> +               ret = PTR_ERR(parser->panel_bklt_gpio);
> +               parser->panel_bklt_gpio = NULL;
> +               DRM_ERROR("%s: cannot get panel-bklt gpio, %d\n", __func__, ret);
> +               goto fail;
> +       }
> +
> +       parser->panel_pwm_gpio = devm_gpiod_get(dev, "panel-pwm", GPIOD_OUT_HIGH);
> +       if (IS_ERR(parser->panel_pwm_gpio)) {
> +               ret = PTR_ERR(parser->panel_pwm_gpio);
> +               parser->panel_pwm_gpio = NULL;
> +               DRM_ERROR("%s: cannot get panel-pwm gpio, %d\n", __func__, ret);
> +               goto fail;
> +       }
> +
> +       DRM_INFO("gpio on");
> +fail:
> +       return 0;
> +}

Don't we have pwm backlight drivers like
drivers/video/backlight/pwm_bl.c to support this? This sort of thing
doesn't belong in the dp driver.

> +
>  static int dp_parser_parse(struct dp_parser *parser)
>  {
>         int rc = 0;
> @@ -269,6 +296,10 @@ static int dp_parser_parse(struct dp_parser *parser)
>         if (rc)
>                 return rc;
>
> +       rc = dp_parser_gpio(parser);
> +       if (rc)
> +               return rc;
> +
>         /* Map the corresponding regulator information according to
>          * version. Currently, since we only have one supported platform,
>          * mapping the regulator directly.

Re: [PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP

[sbillaka]

On 2021-08-12 06:11, Stephen Boyd wrote:
> Quoting Sankeerth Billakanti (2021-08-11 17:08:01)
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> index b131fd37..1096c44 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -856,9 +856,9 @@ static const struct dpu_intf_cfg sm8150_intf[] = {
>>  };
>> 
>>  static const struct dpu_intf_cfg sc7280_intf[] = {
>> -       INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 0, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>> +       INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 1, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>>         INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>> -       INTF_BLK("intf_5", INTF_5, 0x39000, INTF_EDP, 0, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>> +       INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, 0, 24, 
>> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>>  };
>> 
>>  /*************************************************************
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index d2569da..06d5a2d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1244,7 +1244,9 @@ static int dp_ctrl_link_train(struct 
>> dp_ctrl_private *ctrl,
>>                 struct dp_cr_status *cr, int *training_step)
>>  {
>>         int ret = 0;
>> +       u8 *dpcd = ctrl->panel->dpcd;
>>         u8 encoding = DP_SET_ANSI_8B10B;
>> +       u8 ssc = 0, assr = 0;
> 
> Please don't initialize to zero and then overwrite it before using it.
> It hides usage before actual initialization bugs.
> 

Okay. I will change it.

>>         struct dp_link_info link_info = {0};
>> 
>>         dp_ctrl_config_ctrl(ctrl);
>> @@ -1254,9 +1256,21 @@ static int dp_ctrl_link_train(struct 
>> dp_ctrl_private *ctrl,
>>         link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
>> 
>>         dp_aux_link_configure(ctrl->aux, &link_info);
>> +
>> +       if (dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
>> +               ssc = DP_SPREAD_AMP_0_5;
>> +               drm_dp_dpcd_write(ctrl->aux, DP_DOWNSPREAD_CTRL, &ssc, 
>> 1);
>> +       }
>> +
>>         drm_dp_dpcd_write(ctrl->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
>>                                 &encoding, 1);
>> 
>> +       if (dpcd[DP_EDP_CONFIGURATION_CAP] & 
>> DP_ALTERNATE_SCRAMBLER_RESET_CAP) {
>> +               assr = DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
>> +               drm_dp_dpcd_write(ctrl->aux, DP_EDP_CONFIGURATION_SET,
>> +                               &assr, 1);
>> +       }
>> +
>>         ret = dp_ctrl_link_train_1(ctrl, cr, training_step);
>>         if (ret) {
>>                 DRM_ERROR("link training #1 failed. ret=%d\n", ret);
>> @@ -1328,9 +1342,11 @@ static int 
>> dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>>         struct dp_io *dp_io = &ctrl->parser->io;
>>         struct phy *phy = dp_io->phy;
>>         struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
>> +       u8 *dpcd = ctrl->panel->dpcd;
> 
> const?
> 

Okay. I will change to const u8 *dpcd at all the required places.

>> 
>>         opts_dp->lanes = ctrl->link->link_params.num_lanes;
>>         opts_dp->link_rate = ctrl->link->link_params.rate / 100;
>> +       opts_dp->ssc = dpcd[DP_MAX_DOWNSPREAD] & 
>> DP_MAX_DOWNSPREAD_0_5;
>>         dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
>>                                         ctrl->link->link_params.rate * 
>> 1000);
>> 
>> @@ -1760,6 +1776,9 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>         ctrl->link->link_params.num_lanes = 
>> ctrl->panel->link_info.num_lanes;
>>         ctrl->dp_ctrl.pixel_rate = 
>> ctrl->panel->dp_mode.drm_mode.clock;
>> 
>> +       if (ctrl->dp_ctrl.pixel_rate == 0)
>> +               return -EINVAL;
>> +
> 
> Why are we enabling the stream with a zero pixel clk?
> 

This was an error condition I encountered while bringing up sc7280. HPD 
processing was delayed and I got a commit with pixel clock = 0. I will 
recheck why this is happening.

>>         DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>>                 ctrl->link->link_params.rate,
>>                 ctrl->link->link_params.num_lanes, 
>> ctrl->dp_ctrl.pixel_rate);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index ee5bf64..a772290 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -117,8 +117,36 @@ struct dp_display_private {
>>         struct dp_audio *audio;
>>  };
>> 
>> +struct msm_dp_config {
>> +       phys_addr_t io_start[3];
>> +       size_t num_dp;
>> +};
>> +
>> +static const struct msm_dp_config sc7180_dp_cfg = {
>> +       .io_start = { 0x0ae90000 },
>> +       .num_dp = 1,
>> +};
>> +
>> +static const struct msm_dp_config sc8180x_dp_cfg = {
>> +       .io_start = { 0xae90000, 0xae98000, 0 },
>> +       .num_dp = 3,
>> +};
>> +
>> +static const struct msm_dp_config sc8180x_edp_cfg = {
>> +       .io_start = { 0, 0, 0xae9a000 },
>> +       .num_dp = 3,
>> +};
>> +
>> +static const struct msm_dp_config sc7280_edp_cfg = {
>> +       .io_start = { 0xaea0000, 0 },
>> +       .num_dp = 2,
>> +};
> 
> Are all of these supposed to be here?

No. I will remove them. Only sc7280_edp_cfg will be there.

> 
>> +
>>  static const struct of_device_id dp_dt_match[] = {
>> -       {.compatible = "qcom,sc7180-dp"},
>> +       { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
>> +       { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
>> +       { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg 
>> },
>> +       { .compatible = "qcom,sc7280-edp", .data = &sc7280_edp_cfg },
> 
> Please sort alphabetically on compatible string, it helps avoid
> conflicts in the future.

Okay

> 
>>         {}
>>  };
>> 
>> @@ -1408,7 +1436,7 @@ void msm_dp_irq_postinstall(struct msm_dp 
>> *dp_display)
>> 
>>         dp_hpd_event_setup(dp);
>> 
>> -       dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>> +       dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1);
>>  }
>> 
>>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor 
>> *minor)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c 
>> b/drivers/gpu/drm/msm/dp/dp_parser.c
>> index 0519dd3..c05fc0a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
>> @@ -248,6 +248,33 @@ static int dp_parser_clock(struct dp_parser 
>> *parser)
>>         return 0;
>>  }
>> 
>> +static int dp_parser_gpio(struct dp_parser *parser)
>> +{
>> +       struct device *dev = &parser->pdev->dev;
>> +       int ret;
>> +
>> +       parser->panel_bklt_gpio = devm_gpiod_get(dev, "panel-bklt",
>> +                       GPIOD_OUT_HIGH);
>> +       if (IS_ERR(parser->panel_bklt_gpio)) {
>> +               ret = PTR_ERR(parser->panel_bklt_gpio);
>> +               parser->panel_bklt_gpio = NULL;
>> +               DRM_ERROR("%s: cannot get panel-bklt gpio, %d\n", 
>> __func__, ret);
>> +               goto fail;
>> +       }
>> +
>> +       parser->panel_pwm_gpio = devm_gpiod_get(dev, "panel-pwm", 
>> GPIOD_OUT_HIGH);
>> +       if (IS_ERR(parser->panel_pwm_gpio)) {
>> +               ret = PTR_ERR(parser->panel_pwm_gpio);
>> +               parser->panel_pwm_gpio = NULL;
>> +               DRM_ERROR("%s: cannot get panel-pwm gpio, %d\n", 
>> __func__, ret);
>> +               goto fail;
>> +       }
>> +
>> +       DRM_INFO("gpio on");
>> +fail:
>> +       return 0;
>> +}
> 
> Don't we have pwm backlight drivers like
> drivers/video/backlight/pwm_bl.c to support this? This sort of thing
> doesn't belong in the dp driver.

Okay. I will explore it.

> 
>> +
>>  static int dp_parser_parse(struct dp_parser *parser)
>>  {
>>         int rc = 0;
>> @@ -269,6 +296,10 @@ static int dp_parser_parse(struct dp_parser 
>> *parser)
>>         if (rc)
>>                 return rc;
>> 
>> +       rc = dp_parser_gpio(parser);
>> +       if (rc)
>> +               return rc;
>> +
>>         /* Map the corresponding regulator information according to
>>          * version. Currently, since we only have one supported 
>> platform,
>>          * mapping the regulator directly.

Thank you,
Sankeerth

Re: [PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP

[Bjorn Andersson]

On Thu 12 Aug 19:28 CDT 2021, sbillaka@codeaurora.org wrote:

> On 2021-08-12 06:11, Stephen Boyd wrote:
> > Quoting Sankeerth Billakanti (2021-08-11 17:08:01)
[..]
> > > +static int dp_parser_gpio(struct dp_parser *parser)
> > > +{
> > > +       struct device *dev = &parser->pdev->dev;
> > > +       int ret;
> > > +
> > > +       parser->panel_bklt_gpio = devm_gpiod_get(dev, "panel-bklt",
> > > +                       GPIOD_OUT_HIGH);
> > > +       if (IS_ERR(parser->panel_bklt_gpio)) {
> > > +               ret = PTR_ERR(parser->panel_bklt_gpio);
> > > +               parser->panel_bklt_gpio = NULL;
> > > +               DRM_ERROR("%s: cannot get panel-bklt gpio, %d\n",
> > > __func__, ret);
> > > +               goto fail;
> > > +       }
> > > +
> > > +       parser->panel_pwm_gpio = devm_gpiod_get(dev, "panel-pwm",
> > > GPIOD_OUT_HIGH);
> > > +       if (IS_ERR(parser->panel_pwm_gpio)) {
> > > +               ret = PTR_ERR(parser->panel_pwm_gpio);
> > > +               parser->panel_pwm_gpio = NULL;
> > > +               DRM_ERROR("%s: cannot get panel-pwm gpio, %d\n",
> > > __func__, ret);
> > > +               goto fail;
> > > +       }
> > > +
> > > +       DRM_INFO("gpio on");
> > > +fail:
> > > +       return 0;
> > > +}
> > 
> > Don't we have pwm backlight drivers like
> > drivers/video/backlight/pwm_bl.c to support this? This sort of thing
> > doesn't belong in the dp driver.
> 
> Okay. I will explore it.
> 

I proposed that we attach a drm_panel and allow that to control the
(pwm-)backlight. Here's the link for the DP patch:

https://lore.kernel.org/linux-arm-msm/20210726231351.655302-1-bjorn.andersson@linaro.org/

Regards,
Bjorn

Re: [PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP

[Matthias Kaehlcke]

On Thu, Aug 12, 2021 at 05:38:01AM +0530, Sankeerth Billakanti wrote:
> The eDP controller on SC7280 is similar to the eDP/DP controllers
> supported by the current driver implementation.
> 
> SC7280 supports one EDP and one DP controller which can operate
> concurrently.
> 
> The following are some required changes for the sc7280 sink:
> 1. Additional gpio configuration for backlight and pwm via pmic.
> 2. ASSR support programming on the sink.
> 3. SSC support programming on the sink.
> 
> Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  4 ++--
>  drivers/gpu/drm/msm/dp/dp_ctrl.c               | 19 +++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_display.c            | 32 ++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_parser.c             | 31 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_parser.h             |  5 ++++
>  5 files changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index b131fd37..1096c44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -856,9 +856,9 @@ static const struct dpu_intf_cfg sm8150_intf[] = {
>  };
>  
>  static const struct dpu_intf_cfg sc7280_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> +	INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 1, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>  	INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -	INTF_BLK("intf_5", INTF_5, 0x39000, INTF_EDP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> +	INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>  };
>  
>  /*************************************************************
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index d2569da..06d5a2d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1244,7 +1244,9 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>  		struct dp_cr_status *cr, int *training_step)
>  {
>  	int ret = 0;
> +	u8 *dpcd = ctrl->panel->dpcd;
>  	u8 encoding = DP_SET_ANSI_8B10B;
> +	u8 ssc = 0, assr = 0;
>  	struct dp_link_info link_info = {0};
>  
>  	dp_ctrl_config_ctrl(ctrl);
> @@ -1254,9 +1256,21 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>  	link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
>  
>  	dp_aux_link_configure(ctrl->aux, &link_info);
> +
> +	if (dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
> +		ssc = DP_SPREAD_AMP_0_5;
> +		drm_dp_dpcd_write(ctrl->aux, DP_DOWNSPREAD_CTRL, &ssc, 1);
> +	}
> +
>  	drm_dp_dpcd_write(ctrl->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
>  				&encoding, 1);
>  
> +	if (dpcd[DP_EDP_CONFIGURATION_CAP] & DP_ALTERNATE_SCRAMBLER_RESET_CAP) {
> +		assr = DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
> +		drm_dp_dpcd_write(ctrl->aux, DP_EDP_CONFIGURATION_SET,
> +				&assr, 1);
> +	}
> +
>  	ret = dp_ctrl_link_train_1(ctrl, cr, training_step);
>  	if (ret) {
>  		DRM_ERROR("link training #1 failed. ret=%d\n", ret);
> @@ -1328,9 +1342,11 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>  	struct dp_io *dp_io = &ctrl->parser->io;
>  	struct phy *phy = dp_io->phy;
>  	struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
> +	u8 *dpcd = ctrl->panel->dpcd;
>  
>  	opts_dp->lanes = ctrl->link->link_params.num_lanes;
>  	opts_dp->link_rate = ctrl->link->link_params.rate / 100;
> +	opts_dp->ssc = dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5;
>  	dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
>  					ctrl->link->link_params.rate * 1000);
>  
> @@ -1760,6 +1776,9 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>  	ctrl->link->link_params.num_lanes = ctrl->panel->link_info.num_lanes;
>  	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>  
> +	if (ctrl->dp_ctrl.pixel_rate == 0)
> +		return -EINVAL;
> +
>  	DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
>  		ctrl->link->link_params.rate,
>  		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ee5bf64..a772290 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -117,8 +117,36 @@ struct dp_display_private {
>  	struct dp_audio *audio;
>  };
>  
> +struct msm_dp_config {
> +	phys_addr_t io_start[3];
> +	size_t num_dp;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> +	.io_start = { 0x0ae90000 },
> +	.num_dp = 1,
> +};
> +
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> +	.io_start = { 0xae90000, 0xae98000, 0 },
> +	.num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc8180x_edp_cfg = {
> +	.io_start = { 0, 0, 0xae9a000 },
> +	.num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc7280_edp_cfg = {
> +	.io_start = { 0xaea0000, 0 },
> +	.num_dp = 2,
> +};

This data isn't used anywhere, is there some patch missing?

And in case it is used (at some point), shouldn't at least the 'io_start'
addresses be specified in the device tree rather than the driver?

> +
>  static const struct of_device_id dp_dt_match[] = {
> -	{.compatible = "qcom,sc7180-dp"},
> +	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
> +	{ .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
> +	{ .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
> +	{ .compatible = "qcom,sc7280-edp", .data = &sc7280_edp_cfg },
>  	{}
>  };
>  
> @@ -1408,7 +1436,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>  
>  	dp_hpd_event_setup(dp);
>  
> -	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> +	dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1);
>  }
>  
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 0519dd3..c05fc0a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -248,6 +248,33 @@ static int dp_parser_clock(struct dp_parser *parser)
>  	return 0;
>  }
>  
> +static int dp_parser_gpio(struct dp_parser *parser)
> +{
> +	struct device *dev = &parser->pdev->dev;
> +	int ret;
> +
> +	parser->panel_bklt_gpio = devm_gpiod_get(dev, "panel-bklt",
> +			GPIOD_OUT_HIGH);
> +	if (IS_ERR(parser->panel_bklt_gpio)) {
> +		ret = PTR_ERR(parser->panel_bklt_gpio);
> +		parser->panel_bklt_gpio = NULL;
> +		DRM_ERROR("%s: cannot get panel-bklt gpio, %d\n", __func__, ret);
> +		goto fail;
> +	}
> +
> +	parser->panel_pwm_gpio = devm_gpiod_get(dev, "panel-pwm", GPIOD_OUT_HIGH);
> +	if (IS_ERR(parser->panel_pwm_gpio)) {
> +		ret = PTR_ERR(parser->panel_pwm_gpio);
> +		parser->panel_pwm_gpio = NULL;
> +		DRM_ERROR("%s: cannot get panel-pwm gpio, %d\n", __func__, ret);
> +		goto fail;
> +	}

Just setting the GPIOs to high on initialization and then leaving them
unattended doesn't look right. I saw Stephen already pointed towards
drivers/video/backlight/pwm_bl.c.

> +
> +	DRM_INFO("gpio on");
> +fail:
> +	return 0;

This function always returns 0, either the return type should be void, or it
should return a different value in the error case

> +}
> +
>  static int dp_parser_parse(struct dp_parser *parser)
>  {
>  	int rc = 0;
> @@ -269,6 +296,10 @@ static int dp_parser_parse(struct dp_parser *parser)
>  	if (rc)
>  		return rc;
>  
> +	rc = dp_parser_gpio(parser);
> +	if (rc)
> +		return rc;

The function never returns a non-zero value (see above).

Re: [PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP

[Stephen Boyd]

Quoting Sankeerth Billakanti (2021-08-11 17:08:01)
> The eDP controller on SC7280 is similar to the eDP/DP controllers
> supported by the current driver implementation.
>
> SC7280 supports one EDP and one DP controller which can operate
> concurrently.
>
> The following are some required changes for the sc7280 sink:
> 1. Additional gpio configuration for backlight and pwm via pmic.
> 2. ASSR support programming on the sink.
> 3. SSC support programming on the sink.
>
> Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  4 ++--
>  drivers/gpu/drm/msm/dp/dp_ctrl.c               | 19 +++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_display.c            | 32 ++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_parser.c             | 31 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_parser.h             |  5 ++++
>  5 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index b131fd37..1096c44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -856,9 +856,9 @@ static const struct dpu_intf_cfg sm8150_intf[] = {
>  };
>
>  static const struct dpu_intf_cfg sc7280_intf[] = {
> -       INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> +       INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 1, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>         INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -       INTF_BLK("intf_5", INTF_5, 0x39000, INTF_EDP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> +       INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),

Why is this INTF_5? Instead of INTF_2? I noticed that if I changed it to
INTF_2 that I could get external DP to work but not the internal eDP.
Then changing it back to INTF_5 got eDP interface working but not DP. I
also noticed that we changed it from INTF_EDP to INTF_DP for the eDP
hardware. Can you please explain this struct? I looked at it and I still
don't understand what's going on.

The index (fifth element above) seems to need to match the index that is
set for the address in sc7280_edp_cfg[]. If the two don't match things
don't seem to work either. But then I also tried flipping that and still
things didn't work. Does that index matter? Or can the first INTF_DP be
0 still?

[PATCH v1 2/2] dt-bindings: Add SC7280 compatible string

[Sankeerth Billakanti]

The Qualcomm SC7280 platform supports an eDP controller, add
compatible string for it to msm/binding.

Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
---
 Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 64d8d9e..23b78ac 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -17,6 +17,9 @@ properties:
   compatible:
     enum:
       - qcom,sc7180-dp
+      - qcom,sc8180x-dp
+      - qcom,sc8180x-edp
+      - qcom,sc7280-edp
 
   reg:
     maxItems: 1
-- 
The Qualcomm Innovatin Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH v1 2/2] dt-bindings: Add SC7280 compatible string

[Stephen Boyd]

Quoting Sankeerth Billakanti (2021-08-11 17:08:02)
> The Qualcomm SC7280 platform supports an eDP controller, add
> compatible string for it to msm/binding.
>
> Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 64d8d9e..23b78ac 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -17,6 +17,9 @@ properties:
>    compatible:
>      enum:
>        - qcom,sc7180-dp
> +      - qcom,sc8180x-dp
> +      - qcom,sc8180x-edp
> +      - qcom,sc7280-edp

Please sort this alphabetically.

         - qcom,sc7180-dp
         - qcom,sc7280-edp
         - qcom,sc8180x-dp
         - qcom,sc8180x-edp

>
>    reg:
>      maxItems: 1

Re: [PATCH v1 2/2] dt-bindings: Add SC7280 compatible string

[Matthias Kaehlcke]

On Thu, Aug 12, 2021 at 05:38:02AM +0530, Sankeerth Billakanti wrote:
> The Qualcomm SC7280 platform supports an eDP controller, add
> compatible string for it to msm/binding.
> 
> Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 64d8d9e..23b78ac 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -17,6 +17,9 @@ properties:
>    compatible:
>      enum:
>        - qcom,sc7180-dp
> +      - qcom,sc8180x-dp
> +      - qcom,sc8180x-edp
> +      - qcom,sc7280-edp

This adds compatible strings for sc8180x and sc7280 (e)DP, however the
commit message only mentions sc7280. So either the commit message needs
and update or the sc8180x compatibles should be removed.

The driver change (https://patchwork.kernel.org/project/linux-arm-msm/patch/1628726882-27841-2-git-send-email-sbillaka@codeaurora.org/)
adds some (currently unused) 'io_start' addresses which are hardcoded,
I wonder if these should be in the device tree instead (and 'num_dp'
too?), if they are needed at all.

Re: [PATCH v1 2/2] dt-bindings: Add SC7280 compatible string

[Doug Anderson]

Hi,

On Wed, Aug 11, 2021 at 5:08 PM Sankeerth Billakanti
<sbillaka@codeaurora.org> wrote:
>
> The Qualcomm SC7280 platform supports an eDP controller, add
> compatible string for it to msm/binding.
>
> Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 3 +++
>  1 file changed, 3 insertions(+)

The ${SUBJECT} of the patch should probably be updated to say _what_
you're adding the SC7280 compatible string to. In this case, that
would be the msm dp-controller.

-Doug

Re: [PATCH v1 2/2] dt-bindings: Add SC7280 compatible string

[Stephen Boyd]

Quoting Sankeerth Billakanti (2021-08-11 17:08:02)
> The Qualcomm SC7280 platform supports an eDP controller, add
> compatible string for it to msm/binding.
>
> Signed-off-by: Sankeerth Billakanti <sbillaka@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 64d8d9e..23b78ac 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -17,6 +17,9 @@ properties:
>    compatible:
>      enum:
>        - qcom,sc7180-dp
> +      - qcom,sc8180x-dp
> +      - qcom,sc8180x-edp
> +      - qcom,sc7280-edp

Also add qcom,sc7280-dp here?