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 <tzimmerm...@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.
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.
> + 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 (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?
>
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
