* [PATCH 2/2] drm/verisilicon: add support for Nuvoton MA35D1 DCUltra Lite display controller
From: Joey Lu @ 2026-05-11 7:51 UTC (permalink / raw)
To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, robh, krzk+dt, conor+dt
Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel,
linux-kernel, Joey Lu
In-Reply-To: <20260511075142.54752-1-a0987203069@gmail.com>
The Nuvoton MA35D1 SoC integrates a Verisilicon DCUltra Lite display
controller, which is a previous generation of the DC8000 series. While
the general register layout is similar to the DC8000, there are several
key differences that require per-variant handling in the driver.
Add a vs_dc_info platform data structure (in vs_hwdb.h) to describe
per-IP-variant capabilities, and use it throughout the driver to select
the correct code paths at runtime.
Key differences between DC8000 and DCUltra Lite handled:
1. No chip identity registers (0x0020-0x0030): DCUltra Lite uses static
platform data instead of reading model/revision/customer_id from HW.
2. No CONFIG_EX commit mechanism: DC8000 uses registers at 0x1CC0
(FB_CONFIG_EX), 0x24D8 (FB_TOP_LEFT), 0x24E0 (FB_BOTTOM_RIGHT),
0x2510 (FB_BLEND_CONFIG), 0x2518 (PANEL_CONFIG_EX). DCUltra Lite
omits all of these and instead uses enable/reset bits in FB_CONFIG
(bit 0 = enable, bit 4 = reset) for direct framebuffer updates.
3. No PANEL_START register (0x1CCC): DCUltra Lite panel output starts
when PANEL_CONFIG.RUNNING is set; no separate multi-display sync
start register is needed.
4. Different IRQ registers: DCUltra Lite uses 0x147C (IRQ_STA) /
0x1480 (IRQ_EN); DC8000 uses 0x0010 (IRQ_ACK) / 0x0014 (IRQ_EN).
5. Different clock/reset topology: DCUltra Lite requires only "core"
(bus gate) and "pix0" (pixel divider) clocks with no reset lines
managed by the driver. DC8000 needs core/axi/ahb clocks and three
resets.
6. Single output only: DCUltra Lite has one display output; per-output
index logic is still in place but display_count is fixed at 1.
7. Reduced register space: max_register is 0x2000 vs DC8000's 0x2544.
Add the "nuvoton,ma35d1-dcu" compatible string to the OF match table,
extend Kconfig to allow building on ARCH_MA35 platforms, and expose
vs_formats_no_yuv444 as the default format table for DCUltra Lite
(YUV444 blending is a DC8000-only feature).
All changes have been tested on Nuvoton MA35D1 hardware and are
functioning correctly.
Signed-off-by: Joey Lu <a0987203069@gmail.com>
---
drivers/gpu/drm/verisilicon/Kconfig | 2 +-
drivers/gpu/drm/verisilicon/vs_bridge.c | 28 ++--
drivers/gpu/drm/verisilicon/vs_crtc.c | 13 +-
drivers/gpu/drm/verisilicon/vs_dc.c | 129 ++++++++++++------
drivers/gpu/drm/verisilicon/vs_dc.h | 1 +
drivers/gpu/drm/verisilicon/vs_drm.c | 16 ++-
drivers/gpu/drm/verisilicon/vs_hwdb.c | 2 +-
drivers/gpu/drm/verisilicon/vs_hwdb.h | 25 ++++
.../gpu/drm/verisilicon/vs_primary_plane.c | 43 +++---
.../drm/verisilicon/vs_primary_plane_regs.h | 2 +
10 files changed, 187 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/verisilicon/Kconfig b/drivers/gpu/drm/verisilicon/Kconfig
index 7cce86ec8603..295d246eb4b4 100644
--- a/drivers/gpu/drm/verisilicon/Kconfig
+++ b/drivers/gpu/drm/verisilicon/Kconfig
@@ -2,7 +2,7 @@
config DRM_VERISILICON_DC
tristate "DRM Support for Verisilicon DC-series display controllers"
depends on DRM && COMMON_CLK
- depends on RISCV || COMPILE_TEST
+ depends on RISCV || ARCH_MA35 || COMPILE_TEST
select DRM_BRIDGE_CONNECTOR
select DRM_CLIENT_SELECTION
select DRM_DISPLAY_HELPER
diff --git a/drivers/gpu/drm/verisilicon/vs_bridge.c b/drivers/gpu/drm/verisilicon/vs_bridge.c
index 7a93049368db..225af322de32 100644
--- a/drivers/gpu/drm/verisilicon/vs_bridge.c
+++ b/drivers/gpu/drm/verisilicon/vs_bridge.c
@@ -164,13 +164,16 @@ static void vs_bridge_enable_common(struct vs_crtc *crtc,
VSDC_DISP_PANEL_CONFIG_CLK_EN);
regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
VSDC_DISP_PANEL_CONFIG_RUNNING);
- regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
- VSDC_DISP_PANEL_START_MULTI_DISP_SYNC);
- regmap_set_bits(dc->regs, VSDC_DISP_PANEL_START,
- VSDC_DISP_PANEL_START_RUNNING(output));
- regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc->id),
- VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
+ if (dc->info->has_config_ex) {
+ regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
+ VSDC_DISP_PANEL_START_MULTI_DISP_SYNC);
+ regmap_set_bits(dc->regs, VSDC_DISP_PANEL_START,
+ VSDC_DISP_PANEL_START_RUNNING(output));
+
+ regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc->id),
+ VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
+ }
}
static void vs_bridge_atomic_enable_dpi(struct drm_bridge *bridge,
@@ -228,14 +231,17 @@ static void vs_bridge_atomic_disable(struct drm_bridge *bridge,
struct vs_dc *dc = crtc->dc;
unsigned int output = crtc->id;
- regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
- VSDC_DISP_PANEL_START_MULTI_DISP_SYNC |
- VSDC_DISP_PANEL_START_RUNNING(output));
regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
VSDC_DISP_PANEL_CONFIG_RUNNING);
- regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc->id),
- VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
+ if (dc->info->has_config_ex) {
+ regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
+ VSDC_DISP_PANEL_START_MULTI_DISP_SYNC |
+ VSDC_DISP_PANEL_START_RUNNING(output));
+
+ regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc->id),
+ VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
+ }
}
static const struct drm_bridge_funcs vs_dpi_bridge_funcs = {
diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c
index 9080344398ca..2f3e6d41c657 100644
--- a/drivers/gpu/drm/verisilicon/vs_crtc.c
+++ b/drivers/gpu/drm/verisilicon/vs_crtc.c
@@ -18,6 +18,7 @@
#include "vs_dc.h"
#include "vs_dc_top_regs.h"
#include "vs_drm.h"
+#include "vs_hwdb.h"
#include "vs_plane.h"
static void vs_crtc_atomic_disable(struct drm_crtc *crtc,
@@ -132,7 +133,11 @@ static int vs_crtc_enable_vblank(struct drm_crtc *crtc)
struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
struct vs_dc *dc = vcrtc->dc;
- regmap_set_bits(dc->regs, VSDC_TOP_IRQ_EN, VSDC_TOP_IRQ_VSYNC(vcrtc->id));
+ if (dc->info->family == VS_DC_FAMILY_DCULTRA_LITE)
+ regmap_write(dc->regs, VSDC_DISP_IRQ_EN, BIT(0));
+ else
+ regmap_set_bits(dc->regs, VSDC_TOP_IRQ_EN,
+ VSDC_TOP_IRQ_VSYNC(vcrtc->id));
return 0;
}
@@ -142,7 +147,11 @@ static void vs_crtc_disable_vblank(struct drm_crtc *crtc)
struct vs_crtc *vcrtc = drm_crtc_to_vs_crtc(crtc);
struct vs_dc *dc = vcrtc->dc;
- regmap_clear_bits(dc->regs, VSDC_TOP_IRQ_EN, VSDC_TOP_IRQ_VSYNC(vcrtc->id));
+ if (dc->info->family == VS_DC_FAMILY_DCULTRA_LITE)
+ regmap_write(dc->regs, VSDC_DISP_IRQ_EN, 0);
+ else
+ regmap_clear_bits(dc->regs, VSDC_TOP_IRQ_EN,
+ VSDC_TOP_IRQ_VSYNC(vcrtc->id));
}
static const struct drm_crtc_funcs vs_crtc_funcs = {
diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c
index dad9967bc10b..82a6a26f6d81 100644
--- a/drivers/gpu/drm/verisilicon/vs_dc.c
+++ b/drivers/gpu/drm/verisilicon/vs_dc.c
@@ -9,21 +9,45 @@
#include <linux/of_graph.h>
#include "vs_crtc.h"
+#include "vs_crtc_regs.h"
#include "vs_dc.h"
#include "vs_dc_top_regs.h"
#include "vs_drm.h"
#include "vs_hwdb.h"
-static const struct regmap_config vs_dc_regmap_cfg = {
+static const struct regmap_config vs_dc8000_regmap_cfg = {
.reg_bits = 32,
.val_bits = 32,
.reg_stride = sizeof(u32),
- /* VSDC_OVL_CONFIG_EX(1) */
.max_register = 0x2544,
};
+static const struct regmap_config vs_dcultra_lite_regmap_cfg = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = sizeof(u32),
+ .max_register = 0x2000,
+};
+
+static const struct vs_dc_info vs_dc8000_info = {
+ .family = VS_DC_FAMILY_DC8000,
+ .has_chip_id = true,
+ .has_config_ex = true,
+ .regmap_cfg = &vs_dc8000_regmap_cfg,
+};
+
+static const struct vs_dc_info vs_dcultra_lite_info = {
+ .family = VS_DC_FAMILY_DCULTRA_LITE,
+ .display_count = 1,
+ .has_chip_id = false,
+ .has_config_ex = false,
+ .regmap_cfg = &vs_dcultra_lite_regmap_cfg,
+ .formats = &vs_formats_no_yuv444,
+};
+
static const struct of_device_id vs_dc_driver_dt_match[] = {
- { .compatible = "verisilicon,dc" },
+ { .compatible = "verisilicon,dc", .data = &vs_dc8000_info },
+ { .compatible = "nuvoton,ma35d1-dcu", .data = &vs_dcultra_lite_info },
{},
};
MODULE_DEVICE_TABLE(of, vs_dc_driver_dt_match);
@@ -33,6 +57,13 @@ static irqreturn_t vs_dc_irq_handler(int irq, void *private)
struct vs_dc *dc = private;
u32 irqs;
+ if (dc->info->family == VS_DC_FAMILY_DCULTRA_LITE) {
+ regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &irqs);
+ if (irqs & BIT(0))
+ vs_drm_handle_irq(dc, VSDC_TOP_IRQ_VSYNC(0));
+ return IRQ_HANDLED;
+ }
+
regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &irqs);
vs_drm_handle_irq(dc, irqs);
@@ -43,6 +74,7 @@ static irqreturn_t vs_dc_irq_handler(int irq, void *private)
static int vs_dc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ const struct vs_dc_info *info;
struct vs_dc *dc;
void __iomem *regs;
unsigned int port_count, i;
@@ -55,6 +87,10 @@ static int vs_dc_probe(struct platform_device *pdev)
return -ENODEV;
}
+ info = of_device_get_match_data(dev);
+ if (!info)
+ return -ENODEV;
+
port_count = of_graph_get_port_count(dev->of_node);
if (!port_count) {
dev_err(dev, "can't find DC downstream ports\n");
@@ -75,15 +111,31 @@ static int vs_dc_probe(struct platform_device *pdev)
if (!dc)
return -ENOMEM;
- dc->rsts[0].id = "core";
- dc->rsts[1].id = "axi";
- dc->rsts[2].id = "ahb";
+ dc->info = info;
- ret = devm_reset_control_bulk_get_optional_shared(dev, VSDC_RESET_COUNT,
- dc->rsts);
- if (ret) {
- dev_err(dev, "can't get reset lines\n");
- return ret;
+ if (info->family == VS_DC_FAMILY_DC8000) {
+ dc->rsts[0].id = "core";
+ dc->rsts[1].id = "axi";
+ dc->rsts[2].id = "ahb";
+
+ ret = devm_reset_control_bulk_get_optional_shared(dev,
+ VSDC_RESET_COUNT, dc->rsts);
+ if (ret) {
+ dev_err(dev, "can't get reset lines\n");
+ return ret;
+ }
+
+ dc->axi_clk = devm_clk_get_enabled(dev, "axi");
+ if (IS_ERR(dc->axi_clk)) {
+ dev_err(dev, "can't get axi clock\n");
+ return PTR_ERR(dc->axi_clk);
+ }
+
+ dc->ahb_clk = devm_clk_get_enabled(dev, "ahb");
+ if (IS_ERR(dc->ahb_clk)) {
+ dev_err(dev, "can't get ahb clock\n");
+ return PTR_ERR(dc->ahb_clk);
+ }
}
dc->core_clk = devm_clk_get_enabled(dev, "core");
@@ -92,28 +144,18 @@ static int vs_dc_probe(struct platform_device *pdev)
return PTR_ERR(dc->core_clk);
}
- dc->axi_clk = devm_clk_get_enabled(dev, "axi");
- if (IS_ERR(dc->axi_clk)) {
- dev_err(dev, "can't get axi clock\n");
- return PTR_ERR(dc->axi_clk);
- }
-
- dc->ahb_clk = devm_clk_get_enabled(dev, "ahb");
- if (IS_ERR(dc->ahb_clk)) {
- dev_err(dev, "can't get ahb clock\n");
- return PTR_ERR(dc->ahb_clk);
- }
-
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
dev_err(dev, "can't get irq\n");
return irq;
}
- ret = reset_control_bulk_deassert(VSDC_RESET_COUNT, dc->rsts);
- if (ret) {
- dev_err(dev, "can't deassert reset lines\n");
- return ret;
+ if (info->family == VS_DC_FAMILY_DC8000) {
+ ret = reset_control_bulk_deassert(VSDC_RESET_COUNT, dc->rsts);
+ if (ret) {
+ dev_err(dev, "can't deassert reset lines\n");
+ return ret;
+ }
}
regs = devm_platform_ioremap_resource(pdev, 0);
@@ -123,23 +165,30 @@ static int vs_dc_probe(struct platform_device *pdev)
goto err_rst_assert;
}
- dc->regs = devm_regmap_init_mmio(dev, regs, &vs_dc_regmap_cfg);
+ dc->regs = devm_regmap_init_mmio(dev, regs, info->regmap_cfg);
if (IS_ERR(dc->regs)) {
ret = PTR_ERR(dc->regs);
goto err_rst_assert;
}
- ret = vs_fill_chip_identity(dc->regs, &dc->identity);
- if (ret)
- goto err_rst_assert;
+ if (info->has_chip_id) {
+ ret = vs_fill_chip_identity(dc->regs, &dc->identity);
+ if (ret)
+ goto err_rst_assert;
- dev_info(dev, "Found DC%x rev %x customer %x\n", dc->identity.model,
- dc->identity.revision, dc->identity.customer_id);
+ dev_info(dev, "Found DC%x rev %x customer %x\n",
+ dc->identity.model, dc->identity.revision,
+ dc->identity.customer_id);
- if (port_count > dc->identity.display_count) {
- dev_err(dev, "too many downstream ports than HW capability\n");
- ret = -EINVAL;
- goto err_rst_assert;
+ if (port_count > dc->identity.display_count) {
+ dev_err(dev, "too many downstream ports than HW capability\n");
+ ret = -EINVAL;
+ goto err_rst_assert;
+ }
+ } else {
+ /* Fill identity from platform data */
+ dc->identity.display_count = info->display_count;
+ dc->identity.formats = info->formats;
}
for (i = 0; i < dc->identity.display_count; i++) {
@@ -168,7 +217,8 @@ static int vs_dc_probe(struct platform_device *pdev)
return 0;
err_rst_assert:
- reset_control_bulk_assert(VSDC_RESET_COUNT, dc->rsts);
+ if (info->family == VS_DC_FAMILY_DC8000)
+ reset_control_bulk_assert(VSDC_RESET_COUNT, dc->rsts);
return ret;
}
@@ -180,7 +230,8 @@ static void vs_dc_remove(struct platform_device *pdev)
dev_set_drvdata(&pdev->dev, NULL);
- reset_control_bulk_assert(VSDC_RESET_COUNT, dc->rsts);
+ if (dc->info->family == VS_DC_FAMILY_DC8000)
+ reset_control_bulk_assert(VSDC_RESET_COUNT, dc->rsts);
}
static void vs_dc_shutdown(struct platform_device *pdev)
diff --git a/drivers/gpu/drm/verisilicon/vs_dc.h b/drivers/gpu/drm/verisilicon/vs_dc.h
index ed1016f18758..f0613519af37 100644
--- a/drivers/gpu/drm/verisilicon/vs_dc.h
+++ b/drivers/gpu/drm/verisilicon/vs_dc.h
@@ -31,6 +31,7 @@ struct vs_dc {
struct clk *pix_clk[VSDC_MAX_OUTPUTS];
struct reset_control_bulk_data rsts[VSDC_RESET_COUNT];
+ const struct vs_dc_info *info;
struct vs_drm_dev *drm_dev;
struct vs_chip_identity identity;
};
diff --git a/drivers/gpu/drm/verisilicon/vs_drm.c b/drivers/gpu/drm/verisilicon/vs_drm.c
index fd259d53f49f..ff0fc6673006 100644
--- a/drivers/gpu/drm/verisilicon/vs_drm.c
+++ b/drivers/gpu/drm/verisilicon/vs_drm.c
@@ -27,6 +27,7 @@
#include "vs_dc.h"
#include "vs_dc_top_regs.h"
#include "vs_drm.h"
+#include "vs_hwdb.h"
#define DRIVER_NAME "verisilicon"
#define DRIVER_DESC "Verisilicon DC-series display controller driver"
@@ -72,12 +73,19 @@ static struct drm_mode_config_helper_funcs vs_mode_config_helper_funcs = {
.atomic_commit_tail = drm_atomic_helper_commit_tail,
};
-static void vs_mode_config_init(struct drm_device *drm)
+static void vs_mode_config_init(struct drm_device *drm, struct vs_dc *dc)
{
drm->mode_config.min_width = 0;
drm->mode_config.min_height = 0;
- drm->mode_config.max_width = 8192;
- drm->mode_config.max_height = 8192;
+
+ if (dc->info->family == VS_DC_FAMILY_DCULTRA_LITE) {
+ drm->mode_config.max_width = 1920;
+ drm->mode_config.max_height = 1080;
+ } else {
+ drm->mode_config.max_width = 8192;
+ drm->mode_config.max_height = 8192;
+ }
+
drm->mode_config.funcs = &vs_mode_config_funcs;
drm->mode_config.helper_private = &vs_mode_config_helper_funcs;
}
@@ -125,7 +133,7 @@ int vs_drm_initialize(struct vs_dc *dc, struct platform_device *pdev)
if (ret)
return ret;
- vs_mode_config_init(drm);
+ vs_mode_config_init(drm, dc);
/* Enable connectors polling */
drm_kms_helper_poll_init(drm);
diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c
index 09336af0900a..39402d75d841 100644
--- a/drivers/gpu/drm/verisilicon/vs_hwdb.c
+++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c
@@ -78,7 +78,7 @@ static const u32 vs_formats_array_with_yuv444[] = {
/* TODO: non-RGB formats */
};
-static const struct vs_formats vs_formats_no_yuv444 = {
+const struct vs_formats vs_formats_no_yuv444 = {
.array = vs_formats_array_no_yuv444,
.num = ARRAY_SIZE(vs_formats_array_no_yuv444)
};
diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.h b/drivers/gpu/drm/verisilicon/vs_hwdb.h
index 92192e4fa086..655cf93ca3aa 100644
--- a/drivers/gpu/drm/verisilicon/vs_hwdb.h
+++ b/drivers/gpu/drm/verisilicon/vs_hwdb.h
@@ -14,6 +14,29 @@ struct vs_formats {
unsigned int num;
};
+enum vs_dc_family {
+ VS_DC_FAMILY_DC8000,
+ VS_DC_FAMILY_DCULTRA_LITE,
+};
+
+/**
+ * struct vs_dc_info - per-SoC DC platform data
+ * @family: DC IP family (DC8000, DCUltra Lite, etc.)
+ * @display_count: number of display outputs (0 = auto-detect from DT/HW)
+ * @has_chip_id: whether chip identity registers exist
+ * @has_config_ex: whether CONFIG_EX commit mechanism exists
+ * @regmap_cfg: regmap configuration for this variant
+ * @formats: supported pixel formats (NULL = auto-detect from chip ID)
+ */
+struct vs_dc_info {
+ enum vs_dc_family family;
+ u32 display_count;
+ bool has_chip_id;
+ bool has_config_ex;
+ const struct regmap_config *regmap_cfg;
+ const struct vs_formats *formats;
+};
+
struct vs_chip_identity {
u32 model;
u32 revision;
@@ -23,6 +46,8 @@ struct vs_chip_identity {
const struct vs_formats *formats;
};
+extern const struct vs_formats vs_formats_no_yuv444;
+
int vs_fill_chip_identity(struct regmap *regs,
struct vs_chip_identity *ident);
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index 1f2be41ae496..197d5d683e22 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -55,8 +55,9 @@ static int vs_primary_plane_atomic_check(struct drm_plane *plane,
static void vs_primary_plane_commit(struct vs_dc *dc, unsigned int output)
{
- regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
- VSDC_FB_CONFIG_EX_COMMIT);
+ if (dc->info->has_config_ex)
+ regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
+ VSDC_FB_CONFIG_EX_COMMIT);
}
static void vs_primary_plane_atomic_enable(struct drm_plane *plane,
@@ -69,11 +70,13 @@ static void vs_primary_plane_atomic_enable(struct drm_plane *plane,
unsigned int output = vcrtc->id;
struct vs_dc *dc = vcrtc->dc;
- regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
- VSDC_FB_CONFIG_EX_FB_EN);
- regmap_update_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
- VSDC_FB_CONFIG_EX_DISPLAY_ID_MASK,
- VSDC_FB_CONFIG_EX_DISPLAY_ID(output));
+ if (dc->info->has_config_ex) {
+ regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
+ VSDC_FB_CONFIG_EX_FB_EN);
+ regmap_update_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
+ VSDC_FB_CONFIG_EX_DISPLAY_ID_MASK,
+ VSDC_FB_CONFIG_EX_DISPLAY_ID(output));
+ }
vs_primary_plane_commit(dc, output);
}
@@ -88,8 +91,9 @@ static void vs_primary_plane_atomic_disable(struct drm_plane *plane,
unsigned int output = vcrtc->id;
struct vs_dc *dc = vcrtc->dc;
- regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
- VSDC_FB_CONFIG_EX_FB_EN);
+ if (dc->info->has_config_ex)
+ regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
+ VSDC_FB_CONFIG_EX_FB_EN);
vs_primary_plane_commit(dc, output);
}
@@ -126,6 +130,11 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
VSDC_FB_CONFIG_UV_SWIZZLE_EN,
vs_state->format.uv_swizzle);
+ /* DCUltra Lite requires explicit enable/reset bits in FB_CONFIG */
+ if (!dc->info->has_config_ex)
+ regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output),
+ VSDC_FB_CONFIG_ENABLE | VSDC_FB_CONFIG_RESET);
+
dma_addr = vs_fb_get_dma_addr(fb, &state->src);
regmap_write(dc->regs, VSDC_FB_ADDRESS(output),
@@ -133,16 +142,18 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
regmap_write(dc->regs, VSDC_FB_STRIDE(output),
fb->pitches[0]);
- regmap_write(dc->regs, VSDC_FB_TOP_LEFT(output),
- VSDC_MAKE_PLANE_POS(state->crtc_x, state->crtc_y));
- regmap_write(dc->regs, VSDC_FB_BOTTOM_RIGHT(output),
- VSDC_MAKE_PLANE_POS(state->crtc_x + state->crtc_w,
- state->crtc_y + state->crtc_h));
regmap_write(dc->regs, VSDC_FB_SIZE(output),
VSDC_MAKE_PLANE_SIZE(state->crtc_w, state->crtc_h));
- regmap_write(dc->regs, VSDC_FB_BLEND_CONFIG(output),
- VSDC_FB_BLEND_CONFIG_BLEND_DISABLE);
+ if (dc->info->has_config_ex) {
+ regmap_write(dc->regs, VSDC_FB_TOP_LEFT(output),
+ VSDC_MAKE_PLANE_POS(state->crtc_x, state->crtc_y));
+ regmap_write(dc->regs, VSDC_FB_BOTTOM_RIGHT(output),
+ VSDC_MAKE_PLANE_POS(state->crtc_x + state->crtc_w,
+ state->crtc_y + state->crtc_h));
+ regmap_write(dc->regs, VSDC_FB_BLEND_CONFIG(output),
+ VSDC_FB_BLEND_CONFIG_BLEND_DISABLE);
+ }
vs_primary_plane_commit(dc, output);
}
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h b/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h
index cbb125c46b39..288064760b48 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h
@@ -16,6 +16,8 @@
#define VSDC_FB_STRIDE(n) (0x1408 + 0x4 * (n))
#define VSDC_FB_CONFIG(n) (0x1518 + 0x4 * (n))
+#define VSDC_FB_CONFIG_ENABLE BIT(0)
+#define VSDC_FB_CONFIG_RESET BIT(4)
#define VSDC_FB_CONFIG_CLEAR_EN BIT(8)
#define VSDC_FB_CONFIG_ROT_MASK GENMASK(13, 11)
#define VSDC_FB_CONFIG_ROT(v) ((v) << 11)
--
2.43.0
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: display: verisilicon,dc: generalize for DCUltra Lite variant
From: Joey Lu @ 2026-05-11 7:51 UTC (permalink / raw)
To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, robh, krzk+dt, conor+dt
Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel,
linux-kernel, Joey Lu
In-Reply-To: <20260511075142.54752-1-a0987203069@gmail.com>
Extend the verisilicon,dc base schema to accommodate the Nuvoton MA35D1
DCUltra Lite (a previous generation of the DC8000 series) which has a
different clock topology, no reset control, and a single output.
- Replace the fixed clock/reset item lists with minItems/maxItems ranges
so sub-schemas can enforce variant-specific constraints
- Add a 'port' property (single-port alias) alongside the existing 'ports'
for single-output variants
- Remove the mandatory 'ports' requirement from the base schema; sub-schemas
shall enforce their own port topology
- Add a 'select' stanza so the validator matches any node whose compatible
contains a known Verisilicon DC string, including SoC-specific glue
- Relax additionalProperties to allow unevaluatedProperties enforcement in
sub-schemas
- Fix a minor whitespace issue in the port@0 description
Add nuvoton,ma35d1-dcu.yaml as a sub-schema for the Nuvoton MA35D1 DCUltra
Lite display controller:
The Nuvoton MA35D1 integrates the Verisilicon DCUltra Lite display
controller. It is a single-output display controller with a 32-bit
RGB (DPI) interface. Unlike the DC8000, it does not have discoverable
chip identity registers, does not support the CONFIG_EX commit path,
and uses dedicated IRQ status/enable registers at offsets 0x147C/0x1480.
The clock topology uses two clocks (bus gate and pixel divider) and
does not require explicit reset control from the driver.
Signed-off-by: Joey Lu <a0987203069@gmail.com>
---
.../bindings/display/nuvoton,ma35d1-dcu.yaml | 94 +++++++++++++++++++
.../bindings/display/verisilicon,dc.yaml | 64 +++++++------
2 files changed, 131 insertions(+), 27 deletions(-)
create mode 100644 Documentation/devicetree/bindings/display/nuvoton,ma35d1-dcu.yaml
diff --git a/Documentation/devicetree/bindings/display/nuvoton,ma35d1-dcu.yaml b/Documentation/devicetree/bindings/display/nuvoton,ma35d1-dcu.yaml
new file mode 100644
index 000000000000..9279004ae27c
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/nuvoton,ma35d1-dcu.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/nuvoton,ma35d1-dcu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 DCUltra Lite display controller
+
+maintainers:
+ - Joey Lu <yclu4@nuvoton.com>
+
+description:
+ The Nuvoton MA35D1 integrates the Verisilicon DCUltra Lite display
+ controller. It is a single-output display controller with a 32-bit
+ RGB (DPI) interface.
+
+select:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - nuvoton,ma35d1-dcu
+ required:
+ - compatible
+
+allOf:
+ - $ref: http://devicetree.org/schemas/display/verisilicon,dc.yaml#
+
+properties:
+ compatible:
+ const: nuvoton,ma35d1-dcu
+
+ reg:
+ maxItems: 1
+ description:
+ Register range of the DCUltra Lite controller. The address space
+ is 0x2000 bytes.
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: Bus clock that gates register access (DCU_GATE)
+ - description: Pixel clock divider for display timing (DCUP_DIV)
+
+ clock-names:
+ items:
+ - const: core
+ - const: pix0
+
+ resets:
+ maxItems: 1
+ description:
+ Optional reset for the display controller. The driver does not
+ assert or deassert this reset; it may be used by firmware or
+ boot loaders to bring the hardware to a clean state.
+
+ port:
+ $ref: /schemas/graph.yaml#/properties/port
+ description:
+ Output port to the downstream display device (e.g. RGB panel).
+ The DCUltra Lite supports a single parallel RGB output.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - port
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+ #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
+
+ display@40260000 {
+ compatible = "nuvoton,ma35d1-dcu";
+ reg = <0x40260000 0x2000>;
+ interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk DCU_GATE>, <&clk DCUP_DIV>;
+ clock-names = "core", "pix0";
+ resets = <&sys MA35D1_RESET_DISP>;
+
+ port {
+ dpi_out: endpoint {
+ remote-endpoint = <&panel_in>;
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/display/verisilicon,dc.yaml b/Documentation/devicetree/bindings/display/verisilicon,dc.yaml
index 9dc35ab973f2..00884529f8c1 100644
--- a/Documentation/devicetree/bindings/display/verisilicon,dc.yaml
+++ b/Documentation/devicetree/bindings/display/verisilicon,dc.yaml
@@ -9,15 +9,34 @@ title: Verisilicon DC-series display controllers
maintainers:
- Icenowy Zheng <uwu@icenowy.me>
+description:
+ Verisilicon DC-series display controllers.
+
+# Select any node whose compatible contains one of the known Verisilicon DC
+# or DC-derived compatible strings, including SoC-specific glue variants.
+select:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - verisilicon,dc
+ - thead,th1520-dc8200
+ - nuvoton,ma35d1-dcu
+ required:
+ - compatible
+
properties:
$nodename:
pattern: "^display@[0-9a-f]+$"
compatible:
- items:
- - enum:
- - thead,th1520-dc8200
- - const: verisilicon,dc # DC IPs have discoverable ID/revision registers
+ # Enumerated in full so the schema validator can verify any compatible
+ # string against this list, including those from child schemas.
+ contains:
+ enum:
+ - verisilicon,dc
+ - thead,th1520-dc8200
+ - nuvoton,ma35d1-dcu
reg:
maxItems: 1
@@ -26,32 +45,24 @@ properties:
maxItems: 1
clocks:
- items:
- - description: DC Core clock
- - description: DMA AXI bus clock
- - description: Configuration AHB bus clock
- - description: Pixel clock of output 0
- - description: Pixel clock of output 1
+ minItems: 2
+ maxItems: 5
clock-names:
- items:
- - const: core
- - const: axi
- - const: ahb
- - const: pix0
- - const: pix1
+ minItems: 2
+ maxItems: 5
resets:
- items:
- - description: DC Core reset
- - description: DMA AXI bus reset
- - description: Configuration AHB bus reset
+ minItems: 1
+ maxItems: 3
reset-names:
- items:
- - const: core
- - const: axi
- - const: ahb
+ minItems: 1
+ maxItems: 3
+
+ port:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: Single video output port for single-output variants.
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -59,7 +70,7 @@ properties:
properties:
port@0:
$ref: /schemas/graph.yaml#/properties/port
- description: The first output channel , endpoint 0 should be
+ description: The first output channel, endpoint 0 should be
used for DPI format output and endpoint 1 should be used
for DP format output.
@@ -75,9 +86,8 @@ required:
- interrupts
- clocks
- clock-names
- - ports
-additionalProperties: false
+additionalProperties: true
examples:
- |
--
2.43.0
^ permalink raw reply related
* [PATCH 0/2] drm/verisilicon: add Nuvoton MA35D1 DCUltra Lite support
From: Joey Lu @ 2026-05-11 7:51 UTC (permalink / raw)
To: zhengxingda, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, robh, krzk+dt, conor+dt
Cc: ychuang3, schung, yclu4, dri-devel, devicetree, linux-arm-kernel,
linux-kernel, Joey Lu
This series adds support for the Verisilicon DCUltra Lite display
controller as integrated in the Nuvoton MA35D1 SoC.
The Verisilicon DC driver and its DT binding were originally written by
Icenowy Zheng <zhengxingda@iscas.ac.cn> for the TH1520 SoC, which
carries a DC8000 IP block. The present series builds on that foundation
with gratitude to Icenowy for the original work.
The DCUltra Lite is a previous generation of the DC8000 series. While
the two IPs share a broadly similar register layout, a number of
differences prevent the existing driver from working on the MA35D1
without modification:
- No chip identity registers: the DCUltra Lite does not expose
model/revision/customer_id hardware registers, so variant detection
must come from platform data rather than register reads.
- No CONFIG_EX commit path: the DC8000 staging registers
(FB_CONFIG_EX, FB_TOP_LEFT, FB_BOTTOM_RIGHT, FB_BLEND_CONFIG,
PANEL_CONFIG_EX) are absent; the DCUltra Lite uses enable and reset
bits in FB_CONFIG (bit 0 / bit 4) for direct framebuffer updates.
- No PANEL_START register: panel output begins when
PANEL_CONFIG.RUNNING is set; the DC8000 multi-display sync start
register at 0x1CCC does not exist.
- Different IRQ registers: IRQ_STA at 0x147C / IRQ_EN at 0x1480,
versus the DC8000's IRQ_ACK at 0x0010 / IRQ_EN at 0x0014.
- Simpler clock/reset topology: two clocks ("core" bus gate and "pix0"
pixel divider), no driver-managed resets. The DC8000 requires
core/axi/ahb clocks and three reset lines.
- Single display output: no per-output indexing is needed.
- Smaller register space: max_register 0x2000 vs. DC8000's 0x2544.
Patch 1 extends the verisilicon,dc DT binding to accommodate variants
with flexible clock/reset counts and adds a new sub-schema for
nuvoton,ma35d1-dcu.
Patch 2 introduces the vs_dc_info platform data structure, selects the
correct code paths based on the detected IP family, extends Kconfig for
ARCH_MA35, and wires up the "nuvoton,ma35d1-dcu" OF compatible string.
Both patches have been tested on Nuvoton MA35D1 hardware and are
functioning correctly.
Joey Lu (2):
dt-bindings: display: verisilicon,dc: generalize for DCUltra Lite
variant
drm/verisilicon: add support for Nuvoton MA35D1 DCUltra Lite display
controller
.../bindings/display/nuvoton,ma35d1-dcu.yaml | 94 +++++++++++++
.../bindings/display/verisilicon,dc.yaml | 64 +++++----
drivers/gpu/drm/verisilicon/Kconfig | 2 +-
drivers/gpu/drm/verisilicon/vs_bridge.c | 28 ++--
drivers/gpu/drm/verisilicon/vs_crtc.c | 13 +-
drivers/gpu/drm/verisilicon/vs_dc.c | 129 ++++++++++++------
drivers/gpu/drm/verisilicon/vs_dc.h | 1 +
drivers/gpu/drm/verisilicon/vs_drm.c | 16 ++-
drivers/gpu/drm/verisilicon/vs_hwdb.c | 2 +-
drivers/gpu/drm/verisilicon/vs_hwdb.h | 25 ++++
.../gpu/drm/verisilicon/vs_primary_plane.c | 43 +++---
.../drm/verisilicon/vs_primary_plane_regs.h | 2 +
12 files changed, 318 insertions(+), 101 deletions(-)
create mode 100644 Documentation/devicetree/bindings/display/nuvoton,ma35d1-dcu.yaml
--
2.43.0
^ permalink raw reply
* [PATCH v2 3/3] i2c: xiic: don't clobber msg->len to signal block-read completion
From: Abdurrahman Hussain @ 2026-05-11 7:46 UTC (permalink / raw)
To: Michal Simek, Andi Shyti; +Cc: linux-arm-kernel, linux-i2c, linux-kernel
In-Reply-To: <20260511-i2c-xiic-v2-0-c16380cb1594@nexthop.ai>
At the end of a SMBus block read the BNB handler force-set
tx_msg->len = 1 to push xiic_tx_space() to zero so the STATE_DONE
branch would fire. Two problems:
1. tx_msg and rx_msg alias the same i2c_msg struct during a receive
(see xiic_start_recv), so overwriting tx_msg->len also changes
rx_msg->len. The i2c core's i2c_smbus_check_pec() then reads the
PEC from the wrong offset -- buf[0] instead of buf[rxmsg_len + 1]
-- and either mis-validates or returns -EBADMSG.
2. xiic_start_recv sets tx_pos = msg->len (typically 2 when PEC is
enabled). xiic_tx_space() is unsigned msg->len - tx_pos, so
setting msg->len = 1 with tx_pos = 2 underflows to 0xFFFFFFFF and
xiic_tx_space() never compares equal to 0 -- the STATE_DONE check
falls through to STATE_ERROR, giving -EIO.
Instead, advance tx_pos up to msg->len. That drives tx_space to 0
without touching msg->len, preserving the buffer length that
xiic_smbus_block_read_setup() already grew to cover the length byte,
the payload and the optional PEC byte.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/i2c/busses/i2c-xiic.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index a9452eca26df..24183483f1b3 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -865,8 +865,11 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
if (i2c->tx_msg && i2c->smbus_block_read) {
i2c->smbus_block_read = false;
- /* Set requested message len=1 to indicate STATE_DONE */
- i2c->tx_msg->len = 1;
+ /*
+ * Drive xiic_tx_space() to 0 to signal STATE_DONE
+ * without truncating the rx_msg length.
+ */
+ i2c->tx_pos = i2c->tx_msg->len;
}
if (!i2c->tx_msg)
--
2.53.0
^ permalink raw reply related
* [PATCH v2 2/3] i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO
From: Abdurrahman Hussain @ 2026-05-11 7:46 UTC (permalink / raw)
To: Michal Simek, Andi Shyti; +Cc: linux-arm-kernel, linux-i2c, linux-kernel
In-Reply-To: <20260511-i2c-xiic-v2-0-c16380cb1594@nexthop.ai>
For the normal path of xiic_smbus_block_read_setup() (rxmsg_len less
than IIC_RX_FIFO_DEPTH), RFD was programmed to rxmsg_len - 2, which
fires the RX_FULL interrupt while the last payload byte is still in
flight. xiic_read_rx()'s bytes_rem == 1 branch then sets NACK on that
byte still on the wire, truncating the read in the PEC-enabled case.
Raise the threshold so RX_FULL fires only once every remaining byte
(payload plus optional PEC) is already buffered in the FIFO. That
routes the drain through xiic_read_rx()'s bytes_rem == 0 path, which
reads everything out and emits the stop cleanly. For the non-PEC path
the full payload is still read out through the same bytes_rem == 0
branch; the only user-visible change is that the controller waits one
extra byte-time before servicing the interrupt.
The deferred-fire formula is rxmsg_len + pec_len - 1, and the RFD
register at XIIC_RFD_REG_OFFSET is a 4-bit field. Widen the
chunk-vs-defer guard to (rxmsg_len + pec_len > IIC_RX_FIFO_DEPTH) so
the boundary case rxmsg_len == IIC_RX_FIFO_DEPTH with PEC enabled
cannot write 16 into that 4-bit register; it routes through the
chunked drain instead, which already caps RFD at IIC_RX_FIFO_DEPTH - 1.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/i2c/busses/i2c-xiic.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 959a47b645a5..a9452eca26df 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -542,10 +542,11 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
unsigned int pec_len = i2c->rx_msg->len - 1;
/* Set Receive fifo depth */
- if (rxmsg_len > IIC_RX_FIFO_DEPTH) {
+ if (rxmsg_len + pec_len > IIC_RX_FIFO_DEPTH) {
/*
- * When Rx msg len greater than or equal to Rx fifo capacity
- * Receive fifo depth should set to Rx fifo capacity minus 1
+ * Trailing payload (data + optional PEC) exceeds Rx FIFO
+ * capacity; drain in chunks. Fire RX_FULL when the FIFO is
+ * full and let the ISR re-arm for the remainder.
*/
rfd_set = IIC_RX_FIFO_DEPTH - 1;
i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
@@ -559,11 +560,8 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
rfd_set = 0;
i2c->rx_msg->len = SMBUS_BLOCK_READ_MIN_LEN;
} else {
- /*
- * When Rx msg len less than Rx fifo capacity
- * Receive fifo depth should set to Rx msg len minus 2
- */
- rfd_set = rxmsg_len - 2;
+ /* Defer RX_FULL until all trailing bytes are in FIFO. */
+ rfd_set = rxmsg_len + pec_len - 1;
i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
}
xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);
--
2.53.0
^ permalink raw reply related
* [PATCH v2 1/3] i2c: xiic: preserve PEC byte length in SMBus block read setup
From: Abdurrahman Hussain @ 2026-05-11 7:46 UTC (permalink / raw)
To: Michal Simek, Andi Shyti; +Cc: linux-arm-kernel, linux-i2c, linux-kernel
In-Reply-To: <20260511-i2c-xiic-v2-0-c16380cb1594@nexthop.ai>
xiic_smbus_block_read_setup() overwrites rx_msg->len based on the
announced block length byte, but the i2c core appends an extra byte to
msg->len when PEC is enabled on the client (see
__i2c_smbus_xfer_emulated in i2c-core-smbus.c). Before this handler
runs, rx_msg->len is therefore 1 + pec_len, where pec_len is 0 or 1.
Overwriting rx_msg->len to rxmsg_len + 1 drops that PEC byte from the
caller's buffer: the PEC byte is never drained from the FIFO and the
core's i2c_smbus_check_pec() reads the last payload byte instead of
the actual PEC, returning -EBADMSG even on clean transfers. Capture
pec_len before the overwrite and add it back when recomputing
rx_msg->len.
This is a pure bug fix; non-PEC behaviour is unchanged (pec_len == 0).
Tested on a Xilinx AXI IIC FPGA block driving an adm1266 PMBus
blackbox read -- with PEC enabled the full 64-byte record transfers
cleanly after this change.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/i2c/busses/i2c-xiic.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 3e7735e1dae0..959a47b645a5 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -539,6 +539,8 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
/* Check if received length is valid */
if (rxmsg_len <= I2C_SMBUS_BLOCK_MAX) {
+ unsigned int pec_len = i2c->rx_msg->len - 1;
+
/* Set Receive fifo depth */
if (rxmsg_len > IIC_RX_FIFO_DEPTH) {
/*
@@ -546,7 +548,7 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
* Receive fifo depth should set to Rx fifo capacity minus 1
*/
rfd_set = IIC_RX_FIFO_DEPTH - 1;
- i2c->rx_msg->len = rxmsg_len + 1;
+ i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
} else if ((rxmsg_len == 1) ||
(rxmsg_len == 0)) {
/*
@@ -562,7 +564,7 @@ static void xiic_smbus_block_read_setup(struct xiic_i2c *i2c)
* Receive fifo depth should set to Rx msg len minus 2
*/
rfd_set = rxmsg_len - 2;
- i2c->rx_msg->len = rxmsg_len + 1;
+ i2c->rx_msg->len = rxmsg_len + 1 + pec_len;
}
xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set);
--
2.53.0
^ permalink raw reply related
* [PATCH v2 0/3] i2c: xiic: fix SMBus block read and PEC support
From: Abdurrahman Hussain @ 2026-05-11 7:46 UTC (permalink / raw)
To: Michal Simek, Andi Shyti; +Cc: linux-arm-kernel, linux-i2c, linux-kernel
This series fixes three independent bugs in the Xilinx AXI IIC driver
that together make SMBus block reads with PEC return -EBADMSG or -EIO
on otherwise clean transfers. They only surface when the client has
I2C_CLIENT_PEC set; non-PEC block reads happen to mask each issue in
turn.
The problems were uncovered driving an adm1266 PMBus device behind a
Xilinx AXI IIC FPGA block and reading its 64-byte blackbox record.
Patch 1 stops xiic_smbus_block_read_setup() from truncating rx_msg->len.
The i2c core appends a byte to msg->len when PEC is enabled, so
overwriting the length to "block size + 1" silently drops the PEC byte
and i2c_smbus_check_pec() then reads the last payload byte as the PEC.
Patch 2 raises the RX_FULL threshold so the interrupt only fires once
every remaining byte (payload plus optional PEC) is already buffered in
the FIFO. The previous threshold of rxmsg_len - 2 caused the
bytes_rem == 1 path in xiic_read_rx() to NACK a byte still on the wire.
The chunk-vs-defer guard now also accounts for the PEC byte so a
rxmsg_len == IIC_RX_FIFO_DEPTH PEC-enabled read does not push
XIIC_RFD_REG_OFFSET past its 4-bit range.
Patch 3 stops the BNB handler from forcing tx_msg->len = 1 to signal
completion. tx_msg and rx_msg alias the same i2c_msg during a receive,
so this also clobbered rx_msg->len; and because tx_pos is already at 2
in the PEC case, the unsigned subtraction in xiic_tx_space() underflowed
and the STATE_DONE check fell through to STATE_ERROR. Advancing tx_pos
up to msg->len drives tx_space to zero without touching the length.
All three patches are pure bug fixes; non-PEC behaviour is unchanged.
Tested on real hardware -- a Xilinx AXI IIC controller talking to an
adm1266, where 64-byte PEC-checked block reads now complete cleanly.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Changes in v2:
- Patch 2: widen the chunk-vs-defer guard in xiic_smbus_block_read_setup()
to include pec_len, so a 16-byte PEC-enabled block read routes through
the chunked drain rather than writing 16 into the 4-bit
XIIC_RFD_REG_OFFSET register. No tree-level change to patches 1 or 3.
- Link to v1: https://patch.msgid.link/20260427-i2c-xiic-v1-0-e6207f9aa5ad@nexthop.ai
To: Michal Simek <michal.simek@amd.com>
To: Andi Shyti <andi.shyti@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-i2c@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Abdurrahman Hussain (3):
i2c: xiic: preserve PEC byte length in SMBus block read setup
i2c: xiic: defer RX_FULL until all trailing bytes are in FIFO
i2c: xiic: don't clobber msg->len to signal block-read completion
drivers/i2c/busses/i2c-xiic.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20260427-i2c-xiic-2aeb501ec02a
Best regards,
--
Abdurrahman Hussain <abdurrahman@nexthop.ai>
^ permalink raw reply
* Re: [PATCH v16 00/12] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O
From: Bartosz Golaszewski @ 2026-05-11 7:38 UTC (permalink / raw)
To: Vinod Koul
Cc: Bartosz Golaszewski, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Stephan Gerhold, Bjorn Andersson, Peter Ujfalusi, Michal Simek,
Frank Li, dmaengine, linux-doc, linux-kernel, linux-arm-msm,
linux-crypto, linux-arm-kernel, Bartosz Golaszewski,
Dmitry Baryshkov, Konrad Dybcio, Manivannan Sadhasivam
In-Reply-To: <ditrkd5jcxlx7onykxh6n3qhyoclfngmpp277y4t4qwc4vswoo@5os4o5lumidn>
On Thu, May 7, 2026 at 11:55 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Mon, Apr 27, 2026 at 11:15:33AM +0200, Bartosz Golaszewski wrote:
> > This missed the v7.1 cycle so let's try to get it in for v7.2.
> >
> > Merging strategy: there are build-time dependencies between the crypto
> > and DMA patches so the best approach is for Vinod to create an immutable
> > branch with the DMA part pulled in by the crypto tree.
> >
> > This iteration continues to build on top of v12 but uses the BAM's NWD
> > bit on data descriptors as suggested by Stephan. To that end, there are
> > some more changes like reversing the order of command and data
> > descriptors queuedy by the QCE driver.
> >
> > Currently the QCE crypto driver accesses the crypto engine registers
> > directly via CPU. Trust Zone may perform crypto operations simultaneously
> > resulting in a race condition. To remedy that, let's introduce support
> > for BAM locking/unlocking to the driver. The BAM driver will now wrap
> > any existing issued descriptor chains with additional descriptors
> > performing the locking when the client starts the transaction
> > (dmaengine_issue_pending()). The client wanting to profit from locking
> > needs to switch to performing register I/O over DMA and communicate the
> > address to which to perform the dummy writes via a call to
> > dmaengine_desc_attach_metadata().
> >
> > In the specific case of the BAM DMA this translates to sending command
> > descriptors performing dummy writes with the relevant flags set. The BAM
> > will then lock all other pipes not related to the current pipe group, and
> > keep handling the current pipe only until it sees the the unlock bit.
> >
> > In order for the locking to work correctly, we also need to switch to
> > using DMA for all register I/O.
> >
> > On top of this, the series contains some additional tweaks and
> > refactoring.
> >
> > The goal of this is not to improve the performance but to prepare the
> > driver for supporting decryption into secure buffers in the future.
> >
> > Tested with tcrypt.ko, kcapi and cryptsetup.
> >
> > Shout out to Daniel and Udit from Qualcomm for helping me out with some
> > DMA issues we encountered.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>
> For the whole series,
>
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
>
> Thanks for incorporating all the comments, Bart!
>
> - Mani
>
Vinod: Can you please queue patches 1-5 on an immutable branch for
v7.2 and provide it to Herbert to queue the following crypto patches?
Thanks,
Bartosz
^ permalink raw reply
* Re: [PATCH v14 1/5] dt-bindings: vendor-prefixes: Add Verisilicon
From: Benjamin Gaignard @ 2026-05-11 7:22 UTC (permalink / raw)
To: Diederik de Haas, joro, will, robin.murphy, krzk+dt, conor+dt,
heiko
Cc: iommu, devicetree, linux-kernel, linux-arm-kernel, linux-rockchip,
kernel, Conor Dooley
In-Reply-To: <DIEC8YT0XK4O.VAG6G4H31KJM@cknow-tech.com>
Le 09/05/2026 à 19:56, Diederik de Haas a écrit :
> On Wed Apr 15, 2026 at 9:23 AM CEST, Benjamin Gaignard wrote:
>> Verisilicon Microelectronics is a company based in Shanghai, China,
>> developping hardware blocks for SoC.
>>
>> https://verisilicon.com/
>>
>> Add their name to the list of vendors.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> index ee7fd3cfe203..ebd9072300a8 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
>> @@ -1761,6 +1761,8 @@ patternProperties:
>> description: Variscite Ltd.
>> "^vdl,.*":
>> description: Van der Laan b.v.
>> + "^verisilicon,.*":
>> + description: VeriSilicon Microelectronics
>> "^vertexcom,.*":
>> description: Vertexcom Technologies, Inc.
>> "^via,.*":
> FTR: Another version of this patch is already present in Linus' tree:
> c131d78840d7 ("dt-bindings: vendor-prefixes: add verisilicon")
It seems it has been merged right 7.0-rc1 that I used for this series.
Both are almost identical so we can ignore mine.
Regards,
Benjamin
^ permalink raw reply
* Re: [PATCH 1/2] [RFC] debugobjects: avoid gcc-16.0.1 section mismatch
From: Thomas Gleixner @ 2026-05-11 7:18 UTC (permalink / raw)
To: Arnd Bergmann, Arnd Bergmann, Will Deacon, Robin Murphy,
Joerg Roedel, Andrew Morton
Cc: linux-arm-kernel, iommu, linux-kernel, Sebastian Andrzej Siewior
In-Reply-To: <d52dcd45-959a-4ff4-9e82-0bae133785f1@app.fastmail.com>
On Mon, May 11 2026 at 08:17, Arnd Bergmann wrote:
> On Sun, May 10, 2026, at 21:31, Thomas Gleixner wrote:
>> On Tue, Feb 03 2026 at 17:23, Arnd Bergmann wrote:
>>> WARNING: modpost: vmlinux: section mismatch in reference: lookup_object_or_alloc.part.0+0x1ac (section: .text) -> is_static_object (section: .init.text)
>>>
>>> From what I can tell, the transformation is correct, as this
>>> is only called when lookup_object_or_alloc() is called from
>>> debug_objects_selftest(), which is also __init.
>>
>> So clearly the compiler is buggy. It creates an __init specific copy of
>> lookup_object_or_alloc() and then fails to attribute it correctly.
>
> I don't see what else the compiler is supposed to do, it has no idea what
> __init means in the kernel, or what the rules are for calling between
> that and normal functions. Putting a non-inlined lookup_object_or_alloc()
> into a special section without an explicit attribute would clearly
> be a bug.
I agree that the compiler does not know what __init means, but this
sucks as it leaves an unused copy of lookup_object_or_alloc() around
after init.
What happens if you mark is_static_object() with 'noinline'?
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v13 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
From: Arnaud POULIQUEN @ 2026-05-11 7:10 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Beleswar Prasad Padhi, Shenwei Wang, Andrew Lunn, Linus Walleij,
Bartosz Golaszewski, Jonathan Corbet, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Frank Li,
Sascha Hauer, Shuah Khan, linux-gpio@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
devicetree@vger.kernel.org, linux-remoteproc@vger.kernel.org,
imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
dl-linux-imx, Bartosz Golaszewski
In-Reply-To: <afzIABSh1xtMEGbf@p14s>
Hello Mathieu,
[...]
>>
>> A third option would be a combination of both approaches: instantiate the
>> device using the same name service from the remote side, as done in
>> rpmsg-tty. In that case, a get_config message, or a similar mechanism, would
>> also be needed to retrieve the port information from the remote side.
>>
>
> I'm not overly fond of a get_config message because it is one more thing we
> have to define and maintain.
>
> Arnaud: is there a get_config message already defined for rpmsg_tty?
No there isn't.
Regards,
Arnaud
>
> Beleswar: Can you provide a link to a virtio device that would use a get_config
> message?
>
>> Tanmaya also proposed another alternative based on reserved addresses.
>>
>> At this point, I suggest letting Mathieu review the discussion and recommend
>> the most suitable approach.
>>
>> Thanks,
>> Arnaud
>>
>>>>
>>>> At the end, whatever solution is implemented, my main concern is that the
>>>> Linux driver design should, if possible, avoid adding unnecessary complexity
>>>> or limitations on the remote side (for instance in openAMP project).
>>>
>>>
>>> Yes definitely, I want the same. Feel free to let me know if this does
>>> not suit with the OpenAMP project.
>>>
>>> Thanks,
>>> Beleswar
>>>
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>
>>>>
>>>>> So Linux does not need to send the port idx everytime while sending a
>>>>> gpio message anymore.
>>>>>
>>>>> Thanks,
>>>>> Beleswar
>>>>>
>>>>> [...]
>>>>>
>>>>
>>
^ permalink raw reply
* Re: [PATCH v3 4/7] PCI: dwc: Use common pci_host_common_link_train_delay() helper
From: Krzysztof Wilczyński @ 2026-05-11 7:02 UTC (permalink / raw)
To: Hans Zhang
Cc: bhelgaas, lpieralisi, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai,
robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel
In-Reply-To: <20260511055923.37117-5-18255117159@163.com>
Hello,
> - /*
> - * As per PCIe r6.0, sec 6.6.1, a Downstream Port that supports Link
> - * speeds greater than 5.0 GT/s, software must wait a minimum of 100 ms
> - * after Link training completes before sending a Configuration Request.
> - */
> - if (pci->max_link_speed > 2)
> - msleep(PCIE_RESET_CONFIG_WAIT_MS);
> + pci_host_common_link_train_delay(pci->max_link_speed);
This comment could move to the helper you added.
Thank you!
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 0/4] remoteproc: add Cortex-A Core remoteproc support on i.MX platforms
From: Peng Fan @ 2026-05-11 7:00 UTC (permalink / raw)
To: Jiafei Pan
Cc: andersson, mathieu.poirier, peng.fan, Frank.Li, s.hauer, kernel,
festevam, imx, linux-arm-kernel, linux-kernel, Zhiqiang.Hou,
mingkai.hu, linux-remoteproc, devicetree
In-Reply-To: <20260511023928.39640-1-Jiafei.Pan@nxp.com>
Hi Jiafei,
On Mon, May 11, 2026 at 10:39:24AM +0800, Jiafei Pan wrote:
>This patch series is to add remoteproc support on Cortex-A Core of i.MX platforms:
>1. Adding dts binding for Cortex-A Core remoteproc
>2. Enable Cortex-A Core remoteproc support in remoteproc driver
>3. Adding dts example on imx93 platforms.
Normally, we only keep one patch or patch series from the same author that
modifies the same file in the review queue at a time.
You already have a patch for imx_rproc.c currently under review(got R-b, but
need Mathieu to handle), so this patch series might be delayed to be reviewed.
As I have replied in internal, to make multiple OS run on Cortex-A SMP without
hypervsior, some GIC changes are required, without those GIC changes,
this patchset is not testable using upstream tree.
Thanks,
Peng
>
>Signed-off-by: Jiafei Pan <Jiafei.Pan@nxp.com>
>
>---
>Changes in v3:
>- Fixed dt_binding_check warnings
>- Updated prefix of patch subject
>
>Changes in v2:
>- Update arch/arm64/boot/dts/freescale/Makefile to add new dts
>
>---
>Hou Zhiqiang (1):
> remoteproc: imx_rproc: add autoboot support for A-core
>
>Jiafei Pan (3):
> dt-bindings: remoteproc: add imx-rproc-psci
> remoteproc: imx_rproc: add support for Cortex-A Core
> arm64: dts: imx93: Cortex-A Core remoteproc device node
>
> .../remoteproc/fsl,imx-rproc-psci.yaml | 51 ++++++
> arch/arm64/boot/dts/freescale/Makefile | 2 +
> .../imx93-11x11-evk-multicore-rtos.dts | 39 +++++
> .../imx93-14x14-evk-multicore-rtos.dts | 39 +++++
> .../boot/dts/freescale/imx93-rproc-ca55.dtsi | 14 ++
> drivers/remoteproc/imx_rproc.c | 150 ++++++++++++++++++
> drivers/remoteproc/imx_rproc.h | 2 +
> 7 files changed, 297 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc-psci.yaml
> create mode 100644 arch/arm64/boot/dts/freescale/imx93-11x11-evk-multicore-rtos.dts
> create mode 100644 arch/arm64/boot/dts/freescale/imx93-14x14-evk-multicore-rtos.dts
> create mode 100644 arch/arm64/boot/dts/freescale/imx93-rproc-ca55.dtsi
>
>--
>2.43.0
>
^ permalink raw reply
* Re: [PATCH 1/2] [RFC] debugobjects: avoid gcc-16.0.1 section mismatch
From: Arnd Bergmann @ 2026-05-11 6:17 UTC (permalink / raw)
To: Thomas Gleixner, Arnd Bergmann, Will Deacon, Robin Murphy,
Joerg Roedel, Andrew Morton
Cc: linux-arm-kernel, iommu, linux-kernel, Sebastian Andrzej Siewior
In-Reply-To: <874ikf9i34.ffs@tglx>
On Sun, May 10, 2026, at 21:31, Thomas Gleixner wrote:
> On Tue, Feb 03 2026 at 17:23, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> gcc-16 has gained some more advanced inlining techniques that enable
>> it to inline the is_static_object() function pointer into a specialized
>> version of lookup_object_or_alloc:
>>
>> WARNING: modpost: vmlinux: section mismatch in reference: lookup_object_or_alloc.part.0+0x1ac (section: .text) -> is_static_object (section: .init.text)
>>
>> From what I can tell, the transformation is correct, as this
>> is only called when lookup_object_or_alloc() is called from
>> debug_objects_selftest(), which is also __init.
>
> So clearly the compiler is buggy. It creates an __init specific copy of
> lookup_object_or_alloc() and then fails to attribute it correctly.
I don't see what else the compiler is supposed to do, it has no idea what
__init means in the kernel, or what the rules are for calling between
that and normal functions. Putting a non-inlined lookup_object_or_alloc()
into a special section without an explicit attribute would clearly
be a bug.
>> I have not come up with a good workaround, so this simply marks
>> is_static_object() as not __init. Since there are currently only two
>> files where this happens, that may be an easy way out.
>
> That's a horrible hack and while it's only two files today, this sounds
> like the start of a whack a mole game.
After thousands of randconfig builds, I found exactly the two
instances from this series.
Since this only happens in a very specific case where a file
uses function pointers to local functions and gcc is able
to turn these into direct calls, I wouldn't expect this to
become a widespread problem. It's normal for new compiler
versions to run into some corner cases like this when inlining
decisions change.
>> If anyone has a better idea for how to deal with that, let me know!
>
> Mark the compiler broken and wait until GCC people get their act
> together.
I'll have to retest with the actual release compiler, maybe they
changed something again that makes it go away already, otherwise
there is probably a flag to tell gcc to skip that optimization pass.
Arnd
^ permalink raw reply
* [PATCH v3 4/7] PCI: dwc: Use common pci_host_common_link_train_delay() helper
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
In-Reply-To: <20260511055923.37117-1-18255117159@163.com>
The DWC driver already implements the 100 ms delay required by PCIe
r6.0 sec 6.6.1 by checking pci->max_link_speed and calling msleep(100).
Replace the open-coded msleep() with the new common helper
pci_host_common_link_train_delay() to reduce code duplication and
improve maintainability. No functional change intended.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/dwc/pcie-designware.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index c11cf61b8319..7021d21bb601 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -22,6 +22,7 @@
#include <linux/sizes.h>
#include <linux/types.h>
+#include "../pci-host-common.h"
#include "../../pci.h"
#include "pcie-designware.h"
@@ -799,13 +800,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
return -ETIMEDOUT;
}
- /*
- * As per PCIe r6.0, sec 6.6.1, a Downstream Port that supports Link
- * speeds greater than 5.0 GT/s, software must wait a minimum of 100 ms
- * after Link training completes before sending a Configuration Request.
- */
- if (pci->max_link_speed > 2)
- msleep(PCIE_RESET_CONFIG_WAIT_MS);
+ pci_host_common_link_train_delay(pci->max_link_speed);
offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
--
2.34.1
^ permalink raw reply related
* [PATCH v3 5/7] PCI: aardvark: Add 100 ms delay after link training
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
In-Reply-To: <20260511055923.37117-1-18255117159@163.com>
The Aardvark PCIe controller driver waits for the link to come up but
does not implement the mandatory 100 ms delay after link training
completes for speeds greater than 5.0 GT/s (PCIe r6.0 sec 6.6.1).
The driver already maintains a 'link_gen' field that holds the negotiated
link speed. Use it together with pci_host_common_link_train_delay() to
insert the required delay immediately after confirming that the link
is up.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/pci-aardvark.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e34bea1ff0ac..fd9c7d53e8a7 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -26,6 +26,7 @@
#include <linux/of_address.h>
#include <linux/of_pci.h>
+#include "pci-host-common.h"
#include "../pci.h"
#include "../pci-bridge-emul.h"
@@ -350,8 +351,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
/* check if the link is up or not */
for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
- if (advk_pcie_link_up(pcie))
+ if (advk_pcie_link_up(pcie)) {
+ pci_host_common_link_train_delay(pcie->link_gen);
return 0;
+ }
usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
}
--
2.34.1
^ permalink raw reply related
* [PATCH v3 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
In-Reply-To: <20260511055923.37117-1-18255117159@163.com>
The Cadence LGA (Legacy Architecture IP) PCIe host controller currently
lacks the mandatory 100 ms delay after link training completes for speeds
> 5.0 GT/s, as required by PCIe r6.0 sec 6.6.1.
Add a 'max_link_speed' field to struct cdns_pcie. In the common host
layer function cdns_pcie_host_start_link(), after the link has been
successfully established, call pci_host_common_link_train_delay() to
insert the required delay.
For the j721e glue driver, set cdns_pcie.max_link_speed from the existing
link speed logic. For other LGA-based glue drivers (sky1, sg2042), the
common LGA host setup (pcie-cadence-host.c) provides a fallback reading
of the device tree property "max-link-speed" when available. This ensures
that the delay is not missed on those platforms once they enable the
property.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/cadence/pci-j721e.c | 1 +
drivers/pci/controller/cadence/pcie-cadence-host-common.c | 4 ++++
drivers/pci/controller/cadence/pcie-cadence-host.c | 4 ++++
drivers/pci/controller/cadence/pcie-cadence.h | 2 ++
4 files changed, 11 insertions(+)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index bfdfe98d5aba..ee85b8e04f5b 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -206,6 +206,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
(pcie_get_link_speed(link_speed) == PCI_SPEED_UNKNOWN))
link_speed = 2;
+ pcie->cdns_pcie.max_link_speed = link_speed;
val = link_speed - 1;
ret = regmap_update_bits(syscon, offset, GENERATION_SEL_MASK, val);
if (ret)
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
index 2b0211870f02..18e4b6c760b5 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
@@ -14,6 +14,7 @@
#include "pcie-cadence.h"
#include "pcie-cadence-host-common.h"
+#include "../pci-host-common.h"
#define LINK_RETRAIN_TIMEOUT HZ
@@ -115,6 +116,9 @@ int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc,
if (!ret && rc->quirk_retrain_flag)
ret = cdns_pcie_retrain(pcie, pcie_link_up);
+ if (!ret)
+ pci_host_common_link_train_delay(pcie->max_link_speed);
+
return ret;
}
EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 0bc9e6e90e0e..058e4e619654 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -13,6 +13,7 @@
#include "pcie-cadence.h"
#include "pcie-cadence-host-common.h"
+#include "../../pci.h"
static u8 bar_aperture_mask[] = {
[RP_BAR0] = 0x1F,
@@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
rc->device_id = 0xffff;
of_property_read_u32(np, "device-id", &rc->device_id);
+ if (pcie->max_link_speed < 1)
+ pcie->max_link_speed = of_pci_get_max_link_speed(np);
+
pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
if (IS_ERR(pcie->reg_base)) {
dev_err(dev, "missing \"reg\"\n");
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 574e9cf4d003..042a4c49bb9a 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
* @ops: Platform-specific ops to control various inputs from Cadence PCIe
* wrapper
* @cdns_pcie_reg_offsets: Register bank offsets for different SoC
+ * @max_link_speed: Maximum supported link speed
*/
struct cdns_pcie {
void __iomem *reg_base;
@@ -98,6 +99,7 @@ struct cdns_pcie {
struct device_link **link;
const struct cdns_pcie_ops *ops;
const struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
+ int max_link_speed;
};
/**
--
2.34.1
^ permalink raw reply related
* [PATCH v3 3/7] PCI: cadence: HPA: Add post-link delay
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
In-Reply-To: <20260511055923.37117-1-18255117159@163.com>
The Cadence HPA (High Performance Architecture IP) specific link setup
function cdns_pcie_hpa_host_link_setup() waits for the link to come up
but does not implement the required 100 ms delay after link training
completes for speeds > 5.0 GT/s (PCIe r6.0 sec 6.6.1).
Add a call to pci_host_common_link_train_delay() immediately after the
link is confirmed to be up, using the max_link_speed field. Also, in the
HPA host setup function, read the device tree property "max-link-speed"
to initialize max_link_speed if not already set by a glue driver.
This ensures compliance for HPA-based platforms.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/cadence/pcie-cadence-host-hpa.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
index 0f540bed58e8..8ef58ed01daa 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
@@ -15,6 +15,8 @@
#include "pcie-cadence.h"
#include "pcie-cadence-host-common.h"
+#include "../pci-host-common.h"
+#include "../../pci.h"
static u8 bar_aperture_mask[] = {
[RP_BAR0] = 0x3F,
@@ -304,6 +306,8 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc *rc)
ret = cdns_pcie_host_wait_for_link(pcie, cdns_pcie_hpa_link_up);
if (ret)
dev_dbg(dev, "PCIe link never came up\n");
+ else
+ pci_host_common_link_train_delay(pcie->max_link_speed);
return ret;
}
@@ -313,6 +317,7 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
{
struct device *dev = rc->pcie.dev;
struct platform_device *pdev = to_platform_device(dev);
+ struct device_node *np = dev->of_node;
struct pci_host_bridge *bridge;
enum cdns_pcie_rp_bar bar;
struct cdns_pcie *pcie;
@@ -343,6 +348,9 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
rc->cfg_res = res;
}
+ if (pcie->max_link_speed < 1)
+ pcie->max_link_speed = of_pci_get_max_link_speed(np);
+
/* Put EROM Bar aperture to 0 */
cdns_pcie_hpa_writel(pcie, REG_BANK_IP_CFG_CTRL_REG, CDNS_PCIE_EROM, 0x0);
--
2.34.1
^ permalink raw reply related
* [PATCH v3 7/7] PCI: rzg3s-host: Use common pci_host_common_link_train_delay() helper
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
In-Reply-To: <20260511055923.37117-1-18255117159@163.com>
Replace the unconditional msleep(100) with the common helper
pci_host_common_link_train_delay(). The helper only waits when
max_link_speed > 2, as required by PCIe r6.0 sec 6.6.1.
This avoids unnecessary delay for Gen1/Gen2 links while retaining
the mandatory 100 ms for higher speeds.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/pcie-rzg3s-host.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index d86e7516dcc2..66f687304c1c 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -35,6 +35,7 @@
#include <linux/slab.h>
#include <linux/units.h>
+#include "pci-host-common.h"
#include "../pci.h"
/* AXI registers */
@@ -1663,7 +1664,7 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
if (ret)
dev_info(dev, "Failed to set max link speed\n");
- msleep(PCIE_RESET_CONFIG_WAIT_MS);
+ pci_host_common_link_train_delay(host->max_link_speed);
return 0;
--
2.34.1
^ permalink raw reply related
* [PATCH v3 6/7] PCI: mediatek-gen3: Add 100 ms delay after link up
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
In-Reply-To: <20260511055923.37117-1-18255117159@163.com>
The MediaTek Gen3 PCIe host driver lacks the required 100 ms delay after
link training completes for speeds > 5.0 GT/s, as specified in PCIe r6.0
sec 6.6.1.
The driver already stores max_link_speed (from the device tree). After
mtk_pcie_startup_port() successfully brings up the link, call
pci_host_common_link_train_delay() to comply with the specification.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/pcie-mediatek-gen3.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index b0accd828589..5abddec4e9be 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -30,6 +30,7 @@
#include <linux/regmap.h>
#include <linux/reset.h>
+#include "pci-host-common.h"
#include "../pci.h"
#define PCIE_BASE_CFG_REG 0x14
@@ -570,6 +571,8 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
goto err_power_down_device;
}
+ pci_host_common_link_train_delay(pcie->max_link_speed);
+
return 0;
err_power_down_device:
--
2.34.1
^ permalink raw reply related
* [PATCH v3 0/7] PCI: Add common helper for 100 ms delay after link training
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
PCIe r6.0, sec 6.6.1 (Conventional Reset) requires that for a Downstream
Port supporting Link speeds greater than 5.0 GT/s, software must wait a
minimum of 100 ms after Link training completes before sending any
Configuration Request.
Several PCIe host controller drivers currently omit this 100 ms delay
when the negotiated link speed is Gen3 (8 GT/s) or higher. Only the DWC
driver already implements it. The missing delay can lead to violations
of the PCIe specification and cause enumeration failures with high-speed
devices (e.g., NVIDIA RTX5070 GPU, PCIe 5.0 NVMe SSDs).
To fix this consistently and avoid code duplication, this series:
1. Adds a static inline helper `pci_host_common_link_train_delay()`
in `drivers/pci/controller/pci-host-common.h`. The helper checks
the given max_link_speed and calls msleep(100) if speed > 5 GT/s.
2. Converts the DWC driver to use this helper.
3. Adds the missing 100 ms delay to the Cadence PCIe controller
(both LGA and HPA IPs). A `max_link_speed` field is introduced
in `struct cdns_pcie`. The j721e glue driver sets this field;
other LGA glue drivers fall back to reading DT property
"max-link-speed". HPA also reads the DT property.
4. Adds the delay to the Aardvark and MediaTek Gen3 host drivers,
and replaces the existing unconditional delay in the Renesas
RZ/G3S driver with the conditional helper (per spec).
All changes are placed immediately after link training completes and
before any Configuration Request would be issued.
---
Our company's product is based on the HPA IP from Cadence. When connecting
to different devices, we encountered issues with the enumeration failure
when connecting to the NVIDIA RTX5070 GPU and the NVMe SSD with PCIe 5.0
interface. Our code is based on: 80dc18a0cba8d ("PCI: dwc: Ensure that
dw_pcie_wait_for_link() waits 100 ms after link up").
---
Changes since v3:
- Renamed helper from pcie_wait_after_link_train() to
pci_host_common_link_train_delay() and moved to pci-host-common.h. (Mani)
- Reorganized Cadence patches: LGA + j721e glue in one patch, HPA separate.
- Added missing DT property reading for HPA and LGA fallback.
- Replace the existing msleep(100) call with the common helper
pci_host_common_link_train_delay(). (Claudiu)
Changes since v2:
- Add pcie_wait_after_link_train() helper
- Reduce repetitive code comments and have each Root Port driver use the
helper function instead.
- Increase the delay to 100ms after enabling the link-up that distinguishes
between Cadence LGA and HPA IPs.
- Add the Aardvark, MediaTek Gen3, and Renesas RZ/G3S Root Port driver. When
the speed is greater than GEN2, a delay of 100ms should be applied.
v2:
https://patchwork.kernel.org/project/linux-pci/cover/20260506152346.166056-1-18255117159@163.com/
v1:
https://patchwork.kernel.org/project/linux-pci/patch/20260501153553.66382-1-18255117159@163.com/
---
Hans Zhang (7):
PCI: Add pci_host_common_link_train_delay() helper
PCI: cadence: Add post-link delay for LGA and j721e glue driver
PCI: cadence: HPA: Add post-link delay
PCI: dwc: Use common pci_host_common_link_train_delay() helper
PCI: aardvark: Add 100 ms delay after link training
PCI: mediatek-gen3: Add 100 ms delay after link up
PCI: rzg3s-host: Use common pci_host_common_link_train_delay() helper
drivers/pci/controller/cadence/pci-j721e.c | 1 +
.../cadence/pcie-cadence-host-common.c | 4 ++++
.../controller/cadence/pcie-cadence-host-hpa.c | 8 ++++++++
.../pci/controller/cadence/pcie-cadence-host.c | 4 ++++
drivers/pci/controller/cadence/pcie-cadence.h | 2 ++
drivers/pci/controller/dwc/pcie-designware.c | 9 ++-------
drivers/pci/controller/pci-aardvark.c | 5 ++++-
drivers/pci/controller/pci-host-common.h | 17 +++++++++++++++++
drivers/pci/controller/pcie-mediatek-gen3.c | 3 +++
drivers/pci/controller/pcie-rzg3s-host.c | 3 ++-
10 files changed, 47 insertions(+), 9 deletions(-)
base-commit: 70390501d1944d4e5b8f7352be180fceb3a44132
--
2.34.1
^ permalink raw reply
* [PATCH v3 1/7] PCI: Add pci_host_common_link_train_delay() helper
From: Hans Zhang @ 2026-05-11 5:59 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, claudiu.beznea,
linux-mediatek, linux-renesas-soc, linux-pci, linux-kernel,
Hans Zhang
In-Reply-To: <20260511055923.37117-1-18255117159@163.com>
PCIe r6.0, sec 6.6.1 (Conventional Reset) requires that for a Downstream
Port supporting Link speeds greater than 5.0 GT/s, software must wait a
minimum of 100 ms after Link training completes before sending any
Configuration Request.
Introduce a static inline helper pci_host_common_link_train_delay() that
checks the given max_link_speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, etc.) and
calls msleep(100) only when the speed is greater than 5.0 GT/s.
This allows multiple host controller drivers to share the same mandatory
delay without duplicating the logic.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/pci-host-common.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
index b5075d4bd7eb..d709f7e3e11a 100644
--- a/drivers/pci/controller/pci-host-common.h
+++ b/drivers/pci/controller/pci-host-common.h
@@ -10,6 +10,9 @@
#ifndef _PCI_HOST_COMMON_H
#define _PCI_HOST_COMMON_H
+#include <linux/delay.h>
+#include "../pci.h"
+
struct pci_ecam_ops;
int pci_host_common_probe(struct platform_device *pdev);
@@ -20,4 +23,18 @@ void pci_host_common_remove(struct platform_device *pdev);
struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
+
+/**
+ * pci_host_common_link_train_delay - Wait 100 ms if link speed > 5 GT/s
+ * @max_link_speed: the maximum link speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, ...)
+ *
+ * Must be called after Link training completes and before the first
+ * Configuration Request is sent.
+ */
+static inline void pci_host_common_link_train_delay(int max_link_speed)
+{
+ if (max_link_speed > 2)
+ msleep(PCIE_RESET_CONFIG_WAIT_MS);
+}
+
#endif
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support
From: Saravana Kannan @ 2026-05-11 5:09 UTC (permalink / raw)
To: Ulf Hansson
Cc: Danilo Krummrich, Saravana Kannan, Rafael J . Wysocki,
Greg Kroah-Hartman, driver-core, linux-pm, Sudeep Holla,
Cristian Marussi, Kevin Hilman, Stephen Boyd, Marek Szyprowski,
Bjorn Andersson, Abel Vesa, Peng Fan, Tomi Valkeinen, Maulik Shah,
Konrad Dybcio, Thierry Reding, Jonathan Hunter,
Geert Uytterhoeven, Dmitry Baryshkov, linux-arm-kernel,
linux-kernel
In-Reply-To: <20260508123910.114273-7-ulf.hansson@linaro.org>
On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> A onecell (#power-domain-cells = <1 or 2>; in DT) power domain provider
> typically provides multiple independent power domains, each with their own
> corresponding consumers. In these cases we have to wait for all consumers
> for all the provided power domains before the ->sync_state() callback gets
> called for the supplier.
>
> In a first step to improve this, let's implement support for fine grained
> sync_state support a per genpd basis by using the ->queue_sync_state()
> callback. To take step by step, let's initially limit the improvement to
> the internal genpd provider driver and to its corresponding genpd devices
> for onecell providers.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v3:
> - Addressed some cosmetic comments from Geert.
>
> ---
> drivers/pmdomain/core.c | 124 ++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 1 +
> 2 files changed, 125 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index ad57846f02a3..c01a9a96e5c2 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2699,6 +2699,119 @@ static struct generic_pm_domain *genpd_get_from_provider(
> return genpd;
> }
>
> +static bool genpd_should_wait_for_consumer(struct device_node *np)
> +{
> + struct generic_pm_domain *genpd;
> + bool should_wait = false;
> +
> + mutex_lock(&gpd_list_lock);
> + list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> + if (genpd->provider == of_fwnode_handle(np)) {
> + genpd_lock(genpd);
> +
> + /* Clear the previous state before reevaluating. */
> + genpd->wait_for_consumer = false;
> +
> + /*
> + * Unless there is at least one genpd for the provider
> + * that is being kept powered-on, we don't have to care
> + * about waiting for consumers.
> + */
> + if (genpd->stay_on)
> + should_wait = true;
> +
> + genpd_unlock(genpd);
> + }
> + }
> + mutex_unlock(&gpd_list_lock);
I think I understand the intent of this function, but haven't dug into
genpd code enough to comment on this yet. I'll come back to this
later.
> +
> + return should_wait;
> +}
> +
> +static void genpd_parse_for_consumer(struct device_node *sup,
> + struct device_node *con)
> +{
> + struct generic_pm_domain *genpd;
> +
> + for (unsigned int i = 0; ; i++) {
> + struct of_phandle_args pd_args;
> +
> + if (of_parse_phandle_with_args(con, "power-domains",
> + "#power-domain-cells",
> + i, &pd_args))
> + break;
Why not use a while or a do while() instead of this infinite for loop
with a break?
> +
> + /*
> + * The phandle must correspond to the supplier's genpd provider
> + * to be relevant else let's move to the next index.
> + */
> + if (sup != pd_args.np) {
> + of_node_put(pd_args.np);
> + continue;
> + }
> +
> + mutex_lock(&gpd_list_lock);
> + genpd = genpd_get_from_provider(&pd_args);
> + if (!IS_ERR(genpd)) {
> + genpd_lock(genpd);
> + genpd->wait_for_consumer = true;
> + genpd_unlock(genpd);
> + }
> + mutex_unlock(&gpd_list_lock);
> +
> + of_node_put(pd_args.np);
> + }
> +}
> +
> +static void _genpd_queue_sync_state(struct device_node *np)
> +{
> + struct generic_pm_domain *genpd;
> +
> + mutex_lock(&gpd_list_lock);
> + list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> + if (genpd->provider == of_fwnode_handle(np)) {
> + genpd_lock(genpd);
> + if (genpd->stay_on && !genpd->wait_for_consumer) {
> + genpd->stay_on = false;
> + genpd_queue_power_off_work(genpd);
> + }
> + genpd_unlock(genpd);
> + }
> + }
> + mutex_unlock(&gpd_list_lock);
> +}
> +
> +static void genpd_queue_sync_state(struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + struct device_link *link;
> +
> + if (!genpd_should_wait_for_consumer(np))
> + return;
> +
> + list_for_each_entry(link, &dev->links.consumers, s_node) {
Couple of issues:
1. I don't want the frameworks to be so deeply aware of driver core
internals. I want the driver core maintainers to be able to change the
devlink implementation without having to worry about going and fixing
all the frameworks. So, please add a for_each_consumer_dev(supplier,
callback) and for_each_supplier_dev(consumer, callback) helper
functions.
2. This doesn't ignore "SYNC_STATE_ONLY" links and that's going to
confuse the consumer count/check you might do or at the least waste
parsing those.
3. **Device** links are not the complete list of consumers because
they can only link consumer **devices** once the consumer **device**
is created.
4. What you really need is a for_each_consumer_fwnode(supplier,
callback) that first loops through all the consumer device links and
calls the callback() on their fwnode and then the same function needs
to loop through all the fwnode links and then pass those consumer
fwnodes to the callback. And inside that callback you can do whatever
you want.
5. You might want to add a for_each_inactive_consumer(supplier,
callback) too to simplify your need for checking if a fwnode has a
device and then checking if it's probed.
Thanks,
Saravana
> + struct device *consumer = link->consumer;
> +
> + if (!device_link_test(link, DL_FLAG_MANAGED))
> + continue;
> +
> + if (link->status == DL_STATE_ACTIVE)
> + continue;
> +
> + if (!consumer->of_node)
> + continue;
> +
> + /*
> + * A consumer device has not been probed yet. Let's parse its
> + * device node for the power-domains property, to find out the
> + * genpds it may belong to and then prevent sync state for them.
> + */
> + genpd_parse_for_consumer(np, consumer->of_node);
> + }
> +
> + _genpd_queue_sync_state(np);
> +}
> +
> static void genpd_sync_state(struct device *dev)
> {
> return of_genpd_sync_state(dev->of_node);
> @@ -3531,6 +3644,16 @@ static int genpd_provider_probe(struct device *dev)
> return 0;
> }
>
> +static void genpd_provider_queue_sync_state(struct device *dev)
> +{
> + struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
> +
> + if (genpd->sync_state != GENPD_SYNC_STATE_ONECELL)
> + return;
> +
> + genpd_queue_sync_state(dev);
> +}
> +
> static void genpd_provider_sync_state(struct device *dev)
> {
> struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
> @@ -3559,6 +3682,7 @@ static struct device_driver genpd_provider_drv = {
> .name = "genpd_provider",
> .bus = &genpd_provider_bus_type,
> .probe = genpd_provider_probe,
> + .queue_sync_state = genpd_provider_queue_sync_state,
> .sync_state = genpd_provider_sync_state,
> .suppress_bind_attrs = true,
> };
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b299dc0128d6..7aa49721cde5 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -215,6 +215,7 @@ struct generic_pm_domain {
> cpumask_var_t cpus; /* A cpumask of the attached CPUs */
> bool synced_poweroff; /* A consumer needs a synced poweroff */
> bool stay_on; /* Stay powered-on during boot. */
> + bool wait_for_consumer; /* Consumers awaits to be probed. */
> enum genpd_sync_state sync_state; /* How sync_state is managed. */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support
From: Saravana Kannan @ 2026-05-11 5:08 UTC (permalink / raw)
To: Ulf Hansson
Cc: Danilo Krummrich, Saravana Kannan, Rafael J . Wysocki,
Greg Kroah-Hartman, driver-core, linux-pm, Sudeep Holla,
Cristian Marussi, Kevin Hilman, Stephen Boyd, Marek Szyprowski,
Bjorn Andersson, Abel Vesa, Peng Fan, Tomi Valkeinen, Maulik Shah,
Konrad Dybcio, Thierry Reding, Jonathan Hunter,
Geert Uytterhoeven, Dmitry Baryshkov, linux-arm-kernel,
linux-kernel
In-Reply-To: <20260508123910.114273-3-ulf.hansson@linaro.org>
On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The common sync_state support isn't fine grained enough for some types of
> suppliers, like power domains for example. Especially when a supplier
> provides multiple independent power domains, each with their own set of
> consumers. In these cases we need to wait for all consumers for all the
> provided power domains before invoking the supplier's ->sync_state().
>
> To allow a more fine grained sync_state support to be implemented on per
> supplier's driver basis, let's add a new optional callback. As soon as
> there is an update worth to consider in regards to managing sync_state for
> a supplier device, __device_links_queue_sync_state() queues the device in a
> list, allowing the new callback to be invoked when flushing the list in
> device_links_flush_sync_list().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v3:
> - Re-worked the approach to use a list to queue/flush devices for
> ->queue_sync_state(). This should make sure the device lock is being
> held when it's needed, as pointed out by Danilo.
>
Hi Ulf,
Thanks for working on this!
And please bear with my slow replies.
> ---
> drivers/base/base.h | 18 ++++++++
> drivers/base/core.c | 77 ++++++++++++++++++++++++++---------
> drivers/base/driver.c | 7 ++++
> include/linux/device.h | 2 +
> include/linux/device/driver.h | 7 ++++
> 5 files changed, 91 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 30b416588617..c8be24af92c3 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -196,6 +196,24 @@ static inline void dev_sync_state(struct device *dev)
> dev->driver->sync_state(dev);
> }
>
> +static inline bool dev_has_queue_sync_state(struct device *dev)
Let's please pick a better name. This is too similar to the actual
queue function you call __device_links_queue_sync_state() and is very
confusing. Maybe something that has a meaning along the lines of
"another consumer probed". So, maybe:
* consumer_probed()
* change_of_active_consumers()
I'm going to refer to this new callback as "consumer_probed()" in the
rest of my replies so I can keep it straight in my head.
> +{
> + struct device_driver *drv;
> +
> + if (!dev)
> + return false;
> + drv = READ_ONCE(dev->driver);
> + if (drv && drv->queue_sync_state)
> + return true;
> + return false;
> +}
> +
> +static inline void dev_queue_sync_state(struct device *dev)
> +{
> + if (dev->driver && dev->driver->queue_sync_state)
> + dev->driver->queue_sync_state(dev);
> +}
> +
> int driver_add_groups(const struct device_driver *drv, const struct attribute_group **groups);
> void driver_remove_groups(const struct device_driver *drv, const struct attribute_group **groups);
> void device_driver_detach(struct device *dev);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d49420e066de..f1f95b3c81e5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1101,15 +1101,18 @@ int device_links_check_suppliers(struct device *dev)
> /**
> * __device_links_queue_sync_state - Queue a device for sync_state() callback
> * @dev: Device to call sync_state() on
> - * @list: List head to queue the @dev on
> + * @s_list: List head for the sync_state to queue the @dev on
> + * @q_list: List head for the queue_sync_state to queue the @dev on
> *
> * Queues a device for a sync_state() callback when the device links write lock
> * isn't held. This allows the sync_state() execution flow to use device links
> * APIs. The caller must ensure this function is called with
> - * device_links_write_lock() held.
> + * device_links_write_lock() held. Note, if the optional queue_sync_state()
> + * callback has been assigned too, the device is queued for that list to allow a
> + * more fine grained support to be implemented on per supplier basis.
Why not keep this in the same format as the existing doc? Something like:
If option list xxx is provided, queues the device for the
consumer_probed() callback.
> *
> * This function does a get_device() to make sure the device is not freed while
> - * on this list.
> + * on the corresponding list.
> *
> * So the caller must also ensure that device_links_flush_sync_list() is called
> * as soon as the caller releases device_links_write_lock(). This is necessary
> @@ -1117,7 +1120,8 @@ int device_links_check_suppliers(struct device *dev)
> * put_device() is called on this device.
> */
> static void __device_links_queue_sync_state(struct device *dev,
> - struct list_head *list)
> + struct list_head *s_list,
> + struct list_head *q_list)
> {
> struct device_link *link;
>
> @@ -1129,8 +1133,14 @@ static void __device_links_queue_sync_state(struct device *dev,
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> if (!device_link_test(link, DL_FLAG_MANAGED))
> continue;
> - if (link->status != DL_STATE_ACTIVE)
> + if (link->status != DL_STATE_ACTIVE) {
> + if (dev_has_queue_sync_state(dev) &&
> + list_empty(&dev->links.queue_sync)) {
> + get_device(dev);
> + list_add_tail(&dev->links.queue_sync, q_list);
> + }
> return;
> + }
> }
>
> /*
> @@ -1144,25 +1154,28 @@ static void __device_links_queue_sync_state(struct device *dev,
> return;
>
> get_device(dev);
> - list_add_tail(&dev->links.defer_sync, list);
> + list_add_tail(&dev->links.defer_sync, s_list);
> }
>
> /**
> - * device_links_flush_sync_list - Call sync_state() on a list of devices
> - * @list: List of devices to call sync_state() on
> + * device_links_flush_sync_list - Call sync_state callbacks for the devices
> + * @s_list: List of devices to call sync_state() on
> + * @q_list: List of devices to call queue_sync_state() on
> * @dont_lock_dev: Device for which lock is already held by the caller
> *
> - * Calls sync_state() on all the devices that have been queued for it. This
> - * function is used in conjunction with __device_links_queue_sync_state(). The
> - * @dont_lock_dev parameter is useful when this function is called from a
> - * context where a device lock is already held.
> + * Calls sync_state() and queue_sync_state() on all the devices that have been
> + * queued for it. This function is used in conjunction with
> + * __device_links_queue_sync_state(). The @dont_lock_dev parameter is useful
> + * when this function is called from a context where a device lock is already
> + * held.
> */
> -static void device_links_flush_sync_list(struct list_head *list,
> +static void device_links_flush_sync_list(struct list_head *s_list,
> + struct list_head *q_list,
> struct device *dont_lock_dev)
> {
> struct device *dev, *tmp;
>
> - list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
> + list_for_each_entry_safe(dev, tmp, s_list, links.defer_sync) {
> list_del_init(&dev->links.defer_sync);
>
> if (dev != dont_lock_dev)
> @@ -1175,6 +1188,25 @@ static void device_links_flush_sync_list(struct list_head *list,
>
> put_device(dev);
> }
> +
> + if (!q_list)
> + return;
> +
> + list_for_each_entry_safe(dev, tmp, q_list, links.queue_sync) {
> + list_del_init(&dev->links.queue_sync);
> +
> + if (dev != dont_lock_dev)
> + device_lock(dev);
> +
> + device_links_write_lock();
> + dev_queue_sync_state(dev);
> + device_links_write_unlock();
> +
> + if (dev != dont_lock_dev)
> + device_unlock(dev);
> +
> + put_device(dev);
> + }
> }
>
> void device_links_supplier_sync_state_pause(void)
> @@ -1188,6 +1220,7 @@ void device_links_supplier_sync_state_resume(void)
> {
> struct device *dev, *tmp;
> LIST_HEAD(sync_list);
> + LIST_HEAD(queue_list);
>
> device_links_write_lock();
> if (!defer_sync_state_count) {
> @@ -1204,12 +1237,12 @@ void device_links_supplier_sync_state_resume(void)
> * sync_list because defer_sync is used for both lists.
> */
> list_del_init(&dev->links.defer_sync);
> - __device_links_queue_sync_state(dev, &sync_list);
> + __device_links_queue_sync_state(dev, &sync_list, &queue_list);
> }
> out:
> device_links_write_unlock();
>
> - device_links_flush_sync_list(&sync_list, NULL);
> + device_links_flush_sync_list(&sync_list, &queue_list, NULL);
> }
>
> static int sync_state_resume_initcall(void)
> @@ -1296,6 +1329,7 @@ void device_links_driver_bound(struct device *dev)
> {
> struct device_link *link, *ln;
> LIST_HEAD(sync_list);
> + LIST_HEAD(queue_list);
>
> /*
> * If a device binds successfully, it's expected to have created all
> @@ -1351,7 +1385,7 @@ void device_links_driver_bound(struct device *dev)
> if (defer_sync_state_count)
> __device_links_supplier_defer_sync(dev);
> else
> - __device_links_queue_sync_state(dev, &sync_list);
> + __device_links_queue_sync_state(dev, &sync_list, &queue_list);
This path is really meant only for situations when the device has no
consumers. So, I don't think fine grained control is relevant.
>
> list_for_each_entry_safe(link, ln, &dev->links.suppliers, c_node) {
> struct device *supplier;
> @@ -1393,14 +1427,15 @@ void device_links_driver_bound(struct device *dev)
> if (defer_sync_state_count)
> __device_links_supplier_defer_sync(supplier);
> else
> - __device_links_queue_sync_state(supplier, &sync_list);
> + __device_links_queue_sync_state(supplier, &sync_list,
> + &queue_list);
> }
>
> dev->links.status = DL_DEV_DRIVER_BOUND;
>
> device_links_write_unlock();
>
> - device_links_flush_sync_list(&sync_list, dev);
> + device_links_flush_sync_list(&sync_list, &queue_list, dev);
> }
>
> /**
> @@ -1516,6 +1551,7 @@ void device_links_driver_cleanup(struct device *dev)
> }
>
> list_del_init(&dev->links.defer_sync);
> + list_del_init(&dev->links.queue_sync);
> __device_links_no_driver(dev);
>
> device_links_write_unlock();
> @@ -1808,7 +1844,7 @@ void fw_devlink_probing_done(void)
> class_for_each_device(&devlink_class, NULL, &sync_list,
> fw_devlink_dev_sync_state);
> device_links_write_unlock();
> - device_links_flush_sync_list(&sync_list, NULL);
> + device_links_flush_sync_list(&sync_list, NULL, NULL);
> }
>
> /**
> @@ -3169,6 +3205,7 @@ void device_initialize(struct device *dev)
> INIT_LIST_HEAD(&dev->links.consumers);
> INIT_LIST_HEAD(&dev->links.suppliers);
> INIT_LIST_HEAD(&dev->links.defer_sync);
> + INIT_LIST_HEAD(&dev->links.queue_sync);
> dev->links.status = DL_DEV_NO_DRIVER;
> dev_assign_dma_coherent(dev, dma_default_coherent);
> swiotlb_dev_init(dev);
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 8ab010ddf709..b8f4d08bbd58 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -239,6 +239,13 @@ int driver_register(struct device_driver *drv)
> pr_warn("Driver '%s' needs updating - please use "
> "bus_type methods\n", drv->name);
>
> + if (drv->queue_sync_state && !drv->sync_state &&
> + !drv->bus->sync_state) {
> + pr_err("Driver '%s' or its bus_type needs ->sync_state()",
> + drv->name);
> + return -EINVAL;
> + }
> +
> other = driver_find(drv->name, drv->bus);
> if (other) {
> pr_err("Error: Driver '%s' is already registered, "
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 56a96e41d2c9..6848b0a2c2d9 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -414,12 +414,14 @@ enum device_removable {
> * @suppliers: List of links to supplier devices.
> * @consumers: List of links to consumer devices.
> * @defer_sync: Hook to global list of devices that have deferred sync_state.
> + * @defer_sync: Hook to global list of devices scheduled for queue_sync_state.
> * @status: Driver status information.
> */
> struct dev_links_info {
> struct list_head suppliers;
> struct list_head consumers;
> struct list_head defer_sync;
> + struct list_head queue_sync;
> enum dl_dev_state status;
> };
>
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index bbc67ec513ed..bc9ae1cbe03c 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -68,6 +68,12 @@ enum probe_type {
> * be called at late_initcall_sync level. If the device has
> * consumers that are never bound to a driver, this function
> * will never get called until they do.
> + * @queue_sync_state: Similar to the ->sync_state() callback, but called to
> + * allow syncing device state to software state in a more fine
> + * grained way. It is called when there is an updated state that
> + * may be worth to consider for any of the consumers linked to
> + * this device. If implemented, the ->sync_state() callback is
> + * required too.
Let's make it clear that if a device gets sync_state() callback, it
needs to sync the state independent of whether it has received any/all
"consumer_probed()".
Thanks,
Saravana
> * @remove: Called when the device is removed from the system to
> * unbind a device from this driver.
> * @shutdown: Called at shut-down time to quiesce the device.
> @@ -110,6 +116,7 @@ struct device_driver {
>
> int (*probe) (struct device *dev);
> void (*sync_state)(struct device *dev);
> + void (*queue_sync_state)(struct device *dev);
> int (*remove) (struct device *dev);
> void (*shutdown) (struct device *dev);
> int (*suspend) (struct device *dev, pm_message_t state);
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v13 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
From: Padhi, Beleswar @ 2026-05-11 4:58 UTC (permalink / raw)
To: Mathieu Poirier, Arnaud POULIQUEN
Cc: Shenwei Wang, Andrew Lunn, Linus Walleij, Bartosz Golaszewski,
Jonathan Corbet, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Frank Li, Sascha Hauer, Shuah Khan,
linux-gpio@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, Pengutronix Kernel Team,
Fabio Estevam, Peng Fan, devicetree@vger.kernel.org,
linux-remoteproc@vger.kernel.org, imx@lists.linux.dev,
linux-arm-kernel@lists.infradead.org, dl-linux-imx,
Bartosz Golaszewski
In-Reply-To: <afzIABSh1xtMEGbf@p14s>
Hi Mathieu,
On 5/7/2026 10:42 PM, Mathieu Poirier wrote:
> On Tue, May 05, 2026 at 10:46:11AM +0200, Arnaud POULIQUEN wrote:
>> Hi Beleswar
>>
>> On 5/5/26 07:25, Beleswar Prasad Padhi wrote:
>>> Hi Arnaud,
>>>
>>> On 04/05/26 22:34, Arnaud POULIQUEN wrote:
>>>> Hi Beleswar,
>>>>
>>>> On 5/4/26 10:17, Beleswar Prasad Padhi wrote:
>>>>
>>> [...]
>>>
>>>>>> I may have misunderstood your solution. Could you please help me
>>>>>> understand your proposal by explaining how you would handle three
>>>>>> GPIO ports defined in the DT, considering that the endpoint
>>>>>> addresses on the Linux side can be random?
>>>>>> If I assume there is a unique endpoint on the remote side,
>>>>>> I do not understand how you can match, on the firmware side,
>>>>>> the Linux endpoint address to the GPIO port.
>>>>>
>>>>> Sure, let me take an example:
>>>>> Assumptions: 3 GPIO ports in DT, 3 endpoints in Linux (one per port),
>>>>> 1 endpoint in remote (0xd) and 1 rpmsg channel (rpmsg-io)
>>>>>
>>>>> rpmsg {
>>>>> rpmsg-io {
>>>>> #address-cells = <1>;
>>>>> #size-cells = <0>;
>>>>>
>>>>> gpio@25 {
>>>>> compatible = "rpmsg-gpio";
>>>>> reg = <25>;
>>>>> gpio-controller;
>>>>> #gpio-cells = <2>;
>>>>> #interrupt-cells = <2>;
>>>>> interrupt-controller;
>>>>> };
>>>>>
>>>>> gpio@32 {
>>>>> compatible = "rpmsg-gpio";
>>>>> reg = <32>;
>>>>> gpio-controller;
>>>>> #gpio-cells = <2>;
>>>>> #interrupt-cells = <2>;
>>>>> interrupt-controller;
>>>>> };
>>>>>
>>>>> gpio@35 {
>>>>> compatible = "rpmsg-gpio";
>>>>> reg = <35>;
>>>>> gpio-controller;
>>>>> #gpio-cells = <2>;
>>>>> #interrupt-cells = <2>;
>>>>> interrupt-controller;
>>>>> };
>>>>> };
>>>>> };
>>>>>
>>>>> Code Flow:
>>>>> 1. "rpmsg-io" channel is announced from remote firmware with unique dst
>>>>> ept = 0xd.
>>>>>
>>>>> 2. rpmsg_core.c creates the default dynamic local ept for the channel
>>>>> ept = 0x405.
>>>>>
>>>>> 3. rpmsg_core.c assigns the allocated addr to rpdev device:
>>>>> rpdev->src = 0x405 and rpdev->dst = 0xd.
>>>>>
>>>>> 4. rpmsg_gpio_channel_probe() is triggered. For *each* of the GPIO ports
>>>>> in DT, it will trigger rpmsg_gpiochip_register() which will now:
>>>>> a. Call port->ept = rpmsg_create_ept(rpdev,
>>>>> rpmsg_gpio_channel_callback,
>>>>> port,
>>>>> {rpdev.id.name,
>>>>> RPMSG_ADDR_ANY,
>>>>> RPMSG_ADDR_ANY});
>>>>> Ex- port->ept->addr = 0x408
>>>>>
>>>>> b. Prepare a 8-byte message having 2 fields:
>>>>> port->ept->addr (0x408) and port->idx (25)
>>>>>
>>>>> c. Send this message to remote firmware on default channel ept
>>>>> (0x405 -> 0xd) by:
>>>>> rpmsg_send(rpdev->ept, &message, sizeof(message));
>>>>>
>>>>> d. Remote side receives this message and creates a map of the
>>>>> linux_ept_addr to gpio_port. (0x408 <-> 25)
>>>>>
>>>>> 5. After this point, any gpio messages sent from Linux from gpio port
>>>>> endpoints (Ex- 0x408) can be decoded at remote side by looking up
>>>>> its map (Ex- map[0x408] = 25).
>>>>>
>>>>> 6. Any messages sent from remote to Linux for a particular gpio port can
>>>>> also be decoded at Linux by simply fetching the priv pointer to get
>>>>> the per-port device:
>>>>> struct rpmsg_gpio_port *port = priv;
>>>>>
>>>> Thanks for the details!
>>>>
>>>> To sum up:
>>>> - the default endpoint acts as the GPIO controller (0x405),
>>>> - one extra Linux endpoint is created per port defined in DT.
>>>>
>>>> This should work, but my concerns remain the same:
>>>>
>>>> 1) This implementation forces the remote processor to handle a single
>>>> endpoint instead of one endpoint per port. This may add complexity to
>>>> the remote firmware if each port is managed in a separate thread.
>>>
>>> A. Not really, I just chose 1 remote endpoint for this example as you
>>> suggested to. We can scale it for two-way communication via the
>>> get_config message like you suggested below.
>>>
>>> B. Isn't it a bad design of the firmware if it is handling 10 gpio ports
>>> in 10 threads? The logic to handle all the ports is the same, only
>>> the parameters (e.g. line number, msg) is different.
>>>
>>>> 2) Linux, as a consumer, should not expose its capabilities to the remote
>>>> side (in your proposal it enumerates the ports defined in the DT). In my view, the remote processor should expose its capabilities as the
>>>> provider.
>>>
>>> Agreed on this.
>>>
>>>> From my perspective, based on your proposal:
>>>> 1) Linux should send a get_config message to the remote proc (0x405 -> 0xD). 2) The remote processor would respond with the list of ports, associated
>>>> with an remote endpoint addresses.
>>>
>>> Agreed, we can scale it for multiple remote endpoints like this.
>>>
>>>> 3) Linux would parse the response, compare it with the DT, enable the GPIO
>>>> ports accordingly, creating it local endpoint and associating it with
>>>> the remote endpoint.
>>>> Using name service to identify the ports should avoid step 1 & 2 ...
>>>
>>> Yes, but won't that make a lot of hard-codings in the driver?
>>>
>>> +static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
>>> + { .name = "rpmsg-io-25" },
>>> + { .name = "rpmsg-io-32" },
>>> + { .name = "rpmsg-io-35" },
>>> + { },
>>> +};
>>>
>>> What if tomorrow another vendor decides to add more remoteproc
>>> controlled GPIO ports to Linux, they would have to update this struct in
>>> the driver everytime. And the port indexes (25/32/35) could also differ
>>> between vendors. We should make the driver dynamic i.e. vendor
>>> agnostic.
>>>
>>> I think querying the remote firmware at runtime (step 1 & 2 above) is a
>>> common design pattern and makes the driver vendor agnostic. But feel
>>> free to correct me.
>>>
>> You are right. My proposal would require a patch in rpmsg-core. The idea of
>> allowing a postfix in the compatible string has been discussed before, but,
>> if I remember correctly, it was not concluded.
>>
> I also remember discussing this. I even reviewed one of Arnaud's patch
> and submitted one myself. This must have been in 2020 and the reason why it
> wasn't merged has escaped my memory.
>
>> /* rpmsg devices and drivers are matched using the service name */
>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev,
>> const struct rpmsg_device_id *id)
>> {
>> size_t len;
>>
>> + len = strnlen(id->name, RPMSG_NAME_SIZE);
>> + if (len && id->name[len - 1] == '*')
>> + return !strncmp(id->name, rpdev->id.name, len - 1);
>>
>> return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0;
>> }
>>
>> Then, in rpmsg-gpio, and possibly in other drivers such as rpmsg-tty and
>> a future rpmsg-i2c, we could use:
>> static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
>> { .name = "rpmsg-io" },
>> { .name = "rpmsg-io-*" },
>> { },
>> };
> That was my initial approach. We don't even need an additional "rpmsg-io-*" in
> rpmsg_gpio_channel_id_table[]. All we need is:
>
> /* rpmsg devices and drivers are matched using the service name */
> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev,
> const struct rpmsg_device_id *id)
> {
> + size_t len = strnlen(id->name, RPMSG_NAME_SIZE);
>
> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0;
> + return strncmp(id->name, rpdev->id.name, len) == 0;
> }
This wildcard channel matching is interesting. It would be good to know
the reasons/cons why this patch was not concluded.
>
> And let the rpmsg-virtio-gpio driver parse @rpdev->id.name to match with a
> GPIO controller in the DT.
>
>> If exact name matching is strongly required, then this proposal would not be
>> suitablea.
>>
>> A third option would be a combination of both approaches: instantiate the
>> device using the same name service from the remote side, as done in
>> rpmsg-tty. In that case, a get_config message, or a similar mechanism, would
>> also be needed to retrieve the port information from the remote side.
>>
> I'm not overly fond of a get_config message because it is one more thing we
> have to define and maintain.
>
> Arnaud: is there a get_config message already defined for rpmsg_tty?
>
> Beleswar: Can you provide a link to a virtio device that would use a get_config
> message?
VirtIO typically uses the feature bits for negotiation and discovery.
And such a get_config message would not be needed in VirtIO layer, as
there is no multiplexing. It's a 1:1 mapping of device to driver
instance.
Thanks,
Beleswar
[...]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox