[PATCH 1/2] [v2] drm/aspeed: Preserve DVO configuration bits during initialization

[PATCH 1/2] [v2] drm/aspeed: Preserve DVO configuration bits during initialization

[Timothy Pearson]

GFX064 contains DVO enable and mode bits.  These are hardware specific, configured
via the pinmux from the DT, and should not be cleared during startup.

Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
---
 drivers/gpu/drm/aspeed/aspeed_gfx.h     | 3 +++
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx.h b/drivers/gpu/drm/aspeed/aspeed_gfx.h
index b7a986e49177..b34c97613aaf 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx.h
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx.h
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 // Copyright 2018 IBM Corporation
+// Copyright 2019 Raptor Engineering, LLC
 
 #include <drm/drm_device.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -73,6 +74,8 @@ int aspeed_gfx_create_output(struct drm_device *drm);
 
 /* CTRL2 */
 #define CRT_CTRL_DAC_EN			BIT(0)
+#define CRT_CTRL_DVO_MODE		BIT(6)
+#define CRT_CTRL_DVO_EN			BIT(7)
 #define CRT_CTRL_VBLANK_LINE(x)		(((x) << 20) & CRT_CTRL_VBLANK_LINE_MASK)
 #define CRT_CTRL_VBLANK_LINE_MASK	GENMASK(20, 31)
 
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 713a3975852b..7e9072fd0ef0 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -98,6 +98,7 @@ static int aspeed_gfx_load(struct drm_device *drm)
 	struct aspeed_gfx *priv;
 	struct resource *res;
 	int ret;
+	u32 reg;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -146,7 +147,9 @@ static int aspeed_gfx_load(struct drm_device *drm)
 
 	/* Sanitize control registers */
 	writel(0, priv->base + CRT_CTRL1);
-	writel(0, priv->base + CRT_CTRL2);
+	/* Preserve CRT_CTRL2[7:6] (DVO configuration) */
+	reg = readl(priv->base + CRT_CTRL2) & (CRT_CTRL_DVO_MODE | CRT_CTRL_DVO_EN);
+	writel(reg, priv->base + CRT_CTRL2);
 
 	aspeed_gfx_setup_mode_config(drm);
 
-- 
2.11.0

Re: [PATCH 1/2] [v2] drm/aspeed: Preserve DVO configuration bits during initialization

[Andrew Jeffery]

Hi Timothy,

A few of things:

1. Please run ./scripts/checkpatch.pl over your patches before sending
2. Version your series coherently - having [v2] of one patch in the series threw
     me out. Please use `git format-patch -vX`, it will handle this for you
     automatically.
3. For multi-patch series I suggest using a cover letter - this helps thread them
     in a sane manner. It's also a great place to describe what you're trying to
     achieve with the series
4. Please use ./scripts/get_maintainer.pl to find the right people and lists to
     which to send patches.

In this case Joel will pick the patch up, but it's better if you have him either
in To: or Cc: as you're more likely to get his attention this way.

Generally, a lot of this is covered in the kernel's documentation - dig around
under Documentation/process.

Aside from that, some minor style points below:

On Fri, 3 May 2019, at 07:21, Timothy Pearson wrote:
> GFX064 contains DVO enable and mode bits.  These are hardware specific, 
> configured

This should be wrapped at 75 chars

> via the pinmux from the DT, and should not be cleared during startup.
> 
> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> ---
>  drivers/gpu/drm/aspeed/aspeed_gfx.h     | 3 +++
>  drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx.h 
> b/drivers/gpu/drm/aspeed/aspeed_gfx.h
> index b7a986e49177..b34c97613aaf 100644
> --- a/drivers/gpu/drm/aspeed/aspeed_gfx.h
> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx.h
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  // Copyright 2018 IBM Corporation
> +// Copyright 2019 Raptor Engineering, LLC
>  
>  #include <drm/drm_device.h>
>  #include <drm/drm_simple_kms_helper.h>
> @@ -73,6 +74,8 @@ int aspeed_gfx_create_output(struct drm_device *drm);
>  
>  /* CTRL2 */
>  #define CRT_CTRL_DAC_EN			BIT(0)
> +#define CRT_CTRL_DVO_MODE		BIT(6)
> +#define CRT_CTRL_DVO_EN			BIT(7)
>  #define CRT_CTRL_VBLANK_LINE(x)		(((x) << 20) & CRT_CTRL_VBLANK_LINE_MASK)
>  #define CRT_CTRL_VBLANK_LINE_MASK	GENMASK(20, 31)
>  
> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c 
> b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
> index 713a3975852b..7e9072fd0ef0 100644
> --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
> @@ -98,6 +98,7 @@ static int aspeed_gfx_load(struct drm_device *drm)
>  	struct aspeed_gfx *priv;
>  	struct resource *res;
>  	int ret;
> +	u32 reg;

This breaks the reverse-christmas-tree ordering, but that's getting quite pedantic.
 
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -146,7 +147,9 @@ static int aspeed_gfx_load(struct drm_device *drm)
>  
>  	/* Sanitize control registers */
>  	writel(0, priv->base + CRT_CTRL1);
> -	writel(0, priv->base + CRT_CTRL2);
> +	/* Preserve CRT_CTRL2[7:6] (DVO configuration) */
> +	reg = readl(priv->base + CRT_CTRL2) & (CRT_CTRL_DVO_MODE | CRT_CTRL_DVO_EN);

Should be wrapped at 80 chars.

Other than that,

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> +	writel(reg, priv->base + CRT_CTRL2);
>  
>  	aspeed_gfx_setup_mode_config(drm);
>  
> -- 
> 2.11.0
>

[PATCH 1/2] [v2] drm/aspeed: Preserve DVO configuration bits during initialization

[Timothy Pearson]

----- Original Message -----
> From: "Andrew Jeffery" <andrew@aj.id.au>
> To: "Timothy Pearson" <tpearson@raptorengineering.com>, "linux-aspeed" <linux-aspeed@lists.ozlabs.org>
> Sent: Thursday, May 2, 2019 5:24:02 PM
> Subject: Re: [PATCH 1/2] [v2] drm/aspeed: Preserve DVO configuration bits during initialization

> Hi Timothy,
> 
> A few of things:
> 
> 1. Please run ./scripts/checkpatch.pl over your patches before sending
> 2. Version your series coherently - having [v2] of one patch in the series threw
>     me out. Please use `git format-patch -vX`, it will handle this for you
>     automatically.
> 3. For multi-patch series I suggest using a cover letter - this helps thread
> them
>     in a sane manner. It's also a great place to describe what you're trying to
>     achieve with the series
> 4. Please use ./scripts/get_maintainer.pl to find the right people and lists to
>     which to send patches.
> 
> In this case Joel will pick the patch up, but it's better if you have him either
> in To: or Cc: as you're more likely to get his attention this way.
> 
> Generally, a lot of this is covered in the kernel's documentation - dig around
> under Documentation/process.

No problem, will do.  Was taking some shortcuts here to meet a couple of deadlines, and was mostly looking for feedback on approach :)  If the patch is good as-is or with minor changes, of course I'm also OK with a merge.

> Aside from that, some minor style points below:
> 
> On Fri, 3 May 2019, at 07:21, Timothy Pearson wrote:
>> GFX064 contains DVO enable and mode bits.  These are hardware specific,
>> configured
> 
> This should be wrapped at 75 chars
> 
>> via the pinmux from the DT, and should not be cleared during startup.
>> 
>> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
>> ---
>>  drivers/gpu/drm/aspeed/aspeed_gfx.h     | 3 +++
>>  drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 5 ++++-
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx.h
>> b/drivers/gpu/drm/aspeed/aspeed_gfx.h
>> index b7a986e49177..b34c97613aaf 100644
>> --- a/drivers/gpu/drm/aspeed/aspeed_gfx.h
>> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx.h
>> @@ -1,5 +1,6 @@
>>  // SPDX-License-Identifier: GPL-2.0+
>>  // Copyright 2018 IBM Corporation
>> +// Copyright 2019 Raptor Engineering, LLC
>>  
>>  #include <drm/drm_device.h>
>>  #include <drm/drm_simple_kms_helper.h>
>> @@ -73,6 +74,8 @@ int aspeed_gfx_create_output(struct drm_device *drm);
>>  
>>  /* CTRL2 */
>>  #define CRT_CTRL_DAC_EN			BIT(0)
>> +#define CRT_CTRL_DVO_MODE		BIT(6)
>> +#define CRT_CTRL_DVO_EN			BIT(7)
>>  #define CRT_CTRL_VBLANK_LINE(x)		(((x) << 20) & CRT_CTRL_VBLANK_LINE_MASK)
>>  #define CRT_CTRL_VBLANK_LINE_MASK	GENMASK(20, 31)
>>  
>> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
>> b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
>> index 713a3975852b..7e9072fd0ef0 100644
>> --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
>> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
>> @@ -98,6 +98,7 @@ static int aspeed_gfx_load(struct drm_device *drm)
>>  	struct aspeed_gfx *priv;
>>  	struct resource *res;
>>  	int ret;
>> +	u32 reg;
> 
> This breaks the reverse-christmas-tree ordering, but that's getting quite
> pedantic.
> 
>>  
>>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>  	if (!priv)
>> @@ -146,7 +147,9 @@ static int aspeed_gfx_load(struct drm_device *drm)
>>  
>>  	/* Sanitize control registers */
>>  	writel(0, priv->base + CRT_CTRL1);
>> -	writel(0, priv->base + CRT_CTRL2);
>> +	/* Preserve CRT_CTRL2[7:6] (DVO configuration) */
>> +	reg = readl(priv->base + CRT_CTRL2) & (CRT_CTRL_DVO_MODE | CRT_CTRL_DVO_EN);
> 
> Should be wrapped at 80 chars.
> 
> Other than that,
> 
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> 
>> +	writel(reg, priv->base + CRT_CTRL2);
>>  
>>  	aspeed_gfx_setup_mode_config(drm);
>>  
>> --
>> 2.11.0