[PATCH 1/2] drm/sun4i: increase lvds dclk max divisor

[PATCH 1/2] drm/sun4i: increase lvds dclk max divisor

[Giulio Benetti]

At the moment both min and max dclk div are set to 7.
This doesn't allow to have lower frequencies.

Increase dclk_max_div to 18 to achieve 30Mhz.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 029d2ce..bb35f41 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -278,7 +278,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
 	u32 reg, val = 0;
 
 	tcon->dclk_min_div = 7;
-	tcon->dclk_max_div = 7;
+	tcon->dclk_max_div = 18;
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Adjust clock delay */
-- 
2.7.4

[PATCH 2/2] drm/sun4i: add lvds mode_valid function

[Giulio Benetti]

mode_valid function is missing for lvds.

Add it based on rgb model, also setting up dclk_min_div and dclk_max_div

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_lvds.c | 55 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index be3f14d..75223ee 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -46,8 +46,63 @@ static int sun4i_lvds_get_modes(struct drm_connector *connector)
 	return drm_panel_get_modes(tcon->panel);
 }
 
+static int sun4i_lvds_mode_valid(struct drm_connector *connector,
+				struct drm_display_mode *mode)
+{
+	struct sun4i_lvds *lvds = drm_connector_to_sun4i_lvds(connector);
+	struct sun4i_tcon *tcon = lvds->tcon;
+	u32 hsync = mode->hsync_end - mode->hsync_start;
+	u32 vsync = mode->vsync_end - mode->vsync_start;
+	unsigned long rate = mode->clock * 1000;
+	long rounded_rate;
+
+	DRM_DEBUG_DRIVER("Validating modes...\n");
+
+	if (hsync < 1)
+		return MODE_HSYNC_NARROW;
+
+	if (hsync > 0x3ff)
+		return MODE_HSYNC_WIDE;
+
+	if ((mode->hdisplay < 1) || (mode->htotal < 1))
+		return MODE_H_ILLEGAL;
+
+	if ((mode->hdisplay > 0x7ff) || (mode->htotal > 0xfff))
+		return MODE_BAD_HVALUE;
+
+	DRM_DEBUG_DRIVER("Horizontal parameters OK\n");
+
+	if (vsync < 1)
+		return MODE_VSYNC_NARROW;
+
+	if (vsync > 0x3ff)
+		return MODE_VSYNC_WIDE;
+
+	if ((mode->vdisplay < 1) || (mode->vtotal < 1))
+		return MODE_V_ILLEGAL;
+
+	if ((mode->vdisplay > 0x7ff) || (mode->vtotal > 0xfff))
+		return MODE_BAD_VVALUE;
+
+	DRM_DEBUG_DRIVER("Vertical parameters OK\n");
+
+	tcon->dclk_min_div = 7;
+	tcon->dclk_max_div = 18;
+	rounded_rate = clk_round_rate(tcon->dclk, rate);
+	if (rounded_rate < rate)
+		return MODE_CLOCK_LOW;
+
+	if (rounded_rate > rate)
+		return MODE_CLOCK_HIGH;
+
+	DRM_DEBUG_DRIVER("Clock rate OK\n");
+
+	return MODE_OK;
+}
+
 static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
 	.get_modes	= sun4i_lvds_get_modes,
+	.mode_valid	= sun4i_lvds_mode_valid,
 };
 
 static void
-- 
2.7.4

Re: [PATCH 2/2] drm/sun4i: add lvds mode_valid function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On Wed, Feb 28, 2018 at 06:53:52PM +0100, Giulio Benetti wrote:
>  static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
>  	.get_modes	= sun4i_lvds_get_modes,
> +	.mode_valid	= sun4i_lvds_mode_valid,
>  };

This should be on the encoder, not the connector.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm/sun4i: add lvds mode_valid function

[Giulio Benetti]

Hi,

Il 01/03/2018 10:57, Maxime Ripard ha scritto:
> On Wed, Feb 28, 2018 at 06:53:52PM +0100, Giulio Benetti wrote:
>>   static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
>>   	.get_modes	= sun4i_lvds_get_modes,
>> +	.mode_valid	= sun4i_lvds_mode_valid,
>>   };
> 
> This should be on the encoder, not the connector.

I've seen it is bound to connector in rgb and to encoder in hdmi.
Is it correct rgb mode_valid under connector funcs?
Otherwise I send a patch also for that one.

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


-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

Re: [PATCH 2/2] drm/sun4i: add lvds mode_valid function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Fri, Mar 02, 2018 at 12:42:14PM +0100, Giulio Benetti wrote:
> Hi,
> 
> Il 01/03/2018 10:57, Maxime Ripard ha scritto:
> > On Wed, Feb 28, 2018 at 06:53:52PM +0100, Giulio Benetti wrote:
> > >   static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
> > >   	.get_modes	= sun4i_lvds_get_modes,
> > > +	.mode_valid	= sun4i_lvds_mode_valid,
> > >   };
> > 
> > This should be on the encoder, not the connector.
> 
> I've seen it is bound to connector in rgb and to encoder in hdmi.
> Is it correct rgb mode_valid under connector funcs?
> Otherwise I send a patch also for that one.

This would need to be fixed as well. Bridges attach to encoder, not
connectors, so if you ever have a bridge connected to the RGB output
(like on the A13-Olinuxino), mode_valid isn't called at the moment.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm/sun4i: add lvds mode_valid function

[Giulio Benetti]

Hi,

Il 02/03/2018 15:37, Maxime Ripard ha scritto:
> On Fri, Mar 02, 2018 at 12:42:14PM +0100, Giulio Benetti wrote:
>> Hi,
>>
>> Il 01/03/2018 10:57, Maxime Ripard ha scritto:
>>> On Wed, Feb 28, 2018 at 06:53:52PM +0100, Giulio Benetti wrote:
>>>>    static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
>>>>    	.get_modes	= sun4i_lvds_get_modes,
>>>> +	.mode_valid	= sun4i_lvds_mode_valid,
>>>>    };
>>>
>>> This should be on the encoder, not the connector.
>>
>> I've seen it is bound to connector in rgb and to encoder in hdmi.
>> Is it correct rgb mode_valid under connector funcs?
>> Otherwise I send a patch also for that one.
> 
> This would need to be fixed as well. Bridges attach to encoder, not
> connectors, so if you ever have a bridge connected to the RGB output
> (like on the A13-Olinuxino), mode_valid isn't called at the moment.

Ok, I will do the same for rgb and submit a patchset,
need some time to test both lvds and rgb.

-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

Re: [PATCH 1/2] drm/sun4i: increase lvds dclk max divisor

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

On Wed, Feb 28, 2018 at 06:53:51PM +0100, Giulio Benetti wrote:
> At the moment both min and max dclk div are set to 7.
> This doesn't allow to have lower frequencies.
> 
> Increase dclk_max_div to 18 to achieve 30Mhz.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 029d2ce..bb35f41 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -278,7 +278,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
>  	u32 reg, val = 0;
>  
>  	tcon->dclk_min_div = 7;
> -	tcon->dclk_max_div = 7;
> +	tcon->dclk_max_div = 18;

This needs much more justification.

What panel did you test it on? Why aren't we able to reach 30MHz
already? Why do you care about 30MHz and not any other frequency?

https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/video/sunxi/disp/disp_clk.c#L686

Why Allwinner is always using 7, just like U-Boot is, and we should
use something different?

Why 18 would be a better choice?

All that should be in your commit log.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]