[PATCH v3 1/7] drm/kmb: Work around for higher system clock

[PATCH v3 1/7] drm/kmb: Work around for higher system clock

[Anitha Chrisanthus]

Use a different value for system clock offset in the
ppl/llp ratio calculations for clocks higher than 500 Mhz.

Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
---
 drivers/gpu/drm/kmb/kmb_dsi.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
index 1793cd31b117..86e8e7943e89 100644
--- a/drivers/gpu/drm/kmb/kmb_dsi.c
+++ b/drivers/gpu/drm/kmb/kmb_dsi.c
@@ -482,6 +482,10 @@ static u32 mipi_tx_fg_section_cfg(struct kmb_dsi *kmb_dsi,
 	return 0;
 }
 
+#define CLK_DIFF_LOW 50
+#define CLK_DIFF_HI 60
+#define SYSCLK_500  500
+
 static void mipi_tx_fg_cfg_regs(struct kmb_dsi *kmb_dsi, u8 frame_gen,
 				struct mipi_tx_frame_timing_cfg *fg_cfg)
 {
@@ -492,7 +496,12 @@ static void mipi_tx_fg_cfg_regs(struct kmb_dsi *kmb_dsi, u8 frame_gen,
 	/* 500 Mhz system clock minus 50 to account for the difference in
 	 * MIPI clock speed in RTL tests
 	 */
-	sysclk = kmb_dsi->sys_clk_mhz - 50;
+	if (kmb_dsi->sys_clk_mhz == SYSCLK_500) {
+		sysclk = kmb_dsi->sys_clk_mhz - CLK_DIFF_LOW;
+	} else {
+		/* 700 Mhz clk*/
+		sysclk = kmb_dsi->sys_clk_mhz - CLK_DIFF_HI;
+	}
 
 	/* PPL-Pixel Packing Layer, LLP-Low Level Protocol
 	 * Frame genartor timing parameters are clocked on the system clock,
-- 
2.25.1

[PATCH v3 2/7] drm/kmb: Limit supported mode to 1080p

[Anitha Chrisanthus]

KMB only supports single resolution(1080p), this commit checks for
1920x1080x60 or 1920x1080x59 in crtc_mode_valid.
Also, modes with vfp < 4 are not supported in KMB display. This change
prunes display modes with vfp < 4.

v2: added vfp check

Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
---
 drivers/gpu/drm/kmb/kmb_crtc.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/kmb/kmb_drv.h  | 13 ++++++++++---
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/kmb/kmb_crtc.c b/drivers/gpu/drm/kmb/kmb_crtc.c
index 44327bc629ca..08a45e813db7 100644
--- a/drivers/gpu/drm/kmb/kmb_crtc.c
+++ b/drivers/gpu/drm/kmb/kmb_crtc.c
@@ -185,11 +185,45 @@ static void kmb_crtc_atomic_flush(struct drm_crtc *crtc,
 	spin_unlock_irq(&crtc->dev->event_lock);
 }
 
+static enum drm_mode_status
+		kmb_crtc_mode_valid(struct drm_crtc *crtc,
+				    const struct drm_display_mode *mode)
+{
+	int refresh;
+	struct drm_device *dev = crtc->dev;
+	int vfp = mode->vsync_start - mode->vdisplay;
+
+	if (mode->vdisplay < KMB_CRTC_MAX_HEIGHT) {
+		drm_dbg(dev, "height = %d less than %d",
+			mode->vdisplay, KMB_CRTC_MAX_HEIGHT);
+		return MODE_BAD_VVALUE;
+	}
+	if (mode->hdisplay < KMB_CRTC_MAX_WIDTH) {
+		drm_dbg(dev, "width = %d less than %d",
+			mode->hdisplay, KMB_CRTC_MAX_WIDTH);
+		return MODE_BAD_HVALUE;
+	}
+	refresh = drm_mode_vrefresh(mode);
+	if (refresh < KMB_MIN_VREFRESH || refresh > KMB_MAX_VREFRESH) {
+		drm_dbg(dev, "refresh = %d less than %d or greater than %d",
+			refresh, KMB_MIN_VREFRESH, KMB_MAX_VREFRESH);
+		return MODE_BAD;
+	}
+
+	if (vfp < KMB_CRTC_MIN_VFP) {
+		drm_dbg(dev, "vfp = %d less than %d", vfp, KMB_CRTC_MIN_VFP);
+		return MODE_BAD;
+	}
+
+	return MODE_OK;
+}
+
 static const struct drm_crtc_helper_funcs kmb_crtc_helper_funcs = {
 	.atomic_begin = kmb_crtc_atomic_begin,
 	.atomic_enable = kmb_crtc_atomic_enable,
 	.atomic_disable = kmb_crtc_atomic_disable,
 	.atomic_flush = kmb_crtc_atomic_flush,
+	.mode_valid = kmb_crtc_mode_valid,
 };
 
 int kmb_setup_crtc(struct drm_device *drm)
diff --git a/drivers/gpu/drm/kmb/kmb_drv.h b/drivers/gpu/drm/kmb/kmb_drv.h
index 69a62e2d03ff..d297218869e8 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.h
+++ b/drivers/gpu/drm/kmb/kmb_drv.h
@@ -18,13 +18,20 @@
 
 #define DRIVER_DATE			"20210223"
 #define DRIVER_MAJOR			1
-#define DRIVER_MINOR			1
-
+#define DRIVER_MINOR			2
+
+/* Platform definitions */
+#define KMB_CRTC_MIN_VFP		4
+#define KMB_CRTC_MAX_WIDTH		1920 /* max width in pixels */
+#define KMB_CRTC_MAX_HEIGHT		1080 /* max height in pixels */
+#define KMB_CRTC_MIN_WIDTH		1920
+#define KMB_CRTC_MIN_HEIGHT		1080
 #define KMB_FB_MAX_WIDTH		1920
 #define KMB_FB_MAX_HEIGHT		1080
 #define KMB_FB_MIN_WIDTH		1
 #define KMB_FB_MIN_HEIGHT		1
-
+#define KMB_MIN_VREFRESH		59    /*vertical refresh in Hz */
+#define KMB_MAX_VREFRESH		60    /*vertical refresh in Hz */
 #define KMB_LCD_DEFAULT_CLK		200000000
 #define KMB_SYS_CLK_MHZ			500
 
-- 
2.25.1

[PATCH v3 3/7] drm/kmb: Remove clearing DPHY regs

[Anitha Chrisanthus]

From: Edmund Dea <edmund.j.dea@intel.com>

Don't clear the shared DPHY registers common to MIPI Rx and MIPI Tx during
DSI initialization since this was causing MIPI Rx reset. Rest of the
writes are bitwise, so will not affect Mipi Rx side.

Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
---
 drivers/gpu/drm/kmb/kmb_dsi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
index 86e8e7943e89..a0669b842ff5 100644
--- a/drivers/gpu/drm/kmb/kmb_dsi.c
+++ b/drivers/gpu/drm/kmb/kmb_dsi.c
@@ -1393,11 +1393,6 @@ int kmb_dsi_mode_set(struct kmb_dsi *kmb_dsi, struct drm_display_mode *mode,
 		mipi_tx_init_cfg.lane_rate_mbps = data_rate;
 	}
 
-	kmb_write_mipi(kmb_dsi, DPHY_ENABLE, 0);
-	kmb_write_mipi(kmb_dsi, DPHY_INIT_CTRL0, 0);
-	kmb_write_mipi(kmb_dsi, DPHY_INIT_CTRL1, 0);
-	kmb_write_mipi(kmb_dsi, DPHY_INIT_CTRL2, 0);
-
 	/* Initialize mipi controller */
 	mipi_tx_init_cntrl(kmb_dsi, &mipi_tx_init_cfg);
 
-- 
2.25.1

[PATCH v3 4/7] drm/kmb: Disable change of plane parameters

[Anitha Chrisanthus]

From: Edmund Dea <edmund.j.dea@intel.com>

Due to HW limitations, KMB cannot change height, width, or
pixel format after initial plane configuration.

v2: removed memset disp_cfg as it is already zero.

Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
---
 drivers/gpu/drm/kmb/kmb_drv.h   |  1 +
 drivers/gpu/drm/kmb/kmb_plane.c | 43 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/kmb/kmb_plane.h |  6 +++++
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/kmb/kmb_drv.h b/drivers/gpu/drm/kmb/kmb_drv.h
index d297218869e8..b3203f583a46 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.h
+++ b/drivers/gpu/drm/kmb/kmb_drv.h
@@ -57,6 +57,7 @@ struct kmb_drm_private {
 	spinlock_t			irq_lock;
 	int				irq_lcd;
 	int				sys_clk_mhz;
+	struct disp_cfg			init_disp_cfg[KMB_MAX_PLANES];
 	struct layer_status		plane_status[KMB_MAX_PLANES];
 	int				kmb_under_flow;
 	int				kmb_flush_done;
diff --git a/drivers/gpu/drm/kmb/kmb_plane.c b/drivers/gpu/drm/kmb/kmb_plane.c
index 06b0c42c9e91..00404ba4126d 100644
--- a/drivers/gpu/drm/kmb/kmb_plane.c
+++ b/drivers/gpu/drm/kmb/kmb_plane.c
@@ -67,8 +67,21 @@ static const u32 kmb_formats_v[] = {
 
 static unsigned int check_pixel_format(struct drm_plane *plane, u32 format)
 {
+	struct kmb_drm_private *kmb;
+	struct kmb_plane *kmb_plane = to_kmb_plane(plane);
 	int i;
+	int plane_id = kmb_plane->id;
+	struct disp_cfg init_disp_cfg;
 
+	kmb = to_kmb(plane->dev);
+	init_disp_cfg = kmb->init_disp_cfg[plane_id];
+	/* Due to HW limitations, changing pixel format after initial
+	 * plane configuration is not supported.
+	 */
+	if (init_disp_cfg.format && init_disp_cfg.format != format) {
+		drm_dbg(&kmb->drm, "Cannot change format after initial plane configuration");
+		return -EINVAL;
+	}
 	for (i = 0; i < plane->format_count; i++) {
 		if (plane->format_types[i] == format)
 			return 0;
@@ -81,11 +94,17 @@ static int kmb_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
+	struct kmb_drm_private *kmb;
+	struct kmb_plane *kmb_plane = to_kmb_plane(plane);
+	int plane_id = kmb_plane->id;
+	struct disp_cfg init_disp_cfg;
 	struct drm_framebuffer *fb;
 	int ret;
 	struct drm_crtc_state *crtc_state;
 	bool can_position;
 
+	kmb = to_kmb(plane->dev);
+	init_disp_cfg = kmb->init_disp_cfg[plane_id];
 	fb = new_plane_state->fb;
 	if (!fb || !new_plane_state->crtc)
 		return 0;
@@ -99,6 +118,16 @@ static int kmb_plane_atomic_check(struct drm_plane *plane,
 	    new_plane_state->crtc_w < KMB_FB_MIN_WIDTH ||
 	    new_plane_state->crtc_h < KMB_FB_MIN_HEIGHT)
 		return -EINVAL;
+
+	/* Due to HW limitations, changing plane height or width after
+	 * initial plane configuration is not supported.
+	 */
+	if ((init_disp_cfg.width && init_disp_cfg.height) &&
+	    (init_disp_cfg.width != fb->width ||
+	    init_disp_cfg.height != fb->height)) {
+		drm_dbg(&kmb->drm, "Cannot change plane height or width after initial configuration");
+		return -EINVAL;
+	}
 	can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
 	crtc_state =
 		drm_atomic_get_existing_crtc_state(state,
@@ -335,6 +364,7 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
 	unsigned char plane_id;
 	int num_planes;
 	static dma_addr_t addr[MAX_SUB_PLANES];
+	struct disp_cfg *init_disp_cfg;
 
 	if (!plane || !new_plane_state || !old_plane_state)
 		return;
@@ -357,7 +387,8 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
 	}
 	spin_unlock_irq(&kmb->irq_lock);
 
-	src_w = (new_plane_state->src_w >> 16);
+	init_disp_cfg = &kmb->init_disp_cfg[plane_id];
+	src_w = new_plane_state->src_w >> 16;
 	src_h = new_plane_state->src_h >> 16;
 	crtc_x = new_plane_state->crtc_x;
 	crtc_y = new_plane_state->crtc_y;
@@ -500,6 +531,16 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
 
 	/* Enable DMA */
 	kmb_write_lcd(kmb, LCD_LAYERn_DMA_CFG(plane_id), dma_cfg);
+
+	/* Save initial display config */
+	if (!init_disp_cfg->width ||
+	    !init_disp_cfg->height ||
+	    !init_disp_cfg->format) {
+		init_disp_cfg->width = width;
+		init_disp_cfg->height = height;
+		init_disp_cfg->format = fb->format->format;
+	}
+
 	drm_dbg(&kmb->drm, "dma_cfg=0x%x LCD_DMA_CFG=0x%x\n", dma_cfg,
 		kmb_read_lcd(kmb, LCD_LAYERn_DMA_CFG(plane_id)));
 
diff --git a/drivers/gpu/drm/kmb/kmb_plane.h b/drivers/gpu/drm/kmb/kmb_plane.h
index 6e8d22cf8819..b51144044fe8 100644
--- a/drivers/gpu/drm/kmb/kmb_plane.h
+++ b/drivers/gpu/drm/kmb/kmb_plane.h
@@ -63,6 +63,12 @@ struct layer_status {
 	u32 ctrl;
 };
 
+struct disp_cfg {
+	unsigned int width;
+	unsigned int height;
+	unsigned int format;
+};
+
 struct kmb_plane *kmb_plane_init(struct drm_device *drm);
 void kmb_plane_destroy(struct drm_plane *plane);
 #endif /* __KMB_PLANE_H__ */
-- 
2.25.1

[PATCH v3 5/7] drm/kmb: Corrected typo in handle_lcd_irq

[Anitha Chrisanthus]

Check for Overflow bits for layer3 in the irq handler.

Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
---
 drivers/gpu/drm/kmb/kmb_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index 12ce669650cc..961ac6fb5fcf 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -380,7 +380,7 @@ static irqreturn_t handle_lcd_irq(struct drm_device *dev)
 		if (val & LAYER3_DMA_FIFO_UNDERFLOW)
 			drm_dbg(&kmb->drm,
 				"LAYER3:GL1 DMA UNDERFLOW val = 0x%lx", val);
-		if (val & LAYER3_DMA_FIFO_UNDERFLOW)
+		if (val & LAYER3_DMA_FIFO_OVERFLOW)
 			drm_dbg(&kmb->drm,
 				"LAYER3:GL1 DMA OVERFLOW val = 0x%lx", val);
 	}
-- 
2.25.1

[PATCH v3 6/7] drm/kmb: Enable ADV bridge after modeset

[Anitha Chrisanthus]

On KMB, ADV bridge must be programmed and powered on prior to
MIPI DSI HW initialization.

Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
---
 drivers/gpu/drm/kmb/kmb_dsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
index a0669b842ff5..7ab6b7b44cbc 100644
--- a/drivers/gpu/drm/kmb/kmb_dsi.c
+++ b/drivers/gpu/drm/kmb/kmb_dsi.c
@@ -1341,6 +1341,7 @@ static void connect_lcd_to_mipi(struct kmb_dsi *kmb_dsi)
 		return;
 	}
 
+	drm_bridge_chain_enable(adv_bridge);
 	/* DISABLE MIPI->CIF CONNECTION */
 	regmap_write(msscam, MSS_MIPI_CIF_CFG, 0);
 
-- 
2.25.1

[PATCH v3 7/7] drm/kmb: Enable support for framebuffer console

[Anitha Chrisanthus]

Enable support for fbcon (framebuffer console).
The user can initialize fbcon by loading kmb-drm with the parameter
console=1.

v2: added missing static clk_enable

Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
---
 drivers/gpu/drm/kmb/kmb_drv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index 961ac6fb5fcf..b4e66eac63b5 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -5,6 +5,7 @@
 
 #include <linux/clk.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/of_graph.h>
 #include <linux/of_platform.h>
 #include <linux/of_reserved_mem.h>
@@ -15,6 +16,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_probe_helper.h>
@@ -24,6 +26,12 @@
 #include "kmb_dsi.h"
 #include "kmb_regs.h"
 
+/* Module Parameters */
+static bool console;
+module_param(console, bool, 0400);
+MODULE_PARM_DESC(console,
+		 "Enable framebuffer console support (0=disable [default], 1=on)");
+
 static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
 {
 	int ret = 0;
@@ -559,6 +567,9 @@ static int kmb_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_register;
 
+	if (console)
+		drm_fbdev_generic_setup(&kmb->drm, 32);
+
 	return 0;
 
  err_register:
-- 
2.25.1

Re: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2082 bytes --]

Hi

Am 14.10.21 um 01:36 schrieb Anitha Chrisanthus:
> Enable support for fbcon (framebuffer console).
> The user can initialize fbcon by loading kmb-drm with the parameter
> console=1.
> 
> v2: added missing static clk_enable
> 
> Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> ---
>   drivers/gpu/drm/kmb/kmb_drv.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
> index 961ac6fb5fcf..b4e66eac63b5 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -5,6 +5,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/module.h>
> +#include <linux/moduleparam.h>
>   #include <linux/of_graph.h>
>   #include <linux/of_platform.h>
>   #include <linux/of_reserved_mem.h>
> @@ -15,6 +16,7 @@
>   
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
>   #include <drm/drm_gem_cma_helper.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_probe_helper.h>
> @@ -24,6 +26,12 @@
>   #include "kmb_dsi.h"
>   #include "kmb_regs.h"
>   
> +/* Module Parameters */
> +static bool console;
> +module_param(console, bool, 0400);
> +MODULE_PARM_DESC(console,
> +		 "Enable framebuffer console support (0=disable [default], 1=on)");

There's already fbdev_emulation in drm_fb_helper.c. No need for a 
separate parameter here.

Best regards
Thomas

> +
>   static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
>   {
>   	int ret = 0;
> @@ -559,6 +567,9 @@ static int kmb_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto err_register;
>   
> +	if (console)
> +		drm_fbdev_generic_setup(&kmb->drm, 32);
> +
>   	return 0;
>   
>    err_register:
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2342 bytes --]

Hi

Am 14.10.21 um 01:36 schrieb Anitha Chrisanthus:
> Enable support for fbcon (framebuffer console).
> The user can initialize fbcon by loading kmb-drm with the parameter
> console=1.
> 
> v2: added missing static clk_enable
> 
> Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> ---
>   drivers/gpu/drm/kmb/kmb_drv.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
> index 961ac6fb5fcf..b4e66eac63b5 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -5,6 +5,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/module.h>
> +#include <linux/moduleparam.h>
>   #include <linux/of_graph.h>
>   #include <linux/of_platform.h>
>   #include <linux/of_reserved_mem.h>
> @@ -15,6 +16,7 @@
>   
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
>   #include <drm/drm_gem_cma_helper.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_probe_helper.h>
> @@ -24,6 +26,12 @@
>   #include "kmb_dsi.h"
>   #include "kmb_regs.h"
>   
> +/* Module Parameters */
> +static bool console;
> +module_param(console, bool, 0400);
> +MODULE_PARM_DESC(console,
> +		 "Enable framebuffer console support (0=disable [default], 1=on)");
> +
>   static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
>   {
>   	int ret = 0;
> @@ -559,6 +567,9 @@ static int kmb_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto err_register;
>   
> +	if (console)
> +		drm_fbdev_generic_setup(&kmb->drm, 32);

The use of the final parameter is somewhat confusing and should probably 
be 0. It's the number of bits per pixel. 32 is the default. But the 
preferred way of selecting color mode is via 
dev->mode_config.preferred_depth, which is the color depth (24!). So 
maybe rather set preferred_depth to 24 and let the fbdev helper figure 
out the details.

Best regards
Thomas

> +
>   	return 0;
>   
>    err_register:
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 1/7] drm/kmb: Work around for higher system clock

[Sam Ravnborg]

Hi Anitha,

On Wed, Oct 13, 2021 at 04:36:26PM -0700, Anitha Chrisanthus wrote:
> Use a different value for system clock offset in the
> ppl/llp ratio calculations for clocks higher than 500 Mhz.
> 
> Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>

Looks fine.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

Re: [PATCH v3 2/7] drm/kmb: Limit supported mode to 1080p

[Sam Ravnborg]

On Wed, Oct 13, 2021 at 04:36:27PM -0700, Anitha Chrisanthus wrote:
> KMB only supports single resolution(1080p), this commit checks for
> 1920x1080x60 or 1920x1080x59 in crtc_mode_valid.
> Also, modes with vfp < 4 are not supported in KMB display. This change
> prunes display modes with vfp < 4.
> 
> v2: added vfp check
> 
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>

This looks wrong. I assume it Edmund also did some development work so
you should also add:
Co-developed-by: Edmund Dea <edmund.j.dea@intel.com>

One nit below. With these nits fixed:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

> ---
>  drivers/gpu/drm/kmb/kmb_crtc.c | 34 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/kmb/kmb_drv.h  | 13 ++++++++++---
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/kmb/kmb_crtc.c b/drivers/gpu/drm/kmb/kmb_crtc.c
> index 44327bc629ca..08a45e813db7 100644
> --- a/drivers/gpu/drm/kmb/kmb_crtc.c
> +++ b/drivers/gpu/drm/kmb/kmb_crtc.c
> @@ -185,11 +185,45 @@ static void kmb_crtc_atomic_flush(struct drm_crtc *crtc,
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  }
>  
> +static enum drm_mode_status
> +		kmb_crtc_mode_valid(struct drm_crtc *crtc,
> +				    const struct drm_display_mode *mode)
> +{
> +	int refresh;
> +	struct drm_device *dev = crtc->dev;
> +	int vfp = mode->vsync_start - mode->vdisplay;
> +
> +	if (mode->vdisplay < KMB_CRTC_MAX_HEIGHT) {
> +		drm_dbg(dev, "height = %d less than %d",
> +			mode->vdisplay, KMB_CRTC_MAX_HEIGHT);
> +		return MODE_BAD_VVALUE;
> +	}
> +	if (mode->hdisplay < KMB_CRTC_MAX_WIDTH) {
> +		drm_dbg(dev, "width = %d less than %d",
> +			mode->hdisplay, KMB_CRTC_MAX_WIDTH);
> +		return MODE_BAD_HVALUE;
> +	}
> +	refresh = drm_mode_vrefresh(mode);
> +	if (refresh < KMB_MIN_VREFRESH || refresh > KMB_MAX_VREFRESH) {
> +		drm_dbg(dev, "refresh = %d less than %d or greater than %d",
> +			refresh, KMB_MIN_VREFRESH, KMB_MAX_VREFRESH);
> +		return MODE_BAD;
> +	}
> +
> +	if (vfp < KMB_CRTC_MIN_VFP) {
> +		drm_dbg(dev, "vfp = %d less than %d", vfp, KMB_CRTC_MIN_VFP);
> +		return MODE_BAD;
> +	}
> +
> +	return MODE_OK;
> +}
> +
>  static const struct drm_crtc_helper_funcs kmb_crtc_helper_funcs = {
>  	.atomic_begin = kmb_crtc_atomic_begin,
>  	.atomic_enable = kmb_crtc_atomic_enable,
>  	.atomic_disable = kmb_crtc_atomic_disable,
>  	.atomic_flush = kmb_crtc_atomic_flush,
> +	.mode_valid = kmb_crtc_mode_valid,
>  };
>  
>  int kmb_setup_crtc(struct drm_device *drm)
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.h b/drivers/gpu/drm/kmb/kmb_drv.h
> index 69a62e2d03ff..d297218869e8 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.h
> +++ b/drivers/gpu/drm/kmb/kmb_drv.h
> @@ -18,13 +18,20 @@
>  
>  #define DRIVER_DATE			"20210223"
>  #define DRIVER_MAJOR			1
> -#define DRIVER_MINOR			1
> -
> +#define DRIVER_MINOR			2
This change looks un-related.

> +
> +/* Platform definitions */
> +#define KMB_CRTC_MIN_VFP		4
> +#define KMB_CRTC_MAX_WIDTH		1920 /* max width in pixels */
> +#define KMB_CRTC_MAX_HEIGHT		1080 /* max height in pixels */
> +#define KMB_CRTC_MIN_WIDTH		1920
> +#define KMB_CRTC_MIN_HEIGHT		1080
>  #define KMB_FB_MAX_WIDTH		1920
>  #define KMB_FB_MAX_HEIGHT		1080
>  #define KMB_FB_MIN_WIDTH		1
>  #define KMB_FB_MIN_HEIGHT		1
> -
> +#define KMB_MIN_VREFRESH		59    /*vertical refresh in Hz */
> +#define KMB_MAX_VREFRESH		60    /*vertical refresh in Hz */
>  #define KMB_LCD_DEFAULT_CLK		200000000
>  #define KMB_SYS_CLK_MHZ			500
>  
> -- 
> 2.25.1

Re: [PATCH v3 3/7] drm/kmb: Remove clearing DPHY regs

[Sam Ravnborg]

On Wed, Oct 13, 2021 at 04:36:28PM -0700, Anitha Chrisanthus wrote:
> From: Edmund Dea <edmund.j.dea@intel.com>
> 
> Don't clear the shared DPHY registers common to MIPI Rx and MIPI Tx during
> DSI initialization since this was causing MIPI Rx reset. Rest of the
> writes are bitwise, so will not affect Mipi Rx side.
> 
> Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>

As Edmund is author, he should be listed first. The s-o-b lines
expressed the order.
With this fixed.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/kmb/kmb_dsi.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
> index 86e8e7943e89..a0669b842ff5 100644
> --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> @@ -1393,11 +1393,6 @@ int kmb_dsi_mode_set(struct kmb_dsi *kmb_dsi, struct drm_display_mode *mode,
>  		mipi_tx_init_cfg.lane_rate_mbps = data_rate;
>  	}
>  
> -	kmb_write_mipi(kmb_dsi, DPHY_ENABLE, 0);
> -	kmb_write_mipi(kmb_dsi, DPHY_INIT_CTRL0, 0);
> -	kmb_write_mipi(kmb_dsi, DPHY_INIT_CTRL1, 0);
> -	kmb_write_mipi(kmb_dsi, DPHY_INIT_CTRL2, 0);
> -
>  	/* Initialize mipi controller */
>  	mipi_tx_init_cntrl(kmb_dsi, &mipi_tx_init_cfg);
>  
> -- 
> 2.25.1

Re: [PATCH v3 4/7] drm/kmb: Disable change of plane parameters

[Sam Ravnborg]

Hi Anitha,

On Wed, Oct 13, 2021 at 04:36:29PM -0700, Anitha Chrisanthus wrote:
> From: Edmund Dea <edmund.j.dea@intel.com>
> 
> Due to HW limitations, KMB cannot change height, width, or
> pixel format after initial plane configuration.
> 
> v2: removed memset disp_cfg as it is already zero.
> 
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>

Looks fine and the s-o-b order is OK.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

Re: [PATCH v3 5/7] drm/kmb: Corrected typo in handle_lcd_irq

[Sam Ravnborg]

Hi Anitha,

On Wed, Oct 13, 2021 at 04:36:30PM -0700, Anitha Chrisanthus wrote:
> Check for Overflow bits for layer3 in the irq handler.
> 
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>

Obvious fix,
Acked-by: Sam Ravnborg <sam@ravnborg.org>

Re: [PATCH v3 6/7] drm/kmb: Enable ADV bridge after modeset

[Sam Ravnborg]

Hi Anitha,

On Wed, Oct 13, 2021 at 04:36:31PM -0700, Anitha Chrisanthus wrote:
> On KMB, ADV bridge must be programmed and powered on prior to
> MIPI DSI HW initialization.
> 
> Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>

> ---
>  drivers/gpu/drm/kmb/kmb_dsi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
> index a0669b842ff5..7ab6b7b44cbc 100644
> --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> @@ -1341,6 +1341,7 @@ static void connect_lcd_to_mipi(struct kmb_dsi *kmb_dsi)
>  		return;
>  	}
>  
> +	drm_bridge_chain_enable(adv_bridge);

Do not use drm_bridge_chain_enable(). If you really need to do this hack
use the atomic variant. The one you use here is about to be deleted.

That said - I expect this to be a workaround for some design issue.
One bridge driver should not need to power on another bridge, the
infrastructure should take care.
I know we did not make kmb_dsi a proper bridge, but this may be the time
to do it so we avoid such a workaround and the driver will then be able
to use the bridge infrastructure as it is supposed to do.

	Sam

>  	/* DISABLE MIPI->CIF CONNECTION */
>  	regmap_write(msscam, MSS_MIPI_CIF_CFG, 0);
>  
> -- 
> 2.25.1

RE: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console

[Chrisanthus, Anitha]

Hi Thomas,
Thanks for your review.

> -----Original Message-----
> From: Thomas Zimmermann <tzimmermann@suse.de>
> Sent: Thursday, October 14, 2021 6:08 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>; dri-
> devel@lists.freedesktop.org
> Cc: sam@ravnborg.org; Dea, Edmund J <edmund.j.dea@intel.com>
> Subject: Re: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console
> 
> Hi
> 
> Am 14.10.21 um 01:36 schrieb Anitha Chrisanthus:
> > Enable support for fbcon (framebuffer console).
> > The user can initialize fbcon by loading kmb-drm with the parameter
> > console=1.
> >
> > v2: added missing static clk_enable
> >
> > Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
> > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> > ---
> >   drivers/gpu/drm/kmb/kmb_drv.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index 961ac6fb5fcf..b4e66eac63b5 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -5,6 +5,7 @@
> >
> >   #include <linux/clk.h>
> >   #include <linux/module.h>
> > +#include <linux/moduleparam.h>
> >   #include <linux/of_graph.h>
> >   #include <linux/of_platform.h>
> >   #include <linux/of_reserved_mem.h>
> > @@ -15,6 +16,7 @@
> >
> >   #include <drm/drm_atomic_helper.h>
> >   #include <drm/drm_drv.h>
> > +#include <drm/drm_fb_helper.h>
> >   #include <drm/drm_gem_cma_helper.h>
> >   #include <drm/drm_gem_framebuffer_helper.h>
> >   #include <drm/drm_probe_helper.h>
> > @@ -24,6 +26,12 @@
> >   #include "kmb_dsi.h"
> >   #include "kmb_regs.h"
> >
> > +/* Module Parameters */
> > +static bool console;
> > +module_param(console, bool, 0400);
> > +MODULE_PARM_DESC(console,
> > +		 "Enable framebuffer console support (0=disable [default],
> 1=on)");
> > +
> >   static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
> >   {
> >   	int ret = 0;
> > @@ -559,6 +567,9 @@ static int kmb_probe(struct platform_device *pdev)
> >   	if (ret)
> >   		goto err_register;
> >
> > +	if (console)
> > +		drm_fbdev_generic_setup(&kmb->drm, 32);
> 
> The use of the final parameter is somewhat confusing and should probably
> be 0. It's the number of bits per pixel. 32 is the default. But the
> preferred way of selecting color mode is via
> dev->mode_config.preferred_depth, which is the color depth (24!). So
> maybe rather set preferred_depth to 24 and let the fbdev helper figure
> out the details.
Will change it in v4.
> 
> Best regards
> Thomas
> 
> > +
> >   	return 0;
> >
> >    err_register:
> >
> 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer

RE: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console

[Chrisanthus, Anitha]

Hi Thomas,
Thanks for your review.

> -----Original Message-----
> From: Thomas Zimmermann <tzimmermann@suse.de>
> Sent: Thursday, October 14, 2021 5:50 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>; dri-
> devel@lists.freedesktop.org
> Cc: sam@ravnborg.org; Dea, Edmund J <edmund.j.dea@intel.com>
> Subject: Re: [PATCH v3 7/7] drm/kmb: Enable support for framebuffer console
> 
> Hi
> 
> Am 14.10.21 um 01:36 schrieb Anitha Chrisanthus:
> > Enable support for fbcon (framebuffer console).
> > The user can initialize fbcon by loading kmb-drm with the parameter
> > console=1.
> >
> > v2: added missing static clk_enable
> >
> > Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
> > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> > ---
> >   drivers/gpu/drm/kmb/kmb_drv.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> > index 961ac6fb5fcf..b4e66eac63b5 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -5,6 +5,7 @@
> >
> >   #include <linux/clk.h>
> >   #include <linux/module.h>
> > +#include <linux/moduleparam.h>
> >   #include <linux/of_graph.h>
> >   #include <linux/of_platform.h>
> >   #include <linux/of_reserved_mem.h>
> > @@ -15,6 +16,7 @@
> >
> >   #include <drm/drm_atomic_helper.h>
> >   #include <drm/drm_drv.h>
> > +#include <drm/drm_fb_helper.h>
> >   #include <drm/drm_gem_cma_helper.h>
> >   #include <drm/drm_gem_framebuffer_helper.h>
> >   #include <drm/drm_probe_helper.h>
> > @@ -24,6 +26,12 @@
> >   #include "kmb_dsi.h"
> >   #include "kmb_regs.h"
> >
> > +/* Module Parameters */
> > +static bool console;
> > +module_param(console, bool, 0400);
> > +MODULE_PARM_DESC(console,
> > +		 "Enable framebuffer console support (0=disable [default],
> 1=on)");
> 
> There's already fbdev_emulation in drm_fb_helper.c. No need for a
> separate parameter here.
Good catch, I'll change it in V4.
> 
> Best regards
> Thomas
> 
> > +
> >   static int kmb_display_clk_enable(struct kmb_drm_private *kmb)
> >   {
> >   	int ret = 0;
> > @@ -559,6 +567,9 @@ static int kmb_probe(struct platform_device *pdev)
> >   	if (ret)
> >   		goto err_register;
> >
> > +	if (console)
> > +		drm_fbdev_generic_setup(&kmb->drm, 32);
> > +
> >   	return 0;
> >
> >    err_register:
> >
> 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer