* [PATCH 02/14] drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 03/14] drm: bridge: icn6211: Switch to atomic operations Marek Vasut
` (12 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
The HFP_HSW_HBP_HI register must be programmed with 2 LSbits of each
Horizontal Front Porch/Sync/Back Porch. Currently the driver programs
this register to 0, which breaks displays with either value above 255.
The HFP_MIN register must be set to the same value as HFP_LI, otherwise
there is visible image distortion, usually in the form of missing lines
at the bottom of the panel.
Fix this by correctly programming the HFP_HSW_HBP_HI and HFP_MIN registers.
Fixes: ce517f18944e3 ("drm: bridge: Add Chipone ICN6211 MIPI-DSI to RGB bridge")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index eb26615b2993a..d7eedf35e8415 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -34,6 +34,9 @@
#define HSYNC_LI 0x24
#define HBP_LI 0x25
#define HFP_HSW_HBP_HI 0x26
+#define HFP_HSW_HBP_HI_HFP(n) (((n) & 0x300) >> 4)
+#define HFP_HSW_HBP_HI_HS(n) (((n) & 0x300) >> 6)
+#define HFP_HSW_HBP_HI_HBP(n) (((n) & 0x300) >> 8)
#define VFP 0x27
#define VSYNC 0x28
#define VBP 0x29
@@ -165,6 +168,7 @@ static void chipone_enable(struct drm_bridge *bridge)
{
struct chipone *icn = bridge_to_chipone(bridge);
struct drm_display_mode *mode = bridge_to_mode(bridge);
+ u16 hfp, hbp, hsync;
ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
@@ -180,13 +184,18 @@ static void chipone_enable(struct drm_bridge *bridge)
((mode->hdisplay >> 8) & 0xf) |
(((mode->vdisplay >> 8) & 0xf) << 4));
- ICN6211_DSI(icn, HFP_LI, mode->hsync_start - mode->hdisplay);
+ hfp = mode->hsync_start - mode->hdisplay;
+ hsync = mode->hsync_end - mode->hsync_start;
+ hbp = mode->htotal - mode->hsync_end;
- ICN6211_DSI(icn, HSYNC_LI, mode->hsync_end - mode->hsync_start);
-
- ICN6211_DSI(icn, HBP_LI, mode->htotal - mode->hsync_end);
-
- ICN6211_DSI(icn, HFP_HSW_HBP_HI, 0x00);
+ ICN6211_DSI(icn, HFP_LI, hfp & 0xff);
+ ICN6211_DSI(icn, HSYNC_LI, hsync & 0xff);
+ ICN6211_DSI(icn, HBP_LI, hbp & 0xff);
+ /* Top two bits of Horizontal Front porch/Sync/Back porch */
+ ICN6211_DSI(icn, HFP_HSW_HBP_HI,
+ HFP_HSW_HBP_HI_HFP(hfp) |
+ HFP_HSW_HBP_HI_HS(hsync) |
+ HFP_HSW_HBP_HI_HBP(hbp));
ICN6211_DSI(icn, VFP, mode->vsync_start - mode->vdisplay);
@@ -196,7 +205,7 @@ static void chipone_enable(struct drm_bridge *bridge)
/* dsi specific sequence */
ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
- ICN6211_DSI(icn, HFP_MIN, 0x28);
+ ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
ICN6211_DSI(icn, BIST_POL, BIST_POL_DE_POL);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 03/14] drm: bridge: icn6211: Switch to atomic operations
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
2022-01-14 3:48 ` [PATCH 02/14] drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 04/14] drm: bridge: icn6211: Implement atomic_get_input_bus_fmts Marek Vasut
` (11 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
Use the atomic version of the enable/disable operations to continue
the transition to the atomic API, started with the introduction of
.atomic_get_input_bus_fmts(). This will be needed to access the mode
from the atomic state.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index d7eedf35e8415..dc909e7ceb958 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -4,6 +4,7 @@
* Author: Jagan Teki <jagan@amarulasolutions.com>
*/
+#include <drm/drm_atomic_helper.h>
#include <drm/drm_of.h>
#include <drm/drm_print.h>
#include <drm/drm_mipi_dsi.h>
@@ -164,7 +165,8 @@ static inline int chipone_dsi_write(struct chipone *icn, const void *seq,
chipone_dsi_write(icn, d, ARRAY_SIZE(d)); \
}
-static void chipone_enable(struct drm_bridge *bridge)
+static void chipone_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
struct drm_display_mode *mode = bridge_to_mode(bridge);
@@ -223,7 +225,8 @@ static void chipone_enable(struct drm_bridge *bridge)
usleep_range(10000, 11000);
}
-static void chipone_pre_enable(struct drm_bridge *bridge)
+static void chipone_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
int ret;
@@ -254,7 +257,8 @@ static void chipone_pre_enable(struct drm_bridge *bridge)
usleep_range(10000, 11000);
}
-static void chipone_post_disable(struct drm_bridge *bridge)
+static void chipone_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
@@ -279,9 +283,12 @@ static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flag
static const struct drm_bridge_funcs chipone_bridge_funcs = {
.attach = chipone_attach,
- .post_disable = chipone_post_disable,
- .pre_enable = chipone_pre_enable,
- .enable = chipone_enable,
+ .atomic_post_disable = chipone_atomic_post_disable,
+ .atomic_pre_enable = chipone_atomic_pre_enable,
+ .atomic_enable = chipone_atomic_enable,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
};
static int chipone_parse_dt(struct chipone *icn)
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 04/14] drm: bridge: icn6211: Implement atomic_get_input_bus_fmts
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
2022-01-14 3:48 ` [PATCH 02/14] drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling Marek Vasut
2022-01-14 3:48 ` [PATCH 03/14] drm: bridge: icn6211: Switch to atomic operations Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 05/14] drm: bridge: icn6211: Retrieve the display mode from the state Marek Vasut
` (10 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
Implement .atomic_get_input_bus_fmts callback, which sets up the
input (DSI-end) format, and that format can then be used in pipeline
format negotiation between the DSI-end of this bridge and the other
component closer to the scanout engine.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 27 ++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index dc909e7ceb958..14d28e7356aaa 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -281,6 +281,32 @@ static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flag
return drm_bridge_attach(bridge->encoder, icn->panel_bridge, bridge, flags);
}
+#define MAX_INPUT_SEL_FORMATS 1
+
+static u32 *
+chipone_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ u32 output_fmt,
+ unsigned int *num_input_fmts)
+{
+ u32 *input_fmts;
+
+ *num_input_fmts = 0;
+
+ input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
+ GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+
+ /* This is the DSI-end bus format */
+ input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+ *num_input_fmts = 1;
+
+ return input_fmts;
+}
+
static const struct drm_bridge_funcs chipone_bridge_funcs = {
.attach = chipone_attach,
.atomic_post_disable = chipone_atomic_post_disable,
@@ -289,6 +315,7 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
.atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_get_input_bus_fmts = chipone_atomic_get_input_bus_fmts,
};
static int chipone_parse_dt(struct chipone *icn)
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 05/14] drm: bridge: icn6211: Retrieve the display mode from the state
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (2 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 04/14] drm: bridge: icn6211: Implement atomic_get_input_bus_fmts Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-02-03 12:09 ` Maxime Ripard
2022-01-14 3:48 ` [PATCH 06/14] drm: bridge: icn6211: Add HS/VS/DE polarity handling Marek Vasut
` (9 subsequent siblings)
13 siblings, 1 reply; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
Retrieve display mode structure from panel or atomic state in
bridge_to_mode(). This completes the transition to the atomic
API.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 26 +++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 14d28e7356aaa..d6db1e77b5a35 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -146,9 +146,28 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
return container_of(bridge, struct chipone, bridge);
}
-static struct drm_display_mode *bridge_to_mode(struct drm_bridge *bridge)
+static const struct drm_display_mode *
+bridge_to_mode(struct drm_bridge *bridge, struct drm_atomic_state *state)
{
- return &bridge->encoder->crtc->state->adjusted_mode;
+ const struct drm_crtc_state *crtc_state;
+ struct drm_connector *connector;
+ struct drm_crtc *crtc;
+
+ /* Try to retrieve panel mode first. */
+ connector = drm_atomic_get_new_connector_for_encoder(state,
+ bridge->encoder);
+ if (!list_empty(&connector->modes)) {
+ return list_first_entry(&connector->modes,
+ struct drm_display_mode, head);
+ }
+
+ /*
+ * Retrieve the CRTC adjusted mode. This requires a little dance to go
+ * from the bridge to the encoder, to the connector and to the CRTC.
+ */
+ crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
+ crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ return &crtc_state->adjusted_mode;
}
static inline int chipone_dsi_write(struct chipone *icn, const void *seq,
@@ -169,7 +188,8 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
- struct drm_display_mode *mode = bridge_to_mode(bridge);
+ struct drm_atomic_state *state = old_bridge_state->base.state;
+ const struct drm_display_mode *mode = bridge_to_mode(bridge, state);
u16 hfp, hbp, hsync;
ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 05/14] drm: bridge: icn6211: Retrieve the display mode from the state
2022-01-14 3:48 ` [PATCH 05/14] drm: bridge: icn6211: Retrieve the display mode from the state Marek Vasut
@ 2022-02-03 12:09 ` Maxime Ripard
2022-02-16 20:13 ` Marek Vasut
0 siblings, 1 reply; 23+ messages in thread
From: Maxime Ripard @ 2022-02-03 12:09 UTC (permalink / raw)
To: Marek Vasut
Cc: Robert Foss, Sam Ravnborg, Thomas Zimmermann, dri-devel, Jagan Teki
[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]
On Fri, Jan 14, 2022 at 04:48:29AM +0100, Marek Vasut wrote:
> Retrieve display mode structure from panel or atomic state in
> bridge_to_mode(). This completes the transition to the atomic
> API.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/bridge/chipone-icn6211.c | 26 +++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 14d28e7356aaa..d6db1e77b5a35 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -146,9 +146,28 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> return container_of(bridge, struct chipone, bridge);
> }
>
> -static struct drm_display_mode *bridge_to_mode(struct drm_bridge *bridge)
> +static const struct drm_display_mode *
> +bridge_to_mode(struct drm_bridge *bridge, struct drm_atomic_state *state)
> {
> - return &bridge->encoder->crtc->state->adjusted_mode;
> + const struct drm_crtc_state *crtc_state;
> + struct drm_connector *connector;
> + struct drm_crtc *crtc;
> +
> + /* Try to retrieve panel mode first. */
> + connector = drm_atomic_get_new_connector_for_encoder(state,
> + bridge->encoder);
> + if (!list_empty(&connector->modes)) {
> + return list_first_entry(&connector->modes,
> + struct drm_display_mode, head);
> + }
If I understand this right, this will return the first mode on the
connector, which should be always set. So you always end up returning
the preferred mode for that panel?
> + /*
> + * Retrieve the CRTC adjusted mode. This requires a little dance to go
> + * from the bridge to the encoder, to the connector and to the CRTC.
> + */
> + crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + return &crtc_state->adjusted_mode;
And thus entirely disregarding the actual mode that was set by the
userspace, or ignoring any other mode than the preferred one?
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 05/14] drm: bridge: icn6211: Retrieve the display mode from the state
2022-02-03 12:09 ` Maxime Ripard
@ 2022-02-16 20:13 ` Marek Vasut
0 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-02-16 20:13 UTC (permalink / raw)
To: Maxime Ripard
Cc: Robert Foss, Sam Ravnborg, Thomas Zimmermann, dri-devel, Jagan Teki
On 2/3/22 13:09, Maxime Ripard wrote:
> On Fri, Jan 14, 2022 at 04:48:29AM +0100, Marek Vasut wrote:
>> Retrieve display mode structure from panel or atomic state in
>> bridge_to_mode(). This completes the transition to the atomic
>> API.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>> Cc: Robert Foss <robert.foss@linaro.org>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> To: dri-devel@lists.freedesktop.org
>> ---
>> drivers/gpu/drm/bridge/chipone-icn6211.c | 26 +++++++++++++++++++++---
>> 1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
>> index 14d28e7356aaa..d6db1e77b5a35 100644
>> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
>> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
>> @@ -146,9 +146,28 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
>> return container_of(bridge, struct chipone, bridge);
>> }
>>
>> -static struct drm_display_mode *bridge_to_mode(struct drm_bridge *bridge)
>> +static const struct drm_display_mode *
>> +bridge_to_mode(struct drm_bridge *bridge, struct drm_atomic_state *state)
>> {
>> - return &bridge->encoder->crtc->state->adjusted_mode;
>> + const struct drm_crtc_state *crtc_state;
>> + struct drm_connector *connector;
>> + struct drm_crtc *crtc;
>> +
>> + /* Try to retrieve panel mode first. */
>> + connector = drm_atomic_get_new_connector_for_encoder(state,
>> + bridge->encoder);
>> + if (!list_empty(&connector->modes)) {
>> + return list_first_entry(&connector->modes,
>> + struct drm_display_mode, head);
>> + }
>
> If I understand this right, this will return the first mode on the
> connector, which should be always set. So you always end up returning
> the preferred mode for that panel?
>
>> + /*
>> + * Retrieve the CRTC adjusted mode. This requires a little dance to go
>> + * from the bridge to the encoder, to the connector and to the CRTC.
>> + */
>> + crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
>> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> + return &crtc_state->adjusted_mode;
>
> And thus entirely disregarding the actual mode that was set by the
> userspace, or ignoring any other mode than the preferred one?
This one is actually no longer needed and can be dropped.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 06/14] drm: bridge: icn6211: Add HS/VS/DE polarity handling
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (3 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 05/14] drm: bridge: icn6211: Retrieve the display mode from the state Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 07/14] drm: bridge: icn6211: Add DSI lane count DT property parsing Marek Vasut
` (8 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
The driver currently hard-codes HS/VS polarity to active-low and DE to
active-high, which is not correct for a lot of supported DPI panels.
Add the missing mode flag handling for HS/VS/DE polarity.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index d6db1e77b5a35..3ad082c1d2bfd 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -190,7 +190,14 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
struct chipone *icn = bridge_to_chipone(bridge);
struct drm_atomic_state *state = old_bridge_state->base.state;
const struct drm_display_mode *mode = bridge_to_mode(bridge, state);
+ const struct drm_bridge_state *bridge_state;
u16 hfp, hbp, hsync;
+ u32 bus_flags;
+ u8 pol;
+
+ /* Get the DPI flags from the bridge state. */
+ bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+ bus_flags = bridge_state->output_bus_cfg.flags;
ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
@@ -230,7 +237,13 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
- ICN6211_DSI(icn, BIST_POL, BIST_POL_DE_POL);
+
+ /* DPI HS/VS/DE polarity */
+ pol = ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIST_POL_HSYNC_POL : 0) |
+ ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIST_POL_VSYNC_POL : 0) |
+ ((bus_flags & DRM_BUS_FLAG_DE_HIGH) ? BIST_POL_DE_POL : 0);
+ ICN6211_DSI(icn, BIST_POL, pol);
+
ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
ICN6211_DSI(icn, PLL_REF_DIV, 0x71);
ICN6211_DSI(icn, PLL_INT(0), 0x2b);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 07/14] drm: bridge: icn6211: Add DSI lane count DT property parsing
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (4 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 06/14] drm: bridge: icn6211: Add HS/VS/DE polarity handling Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-02-03 12:13 ` Maxime Ripard
2022-01-14 3:48 ` [PATCH 08/14] drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration Marek Vasut
` (7 subsequent siblings)
13 siblings, 1 reply; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
The driver currently hard-codes DSI lane count to two, however the chip
is capable of operating in 1..4 DSI lanes mode. Parse 'data-lanes' DT
property and program the result into DSI_CTRL register.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 35 ++++++++++++++++++++----
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 3ad082c1d2bfd..400a566026ab4 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -135,10 +135,12 @@ struct chipone {
struct device *dev;
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
+ struct device_node *host_node;
struct gpio_desc *enable_gpio;
struct regulator *vdd1;
struct regulator *vdd2;
struct regulator *vdd3;
+ int dsi_lanes;
};
static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
@@ -235,6 +237,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
/* dsi specific sequence */
ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
+
+ /* DSI data lane count */
+ ICN6211_DSI(icn, DSI_CTRL,
+ DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1));
+
ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
@@ -354,6 +361,8 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
static int chipone_parse_dt(struct chipone *icn)
{
struct device *dev = icn->dev;
+ struct drm_bridge *panel_bridge;
+ struct device_node *endpoint;
struct drm_panel *panel;
int ret;
@@ -390,13 +399,26 @@ static int chipone_parse_dt(struct chipone *icn)
return PTR_ERR(icn->enable_gpio);
}
- ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
- if (ret)
+ endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
+ icn->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
+ icn->host_node = of_graph_get_remote_port_parent(endpoint);
+ of_node_put(endpoint);
+
+ if (icn->dsi_lanes < 0 || icn->dsi_lanes > 4)
+ return -EINVAL;
+ if (!icn->host_node)
+ return -ENODEV;
+
+ ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &panel_bridge);
+ if (ret < 0)
return ret;
+ if (panel) {
+ panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+ if (IS_ERR(panel_bridge))
+ return PTR_ERR(panel_bridge);
+ }
- icn->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
- if (IS_ERR(icn->panel_bridge))
- return PTR_ERR(icn->panel_bridge);
+ icn->panel_bridge = panel_bridge;
return 0;
}
@@ -424,7 +446,7 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
drm_bridge_add(&icn->bridge);
- dsi->lanes = 4;
+ dsi->lanes = icn->dsi_lanes;
dsi->format = MIPI_DSI_FMT_RGB888;
dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
@@ -443,6 +465,7 @@ static int chipone_remove(struct mipi_dsi_device *dsi)
mipi_dsi_detach(dsi);
drm_bridge_remove(&icn->bridge);
+ of_node_put(icn->host_node);
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 07/14] drm: bridge: icn6211: Add DSI lane count DT property parsing
2022-01-14 3:48 ` [PATCH 07/14] drm: bridge: icn6211: Add DSI lane count DT property parsing Marek Vasut
@ 2022-02-03 12:13 ` Maxime Ripard
2022-02-16 20:24 ` Marek Vasut
0 siblings, 1 reply; 23+ messages in thread
From: Maxime Ripard @ 2022-02-03 12:13 UTC (permalink / raw)
To: Marek Vasut
Cc: Robert Foss, Sam Ravnborg, Thomas Zimmermann, dri-devel, Jagan Teki
[-- Attachment #1: Type: text/plain, Size: 3427 bytes --]
On Fri, Jan 14, 2022 at 04:48:31AM +0100, Marek Vasut wrote:
> The driver currently hard-codes DSI lane count to two, however the chip
> is capable of operating in 1..4 DSI lanes mode. Parse 'data-lanes' DT
> property and program the result into DSI_CTRL register.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/bridge/chipone-icn6211.c | 35 ++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 3ad082c1d2bfd..400a566026ab4 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -135,10 +135,12 @@ struct chipone {
> struct device *dev;
> struct drm_bridge bridge;
> struct drm_bridge *panel_bridge;
> + struct device_node *host_node;
> struct gpio_desc *enable_gpio;
> struct regulator *vdd1;
> struct regulator *vdd2;
> struct regulator *vdd3;
> + int dsi_lanes;
> };
>
> static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> @@ -235,6 +237,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
> /* dsi specific sequence */
> ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
> ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
> +
> + /* DSI data lane count */
> + ICN6211_DSI(icn, DSI_CTRL,
> + DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1));
> +
> ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
> ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
>
> @@ -354,6 +361,8 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
> static int chipone_parse_dt(struct chipone *icn)
> {
> struct device *dev = icn->dev;
> + struct drm_bridge *panel_bridge;
> + struct device_node *endpoint;
> struct drm_panel *panel;
> int ret;
>
> @@ -390,13 +399,26 @@ static int chipone_parse_dt(struct chipone *icn)
> return PTR_ERR(icn->enable_gpio);
> }
>
> - ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> - if (ret)
> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> + icn->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> + icn->host_node = of_graph_get_remote_port_parent(endpoint);
> + of_node_put(endpoint);
> +
> + if (icn->dsi_lanes < 0 || icn->dsi_lanes > 4)
> + return -EINVAL;
> + if (!icn->host_node)
> + return -ENODEV;
> +
> + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &panel_bridge);
> + if (ret < 0)
> return ret;
> + if (panel) {
> + panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> + if (IS_ERR(panel_bridge))
> + return PTR_ERR(panel_bridge);
> + }
>
> - icn->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> - if (IS_ERR(icn->panel_bridge))
> - return PTR_ERR(icn->panel_bridge);
> + icn->panel_bridge = panel_bridge;
It looks like you're doing more than what you said in the commit log
here? There's at least a change on the error condition for
drm_of_find_panel_or_bridge, some reworking of the drm_panel_bridge_add
call, plus the data-lanes property parsing you were mentioning.
All those should be separate patches
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 07/14] drm: bridge: icn6211: Add DSI lane count DT property parsing
2022-02-03 12:13 ` Maxime Ripard
@ 2022-02-16 20:24 ` Marek Vasut
0 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-02-16 20:24 UTC (permalink / raw)
To: Maxime Ripard
Cc: Robert Foss, Sam Ravnborg, Thomas Zimmermann, dri-devel, Jagan Teki
On 2/3/22 13:13, Maxime Ripard wrote:
> On Fri, Jan 14, 2022 at 04:48:31AM +0100, Marek Vasut wrote:
>> The driver currently hard-codes DSI lane count to two, however the chip
>> is capable of operating in 1..4 DSI lanes mode. Parse 'data-lanes' DT
>> property and program the result into DSI_CTRL register.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Jagan Teki <jagan@amarulasolutions.com>
>> Cc: Robert Foss <robert.foss@linaro.org>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> To: dri-devel@lists.freedesktop.org
>> ---
>> drivers/gpu/drm/bridge/chipone-icn6211.c | 35 ++++++++++++++++++++----
>> 1 file changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
>> index 3ad082c1d2bfd..400a566026ab4 100644
>> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
>> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
>> @@ -135,10 +135,12 @@ struct chipone {
>> struct device *dev;
>> struct drm_bridge bridge;
>> struct drm_bridge *panel_bridge;
>> + struct device_node *host_node;
>> struct gpio_desc *enable_gpio;
>> struct regulator *vdd1;
>> struct regulator *vdd2;
>> struct regulator *vdd3;
>> + int dsi_lanes;
>> };
>>
>> static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
>> @@ -235,6 +237,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
>> /* dsi specific sequence */
>> ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
>> ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
>> +
>> + /* DSI data lane count */
>> + ICN6211_DSI(icn, DSI_CTRL,
>> + DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1));
>> +
>> ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
>> ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
>>
>> @@ -354,6 +361,8 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = {
>> static int chipone_parse_dt(struct chipone *icn)
>> {
>> struct device *dev = icn->dev;
>> + struct drm_bridge *panel_bridge;
>> + struct device_node *endpoint;
>> struct drm_panel *panel;
>> int ret;
>>
>> @@ -390,13 +399,26 @@ static int chipone_parse_dt(struct chipone *icn)
>> return PTR_ERR(icn->enable_gpio);
>> }
>>
>> - ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
>> - if (ret)
>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>> + icn->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
>> + icn->host_node = of_graph_get_remote_port_parent(endpoint);
>> + of_node_put(endpoint);
>> +
>> + if (icn->dsi_lanes < 0 || icn->dsi_lanes > 4)
>> + return -EINVAL;
>> + if (!icn->host_node)
>> + return -ENODEV;
>> +
>> + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &panel_bridge);
>> + if (ret < 0)
>> return ret;
>> + if (panel) {
>> + panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>> + if (IS_ERR(panel_bridge))
>> + return PTR_ERR(panel_bridge);
>> + }
>>
>> - icn->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>> - if (IS_ERR(icn->panel_bridge))
>> - return PTR_ERR(icn->panel_bridge);
>> + icn->panel_bridge = panel_bridge;
>
> It looks like you're doing more than what you said in the commit log
> here? There's at least a change on the error condition for
> drm_of_find_panel_or_bridge, some reworking of the drm_panel_bridge_add
> call, plus the data-lanes property parsing you were mentioning.
>
> All those should be separate patches
Actually, I think I can drop this panel_bridge hunk too, which leaves
only the lane count in this patch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 08/14] drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (5 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 07/14] drm: bridge: icn6211: Add DSI lane count DT property parsing Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 09/14] drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode Marek Vasut
` (6 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
The chip contains fractional PLL, however the driver currently hard-codes
one specific PLL setting. Implement generic PLL parameter calculation code,
so any DPI panel with arbitrary pixel clock can be attached to this bridge.
The datasheet for this bridge is not available, the PLL behavior has been
inferred from [1] and [2] and by analyzing the DPI pixel clock with scope.
The PLL limits might be wrong, but at least the calculated values match all
the example code available. This is better than one hard-coded pixel clock
value anyway.
[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
[2] https://github.com/tdjastrzebski/ICN6211-Configurator
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 87 +++++++++++++++++++++++-
1 file changed, 84 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 400a566026ab4..aa58f957651e2 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -186,6 +186,87 @@ static inline int chipone_dsi_write(struct chipone *icn, const void *seq,
chipone_dsi_write(icn, d, ARRAY_SIZE(d)); \
}
+static void chipone_configure_pll(struct chipone *icn,
+ const struct drm_display_mode *mode)
+{
+ unsigned int best_p = 0, best_m = 0, best_s = 0;
+ unsigned int delta, min_delta = 0xffffffff;
+ unsigned int freq_p, freq_s, freq_out;
+ unsigned int p_min, p_max;
+ unsigned int p, m, s;
+ unsigned int fin;
+
+ /*
+ * DSI clock lane frequency (input into PLL) is calculated as:
+ * DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
+ * the 2 is there because the bus is DDR.
+ *
+ * DPI pixel clock frequency (output from PLL) is mode clock.
+ *
+ * The chip contains fractional PLL which works as follows:
+ * DPI_CLK = ((DSI_CLK / P) * M) / S
+ * P is pre-divider, register PLL_REF_DIV[3:0] is 2^(n+1) divider
+ * register PLL_REF_DIV[4] is extra 1:2 divider
+ * M is integer multiplier, register PLL_INT(0) is multiplier
+ * S is post-divider, register PLL_REF_DIV[7:5] is 2^(n+1) divider
+ *
+ * It seems the PLL input clock after applying P pre-divider have
+ * to be lower than 20 MHz.
+ */
+ fin = mode->clock * mipi_dsi_pixel_format_to_bpp(icn->dsi->format) /
+ icn->dsi_lanes / 2; /* in kHz */
+
+ /* Minimum value of P predivider for PLL input in 5..20 MHz */
+ p_min = ffs(fin / 20000);
+ p_max = (fls(fin / 5000) - 1) & 0x1f;
+
+ for (p = p_min; p < p_max; p++) { /* PLL_REF_DIV[4,3:0] */
+ freq_p = fin / BIT(p + 1);
+ if (freq_p == 0) /* Divider too high */
+ break;
+
+ for (s = 0; s < 0x7; s++) { /* PLL_REF_DIV[7:5] */
+ freq_s = freq_p / BIT(s + 1);
+ if (freq_s == 0) /* Divider too high */
+ break;
+
+ m = mode->clock / freq_s;
+
+ /* Multiplier is 8 bit */
+ if (m > 0xff)
+ continue;
+
+ /* Limit PLL VCO frequency to 1 GHz */
+ freq_out = (fin * m) / BIT(p + 1);
+ if (freq_out > 1000000)
+ continue;
+
+ /* Apply post-divider */
+ freq_out /= BIT(s + 1);
+
+ delta = abs(mode->clock - freq_out);
+ if (delta < min_delta) {
+ best_p = p;
+ best_m = m;
+ best_s = s;
+ min_delta = delta;
+ }
+ }
+ }
+
+ dev_dbg(icn->dev,
+ "PLL: P[3:0]=2^%d P[4]=2*%d M=%d S[7:5]=2^%d delta=%d => DSI f_in=%d kHz ; DPI f_out=%ld kHz\n",
+ best_p, !!best_p, best_m, best_s + 1, min_delta, fin,
+ (fin * best_m) / BIT(best_p + best_s + 2));
+
+ /* Clock source selection fixed to MIPI DSI clock lane */
+ ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
+ ICN6211_DSI(icn, PLL_REF_DIV,
+ (best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2 pre-divider */
+ PLL_REF_DIV_P(best_p) | PLL_REF_DIV_S(best_s));
+ ICN6211_DSI(icn, PLL_INT(0), best_m);
+}
+
static void chipone_atomic_enable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
@@ -251,9 +332,9 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
((bus_flags & DRM_BUS_FLAG_DE_HIGH) ? BIST_POL_DE_POL : 0);
ICN6211_DSI(icn, BIST_POL, pol);
- ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
- ICN6211_DSI(icn, PLL_REF_DIV, 0x71);
- ICN6211_DSI(icn, PLL_INT(0), 0x2b);
+ /* Configure PLL settings */
+ chipone_configure_pll(icn, mode);
+
ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
ICN6211_DSI(icn, SYS_CTRL(1), 0x98);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 09/14] drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (6 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 08/14] drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 10/14] drm: bridge: icn6211: Disable DPI color swap Marek Vasut
` (5 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
The DSI burst mode is more energy efficient than the DSI sync pulse mode,
make use of the burst mode since the chip supports it as well. Disable the
generation of EoT packet, the chip ignores it, so no point in emitting it.
Enable transmission of data in LP mode, otherwise register read via DSI
does not work with this chip.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index aa58f957651e2..6daf2f8013733 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -529,7 +529,8 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
dsi->lanes = icn->dsi_lanes;
dsi->format = MIPI_DSI_FMT_RGB888;
- dsi->mode_flags = MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+ dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+ MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
ret = mipi_dsi_attach(dsi);
if (ret < 0) {
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 10/14] drm: bridge: icn6211: Disable DPI color swap
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (7 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 09/14] drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 11/14] drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples Marek Vasut
` (4 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
The chip is capable of swapping DPI RGB channels. The driver currently
does not implement support for this functionality. Write the MIPI_PN_SWAP
register to 0 to assure the color swap is disabled.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 6daf2f8013733..e41e671fed46d 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -325,6 +325,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
+ ICN6211_DSI(icn, MIPI_PN_SWAP, 0x00);
/* DPI HS/VS/DE polarity */
pol = ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIST_POL_HSYNC_POL : 0) |
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 11/14] drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (8 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 10/14] drm: bridge: icn6211: Disable DPI color swap Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support Marek Vasut
` (3 subsequent siblings)
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
Both example code [1], [2] as well as one provided by custom panel vendor
set register SYS_CTRL_1 to 0x88. What exactly does the value mean is unknown
due to unavailable datasheet. Align this register value with example code.
[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
[2] https://github.com/tdjastrzebski/ICN6211-Configurator
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index e41e671fed46d..8226fefeedfc9 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -337,7 +337,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
chipone_configure_pll(icn, mode);
ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
- ICN6211_DSI(icn, SYS_CTRL(1), 0x98);
+ ICN6211_DSI(icn, SYS_CTRL(1), 0x88);
/* icn6211 specific sequence */
ICN6211_DSI(icn, MIPI_FORCE_0, 0x20);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (9 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 11/14] drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 10:24 ` kernel test robot
2022-02-03 12:17 ` Maxime Ripard
2022-01-14 3:48 ` [PATCH 13/14] drm: bridge: icn6211: Rename ICN6211_DSI to chipone_writeb Marek Vasut
` (2 subsequent siblings)
13 siblings, 2 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
The ICN6211 chip starts in I2C configuration mode after cold boot.
Implement support for configuring the chip via I2C in addition to
the current DSI LP command mode configuration support. The later
seems to be available only on chips which have additional MCU on
the panel/bridge board which preconfigures the ICN6211, while the
I2C configuration mode added by this patch does not require any
such MCU.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 219 +++++++++++++++++++----
1 file changed, 188 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 8226fefeedfc9..313c588297eca 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -11,6 +11,7 @@
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/regulator/consumer.h>
@@ -133,14 +134,17 @@
struct chipone {
struct device *dev;
+ struct i2c_client *client;
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
struct device_node *host_node;
+ struct mipi_dsi_device *dsi;
struct gpio_desc *enable_gpio;
struct regulator *vdd1;
struct regulator *vdd2;
struct regulator *vdd3;
int dsi_lanes;
+ bool interface_i2c;
};
static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
@@ -172,20 +176,14 @@ bridge_to_mode(struct drm_bridge *bridge, struct drm_atomic_state *state)
return &crtc_state->adjusted_mode;
}
-static inline int chipone_dsi_write(struct chipone *icn, const void *seq,
- size_t len)
+static void ICN6211_DSI(struct chipone *icn, u8 reg, u8 val)
{
- struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
-
- return mipi_dsi_generic_write(dsi, seq, len);
+ if (icn->interface_i2c)
+ i2c_smbus_write_byte_data(icn->client, reg, val);
+ else
+ mipi_dsi_generic_write(icn->dsi, (u8[]){reg, val}, 2);
}
-#define ICN6211_DSI(icn, seq...) \
- { \
- const u8 d[] = { seq }; \
- chipone_dsi_write(icn, d, ARRAY_SIZE(d)); \
- }
-
static void chipone_configure_pll(struct chipone *icn,
const struct drm_display_mode *mode)
{
@@ -282,7 +280,10 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
bus_flags = bridge_state->output_bus_cfg.flags;
- ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
+ if (icn->interface_i2c)
+ ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_I2C);
+ else
+ ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
ICN6211_DSI(icn, HACTIVE_LI, mode->hdisplay & 0xff);
@@ -396,11 +397,86 @@ static void chipone_atomic_post_disable(struct drm_bridge *bridge,
gpiod_set_value(icn->enable_gpio, 0);
}
+static int chipone_dsi_attach(struct chipone *icn)
+{
+ struct mipi_dsi_device *dsi = icn->dsi;
+ int ret;
+
+ dsi->lanes = icn->dsi_lanes;
+ dsi->format = MIPI_DSI_FMT_RGB888;
+ dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+ MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
+
+ ret = mipi_dsi_attach(dsi);
+ if (ret < 0)
+ dev_err(icn->dev, "failed to attach dsi\n");
+
+ return ret;
+}
+
+static int chipone_dsi_setup(struct chipone *icn)
+{
+ struct device *dev = icn->dev;
+ struct mipi_dsi_device *dsi;
+ struct mipi_dsi_host *host;
+ int ret = 0;
+
+ const struct mipi_dsi_device_info info = {
+ .type = "chipone",
+ .channel = 0,
+ .node = NULL,
+ };
+
+ host = of_find_mipi_dsi_host_by_node(icn->host_node);
+ if (!host) {
+ dev_err(dev, "failed to find dsi host\n");
+ return -EPROBE_DEFER;
+ }
+
+ dsi = mipi_dsi_device_register_full(host, &info);
+ if (IS_ERR(dsi)) {
+ return dev_err_probe(dev, PTR_ERR(dsi),
+ "failed to create dsi device\n");
+ }
+
+ icn->dsi = dsi;
+
+ ret = chipone_dsi_attach(icn);
+ if (ret < 0)
+ mipi_dsi_device_unregister(dsi);
+
+ return ret;
+}
+
static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
{
struct chipone *icn = bridge_to_chipone(bridge);
+ struct drm_bridge *abridge = bridge;
+ int ret;
+
+ if (icn->interface_i2c) {
+ ret = chipone_dsi_setup(icn);
+ if (ret)
+ return ret;
+
+ abridge = &icn->bridge;
+ }
+
+ return drm_bridge_attach(bridge->encoder, icn->panel_bridge,
+ bridge, flags);
+}
+
+static void chipone_detach(struct drm_bridge *bridge)
+{
+ struct chipone *icn = bridge_to_chipone(bridge);
+
+ if (!icn->dsi || icn->interface_i2c)
+ return;
- return drm_bridge_attach(bridge->encoder, icn->panel_bridge, bridge, flags);
+ mipi_dsi_detach(icn->dsi);
+ mipi_dsi_device_unregister(icn->dsi);
+ drm_bridge_remove(&icn->bridge);
+ icn->dsi = NULL;
}
#define MAX_INPUT_SEL_FORMATS 1
@@ -431,6 +507,7 @@ chipone_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
static const struct drm_bridge_funcs chipone_bridge_funcs = {
.attach = chipone_attach,
+ .detach = chipone_detach,
.atomic_post_disable = chipone_atomic_post_disable,
.atomic_pre_enable = chipone_atomic_pre_enable,
.atomic_enable = chipone_atomic_enable,
@@ -505,9 +582,8 @@ static int chipone_parse_dt(struct chipone *icn)
return 0;
}
-static int chipone_probe(struct mipi_dsi_device *dsi)
+static int chipone_common_probe(struct device *dev, struct chipone **icnr)
{
- struct device *dev = &dsi->dev;
struct chipone *icn;
int ret;
@@ -515,7 +591,6 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
if (!icn)
return -ENOMEM;
- mipi_dsi_set_drvdata(dsi, icn);
icn->dev = dev;
ret = chipone_parse_dt(icn);
@@ -526,29 +601,77 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
icn->bridge.type = DRM_MODE_CONNECTOR_DPI;
icn->bridge.of_node = dev->of_node;
- drm_bridge_add(&icn->bridge);
+ *icnr = icn;
- dsi->lanes = icn->dsi_lanes;
- dsi->format = MIPI_DSI_FMT_RGB888;
- dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
- MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
+ return ret;
+}
- ret = mipi_dsi_attach(dsi);
- if (ret < 0) {
+static int chipone_dsi_probe(struct mipi_dsi_device *dsi)
+{
+ struct device *dev = &dsi->dev;
+ struct chipone *icn;
+ int ret;
+
+ ret = chipone_common_probe(dev, &icn);
+ if (ret)
+ return ret;
+
+ icn->interface_i2c = false;
+ icn->dsi = dsi;
+
+ mipi_dsi_set_drvdata(dsi, icn);
+
+ drm_bridge_add(&icn->bridge);
+
+ ret = chipone_dsi_attach(icn);
+ if (ret)
drm_bridge_remove(&icn->bridge);
- dev_err(dev, "failed to attach dsi\n");
- }
return ret;
}
-static int chipone_remove(struct mipi_dsi_device *dsi)
+static int chipone_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct chipone *icn;
+ int ret;
+
+ ret = chipone_common_probe(dev, &icn);
+ if (ret)
+ return ret;
+
+ icn->interface_i2c = true;
+ icn->client = client;
+ dev_set_drvdata(dev, icn);
+ i2c_set_clientdata(client, icn);
+
+ drm_bridge_add(&icn->bridge);
+
+ return ret;
+}
+
+static void chipone_common_remove(struct chipone *icn)
+{
+ of_node_put(icn->host_node);
+}
+
+static int chipone_dsi_remove(struct mipi_dsi_device *dsi)
{
struct chipone *icn = mipi_dsi_get_drvdata(dsi);
mipi_dsi_detach(dsi);
drm_bridge_remove(&icn->bridge);
- of_node_put(icn->host_node);
+ chipone_common_remove(icn);
+
+ return 0;
+}
+
+static int chipone_i2c_remove(struct i2c_client *client)
+{
+ struct chipone *icn = i2c_get_clientdata(client);
+
+ chipone_common_remove(icn);
return 0;
}
@@ -559,16 +682,50 @@ static const struct of_device_id chipone_of_match[] = {
};
MODULE_DEVICE_TABLE(of, chipone_of_match);
-static struct mipi_dsi_driver chipone_driver = {
- .probe = chipone_probe,
- .remove = chipone_remove,
+static struct mipi_dsi_driver chipone_dsi_driver = {
+ .probe = chipone_dsi_probe,
+ .remove = chipone_dsi_remove,
.driver = {
.name = "chipone-icn6211",
.owner = THIS_MODULE,
.of_match_table = chipone_of_match,
},
};
-module_mipi_dsi_driver(chipone_driver);
+
+static struct i2c_device_id chipone_i2c_id[] = {
+ { "chipone,icn6211" },
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, chipone_i2c_id);
+
+static struct i2c_driver chipone_i2c_driver = {
+ .probe = chipone_i2c_probe,
+ .remove = chipone_i2c_remove,
+ .id_table = chipone_i2c_id,
+ .driver = {
+ .name = "chipone-icn6211-i2c",
+ .owner = THIS_MODULE,
+ .of_match_table = chipone_of_match,
+ },
+};
+
+static int __init chipone_init(void)
+{
+ if (IS_ENABLED(CONFIG_DRM_MIPI_DSI))
+ mipi_dsi_driver_register(&chipone_dsi_driver);
+
+ return i2c_add_driver(&chipone_i2c_driver);
+}
+module_init(chipone_init);
+
+static void __init chipone_exit(void)
+{
+ i2c_del_driver(&chipone_i2c_driver);
+
+ if (IS_ENABLED(CONFIG_DRM_MIPI_DSI))
+ mipi_dsi_driver_unregister(&chipone_dsi_driver);
+}
+module_exit(chipone_exit);
MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>");
MODULE_DESCRIPTION("Chipone ICN6211 MIPI-DSI to RGB Converter Bridge");
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support
2022-01-14 3:48 ` [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support Marek Vasut
@ 2022-01-14 10:24 ` kernel test robot
2022-02-03 12:17 ` Maxime Ripard
1 sibling, 0 replies; 23+ messages in thread
From: kernel test robot @ 2022-01-14 10:24 UTC (permalink / raw)
To: Marek Vasut, dri-devel
Cc: Marek Vasut, kbuild-all, Robert Foss, Jagan Teki,
Thomas Zimmermann, Sam Ravnborg
Hi Marek,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.16 next-20220114]
[cannot apply to drm-tip/drm-tip airlied/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/Marek-Vasut/drm-bridge-icn6211-Fix-register-layout/20220114-115112
base: git://anongit.freedesktop.org/drm/drm drm-next
config: h8300-buildonly-randconfig-r001-20220113 (https://download.01.org/0day-ci/archive/20220114/202201141818.0EmBjuCm-lkp@intel.com/config)
compiler: h8300-linux-gcc (GCC) 11.2.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/730ece8a3c97460eaf81603bc246cd631d1f7461
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Marek-Vasut/drm-bridge-icn6211-Fix-register-layout/20220114-115112
git checkout 730ece8a3c97460eaf81603bc246cd631d1f7461
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=h8300 SHELL=/bin/bash drivers/gpu/drm/bridge/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/chipone-icn6211.c: In function 'chipone_attach':
>> drivers/gpu/drm/bridge/chipone-icn6211.c:454:28: warning: variable 'abridge' set but not used [-Wunused-but-set-variable]
454 | struct drm_bridge *abridge = bridge;
| ^~~~~~~
vim +/abridge +454 drivers/gpu/drm/bridge/chipone-icn6211.c
450
451 static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
452 {
453 struct chipone *icn = bridge_to_chipone(bridge);
> 454 struct drm_bridge *abridge = bridge;
455 int ret;
456
457 if (icn->interface_i2c) {
458 ret = chipone_dsi_setup(icn);
459 if (ret)
460 return ret;
461
462 abridge = &icn->bridge;
463 }
464
465 return drm_bridge_attach(bridge->encoder, icn->panel_bridge,
466 bridge, flags);
467 }
468
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support
@ 2022-01-14 10:24 ` kernel test robot
0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2022-01-14 10:24 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2758 bytes --]
Hi Marek,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.16 next-20220114]
[cannot apply to drm-tip/drm-tip airlied/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/Marek-Vasut/drm-bridge-icn6211-Fix-register-layout/20220114-115112
base: git://anongit.freedesktop.org/drm/drm drm-next
config: h8300-buildonly-randconfig-r001-20220113 (https://download.01.org/0day-ci/archive/20220114/202201141818.0EmBjuCm-lkp(a)intel.com/config)
compiler: h8300-linux-gcc (GCC) 11.2.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/730ece8a3c97460eaf81603bc246cd631d1f7461
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Marek-Vasut/drm-bridge-icn6211-Fix-register-layout/20220114-115112
git checkout 730ece8a3c97460eaf81603bc246cd631d1f7461
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=h8300 SHELL=/bin/bash drivers/gpu/drm/bridge/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/chipone-icn6211.c: In function 'chipone_attach':
>> drivers/gpu/drm/bridge/chipone-icn6211.c:454:28: warning: variable 'abridge' set but not used [-Wunused-but-set-variable]
454 | struct drm_bridge *abridge = bridge;
| ^~~~~~~
vim +/abridge +454 drivers/gpu/drm/bridge/chipone-icn6211.c
450
451 static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
452 {
453 struct chipone *icn = bridge_to_chipone(bridge);
> 454 struct drm_bridge *abridge = bridge;
455 int ret;
456
457 if (icn->interface_i2c) {
458 ret = chipone_dsi_setup(icn);
459 if (ret)
460 return ret;
461
462 abridge = &icn->bridge;
463 }
464
465 return drm_bridge_attach(bridge->encoder, icn->panel_bridge,
466 bridge, flags);
467 }
468
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support
2022-01-14 3:48 ` [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support Marek Vasut
2022-01-14 10:24 ` kernel test robot
@ 2022-02-03 12:17 ` Maxime Ripard
1 sibling, 0 replies; 23+ messages in thread
From: Maxime Ripard @ 2022-02-03 12:17 UTC (permalink / raw)
To: Marek Vasut
Cc: Robert Foss, Sam Ravnborg, Thomas Zimmermann, dri-devel, Jagan Teki
[-- Attachment #1: Type: text/plain, Size: 5185 bytes --]
On Fri, Jan 14, 2022 at 04:48:36AM +0100, Marek Vasut wrote:
> The ICN6211 chip starts in I2C configuration mode after cold boot.
> Implement support for configuring the chip via I2C in addition to
> the current DSI LP command mode configuration support. The later
> seems to be available only on chips which have additional MCU on
> the panel/bridge board which preconfigures the ICN6211, while the
> I2C configuration mode added by this patch does not require any
> such MCU.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/bridge/chipone-icn6211.c | 219 +++++++++++++++++++----
> 1 file changed, 188 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 8226fefeedfc9..313c588297eca 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -11,6 +11,7 @@
>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/regulator/consumer.h>
> @@ -133,14 +134,17 @@
>
> struct chipone {
> struct device *dev;
> + struct i2c_client *client;
> struct drm_bridge bridge;
> struct drm_bridge *panel_bridge;
> struct device_node *host_node;
> + struct mipi_dsi_device *dsi;
> struct gpio_desc *enable_gpio;
> struct regulator *vdd1;
> struct regulator *vdd2;
> struct regulator *vdd3;
> int dsi_lanes;
> + bool interface_i2c;
> };
>
> static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge)
> @@ -172,20 +176,14 @@ bridge_to_mode(struct drm_bridge *bridge, struct drm_atomic_state *state)
> return &crtc_state->adjusted_mode;
> }
>
> -static inline int chipone_dsi_write(struct chipone *icn, const void *seq,
> - size_t len)
> +static void ICN6211_DSI(struct chipone *icn, u8 reg, u8 val)
> {
> - struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev);
> -
> - return mipi_dsi_generic_write(dsi, seq, len);
> + if (icn->interface_i2c)
> + i2c_smbus_write_byte_data(icn->client, reg, val);
> + else
> + mipi_dsi_generic_write(icn->dsi, (u8[]){reg, val}, 2);
> }
>
> -#define ICN6211_DSI(icn, seq...) \
> - { \
> - const u8 d[] = { seq }; \
> - chipone_dsi_write(icn, d, ARRAY_SIZE(d)); \
> - }
> -
> static void chipone_configure_pll(struct chipone *icn,
> const struct drm_display_mode *mode)
> {
> @@ -282,7 +280,10 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
> bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> bus_flags = bridge_state->output_bus_cfg.flags;
>
> - ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
> + if (icn->interface_i2c)
> + ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_I2C);
> + else
> + ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
>
> ICN6211_DSI(icn, HACTIVE_LI, mode->hdisplay & 0xff);
>
> @@ -396,11 +397,86 @@ static void chipone_atomic_post_disable(struct drm_bridge *bridge,
> gpiod_set_value(icn->enable_gpio, 0);
> }
>
> +static int chipone_dsi_attach(struct chipone *icn)
> +{
> + struct mipi_dsi_device *dsi = icn->dsi;
> + int ret;
> +
> + dsi->lanes = icn->dsi_lanes;
> + dsi->format = MIPI_DSI_FMT_RGB888;
> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> + MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
> +
> + ret = mipi_dsi_attach(dsi);
> + if (ret < 0)
> + dev_err(icn->dev, "failed to attach dsi\n");
> +
> + return ret;
> +}
> +
> +static int chipone_dsi_setup(struct chipone *icn)
> +{
> + struct device *dev = icn->dev;
> + struct mipi_dsi_device *dsi;
> + struct mipi_dsi_host *host;
> + int ret = 0;
> +
> + const struct mipi_dsi_device_info info = {
> + .type = "chipone",
> + .channel = 0,
> + .node = NULL,
> + };
> +
> + host = of_find_mipi_dsi_host_by_node(icn->host_node);
> + if (!host) {
> + dev_err(dev, "failed to find dsi host\n");
> + return -EPROBE_DEFER;
> + }
> +
> + dsi = mipi_dsi_device_register_full(host, &info);
> + if (IS_ERR(dsi)) {
> + return dev_err_probe(dev, PTR_ERR(dsi),
> + "failed to create dsi device\n");
> + }
> +
> + icn->dsi = dsi;
> +
> + ret = chipone_dsi_attach(icn);
> + if (ret < 0)
> + mipi_dsi_device_unregister(dsi);
> +
> + return ret;
> +}
> +
> static int chipone_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags)
> {
> struct chipone *icn = bridge_to_chipone(bridge);
> + struct drm_bridge *abridge = bridge;
> + int ret;
> +
> + if (icn->interface_i2c) {
> + ret = chipone_dsi_setup(icn);
> + if (ret)
> + return ret;
> +
> + abridge = &icn->bridge;
This needs to happen at probe/bind time. See:
https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 13/14] drm: bridge: icn6211: Rename ICN6211_DSI to chipone_writeb
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (10 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 3:48 ` [PATCH 14/14] drm: bridge: icn6211: Read and validate chip IDs before configuration Marek Vasut
2022-01-14 8:16 ` [PATCH 01/14] drm: bridge: icn6211: Fix register layout Jagan Teki
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
Rename function ICN6211_DSI() to chipone_writeb() to keep all function
names lower-case. No functional change.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 56 ++++++++++++------------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 313c588297eca..3023edb6f31b5 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -176,7 +176,7 @@ bridge_to_mode(struct drm_bridge *bridge, struct drm_atomic_state *state)
return &crtc_state->adjusted_mode;
}
-static void ICN6211_DSI(struct chipone *icn, u8 reg, u8 val)
+static void chipone_writeb(struct chipone *icn, u8 reg, u8 val)
{
if (icn->interface_i2c)
i2c_smbus_write_byte_data(icn->client, reg, val);
@@ -258,11 +258,11 @@ static void chipone_configure_pll(struct chipone *icn,
(fin * best_m) / BIT(best_p + best_s + 2));
/* Clock source selection fixed to MIPI DSI clock lane */
- ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
- ICN6211_DSI(icn, PLL_REF_DIV,
+ chipone_writeb(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
+ chipone_writeb(icn, PLL_REF_DIV,
(best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2 pre-divider */
PLL_REF_DIV_P(best_p) | PLL_REF_DIV_S(best_s));
- ICN6211_DSI(icn, PLL_INT(0), best_m);
+ chipone_writeb(icn, PLL_INT(0), best_m);
}
static void chipone_atomic_enable(struct drm_bridge *bridge,
@@ -281,19 +281,19 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
bus_flags = bridge_state->output_bus_cfg.flags;
if (icn->interface_i2c)
- ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_I2C);
+ chipone_writeb(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_I2C);
else
- ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
+ chipone_writeb(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
- ICN6211_DSI(icn, HACTIVE_LI, mode->hdisplay & 0xff);
+ chipone_writeb(icn, HACTIVE_LI, mode->hdisplay & 0xff);
- ICN6211_DSI(icn, VACTIVE_LI, mode->vdisplay & 0xff);
+ chipone_writeb(icn, VACTIVE_LI, mode->vdisplay & 0xff);
/*
* lsb nibble: 2nd nibble of hdisplay
* msb nibble: 2nd nibble of vdisplay
*/
- ICN6211_DSI(icn, VACTIVE_HACTIVE_HI,
+ chipone_writeb(icn, VACTIVE_HACTIVE_HI,
((mode->hdisplay >> 8) & 0xf) |
(((mode->vdisplay >> 8) & 0xf) << 4));
@@ -301,49 +301,49 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
hsync = mode->hsync_end - mode->hsync_start;
hbp = mode->htotal - mode->hsync_end;
- ICN6211_DSI(icn, HFP_LI, hfp & 0xff);
- ICN6211_DSI(icn, HSYNC_LI, hsync & 0xff);
- ICN6211_DSI(icn, HBP_LI, hbp & 0xff);
+ chipone_writeb(icn, HFP_LI, hfp & 0xff);
+ chipone_writeb(icn, HSYNC_LI, hsync & 0xff);
+ chipone_writeb(icn, HBP_LI, hbp & 0xff);
/* Top two bits of Horizontal Front porch/Sync/Back porch */
- ICN6211_DSI(icn, HFP_HSW_HBP_HI,
+ chipone_writeb(icn, HFP_HSW_HBP_HI,
HFP_HSW_HBP_HI_HFP(hfp) |
HFP_HSW_HBP_HI_HS(hsync) |
HFP_HSW_HBP_HI_HBP(hbp));
- ICN6211_DSI(icn, VFP, mode->vsync_start - mode->vdisplay);
+ chipone_writeb(icn, VFP, mode->vsync_start - mode->vdisplay);
- ICN6211_DSI(icn, VSYNC, mode->vsync_end - mode->vsync_start);
+ chipone_writeb(icn, VSYNC, mode->vsync_end - mode->vsync_start);
- ICN6211_DSI(icn, VBP, mode->vtotal - mode->vsync_end);
+ chipone_writeb(icn, VBP, mode->vtotal - mode->vsync_end);
/* dsi specific sequence */
- ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
- ICN6211_DSI(icn, HFP_MIN, hfp & 0xff);
+ chipone_writeb(icn, SYNC_EVENT_DLY, 0x80);
+ chipone_writeb(icn, HFP_MIN, hfp & 0xff);
/* DSI data lane count */
- ICN6211_DSI(icn, DSI_CTRL,
+ chipone_writeb(icn, DSI_CTRL,
DSI_CTRL_UNKNOWN | DSI_CTRL_DSI_LANES(icn->dsi_lanes - 1));
- ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
- ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
- ICN6211_DSI(icn, MIPI_PN_SWAP, 0x00);
+ chipone_writeb(icn, MIPI_PD_CK_LANE, 0xa0);
+ chipone_writeb(icn, PLL_CTRL(12), 0xff);
+ chipone_writeb(icn, MIPI_PN_SWAP, 0x00);
/* DPI HS/VS/DE polarity */
pol = ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIST_POL_HSYNC_POL : 0) |
((mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIST_POL_VSYNC_POL : 0) |
((bus_flags & DRM_BUS_FLAG_DE_HIGH) ? BIST_POL_DE_POL : 0);
- ICN6211_DSI(icn, BIST_POL, pol);
+ chipone_writeb(icn, BIST_POL, pol);
/* Configure PLL settings */
chipone_configure_pll(icn, mode);
- ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
- ICN6211_DSI(icn, SYS_CTRL(1), 0x88);
+ chipone_writeb(icn, SYS_CTRL(0), 0x40);
+ chipone_writeb(icn, SYS_CTRL(1), 0x88);
/* icn6211 specific sequence */
- ICN6211_DSI(icn, MIPI_FORCE_0, 0x20);
- ICN6211_DSI(icn, PLL_CTRL(1), 0x20);
- ICN6211_DSI(icn, CONFIG_FINISH, 0x10);
+ chipone_writeb(icn, MIPI_FORCE_0, 0x20);
+ chipone_writeb(icn, PLL_CTRL(1), 0x20);
+ chipone_writeb(icn, CONFIG_FINISH, 0x10);
usleep_range(10000, 11000);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 14/14] drm: bridge: icn6211: Read and validate chip IDs before configuration
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (11 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 13/14] drm: bridge: icn6211: Rename ICN6211_DSI to chipone_writeb Marek Vasut
@ 2022-01-14 3:48 ` Marek Vasut
2022-01-14 8:16 ` [PATCH 01/14] drm: bridge: icn6211: Fix register layout Jagan Teki
13 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 3:48 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Thomas Zimmermann, Sam Ravnborg, Jagan Teki, Robert Foss
Read out the Vendor/Chip/Version ID registers from the chip before
performing any configuration, and validate that the registers have
correct values. This is mostly a simple test whether DSI register
access does work, since that tends to be broken on various bridges.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 3023edb6f31b5..85cba739afd9c 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -176,6 +176,14 @@ bridge_to_mode(struct drm_bridge *bridge, struct drm_atomic_state *state)
return &crtc_state->adjusted_mode;
}
+static void chipone_readb(struct chipone *icn, u8 reg, u8 *val)
+{
+ if (icn->interface_i2c)
+ *val = i2c_smbus_read_byte_data(icn->client, reg);
+ else
+ mipi_dsi_generic_read(icn->dsi, (u8[]){reg, 1}, 2, val, 1);
+}
+
static void chipone_writeb(struct chipone *icn, u8 reg, u8 val)
{
if (icn->interface_i2c)
@@ -274,7 +282,21 @@ 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;
+ u8 pol, id[4];
+
+ chipone_readb(icn, VENDOR_ID, id);
+ chipone_readb(icn, DEVICE_ID_H, id + 1);
+ chipone_readb(icn, DEVICE_ID_L, id + 2);
+ chipone_readb(icn, VERSION_ID, id + 3);
+
+ dev_dbg(icn->dev,
+ "Chip IDs: Vendor=0x%02x Device=0x%02x:0x%02x Version=0x%02x\n",
+ id[0], id[1], id[2], id[3]);
+
+ if (id[0] != 0xc1 || id[1] != 0x62 || id[2] != 0x11) {
+ dev_dbg(icn->dev, "Invalid Chip IDs, aborting configuration\n");
+ return;
+ }
/* Get the DPI flags from the bridge state. */
bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 01/14] drm: bridge: icn6211: Fix register layout
2022-01-14 3:48 [PATCH 01/14] drm: bridge: icn6211: Fix register layout Marek Vasut
` (12 preceding siblings ...)
2022-01-14 3:48 ` [PATCH 14/14] drm: bridge: icn6211: Read and validate chip IDs before configuration Marek Vasut
@ 2022-01-14 8:16 ` Jagan Teki
2022-01-14 14:33 ` Marek Vasut
13 siblings, 1 reply; 23+ messages in thread
From: Jagan Teki @ 2022-01-14 8:16 UTC (permalink / raw)
To: Marek Vasut; +Cc: Thomas Zimmermann, Sam Ravnborg, Robert Foss, dri-devel
Hi Marek,
Thanks for the patches.
On Fri, Jan 14, 2022 at 9:18 AM Marek Vasut <marex@denx.de> wrote:
>
> The chip register layout has nothing to do with MIPI DCS, the registers
> incorrectly marked as MIPI DCS in the driver are regular chip registers
> often with completely different function.
>
> Fill in the actual register names and bits from [1] and [2] and add the
> entire register layout, since the documentation for this chip is hard to
> come by.
>
> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
> [2] https://github.com/tdjastrzebski/ICN6211-Configurator
>
> Fixes: ce517f18944e3 ("drm: bridge: Add Chipone ICN6211 MIPI-DSI to RGB bridge")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/bridge/chipone-icn6211.c | 134 ++++++++++++++++++++---
> 1 file changed, 117 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index a6151db955868..eb26615b2993a 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -14,8 +14,19 @@
> #include <linux/of_device.h>
> #include <linux/regulator/consumer.h>
>
> -#include <video/mipi_display.h>
> -
> +#define VENDOR_ID 0x00
> +#define DEVICE_ID_H 0x01
> +#define DEVICE_ID_L 0x02
> +#define VERSION_ID 0x03
> +#define FIRMWARE_VERSION 0x08
> +#define CONFIG_FINISH 0x09
> +#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 RGB_DRV(n) (0x18 + ((n) & 0x3)) /* 0..3 */
> +#define RGB_DLY(n) (0x1c + ((n) & 0x1)) /* 0..1 */
> +#define RGB_TEST_CTRL 0x1e
> +#define ATE_PLL_EN 0x1f
> #define HACTIVE_LI 0x20
> #define VACTIVE_LI 0x21
> #define VACTIVE_HACTIVE_HI 0x22
> @@ -26,6 +37,95 @@
> #define VFP 0x27
> #define VSYNC 0x28
> #define VBP 0x29
> +#define BIST_POL 0x2a
> +#define BIST_POL_BIST_MODE(n) (((n) & 0xf) << 4)
> +#define BIST_POL_BIST_GEN BIT(3)
> +#define BIST_POL_HSYNC_POL BIT(2)
> +#define BIST_POL_VSYNC_POL BIT(1)
> +#define BIST_POL_DE_POL BIT(0)
> +#define BIST_RED 0x2b
> +#define BIST_GREEN 0x2c
> +#define BIST_BLUE 0x2d
> +#define BIST_CHESS_X 0x2e
> +#define BIST_CHESS_Y 0x2f
> +#define BIST_CHESS_XY_H 0x30
> +#define BIST_FRAME_TIME_L 0x31
> +#define BIST_FRAME_TIME_H 0x32
> +#define FIFO_MAX_ADDR_LOW 0x33
> +#define SYNC_EVENT_DLY 0x34
> +#define HSW_MIN 0x35
> +#define HFP_MIN 0x36
> +#define LOGIC_RST_NUM 0x37
> +#define OSC_CTRL(n) (0x48 + ((n) & 0x7)) /* 0..5 */
> +#define BG_CTRL 0x4e
> +#define LDO_PLL 0x4f
> +#define PLL_CTRL(n) (0x50 + ((n) & 0xf)) /* 0..15 */
> +#define PLL_CTRL_6_EXTERNAL 0x90
> +#define PLL_CTRL_6_MIPI_CLK 0x92
> +#define PLL_CTRL_6_INTERNAL 0x93
> +#define PLL_REM(n) (0x60 + ((n) & 0x3)) /* 0..2 */
> +#define PLL_DIV(n) (0x63 + ((n) & 0x3)) /* 0..2 */
> +#define PLL_FRAC(n) (0x66 + ((n) & 0x3)) /* 0..2 */
> +#define PLL_INT(n) (0x69 + ((n) & 0x1)) /* 0..1 */
> +#define PLL_REF_DIV 0x6b
> +#define PLL_REF_DIV_P(n) ((n) & 0xf)
> +#define PLL_REF_DIV_Pe BIT(4)
> +#define PLL_REF_DIV_S(n) (((n) & 0x7) << 5)
> +#define PLL_SSC_P(n) (0x6c + ((n) & 0x3)) /* 0..2 */
> +#define PLL_SSC_STEP(n) (0x6f + ((n) & 0x3)) /* 0..2 */
> +#define PLL_SSC_OFFSET(n) (0x72 + ((n) & 0x3)) /* 0..3 */
> +#define GPIO_OEN 0x79
> +#define MIPI_CFG_PW 0x7a
> +#define MIPI_CFG_PW_CONFIG_DSI 0xc1
> +#define MIPI_CFG_PW_CONFIG_I2C 0x3e
> +#define GPIO_SEL(n) (0x7b + ((n) & 0x1)) /* 0..1 */
> +#define IRQ_SEL 0x7d
> +#define DBG_SEL 0x7e
> +#define DBG_SIGNAL 0x7f
> +#define MIPI_ERR_VECTOR_L 0x80
> +#define MIPI_ERR_VECTOR_H 0x81
> +#define MIPI_ERR_VECTOR_EN_L 0x82
> +#define MIPI_ERR_VECTOR_EN_H 0x83
> +#define MIPI_MAX_SIZE_L 0x84
> +#define MIPI_MAX_SIZE_H 0x85
> +#define DSI_CTRL 0x86
> +#define DSI_CTRL_UNKNOWN 0x28
> +#define DSI_CTRL_DSI_LANES(n) ((n) & 0x3)
> +#define MIPI_PN_SWAP 0x87
> +#define MIPI_PN_SWAP_CLK BIT(4)
> +#define MIPI_PN_SWAP_D(n) BIT((n) & 0x3)
> +#define MIPI_SOT_SYNC_BIT_(n) (0x88 + ((n) & 0x1)) /* 0..1 */
> +#define MIPI_ULPS_CTRL 0x8a
> +#define MIPI_CLK_CHK_VAR 0x8e
> +#define MIPI_CLK_CHK_INI 0x8f
> +#define MIPI_T_TERM_EN 0x90
> +#define MIPI_T_HS_SETTLE 0x91
> +#define MIPI_T_TA_SURE_PRE 0x92
> +#define MIPI_T_LPX_SET 0x94
> +#define MIPI_T_CLK_MISS 0x95
> +#define MIPI_INIT_TIME_L 0x96
> +#define MIPI_INIT_TIME_H 0x97
> +#define MIPI_T_CLK_TERM_EN 0x99
> +#define MIPI_T_CLK_SETTLE 0x9a
> +#define MIPI_TO_HS_RX_L 0x9e
> +#define MIPI_TO_HS_RX_H 0x9f
> +#define MIPI_PHY_(n) (0xa0 + ((n) & 0x7)) /* 0..5 */
> +#define MIPI_PD_RX 0xb0
> +#define MIPI_PD_TERM 0xb1
> +#define MIPI_PD_HSRX 0xb2
> +#define MIPI_PD_LPTX 0xb3
> +#define MIPI_PD_LPRX 0xb4
> +#define MIPI_PD_CK_LANE 0xb5
> +#define MIPI_FORCE_0 0xb6
> +#define MIPI_RST_CTRL 0xb7
> +#define MIPI_RST_NUM 0xb8
> +#define MIPI_DBG_SET_(n) (0xc0 + ((n) & 0xf)) /* 0..9 */
> +#define MIPI_DBG_SEL 0xe0
> +#define MIPI_DBG_DATA 0xe1
> +#define MIPI_ATE_TEST_SEL 0xe2
> +#define MIPI_ATE_STATUS_(n) (0xe3 + ((n) & 0x1)) /* 0..1 */
> +#define MIPI_ATE_STATUS_1 0xe4
> +#define ICN6211_MAX_REGISTER MIPI_ATE_STATUS(1)
>
> struct chipone {
> struct device *dev;
> @@ -66,13 +166,13 @@ static void chipone_enable(struct drm_bridge *bridge)
> struct chipone *icn = bridge_to_chipone(bridge);
> struct drm_display_mode *mode = bridge_to_mode(bridge);
>
> - ICN6211_DSI(icn, 0x7a, 0xc1);
> + ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
>
> ICN6211_DSI(icn, HACTIVE_LI, mode->hdisplay & 0xff);
>
> ICN6211_DSI(icn, VACTIVE_LI, mode->vdisplay & 0xff);
>
> - /**
> + /*
> * lsb nibble: 2nd nibble of hdisplay
> * msb nibble: 2nd nibble of vdisplay
> */
> @@ -95,21 +195,21 @@ static void chipone_enable(struct drm_bridge *bridge)
> ICN6211_DSI(icn, VBP, mode->vtotal - mode->vsync_end);
>
> /* dsi specific sequence */
> - ICN6211_DSI(icn, MIPI_DCS_SET_TEAR_OFF, 0x80);
> - ICN6211_DSI(icn, MIPI_DCS_SET_ADDRESS_MODE, 0x28);
> - ICN6211_DSI(icn, 0xb5, 0xa0);
> - ICN6211_DSI(icn, 0x5c, 0xff);
> - ICN6211_DSI(icn, MIPI_DCS_SET_COLUMN_ADDRESS, 0x01);
> - ICN6211_DSI(icn, MIPI_DCS_GET_POWER_SAVE, 0x92);
> - ICN6211_DSI(icn, 0x6b, 0x71);
> - ICN6211_DSI(icn, 0x69, 0x2b);
> - ICN6211_DSI(icn, MIPI_DCS_ENTER_SLEEP_MODE, 0x40);
> - ICN6211_DSI(icn, MIPI_DCS_EXIT_SLEEP_MODE, 0x98);
> + ICN6211_DSI(icn, SYNC_EVENT_DLY, 0x80);
> + ICN6211_DSI(icn, HFP_MIN, 0x28);
> + ICN6211_DSI(icn, MIPI_PD_CK_LANE, 0xa0);
> + ICN6211_DSI(icn, PLL_CTRL(12), 0xff);
> + ICN6211_DSI(icn, BIST_POL, BIST_POL_DE_POL);
> + ICN6211_DSI(icn, PLL_CTRL(6), PLL_CTRL_6_MIPI_CLK);
> + ICN6211_DSI(icn, PLL_REF_DIV, 0x71);
> + ICN6211_DSI(icn, PLL_INT(0), 0x2b);
> + ICN6211_DSI(icn, SYS_CTRL(0), 0x40);
> + ICN6211_DSI(icn, SYS_CTRL(1), 0x98);
>
> /* icn6211 specific sequence */
> - ICN6211_DSI(icn, 0xb6, 0x20);
> - ICN6211_DSI(icn, 0x51, 0x20);
> - ICN6211_DSI(icn, 0x09, 0x10);
> + ICN6211_DSI(icn, MIPI_FORCE_0, 0x20);
> + ICN6211_DSI(icn, PLL_CTRL(1), 0x20);
> + ICN6211_DSI(icn, CONFIG_FINISH, 0x10);
All these fixes and few of features support are valid only for
I2C-based ICN6211. If possible please confirm with the vendor. The
driver I've written based on non-I2C-based ICN6211 chip, which is
present in BananaPi Panel.
Chip part: ICN6211 A59058 1634.
Not sure, may be we can have separated bridge driver for I2C-based
ICN6211 that I don't have in my design for testing.
Thanks,
Jagan.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 01/14] drm: bridge: icn6211: Fix register layout
2022-01-14 8:16 ` [PATCH 01/14] drm: bridge: icn6211: Fix register layout Jagan Teki
@ 2022-01-14 14:33 ` Marek Vasut
0 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2022-01-14 14:33 UTC (permalink / raw)
To: Jagan Teki; +Cc: Thomas Zimmermann, Sam Ravnborg, Robert Foss, dri-devel
On 1/14/22 09:16, Jagan Teki wrote:
Hi
[...]
>> Fill in the actual register names and bits from [1] and [2] and add the
>> entire register layout, since the documentation for this chip is hard to
>> come by.
>>
>> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c
>> [2] https://github.com/tdjastrzebski/ICN6211-Configurator
[...]
>> /* icn6211 specific sequence */
>> - ICN6211_DSI(icn, 0xb6, 0x20);
>> - ICN6211_DSI(icn, 0x51, 0x20);
>> - ICN6211_DSI(icn, 0x09, 0x10);
>> + ICN6211_DSI(icn, MIPI_FORCE_0, 0x20);
>> + ICN6211_DSI(icn, PLL_CTRL(1), 0x20);
>> + ICN6211_DSI(icn, CONFIG_FINISH, 0x10);
>
> All these fixes and few of features support are valid only for
> I2C-based ICN6211.
No, they are valid for both DSI command mode configuration as well as
I2C configuration. They are also tested in both configurations, see patch
[PATCH 12/14] drm: bridge: icn6211: Add I2C configuration support
The register layout is exactly the same for DSI command mode and I2C
configuration mode too.
> If possible please confirm with the vendor.
See the commit message, the datasheet is difficult to come by, however
there are FOSS driver(s) and tooling source which confirms the above.
> The
> driver I've written based on non-I2C-based ICN6211 chip, which is
> present in BananaPi Panel.
>
> Chip part: ICN6211 A59058 1634.
>
> Not sure, may be we can have separated bridge driver for I2C-based
> ICN6211 that I don't have in my design for testing.
There is only one ICN6211 chip variant, you can configure it either via
DSI command mode or I2C mode, both were tested with this series which
adds support for the later mode.
[...]
^ permalink raw reply [flat|nested] 23+ messages in thread