Re: [Intel-gfx] [PATCH] drm/i915/display/dg1: Correctly map DPLLs during state readout

2021-06-30 Thread Lucas De Marchi 

On Wed, Jun 30, 2021 at 02:05:22PM -0700, Jose Souza wrote:

_DG1_DPCLKA0_CFGCR0 maps between DPLL 0 and 1 with one bit for phy A
and B while _DG1_DPCLKA1_CFGCR0 maps between DPLL 2 and 3 with one
bit for phy C and D.

Reusing _cnl_ddi_get_pll() don't take that into cosideration returing
DPLL 0 and 1 for phy C and D.

That is a regression introduced in the refactor done in commit
c9fdceaa64dc ("drm/i915: Move DDI clock readout to encoder->get_config()").
While at it also dropping the macros previously used, not reusing it
to improve readability.

BSpec: 50286
Fixes: c9fdceaa64dc ("drm/i915: Move DDI clock readout to 
encoder->get_config()")
Cc: Lucas De Marchi 
Cc: Ville Syrjälä 
Signed-off-by: José Roberto de Souza 



Reviewed-by: Lucas De Marchi 

thanks
Lucas De Marchi


---
drivers/gpu/drm/i915/display/intel_ddi.c | 19 ---
drivers/gpu/drm/i915/i915_reg.h  |  3 ---
2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
b/drivers/gpu/drm/i915/display/intel_ddi.c
index 91fd85bee1d27..26a3aa73fcc43 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1738,10 +1738,23 @@ static struct intel_shared_dpll *dg1_ddi_get_pll(struct 
intel_encoder *encoder)
{
struct drm_i915_private *i915 = to_i915(encoder->base.dev);
enum phy phy = intel_port_to_phy(i915, encoder->port);
+   enum intel_dpll_id id;
+   u32 val;

-   return _cnl_ddi_get_pll(i915, DG1_DPCLKA_CFGCR0(phy),
-   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy),
-   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy));
+   val = intel_de_read(i915, DG1_DPCLKA_CFGCR0(phy));
+   val &= DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
+   val >>= DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy);
+   id = val;
+
+   /*
+* _DG1_DPCLKA0_CFGCR0 maps between DPLL 0 and 1 with one bit for phy A
+* and B while _DG1_DPCLKA1_CFGCR0 maps between DPLL 2 and 3 with one
+* bit for phy C and D.
+*/
+   if (phy >= PHY_C)
+   id += DPLL_ID_DG1_DPLL2;
+
+   return intel_get_shared_dpll_by_id(i915, id);
}

static void icl_ddi_combo_enable_clock(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 65c155b141899..16a19239d86dd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10525,7 +10525,6 @@ enum skl_power_gate {
#define _DG1_DPCLKA1_CFGCR0 0x16C280
#define _DG1_DPCLKA_PHY_IDX(phy)((phy) % 2)
#define _DG1_DPCLKA_PLL_IDX(pll)((pll) % 2)
-#define _DG1_PHY_DPLL_MAP(phy) ((phy) >= PHY_C ? 
DPLL_ID_DG1_DPLL2 : DPLL_ID_DG1_DPLL0)
#define DG1_DPCLKA_CFGCR0(phy)  _MMIO_PHY((phy) / 2, \
  
_DG1_DPCLKA_CFGCR0, \
  
_DG1_DPCLKA1_CFGCR0)
@@ -10533,8 +10532,6 @@ enum skl_power_gate {
#define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy)  
(_DG1_DPCLKA_PHY_IDX(phy) * 2)
#define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL(pll, phy)   (_DG1_DPCLKA_PLL_IDX(pll) 
<< DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
#define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy)   (0x3 << 
DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
-#define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_DPLL_MAP(clk_sel, phy) \
-   (((clk_sel) >> DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy)) + 
_DG1_PHY_DPLL_MAP(phy))

/* ADLS Clocks */
#define _ADLS_DPCLKA_CFGCR0 0x164280
--
2.32.0


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/display/dg1: Correctly map DPLLs during state readout

2021-06-30 Thread José Roberto de Souza 
_DG1_DPCLKA0_CFGCR0 maps between DPLL 0 and 1 with one bit for phy A
and B while _DG1_DPCLKA1_CFGCR0 maps between DPLL 2 and 3 with one
bit for phy C and D.

Reusing _cnl_ddi_get_pll() don't take that into cosideration returing
DPLL 0 and 1 for phy C and D.

That is a regression introduced in the refactor done in commit
c9fdceaa64dc ("drm/i915: Move DDI clock readout to encoder->get_config()").
While at it also dropping the macros previously used, not reusing it
to improve readability.

BSpec: 50286
Fixes: c9fdceaa64dc ("drm/i915: Move DDI clock readout to 
encoder->get_config()")
Cc: Lucas De Marchi 
Cc: Ville Syrjälä 
Signed-off-by: José Roberto de Souza 
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 19 ---
 drivers/gpu/drm/i915/i915_reg.h  |  3 ---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
b/drivers/gpu/drm/i915/display/intel_ddi.c
index 91fd85bee1d27..26a3aa73fcc43 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -1738,10 +1738,23 @@ static struct intel_shared_dpll *dg1_ddi_get_pll(struct 
intel_encoder *encoder)
 {
struct drm_i915_private *i915 = to_i915(encoder->base.dev);
enum phy phy = intel_port_to_phy(i915, encoder->port);
+   enum intel_dpll_id id;
+   u32 val;
 
-   return _cnl_ddi_get_pll(i915, DG1_DPCLKA_CFGCR0(phy),
-   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy),
-   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy));
+   val = intel_de_read(i915, DG1_DPCLKA_CFGCR0(phy));
+   val &= DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
+   val >>= DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy);
+   id = val;
+
+   /*
+* _DG1_DPCLKA0_CFGCR0 maps between DPLL 0 and 1 with one bit for phy A
+* and B while _DG1_DPCLKA1_CFGCR0 maps between DPLL 2 and 3 with one
+* bit for phy C and D.
+*/
+   if (phy >= PHY_C)
+   id += DPLL_ID_DG1_DPLL2;
+
+   return intel_get_shared_dpll_by_id(i915, id);
 }
 
 static void icl_ddi_combo_enable_clock(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 65c155b141899..16a19239d86dd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10525,7 +10525,6 @@ enum skl_power_gate {
 #define _DG1_DPCLKA1_CFGCR00x16C280
 #define _DG1_DPCLKA_PHY_IDX(phy)   ((phy) % 2)
 #define _DG1_DPCLKA_PLL_IDX(pll)   ((pll) % 2)
-#define _DG1_PHY_DPLL_MAP(phy) ((phy) >= PHY_C ? 
DPLL_ID_DG1_DPLL2 : DPLL_ID_DG1_DPLL0)
 #define DG1_DPCLKA_CFGCR0(phy) _MMIO_PHY((phy) / 2, \
  
_DG1_DPCLKA_CFGCR0, \
  
_DG1_DPCLKA1_CFGCR0)
@@ -10533,8 +10532,6 @@ enum skl_power_gate {
 #define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy) 
(_DG1_DPCLKA_PHY_IDX(phy) * 2)
 #define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL(pll, phy)  
(_DG1_DPCLKA_PLL_IDX(pll) << DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
 #define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy)  (0x3 << 
DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
-#define   DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_DPLL_MAP(clk_sel, phy) \
-   (((clk_sel) >> DG1_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy)) + 
_DG1_PHY_DPLL_MAP(phy))
 
 /* ADLS Clocks */
 #define _ADLS_DPCLKA_CFGCR00x164280
-- 
2.32.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx