Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: daniel@ffwll.ch, airlied@redhat.com,
	maarten.lankhorst@linux.intel.com, mripard@kernel.org,
	emil.velikov@collabora.com, John.p.donnelly@oracle.com,
	dri-devel@lists.freedesktop.org, stable@vger.kernel.org
Subject: Re: [PATCH 01/12] drm/mgag200: Select clock in PLL update functions
Date: Mon, 12 Jul 2021 15:36:29 +0200	[thread overview]
Message-ID: <31e6618d-9048-84c6-b933-79ce2de11e81@suse.de> (raw)
In-Reply-To: <YOiaX7UJ9Ka5xTM2@ravnborg.org>

[-- Attachment #1.1: Type: text/plain, Size: 2296 bytes --]

Hi

Am 09.07.21 um 20:50 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Mon, Jul 05, 2021 at 02:45:04PM +0200, Thomas Zimmermann wrote:
>> Put the clock-selection code into each of the PLL-update functions to
>> make them select the correct pixel clock.
>>
>> The pixel clock for video output was not actually set before programming
>> the clock's values. It worked because the device had the correct clock
>> pre-set.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: db05f8d3dc87 ("drm/mgag200: Split MISC register update into PLL selection, SYNC and I/O")
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Emil Velikov <emil.velikov@collabora.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v5.9+
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_mode.c | 47 ++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 3b3059f471c2..482843ebb69f 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -130,6 +130,7 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
>>   	long ref_clk = mdev->model.g200.ref_clk;
>>   	long p_clk_min = mdev->model.g200.pclk_min;
>>   	long p_clk_max =  mdev->model.g200.pclk_max;
>> +	u8 misc;
>>   
>>   	if (clock > p_clk_max) {
>>   		drm_err(dev, "Pixel Clock %ld too high\n", clock);
>> @@ -174,6 +175,11 @@ static int mgag200_g200_set_plls(struct mga_device *mdev, long clock)
>>   	drm_dbg_kms(dev, "clock: %ld vco: %ld m: %d n: %d p: %d s: %d\n",
>>   		    clock, f_vco, m, n, p, s);
>>   
>> +	misc = RREG8(MGA_MISC_IN);
>> +	misc &= ~MGAREG_MISC_CLK_SEL_MASK;
>> +	misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
>> +	WREG8(MGA_MISC_OUT, misc);
> 
> This chunk is repeated a number of times.
> Any good reason why this is not a small helper?

Good point. I'll make a helper from this.

Best regards
Thomas

> 
> 	Sam
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]