Re: [PATCH 1/5] drm/msm/dsi: add support for DSI-PHY on SM8350 and SM8450

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 11/7/2022 3:02 PM, Dmitry Baryshkov wrote:
On 07/11/2022 07:27, Abhinav Kumar wrote:


On 9/22/2022 4:30 AM, Dmitry Baryshkov wrote:
SM8350 and SM8450 use 5nm DSI PHYs, which share register definitions
with 7nm DSI PHYs. Rather than duplicating the driver, handle 5nm
variants inside the common 5+7nm driver.

I do realize that there is common code across PHYs but i am concerned about this type of unification of phy drivers.

This more or less follows downstream, which has unifier v4.0 driver.


Ack, I just observed the same. Thanks for the pointer.
Its still prone to sequencing issues though. But for now, this is okay.

If we have features which are PHY sequence dependent such as ULPS, this will just complicate things for us.

During development we initially tried to create a separate 5nm driver. However this resulted in huuuge code duplication. This would be prone to significant amount of errors if we change one of the drivers at some point and not another one.

Also some PHY registers might get added some might get removed across chipsets as this is the most frequently changed component.

Yes, I completely agree here. However beforehand we have successfully managed to have per-generation drivers, handling minor differences with quirks.


I kind of agree with Konrad's earlier comment that "QUIRK" is really not the right naming for this. Its actually different phy sub-versions.

A cleanup is needed to change or move away from quirk terminology for this.


Even in this patch, I see this added to dsi_7nm_phy_disable()

 > +    /* Turn off REFGEN Vote */
 > +        dsi_phy_write(base + REG_DSI_7nm_PHY_CMN_GLBL_DIGTOP_SPARE10, 0x0);
 > +        wmb();
 > +        /* Delay to ensure HW removes vote before PHY shut down */
 > +        udelay(2);
 > +

What would be the impact of writing this for the existing 7nm PHY?

Let's probably guard this with the v4.3 check.

hmmm, i see that in downstream also we are doing this for 7nm and 5nm PHYs. I am now wondering whether a write to 0 along with the 2 microsec delay was just missed out while writing the 7nm PHY. If so, can you move that to a separate patch instead of this one as a "fix". Adding it to this patch creates an impression that this is specific to the 5nm changes but seems like they arent.



I am having some access issues to check the software interface so wanted to check.

I don't remember having any issues on RB5, but I did not run any special checks.

When I meant access issues, I meant some permissions issue internally for accessing software register documentation not register access issues on the device :)




Co-developed-by: Robert Foss <robert.foss@xxxxxxxxxx>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
  drivers/gpu/drm/msm/Kconfig               |   6 +-
  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c     |   4 +
  drivers/gpu/drm/msm/dsi/phy/dsi_phy.h     |   2 +
  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 132 ++++++++++++++++++++--
  4 files changed, 131 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 4e0cbd682725..e6c5dfbad009 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -140,12 +140,12 @@ config DRM_MSM_DSI_10NM_PHY
        Choose this option if DSI PHY on SDM845 is used on the platform.
  config DRM_MSM_DSI_7NM_PHY
-    bool "Enable DSI 7nm PHY driver in MSM DRM"
+    bool "Enable DSI 7nm/5nm PHY driver in MSM DRM"
      depends on DRM_MSM_DSI
      default y
      help
-      Choose this option if DSI PHY on SM8150/SM8250/SC7280 is used on
-      the platform.
+      Choose this option if DSI PHY on SM8150/SM8250/SM8350/SM8450/SC7280
+      is used on the platform.
  config DRM_MSM_HDMI
      bool "Enable HDMI support in MSM DRM driver"
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 7fc0975cb869..97cf6b8b34cc 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -567,6 +567,10 @@ static const struct of_device_id dsi_phy_dt_match[] = {
        .data = &dsi_phy_7nm_8150_cfgs },
      { .compatible = "qcom,sc7280-dsi-phy-7nm",
        .data = &dsi_phy_7nm_7280_cfgs },
+    { .compatible = "qcom,dsi-phy-5nm-8350",
+      .data = &dsi_phy_5nm_8350_cfgs },
+    { .compatible = "qcom,dsi-phy-5nm-8450",
+      .data = &dsi_phy_5nm_8450_cfgs },
  #endif
      {}
  };
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index 60a99c6525b2..654cbfa14d6e 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -56,6 +56,8 @@ extern const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs;
  extern const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs;
  extern const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs;
  extern const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs;
+extern const struct msm_dsi_phy_cfg dsi_phy_5nm_8350_cfgs;
+extern const struct msm_dsi_phy_cfg dsi_phy_5nm_8450_cfgs;
  struct msm_dsi_dphy_timing {
      u32 clk_zero;
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 9e7fa7d88ead..1696ff150b9e 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -39,8 +39,14 @@
  #define VCO_REF_CLK_RATE        19200000
  #define FRAC_BITS 18
+/* Hardware is pre V4.1 */
+#define DSI_PHY_7NM_QUIRK_PRE_V4_1    BIT(0)
  /* Hardware is V4.1 */
-#define DSI_PHY_7NM_QUIRK_V4_1        BIT(0)
+#define DSI_PHY_7NM_QUIRK_V4_1        BIT(1)
+/* Hardware is V4.2 */
+#define DSI_PHY_7NM_QUIRK_V4_2        BIT(2)
+/* Hardware is V4.3 */
+#define DSI_PHY_7NM_QUIRK_V4_3        BIT(3)
  struct dsi_pll_config {
      bool enable_ssc;
@@ -116,7 +122,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_7nm *pll, struct dsi_pll_config
      dec_multiple = div_u64(pll_freq * multiplier, divider);
      dec = div_u64_rem(dec_multiple, multiplier, &frac);
-    if (!(pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1))
+    if (pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_PRE_V4_1)
          config->pll_clock_inverters = 0x28;
      else if (pll_freq <= 1000000000ULL)
          config->pll_clock_inverters = 0xa0;
@@ -197,16 +203,25 @@ static void dsi_pll_config_hzindep_reg(struct dsi_pll_7nm *pll)
      void __iomem *base = pll->phy->pll_base;
      u8 analog_controls_five_1 = 0x01, vco_config_1 = 0x00;
-    if (pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1) {
+    if (!(pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_PRE_V4_1))
          if (pll->vco_current_rate >= 3100000000ULL)
              analog_controls_five_1 = 0x03;
+    if (pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1) {
          if (pll->vco_current_rate < 1520000000ULL)
              vco_config_1 = 0x08;
          else if (pll->vco_current_rate < 2990000000ULL)
              vco_config_1 = 0x01;
      }
+    if ((pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_2) ||
+        (pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_3)) {
+        if (pll->vco_current_rate < 1520000000ULL)
+            vco_config_1 = 0x08;
+        else if (pll->vco_current_rate >= 2990000000ULL)
+            vco_config_1 = 0x01;
+    }
+
      dsi_phy_write(base + REG_DSI_7nm_PHY_PLL_ANALOG_CONTROLS_FIVE_1,
                analog_controls_five_1);
      dsi_phy_write(base + REG_DSI_7nm_PHY_PLL_VCO_CONFIG_1, vco_config_1); @@ -231,9 +246,9 @@ static void dsi_pll_config_hzindep_reg(struct dsi_pll_7nm *pll)
      dsi_phy_write(base + REG_DSI_7nm_PHY_PLL_PFILT, 0x2f);
      dsi_phy_write(base + REG_DSI_7nm_PHY_PLL_IFILT, 0x2a);
      dsi_phy_write(base + REG_DSI_7nm_PHY_PLL_IFILT,
-          pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1 ? 0x3f : 0x22); +          !(pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_PRE_V4_1) ? 0x3f : 0x22);
-    if (pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1) {
+    if (!(pll->phy->cfg->quirks & DSI_PHY_7NM_QUIRK_PRE_V4_1)) {
          dsi_phy_write(base + REG_DSI_7nm_PHY_PLL_PERF_OPTIMIZE, 0x22);
          if (pll->slave)
              dsi_phy_write(pll->slave->phy->pll_base + REG_DSI_7nm_PHY_PLL_PERF_OPTIMIZE, 0x22); @@ -788,7 +803,7 @@ static void dsi_phy_hw_v4_0_lane_settings(struct msm_dsi_phy *phy)
      const u8 *tx_dctrl = tx_dctrl_0;
      void __iomem *lane_base = phy->lane_base;
-    if (phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1)
+    if (!(phy->cfg->quirks & DSI_PHY_7NM_QUIRK_PRE_V4_1))
          tx_dctrl = tx_dctrl_1;
      /* Strength ctrl settings */
@@ -844,6 +859,12 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
      if (dsi_phy_hw_v4_0_is_pll_on(phy))
          pr_warn("PLL turned on before configuring PHY\n");
+    /* Request for REFGEN READY */
+    if (phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_3) {
+        dsi_phy_write(phy->base + REG_DSI_7nm_PHY_CMN_GLBL_DIGTOP_SPARE10, 0x1);
+        udelay(500);
+    }
+
      /* wait for REFGEN READY */
      ret = readl_poll_timeout_atomic(base + REG_DSI_7nm_PHY_CMN_PHY_STATUS,
                      status, (status & BIT(0)),
@@ -858,23 +879,53 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
      /* Alter PHY configurations if data rate less than 1.5GHZ*/
      less_than_1500_mhz = (clk_req->bitclk_rate <= 1500000000);
-    if (phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1) {
-        vreg_ctrl_0 = less_than_1500_mhz ? 0x53 : 0x52;
+    if (phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_3) {
+        if (phy->cphy_mode) {
+            vreg_ctrl_0 = 0x51;
+            glbl_rescode_top_ctrl = less_than_1500_mhz ? 0x3d :  0x01;
+            glbl_rescode_bot_ctrl = less_than_1500_mhz ? 0x38 :  0x3b;
+            glbl_str_swi_cal_sel_ctrl = 0x00;
+            glbl_hstx_str_ctrl_0 = 0x00;
+        } else {
+            vreg_ctrl_0 = less_than_1500_mhz ? 0x53 : 0x52;
+            glbl_rescode_top_ctrl = less_than_1500_mhz ? 0x3d :  0x01;
+            glbl_rescode_bot_ctrl = less_than_1500_mhz ? 0x38 :  0x39;
+            glbl_str_swi_cal_sel_ctrl = 0x00;
+            glbl_hstx_str_ctrl_0 = 0x88;
+        }
+    } else if (phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_2) {
+        if (phy->cphy_mode) {
+            vreg_ctrl_0 = 0x51;
+            glbl_rescode_top_ctrl = less_than_1500_mhz ? 0x3d :  0x01;
+            glbl_rescode_bot_ctrl = less_than_1500_mhz ? 0x38 :  0x3b;
+            glbl_str_swi_cal_sel_ctrl = 0x00;
+            glbl_hstx_str_ctrl_0 = 0x00;
+        } else {
+            vreg_ctrl_0 = less_than_1500_mhz ? 0x53 : 0x52;
+            glbl_rescode_top_ctrl = less_than_1500_mhz ? 0x3c :  0x00;
+            glbl_rescode_bot_ctrl = less_than_1500_mhz ? 0x38 :  0x39;
+            glbl_str_swi_cal_sel_ctrl = 0x00;
+            glbl_hstx_str_ctrl_0 = 0x88;
+        }
+    } else if (phy->cfg->quirks & DSI_PHY_7NM_QUIRK_V4_1) {
          if (phy->cphy_mode) {
+            vreg_ctrl_0 = 0x51;
              glbl_rescode_top_ctrl = 0x00;
              glbl_rescode_bot_ctrl = 0x3c;
          } else {
+            vreg_ctrl_0 = less_than_1500_mhz ? 0x53 : 0x52;
              glbl_rescode_top_ctrl = less_than_1500_mhz ? 0x3d :  0x00;
              glbl_rescode_bot_ctrl = less_than_1500_mhz ? 0x39 :  0x3c;
          }
          glbl_str_swi_cal_sel_ctrl = 0x00;
          glbl_hstx_str_ctrl_0 = 0x88;
      } else {
-        vreg_ctrl_0 = less_than_1500_mhz ? 0x5B : 0x59;
          if (phy->cphy_mode) {
+            vreg_ctrl_0 = 0x51;
              glbl_str_swi_cal_sel_ctrl = 0x03;
              glbl_hstx_str_ctrl_0 = 0x66;
          } else {
+            vreg_ctrl_0 = less_than_1500_mhz ? 0x5B : 0x59;
              glbl_str_swi_cal_sel_ctrl = less_than_1500_mhz ? 0x03 : 0x00;
              glbl_hstx_str_ctrl_0 = less_than_1500_mhz ? 0x66 : 0x88;
          }
@@ -883,7 +934,6 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy,
      }
      if (phy->cphy_mode) {
-        vreg_ctrl_0 = 0x51;
          vreg_ctrl_1 = 0x55;
          glbl_pemph_ctrl_0 = 0x11;
          lane_ctrl0 = 0x17;

So far this is the largest chunk to handle the difference between platforms. And I think it was a mess even before we tried adding the 4.2/4.3 versions.

A cleaner way to handle this would be to have separate _phy_enable() calls for DPHY and CPHY to clear out this mess.

Add a "phy_type" to the phy_cfg which shall decide which phy call gets executed (an idea derived from the downstream phy).

I am not sure if you want to absorb that change in this series but if you do it will be a welcome cleanup otherwise we will keep compounding the mess.


@@ -1017,6 +1067,13 @@ static void dsi_7nm_phy_disable(struct msm_dsi_phy *phy)
          pr_warn("Turning OFF PHY while PLL is on\n");
      dsi_phy_hw_v4_0_config_lpcdrx(phy, false);
+
+    /* Turn off REFGEN Vote */
+        dsi_phy_write(base + REG_DSI_7nm_PHY_CMN_GLBL_DIGTOP_SPARE10, 0x0);
+        wmb();
+        /* Delay to ensure HW removes vote before PHY shut down */
+        udelay(2);
+
      data = dsi_phy_read(base + REG_DSI_7nm_PHY_CMN_CTRL_0);
      /* disable all lanes */
@@ -1079,6 +1136,7 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_8150_cfgs = {
      .max_pll_rate = 3500000000UL,
      .io_start = { 0xae94400, 0xae96400 },
      .num_dsi_phy = 2,
+    .quirks = DSI_PHY_7NM_QUIRK_PRE_V4_1,
  };
  const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs = {
@@ -1102,3 +1160,57 @@ const struct msm_dsi_phy_cfg dsi_phy_7nm_7280_cfgs = {
      .num_dsi_phy = 1,
      .quirks = DSI_PHY_7NM_QUIRK_V4_1,
  };
+
+const struct msm_dsi_phy_cfg dsi_phy_5nm_8350_cfgs = {
+    .has_phy_lane = true,
+    .reg_cfg = {
+        .num = 1,
+        .regs = {
+            {"vdds", 37550, 0},
+        },
+    },
+    .ops = {
+        .enable = dsi_7nm_phy_enable,
+        .disable = dsi_7nm_phy_disable,
+        .pll_init = dsi_pll_7nm_init,
+        .save_pll_state = dsi_7nm_pll_save_state,
+        .restore_pll_state = dsi_7nm_pll_restore_state,
+        .set_continuous_clock = dsi_7nm_set_continuous_clock,
+    },
+    .min_pll_rate = 600000000UL,
+#ifdef CONFIG_64BIT
+    .max_pll_rate = 5000000000UL,
+#else
+    .max_pll_rate = ULONG_MAX,
+#endif
+    .io_start = { 0xae94400, 0xae96400 },
+    .num_dsi_phy = 2,
+    .quirks = DSI_PHY_7NM_QUIRK_V4_2,
+};
+
+const struct msm_dsi_phy_cfg dsi_phy_5nm_8450_cfgs = {
+    .has_phy_lane = true,
+    .reg_cfg = {
+        .num = 1,
+        .regs = {
+            {"vdds", 97800, 0},
+        },
+    },
+    .ops = {
+        .enable = dsi_7nm_phy_enable,
+        .disable = dsi_7nm_phy_disable,
+        .pll_init = dsi_pll_7nm_init,
+        .save_pll_state = dsi_7nm_pll_save_state,
+        .restore_pll_state = dsi_7nm_pll_restore_state,
+        .set_continuous_clock = dsi_7nm_set_continuous_clock,
+    },
+    .min_pll_rate = 600000000UL,
+#ifdef CONFIG_64BIT
+    .max_pll_rate = 5000000000UL,
+#else
+    .max_pll_rate = ULONG_MAX,
+#endif
+    .io_start = { 0xae94400, 0xae96400 },
+    .num_dsi_phy = 2,
+    .quirks = DSI_PHY_7NM_QUIRK_V4_3,
+};




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux