Changes in v3:
- Rebased over next-20240131
- Added bitfield.h inclusion in patch 3
- Added three more cleanup commits to the mix to simplify
the probe function and remove gotos.
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 (7):
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
drm/mediatek: dsi: Register DSI host after acquiring clocks and PHY
drm/mediatek: dsi: Simplify with dev_err_probe and remove gotos
drm/mediatek: dsi: Compress of_device_id entries and add sentinel
drivers/gpu/drm/mediatek/mtk_dsi.c | 284 ++++++++++++-----------------
1 file changed, 117 insertions(+), 167 deletions(-)
--
2.43.0
Change magic numerical masks with usage of the GENMASK() macro
to improve readability.
While at it, also fix the DSI_PS_SEL mask to include all bits instead
of just a subset of them.
This commit brings no functional changes.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++---------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index a2fdfc8ddb15..3b7392c03b4d 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,26 @@
#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 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 +138,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
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 cba63de5092d..cc625febe6c8 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -13,6 +13,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>
@@ -238,7 +239,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
All entries fit in 82 columns, which is acceptable: compress all of
the mtk_dsi_of_match[] entries to a single line for each.
While at it, also add the usual sentinel comment to the last entry.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 5a0f078987d3..2c482742183e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1204,17 +1204,12 @@ static const struct mtk_dsi_driver_data mt8188_dsi_driver_data = {
};
static const struct of_device_id mtk_dsi_of_match[] = {
- { .compatible = "mediatek,mt2701-dsi",
- .data = &mt2701_dsi_driver_data },
- { .compatible = "mediatek,mt8173-dsi",
- .data = &mt8173_dsi_driver_data },
- { .compatible = "mediatek,mt8183-dsi",
- .data = &mt8183_dsi_driver_data },
- { .compatible = "mediatek,mt8186-dsi",
- .data = &mt8186_dsi_driver_data },
- { .compatible = "mediatek,mt8188-dsi",
- .data = &mt8188_dsi_driver_data },
- { },
+ { .compatible = "mediatek,mt2701-dsi", .data = &mt2701_dsi_driver_data },
+ { .compatible = "mediatek,mt8173-dsi", .data = &mt8173_dsi_driver_data },
+ { .compatible = "mediatek,mt8183-dsi", .data = &mt8183_dsi_driver_data },
+ { .compatible = "mediatek,mt8186-dsi", .data = &mt8186_dsi_driver_data },
+ { .compatible = "mediatek,mt8188-dsi", .data = &mt8188_dsi_driver_data },
+ { /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mtk_dsi_of_match);
--
2.43.0
Registering the dsi host with its ops before getting dsi->regs is
simply wrong: even though there's nothing (for now) asynchronously
calling those ops before the end of the probe function, installing
ops that are using iospace(s) and clocks before even initializing
those is too fragile.
Register the DSI host after getting clocks, iospace and PHY.
This wil also allow to simplify the error paths in a later commit.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index cc625febe6c8..709a65656b79 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1114,14 +1114,6 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (!dsi)
return -ENOMEM;
- dsi->host.ops = &mtk_dsi_ops;
- dsi->host.dev = dev;
- ret = mipi_dsi_host_register(&dsi->host);
- if (ret < 0) {
- dev_err(dev, "failed to register DSI host: %d\n", ret);
- return ret;
- }
-
dsi->driver_data = of_device_get_match_data(dev);
dsi->engine_clk = devm_clk_get(dev, "engine");
@@ -1130,7 +1122,7 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (ret != -EPROBE_DEFER)
dev_err(dev, "Failed to get engine clock: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
dsi->digital_clk = devm_clk_get(dev, "digital");
@@ -1139,14 +1131,14 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (ret != -EPROBE_DEFER)
dev_err(dev, "Failed to get digital clock: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
dsi->hs_clk = devm_clk_get(dev, "hs");
if (IS_ERR(dsi->hs_clk)) {
ret = PTR_ERR(dsi->hs_clk);
dev_err(dev, "Failed to get hs clock: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1154,20 +1146,28 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (IS_ERR(dsi->regs)) {
ret = PTR_ERR(dsi->regs);
dev_err(dev, "Failed to ioremap memory: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
dsi->phy = devm_phy_get(dev, "dphy");
if (IS_ERR(dsi->phy)) {
ret = PTR_ERR(dsi->phy);
dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
irq_num = platform_get_irq(pdev, 0);
if (irq_num < 0) {
ret = irq_num;
- goto err_unregister_host;
+ return ret;
+ }
+
+ dsi->host.ops = &mtk_dsi_ops;
+ dsi->host.dev = dev;
+ ret = mipi_dsi_host_register(&dsi->host);
+ if (ret < 0) {
+ dev_err(dev, "failed to register DSI host: %d\n", ret);
+ return ret;
}
ret = devm_request_irq(&pdev->dev, irq_num, mtk_dsi_irq,
--
2.43.0
Most of the functions that are called in the probe callback are
devm managed, or all but mipi_dsi_host_register(): simplify the probe
function's error paths with dev_err_probe() and remove the lonely
instance of `goto err_unregister_host` by just directly calling the
mipi_dsi_host_unregister() function in the devm_request_irq() error
path, allowing to also remove the same label.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 60 +++++++++---------------------
1 file changed, 18 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 709a65656b79..5a0f078987d3 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1117,64 +1117,44 @@ static int mtk_dsi_probe(struct platform_device *pdev)
dsi->driver_data = of_device_get_match_data(dev);
dsi->engine_clk = devm_clk_get(dev, "engine");
- if (IS_ERR(dsi->engine_clk)) {
- ret = PTR_ERR(dsi->engine_clk);
+ if (IS_ERR(dsi->engine_clk))
+ return dev_err_probe(dev, PTR_ERR(dsi->engine_clk),
+ "Failed to get engine clock\n");
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "Failed to get engine clock: %d\n", ret);
- return ret;
- }
dsi->digital_clk = devm_clk_get(dev, "digital");
- if (IS_ERR(dsi->digital_clk)) {
- ret = PTR_ERR(dsi->digital_clk);
-
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "Failed to get digital clock: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->digital_clk))
+ return dev_err_probe(dev, PTR_ERR(dsi->digital_clk),
+ "Failed to get digital clock\n");
dsi->hs_clk = devm_clk_get(dev, "hs");
- if (IS_ERR(dsi->hs_clk)) {
- ret = PTR_ERR(dsi->hs_clk);
- dev_err(dev, "Failed to get hs clock: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->hs_clk))
+ return dev_err_probe(dev, PTR_ERR(dsi->hs_clk), "Failed to get hs clock\n");
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dsi->regs = devm_ioremap_resource(dev, regs);
- if (IS_ERR(dsi->regs)) {
- ret = PTR_ERR(dsi->regs);
- dev_err(dev, "Failed to ioremap memory: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->regs))
+ return dev_err_probe(dev, PTR_ERR(dsi->regs), "Failed to ioremap memory\n");
dsi->phy = devm_phy_get(dev, "dphy");
- if (IS_ERR(dsi->phy)) {
- ret = PTR_ERR(dsi->phy);
- dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->phy))
+ return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get MIPI-DPHY\n");
irq_num = platform_get_irq(pdev, 0);
- if (irq_num < 0) {
- ret = irq_num;
- return ret;
- }
+ if (irq_num < 0)
+ return irq_num;
dsi->host.ops = &mtk_dsi_ops;
dsi->host.dev = dev;
ret = mipi_dsi_host_register(&dsi->host);
- if (ret < 0) {
- dev_err(dev, "failed to register DSI host: %d\n", ret);
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to register DSI host\n");
ret = devm_request_irq(&pdev->dev, irq_num, mtk_dsi_irq,
IRQF_TRIGGER_NONE, dev_name(&pdev->dev), dsi);
if (ret) {
- dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
- goto err_unregister_host;
+ mipi_dsi_host_unregister(&dsi->host);
+ return dev_err_probe(&pdev->dev, ret, "Failed to request DSI irq\n");
}
init_waitqueue_head(&dsi->irq_wait_queue);
@@ -1186,10 +1166,6 @@ static int mtk_dsi_probe(struct platform_device *pdev)
dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
return 0;
-
-err_unregister_host:
- mipi_dsi_host_unregister(&dsi->host);
- return ret;
}
static void mtk_dsi_remove(struct platform_device *pdev)
--
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.
While at it, also add the missing DA_HS_SYNC bitmask, used in
mtk_dsi_phy_timconfig().
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 97 ++++++++++++++++--------------
1 file changed, 52 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 8414ce73ce9f..cba63de5092d 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -3,6 +3,7 @@
* Copyright (c) 2015 MediaTek Inc.
*/
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/component.h>
#include <linux/iopoll.h>
@@ -70,16 +71,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
@@ -122,6 +126,7 @@
#define DSI_PHY_TIMECON2 0x118
#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)
@@ -253,14 +258,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);
@@ -353,69 +367,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)
@@ -442,7 +448,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
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.
Reviewed-by: Fei Shao <[email protected]>
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 3b7392c03b4d..8414ce73ce9f 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -351,40 +351,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;
@@ -416,36 +382,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)
@@ -521,7 +491,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)
@@ -666,7 +636,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
Hi, Angelo:
On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno wrote:
> Change magic numerical masks with usage of the GENMASK() macro
> to improve readability.
>
> While at it, also fix the DSI_PS_SEL mask to include all bits instead
> of just a subset of them.
>
> This commit brings no functional changes.
>
> Signed-off-by: AngeloGioacchino Del Regno <
> [email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++-------------
> --
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index a2fdfc8ddb15..3b7392c03b4d 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)
The original definition of DSI_PS_WC/DSI_PS_SEL is correct in MT8173.
So both need two definition and let each SoC select its own definition.
Regards,
CK
> #define PACKED_PS_16BIT_RGB565 (0 << 16)
> #define LOOSELY_PS_18BIT_RGB666 (1 << 16)
> #define PACKED_PS_18BIT_RGB666 (2 << 16)
> @@ -109,26 +109,26 @@
> #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 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 +138,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))
>
Hi, Angelo:
On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno 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.
>
> Reviewed-by: Fei Shao <[email protected]>
> 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 3b7392c03b4d..8414ce73ce9f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -351,40 +351,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;
> @@ -416,36 +382,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;
You change the original logic here. If it is a fixup, separate to a
independent patch not hiding in a clean up patch. So we could backport
the fixup patch.
Regards,
CK
> 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)
> @@ -521,7 +491,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)
> @@ -666,7 +636,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);
Il 06/02/24 10:50, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
>
> On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno 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.
>>
>> Reviewed-by: Fei Shao <[email protected]>
>> 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 3b7392c03b4d..8414ce73ce9f 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -351,40 +351,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;
>> @@ -416,36 +382,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;
>
> You change the original logic here. If it is a fixup, separate to a
> independent patch not hiding in a clean up patch. So we could backport
> the fixup patch.
>
Thank you CK! Thanks to you noticing that, I've discovered that actually
besides the two functions not agreeing on the bit to set, the definitions
of those bits are actually wrong (as you can verify on datasheets for
multiple SoCs, including MT8195, MT8188, MT8186, MT8183).
v4 will fix that by adding a fixup commit to rectify the whole thing.
Cheers,
Angelo
> Regards,
> CK
>
>> 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)
>> @@ -521,7 +491,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)
>> @@ -666,7 +636,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);
Il 06/02/24 09:57, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
>
> On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno wrote:
>> Change magic numerical masks with usage of the GENMASK() macro
>> to improve readability.
>>
>> While at it, also fix the DSI_PS_SEL mask to include all bits instead
>> of just a subset of them.
>>
>> This commit brings no functional changes.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <
>> [email protected]>
>> ---
>> drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++-------------
>> --
>> 1 file changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index a2fdfc8ddb15..3b7392c03b4d 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)
>
> The original definition of DSI_PS_WC/DSI_PS_SEL is correct in MT8173.
> So both need two definition and let each SoC select its own definition.
>
The additional bits are unused on older SoCs and, if set, will be simply ignored;
if we want to prevent setting bits that don't exist on the old ones, that should
be done as a later commit introducing SoC capabilities for those and when the new
capabilities for the new SoCs are introduced anyway.
As of now, this doesn't break anything.
Regards,
Angelo
Hi, Angelo:
On Tue, 2024-02-06 at 14:27 +0100, AngeloGioacchino Del Regno wrote:
> Il 06/02/24 09:57, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> >
> > On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > Change magic numerical masks with usage of the GENMASK() macro
> > > to improve readability.
> > >
> > > While at it, also fix the DSI_PS_SEL mask to include all bits
> > > instead
> > > of just a subset of them.
> > >
> > > This commit brings no functional changes.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno <
> > > [email protected]>
> > > ---
> > > drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++------
> > > -------
> > > --
> > > 1 file changed, 23 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index a2fdfc8ddb15..3b7392c03b4d 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)
> >
> > The original definition of DSI_PS_WC/DSI_PS_SEL is correct in
> > MT8173.
> > So both need two definition and let each SoC select its own
> > definition.
> >
>
> The additional bits are unused on older SoCs and, if set, will be
> simply ignored;
> if we want to prevent setting bits that don't exist on the old ones,
> that should
> be done as a later commit introducing SoC capabilities for those and
> when the new
> capabilities for the new SoCs are introduced anyway.
>
> As of now, this doesn't break anything.
The title of this patch is only to use GENMASK(), but here does more
things. I agree this does not break anything, but I would like to
separate this to an independent patch just for new bits. In your later
patch, DSI_PS_WC is not used any more. So maybe after that patch, you
could define as:
#define DSI_PS_WC_MT8173 GENMASK(13, 0)
#define DSI_PS_WC_MT8xxx GENMASK(14, 0)
DSI_PS_SEL is not used now, so it could also define as:
#define DSI_PS_SEL_MT8137 GENMASK(17, 16)
#define DSI_PS_SEL_MT8xxx GENMASK(19, 16)
And add definition of value 4 ~ 15.
Regards,
CK
>
> Regards,
> Angelo
>
>