All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
@ 2022-05-23 13:01 ` Jonathan Liu
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Liu @ 2022-05-23 13:01 UTC (permalink / raw)
  To: dri-devel
  Cc: Marek Vasut, Maxime Ripard, Jonathan Liu, Jagan Teki,
	Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	linux-kernel

The code from [1] sets SYS_CTRL_1 to different values depending on the
desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
positive edge of the clock with the pixel data while other values delay
the clock by a fraction of the clock period. A clock phase of 1/2 aligns
the negative edge of the clock with the pixel data.

The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
aligning the positive edge of the clock with the pixel data. This won't
work correctly for panels that require aligning the negative edge of the
clock with the pixel data.

Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
present in bus_flags, otherwise adjust the clock phase to 1/2 as
appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.

[1] https://github.com/tdjastrzebski/ICN6211-Configurator

Signed-off-by: Jonathan Liu <net147@gmail.com>
---
V2: Use GENMASK and FIELD_PREP macros
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 47dea657a752..f1538fb5f8a9 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -9,6 +9,8 @@
 #include <drm/drm_print.h>
 #include <drm/drm_mipi_dsi.h>
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -26,6 +28,11 @@
 #define PD_CTRL(n)		(0x0a + ((n) & 0x3)) /* 0..3 */
 #define RST_CTRL(n)		(0x0e + ((n) & 0x1)) /* 0..1 */
 #define SYS_CTRL(n)		(0x10 + ((n) & 0x7)) /* 0..4 */
+#define SYS_CTRL_1_CLK_PHASE_MSK	GENMASK(5, 4)
+#define CLK_PHASE_0			0
+#define CLK_PHASE_1_4			1
+#define CLK_PHASE_1_2			2
+#define CLK_PHASE_3_4			3
 #define RGB_DRV(n)		(0x18 + ((n) & 0x3)) /* 0..3 */
 #define RGB_DLY(n)		(0x1c + ((n) & 0x1)) /* 0..1 */
 #define RGB_TEST_CTRL		0x1e
@@ -336,7 +343,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	const struct drm_bridge_state *bridge_state;
 	u16 hfp, hbp, hsync;
 	u32 bus_flags;
-	u8 pol, id[4];
+	u8 pol, sys_ctrl_1, id[4];
 
 	chipone_readb(icn, VENDOR_ID, id);
 	chipone_readb(icn, DEVICE_ID_H, id + 1);
@@ -414,7 +421,14 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	chipone_configure_pll(icn, mode);
 
 	chipone_writeb(icn, SYS_CTRL(0), 0x40);
-	chipone_writeb(icn, SYS_CTRL(1), 0x88);
+	sys_ctrl_1 = 0x88;
+
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
+		sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, CLK_PHASE_0);
+	else
+		sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, CLK_PHASE_1_2);
+
+	chipone_writeb(icn, SYS_CTRL(1), sys_ctrl_1);
 
 	/* icn6211 specific sequence */
 	chipone_writeb(icn, MIPI_FORCE_0, 0x20);
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
@ 2022-05-23 13:01 ` Jonathan Liu
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Liu @ 2022-05-23 13:01 UTC (permalink / raw)
  To: dri-devel
  Cc: Marek Vasut, Jonathan Liu, Jernej Skrabec, Laurent Pinchart,
	Neil Armstrong, David Airlie, Jonas Karlman, linux-kernel,
	Robert Foss, Maxime Ripard, Andrzej Hajda, Jagan Teki

The code from [1] sets SYS_CTRL_1 to different values depending on the
desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
positive edge of the clock with the pixel data while other values delay
the clock by a fraction of the clock period. A clock phase of 1/2 aligns
the negative edge of the clock with the pixel data.

The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
aligning the positive edge of the clock with the pixel data. This won't
work correctly for panels that require aligning the negative edge of the
clock with the pixel data.

Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
present in bus_flags, otherwise adjust the clock phase to 1/2 as
appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.

[1] https://github.com/tdjastrzebski/ICN6211-Configurator

Signed-off-by: Jonathan Liu <net147@gmail.com>
---
V2: Use GENMASK and FIELD_PREP macros
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 47dea657a752..f1538fb5f8a9 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -9,6 +9,8 @@
 #include <drm/drm_print.h>
 #include <drm/drm_mipi_dsi.h>
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -26,6 +28,11 @@
 #define PD_CTRL(n)		(0x0a + ((n) & 0x3)) /* 0..3 */
 #define RST_CTRL(n)		(0x0e + ((n) & 0x1)) /* 0..1 */
 #define SYS_CTRL(n)		(0x10 + ((n) & 0x7)) /* 0..4 */
+#define SYS_CTRL_1_CLK_PHASE_MSK	GENMASK(5, 4)
+#define CLK_PHASE_0			0
+#define CLK_PHASE_1_4			1
+#define CLK_PHASE_1_2			2
+#define CLK_PHASE_3_4			3
 #define RGB_DRV(n)		(0x18 + ((n) & 0x3)) /* 0..3 */
 #define RGB_DLY(n)		(0x1c + ((n) & 0x1)) /* 0..1 */
 #define RGB_TEST_CTRL		0x1e
@@ -336,7 +343,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	const struct drm_bridge_state *bridge_state;
 	u16 hfp, hbp, hsync;
 	u32 bus_flags;
-	u8 pol, id[4];
+	u8 pol, sys_ctrl_1, id[4];
 
 	chipone_readb(icn, VENDOR_ID, id);
 	chipone_readb(icn, DEVICE_ID_H, id + 1);
@@ -414,7 +421,14 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
 	chipone_configure_pll(icn, mode);
 
 	chipone_writeb(icn, SYS_CTRL(0), 0x40);
-	chipone_writeb(icn, SYS_CTRL(1), 0x88);
+	sys_ctrl_1 = 0x88;
+
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
+		sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, CLK_PHASE_0);
+	else
+		sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, CLK_PHASE_1_2);
+
+	chipone_writeb(icn, SYS_CTRL(1), sys_ctrl_1);
 
 	/* icn6211 specific sequence */
 	chipone_writeb(icn, MIPI_FORCE_0, 0x20);
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
  2022-05-23 13:01 ` Jonathan Liu
@ 2022-05-23 13:15   ` Marek Vasut
  -1 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2022-05-23 13:15 UTC (permalink / raw)
  To: Jonathan Liu, dri-devel
  Cc: Jagan Teki, Jernej Skrabec, Neil Armstrong, David Airlie,
	Jonas Karlman, linux-kernel, Robert Foss, Laurent Pinchart,
	Andrzej Hajda, Maxime Ripard

On 5/23/22 15:01, Jonathan Liu wrote:
> The code from [1] sets SYS_CTRL_1 to different values depending on the
> desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
> positive edge of the clock with the pixel data while other values delay
> the clock by a fraction of the clock period. A clock phase of 1/2 aligns
> the negative edge of the clock with the pixel data.
> 
> The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
> aligning the positive edge of the clock with the pixel data. This won't
> work correctly for panels that require aligning the negative edge of the
> clock with the pixel data.
> 
> Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
> present in bus_flags, otherwise adjust the clock phase to 1/2 as
> appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
> 
> [1] https://github.com/tdjastrzebski/ICN6211-Configurator
> 
> Signed-off-by: Jonathan Liu <net147@gmail.com>
> ---
> V2: Use GENMASK and FIELD_PREP macros
> ---
>   drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 47dea657a752..f1538fb5f8a9 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -9,6 +9,8 @@
>   #include <drm/drm_print.h>
>   #include <drm/drm_mipi_dsi.h>
>   
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>   #include <linux/delay.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
> @@ -26,6 +28,11 @@
>   #define PD_CTRL(n)		(0x0a + ((n) & 0x3)) /* 0..3 */
>   #define RST_CTRL(n)		(0x0e + ((n) & 0x1)) /* 0..1 */
>   #define SYS_CTRL(n)		(0x10 + ((n) & 0x7)) /* 0..4 */
> +#define SYS_CTRL_1_CLK_PHASE_MSK	GENMASK(5, 4)

This should be GENMASK(7, 6) , no ?

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
@ 2022-05-23 13:15   ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2022-05-23 13:15 UTC (permalink / raw)
  To: Jonathan Liu, dri-devel
  Cc: Maxime Ripard, Jagan Teki, Andrzej Hajda, Neil Armstrong,
	Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	David Airlie, Daniel Vetter, linux-kernel

On 5/23/22 15:01, Jonathan Liu wrote:
> The code from [1] sets SYS_CTRL_1 to different values depending on the
> desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
> positive edge of the clock with the pixel data while other values delay
> the clock by a fraction of the clock period. A clock phase of 1/2 aligns
> the negative edge of the clock with the pixel data.
> 
> The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
> aligning the positive edge of the clock with the pixel data. This won't
> work correctly for panels that require aligning the negative edge of the
> clock with the pixel data.
> 
> Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
> present in bus_flags, otherwise adjust the clock phase to 1/2 as
> appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
> 
> [1] https://github.com/tdjastrzebski/ICN6211-Configurator
> 
> Signed-off-by: Jonathan Liu <net147@gmail.com>
> ---
> V2: Use GENMASK and FIELD_PREP macros
> ---
>   drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 47dea657a752..f1538fb5f8a9 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -9,6 +9,8 @@
>   #include <drm/drm_print.h>
>   #include <drm/drm_mipi_dsi.h>
>   
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>   #include <linux/delay.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
> @@ -26,6 +28,11 @@
>   #define PD_CTRL(n)		(0x0a + ((n) & 0x3)) /* 0..3 */
>   #define RST_CTRL(n)		(0x0e + ((n) & 0x1)) /* 0..1 */
>   #define SYS_CTRL(n)		(0x10 + ((n) & 0x7)) /* 0..4 */
> +#define SYS_CTRL_1_CLK_PHASE_MSK	GENMASK(5, 4)

This should be GENMASK(7, 6) , no ?

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
  2022-05-23 13:15   ` Marek Vasut
@ 2022-05-23 13:20     ` Jonathan Liu
  -1 siblings, 0 replies; 10+ messages in thread
From: Jonathan Liu @ 2022-05-23 13:20 UTC (permalink / raw)
  To: Marek Vasut
  Cc: dri-devel, Maxime Ripard, Jagan Teki, Andrzej Hajda,
	Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, David Airlie, Daniel Vetter, linux-kernel

Hi Marek,

On Mon, 23 May 2022 at 23:15, Marek Vasut <marex@denx.de> wrote:
>
> On 5/23/22 15:01, Jonathan Liu wrote:
> > The code from [1] sets SYS_CTRL_1 to different values depending on the
> > desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
> > positive edge of the clock with the pixel data while other values delay
> > the clock by a fraction of the clock period. A clock phase of 1/2 aligns
> > the negative edge of the clock with the pixel data.
> >
> > The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
> > aligning the positive edge of the clock with the pixel data. This won't
> > work correctly for panels that require aligning the negative edge of the
> > clock with the pixel data.
> >
> > Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
> > present in bus_flags, otherwise adjust the clock phase to 1/2 as
> > appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
> >
> > [1] https://github.com/tdjastrzebski/ICN6211-Configurator
> >
> > Signed-off-by: Jonathan Liu <net147@gmail.com>
> > ---
> > V2: Use GENMASK and FIELD_PREP macros
> > ---
> >   drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > index 47dea657a752..f1538fb5f8a9 100644
> > --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > @@ -9,6 +9,8 @@
> >   #include <drm/drm_print.h>
> >   #include <drm/drm_mipi_dsi.h>
> >
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> >   #include <linux/delay.h>
> >   #include <linux/gpio/consumer.h>
> >   #include <linux/i2c.h>
> > @@ -26,6 +28,11 @@
> >   #define PD_CTRL(n)          (0x0a + ((n) & 0x3)) /* 0..3 */
> >   #define RST_CTRL(n)         (0x0e + ((n) & 0x1)) /* 0..1 */
> >   #define SYS_CTRL(n)         (0x10 + ((n) & 0x7)) /* 0..4 */
> > +#define SYS_CTRL_1_CLK_PHASE_MSK     GENMASK(5, 4)
>
> This should be GENMASK(7, 6) , no ?

Clock phase 0 = 0b_1000_1000 = 0x88
Clock phase 1/4 = 0b_1001_1000 = 0x98
Clock phase 1/2 = 0b_1010_1000 = 0xA8
Clock phase 3/4 = 0b_1011_1000 = 0xB8

The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4
bits are unknown.

Regards,
Jonathan

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
@ 2022-05-23 13:20     ` Jonathan Liu
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Liu @ 2022-05-23 13:20 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Jernej Skrabec, Laurent Pinchart, Neil Armstrong, David Airlie,
	dri-devel, Jonas Karlman, linux-kernel, Robert Foss,
	Maxime Ripard, Andrzej Hajda, Jagan Teki

Hi Marek,

On Mon, 23 May 2022 at 23:15, Marek Vasut <marex@denx.de> wrote:
>
> On 5/23/22 15:01, Jonathan Liu wrote:
> > The code from [1] sets SYS_CTRL_1 to different values depending on the
> > desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
> > positive edge of the clock with the pixel data while other values delay
> > the clock by a fraction of the clock period. A clock phase of 1/2 aligns
> > the negative edge of the clock with the pixel data.
> >
> > The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
> > aligning the positive edge of the clock with the pixel data. This won't
> > work correctly for panels that require aligning the negative edge of the
> > clock with the pixel data.
> >
> > Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
> > present in bus_flags, otherwise adjust the clock phase to 1/2 as
> > appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
> >
> > [1] https://github.com/tdjastrzebski/ICN6211-Configurator
> >
> > Signed-off-by: Jonathan Liu <net147@gmail.com>
> > ---
> > V2: Use GENMASK and FIELD_PREP macros
> > ---
> >   drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > index 47dea657a752..f1538fb5f8a9 100644
> > --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > @@ -9,6 +9,8 @@
> >   #include <drm/drm_print.h>
> >   #include <drm/drm_mipi_dsi.h>
> >
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> >   #include <linux/delay.h>
> >   #include <linux/gpio/consumer.h>
> >   #include <linux/i2c.h>
> > @@ -26,6 +28,11 @@
> >   #define PD_CTRL(n)          (0x0a + ((n) & 0x3)) /* 0..3 */
> >   #define RST_CTRL(n)         (0x0e + ((n) & 0x1)) /* 0..1 */
> >   #define SYS_CTRL(n)         (0x10 + ((n) & 0x7)) /* 0..4 */
> > +#define SYS_CTRL_1_CLK_PHASE_MSK     GENMASK(5, 4)
>
> This should be GENMASK(7, 6) , no ?

Clock phase 0 = 0b_1000_1000 = 0x88
Clock phase 1/4 = 0b_1001_1000 = 0x98
Clock phase 1/2 = 0b_1010_1000 = 0xA8
Clock phase 3/4 = 0b_1011_1000 = 0xB8

The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4
bits are unknown.

Regards,
Jonathan

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
  2022-05-23 13:20     ` Jonathan Liu
@ 2022-05-23 13:25       ` Marek Vasut
  -1 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2022-05-23 13:25 UTC (permalink / raw)
  To: Jonathan Liu
  Cc: Jernej Skrabec, Laurent Pinchart, Neil Armstrong, David Airlie,
	dri-devel, Jonas Karlman, linux-kernel, Robert Foss,
	Maxime Ripard, Andrzej Hajda, Jagan Teki

On 5/23/22 15:20, Jonathan Liu wrote:
> Hi Marek,
> 
> On Mon, 23 May 2022 at 23:15, Marek Vasut <marex@denx.de> wrote:
>>
>> On 5/23/22 15:01, Jonathan Liu wrote:
>>> The code from [1] sets SYS_CTRL_1 to different values depending on the
>>> desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
>>> positive edge of the clock with the pixel data while other values delay
>>> the clock by a fraction of the clock period. A clock phase of 1/2 aligns
>>> the negative edge of the clock with the pixel data.
>>>
>>> The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
>>> aligning the positive edge of the clock with the pixel data. This won't
>>> work correctly for panels that require aligning the negative edge of the
>>> clock with the pixel data.
>>>
>>> Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
>>> present in bus_flags, otherwise adjust the clock phase to 1/2 as
>>> appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
>>>
>>> [1] https://github.com/tdjastrzebski/ICN6211-Configurator
>>>
>>> Signed-off-by: Jonathan Liu <net147@gmail.com>
>>> ---
>>> V2: Use GENMASK and FIELD_PREP macros
>>> ---
>>>    drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
>>>    1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
>>> index 47dea657a752..f1538fb5f8a9 100644
>>> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
>>> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
>>> @@ -9,6 +9,8 @@
>>>    #include <drm/drm_print.h>
>>>    #include <drm/drm_mipi_dsi.h>
>>>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bits.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/gpio/consumer.h>
>>>    #include <linux/i2c.h>
>>> @@ -26,6 +28,11 @@
>>>    #define PD_CTRL(n)          (0x0a + ((n) & 0x3)) /* 0..3 */
>>>    #define RST_CTRL(n)         (0x0e + ((n) & 0x1)) /* 0..1 */
>>>    #define SYS_CTRL(n)         (0x10 + ((n) & 0x7)) /* 0..4 */
>>> +#define SYS_CTRL_1_CLK_PHASE_MSK     GENMASK(5, 4)
>>
>> This should be GENMASK(7, 6) , no ?
> 
> Clock phase 0 = 0b_1000_1000 = 0x88
> Clock phase 1/4 = 0b_1001_1000 = 0x98
> Clock phase 1/2 = 0b_1010_1000 = 0xA8
> Clock phase 3/4 = 0b_1011_1000 = 0xB8
> 
> The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4
> bits are unknown.

Doh, you're right.

Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
@ 2022-05-23 13:25       ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2022-05-23 13:25 UTC (permalink / raw)
  To: Jonathan Liu
  Cc: dri-devel, Maxime Ripard, Jagan Teki, Andrzej Hajda,
	Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, David Airlie, Daniel Vetter, linux-kernel

On 5/23/22 15:20, Jonathan Liu wrote:
> Hi Marek,
> 
> On Mon, 23 May 2022 at 23:15, Marek Vasut <marex@denx.de> wrote:
>>
>> On 5/23/22 15:01, Jonathan Liu wrote:
>>> The code from [1] sets SYS_CTRL_1 to different values depending on the
>>> desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
>>> positive edge of the clock with the pixel data while other values delay
>>> the clock by a fraction of the clock period. A clock phase of 1/2 aligns
>>> the negative edge of the clock with the pixel data.
>>>
>>> The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
>>> aligning the positive edge of the clock with the pixel data. This won't
>>> work correctly for panels that require aligning the negative edge of the
>>> clock with the pixel data.
>>>
>>> Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
>>> present in bus_flags, otherwise adjust the clock phase to 1/2 as
>>> appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
>>>
>>> [1] https://github.com/tdjastrzebski/ICN6211-Configurator
>>>
>>> Signed-off-by: Jonathan Liu <net147@gmail.com>
>>> ---
>>> V2: Use GENMASK and FIELD_PREP macros
>>> ---
>>>    drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
>>>    1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
>>> index 47dea657a752..f1538fb5f8a9 100644
>>> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
>>> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
>>> @@ -9,6 +9,8 @@
>>>    #include <drm/drm_print.h>
>>>    #include <drm/drm_mipi_dsi.h>
>>>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bits.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/gpio/consumer.h>
>>>    #include <linux/i2c.h>
>>> @@ -26,6 +28,11 @@
>>>    #define PD_CTRL(n)          (0x0a + ((n) & 0x3)) /* 0..3 */
>>>    #define RST_CTRL(n)         (0x0e + ((n) & 0x1)) /* 0..1 */
>>>    #define SYS_CTRL(n)         (0x10 + ((n) & 0x7)) /* 0..4 */
>>> +#define SYS_CTRL_1_CLK_PHASE_MSK     GENMASK(5, 4)
>>
>> This should be GENMASK(7, 6) , no ?
> 
> Clock phase 0 = 0b_1000_1000 = 0x88
> Clock phase 1/4 = 0b_1001_1000 = 0x98
> Clock phase 1/2 = 0b_1010_1000 = 0xA8
> Clock phase 3/4 = 0b_1011_1000 = 0xB8
> 
> The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4
> bits are unknown.

Doh, you're right.

Reviewed-by: Marek Vasut <marex@denx.de>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
  2022-05-23 13:25       ` Marek Vasut
@ 2022-05-26 12:08         ` Robert Foss
  -1 siblings, 0 replies; 10+ messages in thread
From: Robert Foss @ 2022-05-26 12:08 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Jonathan Liu, dri-devel, Maxime Ripard, Jagan Teki,
	Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
	Jernej Skrabec, David Airlie, Daniel Vetter, linux-kernel

On Mon, 23 May 2022 at 15:25, Marek Vasut <marex@denx.de> wrote:
>
> On 5/23/22 15:20, Jonathan Liu wrote:
> > Hi Marek,
> >
> > On Mon, 23 May 2022 at 23:15, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 5/23/22 15:01, Jonathan Liu wrote:
> >>> The code from [1] sets SYS_CTRL_1 to different values depending on the
> >>> desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
> >>> positive edge of the clock with the pixel data while other values delay
> >>> the clock by a fraction of the clock period. A clock phase of 1/2 aligns
> >>> the negative edge of the clock with the pixel data.
> >>>
> >>> The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
> >>> aligning the positive edge of the clock with the pixel data. This won't
> >>> work correctly for panels that require aligning the negative edge of the
> >>> clock with the pixel data.
> >>>
> >>> Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
> >>> present in bus_flags, otherwise adjust the clock phase to 1/2 as
> >>> appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
> >>>
> >>> [1] https://github.com/tdjastrzebski/ICN6211-Configurator
> >>>
> >>> Signed-off-by: Jonathan Liu <net147@gmail.com>
> >>> ---
> >>> V2: Use GENMASK and FIELD_PREP macros
> >>> ---
> >>>    drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
> >>>    1 file changed, 16 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> >>> index 47dea657a752..f1538fb5f8a9 100644
> >>> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> >>> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> >>> @@ -9,6 +9,8 @@
> >>>    #include <drm/drm_print.h>
> >>>    #include <drm/drm_mipi_dsi.h>
> >>>
> >>> +#include <linux/bitfield.h>
> >>> +#include <linux/bits.h>
> >>>    #include <linux/delay.h>
> >>>    #include <linux/gpio/consumer.h>
> >>>    #include <linux/i2c.h>
> >>> @@ -26,6 +28,11 @@
> >>>    #define PD_CTRL(n)          (0x0a + ((n) & 0x3)) /* 0..3 */
> >>>    #define RST_CTRL(n)         (0x0e + ((n) & 0x1)) /* 0..1 */
> >>>    #define SYS_CTRL(n)         (0x10 + ((n) & 0x7)) /* 0..4 */
> >>> +#define SYS_CTRL_1_CLK_PHASE_MSK     GENMASK(5, 4)
> >>
> >> This should be GENMASK(7, 6) , no ?
> >
> > Clock phase 0 = 0b_1000_1000 = 0x88
> > Clock phase 1/4 = 0b_1001_1000 = 0x98
> > Clock phase 1/2 = 0b_1010_1000 = 0xA8
> > Clock phase 3/4 = 0b_1011_1000 = 0xB8
> >
> > The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4
> > bits are unknown.
>
> Doh, you're right.
>
> Reviewed-by: Marek Vasut <marex@denx.de>

Applied to drm-misc-next.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
@ 2022-05-26 12:08         ` Robert Foss
  0 siblings, 0 replies; 10+ messages in thread
From: Robert Foss @ 2022-05-26 12:08 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Jonathan Liu, Jernej Skrabec, Laurent Pinchart, Neil Armstrong,
	David Airlie, Jonas Karlman, linux-kernel, dri-devel,
	Maxime Ripard, Andrzej Hajda, Jagan Teki

On Mon, 23 May 2022 at 15:25, Marek Vasut <marex@denx.de> wrote:
>
> On 5/23/22 15:20, Jonathan Liu wrote:
> > Hi Marek,
> >
> > On Mon, 23 May 2022 at 23:15, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 5/23/22 15:01, Jonathan Liu wrote:
> >>> The code from [1] sets SYS_CTRL_1 to different values depending on the
> >>> desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
> >>> positive edge of the clock with the pixel data while other values delay
> >>> the clock by a fraction of the clock period. A clock phase of 1/2 aligns
> >>> the negative edge of the clock with the pixel data.
> >>>
> >>> The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
> >>> aligning the positive edge of the clock with the pixel data. This won't
> >>> work correctly for panels that require aligning the negative edge of the
> >>> clock with the pixel data.
> >>>
> >>> Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
> >>> present in bus_flags, otherwise adjust the clock phase to 1/2 as
> >>> appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
> >>>
> >>> [1] https://github.com/tdjastrzebski/ICN6211-Configurator
> >>>
> >>> Signed-off-by: Jonathan Liu <net147@gmail.com>
> >>> ---
> >>> V2: Use GENMASK and FIELD_PREP macros
> >>> ---
> >>>    drivers/gpu/drm/bridge/chipone-icn6211.c | 18 ++++++++++++++++--
> >>>    1 file changed, 16 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> >>> index 47dea657a752..f1538fb5f8a9 100644
> >>> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> >>> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> >>> @@ -9,6 +9,8 @@
> >>>    #include <drm/drm_print.h>
> >>>    #include <drm/drm_mipi_dsi.h>
> >>>
> >>> +#include <linux/bitfield.h>
> >>> +#include <linux/bits.h>
> >>>    #include <linux/delay.h>
> >>>    #include <linux/gpio/consumer.h>
> >>>    #include <linux/i2c.h>
> >>> @@ -26,6 +28,11 @@
> >>>    #define PD_CTRL(n)          (0x0a + ((n) & 0x3)) /* 0..3 */
> >>>    #define RST_CTRL(n)         (0x0e + ((n) & 0x1)) /* 0..1 */
> >>>    #define SYS_CTRL(n)         (0x10 + ((n) & 0x7)) /* 0..4 */
> >>> +#define SYS_CTRL_1_CLK_PHASE_MSK     GENMASK(5, 4)
> >>
> >> This should be GENMASK(7, 6) , no ?
> >
> > Clock phase 0 = 0b_1000_1000 = 0x88
> > Clock phase 1/4 = 0b_1001_1000 = 0x98
> > Clock phase 1/2 = 0b_1010_1000 = 0xA8
> > Clock phase 3/4 = 0b_1011_1000 = 0xB8
> >
> > The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4
> > bits are unknown.
>
> Doh, you're right.
>
> Reviewed-by: Marek Vasut <marex@denx.de>

Applied to drm-misc-next.

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-05-26 12:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-23 13:01 [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1 Jonathan Liu
2022-05-23 13:01 ` Jonathan Liu
2022-05-23 13:15 ` Marek Vasut
2022-05-23 13:15   ` Marek Vasut
2022-05-23 13:20   ` Jonathan Liu
2022-05-23 13:20     ` Jonathan Liu
2022-05-23 13:25     ` Marek Vasut
2022-05-23 13:25       ` Marek Vasut
2022-05-26 12:08       ` Robert Foss
2022-05-26 12:08         ` Robert Foss

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.