[PATCH 08/17] drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O
Thomas Zimmermann
tzimmermann at suse.de
Mon May 4 13:03:04 UTC 2020
Hi
Am 03.05.20 um 17:34 schrieb Sam Ravnborg:
> Hi Thomas.
>
> On Wed, Apr 29, 2020 at 04:32:29PM +0200, Thomas Zimmermann wrote:
>> Set different fields in MISC in their rsp location in the code. This
>> patch also fixes a bug in the original code where the mode's SYNC flags
>> were never written into the MISC register.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/mgag200/mgag200_mode.c | 37 ++++++++++++++++++--------
>> drivers/gpu/drm/mgag200/mgag200_reg.h | 5 +++-
>> 2 files changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 749ba6e420ac7..b5bb02e9f05d6 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -704,6 +704,8 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)
>>
>> static int mga_crtc_set_plls(struct mga_device *mdev, long clock)
>> {
>> + uint8_t misc;
>
> General comment.
> uint8_t and friends are for uapi stuff.
> kernel internal prefer u8 and friends.
Ok.
>
> Would be good to clean this up in the drivire, maybe as a follow-up
> patch after is becomes atomic.
>
>
>> +
>> switch(mdev->type) {
>> case G200_SE_A:
>> case G200_SE_B:
>> @@ -724,6 +726,12 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock)
>> return mga_g200er_set_plls(mdev, clock);
>> break;
>> }
>> +
>> + misc = RREG8(MGA_MISC_IN);
>> + misc &= ~GENMASK(3, 2);
> The code would be easier to read if we had a
> #define MGAREG_MISC_CLK_SEL_MASK GENMASK(3, 2)
>
> So the above became:
> misc &= ~MGAREG_MISC_CLK_SEL_MASK;
>
> Then it is more clear that the CLK_SEL bits are clared and then
> MGA_MSK is set.
Sure.
>
>> + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
>> + WREG8(MGA_MISC_OUT, misc);
>> +
>> return 0;
>> }
>>
>> @@ -916,7 +924,7 @@ static void mgag200_set_mode_regs(struct mga_device *mdev,
>> {
>> unsigned int hdisplay, hsyncstart, hsyncend, htotal;
>> unsigned int vdisplay, vsyncstart, vsyncend, vtotal;
>> - uint8_t misc = 0;
>> + uint8_t misc;
>> uint8_t crtcext1, crtcext2, crtcext5;
>>
>> hdisplay = mode->hdisplay / 8 - 1;
>> @@ -933,10 +941,17 @@ static void mgag200_set_mode_regs(struct mga_device *mdev,
>> vsyncend = mode->vsync_end - 1;
>> vtotal = mode->vtotal - 2;
>>
>> + misc = RREG8(MGA_MISC_IN);
>> +
>> if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> - misc |= 0x40;
>> + misc |= MGAREG_MISC_HSYNCPOL;
>> + else
>> + misc &= ~MGAREG_MISC_HSYNCPOL;
>> +
> So the code just assumes DRM_MODE_FLAG_PHSYNC if
> DRM_MODE_FLAG_NHSYNC is not set.
> I think that is fine, but it also indicate how hoorible the
> flags definitions are in mode->flags
If polarity is not negative (i.e., bit set), it is positive (i.e., bit
cleared). What else could you set in MISC?
Having multiple flags in mode->flags that signal the same state is
somewhat problematic. I expect that the consistency of a mode's flags is
validated somewhere within the core.
>
>
>> if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>> - misc |= 0x80;
>> + misc |= MGAREG_MISC_VSYNCPOL;
>> + else
>> + misc &= ~MGAREG_MISC_VSYNCPOL;
> And this code was touched in previous patch, but I gess it is better
> to update it here.
>
>>
>> crtcext1 = (((htotal - 4) & 0x100) >> 8) |
>> ((hdisplay & 0x100) >> 7) |
>> @@ -982,6 +997,10 @@ static void mgag200_set_mode_regs(struct mga_device *mdev,
>> WREG_ECRT(0x01, crtcext1);
>> WREG_ECRT(0x02, crtcext2);
>> WREG_ECRT(0x05, crtcext5);
>> +
>> + WREG8(MGA_MISC_OUT, misc);
>> +
>> + mga_crtc_set_plls(mdev, mode->clock);
>> }
>>
>> static int mga_crtc_mode_set(struct drm_crtc *crtc,
>> @@ -1140,12 +1159,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>> ext_vga[3] = ((1 << bppshift) - 1) | 0x80;
>> ext_vga[4] = 0;
>>
>> - /* Set pixel clocks */
>> - misc = 0x2d;
>> - WREG8(MGA_MISC_OUT, misc);
>> -
>> - mga_crtc_set_plls(mdev, mode->clock);
>> -
>> WREG_ECRT(0, ext_vga[0]);
>> WREG_ECRT(3, ext_vga[3]);
>> WREG_ECRT(4, ext_vga[4]);
>> @@ -1161,9 +1174,11 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>> }
>>
>> WREG_ECRT(0, ext_vga[0]);
>> - /* Enable mga pixel clock */
>> - misc = 0x2d;
>>
>> + misc = RREG8(MGA_MISC_IN);
>> + misc |= MGAREG_MISC_IOADSEL |
>> + MGAREG_MISC_RAMMAPEN |
>> + MGAREG_MISC_HIGH_PG_SEL;
>> WREG8(MGA_MISC_OUT, misc);
>
> I am left puzzeled why there is several writes to MGA_MISC_OUT.
> The driver needs to read back and then write again.
>
> Would it be simpler to have one function that only took care of
> misc register update?
I'm not quite sure what you mean. MISC contains all kinds of unrelated
state. In the final atomic code, different state is set in different
places. It's simple to have a function read-modify-write the content of
MISC for the bits that it cares about. With multiple functions, that
leads to some duplicated reads and write, but the code as a whole is simple.
Best regards
Thomas
>
>>
>> mga_crtc_do_set_base(mdev, fb, old_fb);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
>> index c096a9d6bcbc1..89e12c55153cf 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
>> @@ -16,10 +16,11 @@
>> * MGA1064SG Mystique register file
>> */
>>
>> -
>> #ifndef _MGA_REG_H_
>> #define _MGA_REG_H_
>>
>> +#include <linux/bits.h>
>> +
>> #define MGAREG_DWGCTL 0x1c00
>> #define MGAREG_MACCESS 0x1c04
>> /* the following is a mystique only register */
>> @@ -227,6 +228,8 @@
>> #define MGAREG_MISC_CLK_SEL_MGA_MSK (0x3 << 2)
>> #define MGAREG_MISC_VIDEO_DIS (0x1 << 4)
>> #define MGAREG_MISC_HIGH_PG_SEL (0x1 << 5)
>> +#define MGAREG_MISC_HSYNCPOL BIT(6)
>> +#define MGAREG_MISC_VSYNCPOL BIT(7)
> I like BIT(), but mixing (1 << N) and BIT() does not look nice.
> Oh, and do not get me started on GENMASK() :-)
>
> Sam
>>
>> /* MMIO VGA registers */
>> #define MGAREG_SEQ_INDEX 0x1fc4
>> --
>> 2.26.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200504/702499e2/attachment.sig>
More information about the dri-devel
mailing list