LinuxLists.cc - [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling

Hi,

Thanks for those patches

On Thu, Jan 07, 2021 at 03:30:32AM +0100, Giulio Benetti wrote:
> From: Giulio Benetti <[email protected]>
>
> 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 <[email protected]>

Suggested-by would be nice here :)

> Signed-off-by: Giulio Benetti <[email protected]>
> ---
> 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

2021-01-08 14:37:29

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling

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 <[email protected]>
>>
>> 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 <[email protected]>
>
> Suggested-by would be nice here :)

Ok, didn't know about this tag

>> Signed-off-by: Giulio Benetti <[email protected]>
>> ---
>> 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
Benetti Engineering sas

2021-01-08 14:49:56

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling

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<[email protected]>
>>>
>>> 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<[email protected]>
>> Suggested-by would be nice here :)
>>
>>> Signed-off-by: Giulio Benetti<[email protected]>
>>> ---
>>> 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

2021-01-11 17:23:55

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling

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 <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > Suggested-by would be nice here :)
>
> Ok, didn't know about this tag
>
> > > Signed-off-by: Giulio Benetti <[email protected]>
> > > ---
> > > 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

Attachments:

(No filename) (3.14 kB)
signature.asc (235.00 B)
Download all attachments

2021-01-11 17:41:02

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] drm/sun4i: tcon: improve DCLK polarity handling

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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>
>>> Suggested-by would be nice here :)
>>
>> Ok, didn't know about this tag
>>
>>>> Signed-off-by: Giulio Benetti <[email protected]>
>>>> ---
>>>> 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
--
Giulio Benetti
Benetti Engineering sas

2021-01-11 17:50:11

[permalink] [raw]

Subject: [PATCH v3] drm/sun4i: tcon: fix inverted DCLK polarity

From: Giulio Benetti <[email protected]>

During commit 88bc4178568b ("drm: Use new
DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
macros have been changed to avoid ambiguity but just because of this
ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
_SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
instead of swapping phase values, let's adopt an easier approach Maxime
suggested:
It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
invert DCLK polarity and this makes things 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 it is done for
all the other signals polarity.

Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
Suggested-by: Maxime Ripard <[email protected]>
Signed-off-by: Giulio Benetti <[email protected]>
---
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 eaaf5d70e352..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, 240);
-
- if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
- clk_set_phase(tcon->dclk, 0);
+ 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)

--
2.25.1

2021-01-13 09:45:19

[permalink] [raw]

Subject: Re: [PATCH v3] drm/sun4i: tcon: fix inverted DCLK polarity

Hi,

On Mon, Jan 11, 2021 at 06:46:16PM +0100, Giulio Benetti wrote:
> From: Giulio Benetti <[email protected]>
>
> During commit 88bc4178568b ("drm: Use new
> DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
> macros have been changed to avoid ambiguity but just because of this
> ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
> _SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
> instead of swapping phase values, let's adopt an easier approach Maxime
> suggested:
> It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
> invert DCLK polarity and this makes things 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 it is done for
> all the other signals polarity.
>
> Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
> Suggested-by: Maxime Ripard <[email protected]>
> Signed-off-by: Giulio Benetti <[email protected]>
> ---
> 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 eaaf5d70e352..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, 240);
> -
> - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> - clk_set_phase(tcon->dclk, 0);
> + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;
>
> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |

You need to add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to the mask you're
going to change here too

Maxime

Attachments:

(No filename) (2.82 kB)
signature.asc (235.00 B)
Download all attachments

2021-01-13 10:51:16

[permalink] [raw]

Subject: Re: [PATCH v3] drm/sun4i: tcon: fix inverted DCLK polarity

On 1/13/21 10:42 AM, Maxime Ripard wrote:
> Hi,
>
> On Mon, Jan 11, 2021 at 06:46:16PM +0100, Giulio Benetti wrote:
>> From: Giulio Benetti <[email protected]>
>>
>> During commit 88bc4178568b ("drm: Use new
>> DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
>> macros have been changed to avoid ambiguity but just because of this
>> ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
>> _SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
>> instead of swapping phase values, let's adopt an easier approach Maxime
>> suggested:
>> It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
>> invert DCLK polarity and this makes things 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 it is done for
>> all the other signals polarity.
>>
>> Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
>> Suggested-by: Maxime Ripard <[email protected]>
>> Signed-off-by: Giulio Benetti <[email protected]>
>> ---
>> 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 eaaf5d70e352..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, 240);
>> -
>> - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>> - clk_set_phase(tcon->dclk, 0);
>> + val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;
>>
>> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
>
> You need to add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to the mask you're
> going to change here too

Ah, lost it during squash, I send a v4.

Thank you
Best regards
--
Giulio Benetti
Benetti Engineering sas

2021-01-13 10:59:21

[permalink] [raw]

Subject: [PATCH v4] drm/sun4i: tcon: fix inverted DCLK polarity

From: Giulio Benetti <[email protected]>

During commit 88bc4178568b ("drm: Use new
DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
macros have been changed to avoid ambiguity but just because of this
ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
_SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
instead of swapping phase values, let's adopt an easier approach Maxime
suggested:
It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
invert DCLK polarity and this makes things 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 it is done for
all the other signals polarity.

Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
Suggested-by: Maxime Ripard <[email protected]>
Signed-off-by: Giulio Benetti <[email protected]>
---
V2->V3:
- squash 2 patches into 1
V3->V4:
- add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to regmap_update_bits()
---
drivers/gpu/drm/sun4i/sun4i_tcon.c | 21 ++-------------------
drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index eaaf5d70e352..6e454d316852 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -569,30 +569,13 @@ 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, 240);
-
- if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
- clk_set_phase(tcon->dclk, 0);
+ val |= SUN4I_TCON0_IO_POL_DCLK_POSITIVE;

regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
+ SUN4I_TCON0_IO_POL_DCLK_POSITIVE |
SUN4I_TCON0_IO_POL_DE_NEGATIVE,
val);

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)

--
2.25.1

2021-01-13 16:09:28

[permalink] [raw]

Subject: [PATCH v5] drm/sun4i: tcon: fix inverted DCLK polarity

From: Giulio Benetti <[email protected]>

During commit 88bc4178568b ("drm: Use new
DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
macros have been changed to avoid ambiguity but just because of this
ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
_SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
instead of swapping phase values, let's adopt an easier approach Maxime
suggested:
It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
invert DCLK polarity and this makes things really easier than before. So
let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
as bit 26 and activating according to bus_flags the same way it is done
for all the other signals polarity.

Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
Suggested-by: Maxime Ripard <[email protected]>
Signed-off-by: Giulio Benetti <[email protected]>
---
V2->V3:
- squash 2 patches into 1
V3->V4:
- add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to regmap_update_bits()
V4->V5:
polarity is still wrong so:
- let's use SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE macro
instead of _DCLK_POSITIVE(that would make sense only in realtion with DCLK)
- invert condition using _NEGEDGE instead of _POSEDGE and then matching with
register bit SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
- correct commit log according to V4->V5 changes
---
drivers/gpu/drm/sun4i/sun4i_tcon.c | 21 ++-------------------
drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index eaaf5d70e352..c172ccfff7e5 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -569,30 +569,13 @@ 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, 240);
-
if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
- clk_set_phase(tcon->dclk, 0);
+ val |= SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE;

regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
+ SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGDGE |
SUN4I_TCON0_IO_POL_DE_NEGATIVE,
val);

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index cfbf4e6c1679..c5ac1b02482c 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_DRIVE_NEGEDGE BIT(26)
#define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE BIT(25)
#define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE BIT(24)

--
2.25.1

2021-01-14 01:43:52

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v5] drm/sun4i: tcon: fix inverted DCLK polarity

Hi Giulio,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sunxi/sunxi/for-next]
[also build test ERROR on drm-exynos/exynos-drm-next drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip v5.11-rc3 next-20210113]
[cannot apply to mripard/sunxi/for-next drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Giulio-Benetti/drm-sun4i-tcon-fix-inverted-DCLK-polarity/20210114-001027
base: https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
config: arm64-randconfig-r005-20210113 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/acbc865cfd67e02cb2e4a37dfd56598eeae38287
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Giulio-Benetti/drm-sun4i-tcon-fix-inverted-DCLK-polarity/20210114-001027
git checkout acbc865cfd67e02cb2e4a37dfd56598eeae38287
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/gpu/drm/sun4i/sun4i_tcon.c: In function 'sun4i_tcon0_mode_set_rgb':
>> drivers/gpu/drm/sun4i/sun4i_tcon.c:578:7: error: 'SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGDGE' undeclared (first use in this function); did you mean 'SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE'?
578 | SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGDGE |
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
drivers/gpu/drm/sun4i/sun4i_tcon.c:578:7: note: each undeclared identifier is reported only once for each function it appears in

vim +578 drivers/gpu/drm/sun4i/sun4i_tcon.c

502
503 static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
504 const struct drm_encoder *encoder,
505 const struct drm_display_mode *mode)
506 {
507 struct drm_connector *connector = sun4i_tcon_get_connector(encoder);
508 const struct drm_display_info *info = &connector->display_info;
509 unsigned int bp, hsync, vsync;
510 u8 clk_delay;
511 u32 val = 0;
512
513 WARN_ON(!tcon->quirks->has_channel_0);
514
515 tcon->dclk_min_div = tcon->quirks->dclk_min_div;
516 tcon->dclk_max_div = 127;
517 sun4i_tcon0_mode_set_common(tcon, mode);
518
519 /* Set dithering if needed */
520 sun4i_tcon0_mode_set_dithering(tcon, connector);
521
522 /* Adjust clock delay */
523 clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
524 regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
525 SUN4I_TCON0_CTL_CLK_DELAY_MASK,
526 SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
527
528 /*
529 * This is called a backporch in the register documentation,
530 * but it really is the back porch + hsync
531 */
532 bp = mode->crtc_htotal - mode->crtc_hsync_start;
533 DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
534 mode->crtc_htotal, bp);
535
536 /* Set horizontal display timings */
537 regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
538 SUN4I_TCON0_BASIC1_H_TOTAL(mode->crtc_htotal) |
539 SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
540
541 /*
542 * This is called a backporch in the register documentation,
543 * but it really is the back porch + hsync
544 */
545 bp = mode->crtc_vtotal - mode->crtc_vsync_start;
546 DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
547 mode->crtc_vtotal, bp);
548
549 /* Set vertical display timings */
550 regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
551 SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
552 SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
553
554 /* Set Hsync and Vsync length */
555 hsync = mode->crtc_hsync_end - mode->crtc_hsync_start;
556 vsync = mode->crtc_vsync_end - mode->crtc_vsync_start;
557 DRM_DEBUG_DRIVER("Setting HSYNC %d, VSYNC %d\n", hsync, vsync);
558 regmap_write(tcon->regs, SUN4I_TCON0_BASIC3_REG,
559 SUN4I_TCON0_BASIC3_V_SYNC(vsync) |
560 SUN4I_TCON0_BASIC3_H_SYNC(hsync));
561
562 /* Setup the polarity of the various signals */
563 if (mode->flags & DRM_MODE_FLAG_PHSYNC)
564 val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
565
566 if (mode->flags & DRM_MODE_FLAG_PVSYNC)
567 val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
568
569 if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
570 val |= SUN4I_TCON0_IO_POL_DE_NEGATIVE;
571
572 if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
573 val |= SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE;
574
575 regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
576 SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
577 SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
> 578 SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGDGE |
579 SUN4I_TCON0_IO_POL_DE_NEGATIVE,
580 val);
581
582 /* Map output pins to channel 0 */
583 regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
584 SUN4I_TCON_GCTL_IOMAP_MASK,
585 SUN4I_TCON_GCTL_IOMAP_TCON0);
586
587 /* Enable the output on the pins */
588 regmap_write(tcon->regs, SUN4I_TCON0_IO_TRI_REG, 0);
589 }
590

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (5.90 kB)
.config.gz (35.33 kB)
Download all attachments

2021-01-14 08:11:10

[permalink] [raw]

Subject: Re: [PATCH v5] drm/sun4i: tcon: fix inverted DCLK polarity

Hi Giulio,

You did a typo

Il 13/01/2021 17:05, Giulio Benetti ha scritto:
> From: Giulio Benetti <[email protected]>
>
> During commit 88bc4178568b ("drm: Use new
> DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
> macros have been changed to avoid ambiguity but just because of this
> ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
> _SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
> instead of swapping phase values, let's adopt an easier approach Maxime
> suggested:
> It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
> invert DCLK polarity and this makes things really easier than before. So
> let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
> as bit 26 and activating according to bus_flags the same way it is done
> for all the other signals polarity.
>
> Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
> Suggested-by: Maxime Ripard <[email protected]>
> Signed-off-by: Giulio Benetti <[email protected]>
> ---
> V2->V3:
> - squash 2 patches into 1
> V3->V4:
> - add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to regmap_update_bits()
> V4->V5:
> polarity is still wrong so:
> - let's use SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE macro
> instead of _DCLK_POSITIVE(that would make sense only in realtion with DCLK)
> - invert condition using _NEGEDGE instead of _POSEDGE and then matching with
> register bit SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
> - correct commit log according to V4->V5 changes
> ---
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 21 ++-------------------
> drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> 2 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index eaaf5d70e352..c172ccfff7e5 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -569,30 +569,13 @@ 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, 240);
> -
> if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> - clk_set_phase(tcon->dclk, 0);
> + val |= SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE;
>
> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
Here Below you missed an 'E'
> + SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGDGE |
> SUN4I_TCON0_IO_POL_DE_NEGATIVE,
> val);
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index cfbf4e6c1679..c5ac1b02482c 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_DRIVE_NEGEDGE BIT(26)
> #define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE BIT(25)
> #define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE BIT(24)
>

2021-01-14 08:14:43

[permalink] [raw]

Subject: Re: [PATCH v5] drm/sun4i: tcon: fix inverted DCLK polarity

Hi Marjan,

On 1/14/21 8:58 AM, Marjan Pascolo wrote:
> Hi Giulio,
>
> You did a typo
>
> Il 13/01/2021 17:05, Giulio Benetti ha scritto:
>> From: Giulio Benetti <[email protected]>
>>
>> During commit 88bc4178568b ("drm: Use new
>> DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
>> macros have been changed to avoid ambiguity but just because of this
>> ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
>> _SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
>> instead of swapping phase values, let's adopt an easier approach Maxime
>> suggested:
>> It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
>> invert DCLK polarity and this makes things really easier than before. So
>> let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
>> as bit 26 and activating according to bus_flags the same way it is done
>> for all the other signals polarity.
>>
>> Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
>> Suggested-by: Maxime Ripard <[email protected]>
>> Signed-off-by: Giulio Benetti <[email protected]>
>> ---
>> V2->V3:
>> - squash 2 patches into 1
>> V3->V4:
>> - add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to regmap_update_bits()
>> V4->V5:
>> polarity is still wrong so:
>> - let's use SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE macro
>> instead of _DCLK_POSITIVE(that would make sense only in realtion with DCLK)
>> - invert condition using _NEGEDGE instead of _POSEDGE and then matching with
>> register bit SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
>> - correct commit log according to V4->V5 changes
>> ---
>> drivers/gpu/drm/sun4i/sun4i_tcon.c | 21 ++-------------------
>> drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
>> 2 files changed, 3 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index eaaf5d70e352..c172ccfff7e5 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -569,30 +569,13 @@ 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, 240);
>> -
>> if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>> - clk_set_phase(tcon->dclk, 0);
>> + val |= SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE;
>>
>> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
>> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
>> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
> Here Below you missed an 'E'

yes, thank you, going to send v6.

Best regards
--
Giulio Benetti
Benetti Engineering sas

>> + SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGDGE |
>> SUN4I_TCON0_IO_POL_DE_NEGATIVE,
>> val);
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
>> index cfbf4e6c1679..c5ac1b02482c 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_DRIVE_NEGEDGE BIT(26)
>> #define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE BIT(25)
>> #define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE BIT(24)
>>

2021-01-14 08:20:12

[permalink] [raw]

Subject: [PATCH v6] drm/sun4i: tcon: fix inverted DCLK polarity

From: Giulio Benetti <[email protected]>

During commit 88bc4178568b ("drm: Use new
DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
macros have been changed to avoid ambiguity but just because of this
ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
_SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
instead of swapping phase values, let's adopt an easier approach Maxime
suggested:
It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
invert DCLK polarity and this makes things really easier than before. So
let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
as bit 26 and activating according to bus_flags the same way it is done
for all the other signals polarity.

Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
Suggested-by: Maxime Ripard <[email protected]>
Signed-off-by: Giulio Benetti <[email protected]>
---
V2->V3:
- squash 2 patches into 1
V3->V4:
- add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to regmap_update_bits() as suggested by Maxime
V4->V5:
polarity is still wrong so:
- let's use SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE macro
instead of _DCLK_POSITIVE(that would make sense only in realtion with DCLK)
- invert condition using _NEGEDGE instead of _POSEDGE and then matching with
register bit SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
- correct commit log according to V4->V5 changes
V5->V6:
- fix typo in SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE as suggested by Marjan
---
drivers/gpu/drm/sun4i/sun4i_tcon.c | 21 ++-------------------
drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index eaaf5d70e352..6b9af4c08cd6 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -569,30 +569,13 @@ 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, 240);
-
if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
- clk_set_phase(tcon->dclk, 0);
+ val |= SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE;

regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
+ SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE |
SUN4I_TCON0_IO_POL_DE_NEGATIVE,
val);

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index cfbf4e6c1679..c5ac1b02482c 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_DRIVE_NEGEDGE BIT(26)
#define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE BIT(25)
#define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE BIT(24)

--
2.25.1

2021-01-14 11:43:33

[permalink] [raw]

Subject: Re: [PATCH v6] drm/sun4i: tcon: fix inverted DCLK polarity

Hello,

Tested on a13 and works.

Here are results:

with DRIVE_NEGEDGE

https://pasteboard.co/JJAGDAy.jpg

https://pasteboard.co/JJAHDAj.jpg

with DRIVE_POSEDGE

https://pasteboard.co/JJAIbBf.jpg

https://pasteboard.co/JJAIGfo.jpg

Il 14/01/2021 09:17, Giulio Benetti ha scritto:
> From: Giulio Benetti <[email protected]>
>
> During commit 88bc4178568b ("drm: Use new
> DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
> macros have been changed to avoid ambiguity but just because of this
> ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
> _SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
> instead of swapping phase values, let's adopt an easier approach Maxime
> suggested:
> It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
> invert DCLK polarity and this makes things really easier than before. So
> let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
> as bit 26 and activating according to bus_flags the same way it is done
> for all the other signals polarity.
>
> Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
> Suggested-by: Maxime Ripard <[email protected]>
> Signed-off-by: Giulio Benetti <[email protected]>

Tested-by: Marjan Pascolo <[email protected]>
[Tested successfully with A13]

> ---
> V2->V3:
> - squash 2 patches into 1
> V3->V4:
> - add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to regmap_update_bits() as suggested by Maxime
> V4->V5:
> polarity is still wrong so:
> - let's use SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE macro
> instead of _DCLK_POSITIVE(that would make sense only in realtion with DCLK)
> - invert condition using _NEGEDGE instead of _POSEDGE and then matching with
> register bit SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
> - correct commit log according to V4->V5 changes
> V5->V6:
> - fix typo in SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE as suggested by Marjan
> ---
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 21 ++-------------------
> drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> 2 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index eaaf5d70e352..6b9af4c08cd6 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -569,30 +569,13 @@ 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, 240);
> -
> if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> - clk_set_phase(tcon->dclk, 0);
> + val |= SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE;
>
> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
> + SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE |
> SUN4I_TCON0_IO_POL_DE_NEGATIVE,
> val);
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index cfbf4e6c1679..c5ac1b02482c 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_DRIVE_NEGEDGE BIT(26)
> #define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE BIT(25)
> #define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE BIT(24)
>

2021-01-14 11:44:52

[permalink] [raw]

Subject: Re: [PATCH v6] drm/sun4i: tcon: fix inverted DCLK polarity

On Thu, Jan 14, 2021 at 09:17:32AM +0100, Giulio Benetti wrote:
> From: Giulio Benetti <[email protected]>
>
> During commit 88bc4178568b ("drm: Use new
> DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags") DRM_BUS_FLAG_*
> macros have been changed to avoid ambiguity but just because of this
> ambiguity previous DRM_BUS_FLAG_PIXDATA_(POS/NEG)EDGE were used meaning
> _SAMPLE_ not _DRIVE_. This leads to DLCK inversion and need to fix but
> instead of swapping phase values, let's adopt an easier approach Maxime
> suggested:
> It turned out that bit 26 of SUN4I_TCON0_IO_POL_REG is dedicated to
> invert DCLK polarity and this makes things really easier than before. So
> let's handle DCLK polarity by adding SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
> as bit 26 and activating according to bus_flags the same way it is done
> for all the other signals polarity.
>
> Fixes: 88bc4178568b ("drm: Use new DRM_BUS_FLAG_*_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags")
> Suggested-by: Maxime Ripard <[email protected]>
> Signed-off-by: Giulio Benetti <[email protected]>
> ---
> V2->V3:
> - squash 2 patches into 1
> V3->V4:
> - add SUN4I_TCON0_IO_POL_DCLK_POSITIVE to regmap_update_bits() as suggested by Maxime
> V4->V5:
> polarity is still wrong so:
> - let's use SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE macro
> instead of _DCLK_POSITIVE(that would make sense only in realtion with DCLK)
> - invert condition using _NEGEDGE instead of _POSEDGE and then matching with
> register bit SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE
> - correct commit log according to V4->V5 changes
> V5->V6:
> - fix typo in SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE as suggested by Marjan
> ---
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 21 ++-------------------
> drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> 2 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index eaaf5d70e352..6b9af4c08cd6 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -569,30 +569,13 @@ 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, 240);
> -
> if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> - clk_set_phase(tcon->dclk, 0);
> + val |= SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE;
>
> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> SUN4I_TCON0_IO_POL_HSYNC_POSITIVE |
> SUN4I_TCON0_IO_POL_VSYNC_POSITIVE |
> + SUN4I_TCON0_IO_POL_DCLK_DRIVE_NEGEDGE |
> SUN4I_TCON0_IO_POL_DE_NEGATIVE,
> val);
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index cfbf4e6c1679..c5ac1b02482c 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_DRIVE_NEGEDGE BIT(26)
> #define SUN4I_TCON0_IO_POL_HSYNC_POSITIVE BIT(25)
> #define SUN4I_TCON0_IO_POL_VSYNC_POSITIVE BIT(24)
>
> --
> 2.25.1
>
Applied, thanks!
Maxime

Attachments:

(No filename) (4.08 kB)
signature.asc (235.00 B)
Download all attachments

2021-02-10 16:26:09

[permalink] [raw]

Subject: [PATCH] pinctrl/sunxi: adding input-debounce-ns property

On Allwinner SoC interrupt debounce can be controlled by two oscillator
(32KHz and 24MHz) and a prescale divider.
Oscillator and prescale divider are set through
device tree property "input-debounce" which have 1uS accuracy.
For acheive nS precision a new device tree poperty is made
named "input-debounce-ns".
"input-debounce-ns" is checked only if "input-debounce"
property is not defined.

Suggested-by: Maxime Ripard <[email protected]>
Signed-off-by: Marjan Pascolo <[email protected]>
---
---
?.../pinctrl/allwinner,sun4i-a10-pinctrl.yaml? |? 9 +++++++
?drivers/pinctrl/sunxi/pinctrl-sunxi.c???????? | 25 ++++++++++++++++---
?2 files changed, 30 insertions(+), 4 deletions(-)

diff --git
a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
index 5240487dfe50..346776de3a44 100644
---
a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
+++
b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
@@ -93,6 +93,15 @@ properties:
???? minItems: 1
???? maxItems: 5

+? input-debounce-ns:
+??? description:
+????? Debouncing periods in nanoseconds, one period per interrupt
+????? bank found in the controller.
+????? Only checked if input-debounce is not present
+??? $ref: /schemas/types.yaml#/definitions/uint32-array
+??? minItems: 1
+??? maxItems: 5
+
?patternProperties:
?? # It's pretty scary, but the basic idea is that:
?? #?? - One node name can start with either s- or r- for PRCM nodes,
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index dc8d39ae045b..869b6d5743ba 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -1335,14 +1335,31 @@ static int sunxi_pinctrl_setup_debounce(struct
sunxi_pinctrl *pctl,
???? struct clk *hosc, *losc;
???? u8 div, src;
???? int i, ret;
+??? /* Keeping for loop below clean */
+??? const char* debounce_prop_name;
+??? unsigned long debounce_dividend;

???? /* Deal with old DTs that didn't have the oscillators */
???? if (of_clk_get_parent_count(node) != 3)
???? ??? return 0;

+??? /*
+??? ?* Distinguish between simple input-debounce
+??? ?* and new input-debounce-ns
+??? ?*/
+
???? /* If we don't have any setup, bail out */
-??? if (!of_find_property(node, "input-debounce", NULL))
-??? ??? return 0;
+??? if (!of_find_property(node, "input-debounce", NULL)) {
+??? ??? if(!of_find_property(node, "input-debounce-ns", NULL)) {
+??? ??? ??? return 0;
+??? ??? } else {
+??? ??? ??? debounce_prop_name="input-debounce-ns";
+??? ??? ??? debounce_dividend=NSEC_PER_SEC;
+??? ??? }
+??? } else {
+??? ??? debounce_prop_name="input-debounce";
+??? ??? debounce_dividend=USEC_PER_SEC;
+??? }

???? losc = devm_clk_get(pctl->dev, "losc");
???? if (IS_ERR(losc))
@@ -1356,7 +1373,7 @@ static int sunxi_pinctrl_setup_debounce(struct
sunxi_pinctrl *pctl,
???? ??? unsigned long debounce_freq;
???? ??? u32 debounce;

-??? ??? ret = of_property_read_u32_index(node, "input-debounce",
+??? ??? ret = of_property_read_u32_index(node, debounce_prop_name,
???? ??? ??? ??? ??? ??? ?i, &debounce);
???? ??? if (ret)
???? ??? ??? return ret;
@@ -1364,7 +1381,7 @@ static int sunxi_pinctrl_setup_debounce(struct
sunxi_pinctrl *pctl,
???? ??? if (!debounce)
???? ??? ??? continue;

-??? ??? debounce_freq = DIV_ROUND_CLOSEST(USEC_PER_SEC, debounce);
+??? ??? debounce_freq = DIV_ROUND_CLOSEST(debounce_dividend, debounce);
???? ??? losc_div = sunxi_pinctrl_get_debounce_div(losc,
???? ??? ??? ??? ??? ??? ??? ? debounce_freq,
???? ??? ??? ??? ??? ??? ??? ? &losc_diff);
--
2.22.0.windows.1

2021-02-17 11:08:08

[permalink] [raw]

Subject: Re: [PATCH] pinctrl/sunxi: adding input-debounce-ns property

Hi,

On Wed, Feb 10, 2021 at 05:22:37PM +0100, Marjan Pascolo wrote:
> On Allwinner SoC interrupt debounce can be controlled by two oscillator
> (32KHz and 24MHz) and a prescale divider.
> Oscillator and prescale divider are set through
> device tree property "input-debounce" which have 1uS accuracy.
> For acheive nS precision a new device tree poperty is made
> named "input-debounce-ns".
> "input-debounce-ns" is checked only if "input-debounce"
> property is not defined.
>
> Suggested-by: Maxime Ripard <[email protected]>
> Signed-off-by: Marjan Pascolo <[email protected]>
> ---
> ---
> ?.../pinctrl/allwinner,sun4i-a10-pinctrl.yaml? |? 9 +++++++
> ?drivers/pinctrl/sunxi/pinctrl-sunxi.c???????? | 25 ++++++++++++++++---
> ?2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> index 5240487dfe50..346776de3a44 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> @@ -93,6 +93,15 @@ properties:
> ???? minItems: 1
> ???? maxItems: 5
>
> +? input-debounce-ns:
> +??? description:
> +????? Debouncing periods in nanoseconds, one period per interrupt
> +????? bank found in the controller.
> +????? Only checked if input-debounce is not present
> +??? $ref: /schemas/types.yaml#/definitions/uint32-array
> +??? minItems: 1
> +??? maxItems: 5
> +

This should be a separate patch, with the DT maintainers in Cc.

You should enforce that the properties are mutually exclusive through
the schema too

> ?patternProperties:
> ?? # It's pretty scary, but the basic idea is that:
> ?? #?? - One node name can start with either s- or r- for PRCM nodes,
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index dc8d39ae045b..869b6d5743ba 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -1335,14 +1335,31 @@ static int sunxi_pinctrl_setup_debounce(struct
> sunxi_pinctrl *pctl,
> ???? struct clk *hosc, *losc;
> ???? u8 div, src;
> ???? int i, ret;
> +??? /* Keeping for loop below clean */
> +??? const char* debounce_prop_name;
> +??? unsigned long debounce_dividend;
>
> ???? /* Deal with old DTs that didn't have the oscillators */
> ???? if (of_clk_get_parent_count(node) != 3)
> ???? ??? return 0;
>
> +??? /*
> +??? ?* Distinguish between simple input-debounce
> +??? ?* and new input-debounce-ns
> +??? ?*/
> +

I'm not sure that comment should stay, the code is obvious enough

> ???? /* If we don't have any setup, bail out */
> -??? if (!of_find_property(node, "input-debounce", NULL))
> -??? ??? return 0;
> +??? if (!of_find_property(node, "input-debounce", NULL)) {
> +??? ??? if(!of_find_property(node, "input-debounce-ns", NULL)) {
> +??? ??? ??? return 0;
> +??? ??? } else {
> +??? ??? ??? debounce_prop_name="input-debounce-ns";
> +??? ??? ??? debounce_dividend=NSEC_PER_SEC;
> +??? ??? }
> +??? } else {
> +??? ??? debounce_prop_name="input-debounce";
> +??? ??? debounce_dividend=USEC_PER_SEC;
> +??? }

This doesn't follow the kernel coding style, make sure to run
scripts/checkpatch.pl on your patches before sending them.

Maxime

Attachments:

(No filename) (3.38 kB)
signature.asc (235.00 B)
Download all attachments

2021-02-26 13:07:38

[permalink] [raw]

Subject: Re: [PATCH] pinctrl/sunxi: adding input-debounce-ns property

Hi Maxime,

Il 17/02/2021 12:03, Maxime Ripard ha scritto:
> Hi,
>
> On Wed, Feb 10, 2021 at 05:22:37PM +0100, Marjan Pascolo wrote:
>> On Allwinner SoC interrupt debounce can be controlled by two oscillator
>> (32KHz and 24MHz) and a prescale divider.
>> Oscillator and prescale divider are set through
>> device tree property "input-debounce" which have 1uS accuracy.
>> For acheive nS precision a new device tree poperty is made
>> named "input-debounce-ns".
>> "input-debounce-ns" is checked only if "input-debounce"
>> property is not defined.
>>
>> Suggested-by: Maxime Ripard <[email protected]>
>> Signed-off-by: Marjan Pascolo <[email protected]>
>> ---
>> ---
>> ?.../pinctrl/allwinner,sun4i-a10-pinctrl.yaml? |? 9 +++++++
>> ?drivers/pinctrl/sunxi/pinctrl-sunxi.c???????? | 25 ++++++++++++++++---
>> ?2 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
>> b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
>> index 5240487dfe50..346776de3a44 100644
>> ---
>> a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
>> +++
>> b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
>> @@ -93,6 +93,15 @@ properties:
>> ???? minItems: 1
>> ???? maxItems: 5
>>
>> +? input-debounce-ns:
>> +??? description:
>> +????? Debouncing periods in nanoseconds, one period per interrupt
>> +????? bank found in the controller.
>> +????? Only checked if input-debounce is not present
>> +??? $ref: /schemas/types.yaml#/definitions/uint32-array
>> +??? minItems: 1
>> +??? maxItems: 5
>> +
> This should be a separate patch, with the DT maintainers in Cc.
>
> You should enforce that the properties are mutually exclusive through
> the schema too

I'm sorry, I've ignored documentaion about /Documentation.

I see that some additional YAML operator (like oneOf) are used.

oneOf should fit the schema, but I can't understand if oneOf's options
must be a literal value, or if could also be a node.

Otherwise I'll use if ..then..else.

>
>> ?patternProperties:
>> ?? # It's pretty scary, but the basic idea is that:
>> ?? #?? - One node name can start with either s- or r- for PRCM nodes,
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index dc8d39ae045b..869b6d5743ba 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -1335,14 +1335,31 @@ static int sunxi_pinctrl_setup_debounce(struct
>> sunxi_pinctrl *pctl,
>> ???? struct clk *hosc, *losc;
>> ???? u8 div, src;
>> ???? int i, ret;
>> +??? /* Keeping for loop below clean */
>> +??? const char* debounce_prop_name;
>> +??? unsigned long debounce_dividend;
>>
>> ???? /* Deal with old DTs that didn't have the oscillators */
>> ???? if (of_clk_get_parent_count(node) != 3)
>> ???? ??? return 0;
>>
>> +??? /*
>> +??? ?* Distinguish between simple input-debounce
>> +??? ?* and new input-debounce-ns
>> +??? ?*/
>> +
> I'm not sure that comment should stay, the code is obvious enough
>
>> ???? /* If we don't have any setup, bail out */
>> -??? if (!of_find_property(node, "input-debounce", NULL))
>> -??? ??? return 0;
>> +??? if (!of_find_property(node, "input-debounce", NULL)) {
>> +??? ??? if(!of_find_property(node, "input-debounce-ns", NULL)) {
>> +??? ??? ??? return 0;
>> +??? ??? } else {
>> +??? ??? ??? debounce_prop_name="input-debounce-ns";
>> +??? ??? ??? debounce_dividend=NSEC_PER_SEC;
>> +??? ??? }
>> +??? } else {
>> +??? ??? debounce_prop_name="input-debounce";
>> +??? ??? debounce_dividend=USEC_PER_SEC;
>> +??? }
> This doesn't follow the kernel coding style, make sure to run
> scripts/checkpatch.pl on your patches before sending them.
Spaces between operators, right. After /Documentation submission I'll
resubmit this one
>
> Maxime

2021-03-04 18:02:33