Re: [PATCH 08/17] drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O

[Sam Ravnborg <sam@ravnborg.org>]

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@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