[PATCH] PLL: RV620 PLL shutdowning fix
This fixes bug http://bugs.freedesktop.org/show_bug.cgi?id=18016 for me. Egbert Eich: can you verify this on your machine affected with the same bug? All: can you test this for regressions, please? -- Rafał Miłecki
W dniu 6 czerwca 2009 21:13 użytkownik Rafał Miłecki
This fixes bug http://bugs.freedesktop.org/show_bug.cgi?id=18016 for me.
Egbert Eich: can you verify this on your machine affected with the same bug?
All: can you test this for regressions, please?
Would be nice if someone checked if I translated AtomBIOS asm to C without mistakes. AtomBIOS commands can be found in bug report. -- Rafał Miłecki -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org
W dniu 6 czerwca 2009 21:13 użytkownik Rafał Miłecki
This fixes bug http://bugs.freedesktop.org/show_bug.cgi?id=18016 for me.
Egbert Eich: can you verify this on your machine affected with the same bug?
All: can you test this for regressions, please?
Hm, I wonder if my patch shouldn't affect some smaller group of devices. For what chipsets we actually use RV620PLL1Power? Is this used for example for RV630? Or maybe even for RV610? If so this patch would need additional condition. I'm also not sure about 0x2000 of P2PLL_CNTL. From rhd_pll.c I can read this comment: RHDRegMask(PLL, P1PLL_CNTL, 0x00002000, 0x00002000); /* reset anti-glitch */ My patch actually adds P1PLL_CNTL and 0x2000 (as that is what AtomBIOS does). Do we really need this? Should we "reset anti-glitch" (whatever it means) for powering down? -- Rafał Miłecki -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org
Am Sonntag, den 07.06.2009, 01:56 +0200 schrieb Rafał Miłecki:
Hm, I wonder if my patch shouldn't affect some smaller group of devices.
For what chipsets we actually use RV620PLL1Power? Is this used for example for RV630? Or maybe even for RV610? If so this patch would need additional condition. Take a look at rhd_pll.c line ~1284: if (rhdPtr->ChipSet < RHD_RV620) { PLL->Set = R500PLL1Set; PLL->Power = R500PLL1Power; PLL->Save = R500PLL1Save; PLL->Restore = R500PLL1Restore; } else { PLL->Set = RV620PLL1Set; PLL->Power = RV620PLL1Power; PLL->Save = RV620PLL1Save; PLL->Restore = RV620PLL1Restore; }
I'm also not sure about 0x2000 of P2PLL_CNTL. From rhd_pll.c I can read this comment: RHDRegMask(PLL, P1PLL_CNTL, 0x00002000, 0x00002000); /* reset anti-glitch */
My patch actually adds P1PLL_CNTL and 0x2000 (as that is what AtomBIOS does). Do we really need this? Should we "reset anti-glitch" (whatever it means) for powering down? Correct me if i am wrong, but if i understand that patch correctly all
And at rhd_id.c around line 77: { RHD_RV610, "RV610" }, { RHD_RV630, "RV630" }, .... { RHD_RV620, "RV620" }, { RHD_RV635, "RV635" }, { RHD_RV670, "RV670" }, { RHD_R680, "R680" }, .... Don't be mislead by the marketing numbers of the chipset, rhd_id defines the real relation between the different chipset generations. that the code from AtomBIOS does is checking if the PLL is still in use, before shutting it down. So i have a lot simpler question: Why do we power down a used PLL in the first place? Bye, Christian. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org
W dniu 7 czerwca 2009 18:26 użytkownik Christian König
Am Sonntag, den 07.06.2009, 01:56 +0200 schrieb Rafał Miłecki:
Hm, I wonder if my patch shouldn't affect some smaller group of devices.
For what chipsets we actually use RV620PLL1Power? Is this used for example for RV630? Or maybe even for RV610? If so this patch would need additional condition. Take a look at rhd_pll.c line ~1284: if (rhdPtr->ChipSet < RHD_RV620) { PLL->Set = R500PLL1Set; PLL->Power = R500PLL1Power; PLL->Save = R500PLL1Save; PLL->Restore = R500PLL1Restore; } else { PLL->Set = RV620PLL1Set; PLL->Power = RV620PLL1Power; PLL->Save = RV620PLL1Save; PLL->Restore = RV620PLL1Restore; }
Hm, OK, that's the easy part. We use RV620PLL* for newer than RHD_RV620 (including RHD_RV620). The next question is when we stop using RV620PLL* and start using AtomBIOS. Browsing rhd_id.c I found: #define USE_ATOMBIOS RHD_RV770 #define USE_ATOM_CRTC USE_ATOMBIOS #define USE_ATOM_PLL USE_ATOMBIOS #define USE_ATOM_OUTPUT USE_ATOMBIOS Next I analyzed Bool RHDUseAtom(...) and found out we force using AtomBIOS for PLL (FromSys == TRUE) starting from USE_ATOM_PLL which is RV770. So to sum up RV620PLL* is used for: { RHD_RV620, "RV620" }, { RHD_RV635, "RV635" }, { RHD_RV670, "RV670" }, { RHD_R680, "R680" }, /* R700 Mobility */ { RHD_M82, "M82" }, { RHD_M88, "M88" }, { RHD_M86, "M86" }, /* R600 integrated */ { RHD_RS780, "RS780" }, { RHD_RS880, "RS880" }, /* R700 */ { RHD_R700, "R700" }, { RHD_RV710, "RV710" }, { RHD_RV730, "RV730" }, { RHD_RV740, "RV740" }, am I right? Can someone check this with me, please? Do we really use RV620PLL* for listed chipsets?
I'm also not sure about 0x2000 of P2PLL_CNTL. From rhd_pll.c I can read this comment: RHDRegMask(PLL, P1PLL_CNTL, 0x00002000, 0x00002000); /* reset anti-glitch */
My patch actually adds P1PLL_CNTL and 0x2000 (as that is what AtomBIOS does). Do we really need this? Should we "reset anti-glitch" (whatever it means) for powering down? Correct me if i am wrong, but if i understand that patch correctly all that the code from AtomBIOS does is checking if the PLL is still in use, before shutting it down.
So i have a lot simpler question: Why do we power down a used PLL in the first place?
For full explaination, please read http://bugs.freedesktop.org/show_bug.cgi?id=18016 Short explaination:
libv: ah, rv620/635. libv: this is where the beautifully separated hw blocks got mucked together on the pll side.
rv620/635 is tricky about PLL engines. These aren't 100% separated and it seems we can't really disable any on them at any time. It seems for example we have to keep PLL1 running if we use PLL2 for driving some output. -- Rafał Miłecki -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org
Am Sonntag, den 07.06.2009, 20:42 +0200 schrieb Rafał Miłecki:
am I right? Can someone check this with me, please? Do we really use RV620PLL* for listed chipsets? Mhm, yes that's seems to be what the current implementation is doing.
For full explaination, please read http://bugs.freedesktop.org/show_bug.cgi?id=18016
Short explaination:
libv: ah, rv620/635. libv: this is where the beautifully separated hw blocks got mucked together on the pll side.
rv620/635 is tricky about PLL engines. These aren't 100% separated and it seems we can't really disable any on them at any time. It seems for example we have to keep PLL1 running if we use PLL2 for driving some output. Wow, this is a really long bugreport. Ok i got it, the problem seems to be a little bit more complicated than i thought, but i think the real solution would be to move the whole handling of RV620_EXT[12]_DIFF_POST_DIV_CNTL from rhd_dig.c to rhd_pll.c.
And bye the way i think i found a bug in rhd_dig.c:EncoderRestore and rhd_dig.c:EncoderSave, RV620_EXT[12]_DIFF_POST_DIV_CNTL is added to the DIG offset, while every other read/write goes to the addresses directly. Bye, Christian. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org
W dniu 7 czerwca 2009 22:48 użytkownik Christian König
For full explaination, please read http://bugs.freedesktop.org/show_bug.cgi?id=18016
Short explaination:
libv: ah, rv620/635. libv: this is where the beautifully separated hw blocks got mucked together on the pll side.
rv620/635 is tricky about PLL engines. These aren't 100% separated and it seems we can't really disable any on them at any time. It seems for example we have to keep PLL1 running if we use PLL2 for driving some output. Wow, this is a really long bugreport. Ok i got it, the problem seems to be a little bit more complicated than i thought, but i think the real solution would be to move the whole handling of RV620_EXT[12]_DIFF_POST_DIV_CNTL from rhd_dig.c to rhd_pll.c.
Do you mean solution which could fix bug #18016 without using method I proposed? I don't see such way, can you tell something more on this? What operations would you like to move from rhd_dig.c to rhd_pll.c? Could you point me to something that would explain me what RV620_EXT[12]_DIFF_POST_DIV_CNTL is used for? I don't understand purpose of this register.
And bye the way i think i found a bug in rhd_dig.c:EncoderRestore and rhd_dig.c:EncoderSave, RV620_EXT[12]_DIFF_POST_DIV_CNTL is added to the DIG offset, while every other read/write goes to the addresses directly.
Which reads/writes do you mean? AFAICS we use "off" in both: EncoderSave and EncoderResore. -- Rafał Miłecki -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org
participants (2)
-
Christian König
-
Rafał Miłecki