diff mbox series

[v2,2/2] drm/sun4i: tcon: improve DCLK polarity handling

Message ID 20210107023032.560182-3-giulio.benetti@benettiengineering.com (mailing list archive)
State Superseded
Headers show
Series drm/sun4i: fix DCLK and improve its handling | expand

Commit Message

Giulio Benetti Jan. 7, 2021, 2:30 a.m. UTC
From: Giulio Benetti <giulio.benetti@micronovasrl.com>

It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is
dedicated to invert DCLK polarity and this makes thing really easier than
before. So let's handle DCLK polarity by adding
SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to
bus_flags the same way is done for all the other signals.

Cc: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +-------------------
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
 2 files changed, 2 insertions(+), 19 deletions(-)

Comments

Maxime Ripard Jan. 8, 2021, 9:23 a.m. UTC | #1
Hi,

Thanks for those patches

On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote:
> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is
> dedicated to invert DCLK polarity and this makes thing really easier than
> before. So let's handle DCLK polarity by adding
> SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to
> bus_flags the same way is done for all the other signals.
> 
> Cc: Maxime Ripard <maxime@cerno.tech>

Suggested-by would be nice here :)

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +-------------------
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
>  2 files changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 52598bb0fb0b..30171ccd87e5 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>  	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
>  		val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
>  
> -	/*
> -	 * On A20 and similar SoCs, the only way to achieve Positive Edge
> -	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
> -	 * By default TCON works in Negative Edge(Falling Edge),
> -	 * this is why phase is set to 0 in that case.
> -	 * Unfortunately there's no way to logically invert dclk through
> -	 * IO_POL register.
> -	 * The only acceptable way to work, triple checked with scope,
> -	 * is using clock phase set to 0° for Negative Edge and set to 240°
> -	 * for Positive Edge.
> -	 * On A33 and similar SoCs there would be a 90° phase option,
> -	 * but it divides also dclk by 2.
> -	 * Following code is a way to avoid quirks all around TCON
> -	 * and DOTCLOCK drivers.
> -	 */
>  	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
> -		clk_set_phase(tcon->dclk, 0);
> -
> -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> -		clk_set_phase(tcon->dclk, 240);
> +		val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;

I'm not really sure why we need the first patch of this series here?
That patch only seem to undo what you did in patch 1

Maxime
Giulio Benetti Jan. 8, 2021, 2:34 p.m. UTC | #2
Hi,

On 1/8/21 10:23 AM, Maxime Ripard wrote:
> Hi,
> 
> Thanks for those patches
> 
> On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote:
>> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>
>> It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is
>> dedicated to invert DCLK polarity and this makes thing really easier than
>> before. So let's handle DCLK polarity by adding
>> SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to
>> bus_flags the same way is done for all the other signals.
>>
>> Cc: Maxime Ripard <maxime@cerno.tech>
> 
> Suggested-by would be nice here :)

Ok, didn't know about this tag

>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>> ---
>>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +-------------------
>>   drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
>>   2 files changed, 2 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index 52598bb0fb0b..30171ccd87e5 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>>   	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
>>   		val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
>>   
>> -	/*
>> -	 * On A20 and similar SoCs, the only way to achieve Positive Edge
>> -	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
>> -	 * By default TCON works in Negative Edge(Falling Edge),
>> -	 * this is why phase is set to 0 in that case.
>> -	 * Unfortunately there's no way to logically invert dclk through
>> -	 * IO_POL register.
>> -	 * The only acceptable way to work, triple checked with scope,
>> -	 * is using clock phase set to 0° for Negative Edge and set to 240°
>> -	 * for Positive Edge.
>> -	 * On A33 and similar SoCs there would be a 90° phase option,
>> -	 * but it divides also dclk by 2.
>> -	 * Following code is a way to avoid quirks all around TCON
>> -	 * and DOTCLOCK drivers.
>> -	 */
>>   	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
>> -		clk_set_phase(tcon->dclk, 0);
>> -
>> -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>> -		clk_set_phase(tcon->dclk, 240);
>> +		val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;
> 
> I'm not really sure why we need the first patch of this series here?

The idea was to have 2 for testing, 1st one is already applicable, while 
the other must be tested, but I can send only one with no problem.

> That patch only seem to undo what you did in patch 1

No, it doesn't, the 2nd one change the way it achieve the same thing, 
because the 1st swap DCLK phase, while the 2nd uses the IO_POL bit to 
set IO polarity according to bus_flags.

Best Regards
Giulio Benetti Jan. 8, 2021, 2:46 p.m. UTC | #3
Hi Marjan,

On 1/8/21 10:32 AM, Marjan Pascolo wrote:
> Hi,

please don't top post, answer in-line as we do, and please use 
plain-test instead of HTML. These are the standards in Mailing Lists(ML).

> Quote "
> 
> I'm not really sure why we need the first patch of this series here?
> That patch only seem to undo what you did in patch 1
> 
> "

Already answered to Maxime

> 
> And another question (probably could be a stupid one):
> 
> in "/[PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling/" I 
> see you deleted:
> 
> -		clk_set_phase(tcon->dclk, 0);
> 
> Is safe to assume that phase register will be always set to 0?

We always assumed it is set to 0 by default.

> 
> Or maybe will be safer manually set it to 0 in every condition to avoid 
> surprises (dirt values due to previous condition)?

That would be a good idea, so something like this:

'''
	int phase = 0;

	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
		phase = 240;

	clk_set_phase(tcon->dclk, phase);
'''

because now DRM_BUS_FLAG_PIXDATA_SAMPLE_ and DRM_BUS_FLAG_PIXDATA_DRIVE_ 
are exclusive, while before not.

But then if bit 26 solution works everything gets easier.

Best Regards
Giulio

> 
> Marjan
> 
> Il 08/01/2021 10:23, Maxime Ripard ha scritto:
>> Hi,
>>
>> Thanks for those patches
>>
>> On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote:
>>> From: Giulio Benetti<giulio.benetti@micronovasrl.com>
>>>
>>> It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is
>>> dedicated to invert DCLK polarity and this makes thing really easier than
>>> before. So let's handle DCLK polarity by adding
>>> SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to
>>> bus_flags the same way is done for all the other signals.
>>>
>>> Cc: Maxime Ripard<maxime@cerno.tech>
>> Suggested-by would be nice here :)
>>
>>> Signed-off-by: Giulio Benetti<giulio.benetti@micronovasrl.com>
>>> ---
>>>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +-------------------
>>>   drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
>>>   2 files changed, 2 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>> index 52598bb0fb0b..30171ccd87e5 100644
>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>> @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>>>   	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
>>>   		val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
>>>   
>>> -	/*
>>> -	 * On A20 and similar SoCs, the only way to achieve Positive Edge
>>> -	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
>>> -	 * By default TCON works in Negative Edge(Falling Edge),
>>> -	 * this is why phase is set to 0 in that case.
>>> -	 * Unfortunately there's no way to logically invert dclk through
>>> -	 * IO_POL register.
>>> -	 * The only acceptable way to work, triple checked with scope,
>>> -	 * is using clock phase set to 0° for Negative Edge and set to 240°
>>> -	 * for Positive Edge.
>>> -	 * On A33 and similar SoCs there would be a 90° phase option,
>>> -	 * but it divides also dclk by 2.
>>> -	 * Following code is a way to avoid quirks all around TCON
>>> -	 * and DOTCLOCK drivers.
>>> -	 */
>>>   	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
>>> -		clk_set_phase(tcon->dclk, 0);
>>> -
>>> -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>>> -		clk_set_phase(tcon->dclk, 240);
>>> +		val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;
>> I'm not really sure why we need the first patch of this series here?
>> That patch only seem to undo what you did in patch 1
>>
>> Maxime
Maxime Ripard Jan. 11, 2021, 5:20 p.m. UTC | #4
On Fri, Jan 08, 2021 at 03:34:52PM +0100, Giulio Benetti wrote:
> Hi,
> 
> On 1/8/21 10:23 AM, Maxime Ripard wrote:
> > Hi,
> > 
> > Thanks for those patches
> > 
> > On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote:
> > > From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > 
> > > It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is
> > > dedicated to invert DCLK polarity and this makes thing really easier than
> > > before. So let's handle DCLK polarity by adding
> > > SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to
> > > bus_flags the same way is done for all the other signals.
> > > 
> > > Cc: Maxime Ripard <maxime@cerno.tech>
> > 
> > Suggested-by would be nice here :)
> 
> Ok, didn't know about this tag
> 
> > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > ---
> > >   drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +-------------------
> > >   drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
> > >   2 files changed, 2 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > index 52598bb0fb0b..30171ccd87e5 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
> > >   	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
> > >   		val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
> > > -	/*
> > > -	 * On A20 and similar SoCs, the only way to achieve Positive Edge
> > > -	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
> > > -	 * By default TCON works in Negative Edge(Falling Edge),
> > > -	 * this is why phase is set to 0 in that case.
> > > -	 * Unfortunately there's no way to logically invert dclk through
> > > -	 * IO_POL register.
> > > -	 * The only acceptable way to work, triple checked with scope,
> > > -	 * is using clock phase set to 0° for Negative Edge and set to 240°
> > > -	 * for Positive Edge.
> > > -	 * On A33 and similar SoCs there would be a 90° phase option,
> > > -	 * but it divides also dclk by 2.
> > > -	 * Following code is a way to avoid quirks all around TCON
> > > -	 * and DOTCLOCK drivers.
> > > -	 */
> > >   	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
> > > -		clk_set_phase(tcon->dclk, 0);
> > > -
> > > -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> > > -		clk_set_phase(tcon->dclk, 240);
> > > +		val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;
> > 
> > I'm not really sure why we need the first patch of this series here?
> 
> The idea was to have 2 for testing, 1st one is already applicable, while the
> other must be tested, but I can send only one with no problem.
> 
> > That patch only seem to undo what you did in patch 1
> 
> No, it doesn't, the 2nd one change the way it achieve the same thing,
> because the 1st swap DCLK phase, while the 2nd uses the IO_POL bit to set IO
> polarity according to bus_flags.

It makes sense for testing, but I'm not sure we want to carry it into
the history. Can you squash them both into the same patch?

Thanks!
Maxime
Giulio Benetti Jan. 11, 2021, 5:37 p.m. UTC | #5
On 1/11/21 6:20 PM, Maxime Ripard wrote:
> On Fri, Jan 08, 2021 at 03:34:52PM +0100, Giulio Benetti wrote:
>> Hi,
>>
>> On 1/8/21 10:23 AM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> Thanks for those patches
>>>
>>> On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote:
>>>> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>
>>>> It turned out(Maxime suggestion) that bit 26 of SUN4I_TCON0_IO_POL_REG is
>>>> dedicated to invert DCLK polarity and this makes thing really easier than
>>>> before. So let's handle DCLK polarity by adding
>>>> SUN4I_TCON0_IO_POL_DCLK_POSITIVE as bit 26 and activating according to
>>>> bus_flags the same way is done for all the other signals.
>>>>
>>>> Cc: Maxime Ripard <maxime@cerno.tech>
>>>
>>> Suggested-by would be nice here :)
>>
>> Ok, didn't know about this tag
>>
>>>> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>> ---
>>>>    drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +-------------------
>>>>    drivers/gpu/drm/sun4i/sun4i_tcon.h |  1 +
>>>>    2 files changed, 2 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> index 52598bb0fb0b..30171ccd87e5 100644
>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>> @@ -569,26 +569,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>>>>    	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
>>>>    		val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
>>>> -	/*
>>>> -	 * On A20 and similar SoCs, the only way to achieve Positive Edge
>>>> -	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
>>>> -	 * By default TCON works in Negative Edge(Falling Edge),
>>>> -	 * this is why phase is set to 0 in that case.
>>>> -	 * Unfortunately there's no way to logically invert dclk through
>>>> -	 * IO_POL register.
>>>> -	 * The only acceptable way to work, triple checked with scope,
>>>> -	 * is using clock phase set to 0° for Negative Edge and set to 240°
>>>> -	 * for Positive Edge.
>>>> -	 * On A33 and similar SoCs there would be a 90° phase option,
>>>> -	 * but it divides also dclk by 2.
>>>> -	 * Following code is a way to avoid quirks all around TCON
>>>> -	 * and DOTCLOCK drivers.
>>>> -	 */
>>>>    	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
>>>> -		clk_set_phase(tcon->dclk, 0);
>>>> -
>>>> -	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>>>> -		clk_set_phase(tcon->dclk, 240);
>>>> +		val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;
>>>
>>> I'm not really sure why we need the first patch of this series here?
>>
>> The idea was to have 2 for testing, 1st one is already applicable, while the
>> other must be tested, but I can send only one with no problem.
>>
>>> That patch only seem to undo what you did in patch 1
>>
>> No, it doesn't, the 2nd one change the way it achieve the same thing,
>> because the 1st swap DCLK phase, while the 2nd uses the IO_POL bit to set IO
>> polarity according to bus_flags.
> 
> It makes sense for testing, but I'm not sure we want to carry it into
> the history. Can you squash them both into the same patch?
Sure, I'm going to send V3 then.

Thank you
Best regards
diff mbox series

Patch

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 52598bb0fb0b..30171ccd87e5 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -569,26 +569,8 @@  static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
 		val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
 
-	/*
-	 * On A20 and similar SoCs, the only way to achieve Positive Edge
-	 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
-	 * By default TCON works in Negative Edge(Falling Edge),
-	 * this is why phase is set to 0 in that case.
-	 * Unfortunately there's no way to logically invert dclk through
-	 * IO_POL register.
-	 * The only acceptable way to work, triple checked with scope,
-	 * is using clock phase set to 0° for Negative Edge and set to 240°
-	 * for Positive Edge.
-	 * On A33 and similar SoCs there would be a 90° phase option,
-	 * but it divides also dclk by 2.
-	 * Following code is a way to avoid quirks all around TCON
-	 * and DOTCLOCK drivers.
-	 */
 	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
-		clk_set_phase(tcon->dclk, 0);
-
-	if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
-		clk_set_phase(tcon->dclk, 240);
+		val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;
 
 	regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
 			   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index cfbf4e6c1679..0ce71d10a31b 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -113,6 +113,7 @@ 
 #define SUN4I_TCON0_IO_POL_REG			0x88
 #define SUN4I_TCON0_IO_POL_DCLK_PHASE(phase)		((phase & 3) << 28)
 #define SUN4I_TCON0_IO_POL_DE_NEGATIVE			BIT(27)
+#define SUN4I_TCON0_IO_POL_DCLK_POSITIVE		BIT(26)
 #define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE		BIT(25)
 #define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE		BIT(24)