| Message ID | 20200929002929.783620-2-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tgl: Fix Combo PHY DPLL fractional divider for 38.4MHz ref clock | expand |
On Tue, Sep 29, 2020 at 03:29:25AM +0300, Imre Deak wrote: > The BIOS of at least one ASUS-Z170M system with an SKL I have programs > the 101b WRPLL PDIV divider value, which is the encoding for PDIV=7 with > bit#0 incorrectly set. > > This happens with the > > "3840x2160": 30 262750 3840 3888 3920 4000 2160 2163 2168 2191 0x48 0x9 > > HDMI mode (scaled from a 1024x768 src fb) set by BIOS and the > > ref_clock=24000, dco_integer=383, dco_fraction=5802, pdiv=7, qdiv=1, kdiv=1 > > WRPLL parameters (assuming PDIV=7 was the intended setting). This > corresponds to 262749 PLL frequency/port clock. > > Later the driver sets the same mode for which it calculates the same > dco_int/dco_frac/div WRPLL parameters (with the correct PDIV=7 encoding). > > Based on the above, let's assume that PDIV=7 was intended and the HW > just ignores bit#0 in the PDIV register field for this setting, treating > 100b and 101b encodings the same way. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 8 ++++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > index e08684e34078..095b53fe3a21 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > @@ -1602,6 +1602,14 @@ static int skl_ddi_wrpll_get_freq(struct drm_i915_private *i915, > case DPLL_CFGCR2_PDIV_3: > p0 = 3; > break; > + case DPLL_CFGCR2_PDIV_7 | (1 << DPLL_CFGCR2_PDIV_SHIFT): Maybe we want a define for this? > + /* > + * Incorrect ASUS-Z170M BIOS setting, the HW seems to ignore bit#0, > + * handling it the same way as PDIV_7. > + */ > + drm_err(&i915->drm, "Invalid WRPLL PDIV divider value, fixing it.\n"); I wonder how many bug reports that will generate. Might want to make it debug insteead. > + p0 = 7; > + break; Or maybe fallthrough? > case DPLL_CFGCR2_PDIV_7: > p0 = 7; > break; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 47730a176698..f70e45bd3810 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -10253,6 +10253,7 @@ enum skl_power_gate { > #define DPLL_CFGCR2_KDIV_3 (2 << 5) > #define DPLL_CFGCR2_KDIV_1 (3 << 5) > #define DPLL_CFGCR2_PDIV_MASK (7 << 2) > +#define DPLL_CFGCR2_PDIV_SHIFT 2 > #define DPLL_CFGCR2_PDIV(x) ((x) << 2) > #define DPLL_CFGCR2_PDIV_1 (0 << 2) > #define DPLL_CFGCR2_PDIV_2 (1 << 2) > -- > 2.25.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 01, 2020 at 07:41:48PM +0300, Ville Syrjälä wrote: > On Tue, Sep 29, 2020 at 03:29:25AM +0300, Imre Deak wrote: > > The BIOS of at least one ASUS-Z170M system with an SKL I have programs > > the 101b WRPLL PDIV divider value, which is the encoding for PDIV=7 with > > bit#0 incorrectly set. > > > > This happens with the > > > > "3840x2160": 30 262750 3840 3888 3920 4000 2160 2163 2168 2191 0x48 0x9 > > > > HDMI mode (scaled from a 1024x768 src fb) set by BIOS and the > > > > ref_clock=24000, dco_integer=383, dco_fraction=5802, pdiv=7, qdiv=1, kdiv=1 > > > > WRPLL parameters (assuming PDIV=7 was the intended setting). This > > corresponds to 262749 PLL frequency/port clock. > > > > Later the driver sets the same mode for which it calculates the same > > dco_int/dco_frac/div WRPLL parameters (with the correct PDIV=7 encoding). > > > > Based on the above, let's assume that PDIV=7 was intended and the HW > > just ignores bit#0 in the PDIV register field for this setting, treating > > 100b and 101b encodings the same way. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 8 ++++++++ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > index e08684e34078..095b53fe3a21 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > @@ -1602,6 +1602,14 @@ static int skl_ddi_wrpll_get_freq(struct drm_i915_private *i915, > > case DPLL_CFGCR2_PDIV_3: > > p0 = 3; > > break; > > + case DPLL_CFGCR2_PDIV_7 | (1 << DPLL_CFGCR2_PDIV_SHIFT): > > Maybe we want a define for this? Ok. > > > + /* > > + * Incorrect ASUS-Z170M BIOS setting, the HW seems to ignore bit#0, > > + * handling it the same way as PDIV_7. > > + */ > > + drm_err(&i915->drm, "Invalid WRPLL PDIV divider value, fixing it.\n"); > > I wonder how many bug reports that will generate. Might want to make > it debug insteead. I thought having reports for this is actually good, since BIOS vendors should be notified then, but can change this to debug. > > > + p0 = 7; > > + break; > > Or maybe fallthrough? can do if you have an idea how to do that with the MISSING_CASE() added later setting pdiv=7 by default. > > > case DPLL_CFGCR2_PDIV_7: > > p0 = 7; > > break; > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 47730a176698..f70e45bd3810 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -10253,6 +10253,7 @@ enum skl_power_gate { > > #define DPLL_CFGCR2_KDIV_3 (2 << 5) > > #define DPLL_CFGCR2_KDIV_1 (3 << 5) > > #define DPLL_CFGCR2_PDIV_MASK (7 << 2) > > +#define DPLL_CFGCR2_PDIV_SHIFT 2 > > #define DPLL_CFGCR2_PDIV(x) ((x) << 2) > > #define DPLL_CFGCR2_PDIV_1 (0 << 2) > > #define DPLL_CFGCR2_PDIV_2 (1 << 2) > > -- > > 2.25.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index e08684e34078..095b53fe3a21 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -1602,6 +1602,14 @@ static int skl_ddi_wrpll_get_freq(struct drm_i915_private *i915, case DPLL_CFGCR2_PDIV_3: p0 = 3; break; + case DPLL_CFGCR2_PDIV_7 | (1 << DPLL_CFGCR2_PDIV_SHIFT): + /* + * Incorrect ASUS-Z170M BIOS setting, the HW seems to ignore bit#0, + * handling it the same way as PDIV_7. + */ + drm_err(&i915->drm, "Invalid WRPLL PDIV divider value, fixing it.\n"); + p0 = 7; + break; case DPLL_CFGCR2_PDIV_7: p0 = 7; break; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 47730a176698..f70e45bd3810 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -10253,6 +10253,7 @@ enum skl_power_gate { #define DPLL_CFGCR2_KDIV_3 (2 << 5) #define DPLL_CFGCR2_KDIV_1 (3 << 5) #define DPLL_CFGCR2_PDIV_MASK (7 << 2) +#define DPLL_CFGCR2_PDIV_SHIFT 2 #define DPLL_CFGCR2_PDIV(x) ((x) << 2) #define DPLL_CFGCR2_PDIV_1 (0 << 2) #define DPLL_CFGCR2_PDIV_2 (1 << 2)
The BIOS of at least one ASUS-Z170M system with an SKL I have programs the 101b WRPLL PDIV divider value, which is the encoding for PDIV=7 with bit#0 incorrectly set. This happens with the "3840x2160": 30 262750 3840 3888 3920 4000 2160 2163 2168 2191 0x48 0x9 HDMI mode (scaled from a 1024x768 src fb) set by BIOS and the ref_clock=24000, dco_integer=383, dco_fraction=5802, pdiv=7, qdiv=1, kdiv=1 WRPLL parameters (assuming PDIV=7 was the intended setting). This corresponds to 262749 PLL frequency/port clock. Later the driver sets the same mode for which it calculates the same dco_int/dco_frac/div WRPLL parameters (with the correct PDIV=7 encoding). Based on the above, let's assume that PDIV=7 was intended and the HW just ignores bit#0 in the PDIV register field for this setting, treating 100b and 101b encodings the same way. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 8 ++++++++ drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 9 insertions(+)