[PATCH] radeon: Workaround PLL access bugs
Benjamin Herrenschmidt
benh at kernel.crashing.org
Tue Mar 8 14:35:48 PST 2005
On Tue, 2005-03-08 at 14:51 -0500, Hui Yu wrote:
> Hi Ben,
> Thanks for putting together the patch. Personally I'd prefer to wrap
> the workaround in a macro with the test condition for the affected
> ASICs even it's not performance sensitive. In this case, there are two
> workarounds: 1. For RV200/M7/RS200 PLL reading, two dummy reads after
> setting CLOCK_CNTL_INDEX are needed before CLOCK_CNTL_DATA can be
> reliably read. It's okay to write to CLOCK_CNTL_DATA without the dummy
> reads, so this doesn't affect OUTPLL macro. 2. For RS100/RS200, one
> dummy read may needed before writing into CLOCK_CNTL_INDEX (I haven't
> seen single case of error caused by this). It's probably cleaner to
> define two macros for these two workarounds and use them accordingly?
Well, in radeonfb, I've defined 2 inline functions (workaround_before
and workaround_after :)
I agree that it's a bit ugly for now. I think the best solution is in
fact to make a wrapper macro for accessing CLOCK_CNTL_INDEX.
This leaves me still with a couple of questions though:
- I've seen weird stuff similar to those workarounds done by Apple's
driver for the rv280, so is it affected too ?
- The r300 workaround wasn't in OUTPLL, is this normal ?
- Do we need any workaround (any of the above and the r300 one) when
just reading CLOCK_CNTL_INDEX (like when reading the current PPLL index
when probing the PLL values).
- Do we need any workaround when just writing the current PPPL index in
CLOCK_CNTL_INDEX ?
With those answers, I'll come up with a better/cleaner solution.
Thanks,
Ben.
> Regards,
> Hui
>
> >-----Original Message-----
> >From: Benjamin Herrenschmidt [mailto:benh at kernel.crashing.org]
> >Sent: Monday, March 07, 2005 10:49 PM
> >To: xorg at lists.freedesktop.org
> >Cc: Hui Yu; Alex Deucher; Dave Airlie
> >Subject: [PATCH] radeon: Workaround PLL access bugs
> >
> >Hi !
> >
> >Here's a patch adding workarounds to various PLL access routines
> >for HW bugs in some radeon chips.
> >
> >Hui, can you check my workarounds are correct ? I'm doing a dummy
> read
> >of CRTC_GEN_CNTL before any access to CLOCK_CNTL_INDEX, followed by
> a
> >dummy read of CRTC_GEN_CNTL and a dummy read of CLOCK_CNTL_DATA,
> which
> >is itself followed by the real read/write of CLOCK_CNTL_DATA if any
> (in
> >some cases, we are only accessing CLOCK_CNTL_INDEX itself).
> >
> >I also added the R300 workarounds in a few places, I'm not sure if
> it's
> >too much at this point, but it seemed logical, and PLL register
> accesses
> >aren't performance sensitive at this point.
> >
> >The patch also fixes a small bug where we wouldn't set the PPLL
> >selection in CLOCK_CNTL_INDEX to 3 if PPLL_DIV_3 already contained
> the
> >same values we are trying to write to it.
> >
> >Finally, the patch fixes a couple of bugs regarding accessing
> >CLOCK_CNTL_DATA instead of CLOCK_CNTL_INDEX for reading the current
> PPLL
> >selection. (Very weird, I was sure I fixed that specific bug a while
> >ago, I wonder if my patches got munged when merged, or if it's me
> who
> >screwed up ...)
> >
> >The patch for fixing the dynamic clocks will come later today
> hopefully.
> >
> >Hui, I'm waiting for your review before asking somebody to push to
> CVS,
> >
> >Ben.
> >
> >Index: xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c
> >===================================================================
> >--- xc.orig/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c
> > 2005-03-04 13:58:17.000000000 +1100
> >+++ xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c
> 2005-03-
> >08 14:46:59.000000000 +1100
> >@@ -729,20 +729,40 @@
> > OUTREG(RADEON_CLOCK_CNTL_INDEX, save);
> > }
> >
> >-/* Read PLL information */
> >+/* Read PLL register */
> > unsigned RADEONINPLL(ScrnInfoPtr pScrn, int addr)
> > {
> > RADEONInfoPtr info = RADEONPTR(pScrn);
> > unsigned char *RADEONMMIO = info->MMIO;
> > CARD32 data;
> >
> >+ /* A few dummy reads of CRTC_GEN_CNTL and CLOCK_CNTL_DATA
> >+ * are used as workarounds for buggy rv200 & similar chips
> >+ * before and after accesses to CLOCK_CNTL_INDEX
> >+ */
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> > OUTREG8(RADEON_CLOCK_CNTL_INDEX, addr & 0x3f);
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> > data = INREG(RADEON_CLOCK_CNTL_DATA);
> > if (info->R300CGWorkaround) R300CGWorkaround(pScrn);
> >
> > return data;
> > }
> >
> >+/* Write PLL information */
> >+unsigned RADEONOUTPLL(ScrnInfoPtr pScrn, int addr, CARD32 data)
> >+{
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ OUTREG8(RADEON_CLOCK_CNTL_INDEX, (((addr) & 0x3f) |
> >+ RADEON_PLL_WR_EN));
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ OUTREG(RADEON_CLOCK_CNTL_DATA, val);
> >+ if (info->R300CGWorkaround) R300CGWorkaround(pScrn);
> >+}
> >+
> >+
> > #if 0
> > /* Read PAL information (only used for debugging) */
> > static int RADEONINPAL(int idx)
> >@@ -1287,8 +1307,11 @@
> > break;
> > }
> >
> >- OUTREG(RADEON_CLOCK_CNTL_INDEX, 1);
> >- ppll_div_sel = INREG8(RADEON_CLOCK_CNTL_DATA + 1) & 0x3;
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ ppll_div_sel = INREG8(RADEON_CLOCK_CNTL_INDEX + 1) & 0x3;
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ if (info->R300CGWorkaround) R300CGWorkaround(pScrn);
> >
> > n = (INPLL(pScrn, RADEON_PPLL_DIV_0 + ppll_div_sel) & 0x7ff);
> > m = (INPLL(pScrn, RADEON_PPLL_REF_DIV) & 0x3ff);
> >@@ -1441,15 +1464,17 @@
> > if (xf86ReturnOptValBool(info->Options, OPTION_LVDS_PROBE_PLL,
> TRUE))
> >{
> > CARD32 ppll_div_sel, ppll_val;
> >
> >- OUTREG(RADEON_CLOCK_CNTL_INDEX, 1);
> >- ppll_div_sel = INREG8(RADEON_CLOCK_CNTL_DATA + 1) & 0x3;
> >- ppll_val = INPLL(pScrn, RADEON_PPLL_DIV_0 +
> ppll_div_sel);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ ppll_div_sel = INREG8(RADEON_CLOCK_CNTL_INDEX + 1) &
> 0x3;
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ ppll_val = INPLL(pScrn, RADEON_PPLL_DIV_0 + ppll_div_sel);
> > if ((ppll_val & 0x000707ff) == 0x1bb)
> >- goto noprobe;
> >- info->FeedbackDivider = ppll_val & 0x7ff;
> >- info->PostDivider = (ppll_val >> 16) & 0x7;
> >- info->RefDivider = info->pll.reference_div;
> >- info->UseBiosDividers = TRUE;
> >+ goto noprobe;
> >+ info->FeedbackDivider = ppll_val & 0x7ff;
> >+ info->PostDivider = (ppll_val >> 16) & 0x7;
> >+ info->RefDivider = info->pll.reference_div;
> >+ info->UseBiosDividers = TRUE;
> >
> > xf86DrvMsg(pScrn->scrnIndex, X_INFO,
> > "Existing panel PLL dividers will be
> used.\n");
> >@@ -5766,8 +5791,17 @@
> > By doing this we can avoid the blanking problem with
> some
> >panels.
> > */
> > if ((restore->ppll_ref_div == (INPLL(pScrn,
> RADEON_PPLL_REF_DIV) &
> >RADEON_PPLL_REF_DIV_MASK)) &&
> >- (restore->ppll_div_3 == (INPLL(pScrn, RADEON_PPLL_DIV_3)
> &
> >(RADEON_PPLL_POST3_DIV_MASK | RADEON_PPLL_FB3_DIV_MASK))))
> >- return;
> >+ (restore->ppll_div_3 == (INPLL(pScrn, RADEON_PPLL_DIV_3)
> &
> >+ (RADEON_PPLL_POST3_DIV_MASK |
> >RADEON_PPLL_FB3_DIV_MASK)))) {
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ OUTREGP(RADEON_CLOCK_CNTL_INDEX,
> >+ RADEON_PLL_DIV_SEL,
> >+ ~(RADEON_PLL_DIV_SEL));
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ if (info->R300CGWorkaround) R300CGWorkaround(pScrn);
> >+ return;
> >+ }
> > }
> >
> > OUTPLLP(pScrn, RADEON_VCLK_ECP_CNTL,
> >@@ -5783,9 +5817,13 @@
> > | RADEON_PPLL_ATOMIC_UPDATE_EN
> > | RADEON_PPLL_VGA_ATOMIC_UPDATE_EN));
> >
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> > OUTREGP(RADEON_CLOCK_CNTL_INDEX,
> > RADEON_PLL_DIV_SEL,
> > ~(RADEON_PLL_DIV_SEL));
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> >+ if (info->R300CGWorkaround) R300CGWorkaround(pScrn);
> >
> > if (IS_R300_VARIANT ||
> > (info->ChipFamily == CHIP_FAMILY_RS300)) {
> >@@ -6393,7 +6431,10 @@
> > }
> > save->dp_datatype = INREG(RADEON_DP_DATATYPE);
> > save->rbbm_soft_reset = INREG(RADEON_RBBM_SOFT_RESET);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> > save->clock_cntl_index = INREG(RADEON_CLOCK_CNTL_INDEX);
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> > if (info->R300CGWorkaround) R300CGWorkaround(pScrn);
> > }
> >
> >@@ -6422,7 +6463,10 @@
> > }
> > RADEONBlank(pScrn);
> >
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> > OUTREG(RADEON_CLOCK_CNTL_INDEX, restore->clock_cntl_index);
> >+ (void)INREG(RADEON_CLOCK_CNTL_DATA);
> >+ (void)INREG(RADEON_CRTC_GEN_CNTL);
> > if (info->R300CGWorkaround) R300CGWorkaround(pScrn);
> > OUTREG(RADEON_RBBM_SOFT_RESET, restore->rbbm_soft_reset);
> > OUTREG(RADEON_DP_DATATYPE, restore->dp_datatype);
> >Index: xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_macros.h
> >===================================================================
> >--- xc.orig/programs/Xserver/hw/xfree86/drivers/ati/radeon_macros.h
> > 2004-07-31 08:20:21.000000000 +1000
> >+++ xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_macros.h
> 2005-03-
> >08 14:39:41.000000000 +1100
> >@@ -84,12 +84,7 @@
> >
> > #define INPLL(pScrn, addr) RADEONINPLL(pScrn, addr)
> >
> >-#define OUTPLL(addr, val)
> \
> >-do {
> \
> >- OUTREG8(RADEON_CLOCK_CNTL_INDEX, (((addr) & 0x3f) |
> \
> >- RADEON_PLL_WR_EN));
> \
> >- OUTREG(RADEON_CLOCK_CNTL_DATA, val);
> \
> >-} while (0)
> >+#define OUTPLL(pScrn, addr, CARD32 val) RADEONOUTPLL(pScrn, addr,
> val)
> >
> > #define OUTPLLP(pScrn, addr, val, mask)
> \
> > do {
> \
> >
> >
>
--
Benjamin Herrenschmidt <benh at kernel.crashing.org>
More information about the xorg
mailing list