* [PATCH v2 1/4] clk: qcom: gcc-glymur: Move EVA clocks to critical clock list
From: Taniya Das @ 2026-06-17 11:07 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Brian Masney,
Konrad Dybcio, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree, Taniya Das
In-Reply-To: <20260617-evacc_glymur-v2-0-905108dacaaa@oss.qualcomm.com>
The gcc_eva_ahb_clk and gcc_eva_xo_clk branch clocks should not be
registered as standalone GCC branch clocks. Drop these clocks from
the GCC clock list and instead add their CBCR registers to the GCC
critical clocks list to ensure they remain enabled during early boot.
If these clocks are registered as normal branch clocks, they may be
gated, which breaks access to the EVA clock controller during clock
controller probe, thus leave them as critical clocks similar to other
subsystem AHB and XO clocks.
Fixes: efe504300a17 ("clk: qcom: gcc: Add support for Global Clock Controller")
Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
---
drivers/clk/qcom/gcc-glymur.c | 32 ++------------------------------
1 file changed, 2 insertions(+), 30 deletions(-)
diff --git a/drivers/clk/qcom/gcc-glymur.c b/drivers/clk/qcom/gcc-glymur.c
index 2736465efdea9b3cf9ec945107d4b002e123b59f..32d23bdc819b7a62472f2a1ad23c9c8a66cfd0d1 100644
--- a/drivers/clk/qcom/gcc-glymur.c
+++ b/drivers/clk/qcom/gcc-glymur.c
@@ -3669,21 +3669,6 @@ static struct clk_branch gcc_disp_hf_axi_clk = {
},
};
-static struct clk_branch gcc_eva_ahb_clk = {
- .halt_reg = 0x9b004,
- .halt_check = BRANCH_HALT_VOTED,
- .hwcg_reg = 0x9b004,
- .hwcg_bit = 1,
- .clkr = {
- .enable_reg = 0x9b004,
- .enable_mask = BIT(0),
- .hw.init = &(const struct clk_init_data) {
- .name = "gcc_eva_ahb_clk",
- .ops = &clk_branch2_ops,
- },
- },
-};
-
static struct clk_branch gcc_eva_axi0_clk = {
.halt_reg = 0x9b008,
.halt_check = BRANCH_HALT_SKIP,
@@ -3714,19 +3699,6 @@ static struct clk_branch gcc_eva_axi0c_clk = {
},
};
-static struct clk_branch gcc_eva_xo_clk = {
- .halt_reg = 0x9b024,
- .halt_check = BRANCH_HALT,
- .clkr = {
- .enable_reg = 0x9b024,
- .enable_mask = BIT(0),
- .hw.init = &(const struct clk_init_data) {
- .name = "gcc_eva_xo_clk",
- .ops = &clk_branch2_ops,
- },
- },
-};
-
static struct clk_branch gcc_gp1_clk = {
.halt_reg = 0x64000,
.halt_check = BRANCH_HALT,
@@ -7993,10 +7965,8 @@ static struct clk_regmap *gcc_glymur_clocks[] = {
[GCC_CFG_NOC_USB_ANOC_AHB_CLK] = &gcc_cfg_noc_usb_anoc_ahb_clk.clkr,
[GCC_CFG_NOC_USB_ANOC_SOUTH_AHB_CLK] = &gcc_cfg_noc_usb_anoc_south_ahb_clk.clkr,
[GCC_DISP_HF_AXI_CLK] = &gcc_disp_hf_axi_clk.clkr,
- [GCC_EVA_AHB_CLK] = &gcc_eva_ahb_clk.clkr,
[GCC_EVA_AXI0_CLK] = &gcc_eva_axi0_clk.clkr,
[GCC_EVA_AXI0C_CLK] = &gcc_eva_axi0c_clk.clkr,
- [GCC_EVA_XO_CLK] = &gcc_eva_xo_clk.clkr,
[GCC_GP1_CLK] = &gcc_gp1_clk.clkr,
[GCC_GP1_CLK_SRC] = &gcc_gp1_clk_src.clkr,
[GCC_GP2_CLK] = &gcc_gp2_clk.clkr,
@@ -8545,6 +8515,8 @@ static const u32 gcc_glymur_critical_cbcrs[] = {
0x71004, /* GCC_GPU_CFG_AHB_CLK */
0x32004, /* GCC_VIDEO_AHB_CLK */
0x32058, /* GCC_VIDEO_XO_CLK */
+ 0x9b004, /* GCC_EVA_AHB_CLK */
+ 0x9b024, /* GCC_EVA_XO_CLK */
};
static const struct regmap_config gcc_glymur_regmap_config = {
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/4] dt-bindings: clock: qcom: Add EVA clock and reset controller for Glymur SoC
From: Taniya Das @ 2026-06-17 11:07 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Brian Masney,
Konrad Dybcio, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree, Taniya Das
In-Reply-To: <20260617-evacc_glymur-v2-0-905108dacaaa@oss.qualcomm.com>
Add the device tree bindings for the enhanced video analytics(EVA) clock
controller which is required on Qualcomm Glymur SoC. The controller
provides clocks, resets and power domains for the EVA subsystem.
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
---
.../bindings/clock/qcom,glymur-evacc.yaml | 72 ++++++++++++++++++++++
include/dt-bindings/clock/qcom,glymur-evacc.h | 38 ++++++++++++
2 files changed, 110 insertions(+)
diff --git a/Documentation/devicetree/bindings/clock/qcom,glymur-evacc.yaml b/Documentation/devicetree/bindings/clock/qcom,glymur-evacc.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..fb0bc1acc920e906033e04fe38bf500c24247e5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,glymur-evacc.yaml
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,glymur-evacc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm EVA Clock & Reset Controller on Glymur SoC
+
+maintainers:
+ - Taniya Das <taniya.das@oss.qualcomm.com>
+
+description: |
+ Qualcomm EVA clock control module which supports the clocks, resets and
+ power domains for the EVA instances on Glymur SoC.
+
+ See also:
+ - include/dt-bindings/clock/qcom,glymur-evacc.h
+
+properties:
+ compatible:
+ const: qcom,glymur-evacc
+
+ clocks:
+ items:
+ - description: Interface clock from GCC
+ - description: Board XO source
+ - description: Sleep clock source
+
+ power-domains:
+ items:
+ - description: MMCX power domain
+ - description: MXC power domain
+
+ required-opps:
+ description:
+ Required OPP nodes for the MMCX and MXC power domains.
+ items:
+ - description: MMCX performance point
+ - description: MXC performance point
+
+required:
+ - compatible
+ - clocks
+ - power-domains
+ - required-opps
+ - '#power-domain-cells'
+
+allOf:
+ - $ref: qcom,gcc.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,glymur-gcc.h>
+ #include <dt-bindings/clock/qcom,rpmh.h>
+ #include <dt-bindings/power/qcom,rpmhpd.h>
+ clock-controller@ab00000 {
+ compatible = "qcom,glymur-evacc";
+ reg = <0x0ab00000 0x10000>;
+ clocks = <&gcc GCC_EVA_AHB_CLK>,
+ <&rpmhcc RPMH_CXO_CLK>,
+ <&sleep_clk>;
+ power-domains = <&rpmhpd RPMHPD_MMCX>,
+ <&rpmhpd RPMHPD_MXC>;
+ required-opps = <&rpmhpd_opp_low_svs>,
+ <&rpmhpd_opp_low_svs>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ #power-domain-cells = <1>;
+ };
+...
diff --git a/include/dt-bindings/clock/qcom,glymur-evacc.h b/include/dt-bindings/clock/qcom,glymur-evacc.h
new file mode 100644
index 0000000000000000000000000000000000000000..35a7b4550351661bdb1f7bdfbeec625fafdfcef7
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,glymur-evacc.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#ifndef _DT_BINDINGS_CLK_QCOM_EVACC_GLYMUR_H
+#define _DT_BINDINGS_CLK_QCOM_EVACC_GLYMUR_H
+
+/* EVA_CC clocks */
+#define EVA_CC_AHB_CLK 0
+#define EVA_CC_AHB_CLK_SRC 1
+#define EVA_CC_MVS0_CLK 2
+#define EVA_CC_MVS0_CLK_SRC 3
+#define EVA_CC_MVS0_DIV_CLK_SRC 4
+#define EVA_CC_MVS0_FREERUN_CLK 5
+#define EVA_CC_MVS0_SHIFT_CLK 6
+#define EVA_CC_MVS0C_CLK 7
+#define EVA_CC_MVS0C_DIV2_DIV_CLK_SRC 8
+#define EVA_CC_MVS0C_FREERUN_CLK 9
+#define EVA_CC_MVS0C_SHIFT_CLK 10
+#define EVA_CC_PLL0 11
+#define EVA_CC_SLEEP_CLK 12
+#define EVA_CC_SLEEP_CLK_SRC 13
+#define EVA_CC_XO_CLK 14
+#define EVA_CC_XO_CLK_SRC 15
+
+/* EVA_CC power domains */
+#define EVA_CC_MVS0_GDSC 0
+#define EVA_CC_MVS0C_GDSC 1
+
+/* EVA_CC resets */
+#define EVA_CC_INTERFACE_BCR 0
+#define EVA_CC_MVS0_BCR 1
+#define EVA_CC_MVS0C_CLK_ARES 2
+#define EVA_CC_MVS0C_BCR 3
+#define EVA_CC_MVS0C_FREERUN_CLK_ARES 4
+
+#endif /* _DT_BINDINGS_CLK_QCOM_EVACC_GLYMUR_H */
--
2.34.1
^ permalink raw reply related
* [PATCH v2 3/4] clk: qcom: Add EVA clock controller driver for Glymur SoC
From: Taniya Das @ 2026-06-17 11:07 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Brian Masney,
Konrad Dybcio, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree, Taniya Das
In-Reply-To: <20260617-evacc_glymur-v2-0-905108dacaaa@oss.qualcomm.com>
Add the Enhanced Video Analytics (EVA) clock controller driver for
the Glymur SoC. The EVACC manages the PLL, RCGs, branch clocks, GDSCs
and resets for the EVA subsystem which handles vision processing
workloads.
Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
---
drivers/clk/qcom/Kconfig | 11 +
drivers/clk/qcom/Makefile | 1 +
drivers/clk/qcom/evacc-glymur.c | 455 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 467 insertions(+)
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index d9cff5b0281d8cc373b8ab14683370cb9b7f8bf3..94378d435162799aa866689377e4a9f1e96ab138 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -55,6 +55,17 @@ config CLK_GLYMUR_DISPCC
Say Y if you want to support display devices and functionality such as
splash screen.
+config CLK_GLYMUR_EVACC
+ tristate "Glymur EVA Clock Controller"
+ depends on ARM64 || COMPILE_TEST
+ default m if ARCH_QCOM
+ select CLK_GLYMUR_GCC
+ help
+ Support for the Enhanced Video Analytics (EVA) clock controller on
+ Qualcomm Technologies, Inc. Glymur devices.
+ Say Y if you want to support EVA devices and functionality such as
+ vision processing.
+
config CLK_GLYMUR_GCC
tristate "Glymur Global Clock Controller"
depends on ARM64 || COMPILE_TEST
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index e100cfd6a52de9f88f11720d9c2043db5e553618..74761f2c767d9ce5988fedf539d80dc1393b4617 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_CLK_ELIZA_GCC) += gcc-eliza.o
obj-$(CONFIG_CLK_ELIZA_TCSRCC) += tcsrcc-eliza.o
obj-$(CONFIG_CLK_GFM_LPASS_SM8250) += lpass-gfm-sm8250.o
obj-$(CONFIG_CLK_GLYMUR_DISPCC) += dispcc-glymur.o
+obj-$(CONFIG_CLK_GLYMUR_EVACC) += evacc-glymur.o
obj-$(CONFIG_CLK_GLYMUR_GCC) += gcc-glymur.o
obj-$(CONFIG_CLK_GLYMUR_GPUCC) += gpucc-glymur.o gxclkctl-kaanapali.o
obj-$(CONFIG_CLK_GLYMUR_TCSRCC) += tcsrcc-glymur.o
diff --git a/drivers/clk/qcom/evacc-glymur.c b/drivers/clk/qcom/evacc-glymur.c
new file mode 100644
index 0000000000000000000000000000000000000000..ba124cfc1a4def4c20962de03dbd8750b2d3f393
--- /dev/null
+++ b/drivers/clk/qcom/evacc-glymur.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,glymur-evacc.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-pll.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "clk-regmap-divider.h"
+#include "clk-regmap-mux.h"
+#include "common.h"
+#include "gdsc.h"
+#include "reset.h"
+
+enum {
+ DT_AHB_CLK,
+ DT_BI_TCXO,
+ DT_SLEEP_CLK,
+};
+
+enum {
+ P_BI_TCXO,
+ P_EVA_CC_PLL0_OUT_MAIN,
+ P_SLEEP_CLK,
+};
+
+static const struct pll_vco taycan_eko_t_vco[] = {
+ { 249600000, 2500000000, 0 },
+};
+
+/* 840.0 MHz Configuration */
+static const struct alpha_pll_config eva_cc_pll0_config = {
+ .l = 0x2b,
+ .alpha = 0xc000,
+ .config_ctl_val = 0x25c400e7,
+ .config_ctl_hi_val = 0x0a8060e0,
+ .config_ctl_hi1_val = 0xf51dea20,
+ .user_ctl_val = 0x00000008,
+ .user_ctl_hi_val = 0x00000002,
+};
+
+static struct clk_alpha_pll eva_cc_pll0 = {
+ .offset = 0x0,
+ .config = &eva_cc_pll0_config,
+ .vco_table = taycan_eko_t_vco,
+ .num_vco = ARRAY_SIZE(taycan_eko_t_vco),
+ .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TAYCAN_EKO_T],
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_pll0",
+ .parent_data = &(const struct clk_parent_data) {
+ .index = DT_BI_TCXO,
+ },
+ .num_parents = 1,
+ .ops = &clk_alpha_pll_taycan_eko_t_ops,
+ },
+ },
+};
+
+static const struct parent_map eva_cc_parent_map_0[] = {
+ { P_BI_TCXO, 0 },
+};
+
+static const struct clk_parent_data eva_cc_parent_data_0[] = {
+ { .index = DT_BI_TCXO },
+};
+
+static const struct parent_map eva_cc_parent_map_1[] = {
+ { P_BI_TCXO, 0 },
+ { P_EVA_CC_PLL0_OUT_MAIN, 1 },
+};
+
+static const struct clk_parent_data eva_cc_parent_data_1[] = {
+ { .index = DT_BI_TCXO },
+ { .hw = &eva_cc_pll0.clkr.hw },
+};
+
+static const struct parent_map eva_cc_parent_map_2[] = {
+ { P_SLEEP_CLK, 0 },
+};
+
+static const struct clk_parent_data eva_cc_parent_data_2[] = {
+ { .index = DT_SLEEP_CLK },
+};
+
+static const struct freq_tbl ftbl_eva_cc_ahb_clk_src[] = {
+ F(19200000, P_BI_TCXO, 1, 0, 0),
+ { }
+};
+
+static struct clk_rcg2 eva_cc_ahb_clk_src = {
+ .cmd_rcgr = 0x8018,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = eva_cc_parent_map_0,
+ .freq_tbl = ftbl_eva_cc_ahb_clk_src,
+ .hw_clk_ctrl = true,
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_ahb_clk_src",
+ .parent_data = eva_cc_parent_data_0,
+ .num_parents = ARRAY_SIZE(eva_cc_parent_data_0),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_shared_ops,
+ },
+ },
+};
+
+static const struct freq_tbl ftbl_eva_cc_mvs0_clk_src[] = {
+ F(840000000, P_EVA_CC_PLL0_OUT_MAIN, 1, 0, 0),
+ F(1050000000, P_EVA_CC_PLL0_OUT_MAIN, 1, 0, 0),
+ F(1350000000, P_EVA_CC_PLL0_OUT_MAIN, 1, 0, 0),
+ F(1500000000, P_EVA_CC_PLL0_OUT_MAIN, 1, 0, 0),
+ F(1650000000, P_EVA_CC_PLL0_OUT_MAIN, 1, 0, 0),
+ { }
+};
+
+static struct clk_rcg2 eva_cc_mvs0_clk_src = {
+ .cmd_rcgr = 0x8000,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = eva_cc_parent_map_1,
+ .freq_tbl = ftbl_eva_cc_mvs0_clk_src,
+ .hw_clk_ctrl = true,
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0_clk_src",
+ .parent_data = eva_cc_parent_data_1,
+ .num_parents = ARRAY_SIZE(eva_cc_parent_data_1),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_shared_ops,
+ },
+ },
+};
+
+static const struct freq_tbl ftbl_eva_cc_sleep_clk_src[] = {
+ F(32000, P_SLEEP_CLK, 1, 0, 0),
+ { }
+};
+
+static struct clk_rcg2 eva_cc_sleep_clk_src = {
+ .cmd_rcgr = 0x80e0,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = eva_cc_parent_map_2,
+ .freq_tbl = ftbl_eva_cc_sleep_clk_src,
+ .hw_clk_ctrl = true,
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_sleep_clk_src",
+ .parent_data = eva_cc_parent_data_2,
+ .num_parents = ARRAY_SIZE(eva_cc_parent_data_2),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_shared_ops,
+ },
+ },
+};
+
+static struct clk_rcg2 eva_cc_xo_clk_src = {
+ .cmd_rcgr = 0x80bc,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = eva_cc_parent_map_0,
+ .freq_tbl = ftbl_eva_cc_ahb_clk_src,
+ .hw_clk_ctrl = true,
+ .clkr = {
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_xo_clk_src",
+ .parent_data = eva_cc_parent_data_0,
+ .num_parents = ARRAY_SIZE(eva_cc_parent_data_0),
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_rcg2_shared_ops,
+ },
+ },
+};
+
+static struct clk_regmap_div eva_cc_mvs0_div_clk_src = {
+ .reg = 0x809c,
+ .shift = 0,
+ .width = 4,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0_div_clk_src",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_mvs0_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_regmap_div_ro_ops,
+ },
+};
+
+static struct clk_regmap_div eva_cc_mvs0c_div2_div_clk_src = {
+ .reg = 0x8060,
+ .shift = 0,
+ .width = 4,
+ .clkr.hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0c_div2_div_clk_src",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_mvs0_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_regmap_div_ro_ops,
+ },
+};
+
+static struct clk_branch eva_cc_mvs0_clk = {
+ .halt_reg = 0x807c,
+ .halt_check = BRANCH_HALT_VOTED,
+ .hwcg_reg = 0x807c,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0x807c,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0_clk",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_mvs0_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch eva_cc_mvs0_freerun_clk = {
+ .halt_reg = 0x808c,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0x808c,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0_freerun_clk",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_mvs0_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch eva_cc_mvs0_shift_clk = {
+ .halt_reg = 0x80d8,
+ .halt_check = BRANCH_HALT_VOTED,
+ .hwcg_reg = 0x80d8,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0x80d8,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0_shift_clk",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_xo_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch eva_cc_mvs0c_clk = {
+ .halt_reg = 0x804c,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0x804c,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0c_clk",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_mvs0c_div2_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch eva_cc_mvs0c_freerun_clk = {
+ .halt_reg = 0x805c,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0x805c,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0c_freerun_clk",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_mvs0c_div2_div_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch eva_cc_mvs0c_shift_clk = {
+ .halt_reg = 0x80dc,
+ .halt_check = BRANCH_HALT_VOTED,
+ .hwcg_reg = 0x80dc,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0x80dc,
+ .enable_mask = BIT(0),
+ .hw.init = &(const struct clk_init_data) {
+ .name = "eva_cc_mvs0c_shift_clk",
+ .parent_hws = (const struct clk_hw*[]) {
+ &eva_cc_xo_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct gdsc eva_cc_mvs0c_gdsc = {
+ .gdscr = 0x8034,
+ .en_rest_wait_val = 0x2,
+ .en_few_wait_val = 0x2,
+ .clk_dis_wait_val = 0x6,
+ .pd = {
+ .name = "eva_cc_mvs0c_gdsc",
+ },
+ .pwrsts = PWRSTS_OFF_ON,
+ .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
+};
+
+static struct gdsc eva_cc_mvs0_gdsc = {
+ .gdscr = 0x8068,
+ .en_rest_wait_val = 0x2,
+ .en_few_wait_val = 0x2,
+ .clk_dis_wait_val = 0x6,
+ .pd = {
+ .name = "eva_cc_mvs0_gdsc",
+ },
+ .pwrsts = PWRSTS_OFF_ON,
+ .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
+ .parent = &eva_cc_mvs0c_gdsc.pd,
+};
+
+static struct clk_regmap *eva_cc_glymur_clocks[] = {
+ [EVA_CC_AHB_CLK_SRC] = &eva_cc_ahb_clk_src.clkr,
+ [EVA_CC_MVS0_CLK] = &eva_cc_mvs0_clk.clkr,
+ [EVA_CC_MVS0_CLK_SRC] = &eva_cc_mvs0_clk_src.clkr,
+ [EVA_CC_MVS0_DIV_CLK_SRC] = &eva_cc_mvs0_div_clk_src.clkr,
+ [EVA_CC_MVS0_FREERUN_CLK] = &eva_cc_mvs0_freerun_clk.clkr,
+ [EVA_CC_MVS0_SHIFT_CLK] = &eva_cc_mvs0_shift_clk.clkr,
+ [EVA_CC_MVS0C_CLK] = &eva_cc_mvs0c_clk.clkr,
+ [EVA_CC_MVS0C_DIV2_DIV_CLK_SRC] = &eva_cc_mvs0c_div2_div_clk_src.clkr,
+ [EVA_CC_MVS0C_FREERUN_CLK] = &eva_cc_mvs0c_freerun_clk.clkr,
+ [EVA_CC_MVS0C_SHIFT_CLK] = &eva_cc_mvs0c_shift_clk.clkr,
+ [EVA_CC_PLL0] = &eva_cc_pll0.clkr,
+ [EVA_CC_SLEEP_CLK_SRC] = &eva_cc_sleep_clk_src.clkr,
+ [EVA_CC_XO_CLK_SRC] = &eva_cc_xo_clk_src.clkr,
+};
+
+static struct gdsc *eva_cc_glymur_gdscs[] = {
+ [EVA_CC_MVS0_GDSC] = &eva_cc_mvs0_gdsc,
+ [EVA_CC_MVS0C_GDSC] = &eva_cc_mvs0c_gdsc,
+};
+
+static const struct qcom_reset_map eva_cc_glymur_resets[] = {
+ [EVA_CC_INTERFACE_BCR] = { 0x80a0 },
+ [EVA_CC_MVS0_BCR] = { 0x8064 },
+ [EVA_CC_MVS0C_CLK_ARES] = { 0x804c, 2 },
+ [EVA_CC_MVS0C_BCR] = { 0x8030 },
+ [EVA_CC_MVS0C_FREERUN_CLK_ARES] = { 0x805c, 2 },
+};
+
+static struct clk_alpha_pll *eva_cc_glymur_plls[] = {
+ &eva_cc_pll0,
+};
+
+static const u32 eva_cc_glymur_critical_cbcrs[] = {
+ 0x80a4, /* EVA_CC_AHB_CLK */
+ 0x80f8, /* EVA_CC_SLEEP_CLK */
+ 0x80d4, /* EVA_CC_XO_CLK */
+};
+
+static const struct regmap_config eva_cc_glymur_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .max_register = 0x9f50,
+ .fast_io = true,
+};
+
+static void clk_glymur_regs_configure(struct device *dev, struct regmap *regmap)
+{
+ /*
+ * Update CTRL_IN register as per HW recommendation to ensure clocks
+ * stay cycle‑aligned when the EVA core is ON.
+ */
+ regmap_set_bits(regmap, 0x9f24, BIT(0));
+}
+
+static const struct qcom_cc_driver_data eva_cc_glymur_driver_data = {
+ .alpha_plls = eva_cc_glymur_plls,
+ .num_alpha_plls = ARRAY_SIZE(eva_cc_glymur_plls),
+ .clk_cbcrs = eva_cc_glymur_critical_cbcrs,
+ .num_clk_cbcrs = ARRAY_SIZE(eva_cc_glymur_critical_cbcrs),
+ .clk_regs_configure = clk_glymur_regs_configure,
+};
+
+static const struct qcom_cc_desc eva_cc_glymur_desc = {
+ .config = &eva_cc_glymur_regmap_config,
+ .clks = eva_cc_glymur_clocks,
+ .num_clks = ARRAY_SIZE(eva_cc_glymur_clocks),
+ .resets = eva_cc_glymur_resets,
+ .num_resets = ARRAY_SIZE(eva_cc_glymur_resets),
+ .gdscs = eva_cc_glymur_gdscs,
+ .num_gdscs = ARRAY_SIZE(eva_cc_glymur_gdscs),
+ .use_rpm = true,
+ .driver_data = &eva_cc_glymur_driver_data,
+};
+
+static const struct of_device_id eva_cc_glymur_match_table[] = {
+ { .compatible = "qcom,glymur-evacc" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, eva_cc_glymur_match_table);
+
+static int eva_cc_glymur_probe(struct platform_device *pdev)
+{
+ return qcom_cc_probe(pdev, &eva_cc_glymur_desc);
+}
+
+static struct platform_driver eva_cc_glymur_driver = {
+ .probe = eva_cc_glymur_probe,
+ .driver = {
+ .name = "evacc-glymur",
+ .of_match_table = eva_cc_glymur_match_table,
+ },
+};
+
+module_platform_driver(eva_cc_glymur_driver);
+
+MODULE_DESCRIPTION("QTI EVACC Glymur Driver");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related
* [PATCH v2 4/4] arm64: dts: qcom: glymur: Add EVA clock controller node
From: Taniya Das @ 2026-06-17 11:07 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Brian Masney,
Konrad Dybcio, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree, Taniya Das
In-Reply-To: <20260617-evacc_glymur-v2-0-905108dacaaa@oss.qualcomm.com>
Add the device node for the EVA clock controller (evacc) for Qualcomm
Glymur SoC. This clock controller provides clocks and resets to the
EVA hardware block.
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
---
arch/arm64/boot/dts/qcom/glymur.dtsi | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/glymur.dtsi b/arch/arm64/boot/dts/qcom/glymur.dtsi
index 20b49af7298e9549d126aa50a0dc7a90943a3249..d7aee11ca9eff85de4813dd08a1a2c20ce098250 100644
--- a/arch/arm64/boot/dts/qcom/glymur.dtsi
+++ b/arch/arm64/boot/dts/qcom/glymur.dtsi
@@ -4,6 +4,7 @@
*/
#include <dt-bindings/clock/qcom,glymur-dispcc.h>
+#include <dt-bindings/clock/qcom,glymur-evacc.h>
#include <dt-bindings/clock/qcom,glymur-gcc.h>
#include <dt-bindings/clock/qcom,glymur-gpucc.h>
#include <dt-bindings/clock/qcom,glymur-tcsr.h>
@@ -4804,6 +4805,23 @@ videocc: clock-controller@aaf0000 {
#power-domain-cells = <1>;
};
+ evacc: clock-controller@abf0000 {
+ compatible = "qcom,glymur-evacc";
+ reg = <0x0 0x0abf0000 0x0 0x10000>;
+ clocks = <&gcc GCC_EVA_AHB_CLK>,
+ <&rpmhcc RPMH_CXO_CLK>,
+ <&sleep_clk>;
+
+ power-domains = <&rpmhpd RPMHPD_MMCX>,
+ <&rpmhpd RPMHPD_MXC>;
+ required-opps = <&rpmhpd_opp_low_svs>,
+ <&rpmhpd_opp_low_svs>;
+
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ #power-domain-cells = <1>;
+ };
+
dispcc: clock-controller@af00000 {
compatible = "qcom,glymur-dispcc";
reg = <0x0 0x0af00000 0x0 0x20000>;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 09/12] rtc: rzn1: Use temporary variable for struct device
From: Wolfram Sang @ 2026-06-17 11:08 UTC (permalink / raw)
To: Prabhakar
Cc: Miquel Raynal, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, linux-rtc, linux-renesas-soc, devicetree,
linux-kernel, Biju Das, Fabrizio Castro, Lad Prabhakar
In-Reply-To: <20260615154805.1619693-10-prabhakar.mahadev-lad.rj@bp.renesas.com>
[-- Attachment #1: Type: text/plain, Size: 3370 bytes --]
Hi,
Sashiko found issues, not introduced by your patch, but still. I
curated the list and left only those which I think are valid. Do you
have time to look at these issues?
Thank you and happy hacking,
Wolfram
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Asynchronous pm_runtime_put() is used instead of pm_runtime_put_sync() before runtime PM is disabled, potentially leaving the device powered on.
> - [High] If the optional 'xtal' clock is missing, readl_poll_timeout() loops forever with a 0 timeout, risking a boot hang.
> - [High] The driver's `remove` callback prematurely disables hardware interrupts and drops the runtime PM reference while the RTC device is still fully registered.
> --
>
> commit b7f73c7d0932a6e78b12419c5912ad3243637b8f
> Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> rtc: rzn1: Use temporary variable for struct device
>
> This patch simplifies the rzn1_rtc_probe function by using a local temporary
> variable for the struct device pointer. This avoids repeated dereferences of
> pdev->dev throughout the function.
>
> > diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> > index bc6af59744e48..25cad9213147d 100644
> > --- a/drivers/rtc/rtc-rzn1.c
> > +++ b/drivers/rtc/rtc-rzn1.c
> [ ... ]
> > if (IS_ERR(xtal)) {
> > ret = PTR_ERR(xtal);
> > goto dis_runtime_pm;
> [ ... ]
> > @@ -486,18 +487,18 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
> [ ... ]
> > device_init_wakeup(dev, true);
> >
> > ret = devm_rtc_register_device(rtc->rtcdev);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does registering the RTC
> device using devm_rtc_register_device() create a race condition during
> teardown?
>
> In rzn1_rtc_remove(), the driver manually disables interrupts and calls
> pm_runtime_put() before devres has unregistered the RTC device:
>
> writel(0, rtc->base + RZN1_RTC_CTL1);
> pm_runtime_put(&pdev->dev);
>
> Could userspace still interact with the RTC during this window via ioctls
> or sysfs, potentially causing synchronous bus faults since the hardware is
> already suspended? Should teardown be bound using
> devm_add_action_or_reset() to guarantee correct reverse execution order?
[wsa] Or maybe not use devm then?
>
> > if (ret)
> > @@ -506,7 +507,7 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
> > return 0;
> >
> > dis_runtime_pm:
> > - pm_runtime_put(&pdev->dev);
> > + pm_runtime_put(dev);
>
> [Severity: High]
> This is a pre-existing issue, but does using pm_runtime_put() here (and in
> rzn1_rtc_remove) leave the device powered on indefinitely?
>
> Since devm_pm_runtime_enable(dev) schedules pm_runtime_disable() to
> execute during devres cleanup, calling pm_runtime_put() only queues an
> asynchronous idle check. The immediate return triggers devres cleanup,
> which executes a barrier that explicitly cancels pending async operations.
>
> Should this use pm_runtime_put_sync() instead to ensure the device is
> synchronously suspended before teardown?
>
> > return ret;
> > }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260615154805.1619693-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=9
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 12/12] rtc: rzn1: Add support for Renesas RZ/T2H and RZ/N2H SoCs
From: Wolfram Sang @ 2026-06-17 11:10 UTC (permalink / raw)
To: Prabhakar
Cc: Miquel Raynal, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, linux-rtc, linux-renesas-soc, devicetree,
linux-kernel, Biju Das, Fabrizio Castro, Lad Prabhakar
In-Reply-To: <20260615154805.1619693-13-prabhakar.mahadev-lad.rj@bp.renesas.com>
[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]
On Mon, Jun 15, 2026 at 04:48:05PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add a new compatible string "renesas,r9a09g077-rtc" to the OF match table
> to support the RTC IP variant found on the RZ/T2H and RZ/N2H SoCs.
>
> These newer SoCs integrate a closely related variant of the RZ/N1 RTC IP.
> The RZ/T2H and RZ/N2H variants lack the RTCA0SUBU and RTCA0TCR registers,
The RTCA0TCR is also not on RZ/N1.
> those registers are not accessed or used when operating under the
> rzn1_rtc_ops_scmp configurations, making the current infrastructure
> compatible.
>
> The RZ/T2H RTC variant also supports a 1 Hz output signal on the
> RTCAT1HZ pin, controlled by the RTCA0CTL1[RTCA01HZE] bit. This bit is
> marked as reserved in the RZ/N1 hardware manual, making RZ/T2H a
> distinct RTC variant despite its overall compatibility with the RZ/N1
> implementation.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
With that fixed:
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/1] arm64: dts: qcom: Add Microsoft Surface Pro 12in
From: Konrad Dybcio @ 2026-06-17 11:11 UTC (permalink / raw)
To: Harrison Vanderbyl, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Dmitry Baryshkov, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260609145906.40854-2-harrison.vanderbyl@gmail.com>
On 6/9/26 4:59 PM, Harrison Vanderbyl wrote:
> Initial device tree for Microsoft Surface Pro 12in
>
> Currently supported:
> - UFS
> - Touchscreen
> - Pen
> - USB 3.2 x2 (DP Alt Mode)
> - Audio
> - Wifi
> - Bluetooth
> - CDSP
> - ADSP
> - GPU
>
> Not currently supported:
> - Accelerometer
> - Front, Back and IR cameras
> - IRIS video decoder
>
> Tested on Surface_Pro_12in_1st_Ed_with_Snapdragon_2110
>
> Signed-off-by: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH 00/12] Add RTC support for Renesas RZ/T2H and RZ/N2H SoCs
From: Wolfram Sang @ 2026-06-17 11:12 UTC (permalink / raw)
To: Prabhakar
Cc: Miquel Raynal, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, linux-rtc, linux-renesas-soc, devicetree,
linux-kernel, Biju Das, Fabrizio Castro, Lad Prabhakar
In-Reply-To: <ajJmacl9ZJtkoLyf@shikoro>
[-- Attachment #1: Type: text/plain, Size: 190 bytes --]
> I will review and test in on my N1D-board today.
Except for the strange alarm-boundary behaviour, the tests went well.
Will do further tests, it is probably not related to this series.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v12 2/7] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
From: sashiko-bot @ 2026-06-17 11:13 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: robh, conor+dt, devicetree, linux-i3c, Frank.Li
In-Reply-To: <20260617110355.1591844-3-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding documents GPIO capability for target ports but omits the standard GPIO provider properties, violating subsystem guidelines.
- [Medium] The binding fails to include compatible strings for the 4-port variants and unconditionally allows up to 8 ports.
- [Low] The unit address in the I3C example does not match its `reg` property.
--
commit af3b77595fece3e15864b37b9b683be835a0e1eb
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
This commit adds device tree bindings for the NXP P3H2x4x multi-port I3C hub
family. It supports connecting a host via I3C/I2C/SMBus to multiple downstream
peripherals across up to 8 ports.
> diff --git a/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> new file mode 100644
> index 0000000000000..c080eeb0eeaa8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> @@ -0,0 +1,291 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2025 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/nxp,p3h2840.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP P3H2X4X I3C HUB
> +
> +maintainers:
> + - Aman Kumar Pandey <aman.kumarpandey@nxp.com>
> + - Vikash Bansal <vikash.bansal@nxp.com>
> + - Lakshay Piplani <lakshay.piplani@nxp.com>
> +
> +description: |
> + P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841) is a family of multiport I3C
> + hub devices that connect to:-
> + 1. A host CPU via I3C/I2C/SMBus bus on upstream side and connect to multiple
> + peripheral devices on the downstream side.
> + 2. Have two Controller Ports which can support either
> + I2C/SMBus or I3C buses and connect to a CPU, BMC or SOC.
> + 3. P3H2840/ P3H2841 are 8 port I3C hub with eight I3C/I2C Target Port.
> + 4. P3H2440/ P3H2441 are 4 port I3C hub with four I3C/I2C Target Port.
> + Target ports can be configured as I2C/SMBus, I3C or GPIO and connect to
> + peripherals.
[Severity: Medium]
Since the description advertises that target ports can be configured as GPIO,
should this binding include the standard GPIO provider properties like
gpio-controller and #gpio-cells?
Because unevaluatedProperties is false and there are no child node patterns
defined for GPIO ports in patternProperties, won't adding a GPIO child node
fail schema validation?
> +
> +properties:
> + compatible:
> + const: nxp,p3h2840
[Severity: Medium]
The commit message and description state the binding supports the P3H2440
and P3H2441 4-port hubs alongside the 8-port P3H2840/P3H2841.
Should compatible strings be included for the 4-port variants here as well?
[ ... ]
> +patternProperties:
> + "^i3c@[0-7]$":
> + type: object
> + $ref: /schemas/i3c/i3c.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description:
> + The I3C HUB Target Port number.
> + maximum: 7
[Severity: Medium]
Does unconditionally allowing a maximum port index of 7 cause issues for the
4-port variants mentioned in the description?
It seems this allows invalid configurations that define non-existent ports
(4-7) for the 4-port hardware.
[ ... ]
> +examples:
> + - |
> + i3c {
> + #address-cells = <3>;
> + #size-cells = <0>;
> +
> + hub@70,236153000c2 {
> + reg = <0x70 0x236 0x3000c2>;
[Severity: Low]
Does the unit address in this example match its reg property?
The reg property combines to a provisioned ID of 0x236003000c2, which appears
to differ from 236153000c2 in the unit address. Could this mismatch trigger a
simple_bus_reg warning during make dt_binding_check?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=2
^ permalink raw reply
* Re: [PATCH v12 4/7] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
From: sashiko-bot @ 2026-06-17 11:17 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: linux-i3c, robh, conor+dt, Frank.Li, devicetree
In-Reply-To: <20260617110355.1591844-5-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Concurrent regulator operations race on the shared device-level protection register, leading to silent hardware configuration failures.
- [Medium] The driver module will not auto-load because it lacks a MODULE_ALIAS or platform_device_id table.
- [Medium] If regmap_read or the unlock sequence fails in the guard constructor, the driver proceeds with the regulator operation on locked registers.
- [Low] Typo in regulator config assignment assigns dev->of_node to itself instead of rcfg.of_node.
--
commit e68081b58005a2422d441a5611dac29f8cffe2f4
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
The NXP P3H2x4x family integrates on-die regulators alongside I3C hub
functionality. This driver registers the regulators using the MFD
framework and exposes them via the regulator subsystem.
> --- /dev/null
> +++ b/drivers/regulator/p3h2840_i3c_hub_regulator.c
[ ... ]
> +static void p3h2x4x_reg_guard_enter(struct regulator_dev *rdev,
> + struct p3h2x4x_reg_state *state)
> +{
> + state->restore = false;
> +
> + if (regmap_read(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + &state->orig))
> + return;
[Severity: Medium]
If regmap_read() fails here, the function returns early and swallows the
error since the guard constructor returns void. Will this cause the caller
to proceed with regulator operations on locked hardware, potentially
resulting in silent hardware configuration failures?
If the device ACKs the transaction despite ignoring the write, wouldn't
the driver incorrectly report success to the regulator core?
> +
> + if (state->orig != P3H2X4X_REGISTERS_UNLOCK_CODE) {
> + regmap_write(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + P3H2X4X_REGISTERS_UNLOCK_CODE);
> + state->restore = true;
> + }
> +}
> +
> +static void p3h2x4x_reg_guard_exit(struct regulator_dev *rdev,
> + struct p3h2x4x_reg_state *state)
> +{
> + if (state->restore)
> + regmap_write(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + state->orig);
> +}
> +
> +DEFINE_LOCK_GUARD_1(p3h2x4x_reg, struct regulator_dev,
> + p3h2x4x_reg_guard_enter(_T->lock, &_T->state),
> + p3h2x4x_reg_guard_exit(_T->lock, &_T->state),
> + struct p3h2x4x_reg_state state);
[Severity: High]
Does this guard provide mutual exclusion across the four different regulators
on the same device?
The regmap lock protects individual I/O operations, but since there is no
device-level lock held across the unlock/modify/lock sequence, can concurrent
operations on different regulators interleave?
For example, could one thread re-lock the device in p3h2x4x_reg_guard_exit()
while another thread is just about to write its configuration, causing the
hardware write to be silently ignored? Is a shared device-level mutex needed
to serialize these sequences?
[ ... ]
> +static int p3h2x4x_regulator_probe(struct platform_device *pdev)
> +{
> + struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
> + struct p3h2x4x_regulator_dev *p3h2x4x_regulator;
> + struct regulator_config rcfg = { };
> + struct device *dev = &pdev->dev;
> + struct regulator_dev *rdev;
> + int i;
> +
> + p3h2x4x_regulator = devm_kzalloc(dev, sizeof(*p3h2x4x_regulator), GFP_KERNEL);
> + if (!p3h2x4x_regulator)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, p3h2x4x_regulator);
> +
> + p3h2x4x_regulator->regmap = p3h2x4x->regmap;
> + device_set_of_node_from_dev(dev, dev->parent);
> +
> + rcfg.dev = dev;
> + rcfg.dev->of_node = dev->of_node;
[Severity: Low]
Should this be rcfg.of_node = dev->of_node? Since rcfg.dev is assigned to dev
on the previous line, this just assigns dev->of_node to itself.
> + rcfg.regmap = p3h2x4x_regulator->regmap;
> + rcfg.driver_data = p3h2x4x_regulator;
[ ... ]
> +static struct platform_driver p3h2x4x_regulator_driver = {
> + .driver = {
> + .name = "p3h2x4x-regulator",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .probe = p3h2x4x_regulator_probe,
> +};
> +module_platform_driver(p3h2x4x_regulator_driver);
> +
> +MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
> +MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB Regulator driver");
> +MODULE_LICENSE("GPL");
[Severity: Medium]
Since the driver is registered as a platform driver with the name
"p3h2x4x-regulator" but the module is compiled as
"p3h2840_i3c_hub_regulator.ko", will this module fail to auto-load?
Without a MODULE_ALIAS("platform:p3h2x4x-regulator") or an exported
platform_device_id table, udev may not be able to load the module when the
MFD core creates the child platform device.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=4
^ permalink raw reply
* Re: [PATCH v12 5/7] i3c: hub: Add support for the I3C interface in the I3C hub
From: sashiko-bot @ 2026-06-17 11:18 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: robh, conor+dt, Frank.Li, devicetree, linux-i3c
In-Reply-To: <20260617110355.1591844-6-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Deterministic NULL pointer dereference in `i3c_hub_free_ibi()` due to recursive callback chain.
- [Critical] Memory leak and double-free memory corruption in `i3c_hub_request_ibi()` error paths.
- [Critical] Critical race condition and parent bus state corruption in `i3c_hub_master_priv_xfers()` due to unlocked address mutation.
- [Critical] Unimplemented `i3c_hub_recycle_ibi_slot()` permanently leaks hardware IBI slots.
- [Critical] Architectural bypass in `i3c_hub_do_daa()` attaches dynamically discovered devices to the parent bus instead of the virtual hub bus.
- [High] Calling `i3c_hub_enable_port()` outside of any locks allows concurrent operations to physically enable multiple downstream ports simultaneously, breaking electrical isolation.
- [High] Silently dropping broadcast RSTDAA commands prevents resetting of downstream devices.
- [Medium] Accepting I2C devices while unconditionally rejecting all I2C transfers violates the API contract.
--
commit 2a201a9e0e9a55b31467d874340085240b34afc3
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
i3c: hub: Add support for the I3C interface in the I3C hub
This patch adds virtual I3C bus support for the hub and provides an
interface to enable or disable downstream ports.
> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 0000000000000..e8744b5172340
> --- /dev/null
> +++ b/drivers/i3c/hub.c
> @@ -0,0 +1,483 @@
[ ... ]
> +static int i3c_hub_do_daa(struct i3c_hub *hub,
> + struct i3c_master_controller *controller)
> +{
> + struct i3c_master_controller *parent;
> + int ret;
> +
> + if (!hub || !hub->hub_dev)
> + return -ENODEV;
> +
> + parent = i3c_dev_get_master(hub->hub_dev->desc);
> + if (!parent)
> + return -ENODEV;
> +
> + i3c_hub_enable_port(controller);
> + ret = i3c_master_do_daa(parent);
[Severity: Critical]
Since `i3c_master_do_daa()` executes the DAA sequence natively on the parent
controller, will the newly discovered devices be allocated and attached
directly to the parent bus instead of this virtual hub's bus?
If they are attached to the parent bus, won't subsequent transfers completely
bypass `i3c_hub_master_priv_xfers()`, causing communication with these
dynamically enumerated devices to fail?
[Severity: High]
Is there a risk of breaking electrical isolation here? `i3c_hub_enable_port()`
is called before acquiring any parent bus locks.
If multiple threads operate on different virtual controllers concurrently,
could they physically enable multiple downstream ports at the same time
before the bus lock is acquired?
> + i3c_hub_disable_port(controller);
> +
> + return ret;
> +}
[ ... ]
> +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> + struct i3c_master_controller *controller,
> + struct i3c_ccc_cmd *cmd)
> +{
> + struct i3c_master_controller *parent;
> + int ret;
> +
> + if (!hub || !hub->hub_dev)
> + return -ENODEV;
> +
> + parent = i3c_dev_get_master(hub->hub_dev->desc);
> + if (!parent)
> + return -ENODEV;
> +
> + i3c_hub_enable_port(controller);
> + ret = i3c_master_send_ccc_cmd(parent, cmd);
[Severity: High]
Similar to `i3c_hub_do_daa()`, does calling `i3c_hub_enable_port()` without a
lock risk enabling multiple ports concurrently if multiple CCC commands are
issued on different ports?
> + i3c_hub_disable_port(controller);
> +
> + return ret;
> +}
[ ... ]
> +static int i3c_hub_master_priv_xfers(struct i3c_dev_desc *dev,
> + struct i3c_xfer *xfers,
> + int nxfers,
> + enum i3c_xfer_mode mode)
> +{
[ ... ]
> + if (hub_addr != target_addr) {
> + down_write(&parent->bus.lock);
> + hub_dev->info.dyn_addr = target_addr;
> +
> + ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
> + up_write(&parent->bus.lock);
> + if (ret) {
> + hub_dev->info.dyn_addr = hub_addr;
> + goto disable;
> + }
> + }
> +
> + ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
[Severity: Critical]
Does dropping `parent->bus.lock` before calling `i3c_device_do_xfers()` expose
a race condition?
If multiple downstream ports initiate transfers concurrently, could they
overwrite the shared `hub_dev->info.dyn_addr` before the transfer completes,
causing packets to be routed to the wrong physical device?
> +
> + if (hub_addr != target_addr) {
> + down_write(&parent->bus.lock);
> + hub_dev->info.dyn_addr = hub_addr;
> +
> + xfer_ret = i3c_master_reattach_i3c_dev_locked(hub_dev,
> + target_addr);
[Severity: Critical]
Could passing `target_addr` as the old address corrupt the parent bus state?
`i3c_master_reattach_i3c_dev_locked()` frees the old address slot in the bus
slot map. Will this erroneously mark `target_addr` as `I3C_ADDR_SLOT_FREE`
on the parent bus?
> +
> + up_write(&parent->bus.lock);
> + if (!ret)
> + ret = xfer_ret;
> + }
> +disable:
> + i3c_hub_disable_port(controller);
> + return ret;
> +}
> +
> +static int i3c_hub_attach_i2c_dev(struct i2c_dev_desc *dev)
> +{
> + return 0;
> +}
[ ... ]
> +static int i3c_hub_i2c_xfers(struct i2c_dev_desc *dev,
> + struct i2c_msg *xfers, int nxfers)
> +{
> + return -EOPNOTSUPP;
> +}
[Severity: Medium]
Is it intentional to return `0` from `i3c_hub_attach_i2c_dev()` while rejecting
all I2C transfers with `-EOPNOTSUPP`?
Would it be safer to return an error from the attach callback to cleanly
prevent I2C driver binding if the hub does not support I2C forwarding?
[ ... ]
> +static int i3c_hub_master_send_ccc_cmd(struct i3c_master_controller *controller,
> + struct i3c_ccc_cmd *cmd)
> +{
> + struct i3c_hub_controller *hub_controller;
> + struct i3c_hub *hub;
> +
> + hub_controller = dev_get_drvdata(&controller->dev);
> + if (!hub_controller || !hub_controller->hub)
> + return -ENODEV;
> +
> + hub = hub_controller->hub;
> +
> + if (!hub->hub_dev)
> + return -ENODEV;
> +
> + if (cmd->id == I3C_CCC_RSTDAA(true))
> + return 0;
[Severity: High]
Does silently dropping the broadcast RSTDAA command prevent downstream devices
from being correctly reset by the hub?
If the command is dropped instead of being forwarded to the downstream ports,
will devices retain their old dynamic addresses and fail to re-initialize
during hot-plug recovery?
> +
> + return i3c_hub_send_ccc_cmd(hub, controller, cmd);
> +}
[ ... ]
> +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> + const struct i3c_ibi_setup *req)
> +{
[ ... ]
> + ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
> + if (ret) {
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> + up_write(&parent->bus.lock);
> + return ret;
> + }
> +
> + kfree(desc->ibi);
[Severity: Critical]
Does freeing `desc->ibi` here leak `desc->ibi->wq`?
When the core function `i3c_dev_request_ibi_locked()` allocates `ibi`, it also
allocates `ibi->wq`.
Also, if the inner call to `i3c_dev_request_ibi_locked()` fails, this function
returns an error to the original core caller. Will the outer caller's error
path execute `kfree(ibi)` on its local variable, resulting in a double-free
since it was already freed here?
> + desc->ibi = NULL;
> + ret = i3c_dev_request_ibi_locked(desc, req);
> + if (ret)
> + i3c_master_direct_detach_i3c_dev_locked(desc);
> +
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +
> + up_write(&parent->bus.lock);
> +
> + return ret;
> +}
> +
> +static void i3c_hub_free_ibi(struct i3c_dev_desc *desc)
> +{
[ ... ]
> + i3c_hub_enable_port(controller);
> +
> + down_write(&parent->bus.lock);
> + orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> + i3c_master_direct_detach_i3c_dev_locked(desc);
> + i3c_dev_free_ibi_locked(desc);
[Severity: Critical]
Can this recursive call to `i3c_dev_free_ibi_locked(desc)` cause a null
pointer dereference?
When the core initially calls `i3c_dev_free_ibi_locked(desc)`, it invokes
this hub callback. The inner call here successfully frees `desc->ibi` and sets
it to null.
When control returns to the outer core function, it unconditionally evaluates:
if (dev->ibi->wq) {
Will this immediately dereference the null pointer?
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> + up_write(&parent->bus.lock);
> +
> + i3c_hub_disable_port(controller);
> +}
[ ... ]
> +static void i3c_hub_recycle_ibi_slot(struct i3c_dev_desc *desc,
> + struct i3c_ibi_slot *slot)
> +{
> +}
[Severity: Critical]
Does leaving this function empty permanently leak hardware IBI slots?
When the core finishes processing an IBI, it returns the slot to the virtual
controller. Because this doesn't forward the recycle request to the parent
hardware controller, will the parent controller eventually exhaust its hardware
slots and permanently stop processing IBIs?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=5
^ permalink raw reply
* Re: [PATCH v12 6/7] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
From: sashiko-bot @ 2026-06-17 11:18 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: devicetree, robh, linux-i3c, Frank.Li, conor+dt
In-Reply-To: <20260617110355.1591844-7-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The SMBus transaction wait logic uses a fixed sleep instead of polling the hardware completion flag, leading to premature reads of incomplete transactions and data corruption.
- [High] Overwriting the parent device's `of_node` pointer iteratively causes a memory leak of the original parent node, a double-free of the target port nodes, and state corruption.
- [Medium] Potential `of_node` reference leak in `p3h2x4x_get_target_port_dt_conf()` if multiple child nodes specify the same target port index.
- [Medium] The hub's protected registers remain permanently unlocked if the hardware configuration or bus registration fails during probe.
- [Low] Comment describes a read followed by a write, but the code implements a write followed by a read.
--
commit 1c982ecf71c6fb3b765caf255191a821a5e37cb5
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
Add I3C hub functionality for the NXP P3H2x4x family of multiport hubs.
These devices support downstream target ports that can be configured
as I3C, I2C, or SMBus.
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> new file mode 100644
> index 0000000000000..05e19bff20d5c
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
[ ... ]
> +static void p3h2x4x_get_target_port_dt_conf(struct device *dev,
> + const struct device_node *node)
> +{
> + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> + u64 tp_port;
> +
> + for_each_available_child_of_node_scoped(node, dev_node) {
> + if (of_property_read_reg(dev_node, 0, &tp_port, NULL))
> + continue;
> +
> + if (tp_port < P3H2X4X_TP_MAX_COUNT) {
> + p3h2x4x_i3c_hub->tp_bus[tp_port].of_node = of_node_get(dev_node);
[Severity: Medium]
Could this leak the of_node reference if a malformed device tree specifies
multiple child nodes with the same target port index?
If multiple child nodes specify the same reg value, the pointer is
overwritten with a new of_node_get() result, which abandons the previous
reference.
> + p3h2x4x_i3c_hub->tp_bus[tp_port].tp_mask = P3H2X4X_SET_BIT(tp_port);
> + p3h2x4x_i3c_hub->tp_bus[tp_port].p3h2x4x_i3c_hub = p3h2x4x_i3c_hub;
> + p3h2x4x_i3c_hub->tp_bus[tp_port].tp_port = tp_port;
> + }
> + }
> +}
[ ... ]
> +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> +{
[ ... ]
> + /* Unlock access to protected registers */
> + ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> + P3H2X4X_REGISTERS_UNLOCK_CODE);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to unlock HUB's protected registers\n");
> +
> + ret = p3h2x4x_configure_hw(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to configure the HUB\n");
[Severity: Medium]
Are the hub's protected registers left permanently unlocked if probe fails
during p3h2x4x_configure_hw() or bus registration?
The driver unlocks the hardware registers at the start of probe and locks
them at the very end. However, if any initialization step fails and returns
an error, the function exits immediately without locking the registers again.
Could this be mitigated by using a devm_add_action_or_reset() callback or
explicit error handling labels to ensure the registers are safely relocked?
> +
> + /* Register virtual I3C master controllers for I3C target ports */
> + if (p3h2x4x->i3cdev) {
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> new file mode 100644
> index 0000000000000..8adb235b992c0
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> +{
[ ... ]
> + for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> + if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> + p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
> + continue;
> +
> + /* Assign DT node for this TP */
> + p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;
[Severity: High]
Does overwriting the parent device's of_node pointer iteratively cause a
memory leak of the original parent node and a double-free of the target port
nodes?
The loop over target ports directly mutates the shared pdev->dev.of_node
pointer which was populated in probe using device_set_of_node_from_dev().
This leaks the reference to the original parent node, and leaves
dev->of_node pointing to the last target port node.
When the driver is unbound, the driver core will drop a reference on
dev->of_node, but p3h2x4x_put_target_port_of_nodes() also drops a reference
on the same node, which can cause a use-after-free or double-free refcount
underflow. Mutating dev->of_node also creates a race with sysfs/uevent.
Should the driver assign the node to controller->dev.of_node directly
instead of mutating the parent's pointer?
> +
> + struct i3c_hub_controller *hub_controller =
> + &p3h2x4x_hub->tp_bus[tp].hub_controller;
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> new file mode 100644
> index 0000000000000..cf664425af071
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> +static int p3h2x4x_read_smbus_transaction_status(struct p3h2x4x_i3c_hub_dev *hub,
> + u8 target_port_status,
> + u8 data_length)
> +{
> + u32 status_read;
> + u8 status;
> + int ret;
> +
> + fsleep(P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length));
> +
> + ret = regmap_read(hub->regmap, target_port_status, &status_read);
[Severity: High]
Does using a hardcoded sleep based on bytes transferred instead of polling a
hardware completion status bit lead to premature reads of incomplete
transactions?
If the downstream device performs clock stretching or is slow to respond,
the transfer will take longer than the hardcoded timeout. The driver then
reads target_port_status and shifts to check the status bits.
Since an ongoing transaction leaves the status bits at 0
(P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK), the driver will incorrectly assume the
transaction has finished successfully. It will then proceed to read stale or
uninitialized memory buffers and return garbage data.
Could this be updated to use a polling loop checking the finish flag?
> + if (ret)
> + return ret;
> +
> + status = (u8)status_read;
[ ... ]
> +static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
[ ... ]
> + rw = (msgs[msg_count].flags & I2C_M_RD) ? 1 : 0;
> + if (!rw) {
> + /* If a read message is immediately followed by a write message to
> + * the same address, consider combining them into a single transaction.
> + */
[Severity: Low]
This isn't a bug, but the comment describes a read followed by a write while
the code implements a write followed by a read.
The actual code block checks !rw for the first message and flags & I2C_M_RD
for the second message, which accurately handles a write message immediately
followed by a read message.
> + if (msg_count + 1 < num &&
> + msgs[msg_count].addr == msgs[msg_count + 1].addr &&
> + (msgs[msg_count + 1].flags & I2C_M_RD)) {
> + if (msgs[msg_count].len + msgs[msg_count + 1].len >
> + P3H2X4X_SMBUS_PAYLOAD_SIZE)
> + return -EINVAL;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=6
^ permalink raw reply
* Re: [PATCH RFC v4 9/9] arm64: dts: qcom: glymur: Wire PCIe3a/3b to shared Gen5x8 PHY
From: Konrad Dybcio @ 2026-06-17 11:19 UTC (permalink / raw)
To: Qiang Yu, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Bjorn Andersson,
Konrad Dybcio
Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel
In-Reply-To: <20260518-link_mode_0519-v4-9-269cd73cc5d1@oss.qualcomm.com>
On 5/19/26 7:47 AM, Qiang Yu wrote:
> Glymur PCIe3 uses a single shared Gen5x8 QMP PHY block. Model PCIe3a and
> PCIe3b as consumers of that shared PHY provider instead of separate PHY
> nodes.
>
> Update the DTS wiring to:
> - point GCC PCIe3A/3B pipe parents to the shared PHY clock outputs
> - add PCIe3a controller node and route PCIe3a/PCIe3b port phys to
> &pcie3_phy using two-cell PHY arguments
> - configure the shared PHY node with link-mode and dual pipe outputs
>
> Use QMP_PCIE_GLYMUR_MODE_* dt-binding macros for mode selection.
>
> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> ---
[...]
> + pcie3a: pci@1c10000 {
> + device_type = "pci";
> + compatible = "qcom,glymur-pcie", "qcom,pcie-x1e80100";
> + reg = <0x0 0x01c10000 0x0 0x3000>,
> + <0x0 0x70000000 0x0 0xf20>,
> + <0x0 0x70000f40 0x0 0xa8>,
> + <0x0 0x70001000 0x0 0x4000>,
> + <0x0 0x70100000 0x0 0x100000>,
> + <0x0 0x01c13000 0x0 0x1000>;
> + reg-names = "parf",
> + "dbi",
> + "elbi",
> + "atu",
> + "config",
> + "mhi";
> + #address-cells = <3>;
> + #size-cells = <2>;
> + ranges = <0x01000000 0x0 0x00000000 0x0 0x70200000 0x0 0x100000>,
> + <0x02000000 0x0 0x70000000 0x0 0x70300000 0x0 0x3d00000>,
> + <0x03000000 0x7 0x00000000 0x7 0x00000000 0x0 0x40000000>,
> + <0x43000000 0x70 0x00000000 0x70 0x00000000 0x10 0x00000000>;
> +
> + bus-range = <0 0xff>;
> +
> + dma-coherent;
> +
> + linux,pci-domain = <3>;
> + num-lanes = <8>;
Is it fine to keep num-lanes 8 here even for configurations with
bifurcated PHY?
I would assume so, given essentially this is a x8 host, whose 4
lanes may simply be effectively NC
Konrad
^ permalink raw reply
* Re: [PATCH v3 2/7] gpio: regmap: add gpio_regmap_get_gpiochip() accessor
From: Michael Walle @ 2026-06-17 11:19 UTC (permalink / raw)
To: Yu-Chun Lin [林祐君], Bartosz Golaszewski,
Andy Shevchenko
Cc: linusw@kernel.org, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, afaerber@suse.com, wbg@kernel.org,
mathieu.dubois-briand@bootlin.com, lars@metafoo.de,
Michael.Hennerich@analog.com, jic23@kernel.org,
nuno.sa@analog.com, andy@kernel.org, dlechner@baylibre.com,
TY_Chang[張子逸], linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-realtek-soc@lists.infradead.org, linux-iio@vger.kernel.org,
CY_Huang[黃鉦晏],
Stanley Chang[昌育德],
James Tai [戴志峰]
In-Reply-To: <61c053a5a8e6461f9e6fcd40b6b5064d@realtek.com>
[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]
Hi,
On Wed Jun 17, 2026 at 11:54 AM CEST, Yu-Chun Lin [林祐君] wrote:
> Hi Michael,
>
>> Hi,
>>
>> On Wed Jun 17, 2026 at 10:36 AM CEST, Yu-Chun Lin [林祐君] wrote:
>>>>>>> Without an accessor like gpio_regmap_get_gpiochip(), we cannot
>>>>>>> retrieve the gpio_chip instantiated inside gpio-regmap.c to
>>>>>>> fulfill these requirements in our
>>>>>>> map() function.
>>>>
>>>> Why is gpiochip_irq_reqres() called in the first place? Isn't that
>>>> only called if the irq handling is set up via gc->irq.chip and not
>>>> via
>>>> gpiochip_irqchip_add_domain() like in gpio-regmap?
>>>>
>>>
>>> The panic was caused by my driver including
>>> 'GPIOCHIP_IRQ_RESOURCE_HELPERS', which forced the call to 'gpiochip_irq_reqres()' and crashed.
>>
>> But why did you use it if your irq domain isn't managed by the gpiolib, but rather your own >irq domain? Before going with option #3 I'd double check if that is correct in your driver.
>>
>> -michael
>
> Do you mean that a custom IRQ domain shouldn't be mixed with gpiolib features like
> 'GPIOCHIP_IRQ_RESOURCE_HELPERS'?
Honestly, I'm not sure. I've never done anything with irq domains
except for using the regmap_irq_chip. But from what I can tell is
that GPIOCHIP_IRQ_RESOURCE_HELPERS are tied to the handling with
gc->irq.chip, which isn't used at all if you add the domain via
gpiochip_irqchip_add_domain(). Please correct me if I'm wrong
though.
-michael
> Additional information: our GPIO controller receives 3 separate interrupt lines.
> Because the standard 'regmap_irq_chip' mechanism in 'gpio-regmap' does not support
> this multi-line hardware design, we are forced to create our own IRQ domain and pass
> it via 'config->irq_domain'.
>
> Given this constraint (that we must use our own IRQ domain), are you suggesting
> that we should implement our own 'irq_request_resources' and
> 'irq_release_resources' callbacks instead of relying on
> 'GPIOCHIP_IRQ_RESOURCE_HELPERS'?
>
> But if that is the case, we would much prefer to let the core gpiolib handle
> these resource and state management tasks for us *as proposed in option 3), rather
> than duplicating the effort in our driver.
>
> Best Regards,
> Yu-Chun
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]
^ permalink raw reply
* Re: [PATCH v12 1/7] i3c: master: Add APIs for I3C hub support
From: sashiko-bot @ 2026-06-17 11:20 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: Frank.Li, linux-i3c, conor+dt, devicetree, robh
In-Reply-To: <20260617110355.1591844-2-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Bypassing core state tracking in `direct_attach` / `direct_detach` causes Use-After-Free, NULL pointer dereferences, and Memory Leaks.
- [High] Bypassing address slot reservation in `direct_attach` leads to unrecoverable I3C bus address collisions.
--
commit ce08f4a6bc0d82c966d4e3337e6b4d082a9d09d2
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
i3c: master: Add APIs for I3C hub support
This commit introduces helper functions for attaching and detaching I3C
devices, alongside CCC command helpers. These additions prepare the core
framework for handling I3C hubs.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1775,6 +1775,70 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
[ ... ]
> +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> + struct i3c_dev_desc *dev)
> +{
> + int ret = 0;
> +
> + /*
> + * We don't attach devices to the controller until they are
> + * addressable on the bus.
> + */
> +
> + if (!dev->info.static_addr && !dev->info.dyn_addr)
> + return 0;
> +
> + /* Do not attach the master device itself. */
> + if (master->this != dev && master->ops->attach_i3c_dev)
> + ret = master->ops->attach_i3c_dev(dev);
[Severity: High]
Does bypassing address slot reservation here lead to I3C bus collisions?
In the standard attachment flow, i3c_master_get_i3c_addrs() is called to
reserve the address slot in the controller's bus.addrslots bitmap. Because
this direct helper skips that reservation, the parent controller's address
map is never updated to reflect the newly occupied address.
Since the parent controller and the virtual hub share the physical I3C bus,
the parent might still believe the downstream device's address is free. If
a new device hot-joins, could the parent controller allocate the exact same
address, resulting in an unrecoverable address collision?
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
> +
> +/**
> + * i3c_master_direct_detach_i3c_dev_locked() - Detach an I3C device from its
> + * master controller.
[ ... ]
> +void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
> +{
> + struct i3c_master_controller *master = i3c_dev_get_master(dev);
> +
> + /*
> + * We don't attach devices to the controller until they are
> + * addressable on the bus, so skip detach for unaddressed devices.
> + */
> + if (!dev->info.static_addr && !dev->info.dyn_addr)
> + return;
> +
> + /* Do not detach the master device itself. */
> + if (master->this != dev && master->ops->detach_i3c_dev)
> + master->ops->detach_i3c_dev(dev);
[Severity: Critical]
Can this lead to a use-after-free or a NULL pointer dereference by bypassing
the core state tracking?
When i3c_master_direct_detach_i3c_dev_locked() invokes the controller's
detach callback, master drivers typically free the master_priv data. However,
the dev->common.master_priv pointer is never cleared here.
If i3c_hub_free_ibi() calls this detach helper, and later a transfer is sent
via i3c_hub_master_priv_xfers():
drivers/i3c/hub.c:i3c_hub_master_priv_xfers() {
...
xfer_ret = i3c_master_priv_xfers(dev, xfers, nxfers, mode);
...
}
Will the parent controller end up dereferencing the freed master_priv
pointer?
Additionally, if a device is never explicitly attached via the direct helper
(leaving master_priv NULL), would a standard transfer immediately cause a
NULL pointer dereference when forwarded to the parent driver?
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=1
^ permalink raw reply
* Re: [PATCH v9 0/6] Add driver for EC found on Qualcomm reference devices
From: Anthony Ruhier @ 2026-06-17 11:23 UTC (permalink / raw)
To: Anvesh Jain P
Cc: Sibi Sankar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
Bjorn Andersson, Konrad Dybcio, Randy Dunlap, linux-arm-msm,
devicetree, linux-kernel, platform-driver-x86, Maya Matuszczyk,
Krzysztof Kozlowski, Dmitry Baryshkov, Konrad Dybcio,
Akhil P Oommen, Abel Vesa, Gaurav Kohli, Pankaj Patil
In-Reply-To: <20260511-add-driver-for-ec-v9-0-e5437c39b7f8@oss.qualcomm.com>
Tested-by: Anthony Ruhier <aruhier@mailbox.org>
Thanks, I tested it on the slim 7x (2024) on which I was previously using the
first patches from Maya, it works great!
I confirm that the following works: keyboard backlight shutdowns when
suspended, led blinks when suspended, keyboard fn shortcuts.
For the Slim 7x, I needed to add the following in the DTS:
---
diff --git a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
index b5247fb91299..423bf7340b83 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
@@ -1070,6 +1070,21 @@ retimer_ss0_con_sbu_out: endpoint {
};
};
+&i2c5 {
+ clock-frequency = <400000>;
+ status = "okay";
+
+ embedded-controller@76 {
+ compatible = "qcom,hamoa-crd-ec";
+ reg = <0x76>;
+
+ interrupts-extended = <&tlmm 66 IRQ_TYPE_EDGE_FALLING>;
+
+ pinctrl-0 = <&ec_int_n_default>;
+ pinctrl-names = "default";
+ };
+};
+
&i2c7 {
clock-frequency = <400000>;
@@ -1487,6 +1502,12 @@ reset-n-pins {
};
};
+ ec_int_n_default: ec-int-n-state {
+ pins = "gpio66";
+ function = "gpio";
+ bias-disable;
+ };
+
edp_reg_en: edp-reg-en-state {
pins = "gpio70";
function = "gpio";
--
Anthony Ruhier
^ permalink raw reply related
* Re: [PATCH v12 7/7] i3c: hub: p3h2x4x: Add SMBus slave mode support
From: sashiko-bot @ 2026-06-17 11:24 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: linux-i3c, Frank.Li, robh, conor+dt, devicetree
In-Reply-To: <20260617110355.1591844-8-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unconditional IBI request causes probe failure if the upstream I3C controller lacks IBI support.
- [High] Over-clearing buffer status flags on error paths discards pending data in the receive buffers.
- [High] DMA-from-stack vulnerabilities by allocating buffers on the stack and passing them to regmap bulk operations.
- [Medium] SMBus slave registration configures IBI without verifying if the upstream connection is I3C.
- [Medium] Missing SMBus agent status clear for unregistered ports leads to IBI storms.
- [Medium] Lockless read of `is_registered` in the IBI handler races with lockless write during SMBus adapter teardown.
--
commit 4badb83d7f4fe074334808f3d555ec26e7923604
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
i3c: hub: p3h2x4x: Add SMBus slave mode support
This commit adds SMBus slave mode support for the P3H2x4x hub target ports.
The hub SMBus slave agent can receive downstream payloads into target buffers
and report receive events through IBI, which are then forwarded to the
registered I2C slave client.
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> @@ -117,5 +135,25 @@ int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> p3h2x4x_hub->tp_bus[tp].is_registered = true;
> p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> }
> - return regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> + if (ret)
> + return ret;
[Severity: High]
Could this unconditional IBI request cause the probe to fail entirely if the
upstream I3C master controller does not support In-Band Interrupts?
If i3c_device_request_ibi() returns an error like -ENOTSUPP, it looks like
it aborts the entire initialization sequence for the hub.
Does this mean the core I3C/I2C target port routing functionality would break
on systems without IBI support merely because an optional SMBus slave feature
could not be enabled?
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> @@ -15,6 +15,146 @@ enum p3h2x4x_smbus_desc_idx {
> P3H2X4X_DESC_READ_LEN,
> };
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void p3h2x4x_read_smbus_agent_rx_buf(struct i3c_device *i3cdev, enum p3h2x4x_rcv_buf rfbuf,
> + enum p3h2x4x_tp tp, bool is_of)
> +{
> + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = i3cdev_get_drvdata(i3cdev);
> + u8 slave_rx_buffer[P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE] = { 0 };
> + u8 target_buffer_page, flag_clear = 0x0f, temp = 0, i, addr;
[Severity: High]
This isn't a bug strictly introduced by this patch, as a similar preexisting
flaw exists in p3h2x4x_tp_i2c_xfer_msg() with the desc array, but allocating
slave_rx_buffer on the stack and passing it to regmap bulk operations can
cause DMA-from-stack vulnerabilities.
Because the underlying regmap is backed by an I3C controller that may use DMA
for transfers, passing stack addresses is unsafe.
If CONFIG_VMAP_STACK is enabled, the stack memory is not guaranteed to be
physically contiguous, which could cause dma_map_single() to fail or corrupt
memory.
Should these buffers be dynamically allocated or pre-allocated in the device
structure instead?
> + u32 packet_len, slave_address, ret;
[ ... ]
> +ibi_err:
> + regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_PAGE_PTR, 0x00);
> + regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + tp, flag_clear);
> +}
[Severity: High]
Could this over-clear the buffer status flags on error paths and discard
pending data?
The local variable flag_clear is initialized to 0x0f, which matches
BUF_RECEIVED_FLAG_TF_MASK.
If an early error occurs before flag_clear is constrained to the specific port
buffer flag, it will write 0x0f to the status register.
Will this unconditionally clear all receive buffer flags, including buffer 0,
buffer 1, and overflow flags, for that port? If the other buffer contained a
valid pending message, would it be silently dropped?
> +
> +/**
> + * p3h2x4x_ibi_handler - IBI handler.
> + * @i3cdev: i3c device.
> + * @payload: two byte IBI payload data.
> + *
> + */
> +void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
> + const struct i3c_ibi_payload *payload)
> +{
[ ... ]
> + guard(mutex)(&p3h2x4x_i3c_hub->etx_mutex);
> +
> + for (i = 0; i < P3H2X4X_TP_MAX_COUNT; ++i) {
> + if (p3h2x4x_i3c_hub->tp_bus[i].is_registered && (payload_byte_two >> i) & 0x01) {
[Severity: Medium]
What happens if an IBI is triggered by a port that is not registered?
Since the loop skips unregistered ports entirely, the hardware interrupt source
at P3H2X4X_TP0_SMBUS_AGNT_STS is never read or cleared.
Does this mean the underlying hardware condition remains active, causing the
hub to immediately re-assert the IBI after the handler returns?
Could this result in an infinite loop of IBIs locking up the I3C bus, and
should the event status be cleared defensively even for unregistered ports?
[Severity: Medium]
Is there a data race here involving the read of is_registered?
During driver unbind or probe failure, p3h2x4x_unregister_smbus_adapters()
executes hub->tp_bus[tp].is_registered = false; without holding etx_mutex.
Because the devm action to free the IBI executes after the SMBus teardown
action, this new handler could be running concurrently and reading
is_registered under etx_mutex.
Since the writer does not hold the mutex, should this state modification be
properly synchronized?
> + ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + i,
[ ... ]
> @@ -205,11 +345,64 @@ static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
> }
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int p3h2x4x_tp_i2c_reg_slave(struct i2c_client *slave)
> +{
> + struct tp_bus *bus = i2c_get_adapdata(slave->adapter);
> + struct p3h2x4x_i3c_hub_dev *hub = bus->p3h2x4x_i3c_hub;
> + int ret;
> +
> + guard(mutex)(&hub->etx_mutex);
> +
> + if (bus->tp_smbus_client)
> + return -EBUSY;
> +
> + ret = regmap_set_bits(hub->regmap,
> + P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
> + bus->tp_mask);
[Severity: Medium]
Does this configure IBI without verifying if the upstream connection is I3C?
The registration enables P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG to route receive
events via IBI. However, it does not check if the hub is connected via an I3C
bus (i.e., whether hub->i3cdev is valid and an IBI handler was successfully
requested).
If the hub is connected via an I2C upstream bus, it cannot generate IBIs.
Enabling IBI generation on an I2C-connected hub would cause slave receive
events to be silently dropped since no handler is registered.
Should this function return -EOPNOTSUPP if !hub->i3cdev?
> + if (ret)
> + return ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=7
^ permalink raw reply
* Re: [PATCH v12 3/7] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
From: sashiko-bot @ 2026-06-17 11:24 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: Frank.Li, linux-i3c, conor+dt, devicetree, robh
In-Reply-To: <20260617110355.1591844-4-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The I3C driver incorrectly binds to any generic I3C Hub instead of specifically to NXP P3H2x4x devices.
--
commit e3f8613aba897f64df36ad159bade0cf0e0c5cf1
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
This commit adds core MFD support for the NXP P3H2x4x family of multiport
I3C hub devices. It registers the devices via I3C/I2C/SMBus and exposes
multiple downstream target ports and an on-die regulator.
> diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
> new file mode 100644
> index 0000000000000..0a80992d6250a
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c
[ ... ]
> +static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
> + I3C_CLASS(I3C_DCR_HUB, NULL),
[Severity: High]
Does this match table inappropriately bind the driver to any generic I3C Hub
instead of specifically NXP P3H2x4x devices?
Since p3h2x4x_device_probe_i3c() does not perform manufacturer or part ID
validation, could this claim non-NXP hubs and cause child drivers to send
proprietary NXP register sequences to them?
Should this use the I3C_DEVICE() macro with MIPI_VENDOR_ID_NXP instead?
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i3c, p3h2x4x_i3c_ids);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=3
^ permalink raw reply
* [PATCH v3 1/2] dt-bindings: rng: timeriomem_rng: add width and mask properties
From: Jad Keskes @ 2026-06-17 11:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Olivia Lu, Herbert Xu, Rob Herring, Alexander Clouter,
linux-crypto, devicetree, linux-kernel, Jad Keskes
Add optional width (8, 16, 32) and mask properties to the binding.
The width selects the bus access size for reads. The mask is ANDed
with the raw register value to allow only the entropy-bearing bits
through.
Update the example to show a typical 8-bit configuration.
Update SPDX to dual license to match kernel convention.
Drop the misleading '32-bit aligned' constraint from the reg
description since alignment now depends on the configured width.
Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
.../bindings/rng/timeriomem_rng.yaml | 48 +++++++++++++++----
1 file changed, 40 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
index 4754174e9849..636305f211c8 100644
--- a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
+++ b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
@@ -1,10 +1,16 @@
-# SPDX-License-Identifier: GPL-2.0-only
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/rng/timeriomem_rng.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: TimerIO Random Number Generator
+title: Timer IOMEM Hardware Random Number Generator
+
+description: |
+ This binding covers platforms that have a single IO memory address which
+ provides periodic random data. The driver reads from the address at a
+ fixed interval, returning a configurable-width value masked to the desired
+ bits.
maintainers:
- Krzysztof Kozlowski <krzk@kernel.org>
@@ -13,9 +19,17 @@ properties:
compatible:
const: timeriomem_rng
+ reg:
+ maxItems: 1
+ description:
+ Base address to sample from. Must be aligned to the configured access
+ width (1, 2, or 4 bytes) and at least that wide.
+
period:
$ref: /schemas/types.yaml#/definitions/uint32
- description: wait time in microseconds to use between samples
+ description:
+ Interval in microseconds between reads. New random data is expected to
+ be available at this rate.
quality:
$ref: /schemas/types.yaml#/definitions/uint32
@@ -26,16 +40,26 @@ properties:
instead. Note that the default quality is usually zero which disables
using this rng to automatically fill the kernel's entropy pool.
- reg:
- maxItems: 1
+ width:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 32
+ enum: [8, 16, 32]
description:
- Base address to sample from. Currently 'reg' must be at least four bytes
- wide and 32-bit aligned.
+ Access width in bits. Determines whether the read is performed as
+ an 8-bit, 16-bit, or 32-bit bus access.
+
+ mask:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 0xFFFFFFFF
+ description:
+ Mask applied to the value read from the register. Bits set to 0 in
+ the mask are cleared in the output data. Default (no mask) passes
+ all bits through.
required:
- compatible
- - period
- reg
+ - period
additionalProperties: false
@@ -46,3 +70,11 @@ examples:
reg = <0x44 0x04>;
period = <1000000>;
};
+
+ rng@64 {
+ compatible = "timeriomem_rng";
+ reg = <0x64 0x01>;
+ period = <50000>;
+ width = <8>;
+ mask = <0xFF>;
+ };
--
2.54.0
^ permalink raw reply related
* [PATCH v3 2/2] hw_random: timeriomem-rng: add configurable read width and data mask
From: Jad Keskes @ 2026-06-17 11:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Olivia Lu, Herbert Xu, Rob Herring, Alexander Clouter,
linux-crypto, devicetree, linux-kernel, Jad Keskes
In-Reply-To: <20260617112642.1897096-1-inasj268@gmail.com>
The TODO for supporting read sizes other than 32 bits and masking has
been sitting in this driver since 2009. Implement it.
Add width (8, 16, or 32 bits) and mask properties to the platform data
and device tree bindings. The read loop dispatches on width using
readb/readw/readl so a configured 8-bit access doesn't trigger a bus
error on hardware that rejects 32-bit reads to that address. The mask
is ANDed with the value before storing.
These are platform properties, not runtime policy -- width depends on
SoC integration, mask reflects which output bits carry entropy.
The alignment check in probe is updated to verify the resource is
aligned to the configured width instead of hardcoding 4-byte alignment.
Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
drivers/char/hw_random/timeriomem-rng.c | 78 ++++++++++++++++++++-----
include/linux/timeriomem-rng.h | 12 ++++
2 files changed, 77 insertions(+), 13 deletions(-)
diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
index e61f06393209..4557326618c9 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -14,7 +14,9 @@
* has to do is provide the address and 'wait time' that new data becomes
* available.
*
- * TODO: add support for reading sizes other than 32bits and masking
+ * The read width (8, 16, or 32 bits) and an optional data mask can be
+ * configured through platform data or device tree properties. Default is
+ * 32-bit reads with no mask.
*/
#include <linux/completion.h>
@@ -34,6 +36,8 @@ struct timeriomem_rng_private {
void __iomem *io_base;
ktime_t period;
unsigned int present:1;
+ unsigned int width;
+ u32 mask;
struct hrtimer timer;
struct completion completion;
@@ -48,6 +52,7 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
container_of(hwrng, struct timeriomem_rng_private, rng_ops);
int retval = 0;
int period_us = ktime_to_us(priv->period);
+ int chunk = priv->width / 8;
/*
* There may not have been enough time for new data to be generated
@@ -71,11 +76,28 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
usleep_range(period_us,
period_us + max(1, period_us / 100));
- *(u32 *)data = readl(priv->io_base);
- retval += sizeof(u32);
- data += sizeof(u32);
- max -= sizeof(u32);
- } while (wait && max > sizeof(u32));
+ switch (priv->width) {
+ case 8: {
+ u8 val = readb(priv->io_base) & priv->mask;
+ *(u8 *)data = val;
+ break;
+ }
+ case 16: {
+ u16 val = readw(priv->io_base) & priv->mask;
+ *(u16 *)data = val;
+ break;
+ }
+ case 32: {
+ u32 val = readl(priv->io_base) & priv->mask;
+ *(u32 *)data = val;
+ break;
+ }
+ }
+
+ retval += chunk;
+ data += chunk;
+ max -= chunk;
+ } while (wait && max > chunk);
/*
* Block any new callers until the RNG has had time to generate new
@@ -125,11 +147,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
if (IS_ERR(priv->io_base))
return PTR_ERR(priv->io_base);
- if (res->start % 4 != 0 || resource_size(res) < 4) {
- dev_err(&pdev->dev,
- "address must be at least four bytes wide and 32-bit aligned\n");
- return -EINVAL;
- }
+ priv->width = 32;
+ priv->mask = 0xFFFFFFFF;
if (pdev->dev.of_node) {
int i;
@@ -145,9 +164,42 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
if (!of_property_read_u32(pdev->dev.of_node,
"quality", &i))
priv->rng_ops.quality = i;
+
+ of_property_read_u32(pdev->dev.of_node,
+ "width", &priv->width);
+ of_property_read_u32(pdev->dev.of_node,
+ "mask", &priv->mask);
} else {
period = pdata->period;
priv->rng_ops.quality = pdata->quality;
+
+ if (pdata->width_set)
+ priv->width = pdata->width;
+ if (pdata->mask_set)
+ priv->mask = pdata->mask;
+ }
+
+ if (priv->width == 0)
+ priv->width = 32;
+
+ switch (priv->width) {
+ case 8:
+ case 16:
+ case 32:
+ break;
+ default:
+ dev_err(&pdev->dev, "invalid width %u, must be 8, 16, or 32\n",
+ priv->width);
+ return -EINVAL;
+ }
+
+ if (!IS_ALIGNED(res->start, priv->width / 8) ||
+ resource_size(res) < priv->width / 8) {
+ dev_err(&pdev->dev,
+ "address must be at least %u-bit aligned (%u byte%s)\n",
+ priv->width, priv->width / 8,
+ priv->width / 8 > 1 ? "s" : "");
+ return -EINVAL;
}
priv->period = us_to_ktime(period);
@@ -167,8 +219,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
return err;
}
- dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
- priv->io_base, period);
+ dev_info(&pdev->dev, "%ubit from %p @ %dus\n",
+ priv->width, priv->io_base, period);
return 0;
}
diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
index 672df7fbf6c1..b4202ad2f507 100644
--- a/include/linux/timeriomem-rng.h
+++ b/include/linux/timeriomem-rng.h
@@ -16,6 +16,18 @@ struct timeriomem_rng_data {
/* bits of entropy per 1024 bits read */
unsigned int quality;
+
+ /* read width (8, 16, or 32), 0 means 32 */
+ unsigned int width;
+
+ /* set to true if width is explicitly provided */
+ bool width_set;
+
+ /* mask applied to raw read value */
+ u32 mask;
+
+ /* set to true if mask is explicitly provided */
+ bool mask_set;
};
#endif /* _LINUX_TIMERIOMEM_RNG_H */
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 3/4] clk: qcom: Add EVA clock controller driver for Glymur SoC
From: Konrad Dybcio @ 2026-06-17 11:34 UTC (permalink / raw)
To: Taniya Das, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Brian Masney, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree
In-Reply-To: <20260617-evacc_glymur-v2-3-905108dacaaa@oss.qualcomm.com>
On 6/17/26 1:07 PM, Taniya Das wrote:
> Add the Enhanced Video Analytics (EVA) clock controller driver for
> the Glymur SoC. The EVACC manages the PLL, RCGs, branch clocks, GDSCs
> and resets for the EVA subsystem which handles vision processing
> workloads.
>
> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH 1/9] dt-bindings: nvmem: imx-ocotp: Add support for secure-enclave
From: Frieder Schrempf @ 2026-06-17 11:36 UTC (permalink / raw)
To: Krzysztof Kozlowski, Frieder Schrempf
Cc: Srinivas Kandagatla, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Shawn Guo, devicetree, imx, linux-arm-kernel,
linux-kernel
In-Reply-To: <20260617-prodigious-private-inchworm-beae1e@quoll>
On 17.06.26 12:49, Krzysztof Kozlowski wrote:
> On Tue, Jun 16, 2026 at 01:52:16PM +0200, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Some SoCs like the i.MX9 family allow full access to the fuses only
>> through the secure enclave firmware API. Add a property to reference
>> the secure enclave node and let the driver use the API.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>> Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml b/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml
>> index a8076d0e2737..14a6429f4a4c 100644
>> --- a/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml
>> +++ b/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml
>> @@ -53,6 +53,10 @@ properties:
>> reg:
>> maxItems: 1
>>
>> + secure-enclave:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: A phandle to the secure enclave node
>
> Two things here:
> 1. Here you describe what for is that phandle, how it is used by the
> hardware. Currently the description repeats the property name and type,
> so not much useful.
Ok, agree.
>
> 2. If you access OTP via firmware, then this is completely different
> interface than MMIO, thus:
> A. reg is not appropriate
> B. Device is very different thus it has different compatible and I even
> claim should be in different binding. Devices having completely
> different SW interface should not be in the same binding, at least
> usually.
>
> If any of above is not accurate, then your commit msg should answer why
> and give some background.
Thanks for the feedback!
The driver currently uses the limited MMIO (FSB) interface to access the
OTPs. The intention is to support the firmware interface alongside the
MMIO interface so the driver can pick the interface that is available
(firmware might not be loaded) and fallback to MMIO.
Following your argument would mean a driver deciding by itself which
interface to use at runtime is not something we want to have in general,
right?
In turn this would mean we need two drivers, or at least two
compatibles/bindings for something that is effectively the same hardware.
Actually, my first RFC approach [1] was to create a separate driver. But
in the end it seemed very weird to have two drivers and two DT nodes for
the same hardware block. Also I have no idea what happens if both
interfaces are used at the same time.
The other idea from back then was to replace the MMIO (FSB) interface
with ELE, but this would mean that we rely on the proprietary ELE
firmware to be available for simple things like reading a MAC address,
which is not desirable either, I guess.
In which direction should I move on with this?
[1]
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250416142715.1042363-1-frieder@fris.de/
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: rng: timeriomem_rng: add width and mask properties
From: sashiko-bot @ 2026-06-17 11:37 UTC (permalink / raw)
To: Jad Keskes; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260617112642.1897096-1-inasj268@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Use of non-standard `width` property instead of `reg-io-width`.
--
commit 7dac0db93dcf3017652753bebd5c0a2a7a2a06b5
Author: Jad Keskes <inasj268@gmail.com>
dt-bindings: rng: timeriomem_rng: add width and mask properties
Add optional width (8, 16, 32) and mask properties to the binding. The width
selects the bus access size for reads, while the mask allows filtering out
non-entropy-bearing bits.
> diff --git a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
> index 4754174e9849b..636305f211c88 100644
> --- a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
> +++ b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
[ ... ]
> @@ -26,16 +40,26 @@ properties:
> instead. Note that the default quality is usually zero which disables
> using this rng to automatically fill the kernel's entropy pool.
>
> - reg:
> - maxItems: 1
> + width:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 32
> + enum: [8, 16, 32]
> description:
> - Base address to sample from. Currently 'reg' must be at least four bytes
> - wide and 32-bit aligned.
> + Access width in bits. Determines whether the read is performed as
> + an 8-bit, 16-bit, or 32-bit bus access.
[Severity: Low]
Should this binding use the standard reg-io-width property (representing the
width in bytes) instead of introducing a custom width property in bits?
Using the established reg-io-width standard prevents fragmentation of the
device tree API conventions for specifying memory-mapped I/O access width.
> +
> + mask:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 0xFFFFFFFF
> + description:
> + Mask applied to the value read from the register. Bits set to 0 in
> + the mask are cleared in the output data. Default (no mask) passes
> + all bits through.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617112642.1897096-1-inasj268@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v3 2/2] hw_random: timeriomem-rng: add configurable read width and data mask
From: sashiko-bot @ 2026-06-17 11:41 UTC (permalink / raw)
To: Jad Keskes; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260617112642.1897096-2-inasj268@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free / Deadlock in the device removal path due to incorrect devres ordering.
--
commit 4300c70f186763a1c545b060ff78e036f3434d38
Author: Jad Keskes <inasj268@gmail.com>
hw_random: timeriomem-rng: add configurable read width and data mask
This commit implements configurable read widths (8, 16, or 32 bits) and data
masking for the timeriomem-rng driver. It updates the read loop to dispatch
based on width to avoid bus errors, and uses properties from platform data or
device tree.
> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> index e61f063932090..4557326618c90 100644
> --- a/drivers/char/hw_random/timeriomem-rng.c
> +++ b/drivers/char/hw_random/timeriomem-rng.c
[ ... ]
> @@ -167,8 +219,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
> return err;
> }
>
> - dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
> - priv->io_base, period);
> + dev_info(&pdev->dev, "%ubit from %p @ %dus\n",
> + priv->width, priv->io_base, period);
>
> return 0;
> }
[Severity: High]
This isn't a bug introduced by this patch, but looking at the probe function
above, it uses devm_hwrng_register() to register the RNG device. This means
that hwrng_unregister() is automatically called by devres after
timeriomem_rng_remove() returns.
In timeriomem_rng_remove(), the driver manually calls
hrtimer_cancel(&priv->timer). At this point, the RNG is still registered and
fully exposed to concurrent readers. This ordering creates two distinct issues:
1. Deadlock: If a concurrent reader is inside timeriomem_rng_read() and
blocking on wait_for_completion(&priv->completion), it will wait forever
because the timer was just cancelled. When devres subsequently calls
hwrng_unregister(), it waits indefinitely for the reader's reference count to
drop, deadlocking the unbind process.
2. Use-After-Free: If a concurrent reader finishes its read loop just after
remove() cancelled the timer, it unconditionally executes
hrtimer_restart(&priv->timer) in the read path:
timeriomem_rng_read() {
...
priv->present = 0;
reinit_completion(&priv->completion);
hrtimer_forward_now(&priv->timer, priv->period);
hrtimer_restart(&priv->timer);
...
}
The reader then completes, remove() finishes, and devres frees the priv
structure. The re-armed timer will then fire and dereference the freed priv
pointer.
Could this devres ordering issue be addressed by manually unregistering the
hwrng before cancelling the timer in remove(), or by using a custom devm
action to ensure correct teardown order?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617112642.1897096-1-inasj268@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: mtd: qcom,nandc: Add MDM9607 QPIC NAND controller
From: Konrad Dybcio @ 2026-06-17 11:42 UTC (permalink / raw)
To: Stephan Gerhold, Miquel Raynal, Manivannan Sadhasivam,
Kathiravan Thirumoorthy
Cc: Krzysztof Kozlowski, Manivannan Sadhasivam, Richard Weinberger,
Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-mtd, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <aifktn_s2qmg7MzI@linaro.org>
On 6/9/26 12:02 PM, Stephan Gerhold wrote:
> On Tue, Jun 09, 2026 at 11:30:54AM +0200, Miquel Raynal wrote:
>> On 09/06/2026 at 11:08:03 +02, Stephan Gerhold <stephan.gerhold@linaro.org> wrote:
>>
>>> On Tue, Jun 09, 2026 at 11:01:18AM +0200, Konrad Dybcio wrote:
>>>> On 6/9/26 10:55 AM, Konrad Dybcio wrote:
>>>>> On 6/9/26 10:10 AM, Stephan Gerhold wrote:
>>>>>> On Tue, Jun 09, 2026 at 09:52:51AM +0200, Miquel Raynal wrote:
>>>>>>>>> On MDM9607, there is only a single controllable clock for the NAND
>>>>>>>>> controller (RPM_SMD_QPIC_CLK). The same situation also applies e.g. for
>>>>>>>>> qcom,sdx55-nand, but the corresponding device tree (qcom-sdx55.dtsi) works
>>>>>>>>> around that by assigning a dummy clock (&nand_clk_dummy) to the second
>>>>>>>>> clock ("aon") that is required by the dt-bindings. This is not really
>>>>>>>>> useful, so avoid doing that for new platforms by excluding the second "aon"
>>>>>>>>> clock entry in the dt-bindings.
>>>>>>>>
>>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>>>>>>>
>>>>>>> What is the problem in giving twice the same clock? If this is what is
>>>>>>> done in the hardware routing, I do not see the reason for more
>>>>>>> complexity in the binding?
>>>>>>>
>>>>>>
>>>>>> I had that in my first draft for this series, but this would be wrong
>>>>>> IMO. I suspect there is no QPIC/NAND related "aon" (always-on) clock on
>>>>>> this platform at all. I'm not sure about MDM9607 in particular (maybe
>>>>>> someone from Qualcomm can confirm), but a similar platform I was looking
>>>>>> into at some point actually had *3* separate clocks for QPIC in the
>>>>>> hardware and none of them were called "aon" ...
>>>>>
>>>>> gcc_qpic_ahb_clk (50/100/133.(3) MHz sourced from PCNoC_bfdcd_clk_src)
>>>>> gcc_qpic_clk (likewise, sourced from qpic_clk_src which is sourced
>>>>> from GPLLs)
>>>>> gcc_qpic_system_clk (32 KHz)
>>>>>
>>>>> No clock containing the substring 'aon' in its name on this platform
>>>>
>>>> Looking at SDX65, perhaps the 32 Khz clock is the "aon" one after all..
>>>> The NAND documentation says
>>>>
>>>> CC_QPIC_SYSTEM_CLK - Always-on timeout clock (32 KHz)
>>>>
>>>
>>> Thanks for looking this up.
>>>
>>> IMO, if we want to describe the actual hardware routing, we should
>>> describe all 3 clocks and assign all of them to RPM_SMD_QPIC_CLK for
>>> MDM9607).
>>
>> Sounds more accurate to me.
>>
>>> The resulting diff would be basically the same as this patch just
>>> inversed (3 clocks for MDM9607+SDX(?) and 2 clocks for the IPQ* SoCs.
>>
>> Diff would not be simpler but more accurate. So if we go for a
>> modification of the bindings, I would prefer that path.
>>
>
> IMO the result wouldn't be much more accurate from the perspective of
> the kernel. If we assign RPM_SMD_QPIC_CLK to all 3 clocks we would be
> effectively saying "there is a single clock with a single rate that is
> sourcing 'core', 'ahb' and 'system'(/'aon')". But in reality, these are
> 3 separate clock domains with separate rates, as shown by Konrad above.
>
> We could try defining dummy clocks like the &nand_clk_dummy in
> qcom-sdx55.dtsi, but this isn't very accurate either. Presumably, all of
> these clocks are toggled by RPM_SMD_QPIC_CLK. So if we define a dummy
> clock for 'ahb', then enabling that clock without also enabling the
> non-dummy 'core' (RPM_SMD_QPIC_CLK) will do nothing.
I can't find a good answer for what RPM_SMD_QPIC_CLK controls, maybe
+Mani or +Kathiravan know where to look
Konrad
>
> At the end, the truth for the OS/kernel running on this hardware is that
> it can only see the 'core' clock (with the option to change its rate).
> All others are invisible, with no way to influence or check the status,
> so pretending that we have separate resources for them doesn't really
> make things more accurate in my opinion.
>
> But yeah, let's leave the decision up to Krzysztof. I'm happy to change
> this patch as needed as long it works at the end. :-)
^ 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