[v1 0/2] drm/msm/dsi: Add 10nm dsi phy tuning configuration support

[v1 0/2] drm/msm/dsi: Add 10nm dsi phy tuning configuration support

[Rajeev Nandan]

This series is to add DSI PHY tuning support in Qualcomm Snapdragon
SoCs with 10nm DSI PHY e.g. SC7180

In most cases the default values of DSI PHY tuning registers
should be sufficient as they are fully optimized. However, in
some cases (for example, where extreme board parasitics cause
the eye shape to degrade), the override bits can be used to
improve the signal quality.

As per the MSM DSI PHY (10nm) tuning guideline, the drive strength
can be adjusted using DSIPHY_RESCODE_OFFSET_TOP & DSIPHY_RESCODE_OFFSET_BOT
registers, and the drive level can be adjusted using DSIPHY_CMN_VREG_CTRL
register.

Add DSI PHY tuning support for 10nm PHY. This can be extended to other
DSI PHY versions if needed. Number of registers to configure the PHY
tuning per lane can be different for different versions of the DSI PHY.
I tried to make it generic so that it can be extended to other versions.

Rajeev Nandan (2):
  dt-bindings: msm/dsi: Add 10nm dsi phy tuning properties
  drm/msm/dsi: Add 10nm dsi phy tuning configuration support

 .../bindings/display/msm/dsi-phy-10nm.yaml         | 19 ++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c              | 55 ++++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h              | 23 +++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c         | 31 +++++++++---
 4 files changed, 122 insertions(+), 6 deletions(-)

-- 
2.7.4

[v1 1/2] dt-bindings: msm/dsi: Add 10nm dsi phy tuning properties

[Rajeev Nandan]

Add 10nm dsi phy tuning properties for phy drive strength and
phy drive level adjustemnt.

Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com>
---
 .../devicetree/bindings/display/msm/dsi-phy-10nm.yaml | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
index 4399715..9406982 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
@@ -35,6 +35,18 @@ properties:
       Connected to DSI0_MIPI_DSI_PLL_VDDA0P9 pin for sc7180 target and
       connected to VDDA_MIPI_DSI_0_PLL_0P9 pin for sdm845 target
 
+  phy-drive-strength-cfg:
+    type: array
+    description:
+      Register values of DSIPHY_RESCODE_OFFSET_TOP and DSIPHY_RESCODE_OFFSET_BOT
+      for all five lanes to adjust the phy drive strength.
+
+  phy-drive-level-cfg:
+    type: array
+    description:
+      Register values of DSIPHY_RESCODE_OFFSET_TOP for all five lanes to adjust
+      phy drive level/amplitude.
+
 required:
   - compatible
   - reg
@@ -64,5 +76,12 @@ examples:
          clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
                   <&rpmhcc RPMH_CXO_CLK>;
          clock-names = "iface", "ref";
+
+         phy-drive-strength-cfg = [00 00
+                                   00 00
+                                   00 00
+                                   00 00
+                                   00 00];
+         phy-drive-level-cfg = [59 59 59 59 59];
      };
 ...
-- 
2.7.4

[v1 2/2] drm/msm/dsi: Add 10nm dsi phy tuning configuration support

[Rajeev Nandan]

In most cases the default values of DSI PHY tuning registers
should be sufficient as they are fully optimized. However, in
some cases (for example, where extreme board parasitics cause
the eye shape to degrade), the override bits can be used to
improve the signal quality.

As per the MSM DSI PHY (10nm) tuning guideline, the drive strength
can be adjusted using DSIPHY_RESCODE_OFFSET_TOP & DSIPHY_RESCODE_OFFSET_BOT
registers, and the drive level can be adjusted using DSIPHY_CMN_VREG_CTRL
register.

Add DSI PHY tuning support for 10nm PHY. This can be extended to other
DSI PHY versions if needed.

Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c      | 55 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h      | 23 +++++++++++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 31 +++++++++++++----
 3 files changed, 103 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 8c65ef6..bf630b7 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -669,10 +669,42 @@ static int dsi_phy_get_id(struct msm_dsi_phy *phy)
 	return -EINVAL;
 }
 
+static int dsi_phy_parse_dt_per_lane_cfgs(struct platform_device *pdev,
+					  struct dsi_phy_per_lane_cfgs *cfg,
+					  char *property)
+{
+	int i = 0, j = 0;
+	const u8 *data;
+	u32 len = 0;
+
+	data = of_get_property(pdev->dev.of_node, property, &len);
+	if (!data) {
+		DRM_DEV_ERROR(&pdev->dev, "couldn't find %s property\n", property);
+		return -EINVAL;
+	}
+
+	if (len != DSI_LANE_MAX * cfg->count_per_lane) {
+		DRM_DEV_ERROR(&pdev->dev, "incorrect phy %s settings, exp=%d, act=%d\n",
+		       property, (DSI_LANE_MAX * cfg->count_per_lane), len);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < DSI_LANE_MAX; i++) {
+		for (j = 0; j < cfg->count_per_lane; j++) {
+			cfg->val[i][j] = *data;
+			data++;
+		}
+	}
+
+	return 0;
+}
+
 static int dsi_phy_driver_probe(struct platform_device *pdev)
 {
 	struct msm_dsi_phy *phy;
 	struct device *dev = &pdev->dev;
+	struct dsi_phy_per_lane_cfgs *strength;
+	struct dsi_phy_per_lane_cfgs *level;
 	u32 phy_type;
 	int ret;
 
@@ -707,6 +739,29 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 	if (!of_property_read_u32(dev->of_node, "phy-type", &phy_type))
 		phy->cphy_mode = (phy_type == PHY_TYPE_CPHY);
 
+	/* dsi phy tuning configurations */
+	if (phy->cfg->drive_strength_cfg_count) {
+		strength = &phy->tuning_cfg.drive_strength;
+		strength->count_per_lane = phy->cfg->drive_strength_cfg_count;
+		ret = dsi_phy_parse_dt_per_lane_cfgs(pdev, strength,
+						"phy-drive-strength-cfg");
+		if (ret) {
+			DRM_DEV_ERROR(dev, "failed to parse PHY drive strength cfg, %d\n", ret);
+			goto fail;
+		}
+	}
+
+	if (phy->cfg->drive_level_cfg_count) {
+		level = &phy->tuning_cfg.drive_level;
+		level->count_per_lane = phy->cfg->drive_level_cfg_count;
+		ret = dsi_phy_parse_dt_per_lane_cfgs(pdev, level,
+						"phy-drive-level-cfg");
+		if (ret) {
+			DRM_DEV_ERROR(dev, "failed to parse PHY drive level cfg, %d\n", ret);
+			goto fail;
+		}
+	}
+
 	phy->base = msm_ioremap_size(pdev, "dsi_phy", "DSI_PHY", &phy->base_size);
 	if (IS_ERR(phy->base)) {
 		DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index b91303a..9ff733a 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -39,6 +39,10 @@ struct msm_dsi_phy_cfg {
 	const int quirks;
 	bool has_phy_regulator;
 	bool has_phy_lane;
+
+	/* phy tuning config counts per lane */
+	u32 drive_strength_cfg_count;
+	u32 drive_level_cfg_count;
 };
 
 extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs;
@@ -81,6 +85,24 @@ struct msm_dsi_dphy_timing {
 #define DSI_PIXEL_PLL_CLK		1
 #define NUM_PROVIDED_CLKS		2
 
+#define DSI_LANE_MAX			5
+#define DSI_MAX_SETTINGS		8
+
+/**
+ * struct dsi_phy_per_lane_cfgs - Holds register values for PHY parameters
+ * @val: Register values for all lanes
+ * @count_per_lane: Number of values per lane.
+ */
+struct dsi_phy_per_lane_cfgs {
+	u8 val[DSI_LANE_MAX][DSI_MAX_SETTINGS];
+	u32 count_per_lane;
+};
+
+struct msm_dsi_phy_tuning_cfg {
+	struct dsi_phy_per_lane_cfgs drive_strength;
+	struct dsi_phy_per_lane_cfgs drive_level;
+};
+
 struct msm_dsi_phy {
 	struct platform_device *pdev;
 	void __iomem *base;
@@ -98,6 +120,7 @@ struct msm_dsi_phy {
 
 	struct msm_dsi_dphy_timing timing;
 	const struct msm_dsi_phy_cfg *cfg;
+	struct msm_dsi_phy_tuning_cfg tuning_cfg;
 
 	enum msm_dsi_phy_usecase usecase;
 	bool regulator_ldo_mode;
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index d8128f5..ac974c06 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -775,10 +775,20 @@ static void dsi_phy_hw_v3_0_lane_settings(struct msm_dsi_phy *phy)
 		dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_CFG2(i), 0x0);
 		dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_CFG3(i),
 			      i == 4 ? 0x80 : 0x0);
-		dsi_phy_write(lane_base +
-			      REG_DSI_10nm_PHY_LN_OFFSET_TOP_CTRL(i), 0x0);
-		dsi_phy_write(lane_base +
-			      REG_DSI_10nm_PHY_LN_OFFSET_BOT_CTRL(i), 0x0);
+
+		/* platform specific dsi phy drive strength adjustment */
+		if (phy->cfg->drive_strength_cfg_count) {
+			dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_OFFSET_TOP_CTRL(i),
+				phy->tuning_cfg.drive_strength.val[i][0]);
+			dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_OFFSET_BOT_CTRL(i),
+				phy->tuning_cfg.drive_strength.val[i][1]);
+		} else {
+			dsi_phy_write(lane_base +
+				      REG_DSI_10nm_PHY_LN_OFFSET_TOP_CTRL(i), 0x0);
+			dsi_phy_write(lane_base +
+				      REG_DSI_10nm_PHY_LN_OFFSET_BOT_CTRL(i), 0x0);
+		}
+
 		dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(i),
 			      tx_dctrl[i]);
 	}
@@ -834,8 +844,13 @@ static int dsi_10nm_phy_enable(struct msm_dsi_phy *phy,
 	/* Select MS1 byte-clk */
 	dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_GLBL_CTRL, 0x10);
 
-	/* Enable LDO */
-	dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_VREG_CTRL, 0x59);
+	/* Enable LDO with platform specific drive level/amplitude adjustment */
+	if (phy->cfg->drive_level_cfg_count) {
+		dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_VREG_CTRL,
+			phy->tuning_cfg.drive_level.val[0][0]);
+	} else {
+		dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_VREG_CTRL, 0x59);
+	}
 
 	/* Configure PHY lane swap (TODO: we need to calculate this) */
 	dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_LANE_CFG0, 0x21);
@@ -941,6 +956,8 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
 	.max_pll_rate = 3500000000UL,
 	.io_start = { 0xae94400, 0xae96400 },
 	.num_dsi_phy = 2,
+	.drive_strength_cfg_count = 2,
+	.drive_level_cfg_count = 1,
 };
 
 const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
@@ -963,4 +980,6 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
 	.io_start = { 0xc994400, 0xc996400 },
 	.num_dsi_phy = 2,
 	.quirks = DSI_PHY_10NM_QUIRK_OLD_TIMINGS,
+	.drive_strength_cfg_count = 2,
+	.drive_level_cfg_count = 1,
 };
-- 
2.7.4

Re: [v1 1/2] dt-bindings: msm/dsi: Add 10nm dsi phy tuning properties

[Dmitry Baryshkov]

On Thu, 30 Dec 2021 at 12:25, Rajeev Nandan <quic_rajeevny@quicinc.com> wrote:
>
> Add 10nm dsi phy tuning properties for phy drive strength and
> phy drive level adjustemnt.
>
> Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com>
> ---
>  .../devicetree/bindings/display/msm/dsi-phy-10nm.yaml | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> index 4399715..9406982 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> @@ -35,6 +35,18 @@ properties:
>        Connected to DSI0_MIPI_DSI_PLL_VDDA0P9 pin for sc7180 target and
>        connected to VDDA_MIPI_DSI_0_PLL_0P9 pin for sdm845 target
>
> +  phy-drive-strength-cfg:
> +    type: array
> +    description:
> +      Register values of DSIPHY_RESCODE_OFFSET_TOP and DSIPHY_RESCODE_OFFSET_BOT
> +      for all five lanes to adjust the phy drive strength.
> +
> +  phy-drive-level-cfg:
> +    type: array
> +    description:
> +      Register values of DSIPHY_RESCODE_OFFSET_TOP for all five lanes to adjust
> +      phy drive level/amplitude.


Description is incorrect, it's not the RESCODE_OFFSET_TOP register.

> +
>  required:
>    - compatible
>    - reg
> @@ -64,5 +76,12 @@ examples:
>           clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>                    <&rpmhcc RPMH_CXO_CLK>;
>           clock-names = "iface", "ref";
> +
> +         phy-drive-strength-cfg = [00 00
> +                                   00 00
> +                                   00 00
> +                                   00 00
> +                                   00 00];
> +         phy-drive-level-cfg = [59 59 59 59 59];

You are writing this value into the PHY_CMN_VREG_CTRL register. So
specifying 5 values here does not make sense.

>       };
>  ...
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [v1 1/2] dt-bindings: msm/dsi: Add 10nm dsi phy tuning properties

[Dmitry Baryshkov]

On Thu, 30 Dec 2021 at 12:25, Rajeev Nandan <quic_rajeevny@quicinc.com> wrote:
>
> Add 10nm dsi phy tuning properties for phy drive strength and
> phy drive level adjustemnt.
>
> Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com>
> ---
>  .../devicetree/bindings/display/msm/dsi-phy-10nm.yaml | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> index 4399715..9406982 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> @@ -35,6 +35,18 @@ properties:
>        Connected to DSI0_MIPI_DSI_PLL_VDDA0P9 pin for sc7180 target and
>        connected to VDDA_MIPI_DSI_0_PLL_0P9 pin for sdm845 target
>
> +  phy-drive-strength-cfg:
> +    type: array
> +    description:
> +      Register values of DSIPHY_RESCODE_OFFSET_TOP and DSIPHY_RESCODE_OFFSET_BOT
> +      for all five lanes to adjust the phy drive strength.
> +
> +  phy-drive-level-cfg:
> +    type: array
> +    description:
> +      Register values of DSIPHY_RESCODE_OFFSET_TOP for all five lanes to adjust
> +      phy drive level/amplitude.
> +
>  required:
>    - compatible
>    - reg
> @@ -64,5 +76,12 @@ examples:
>           clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>                    <&rpmhcc RPMH_CXO_CLK>;
>           clock-names = "iface", "ref";
> +
> +         phy-drive-strength-cfg = [00 00
> +                                   00 00
> +                                   00 00
> +                                   00 00
> +                                   00 00];
> +         phy-drive-level-cfg = [59 59 59 59 59];

And second notice. This interface seems to be too register-centric.
You provide register values without any actual way to interpret them.
I'd prefer to have something closer to pinctrl. Specify strength and
level in some logical way and then in the driver interpret that into
register values.

-- 
With best wishes
Dmitry

Re: [v1 2/2] drm/msm/dsi: Add 10nm dsi phy tuning configuration support

[Dmitry Baryshkov]

On Thu, 30 Dec 2021 at 12:25, Rajeev Nandan <quic_rajeevny@quicinc.com> wrote:
>
> In most cases the default values of DSI PHY tuning registers
> should be sufficient as they are fully optimized. However, in
> some cases (for example, where extreme board parasitics cause
> the eye shape to degrade), the override bits can be used to
> improve the signal quality.
>
> As per the MSM DSI PHY (10nm) tuning guideline, the drive strength
> can be adjusted using DSIPHY_RESCODE_OFFSET_TOP & DSIPHY_RESCODE_OFFSET_BOT
> registers, and the drive level can be adjusted using DSIPHY_CMN_VREG_CTRL
> register.
>
> Add DSI PHY tuning support for 10nm PHY. This can be extended to other
> DSI PHY versions if needed.

Could you please split this patch into generic code and 10nm-specific part?

>
> Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c      | 55 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy.h      | 23 +++++++++++++
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 31 +++++++++++++----
>  3 files changed, 103 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 8c65ef6..bf630b7 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -669,10 +669,42 @@ static int dsi_phy_get_id(struct msm_dsi_phy *phy)
>         return -EINVAL;
>  }
>
> +static int dsi_phy_parse_dt_per_lane_cfgs(struct platform_device *pdev,
> +                                         struct dsi_phy_per_lane_cfgs *cfg,
> +                                         char *property)
> +{
> +       int i = 0, j = 0;
> +       const u8 *data;
> +       u32 len = 0;
> +
> +       data = of_get_property(pdev->dev.of_node, property, &len);
> +       if (!data) {
> +               DRM_DEV_ERROR(&pdev->dev, "couldn't find %s property\n", property);
> +               return -EINVAL;
> +       }
> +
> +       if (len != DSI_LANE_MAX * cfg->count_per_lane) {
> +               DRM_DEV_ERROR(&pdev->dev, "incorrect phy %s settings, exp=%d, act=%d\n",
> +                      property, (DSI_LANE_MAX * cfg->count_per_lane), len);
> +               return -EINVAL;
> +       }
> +
> +       for (i = 0; i < DSI_LANE_MAX; i++) {
> +               for (j = 0; j < cfg->count_per_lane; j++) {
> +                       cfg->val[i][j] = *data;
> +                       data++;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int dsi_phy_driver_probe(struct platform_device *pdev)
>  {
>         struct msm_dsi_phy *phy;
>         struct device *dev = &pdev->dev;
> +       struct dsi_phy_per_lane_cfgs *strength;
> +       struct dsi_phy_per_lane_cfgs *level;
>         u32 phy_type;
>         int ret;
>
> @@ -707,6 +739,29 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>         if (!of_property_read_u32(dev->of_node, "phy-type", &phy_type))
>                 phy->cphy_mode = (phy_type == PHY_TYPE_CPHY);
>
> +       /* dsi phy tuning configurations */
> +       if (phy->cfg->drive_strength_cfg_count) {
> +               strength = &phy->tuning_cfg.drive_strength;
> +               strength->count_per_lane = phy->cfg->drive_strength_cfg_count;
> +               ret = dsi_phy_parse_dt_per_lane_cfgs(pdev, strength,
> +                                               "phy-drive-strength-cfg");
> +               if (ret) {
> +                       DRM_DEV_ERROR(dev, "failed to parse PHY drive strength cfg, %d\n", ret);
> +                       goto fail;
> +               }
> +       }
> +
> +       if (phy->cfg->drive_level_cfg_count) {
> +               level = &phy->tuning_cfg.drive_level;
> +               level->count_per_lane = phy->cfg->drive_level_cfg_count;
> +               ret = dsi_phy_parse_dt_per_lane_cfgs(pdev, level,
> +                                               "phy-drive-level-cfg");
> +               if (ret) {
> +                       DRM_DEV_ERROR(dev, "failed to parse PHY drive level cfg, %d\n", ret);
> +                       goto fail;
> +               }
> +       }

After reading the code (and the way you use those values later), I'd
suggest adding the parse_dt hook instead of parsing it from the
generic code and putting the values into phy-specific data instead.
This way in the parse_dt you can read, validate and prepare register
values that are going to be written into the hardware. Then in the
phy_enable you can write those values directly, without any ifs.

> +
>         phy->base = msm_ioremap_size(pdev, "dsi_phy", "DSI_PHY", &phy->base_size);
>         if (IS_ERR(phy->base)) {
>                 DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> index b91303a..9ff733a 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> @@ -39,6 +39,10 @@ struct msm_dsi_phy_cfg {
>         const int quirks;
>         bool has_phy_regulator;
>         bool has_phy_lane;
> +
> +       /* phy tuning config counts per lane */
> +       u32 drive_strength_cfg_count;
> +       u32 drive_level_cfg_count;
>  };
>
>  extern const struct msm_dsi_phy_cfg dsi_phy_28nm_hpm_cfgs;
> @@ -81,6 +85,24 @@ struct msm_dsi_dphy_timing {
>  #define DSI_PIXEL_PLL_CLK              1
>  #define NUM_PROVIDED_CLKS              2
>
> +#define DSI_LANE_MAX                   5
> +#define DSI_MAX_SETTINGS               8
> +
> +/**
> + * struct dsi_phy_per_lane_cfgs - Holds register values for PHY parameters
> + * @val: Register values for all lanes
> + * @count_per_lane: Number of values per lane.
> + */
> +struct dsi_phy_per_lane_cfgs {
> +       u8 val[DSI_LANE_MAX][DSI_MAX_SETTINGS];
> +       u32 count_per_lane;
> +};
> +
> +struct msm_dsi_phy_tuning_cfg {
> +       struct dsi_phy_per_lane_cfgs drive_strength;
> +       struct dsi_phy_per_lane_cfgs drive_level;
> +};
> +
>  struct msm_dsi_phy {
>         struct platform_device *pdev;
>         void __iomem *base;
> @@ -98,6 +120,7 @@ struct msm_dsi_phy {
>
>         struct msm_dsi_dphy_timing timing;
>         const struct msm_dsi_phy_cfg *cfg;
> +       struct msm_dsi_phy_tuning_cfg tuning_cfg;
>
>         enum msm_dsi_phy_usecase usecase;
>         bool regulator_ldo_mode;
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> index d8128f5..ac974c06 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
> @@ -775,10 +775,20 @@ static void dsi_phy_hw_v3_0_lane_settings(struct msm_dsi_phy *phy)
>                 dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_CFG2(i), 0x0);
>                 dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_CFG3(i),
>                               i == 4 ? 0x80 : 0x0);
> -               dsi_phy_write(lane_base +
> -                             REG_DSI_10nm_PHY_LN_OFFSET_TOP_CTRL(i), 0x0);
> -               dsi_phy_write(lane_base +
> -                             REG_DSI_10nm_PHY_LN_OFFSET_BOT_CTRL(i), 0x0);
> +
> +               /* platform specific dsi phy drive strength adjustment */
> +               if (phy->cfg->drive_strength_cfg_count) {

Something is not correct here. You are checking the
phy->cfg->drive_strength_cfg_count (which should be always set for 10
nm), but then you are writing the values without checking if they were
provided or not.

> +                       dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_OFFSET_TOP_CTRL(i),
> +                               phy->tuning_cfg.drive_strength.val[i][0]);
> +                       dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_OFFSET_BOT_CTRL(i),
> +                               phy->tuning_cfg.drive_strength.val[i][1]);
> +               } else {
> +                       dsi_phy_write(lane_base +
> +                                     REG_DSI_10nm_PHY_LN_OFFSET_TOP_CTRL(i), 0x0);
> +                       dsi_phy_write(lane_base +
> +                                     REG_DSI_10nm_PHY_LN_OFFSET_BOT_CTRL(i), 0x0);
> +               }
> +
>                 dsi_phy_write(lane_base + REG_DSI_10nm_PHY_LN_TX_DCTRL(i),
>                               tx_dctrl[i]);
>         }
> @@ -834,8 +844,13 @@ static int dsi_10nm_phy_enable(struct msm_dsi_phy *phy,
>         /* Select MS1 byte-clk */
>         dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_GLBL_CTRL, 0x10);
>
> -       /* Enable LDO */
> -       dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_VREG_CTRL, 0x59);
> +       /* Enable LDO with platform specific drive level/amplitude adjustment */
> +       if (phy->cfg->drive_level_cfg_count) {
> +               dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_VREG_CTRL,
> +                       phy->tuning_cfg.drive_level.val[0][0]);
> +       } else {
> +               dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_VREG_CTRL, 0x59);
> +       }

And this is even worse. If the drive_level_cfg_count is set, but the
values were not provided in the DTS, you end up writing zero to the
register, thus breaking backwards compatibility.

>
>         /* Configure PHY lane swap (TODO: we need to calculate this) */
>         dsi_phy_write(base + REG_DSI_10nm_PHY_CMN_LANE_CFG0, 0x21);
> @@ -941,6 +956,8 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs = {
>         .max_pll_rate = 3500000000UL,
>         .io_start = { 0xae94400, 0xae96400 },
>         .num_dsi_phy = 2,
> +       .drive_strength_cfg_count = 2,
> +       .drive_level_cfg_count = 1,
>  };
>
>  const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
> @@ -963,4 +980,6 @@ const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs = {
>         .io_start = { 0xc994400, 0xc996400 },
>         .num_dsi_phy = 2,
>         .quirks = DSI_PHY_10NM_QUIRK_OLD_TIMINGS,
> +       .drive_strength_cfg_count = 2,
> +       .drive_level_cfg_count = 1,
>  };
> --
> 2.7.4
>


--
With best wishes

Dmitry

Re: [v1 1/2] dt-bindings: msm/dsi: Add 10nm dsi phy tuning properties

[Rob Herring]

On Thu, 30 Dec 2021 14:54:35 +0530, Rajeev Nandan wrote:
> Add 10nm dsi phy tuning properties for phy drive strength and
> phy drive level adjustemnt.
> 
> Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com>
> ---
>  .../devicetree/bindings/display/msm/dsi-phy-10nm.yaml | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml: properties:phy-drive-strength-cfg:type: 'array' is not one of ['boolean', 'object']
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml: properties:phy-drive-level-cfg:type: 'array' is not one of ['boolean', 'object']
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml: ignoring, error in schema: properties: phy-drive-strength-cfg: type
Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.example.dt.yaml:0:0: /example-0/dsi-phy@ae94400: failed to match any schema with compatible: ['qcom,dsi-phy-10nm']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1574124

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [v1 1/2] dt-bindings: msm/dsi: Add 10nm dsi phy tuning properties

[Stephen Boyd]

Quoting Rajeev Nandan (2021-12-30 01:24:35)
> Add 10nm dsi phy tuning properties for phy drive strength and
> phy drive level adjustemnt.

s/adjustemnt/adjustment/

Please add the details about parasitics and eye shape tuning to this
commit text.

>
> Signed-off-by: Rajeev Nandan <quic_rajeevny@quicinc.com>
> ---
>  .../devicetree/bindings/display/msm/dsi-phy-10nm.yaml | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> index 4399715..9406982 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> @@ -35,6 +35,18 @@ properties:
>        Connected to DSI0_MIPI_DSI_PLL_VDDA0P9 pin for sc7180 target and
>        connected to VDDA_MIPI_DSI_0_PLL_0P9 pin for sdm845 target
>
> +  phy-drive-strength-cfg:
> +    type: array
> +    description:
> +      Register values of DSIPHY_RESCODE_OFFSET_TOP and DSIPHY_RESCODE_OFFSET_BOT
> +      for all five lanes to adjust the phy drive strength.
> +
> +  phy-drive-level-cfg:
> +    type: array
> +    description:
> +      Register values of DSIPHY_RESCODE_OFFSET_TOP for all five lanes to adjust
> +      phy drive level/amplitude.

It would be better to put human understandable values into DT here. This
looks like a black box to anyone outside of qcom, so they won't know how
to tune or set these register values.

At least for phy-drive-level-cfg it sounds like it could be some sort of
property that is a u32 array of 5 elements for each lane indicating some
sort of amplitude, i.e.

	phy-max-amplitudes = <0 1 2 3 4>;
	phy-min-amplitudes = <0 1 2 3 4>;

where each index corresponds to a particular lane. Then the driver can
parse the amplitude and convert it into some sort of register value.