Changes in v2:
- Rebased over next-20231213
This series performs some cleanups for mtk_dsi, enhancing human
readability, using kernel provided macros where possible and
also reducing code size.
Tested on MT8173 and MT8192 Chromebooks (using a DSI<->DP bridge)
and on MT6795 Sony Xperia M5 (DSI video mode panel).
AngeloGioacchino Del Regno (4):
drm/mediatek: dsi: Use GENMASK() for register mask definitions
drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()
drm/mediatek: dsi: Use bitfield macros where useful
drm/mediatek: dsi: Replace open-coded instance of HZ_PER_MHZ
drivers/gpu/drm/mediatek/mtk_dsi.c | 198 +++++++++++++----------------
1 file changed, 88 insertions(+), 110 deletions(-)
--
2.43.0
Change magic numerical masks with usage of the GENMASK() macro
to improve readability.
This commit brings no functional changes.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 46 ++++++++++++++++--------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index a2fdfc8ddb15..23d2c5be8dbb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -58,18 +58,18 @@
#define DSI_TXRX_CTRL 0x18
#define VC_NUM BIT(1)
-#define LANE_NUM (0xf << 2)
+#define LANE_NUM GENMASK(5, 2)
#define DIS_EOT BIT(6)
#define NULL_EN BIT(7)
#define TE_FREERUN BIT(8)
#define EXT_TE_EN BIT(9)
#define EXT_TE_EDGE BIT(10)
-#define MAX_RTN_SIZE (0xf << 12)
+#define MAX_RTN_SIZE GENMASK(15, 12)
#define HSTX_CKLP_EN BIT(16)
#define DSI_PSCTRL 0x1c
-#define DSI_PS_WC 0x3fff
-#define DSI_PS_SEL (3 << 16)
+#define DSI_PS_WC GENMASK(14, 0)
+#define DSI_PS_SEL GENMASK(19, 16)
#define PACKED_PS_16BIT_RGB565 (0 << 16)
#define LOOSELY_PS_18BIT_RGB666 (1 << 16)
#define PACKED_PS_18BIT_RGB666 (2 << 16)
@@ -109,26 +109,27 @@
#define LD0_WAKEUP_EN BIT(2)
#define DSI_PHY_TIMECON0 0x110
-#define LPX (0xff << 0)
-#define HS_PREP (0xff << 8)
-#define HS_ZERO (0xff << 16)
-#define HS_TRAIL (0xff << 24)
+#define LPX GENMASK(7, 0)
+#define HS_PREP GENMASK(15, 8)
+#define HS_ZERO GENMASK(23, 16)
+#define HS_TRAIL GENMASK(31, 24)
#define DSI_PHY_TIMECON1 0x114
-#define TA_GO (0xff << 0)
-#define TA_SURE (0xff << 8)
-#define TA_GET (0xff << 16)
-#define DA_HS_EXIT (0xff << 24)
+#define TA_GO GENMASK(7, 0)
+#define TA_SURE GENMASK(15, 8)
+#define TA_GET GENMASK(23, 16)
+#define DA_HS_EXIT GENMASK(31, 24)
#define DSI_PHY_TIMECON2 0x118
-#define CONT_DET (0xff << 0)
-#define CLK_ZERO (0xff << 16)
-#define CLK_TRAIL (0xff << 24)
+#define CONT_DET GENMASK(7, 0)
+#define DA_HS_SYNC GENMASK(15, 8)
+#define CLK_ZERO GENMASK(23, 16)
+#define CLK_TRAIL GENMASK(31, 24)
#define DSI_PHY_TIMECON3 0x11c
-#define CLK_HS_PREP (0xff << 0)
-#define CLK_HS_POST (0xff << 8)
-#define CLK_HS_EXIT (0xff << 16)
+#define CLK_HS_PREP GENMASK(7, 0)
+#define CLK_HS_POST GENMASK(15, 8)
+#define CLK_HS_EXIT GENMASK(23, 16)
#define DSI_VM_CMD_CON 0x130
#define VM_CMD_EN BIT(0)
@@ -138,13 +139,14 @@
#define FORCE_COMMIT BIT(0)
#define BYPASS_SHADOW BIT(1)
-#define CONFIG (0xff << 0)
+/* CMDQ related bits */
+#define CONFIG GENMASK(7, 0)
#define SHORT_PACKET 0
#define LONG_PACKET 2
#define BTA BIT(2)
-#define DATA_ID (0xff << 8)
-#define DATA_0 (0xff << 16)
-#define DATA_1 (0xff << 24)
+#define DATA_ID GENMASK(15, 8)
+#define DATA_0 GENMASK(23, 16)
+#define DATA_1 GENMASK(31, 24)
#define NS_TO_CYCLE(n, c) ((n) / (c) + (((n) % (c)) ? 1 : 0))
--
2.43.0
Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
merge the two in one mtk_dsi_ps_control() function by adding one
function parameter `config_vact` which, when true, writes the VACT
related registers.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +++++++++---------------------
1 file changed, 23 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 23d2c5be8dbb..b618e2e31022 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -352,40 +352,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
}
-static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
-{
- struct videomode *vm = &dsi->vm;
- u32 dsi_buf_bpp, ps_wc;
- u32 ps_bpp_mode;
-
- if (dsi->format == MIPI_DSI_FMT_RGB565)
- dsi_buf_bpp = 2;
- else
- dsi_buf_bpp = 3;
-
- ps_wc = vm->hactive * dsi_buf_bpp;
- ps_bpp_mode = ps_wc;
-
- switch (dsi->format) {
- case MIPI_DSI_FMT_RGB888:
- ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
- break;
- case MIPI_DSI_FMT_RGB666:
- ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
- break;
- case MIPI_DSI_FMT_RGB666_PACKED:
- ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
- break;
- case MIPI_DSI_FMT_RGB565:
- ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
- break;
- }
-
- writel(vm->vactive, dsi->regs + DSI_VACT_NL);
- writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
- writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
-}
-
static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
{
u32 tmp_reg;
@@ -417,36 +383,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
}
-static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
+static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
{
- u32 dsi_tmp_buf_bpp;
- u32 tmp_reg;
+ struct videomode *vm = &dsi->vm;
+ u32 dsi_buf_bpp, ps_wc;
+ u32 ps_bpp_mode;
+
+ if (dsi->format == MIPI_DSI_FMT_RGB565)
+ dsi_buf_bpp = 2;
+ else
+ dsi_buf_bpp = 3;
+
+ ps_wc = vm->hactive * dsi_buf_bpp;
+ ps_bpp_mode = ps_wc;
switch (dsi->format) {
case MIPI_DSI_FMT_RGB888:
- tmp_reg = PACKED_PS_24BIT_RGB888;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
break;
case MIPI_DSI_FMT_RGB666:
- tmp_reg = LOOSELY_PS_18BIT_RGB666;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
break;
case MIPI_DSI_FMT_RGB666_PACKED:
- tmp_reg = PACKED_PS_18BIT_RGB666;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
break;
case MIPI_DSI_FMT_RGB565:
- tmp_reg = PACKED_PS_16BIT_RGB565;
- dsi_tmp_buf_bpp = 2;
- break;
- default:
- tmp_reg = PACKED_PS_24BIT_RGB888;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
break;
}
- tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC;
- writel(tmp_reg, dsi->regs + DSI_PSCTRL);
+ if (config_vact) {
+ writel(vm->vactive, dsi->regs + DSI_VACT_NL);
+ writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
+ }
+ writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
}
static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
@@ -522,7 +492,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
- mtk_dsi_ps_control(dsi);
+ mtk_dsi_ps_control(dsi, false);
}
static void mtk_dsi_start(struct mtk_dsi *dsi)
@@ -667,7 +637,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
mtk_dsi_reset_engine(dsi);
mtk_dsi_phy_timconfig(dsi);
- mtk_dsi_ps_control_vact(dsi);
+ mtk_dsi_ps_control(dsi, true);
mtk_dsi_set_vm_cmd(dsi);
mtk_dsi_config_vdo_timing(dsi);
mtk_dsi_set_interrupt_enable(dsi);
--
2.43.0
Instead of open coding bitshifting for various register fields,
use the bitfield macro FIELD_PREP(): this allows to enhance the
human readability, decrease likeliness of mistakes (and register
field overflowing) and also to simplify the code.
The latter is especially seen in mtk_dsi_rxtx_control(), where
it was possible to change a switch to a short for loop and to
also remove the need to check for maximum DSI lanes == 4 thanks
to the FIELD_PREP macro masking the value.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 95 ++++++++++++++++--------------
1 file changed, 50 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index b618e2e31022..2ba6cd129150 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -70,16 +70,19 @@
#define DSI_PSCTRL 0x1c
#define DSI_PS_WC GENMASK(14, 0)
#define DSI_PS_SEL GENMASK(19, 16)
-#define PACKED_PS_16BIT_RGB565 (0 << 16)
-#define LOOSELY_PS_18BIT_RGB666 (1 << 16)
-#define PACKED_PS_18BIT_RGB666 (2 << 16)
-#define PACKED_PS_24BIT_RGB888 (3 << 16)
+#define PACKED_PS_16BIT_RGB565 0
+#define LOOSELY_PS_18BIT_RGB666 1
+#define PACKED_PS_18BIT_RGB666 2
+#define PACKED_PS_24BIT_RGB888 3
#define DSI_VSA_NL 0x20
#define DSI_VBP_NL 0x24
#define DSI_VFP_NL 0x28
#define DSI_VACT_NL 0x2C
+#define VACT_NL GENMASK(14, 0)
#define DSI_SIZE_CON 0x38
+#define DSI_HEIGHT GENMASK(30, 16)
+#define DSI_WIDTH GENMASK(14, 0)
#define DSI_HSA_WC 0x50
#define DSI_HBP_WC 0x54
#define DSI_HFP_WC 0x58
@@ -254,14 +257,23 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
timing->clk_hs_zero = timing->clk_hs_trail * 4;
timing->clk_hs_exit = 2 * timing->clk_hs_trail;
- timcon0 = timing->lpx | timing->da_hs_prepare << 8 |
- timing->da_hs_zero << 16 | timing->da_hs_trail << 24;
- timcon1 = timing->ta_go | timing->ta_sure << 8 |
- timing->ta_get << 16 | timing->da_hs_exit << 24;
- timcon2 = 1 << 8 | timing->clk_hs_zero << 16 |
- timing->clk_hs_trail << 24;
- timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 |
- timing->clk_hs_exit << 16;
+ timcon0 = FIELD_PREP(LPX, timing->lpx) |
+ FIELD_PREP(HS_PREP, timing->da_hs_prepare) |
+ FIELD_PREP(HS_ZERO, timing->da_hs_zero) |
+ FIELD_PREP(HS_TRAIL, timing->da_hs_trail);
+
+ timcon1 = FIELD_PREP(TA_GO, timing->ta_go) |
+ FIELD_PREP(TA_SURE, timing->ta_sure) |
+ FIELD_PREP(TA_GET, timing->ta_get) |
+ FIELD_PREP(DA_HS_EXIT, timing->da_hs_exit);
+
+ timcon2 = FIELD_PREP(DA_HS_SYNC, 1) |
+ FIELD_PREP(CLK_ZERO, timing->clk_hs_zero) |
+ FIELD_PREP(CLK_TRAIL, timing->clk_hs_trail);
+
+ timcon3 = FIELD_PREP(CLK_HS_PREP, timing->clk_hs_prepare) |
+ FIELD_PREP(CLK_HS_POST, timing->clk_hs_post) |
+ FIELD_PREP(CLK_HS_EXIT, timing->clk_hs_exit);
writel(timcon0, dsi->regs + DSI_PHY_TIMECON0);
writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);
@@ -354,69 +366,61 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
{
- u32 tmp_reg;
+ u32 regval, tmp_reg = 0;
+ u8 i;
- switch (dsi->lanes) {
- case 1:
- tmp_reg = 1 << 2;
- break;
- case 2:
- tmp_reg = 3 << 2;
- break;
- case 3:
- tmp_reg = 7 << 2;
- break;
- case 4:
- tmp_reg = 0xf << 2;
- break;
- default:
- tmp_reg = 0xf << 2;
- break;
- }
+ /* Number of DSI lanes (max 4 lanes), each bit enables one DSI lane. */
+ for (i = 0; i < dsi->lanes; i++)
+ tmp_reg |= BIT(i);
+
+ regval = FIELD_PREP(LANE_NUM, tmp_reg);
if (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
- tmp_reg |= HSTX_CKLP_EN;
+ regval |= HSTX_CKLP_EN;
if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
- tmp_reg |= DIS_EOT;
+ regval |= DIS_EOT;
- writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
+ writel(regval, dsi->regs + DSI_TXRX_CTRL);
}
static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
{
- struct videomode *vm = &dsi->vm;
- u32 dsi_buf_bpp, ps_wc;
- u32 ps_bpp_mode;
+ u32 dsi_buf_bpp, ps_val, ps_wc, vact_nl;
if (dsi->format == MIPI_DSI_FMT_RGB565)
dsi_buf_bpp = 2;
else
dsi_buf_bpp = 3;
- ps_wc = vm->hactive * dsi_buf_bpp;
- ps_bpp_mode = ps_wc;
+ /* Word count */
+ ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp);
+ ps_val = ps_wc;
+ /* Pixel Stream type */
switch (dsi->format) {
+ default:
+ /* fallthrough */
case MIPI_DSI_FMT_RGB888:
- ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_24BIT_RGB888);
break;
case MIPI_DSI_FMT_RGB666:
- ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_18BIT_RGB666);
break;
case MIPI_DSI_FMT_RGB666_PACKED:
- ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, LOOSELY_PS_18BIT_RGB666);
break;
case MIPI_DSI_FMT_RGB565:
- ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_16BIT_RGB565);
break;
}
if (config_vact) {
- writel(vm->vactive, dsi->regs + DSI_VACT_NL);
+ vact_nl = FIELD_PREP(VACT_NL, dsi->vm.vactive);
+ writel(vact_nl, dsi->regs + DSI_VACT_NL);
writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
}
- writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
+ writel(ps_val, dsi->regs + DSI_PSCTRL);
}
static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
@@ -443,7 +447,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
writel(vm->vactive, dsi->regs + DSI_VACT_NL);
if (dsi->driver_data->has_size_ctl)
- writel(vm->vactive << 16 | vm->hactive,
+ writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) |
+ FIELD_PREP(DSI_WIDTH, vm->hactive),
dsi->regs + DSI_SIZE_CON);
horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10);
--
2.43.0
In mtk_dsi_phy_timconfig(), we're dividing the `data_rate` variable,
expressed in Hz to retrieve a value in MHz: instead of open-coding,
use the HZ_PER_MHZ definition, available in linux/units.h.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 2ba6cd129150..b9a37407f3b4 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -12,6 +12,7 @@
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/reset.h>
+#include <linux/units.h>
#include <video/mipi_display.h>
#include <video/videomode.h>
@@ -237,7 +238,7 @@ static void mtk_dsi_mask(struct mtk_dsi *dsi, u32 offset, u32 mask, u32 data)
static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
{
u32 timcon0, timcon1, timcon2, timcon3;
- u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, 1000000);
+ u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, HZ_PER_MHZ);
struct mtk_phy_timing *timing = &dsi->phy_timing;
timing->lpx = (60 * data_rate_mhz / (8 * 1000)) + 1;
--
2.43.0
Hi AngeloGioacchino,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.7-rc6 next-20231222]
[cannot apply to drm-intel/for-linux-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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/AngeloGioacchino-Del-Regno/drm-mediatek-dsi-Use-GENMASK-for-register-mask-definitions/20231222-164513
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20231220135722.192080-4-angelogioacchino.delregno%40collabora.com
patch subject: [PATCH v2 3/4] drm/mediatek: dsi: Use bitfield macros where useful
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20231223/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231223/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/gpu/drm/mediatek/mtk_dsi.c: In function 'mtk_dsi_phy_timconfig':
>> drivers/gpu/drm/mediatek/mtk_dsi.c:260:19: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
260 | timcon0 = FIELD_PREP(LPX, timing->lpx) |
| ^~~~~~~~~~
cc1: some warnings being treated as errors
vim +/FIELD_PREP +260 drivers/gpu/drm/mediatek/mtk_dsi.c
236
237 static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
238 {
239 u32 timcon0, timcon1, timcon2, timcon3;
240 u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, 1000000);
241 struct mtk_phy_timing *timing = &dsi->phy_timing;
242
243 timing->lpx = (60 * data_rate_mhz / (8 * 1000)) + 1;
244 timing->da_hs_prepare = (80 * data_rate_mhz + 4 * 1000) / 8000;
245 timing->da_hs_zero = (170 * data_rate_mhz + 10 * 1000) / 8000 + 1 -
246 timing->da_hs_prepare;
247 timing->da_hs_trail = timing->da_hs_prepare + 1;
248
249 timing->ta_go = 4 * timing->lpx - 2;
250 timing->ta_sure = timing->lpx + 2;
251 timing->ta_get = 4 * timing->lpx;
252 timing->da_hs_exit = 2 * timing->lpx + 1;
253
254 timing->clk_hs_prepare = 70 * data_rate_mhz / (8 * 1000);
255 timing->clk_hs_post = timing->clk_hs_prepare + 8;
256 timing->clk_hs_trail = timing->clk_hs_prepare;
257 timing->clk_hs_zero = timing->clk_hs_trail * 4;
258 timing->clk_hs_exit = 2 * timing->clk_hs_trail;
259
> 260 timcon0 = FIELD_PREP(LPX, timing->lpx) |
261 FIELD_PREP(HS_PREP, timing->da_hs_prepare) |
262 FIELD_PREP(HS_ZERO, timing->da_hs_zero) |
263 FIELD_PREP(HS_TRAIL, timing->da_hs_trail);
264
265 timcon1 = FIELD_PREP(TA_GO, timing->ta_go) |
266 FIELD_PREP(TA_SURE, timing->ta_sure) |
267 FIELD_PREP(TA_GET, timing->ta_get) |
268 FIELD_PREP(DA_HS_EXIT, timing->da_hs_exit);
269
270 timcon2 = FIELD_PREP(DA_HS_SYNC, 1) |
271 FIELD_PREP(CLK_ZERO, timing->clk_hs_zero) |
272 FIELD_PREP(CLK_TRAIL, timing->clk_hs_trail);
273
274 timcon3 = FIELD_PREP(CLK_HS_PREP, timing->clk_hs_prepare) |
275 FIELD_PREP(CLK_HS_POST, timing->clk_hs_post) |
276 FIELD_PREP(CLK_HS_EXIT, timing->clk_hs_exit);
277
278 writel(timcon0, dsi->regs + DSI_PHY_TIMECON0);
279 writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);
280 writel(timcon2, dsi->regs + DSI_PHY_TIMECON2);
281 writel(timcon3, dsi->regs + DSI_PHY_TIMECON3);
282 }
283
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Angelo,
On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Change magic numerical masks with usage of the GENMASK() macro
> to improve readability.
>
> This commit brings no functional changes.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 46 ++++++++++++++++--------------
> 1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index a2fdfc8ddb15..23d2c5be8dbb 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -58,18 +58,18 @@
>
> #define DSI_TXRX_CTRL 0x18
> #define VC_NUM BIT(1)
> -#define LANE_NUM (0xf << 2)
> +#define LANE_NUM GENMASK(5, 2)
> #define DIS_EOT BIT(6)
> #define NULL_EN BIT(7)
> #define TE_FREERUN BIT(8)
> #define EXT_TE_EN BIT(9)
> #define EXT_TE_EDGE BIT(10)
> -#define MAX_RTN_SIZE (0xf << 12)
> +#define MAX_RTN_SIZE GENMASK(15, 12)
> #define HSTX_CKLP_EN BIT(16)
>
> #define DSI_PSCTRL 0x1c
> -#define DSI_PS_WC 0x3fff
> -#define DSI_PS_SEL (3 << 16)
> +#define DSI_PS_WC GENMASK(14, 0)
> +#define DSI_PS_SEL GENMASK(19, 16)
GENMASK(17, 16)
>
> #define PACKED_PS_16BIT_RGB565 (0 << 16)
> #define LOOSELY_PS_18BIT_RGB666 (1 << 16)
> #define PACKED_PS_18BIT_RGB666 (2 << 16)
> @@ -109,26 +109,27 @@
> #define LD0_WAKEUP_EN BIT(2)
>
> #define DSI_PHY_TIMECON0 0x110
> -#define LPX (0xff << 0)
> -#define HS_PREP (0xff << 8)
> -#define HS_ZERO (0xff << 16)
> -#define HS_TRAIL (0xff << 24)
> +#define LPX GENMASK(7, 0)
> +#define HS_PREP GENMASK(15, 8)
> +#define HS_ZERO GENMASK(23, 16)
> +#define HS_TRAIL GENMASK(31, 24)
>
> #define DSI_PHY_TIMECON1 0x114
> -#define TA_GO (0xff << 0)
> -#define TA_SURE (0xff << 8)
> -#define TA_GET (0xff << 16)
> -#define DA_HS_EXIT (0xff << 24)
> +#define TA_GO GENMASK(7, 0)
> +#define TA_SURE GENMASK(15, 8)
> +#define TA_GET GENMASK(23, 16)
> +#define DA_HS_EXIT GENMASK(31, 24)
>
> #define DSI_PHY_TIMECON2 0x118
> -#define CONT_DET (0xff << 0)
> -#define CLK_ZERO (0xff << 16)
> -#define CLK_TRAIL (0xff << 24)
> +#define CONT_DET GENMASK(7, 0)
> +#define DA_HS_SYNC GENMASK(15, 8)
This is new, so please introduce it in a separate patch if intended.
The rest looks good to me.
Regards,
Fei
>
> +#define CLK_ZERO GENMASK(23, 16)
> +#define CLK_TRAIL GENMASK(31, 24)
>
> #define DSI_PHY_TIMECON3 0x11c
> -#define CLK_HS_PREP (0xff << 0)
> -#define CLK_HS_POST (0xff << 8)
> -#define CLK_HS_EXIT (0xff << 16)
> +#define CLK_HS_PREP GENMASK(7, 0)
> +#define CLK_HS_POST GENMASK(15, 8)
> +#define CLK_HS_EXIT GENMASK(23, 16)
>
> #define DSI_VM_CMD_CON 0x130
> #define VM_CMD_EN BIT(0)
> @@ -138,13 +139,14 @@
> #define FORCE_COMMIT BIT(0)
> #define BYPASS_SHADOW BIT(1)
>
> -#define CONFIG (0xff << 0)
> +/* CMDQ related bits */
> +#define CONFIG GENMASK(7, 0)
> #define SHORT_PACKET 0
> #define LONG_PACKET 2
> #define BTA BIT(2)
> -#define DATA_ID (0xff << 8)
> -#define DATA_0 (0xff << 16)
> -#define DATA_1 (0xff << 24)
> +#define DATA_ID GENMASK(15, 8)
> +#define DATA_0 GENMASK(23, 16)
> +#define DATA_1 GENMASK(31, 24)
>
> #define NS_TO_CYCLE(n, c) ((n) / (c) + (((n) % (c)) ? 1 : 0))
>
> --
> 2.43.0
>
>
Hi Angelo,
On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
> merge the two in one mtk_dsi_ps_control() function by adding one
> function parameter `config_vact` which, when true, writes the VACT
> related registers.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +++++++++---------------------
> 1 file changed, 23 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 23d2c5be8dbb..b618e2e31022 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -352,40 +352,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
> mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
> }
>
> -static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
> -{
> - struct videomode *vm = &dsi->vm;
> - u32 dsi_buf_bpp, ps_wc;
> - u32 ps_bpp_mode;
> -
> - if (dsi->format == MIPI_DSI_FMT_RGB565)
> - dsi_buf_bpp = 2;
> - else
> - dsi_buf_bpp = 3;
> -
> - ps_wc = vm->hactive * dsi_buf_bpp;
> - ps_bpp_mode = ps_wc;
> -
> - switch (dsi->format) {
> - case MIPI_DSI_FMT_RGB888:
> - ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
> - break;
> - case MIPI_DSI_FMT_RGB666:
> - ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
> - break;
> - case MIPI_DSI_FMT_RGB666_PACKED:
> - ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
> - break;
> - case MIPI_DSI_FMT_RGB565:
> - ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
> - break;
> - }
> -
> - writel(vm->vactive, dsi->regs + DSI_VACT_NL);
> - writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
> - writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
> -}
> -
> static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
> {
> u32 tmp_reg;
> @@ -417,36 +383,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
> writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
> }
>
> -static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
> +static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
> {
> - u32 dsi_tmp_buf_bpp;
> - u32 tmp_reg;
> + struct videomode *vm = &dsi->vm;
> + u32 dsi_buf_bpp, ps_wc;
> + u32 ps_bpp_mode;
> +
> + if (dsi->format == MIPI_DSI_FMT_RGB565)
> + dsi_buf_bpp = 2;
> + else
> + dsi_buf_bpp = 3;
The same is also in mtk_dsi_config_vdo_timing(). Given this is a
cleanup series, I think it'd be a good chance to add another patch
and integrate those usages. Just a thought. :)
>
> +
> + ps_wc = vm->hactive * dsi_buf_bpp;
I noticed the "& DSI_PS_WC" part was dropped (but perhaps with awareness?).
While the outcome seems to always fall within the range of
DSI_PS_WC so we should be fine in practice, I think it doesn't hurt to
keep the value masked to save readers some time to check if this would
ever be possible to overflow and write undesired bits down the line.
WDYT?
Regardless of that, I didn't find obvious issue in this patch, so
Reviewed-by: Fei Shao <[email protected]>
Regards,
Fei
>
> + ps_bpp_mode = ps_wc;
>
> switch (dsi->format) {
> case MIPI_DSI_FMT_RGB888:
> - tmp_reg = PACKED_PS_24BIT_RGB888;
> - dsi_tmp_buf_bpp = 3;
> + ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
> break;
> case MIPI_DSI_FMT_RGB666:
> - tmp_reg = LOOSELY_PS_18BIT_RGB666;
> - dsi_tmp_buf_bpp = 3;
> + ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
> break;
> case MIPI_DSI_FMT_RGB666_PACKED:
> - tmp_reg = PACKED_PS_18BIT_RGB666;
> - dsi_tmp_buf_bpp = 3;
> + ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
> break;
> case MIPI_DSI_FMT_RGB565:
> - tmp_reg = PACKED_PS_16BIT_RGB565;
> - dsi_tmp_buf_bpp = 2;
> - break;
> - default:
>
> - tmp_reg = PACKED_PS_24BIT_RGB888;
> - dsi_tmp_buf_bpp = 3;
> + ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
> break;
> }
>
> - tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC;
>
> - writel(tmp_reg, dsi->regs + DSI_PSCTRL);
> + if (config_vact) {
> + writel(vm->vactive, dsi->regs + DSI_VACT_NL);
> + writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
> + }
> + writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
> }
>
> static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> @@ -522,7 +492,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
> writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
>
> - mtk_dsi_ps_control(dsi);
> + mtk_dsi_ps_control(dsi, false);
> }
>
> static void mtk_dsi_start(struct mtk_dsi *dsi)
> @@ -667,7 +637,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> mtk_dsi_reset_engine(dsi);
> mtk_dsi_phy_timconfig(dsi);
>
> - mtk_dsi_ps_control_vact(dsi);
> + mtk_dsi_ps_control(dsi, true);
> mtk_dsi_set_vm_cmd(dsi);
> mtk_dsi_config_vdo_timing(dsi);
> mtk_dsi_set_interrupt_enable(dsi);
> --
> 2.43.0
>
>
Il 26/12/23 11:48, Fei Shao ha scritto:
> Hi Angelo,
>
> On Wed, Dec 20, 2023 at 9:57 PM AngeloGioacchino Del Regno
> <[email protected]> wrote:
>>
>> Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
>> merge the two in one mtk_dsi_ps_control() function by adding one
>> function parameter `config_vact` which, when true, writes the VACT
>> related registers.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
>> ---
>> drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +++++++++---------------------
>> 1 file changed, 23 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index 23d2c5be8dbb..b618e2e31022 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -352,40 +352,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
>> mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
>> }
>>
>> -static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
>> -{
>> - struct videomode *vm = &dsi->vm;
>> - u32 dsi_buf_bpp, ps_wc;
>> - u32 ps_bpp_mode;
>> -
>> - if (dsi->format == MIPI_DSI_FMT_RGB565)
>> - dsi_buf_bpp = 2;
>> - else
>> - dsi_buf_bpp = 3;
>> -
>> - ps_wc = vm->hactive * dsi_buf_bpp;
>> - ps_bpp_mode = ps_wc;
>> -
>> - switch (dsi->format) {
>> - case MIPI_DSI_FMT_RGB888:
>> - ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
>> - break;
>> - case MIPI_DSI_FMT_RGB666:
>> - ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
>> - break;
>> - case MIPI_DSI_FMT_RGB666_PACKED:
>> - ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
>> - break;
>> - case MIPI_DSI_FMT_RGB565:
>> - ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
>> - break;
>> - }
>> -
>> - writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>> - writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
>> - writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
>> -}
>> -
>> static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
>> {
>> u32 tmp_reg;
>> @@ -417,36 +383,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
>> writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
>> }
>>
>> -static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
>> +static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
>> {
>> - u32 dsi_tmp_buf_bpp;
>> - u32 tmp_reg;
>> + struct videomode *vm = &dsi->vm;
>> + u32 dsi_buf_bpp, ps_wc;
>> + u32 ps_bpp_mode;
>> +
>> + if (dsi->format == MIPI_DSI_FMT_RGB565)
>> + dsi_buf_bpp = 2;
>> + else
>> + dsi_buf_bpp = 3;
>
> The same is also in mtk_dsi_config_vdo_timing(). Given this is a
> cleanup series, I think it'd be a good chance to add another patch
> and integrate those usages. Just a thought. :)
Checking that right now.
>>
>> +
>> + ps_wc = vm->hactive * dsi_buf_bpp;
>
> I noticed the "& DSI_PS_WC" part was dropped (but perhaps with awareness?).
>
> While the outcome seems to always fall within the range of
> DSI_PS_WC so we should be fine in practice, I think it doesn't hurt to
> keep the value masked to save readers some time to check if this would
> ever be possible to overflow and write undesired bits down the line.
> WDYT?
>
Masking a pre-masked value doesn't look right.
Besides, as for the concern of overflowing, if we masked that we'd still end up
with broken functionality, as if the value is invalid... well, it's invalid.
Masked or not. :-)
Thanks for the R-b, sending a v3 soon with some fixes.
Regards,
Angelo
> Regardless of that, I didn't find obvious issue in this patch, so
>
> Reviewed-by: Fei Shao <[email protected]>
>
> Regards,
> Fei
>
>
>
>
>>
>> + ps_bpp_mode = ps_wc;
>>
>> switch (dsi->format) {
>> case MIPI_DSI_FMT_RGB888:
>> - tmp_reg = PACKED_PS_24BIT_RGB888;
>> - dsi_tmp_buf_bpp = 3;
>> + ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
>> break;
>> case MIPI_DSI_FMT_RGB666:
>> - tmp_reg = LOOSELY_PS_18BIT_RGB666;
>> - dsi_tmp_buf_bpp = 3;
>> + ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
>> break;
>> case MIPI_DSI_FMT_RGB666_PACKED:
>> - tmp_reg = PACKED_PS_18BIT_RGB666;
>> - dsi_tmp_buf_bpp = 3;
>> + ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
>> break;
>> case MIPI_DSI_FMT_RGB565:
>> - tmp_reg = PACKED_PS_16BIT_RGB565;
>> - dsi_tmp_buf_bpp = 2;
>> - break;
>> - default:
>>
>> - tmp_reg = PACKED_PS_24BIT_RGB888;
>> - dsi_tmp_buf_bpp = 3;
>> + ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
>> break;
>> }
>>
>> - tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC;
>>
>> - writel(tmp_reg, dsi->regs + DSI_PSCTRL);
>> + if (config_vact) {
>> + writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>> + writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
>> + }
>> + writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
>> }
>>
>> static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>> @@ -522,7 +492,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>> writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
>> writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
>>
>> - mtk_dsi_ps_control(dsi);
>> + mtk_dsi_ps_control(dsi, false);
>> }
>>
>> static void mtk_dsi_start(struct mtk_dsi *dsi)
>> @@ -667,7 +637,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>> mtk_dsi_reset_engine(dsi);
>> mtk_dsi_phy_timconfig(dsi);
>>
>> - mtk_dsi_ps_control_vact(dsi);
>> + mtk_dsi_ps_control(dsi, true);
>> mtk_dsi_set_vm_cmd(dsi);
>> mtk_dsi_config_vdo_timing(dsi);
>> mtk_dsi_set_interrupt_enable(dsi);
>> --
>> 2.43.0
>>
>>