| Message ID | 1493214013-15580-8-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 26, 2017 at 04:40:12PM +0300, Imre Deak wrote: > This looks like a left-over from enabling work. I don't have the > specification to check whether we have to set > CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED, for now just keep things > as-is, removing the magic so that static checkers don't complain. The spec does list the top two bits as reserved with the default value of 10b. I don't see any mention of how reserved bits should be handled though. But I think I'd just change it to set them to the default value. The whole thing just looks like an oversight in the original ddx commit, whose commit message isn't all that helpful: commit 04e936935f0b0045600241424f1d04a6721a2432 Author: Eric Anholt <eric@anholt.net> Date: Mon Oct 1 17:29:35 2007 -0700 Bring the CH7017 driver closer to spec. This is also closer to what my hardware is programmed with, except for some very confusing off-by-one bugs in an unexpected direction. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/dvo_ch7017.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c > index b3c7c19..c0712a5 100644 > --- a/drivers/gpu/drm/i915/dvo_ch7017.c > +++ b/drivers/gpu/drm/i915/dvo_ch7017.c > @@ -280,10 +280,13 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo, > (0 << CH7017_PHASE_DETECTOR_SHIFT); > } else { > outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH; > - lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED | > + /* > + * FIXME: Check if CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED > + * needs to be also set for the following. > + */ > + lvds_pll_feedback_div = > (2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) | > (3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT); > - lvds_pll_feedback_div = 35; > lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) | > (0 << CH7017_PHASE_DETECTOR_SHIFT); > if (1) { /* XXX: dual channel panel detection. Assume yes for now. */ > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 26, 2017 at 05:50:06PM +0300, Ville Syrjälä wrote: > On Wed, Apr 26, 2017 at 04:40:12PM +0300, Imre Deak wrote: > > This looks like a left-over from enabling work. I don't have the > > specification to check whether we have to set > > CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED, for now just keep things > > as-is, removing the magic so that static checkers don't complain. > > The spec does list the top two bits as reserved with the default value > of 10b. I don't see any mention of how reserved bits should be handled > though. But I think I'd just change it to set them to the default value. Ok, can change that based on the above. --Imre > > The whole thing just looks like an oversight in the original ddx commit, > whose commit message isn't all that helpful: > > commit 04e936935f0b0045600241424f1d04a6721a2432 > Author: Eric Anholt <eric@anholt.net> > Date: Mon Oct 1 17:29:35 2007 -0700 > > Bring the CH7017 driver closer to spec. > > This is also closer to what my hardware is programmed with, except for some > very confusing off-by-one bugs in an unexpected direction. > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/dvo_ch7017.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c > > index b3c7c19..c0712a5 100644 > > --- a/drivers/gpu/drm/i915/dvo_ch7017.c > > +++ b/drivers/gpu/drm/i915/dvo_ch7017.c > > @@ -280,10 +280,13 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo, > > (0 << CH7017_PHASE_DETECTOR_SHIFT); > > } else { > > outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH; > > - lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED | > > + /* > > + * FIXME: Check if CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED > > + * needs to be also set for the following. > > + */ > > + lvds_pll_feedback_div = > > (2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) | > > (3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT); > > - lvds_pll_feedback_div = 35; > > lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) | > > (0 << CH7017_PHASE_DETECTOR_SHIFT); > > if (1) { /* XXX: dual channel panel detection. Assume yes for now. */ > > -- > > 2.5.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c b/drivers/gpu/drm/i915/dvo_ch7017.c index b3c7c19..c0712a5 100644 --- a/drivers/gpu/drm/i915/dvo_ch7017.c +++ b/drivers/gpu/drm/i915/dvo_ch7017.c @@ -280,10 +280,13 @@ static void ch7017_mode_set(struct intel_dvo_device *dvo, (0 << CH7017_PHASE_DETECTOR_SHIFT); } else { outputs_enable = CH7017_LVDS_CHANNEL_A | CH7017_CHARGE_PUMP_HIGH; - lvds_pll_feedback_div = CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED | + /* + * FIXME: Check if CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED + * needs to be also set for the following. + */ + lvds_pll_feedback_div = (2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) | (3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT); - lvds_pll_feedback_div = 35; lvds_control_2 = (3 << CH7017_LOOP_FILTER_SHIFT) | (0 << CH7017_PHASE_DETECTOR_SHIFT); if (1) { /* XXX: dual channel panel detection. Assume yes for now. */
This looks like a left-over from enabling work. I don't have the specification to check whether we have to set CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED, for now just keep things as-is, removing the magic so that static checkers don't complain. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/dvo_ch7017.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)