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

From: Sam Ravnborg <sam@ravnborg.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: john.p.donnelly@oracle.com, dri-devel@lists.freedesktop.org,
	kraxel@redhat.com, airlied@redhat.com
Subject: Re: [PATCH 08/17] drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O
Date: Mon, 4 May 2020 16:25:45 +0200	[thread overview]
Message-ID: <20200504142545.GA28466@ravnborg.org> (raw)
In-Reply-To: <3e1b1164-7dae-91a5-6973-1eec17d02f6d@suse.de>

Hi Thomas.

> >> @@ -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.
OK, when I looked at the code I initially got the impression that it was
a bit random. But then I also switched between patch and code etc.
The explanation above makes sense.

	Sam

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel