[PATCH v5 00/11] drm: bridge: icn6211: Fix hard-coded panel settings and add I2C support
This series fixes multiple problems with the ICN6211 driver and adds support for configuration of the chip via I2C bus.
First, in the current state, the ICN6211 driver hard-codes DPI timing and clock settings specific to some unknown panel. The settings provided by panel driver are ignored. Using any other panel than the one for which this driver is currently hard-coded can lead to permanent damage of the panel (per display supplier warning, and it sure did in my case. The damage looks like multiple rows of dead pixels at the bottom of the panel, and this is not going away even after long power off time).
Much of this series thus fixes incorrect register layout, DPI timing programming, clock generation by adding actual PLL configuration code. This series no longer adds lane count decoding and retains current hard-coded lane count 4 due to disagreement over lane count parsing from DT. The lane count support will come later. The series also fills in a couple of registers with likely correct default values.
Second, this series adds support for I2C configuration of the ICN6211. The device can be configured either via DSI command mode or via I2C, the register layout is the same in both cases.
Since the datasheet for this device is very hard to come by, a lot of information has been salvaged from [1] and [2].
[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/b... [2] https://github.com/tdjastrzebski/ICN6211-Configurator
Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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
Marek Vasut (11): drm: bridge: icn6211: Fix register layout drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling drm: bridge: icn6211: Add HS/VS/DE polarity handling drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode drm: bridge: icn6211: Disable DPI color swap drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples drm: bridge: icn6211: Implement atomic_get_input_bus_fmts drm: bridge: icn6211: Add I2C configuration support drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() drm: bridge: icn6211: Read and validate chip IDs before configuration
drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- 1 file changed, 434 insertions(+), 52 deletions(-)
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/b... [2] https://github.com/tdjastrzebski/ICN6211-Configurator
Acked-by: Maxime Ripard maxime@cerno.tech 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: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- 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 d9b7f48b99fbf..376e0f80da5ca 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -15,8 +15,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 @@ -27,6 +38,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; @@ -64,13 +164,13 @@ static void chipone_atomic_enable(struct drm_bridge *bridge, struct chipone *icn = bridge_to_chipone(bridge); struct drm_display_mode *mode = &icn->mode;
- 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 */ @@ -93,21 +193,21 @@ static void chipone_atomic_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);
usleep_range(10000, 11000); }
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.
Acked-by: Maxime Ripard maxime@cerno.tech 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: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- 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 376e0f80da5ca..c871a90c0b8f4 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -35,6 +35,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 @@ -163,6 +166,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge, { struct chipone *icn = bridge_to_chipone(bridge); struct drm_display_mode *mode = &icn->mode; + u16 hfp, hbp, hsync;
ICN6211_DSI(icn, MIPI_CFG_PW, MIPI_CFG_PW_CONFIG_DSI);
@@ -178,13 +182,18 @@ static void chipone_atomic_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);
@@ -194,7 +203,7 @@ static void chipone_atomic_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);
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.
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: No change V4: Add AB from Maxime V5: No change --- drivers/gpu/drm/bridge/chipone-icn6211.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index c871a90c0b8f4..30db8d1783cef 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -165,8 +165,16 @@ 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_atomic_state *state = old_bridge_state->base.state; struct drm_display_mode *mode = &icn->mode; + 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);
@@ -206,7 +214,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);
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/b... [2] https://github.com/tdjastrzebski/ICN6211-Configurator
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: - Cache mipi_dsi_device pointer in struct chipone {}, this is moved here from drm: "bridge: icn6211: Add I2C configuration support" V5: Use dsi lane count from struct mipi_dsi_device . --- drivers/gpu/drm/bridge/chipone-icn6211.c | 89 +++++++++++++++++++++++- 1 file changed, 86 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index 30db8d1783cef..d4a52176814ce 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -136,6 +136,7 @@ struct chipone { struct drm_bridge bridge; struct drm_display_mode mode; struct drm_bridge *panel_bridge; + struct mipi_dsi_device *dsi; struct gpio_desc *enable_gpio; struct regulator *vdd1; struct regulator *vdd2; @@ -161,6 +162,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) { @@ -221,9 +303,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);
@@ -376,6 +458,7 @@ static int chipone_probe(struct mipi_dsi_device *dsi) icn->bridge.funcs = &chipone_bridge_funcs; icn->bridge.type = DRM_MODE_CONNECTOR_DPI; icn->bridge.of_node = dev->of_node; + icn->dsi = dsi;
drm_bridge_add(&icn->bridge);
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.
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- 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 d4a52176814ce..e3a7b945a0ef5 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -464,7 +464,8 @@ static int chipone_probe(struct mipi_dsi_device *dsi)
dsi->lanes = 4; 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) {
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.
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- 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 e3a7b945a0ef5..4d6baef7ce16c 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -296,6 +296,7 @@ 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, MIPI_PN_SWAP, 0x00);
/* DPI HS/VS/DE polarity */ pol = ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIST_POL_HSYNC_POL : 0) |
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/b... [2] https://github.com/tdjastrzebski/ICN6211-Configurator
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- 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 4d6baef7ce16c..b2faad4cb8a6d 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -308,7 +308,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);
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.
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- 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 b2faad4cb8a6d..42e3581d76521 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -383,6 +383,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 = { .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, @@ -392,6 +418,7 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = { .atomic_post_disable = chipone_atomic_post_disable, .mode_set = chipone_mode_set, .attach = chipone_attach, + .atomic_get_input_bus_fmts = chipone_atomic_get_input_bus_fmts, };
static int chipone_parse_dt(struct chipone *icn)
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: Maxime Ripard maxime@cerno.tech 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 --- V2: - Drop the abridge variable - Rename chipone_dsi_setup to chipone_dsi_host_attach and call it from chipone_i2c_probe() V3: Add AB from Maxime V4: - Update on top of small change in drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration - Look up host_node locally in chipone_dsi_host_attach() - Drop AB from Maxime V5: Fix compile error in chipone_dsi_write() --- drivers/gpu/drm/bridge/chipone-icn6211.c | 178 ++++++++++++++++++++--- 1 file changed, 156 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index 42e3581d76521..9fdc0307266d3 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,6 +134,7 @@
struct chipone { struct device *dev; + struct i2c_client *client; struct drm_bridge bridge; struct drm_display_mode mode; struct drm_bridge *panel_bridge; @@ -141,6 +143,7 @@ struct chipone { struct regulator *vdd1; struct regulator *vdd2; struct regulator *vdd3; + bool interface_i2c; };
static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge) @@ -148,12 +151,15 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge) return container_of(bridge, struct chipone, bridge); }
-static inline int chipone_dsi_write(struct chipone *icn, const void *seq, +static inline int chipone_dsi_write(struct chipone *icn, const u8 *seq, size_t len) { - struct mipi_dsi_device *dsi = to_mipi_dsi_device(icn->dev); - - return mipi_dsi_generic_write(dsi, seq, len); + if (icn->interface_i2c) { + return i2c_smbus_write_byte_data(icn->client, seq[0], seq[1]); + } else { + return mipi_dsi_generic_write(icn->dsi, + (u8[]){seq[0], seq[1]}, 2); + } }
#define ICN6211_DSI(icn, seq...) \ @@ -258,7 +264,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);
@@ -374,6 +383,67 @@ static void chipone_mode_set(struct drm_bridge *bridge, struct chipone *icn = bridge_to_chipone(bridge);
drm_mode_copy(&icn->mode, adjusted_mode); +}; + +static int chipone_dsi_attach(struct chipone *icn) +{ + struct mipi_dsi_device *dsi = icn->dsi; + int ret; + + dsi->lanes = 4; + 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_host_attach(struct chipone *icn) +{ + struct device *dev = icn->dev; + struct device_node *host_node; + struct device_node *endpoint; + 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, + }; + + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); + host_node = of_graph_get_remote_port_parent(endpoint); + of_node_put(endpoint); + + if (!host_node) + return -EINVAL; + + host = of_find_mipi_dsi_host_by_node(host_node); + of_node_put(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) @@ -466,9 +536,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;
@@ -476,7 +545,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); @@ -486,25 +554,58 @@ static int chipone_probe(struct mipi_dsi_device *dsi) icn->bridge.funcs = &chipone_bridge_funcs; icn->bridge.type = DRM_MODE_CONNECTOR_DPI; icn->bridge.of_node = dev->of_node; + + *icnr = icn; + + return ret; +} + +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;
- drm_bridge_add(&icn->bridge); + mipi_dsi_set_drvdata(dsi, icn);
- dsi->lanes = 4; - 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; + drm_bridge_add(&icn->bridge);
- ret = mipi_dsi_attach(dsi); - if (ret < 0) { + 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 chipone_dsi_host_attach(icn); +} + +static int chipone_dsi_remove(struct mipi_dsi_device *dsi) { struct chipone *icn = mipi_dsi_get_drvdata(dsi);
@@ -520,16 +621,49 @@ 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, + .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");
On Fri, Mar 18, 2022 at 07:47:53PM +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: Maxime Ripard maxime@cerno.tech 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
I still don't get why removing a line in a binding is too hard, but I guess I'll have to do it myself.
Acked-by: Maxime Ripard maxime@cerno.tech
Maxime
Rename and inline macro ICN6211_DSI() into function chipone_writeb() to keep all function names lower-case. No functional change.
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- drivers/gpu/drm/bridge/chipone-icn6211.c | 71 +++++++++++------------- 1 file changed, 31 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index 9fdc0307266d3..7c6dc25082964 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -151,23 +151,14 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge) return container_of(bridge, struct chipone, bridge); }
-static inline int chipone_dsi_write(struct chipone *icn, const u8 *seq, - size_t len) +static int chipone_writeb(struct chipone *icn, u8 reg, u8 val) { - if (icn->interface_i2c) { - return i2c_smbus_write_byte_data(icn->client, seq[0], seq[1]); - } else { - return mipi_dsi_generic_write(icn->dsi, - (u8[]){seq[0], seq[1]}, 2); - } + if (icn->interface_i2c) + return i2c_smbus_write_byte_data(icn->client, reg, val); + else + return 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) { @@ -242,11 +233,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, @@ -265,19 +256,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));
@@ -285,44 +276,44 @@ 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); - 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, SYNC_EVENT_DLY, 0x80); + chipone_writeb(icn, HFP_MIN, hfp & 0xff); + 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); }
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.
Acked-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Marek Vasut marex@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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 --- V2: Rebase on next-20220214 V3: Add AB from Maxime V4: No change V5: No change --- 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 7c6dc25082964..0cd2c32dbdb32 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -151,6 +151,14 @@ static inline struct chipone *bridge_to_chipone(struct drm_bridge *bridge) return container_of(bridge, struct chipone, bridge); }
+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 int chipone_writeb(struct chipone *icn, u8 reg, u8 val) { if (icn->interface_i2c) @@ -249,7 +257,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);
Hey Marek,
On Fri, 18 Mar 2022 at 19:48, Marek Vasut marex@denx.de wrote:
This series fixes multiple problems with the ICN6211 driver and adds support for configuration of the chip via I2C bus.
First, in the current state, the ICN6211 driver hard-codes DPI timing and clock settings specific to some unknown panel. The settings provided by panel driver are ignored. Using any other panel than the one for which this driver is currently hard-coded can lead to permanent damage of the panel (per display supplier warning, and it sure did in my case. The damage looks like multiple rows of dead pixels at the bottom of the panel, and this is not going away even after long power off time).
Much of this series thus fixes incorrect register layout, DPI timing programming, clock generation by adding actual PLL configuration code. This series no longer adds lane count decoding and retains current hard-coded lane count 4 due to disagreement over lane count parsing from DT. The lane count support will come later. The series also fills in a couple of registers with likely correct default values.
Second, this series adds support for I2C configuration of the ICN6211. The device can be configured either via DSI command mode or via I2C, the register layout is the same in both cases.
Since the datasheet for this device is very hard to come by, a lot of information has been salvaged from [1] and [2].
[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/b... [2] https://github.com/tdjastrzebski/ICN6211-Configurator
Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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
Marek Vasut (11): drm: bridge: icn6211: Fix register layout drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling drm: bridge: icn6211: Add HS/VS/DE polarity handling drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode drm: bridge: icn6211: Disable DPI color swap drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples drm: bridge: icn6211: Implement atomic_get_input_bus_fmts drm: bridge: icn6211: Add I2C configuration support drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() drm: bridge: icn6211: Read and validate chip IDs before configuration
drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- 1 file changed, 434 insertions(+), 52 deletions(-)
-- 2.35.1
This series looks ready to be merged, could you fix the remaining 'checkpatch --strict' warnings that are applicable? Ideally the line removal suggested by Maxime would be included too.
On 3/31/22 14:02, Robert Foss wrote:
Hey Marek,
Hi,
On Fri, 18 Mar 2022 at 19:48, Marek Vasut marex@denx.de wrote:
This series fixes multiple problems with the ICN6211 driver and adds support for configuration of the chip via I2C bus.
First, in the current state, the ICN6211 driver hard-codes DPI timing and clock settings specific to some unknown panel. The settings provided by panel driver are ignored. Using any other panel than the one for which this driver is currently hard-coded can lead to permanent damage of the panel (per display supplier warning, and it sure did in my case. The damage looks like multiple rows of dead pixels at the bottom of the panel, and this is not going away even after long power off time).
Much of this series thus fixes incorrect register layout, DPI timing programming, clock generation by adding actual PLL configuration code. This series no longer adds lane count decoding and retains current hard-coded lane count 4 due to disagreement over lane count parsing from DT. The lane count support will come later. The series also fills in a couple of registers with likely correct default values.
Second, this series adds support for I2C configuration of the ICN6211. The device can be configured either via DSI command mode or via I2C, the register layout is the same in both cases.
Since the datasheet for this device is very hard to come by, a lot of information has been salvaged from [1] and [2].
[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/b... [2] https://github.com/tdjastrzebski/ICN6211-Configurator
Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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
Marek Vasut (11): drm: bridge: icn6211: Fix register layout drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling drm: bridge: icn6211: Add HS/VS/DE polarity handling drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode drm: bridge: icn6211: Disable DPI color swap drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples drm: bridge: icn6211: Implement atomic_get_input_bus_fmts drm: bridge: icn6211: Add I2C configuration support drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() drm: bridge: icn6211: Read and validate chip IDs before configuration
drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- 1 file changed, 434 insertions(+), 52 deletions(-)
-- 2.35.1
This series looks ready to be merged
I was waiting for 5.18-rc1 to be out and MW closed before picking it into drm-misc-next . Maybe I can pick it up now already ?
, could you fix the remaining 'checkpatch --strict' warnings that are applicable?
There are only these left, which I think is OK: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
And then this one strict CHECK, but if I change that the formatting looks even uglier: 0010-drm-bridge-icn6211-Rework-ICN6211_DSI-to-chipone_wri.patch
CHECK: Alignment should match open parenthesis #68: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:238: + chipone_writeb(icn, PLL_REF_DIV, (best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2 pre-divider */
CHECK: Alignment should match open parenthesis #97: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:272: + chipone_writeb(icn, VACTIVE_HACTIVE_HI, ((mode->hdisplay >> 8) & 0xf) |
CHECK: Alignment should match open parenthesis #113: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:284: + chipone_writeb(icn, HFP_HSW_HBP_HI, HFP_HSW_HBP_HI_HFP(hfp) |
Ideally the line removal suggested by Maxime would be included too.
This line removal comment has nothing to do with changes in this series, it is about the following patch, which is no longer part of this series because there is ongoing disagreement about that part and OF graph, so that patch will be resubmitted separately later:
[PATCH v4 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
The continuation of that discussion is already in:
[PATCH] dt-bindings: display: bridge: Drop requirement on input port for DSI devices
So this series itself has no outstanding changes pending, unless you really want the uglier formatting above.
On Thu, 31 Mar 2022 at 15:42, Marek Vasut marex@denx.de wrote:
On 3/31/22 14:02, Robert Foss wrote:
Hey Marek,
Hi,
On Fri, 18 Mar 2022 at 19:48, Marek Vasut marex@denx.de wrote:
This series fixes multiple problems with the ICN6211 driver and adds support for configuration of the chip via I2C bus.
First, in the current state, the ICN6211 driver hard-codes DPI timing and clock settings specific to some unknown panel. The settings provided by panel driver are ignored. Using any other panel than the one for which this driver is currently hard-coded can lead to permanent damage of the panel (per display supplier warning, and it sure did in my case. The damage looks like multiple rows of dead pixels at the bottom of the panel, and this is not going away even after long power off time).
Much of this series thus fixes incorrect register layout, DPI timing programming, clock generation by adding actual PLL configuration code. This series no longer adds lane count decoding and retains current hard-coded lane count 4 due to disagreement over lane count parsing from DT. The lane count support will come later. The series also fills in a couple of registers with likely correct default values.
Second, this series adds support for I2C configuration of the ICN6211. The device can be configured either via DSI command mode or via I2C, the register layout is the same in both cases.
Since the datasheet for this device is very hard to come by, a lot of information has been salvaged from [1] and [2].
[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/b... [2] https://github.com/tdjastrzebski/ICN6211-Configurator
Cc: Jagan Teki jagan@amarulasolutions.com Cc: Maxime Ripard maxime@cerno.tech 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
Marek Vasut (11): drm: bridge: icn6211: Fix register layout drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling drm: bridge: icn6211: Add HS/VS/DE polarity handling drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode drm: bridge: icn6211: Disable DPI color swap drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples drm: bridge: icn6211: Implement atomic_get_input_bus_fmts drm: bridge: icn6211: Add I2C configuration support drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() drm: bridge: icn6211: Read and validate chip IDs before configuration
drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- 1 file changed, 434 insertions(+), 52 deletions(-)
-- 2.35.1
This series looks ready to be merged
I was waiting for 5.18-rc1 to be out and MW closed before picking it into drm-misc-next . Maybe I can pick it up now already ?
, could you fix the remaining 'checkpatch --strict' warnings that are applicable?
There are only these left, which I think is OK: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
And then this one strict CHECK, but if I change that the formatting looks even uglier: 0010-drm-bridge-icn6211-Rework-ICN6211_DSI-to-chipone_wri.patch
CHECK: Alignment should match open parenthesis #68: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:238:
chipone_writeb(icn, PLL_REF_DIV, (best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2pre-divider */
CHECK: Alignment should match open parenthesis #97: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:272:
chipone_writeb(icn, VACTIVE_HACTIVE_HI, ((mode->hdisplay >> 8) & 0xf) |CHECK: Alignment should match open parenthesis #113: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:284:
chipone_writeb(icn, HFP_HSW_HBP_HI, HFP_HSW_HBP_HI_HFP(hfp) |
I'd like to at least strive for uniformity, so checkpatch is king, and whatever formatting it preferes is what should be used.
Ideally the line removal suggested by Maxime would be included too.
This line removal comment has nothing to do with changes in this series, it is about the following patch, which is no longer part of this series because there is ongoing disagreement about that part and OF graph, so that patch will be resubmitted separately later:
Ack, thanks for explaining.
[PATCH v4 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing
The continuation of that discussion is already in:
[PATCH] dt-bindings: display: bridge: Drop requirement on input port for DSI devices
So this series itself has no outstanding changes pending, unless you really want the uglier formatting above.
Yes, please resend with this fixed, and I'll merge it.
dri-devel@lists.freedesktop.org
-
Marek Vasut -
Maxime Ripard -
Robert Foss