[PATCH v1 00/12] Enable I2C on SA8255p Qualcomm platforms

[PATCH v1 00/12] Enable I2C on SA8255p Qualcomm platforms

[Praveen Talari]

The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power
and performance protocols.

The SCMI power protocol enables or disables resources like clocks,
interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
such as resume/suspend, to control power states(on/off).

The SCMI performance protocol manages I2C frequency, with each
frequency rate represented by a performance level. The driver uses
geni_se_set_perf_opp() API to request the desired frequency rate..

As part of geni_se_set_perf_opp(), the OPP for the requested frequency
is obtained using dev_pm_opp_find_freq_floor() and the performance
level is set using dev_pm_opp_set_opp().

Praveen Talari (12):
  soc: qcom: geni-se: Refactor geni_icc_get() and make qup-memory ICC
    path optional
  soc: qcom: geni-se: Add geni_icc_set_bw_ab() function
  soc: qcom: geni-se: Introduce helper API for resource initialization
  soc: qcom: geni-se: Add geni_se_resource_state() helper
  soc: qcom: geni-se: Introduce helper API for attaching power domains
  soc: qcom: geni-se: Introduce helper APIs for performance control
  dt-bindings: i2c: Describe SA8255p
  i2c: qcom-geni: Isolate serial engine setup
  i2c: qcom-geni: Move resource initialization to separate function
  i2c: qcom-geni: Use geni_se_resources_state() API in runtime PM
    functions
  i2c: qcom-geni: Store of_device_id data in driver private struct
  i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms

 .../bindings/i2c/qcom,sa8255p-geni-i2c.yaml   |  61 ++++
 drivers/i2c/busses/i2c-qcom-geni.c            | 287 +++++++++---------
 drivers/soc/qcom/qcom-geni-se.c               | 245 +++++++++++++--
 include/linux/soc/qcom/geni-se.h              |  17 ++
 4 files changed, 447 insertions(+), 163 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml


base-commit: d724c6f85e80a23ed46b7ebc6e38b527c09d64f5
-- 
2.34.1

[PATCH v1 01/12] soc: qcom: geni-se: Refactor geni_icc_get() and make qup-memory ICC path optional

[Praveen Talari]

Refactor the geni_icc_get() function to replace the loop-based ICC path
initialization with explicit handling of each interconnect path. This
improves code readability and allows for different error handling per
path type.

The "qup-core" and "qup-config" paths remain mandatory, while "qup-memory"
is now optional and skipped if not defined in DT.

Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c | 36 +++++++++++++++++----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index cd1779b6a91a..b6167b968ef6 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -899,30 +899,32 @@ EXPORT_SYMBOL_GPL(geni_se_rx_dma_unprep);
 
 int geni_icc_get(struct geni_se *se, const char *icc_ddr)
 {
-	int i, err;
-	const char *icc_names[] = {"qup-core", "qup-config", icc_ddr};
+	struct geni_icc_path *icc_paths = se->icc_paths;
 
 	if (has_acpi_companion(se->dev))
 		return 0;
 
-	for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) {
-		if (!icc_names[i])
-			continue;
-
-		se->icc_paths[i].path = devm_of_icc_get(se->dev, icc_names[i]);
-		if (IS_ERR(se->icc_paths[i].path))
-			goto err;
+	icc_paths[GENI_TO_CORE].path = devm_of_icc_get(se->dev, "qup-core");
+	if (IS_ERR(icc_paths[GENI_TO_CORE].path))
+		return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_CORE].path),
+				     "Failed to get 'qup-core' ICC path\n");
+
+	icc_paths[CPU_TO_GENI].path = devm_of_icc_get(se->dev, "qup-config");
+	if (IS_ERR(icc_paths[CPU_TO_GENI].path))
+		return dev_err_probe(se->dev, PTR_ERR(icc_paths[CPU_TO_GENI].path),
+				     "Failed to get 'qup-config' ICC path\n");
+
+	/* The DDR path is optional, depending on protocol and hw capabilities */
+	icc_paths[GENI_TO_DDR].path = devm_of_icc_get(se->dev, "qup-memory");
+	if (IS_ERR(icc_paths[GENI_TO_DDR].path)) {
+		if (PTR_ERR(icc_paths[GENI_TO_DDR].path) == -ENODATA)
+			icc_paths[GENI_TO_DDR].path = NULL;
+		else
+			return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_DDR].path),
+					     "Failed to get 'qup-memory' ICC path\n");
 	}
 
 	return 0;
-
-err:
-	err = PTR_ERR(se->icc_paths[i].path);
-	if (err != -EPROBE_DEFER)
-		dev_err_ratelimited(se->dev, "Failed to get ICC path '%s': %d\n",
-					icc_names[i], err);
-	return err;
-
 }
 EXPORT_SYMBOL_GPL(geni_icc_get);
 
-- 
2.34.1

[PATCH v1 02/12] soc: qcom: geni-se: Add geni_icc_set_bw_ab() function

[Praveen Talari]

Add a new function geni_icc_set_bw_ab() that allows callers to set
average bandwidth values for all ICC (Interconnect) paths in a single
call. This function takes separate parameters for core, config, and DDR
average bandwidth values and applies them to the respective ICC paths.

This provides a more convenient API for drivers that need to configure
specific average bandwidth values.

Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c  | 22 ++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index b6167b968ef6..b0542f836453 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -946,6 +946,28 @@ int geni_icc_set_bw(struct geni_se *se)
 }
 EXPORT_SYMBOL_GPL(geni_icc_set_bw);
 
+/**
+ * geni_icc_set_bw_ab() - Set average bandwidth for all ICC paths and apply
+ * @se:		Pointer to the concerned serial engine.
+ * @core_ab:	Average bandwidth in kBps for GENI_TO_CORE path.
+ * @cfg_ab:	Average bandwidth in kBps for CPU_TO_GENI path.
+ * @ddr_ab:	Average bandwidth in kBps for GENI_TO_DDR path.
+ *
+ * Sets bandwidth values for all ICC paths and applies them. DDR path is
+ * optional and only set if it exists.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int geni_icc_set_bw_ab(struct geni_se *se, u32 core_ab, u32 cfg_ab, u32 ddr_ab)
+{
+	se->icc_paths[GENI_TO_CORE].avg_bw = core_ab;
+	se->icc_paths[CPU_TO_GENI].avg_bw = cfg_ab;
+	se->icc_paths[GENI_TO_DDR].avg_bw = ddr_ab;
+
+	return geni_icc_set_bw(se);
+}
+EXPORT_SYMBOL_GPL(geni_icc_set_bw_ab);
+
 void geni_icc_set_tag(struct geni_se *se, u32 tag)
 {
 	int i;
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 0a984e2579fe..980aabea2157 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -528,6 +528,7 @@ void geni_se_rx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len);
 int geni_icc_get(struct geni_se *se, const char *icc_ddr);
 
 int geni_icc_set_bw(struct geni_se *se);
+int geni_icc_set_bw_ab(struct geni_se *se, u32 core_ab, u32 cfg_ab, u32 ddr_ab);
 void geni_icc_set_tag(struct geni_se *se, u32 tag);
 
 int geni_icc_enable(struct geni_se *se);
-- 
2.34.1

[PATCH v1 03/12] soc: qcom: geni-se: Introduce helper API for resource initialization

[Praveen Talari]

The GENI Serial Engine drivers (I2C, SPI, and SERIAL) currently duplicate
code for initializing shared resources such as clocks and interconnect
paths.

Introduce a new helper API, geni_se_resources_init(), to centralize this
initialization logic, improving modularity and simplifying the probe
function.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c  | 47 ++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h |  6 ++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index b0542f836453..726b77650007 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -19,6 +19,7 @@
 #include <linux/of_platform.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/soc/qcom/geni-se.h>
 
 /**
@@ -1012,6 +1013,52 @@ int geni_icc_disable(struct geni_se *se)
 }
 EXPORT_SYMBOL_GPL(geni_icc_disable);
 
+/**
+ * geni_se_resources_init() - Initialize resources for a GENI SE device.
+ * @se: Pointer to the geni_se structure representing the GENI SE device.
+ *
+ * This function initializes various resources required by the GENI Serial Engine
+ * (SE) device, including clock resources (core and SE clocks), interconnect
+ * paths for communication.
+ * It retrieves optional and mandatory clock resources, adds an OF-based
+ * operating performance point (OPP) table, and sets up interconnect paths
+ * with default bandwidths. The function also sets a flag (`has_opp`) to
+ * indicate whether OPP support is available for the device.
+ *
+ * Return: 0 on success, or a negative errno on failure.
+ */
+int geni_se_resources_init(struct geni_se *se)
+{
+	int ret;
+
+	se->core_clk = devm_clk_get_optional(se->dev, "core");
+	if (IS_ERR(se->core_clk))
+		return dev_err_probe(se->dev, PTR_ERR(se->core_clk),
+				     "Failed to get optional core clk\n");
+
+	se->clk = devm_clk_get(se->dev, "se");
+	if (IS_ERR(se->clk) && !has_acpi_companion(se->dev))
+		return dev_err_probe(se->dev, PTR_ERR(se->clk),
+				     "Failed to get SE clk\n");
+
+	ret = devm_pm_opp_set_clkname(se->dev, "se");
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = devm_pm_opp_of_add_table(se->dev);
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(se->dev, ret, "Failed to add OPP table\n");
+
+	se->has_opp = (ret == 0);
+
+	ret = geni_icc_get(se, "qup-memory");
+	if (ret)
+		return ret;
+
+	return geni_icc_set_bw_ab(se, GENI_DEFAULT_BW, GENI_DEFAULT_BW, GENI_DEFAULT_BW);
+}
+EXPORT_SYMBOL_GPL(geni_se_resources_init);
+
 /**
  * geni_find_protocol_fw() - Locate and validate SE firmware for a protocol.
  * @dev: Pointer to the device structure.
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 980aabea2157..c182dd0f0bde 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -60,18 +60,22 @@ struct geni_icc_path {
  * @dev:		Pointer to the Serial Engine device
  * @wrapper:		Pointer to the parent QUP Wrapper core
  * @clk:		Handle to the core serial engine clock
+ * @core_clk:		Auxiliary clock, which may be required by a protocol
  * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
  * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
  * @icc_paths:		Array of ICC paths for SE
+ * @has_opp:		Indicates if OPP is supported
  */
 struct geni_se {
 	void __iomem *base;
 	struct device *dev;
 	struct geni_wrapper *wrapper;
 	struct clk *clk;
+	struct clk *core_clk;
 	unsigned int num_clk_levels;
 	unsigned long *clk_perf_tbl;
 	struct geni_icc_path icc_paths[3];
+	bool has_opp;
 };
 
 /* Common SE registers */
@@ -535,6 +539,8 @@ int geni_icc_enable(struct geni_se *se);
 
 int geni_icc_disable(struct geni_se *se);
 
+int geni_se_resources_init(struct geni_se *se);
+
 int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
 #endif
 #endif
-- 
2.34.1

[PATCH v1 04/12] soc: qcom: geni-se: Add geni_se_resource_state() helper

[Praveen Talari]

The GENI SE protocol drivers (I2C, SPI, UART) implement similar resource
activation/deactivation sequences independently, leading to code
duplication.

Introduce geni_se_resource_state() to control power state of GENI SE
resources. This function provides a unified interface that calls either
geni_se_resources_activate() to power on resources or
geni_se_resources_deactivate() to power off resources based on the
power_on parameter.

The activate function enables ICC, clocks, and TLMM with proper error
handling and cleanup paths. The deactivate function disables resources
in reverse order including OPP rate reset, clocks, ICC and TLMM.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c  | 61 ++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h |  2 ++
 2 files changed, 63 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 726b77650007..7aee7fd2e240 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1013,6 +1013,67 @@ int geni_icc_disable(struct geni_se *se)
 }
 EXPORT_SYMBOL_GPL(geni_icc_disable);
 
+static int geni_se_resources_deactivate(struct geni_se *se)
+{
+	int ret;
+
+	if (se->has_opp)
+		dev_pm_opp_set_rate(se->dev, 0);
+
+	ret = geni_se_resources_off(se);
+	if (ret)
+		return ret;
+
+	if (se->core_clk)
+		clk_disable_unprepare(se->core_clk);
+
+	return geni_icc_disable(se);
+}
+
+static int geni_se_resources_activate(struct geni_se *se)
+{
+	int ret;
+
+	ret = geni_icc_enable(se);
+	if (ret)
+		return ret;
+
+	if (se->core_clk) {
+		ret = clk_prepare_enable(se->core_clk);
+		if (ret)
+			goto out_icc_disable;
+	}
+
+	ret = geni_se_resources_on(se);
+	if (ret)
+		goto out_clk_disable;
+
+	return 0;
+
+out_clk_disable:
+	if (se->core_clk)
+		clk_disable_unprepare(se->core_clk);
+out_icc_disable:
+	geni_icc_disable(se);
+	return ret;
+}
+
+/**
+ * geni_se_resources_state() - Control power state of GENI SE resources
+ * @se: Pointer to the geni_se structure
+ * @power_on: Boolean flag for desired power state (true = on, false = off)
+ *
+ * Controls GENI SE resource power state by calling activate or deactivate
+ * functions based on the power_on parameter.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+int geni_se_resources_state(struct geni_se *se, bool power_on)
+{
+	return power_on ? geni_se_resources_activate(se) : geni_se_resources_deactivate(se);
+}
+EXPORT_SYMBOL_GPL(geni_se_resources_state);
+
 /**
  * geni_se_resources_init() - Initialize resources for a GENI SE device.
  * @se: Pointer to the geni_se structure representing the GENI SE device.
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index c182dd0f0bde..d1ca13a4e54c 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -541,6 +541,8 @@ int geni_icc_disable(struct geni_se *se);
 
 int geni_se_resources_init(struct geni_se *se);
 
+int geni_se_resources_state(struct geni_se *se, bool power_on);
+
 int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
 #endif
 #endif
-- 
2.34.1

[PATCH v1 05/12] soc: qcom: geni-se: Introduce helper API for attaching power domains

[Praveen Talari]

The GENI Serial Engine drivers (I2C, SPI, and SERIAL) currently handle
the attachment of power domains. This often leads to duplicated code
logic across different driver probe functions.

Introduce a new helper API, geni_se_domain_attach(), to centralize
the logic for attaching "power" and "perf" domains to the GENI SE
device.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c  | 29 +++++++++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h |  4 ++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 7aee7fd2e240..30b58f2f2e5d 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -19,6 +19,7 @@
 #include <linux/of_platform.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_opp.h>
 #include <linux/soc/qcom/geni-se.h>
 
@@ -1074,6 +1075,34 @@ int geni_se_resources_state(struct geni_se *se, bool power_on)
 }
 EXPORT_SYMBOL_GPL(geni_se_resources_state);
 
+/**
+ * geni_se_domain_attach() - Attach power domains to a GENI SE device.
+ * @se: Pointer to the geni_se structure representing the GENI SE device.
+ *
+ * This function attaches the necessary power domains ("power" and "perf")
+ * to the GENI Serial Engine device. It initializes `se->pd_list` with the
+ * attached domains.
+ *
+ * Return: 0 on success, or a negative error code on failure.
+ */
+int geni_se_domain_attach(struct geni_se *se)
+{
+	struct dev_pm_domain_attach_data pd_data = {
+		.pd_flags = PD_FLAG_DEV_LINK_ON,
+		.pd_names = (const char*[]) { "power", "perf" },
+		.num_pd_names = 2,
+	};
+	int ret;
+
+	ret = dev_pm_domain_attach_list(se->dev,
+					&pd_data, &se->pd_list);
+	if (ret <= 0)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(geni_se_domain_attach);
+
 /**
  * geni_se_resources_init() - Initialize resources for a GENI SE device.
  * @se: Pointer to the geni_se structure representing the GENI SE device.
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index d1ca13a4e54c..8c9b847aaf20 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -64,6 +64,7 @@ struct geni_icc_path {
  * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
  * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
  * @icc_paths:		Array of ICC paths for SE
+ * @pd_list:		Power domain list for managing power domains
  * @has_opp:		Indicates if OPP is supported
  */
 struct geni_se {
@@ -75,6 +76,7 @@ struct geni_se {
 	unsigned int num_clk_levels;
 	unsigned long *clk_perf_tbl;
 	struct geni_icc_path icc_paths[3];
+	struct dev_pm_domain_list *pd_list;
 	bool has_opp;
 };
 
@@ -544,5 +546,7 @@ int geni_se_resources_init(struct geni_se *se);
 int geni_se_resources_state(struct geni_se *se, bool power_on);
 
 int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
+
+int geni_se_domain_attach(struct geni_se *se);
 #endif
 #endif
-- 
2.34.1

[PATCH v1 06/12] soc: qcom: geni-se: Introduce helper APIs for performance control

[Praveen Talari]

The GENI Serial Engine (SE) drivers (I2C, SPI, and SERIAL) currently
manage performance levels and operating points directly. This resulting
in code duplication across drivers. such as configuring a specific level
or find and apply an OPP based on a clock frequency.

Introduce two new helper APIs, geni_se_set_perf_level() and
geni_se_set_perf_opp(), addresses this issue by providing a streamlined
method for the GENI Serial Engine (SE) drivers to find and set the OPP
based on the desired performance level, thereby eliminating redundancy.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c  | 50 ++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/geni-se.h |  4 +++
 2 files changed, 54 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 30b58f2f2e5d..292afa18b86c 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -282,6 +282,12 @@ struct se_fw_hdr {
 #define geni_setbits32(_addr, _v) writel(readl(_addr) |  (_v), _addr)
 #define geni_clrbits32(_addr, _v) writel(readl(_addr) & ~(_v), _addr)
 
+enum domain_idx {
+	DOMAIN_IDX_POWER,
+	DOMAIN_IDX_PERF,
+	DOMAIN_IDX_MAX
+};
+
 /**
  * geni_se_get_qup_hw_version() - Read the QUP wrapper Hardware version
  * @se:	Pointer to the corresponding serial engine.
@@ -1075,6 +1081,50 @@ int geni_se_resources_state(struct geni_se *se, bool power_on)
 }
 EXPORT_SYMBOL_GPL(geni_se_resources_state);
 
+/**
+ * geni_se_set_perf_level() - Set performance level for GENI SE.
+ * @se: Pointer to the struct geni_se instance.
+ * @level: The desired performance level.
+ *
+ * Sets the performance level by directly calling dev_pm_opp_set_level
+ * on the performance device associated with the SE.
+ *
+ * Return: 0 on success, or a negative error code on failure.
+ */
+int geni_se_set_perf_level(struct geni_se *se, unsigned long level)
+{
+	return dev_pm_opp_set_level(se->pd_list->pd_devs[DOMAIN_IDX_PERF], level);
+}
+EXPORT_SYMBOL_GPL(geni_se_set_perf_level);
+
+/**
+ * geni_se_set_perf_opp() - Set performance OPP for GENI SE by frequency.
+ * @se: Pointer to the struct geni_se instance.
+ * @clk_freq: The requested clock frequency.
+ *
+ * Finds the nearest operating performance point (OPP) for the given
+ * clock frequency and applies it to the SE's performance device.
+ *
+ * Return: 0 on success, or a negative error code on failure.
+ */
+int geni_se_set_perf_opp(struct geni_se *se, unsigned long clk_freq)
+{
+	struct device *perf_dev = se->pd_list->pd_devs[DOMAIN_IDX_PERF];
+	struct dev_pm_opp *opp;
+	int ret;
+
+	opp = dev_pm_opp_find_freq_floor(perf_dev, &clk_freq);
+	if (IS_ERR(opp)) {
+		dev_err(se->dev, "failed to find opp for freq %lu\n", clk_freq);
+		return PTR_ERR(opp);
+	}
+
+	ret = dev_pm_opp_set_opp(perf_dev, opp);
+	dev_pm_opp_put(opp);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(geni_se_set_perf_opp);
+
 /**
  * geni_se_domain_attach() - Attach power domains to a GENI SE device.
  * @se: Pointer to the geni_se structure representing the GENI SE device.
diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 8c9b847aaf20..cac999d6ca31 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -548,5 +548,9 @@ int geni_se_resources_state(struct geni_se *se, bool power_on);
 int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
 
 int geni_se_domain_attach(struct geni_se *se);
+
+int geni_se_set_perf_level(struct geni_se *se, unsigned long level);
+
+int geni_se_set_perf_opp(struct geni_se *se, unsigned long clk_freq);
 #endif
 #endif
-- 
2.34.1

[PATCH v1 07/12] dt-bindings: i2c: Describe SA8255p

[Praveen Talari]

Add DT bindings for the QUP GENI I2C controller on sa8255p platforms.

SA8255p platform abstracts resources such as clocks, interconnect and
GPIO pins configuration in Firmware. SCMI power and perf protocol
are utilized to request resource configurations.

Co-developed-by: Nikunj Kela <quic_nkela@quicinc.com>
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 .../bindings/i2c/qcom,sa8255p-geni-i2c.yaml   | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml b/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml
new file mode 100644
index 000000000000..3ce0e0ba365b
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/qcom,sa8255p-geni-i2c.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/qcom,sa8255p-geni-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SA8255p QUP GENI I2C Controller
+
+maintainers:
+  - Praveen Talari <praveen.talari@oss.qualcomm.com>
+
+properties:
+  compatible:
+    const: qcom,sa8255p-geni-i2c
+
+  reg:
+    maxItems: 1
+
+  dmas:
+    maxItems: 2
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+
+  interrupts:
+    maxItems: 1
+
+  power-domains:
+    minItems: 2
+    maxItems: 2
+
+  power-domain-names:
+    items:
+      - const: power
+      - const: perf
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - power-domains
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i2c@a90000 {
+        compatible = "qcom,sa8255p-geni-i2c";
+        reg = <0xa90000 0x4000>;
+        interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
+        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
+        power-domain-names = "power", "perf";
+    };
+...
-- 
2.34.1

[PATCH v1 08/12] i2c: qcom-geni: Isolate serial engine setup

[Praveen Talari]

Move serial engine configuration from probe to geni_i2c_init().

Relocating the serial engine setup to a dedicated initialization function
enhances code clarity and simplifies future modifications.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 148 ++++++++++++++---------------
 1 file changed, 73 insertions(+), 75 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 3a04016db2c3..4111afe2713e 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -976,10 +976,75 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
 	return ret;
 }
 
+static int geni_i2c_init(struct geni_i2c_dev *gi2c)
+{
+	const struct geni_i2c_desc *desc = NULL;
+	u32 proto, tx_depth;
+	bool fifo_disable;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(gi2c->se.dev);
+	if (ret < 0) {
+		dev_err(gi2c->se.dev, "error turning on device :%d\n", ret);
+		return ret;
+	}
+
+	proto = geni_se_read_proto(&gi2c->se);
+	if (proto == GENI_SE_INVALID_PROTO) {
+		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
+		if (ret) {
+			dev_err_probe(gi2c->se.dev, ret, "i2c firmware load failed ret: %d\n", ret);
+			goto err;
+		}
+	} else if (proto != GENI_SE_I2C) {
+		ret = dev_err_probe(gi2c->se.dev, -ENXIO, "Invalid proto %d\n", proto);
+		goto err;
+	}
+
+	desc = device_get_match_data(gi2c->se.dev);
+	if (desc && desc->no_dma_support)
+		fifo_disable = false;
+	else
+		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
+
+	if (fifo_disable) {
+		/* FIFO is disabled, so we can only use GPI DMA */
+		gi2c->gpi_mode = true;
+		ret = setup_gpi_dma(gi2c);
+		if (ret)
+			goto err;
+
+		dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
+	} else {
+		gi2c->gpi_mode = false;
+		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
+
+		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
+		if (!tx_depth && desc)
+			tx_depth = desc->tx_fifo_depth;
+
+		if (!tx_depth) {
+			ret = dev_err_probe(gi2c->se.dev, -EINVAL,
+					    "Invalid TX FIFO depth\n");
+			goto err;
+		}
+
+		gi2c->tx_wm = tx_depth - 1;
+		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
+		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
+				       PACKING_BYTES_PW, true, true, true);
+
+		dev_dbg(gi2c->se.dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
+	}
+
+err:
+	pm_runtime_put(gi2c->se.dev);
+	return ret;
+}
+
 static int geni_i2c_probe(struct platform_device *pdev)
 {
 	struct geni_i2c_dev *gi2c;
-	u32 proto, tx_depth, fifo_disable;
 	int ret;
 	struct device *dev = &pdev->dev;
 	const struct geni_i2c_desc *desc = NULL;
@@ -1059,79 +1124,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(gi2c->core_clk);
-	if (ret)
-		return ret;
-
-	ret = geni_se_resources_on(&gi2c->se);
-	if (ret) {
-		dev_err_probe(dev, ret, "Error turning on resources\n");
-		goto err_clk;
-	}
-	proto = geni_se_read_proto(&gi2c->se);
-	if (proto == GENI_SE_INVALID_PROTO) {
-		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
-		if (ret) {
-			dev_err_probe(dev, ret, "i2c firmware load failed ret: %d\n", ret);
-			goto err_resources;
-		}
-	} else if (proto != GENI_SE_I2C) {
-		ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
-		goto err_resources;
-	}
-
-	if (desc && desc->no_dma_support)
-		fifo_disable = false;
-	else
-		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
-
-	if (fifo_disable) {
-		/* FIFO is disabled, so we can only use GPI DMA */
-		gi2c->gpi_mode = true;
-		ret = setup_gpi_dma(gi2c);
-		if (ret)
-			goto err_resources;
-
-		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
-	} else {
-		gi2c->gpi_mode = false;
-		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
-
-		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
-		if (!tx_depth && desc)
-			tx_depth = desc->tx_fifo_depth;
-
-		if (!tx_depth) {
-			ret = dev_err_probe(dev, -EINVAL,
-					    "Invalid TX FIFO depth\n");
-			goto err_resources;
-		}
-
-		gi2c->tx_wm = tx_depth - 1;
-		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
-		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
-				       PACKING_BYTES_PW, true, true, true);
-
-		dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
-	}
-
-	clk_disable_unprepare(gi2c->core_clk);
-	ret = geni_se_resources_off(&gi2c->se);
-	if (ret) {
-		dev_err_probe(dev, ret, "Error turning off resources\n");
-		goto err_dma;
-	}
-
-	ret = geni_icc_disable(&gi2c->se);
-	if (ret)
-		goto err_dma;
-
 	gi2c->suspended = 1;
 	pm_runtime_set_suspended(gi2c->se.dev);
 	pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
 	pm_runtime_use_autosuspend(gi2c->se.dev);
 	pm_runtime_enable(gi2c->se.dev);
 
+	ret =  geni_i2c_init(gi2c);
+	if (ret < 0) {
+		dev_err(gi2c->se.dev, "I2C init failed :%d\n", ret);
+		pm_runtime_disable(gi2c->se.dev);
+		goto err_dma;
+	}
+
 	ret = i2c_add_adapter(&gi2c->adap);
 	if (ret) {
 		dev_err_probe(dev, ret, "Error adding i2c adapter\n");
@@ -1143,13 +1148,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	return ret;
 
-err_resources:
-	geni_se_resources_off(&gi2c->se);
-err_clk:
-	clk_disable_unprepare(gi2c->core_clk);
-
-	return ret;
-
 err_dma:
 	release_gpi_dma(gi2c);
 
-- 
2.34.1

[PATCH v1 09/12] i2c: qcom-geni: Move resource initialization to separate function

[Praveen Talari]

Enhances code readability and future modifications within the new API.

Refactor the resource initialization in geni_i2c_probe() by introducing
a new geni_i2c_resources_init() function and utilizing the common
geni_se_resources_init() framework and clock frequency mapping, making the
probe function cleaner.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 53 ++++++++++++------------------
 1 file changed, 21 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 4111afe2713e..790cbca2c22e 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -1042,6 +1042,23 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 	return ret;
 }
 
+static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
+{
+	int ret;
+
+	ret = geni_se_resources_init(&gi2c->se);
+	if (ret)
+		return ret;
+
+	ret = geni_i2c_clk_map_idx(gi2c);
+	if (ret)
+		return dev_err_probe(gi2c->se.dev, ret, "Invalid clk frequency %d Hz\n",
+				     gi2c->clk_freq_out);
+
+	return geni_icc_set_bw_ab(&gi2c->se, GENI_DEFAULT_BW, GENI_DEFAULT_BW,
+				  Bps_to_icc(gi2c->clk_freq_out));
+}
+
 static int geni_i2c_probe(struct platform_device *pdev)
 {
 	struct geni_i2c_dev *gi2c;
@@ -1061,16 +1078,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	desc = device_get_match_data(&pdev->dev);
 
-	if (desc && desc->has_core_clk) {
-		gi2c->core_clk = devm_clk_get(dev, "core");
-		if (IS_ERR(gi2c->core_clk))
-			return PTR_ERR(gi2c->core_clk);
-	}
-
-	gi2c->se.clk = devm_clk_get(dev, "se");
-	if (IS_ERR(gi2c->se.clk) && !has_acpi_companion(dev))
-		return PTR_ERR(gi2c->se.clk);
-
 	ret = device_property_read_u32(dev, "clock-frequency",
 				       &gi2c->clk_freq_out);
 	if (ret) {
@@ -1085,16 +1092,15 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (gi2c->irq < 0)
 		return gi2c->irq;
 
-	ret = geni_i2c_clk_map_idx(gi2c);
-	if (ret)
-		return dev_err_probe(dev, ret, "Invalid clk frequency %d Hz\n",
-				     gi2c->clk_freq_out);
-
 	gi2c->adap.algo = &geni_i2c_algo;
 	init_completion(&gi2c->done);
 	spin_lock_init(&gi2c->lock);
 	platform_set_drvdata(pdev, gi2c);
 
+	ret = geni_i2c_resources_init(gi2c);
+	if (ret)
+		return ret;
+
 	/* Keep interrupts disabled initially to allow for low-power modes */
 	ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN,
 			       dev_name(dev), gi2c);
@@ -1107,23 +1113,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	gi2c->adap.dev.of_node = dev->of_node;
 	strscpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
 
-	ret = geni_icc_get(&gi2c->se, desc ? desc->icc_ddr : "qup-memory");
-	if (ret)
-		return ret;
-	/*
-	 * Set the bus quota for core and cpu to a reasonable value for
-	 * register access.
-	 * Set quota for DDR based on bus speed.
-	 */
-	gi2c->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
-	gi2c->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
-	if (!desc || desc->icc_ddr)
-		gi2c->se.icc_paths[GENI_TO_DDR].avg_bw = Bps_to_icc(gi2c->clk_freq_out);
-
-	ret = geni_icc_set_bw(&gi2c->se);
-	if (ret)
-		return ret;
-
 	gi2c->suspended = 1;
 	pm_runtime_set_suspended(gi2c->se.dev);
 	pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
-- 
2.34.1

[PATCH v1 10/12] i2c: qcom-geni: Use geni_se_resources_state() API in runtime PM functions

[Praveen Talari]

To manage GENI serial engine resources during runtime power management,
drivers currently need to call functions for ICC, clock, and
SE resource operations in both suspend and resume paths, resulting in
code duplication across drivers.

The new geni_se_resources_state() helper API addresses this issue by
providing a streamlined method to enable or disable all resources
based on a boolean parameter, thereby eliminating redundancy across
drivers.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 790cbca2c22e..ea117a4667e0 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -1166,18 +1166,15 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
 	disable_irq(gi2c->irq);
-	ret = geni_se_resources_off(&gi2c->se);
+
+	ret = geni_se_resources_state(&gi2c->se, false);
 	if (ret) {
 		enable_irq(gi2c->irq);
 		return ret;
-
-	} else {
-		gi2c->suspended = 1;
 	}
 
-	clk_disable_unprepare(gi2c->core_clk);
-
-	return geni_icc_disable(&gi2c->se);
+	gi2c->suspended = 1;
+	return ret;
 }
 
 static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
@@ -1185,28 +1182,13 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
 	int ret;
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
-	ret = geni_icc_enable(&gi2c->se);
+	ret = geni_se_resources_state(&gi2c->se, true);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(gi2c->core_clk);
-	if (ret)
-		goto out_icc_disable;
-
-	ret = geni_se_resources_on(&gi2c->se);
-	if (ret)
-		goto out_clk_disable;
-
 	enable_irq(gi2c->irq);
 	gi2c->suspended = 0;
 
-	return 0;
-
-out_clk_disable:
-	clk_disable_unprepare(gi2c->core_clk);
-out_icc_disable:
-	geni_icc_disable(&gi2c->se);
-
 	return ret;
 }
 
-- 
2.34.1

[PATCH v1 11/12] i2c: qcom-geni: Store of_device_id data in driver private struct

[Praveen Talari]

To avoid repeatedly fetching and checking platform data across various
functions, store the struct of_device_id data directly in the i2c
private structure. This change enhances code maintainability and reduces
redundancy.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 32 ++++++++++++++++--------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index ea117a4667e0..a0f68fdd4078 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -77,6 +77,13 @@ enum geni_i2c_err_code {
 #define XFER_TIMEOUT		HZ
 #define RST_TIMEOUT		HZ
 
+struct geni_i2c_desc {
+	bool has_core_clk;
+	char *icc_ddr;
+	bool no_dma_support;
+	unsigned int tx_fifo_depth;
+};
+
 #define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC	2
 
 /**
@@ -121,13 +128,7 @@ struct geni_i2c_dev {
 	bool is_tx_multi_desc_xfer;
 	u32 num_msgs;
 	struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
-};
-
-struct geni_i2c_desc {
-	bool has_core_clk;
-	char *icc_ddr;
-	bool no_dma_support;
-	unsigned int tx_fifo_depth;
+	const struct geni_i2c_desc *dev_data;
 };
 
 struct geni_i2c_err_log {
@@ -978,7 +979,6 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
 
 static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 {
-	const struct geni_i2c_desc *desc = NULL;
 	u32 proto, tx_depth;
 	bool fifo_disable;
 	int ret;
@@ -1001,8 +1001,7 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 		goto err;
 	}
 
-	desc = device_get_match_data(gi2c->se.dev);
-	if (desc && desc->no_dma_support)
+	if (gi2c->dev_data->no_dma_support)
 		fifo_disable = false;
 	else
 		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
@@ -1020,8 +1019,8 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
 
 		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
-		if (!tx_depth && desc)
-			tx_depth = desc->tx_fifo_depth;
+		if (!tx_depth && gi2c->dev_data->has_core_clk)
+			tx_depth = gi2c->dev_data->tx_fifo_depth;
 
 		if (!tx_depth) {
 			ret = dev_err_probe(gi2c->se.dev, -EINVAL,
@@ -1064,7 +1063,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	struct geni_i2c_dev *gi2c;
 	int ret;
 	struct device *dev = &pdev->dev;
-	const struct geni_i2c_desc *desc = NULL;
 
 	gi2c = devm_kzalloc(dev, sizeof(*gi2c), GFP_KERNEL);
 	if (!gi2c)
@@ -1076,7 +1074,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(gi2c->se.base))
 		return PTR_ERR(gi2c->se.base);
 
-	desc = device_get_match_data(&pdev->dev);
+	gi2c->dev_data = device_get_match_data(&pdev->dev);
 
 	ret = device_property_read_u32(dev, "clock-frequency",
 				       &gi2c->clk_freq_out);
@@ -1221,6 +1219,10 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
 									NULL)
 };
 
+static const struct geni_i2c_desc geni_i2c = {
+	.icc_ddr = "qup-memory",
+};
+
 static const struct geni_i2c_desc i2c_master_hub = {
 	.has_core_clk = true,
 	.icc_ddr = NULL,
@@ -1229,7 +1231,7 @@ static const struct geni_i2c_desc i2c_master_hub = {
 };
 
 static const struct of_device_id geni_i2c_dt_match[] = {
-	{ .compatible = "qcom,geni-i2c" },
+	{ .compatible = "qcom,geni-i2c", .data = &geni_i2c },
 	{ .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
 	{}
 };
-- 
2.34.1

[PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms

[Praveen Talari]

The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power
and performance protocols.

The SCMI power protocol enables or disables resources like clocks,
interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
such as resume/suspend, to control power states(on/off).

The SCMI performance protocol manages I2C frequency, with each
frequency rate represented by a performance level. The driver uses
geni_se_set_perf_opp() API to request the desired frequency rate..

As part of geni_se_set_perf_opp(), the OPP for the requested frequency
is obtained using dev_pm_opp_find_freq_floor() and the performance
level is set using dev_pm_opp_set_opp().

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index a0f68fdd4078..78154879f02d 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -82,6 +82,9 @@ struct geni_i2c_desc {
 	char *icc_ddr;
 	bool no_dma_support;
 	unsigned int tx_fifo_depth;
+	int (*resources_init)(struct geni_se *se);
+	int (*set_rate)(struct geni_se *se, unsigned long freq);
+	int (*power_state)(struct geni_se *se, bool state);
 };
 
 #define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC	2
@@ -203,8 +206,9 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
 	return -EINVAL;
 }
 
-static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
+static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
 {
+	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
 	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
 	u32 val;
 
@@ -217,6 +221,7 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
 	val |= itr->t_low_cnt << LOW_COUNTER_SHFT;
 	val |= itr->t_cycle_cnt;
 	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
+	return 0;
 }
 
 static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
@@ -908,7 +913,9 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 		return ret;
 	}
 
-	qcom_geni_i2c_conf(gi2c);
+	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
+	if (ret)
+		return ret;
 
 	if (gi2c->gpi_mode)
 		ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
@@ -1041,8 +1048,9 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 	return ret;
 }
 
-static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
+static int geni_i2c_resources_init(struct geni_se *se)
 {
+	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
 	int ret;
 
 	ret = geni_se_resources_init(&gi2c->se);
@@ -1095,7 +1103,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	spin_lock_init(&gi2c->lock);
 	platform_set_drvdata(pdev, gi2c);
 
-	ret = geni_i2c_resources_init(gi2c);
+	ret = gi2c->dev_data->resources_init(&gi2c->se);
 	if (ret)
 		return ret;
 
@@ -1165,10 +1173,12 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 
 	disable_irq(gi2c->irq);
 
-	ret = geni_se_resources_state(&gi2c->se, false);
-	if (ret) {
-		enable_irq(gi2c->irq);
-		return ret;
+	if (gi2c->dev_data->power_state) {
+		ret = gi2c->dev_data->power_state(&gi2c->se, false);
+		if (ret) {
+			enable_irq(gi2c->irq);
+			return ret;
+		}
 	}
 
 	gi2c->suspended = 1;
@@ -1180,9 +1190,11 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
 	int ret;
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
-	ret = geni_se_resources_state(&gi2c->se, true);
-	if (ret)
-		return ret;
+	if (gi2c->dev_data->power_state) {
+		ret = gi2c->dev_data->power_state(&gi2c->se, true);
+		if (ret)
+			return ret;
+	}
 
 	enable_irq(gi2c->irq);
 	gi2c->suspended = 0;
@@ -1221,6 +1233,9 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
 
 static const struct geni_i2c_desc geni_i2c = {
 	.icc_ddr = "qup-memory",
+	.resources_init = geni_i2c_resources_init,
+	.set_rate = qcom_geni_i2c_conf,
+	.power_state = geni_se_resources_state,
 };
 
 static const struct geni_i2c_desc i2c_master_hub = {
@@ -1228,11 +1243,20 @@ static const struct geni_i2c_desc i2c_master_hub = {
 	.icc_ddr = NULL,
 	.no_dma_support = true,
 	.tx_fifo_depth = 16,
+	.resources_init = geni_i2c_resources_init,
+	.set_rate = qcom_geni_i2c_conf,
+	.power_state = geni_se_resources_state,
+};
+
+static const struct geni_i2c_desc sa8255p_geni_i2c = {
+	.resources_init = geni_se_domain_attach,
+	.set_rate = geni_se_set_perf_opp,
 };
 
 static const struct of_device_id geni_i2c_dt_match[] = {
 	{ .compatible = "qcom,geni-i2c", .data = &geni_i2c },
 	{ .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
+	{ .compatible = "qcom,sa8255p-geni-i2c", .data = &sa8255p_geni_i2c },
 	{}
 };
 MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
-- 
2.34.1

Re: [PATCH v1 07/12] dt-bindings: i2c: Describe SA8255p

[Krzysztof Kozlowski]

On Sat, Nov 22, 2025 at 10:30:13AM +0530, Praveen Talari wrote:
> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    minItems: 2

Drop

> +    maxItems: 2
> +
> +  power-domain-names:
> +    items:
> +      - const: power
> +      - const: perf
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - power-domains
> +
> +allOf:

So common SE properties are not applicable? If so explain why in the
commit msg.

> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    i2c@a90000 {
> +        compatible = "qcom,sa8255p-geni-i2c";
> +        reg = <0xa90000 0x4000>;
> +        interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
> +        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
> +        power-domain-names = "power", "perf";

dmas and dma-names

Best regards,
Krzysztof

Re: [PATCH v1 03/12] soc: qcom: geni-se: Introduce helper API for resource initialization

[kernel test robot]

Hi Praveen,

kernel test robot noticed the following build errors:

[auto build test ERROR on d724c6f85e80a23ed46b7ebc6e38b527c09d64f5]

url:    https://github.com/intel-lab-lkp/linux/commits/Praveen-Talari/soc-qcom-geni-se-Refactor-geni_icc_get-and-make-qup-memory-ICC-path-optional/20251122-130449
base:   d724c6f85e80a23ed46b7ebc6e38b527c09d64f5
patch link:    https://lore.kernel.org/r/20251122050018.283669-4-praveen.talari%40oss.qualcomm.com
patch subject: [PATCH v1 03/12] soc: qcom: geni-se: Introduce helper API for resource initialization
config: loongarch-randconfig-002-20251123 (https://download.01.org/0day-ci/archive/20251123/202511231819.jiLYo6Fl-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251123/202511231819.jiLYo6Fl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511231819.jiLYo6Fl-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/soc/qcom/qcom-geni-se.c:1046:10: error: incompatible pointer to integer conversion returning 'void *' from a function with result type 'int' [-Wint-conversion]
    1046 |                 return ERR_PTR(ret);
         |                        ^~~~~~~~~~~~
   1 error generated.


vim +1046 drivers/soc/qcom/qcom-geni-se.c

  1015	
  1016	/**
  1017	 * geni_se_resources_init() - Initialize resources for a GENI SE device.
  1018	 * @se: Pointer to the geni_se structure representing the GENI SE device.
  1019	 *
  1020	 * This function initializes various resources required by the GENI Serial Engine
  1021	 * (SE) device, including clock resources (core and SE clocks), interconnect
  1022	 * paths for communication.
  1023	 * It retrieves optional and mandatory clock resources, adds an OF-based
  1024	 * operating performance point (OPP) table, and sets up interconnect paths
  1025	 * with default bandwidths. The function also sets a flag (`has_opp`) to
  1026	 * indicate whether OPP support is available for the device.
  1027	 *
  1028	 * Return: 0 on success, or a negative errno on failure.
  1029	 */
  1030	int geni_se_resources_init(struct geni_se *se)
  1031	{
  1032		int ret;
  1033	
  1034		se->core_clk = devm_clk_get_optional(se->dev, "core");
  1035		if (IS_ERR(se->core_clk))
  1036			return dev_err_probe(se->dev, PTR_ERR(se->core_clk),
  1037					     "Failed to get optional core clk\n");
  1038	
  1039		se->clk = devm_clk_get(se->dev, "se");
  1040		if (IS_ERR(se->clk) && !has_acpi_companion(se->dev))
  1041			return dev_err_probe(se->dev, PTR_ERR(se->clk),
  1042					     "Failed to get SE clk\n");
  1043	
  1044		ret = devm_pm_opp_set_clkname(se->dev, "se");
  1045		if (ret)
> 1046			return ERR_PTR(ret);
  1047	
  1048		ret = devm_pm_opp_of_add_table(se->dev);
  1049		if (ret && ret != -ENODEV)
  1050			return dev_err_probe(se->dev, ret, "Failed to add OPP table\n");
  1051	
  1052		se->has_opp = (ret == 0);
  1053	
  1054		ret = geni_icc_get(se, "qup-memory");
  1055		if (ret)
  1056			return ret;
  1057	
  1058		return geni_icc_set_bw_ab(se, GENI_DEFAULT_BW, GENI_DEFAULT_BW, GENI_DEFAULT_BW);
  1059	}
  1060	EXPORT_SYMBOL_GPL(geni_se_resources_init);
  1061	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms

[kernel test robot]

Hi Praveen,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d724c6f85e80a23ed46b7ebc6e38b527c09d64f5]

url:    https://github.com/intel-lab-lkp/linux/commits/Praveen-Talari/soc-qcom-geni-se-Refactor-geni_icc_get-and-make-qup-memory-ICC-path-optional/20251122-130449
base:   d724c6f85e80a23ed46b7ebc6e38b527c09d64f5
patch link:    https://lore.kernel.org/r/20251122050018.283669-13-praveen.talari%40oss.qualcomm.com
patch subject: [PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms
config: loongarch-randconfig-002-20251123 (https://download.01.org/0day-ci/archive/20251123/202511231944.MieDLdu8-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251123/202511231944.MieDLdu8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511231944.MieDLdu8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-qcom-geni.c:1176:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1176 |         if (gi2c->dev_data->power_state) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:1185:9: note: uninitialized use occurs here
    1185 |         return ret;
         |                ^~~
   drivers/i2c/busses/i2c-qcom-geni.c:1176:2: note: remove the 'if' if its condition is always true
    1176 |         if (gi2c->dev_data->power_state) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:1171:9: note: initialize the variable 'ret' to silence this warning
    1171 |         int ret;
         |                ^
         |                 = 0
   drivers/i2c/busses/i2c-qcom-geni.c:1193:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1193 |         if (gi2c->dev_data->power_state) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:1202:9: note: uninitialized use occurs here
    1202 |         return ret;
         |                ^~~
   drivers/i2c/busses/i2c-qcom-geni.c:1193:2: note: remove the 'if' if its condition is always true
    1193 |         if (gi2c->dev_data->power_state) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:1190:9: note: initialize the variable 'ret' to silence this warning
    1190 |         int ret;
         |                ^
         |                 = 0
   2 warnings generated.


vim +1176 drivers/i2c/busses/i2c-qcom-geni.c

  1168	
  1169	static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
  1170	{
  1171		int ret;
  1172		struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
  1173	
  1174		disable_irq(gi2c->irq);
  1175	
> 1176		if (gi2c->dev_data->power_state) {
  1177			ret = gi2c->dev_data->power_state(&gi2c->se, false);
  1178			if (ret) {
  1179				enable_irq(gi2c->irq);
  1180				return ret;
  1181			}
  1182		}
  1183	
  1184		gi2c->suspended = 1;
  1185		return ret;
  1186	}
  1187	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 07/12] dt-bindings: i2c: Describe SA8255p

[Praveen Talari]

Hi Krzysztof,

Thank you for review.

On 11/22/2025 5:10 PM, Krzysztof Kozlowski wrote:
> On Sat, Nov 22, 2025 at 10:30:13AM +0530, Praveen Talari wrote:
>> +  dmas:
>> +    maxItems: 2
>> +
>> +  dma-names:
>> +    items:
>> +      - const: tx
>> +      - const: rx
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  power-domains:
>> +    minItems: 2
> 
> Drop

sure, will do it in next version.

> 
>> +    maxItems: 2
>> +
>> +  power-domain-names:
>> +    items:
>> +      - const: power
>> +      - const: perf
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - power-domains
>> +
>> +allOf:
> 
> So common SE properties are not applicable? If so explain why in the
> commit msg.

Are you referring to clocks, ICC paths, and pin control?
Please let me know if I’m mistaken.

> 
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    i2c@a90000 {
>> +        compatible = "qcom,sa8255p-geni-i2c";
>> +        reg = <0xa90000 0x4000>;
>> +        interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
>> +        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
>> +        power-domain-names = "power", "perf";
> 
> dmas and dma-names

sure.

Thanks,
Praveen

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 07/12] dt-bindings: i2c: Describe SA8255p

[Krzysztof Kozlowski]

On 25/11/2025 05:03, Praveen Talari wrote:
> Hi Krzysztof,
> 
> Thank you for review.
> 
> On 11/22/2025 5:10 PM, Krzysztof Kozlowski wrote:
>> On Sat, Nov 22, 2025 at 10:30:13AM +0530, Praveen Talari wrote:
>>> +  dmas:
>>> +    maxItems: 2
>>> +
>>> +  dma-names:
>>> +    items:
>>> +      - const: tx
>>> +      - const: rx
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  power-domains:
>>> +    minItems: 2
>>
>> Drop
> 
> sure, will do it in next version.
> 
>>
>>> +    maxItems: 2
>>> +
>>> +  power-domain-names:
>>> +    items:
>>> +      - const: power
>>> +      - const: perf
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - power-domains
>>> +
>>> +allOf:
>>
>> So common SE properties are not applicable? If so explain why in the
>> commit msg.
> 
> Are you referring to clocks, ICC paths, and pin control?
> Please let me know if I’m mistaken.

No, I refer to common SE properties. See common qcom geni SE schema. See
every other geni SE schema...

Best regards,
Krzysztof

Re: [PATCH v1 07/12] dt-bindings: i2c: Describe SA8255p

[Praveen Talari]

Hi Krzysztof,

On 11/22/2025 5:10 PM, Krzysztof Kozlowski wrote:
> On Sat, Nov 22, 2025 at 10:30:13AM +0530, Praveen Talari wrote:
>> +  dmas:
>> +    maxItems: 2
>> +
>> +  dma-names:
>> +    items:
>> +      - const: tx
>> +      - const: rx
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  power-domains:
>> +    minItems: 2
> 
> Drop
> 
>> +    maxItems: 2
>> +
>> +  power-domain-names:
>> +    items:
>> +      - const: power
>> +      - const: perf
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - power-domains
>> +
>> +allOf:
> 
> So common SE properties are not applicable? If so explain why in the
> commit msg.
> 
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    i2c@a90000 {
>> +        compatible = "qcom,sa8255p-geni-i2c";
>> +        reg = <0xa90000 0x4000>;
>> +        interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
>> +        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
>> +        power-domain-names = "power", "perf";
> 
> dmas and dma-names

For this platform (all Auto targets), we primarily use FIFO/CPU_DMA mode 
rather than GSI mode, and these are not defined in the Device Tree file 
as well now. Should we still include the dmas and dma-names properties 
in the example node?

Thanks,
Praveen

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 01/12] soc: qcom: geni-se: Refactor geni_icc_get() and make qup-memory ICC path optional

[Bjorn Andersson]

On Sat, Nov 22, 2025 at 10:30:07AM +0530, Praveen Talari wrote:
> Refactor the geni_icc_get() function to replace the loop-based ICC path
> initialization with explicit handling of each interconnect path. This
> improves code readability and allows for different error handling per
> path type.

I don't think this "improves code readability", IMO you're turning a
clean loop into a unrolled mess.


But then comes the least significant portion of your "problem
description" (i.e. the last words of it), where you indicate that this
would allow you to have different error handling for "qup-memory".

This is actually a valid reason to make this change, so say that!


> 
> The "qup-core" and "qup-config" paths remain mandatory, while "qup-memory"
> is now optional and skipped if not defined in DT.
> 

Please rewrite this message to _start_ with the problem description.
Make it clear on the first line/sentence why the change should be done.

E.g. compare with something like this:

"""
"qup-memory" is an optional interconnect path, unroll the geni_icc_get()
loop in order to allow specific error handling for this path.
"""

You only need to read 4 words to understand exactly why this patch
exists.

> Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 36 +++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index cd1779b6a91a..b6167b968ef6 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -899,30 +899,32 @@ EXPORT_SYMBOL_GPL(geni_se_rx_dma_unprep);
>  
>  int geni_icc_get(struct geni_se *se, const char *icc_ddr)
>  {
> -	int i, err;
> -	const char *icc_names[] = {"qup-core", "qup-config", icc_ddr};
> +	struct geni_icc_path *icc_paths = se->icc_paths;
>  
>  	if (has_acpi_companion(se->dev))
>  		return 0;
>  
> -	for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) {
> -		if (!icc_names[i])
> -			continue;
> -
> -		se->icc_paths[i].path = devm_of_icc_get(se->dev, icc_names[i]);
> -		if (IS_ERR(se->icc_paths[i].path))
> -			goto err;
> +	icc_paths[GENI_TO_CORE].path = devm_of_icc_get(se->dev, "qup-core");
> +	if (IS_ERR(icc_paths[GENI_TO_CORE].path))
> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_CORE].path),
> +				     "Failed to get 'qup-core' ICC path\n");

To taste, but I think a local variable would be helpful to make this
less dense.

	path = devm_of_icc_get(se->dev, "qup-core");
	if (IS_ERR(path))
		return dev_err_probe(se->dev, PTR_ERR(path), "Failed to get 'qup-core' ICC path\n");
	icc_paths[GENI_TO_CORE].path = path;

Regards,
Bjorn

> +
> +	icc_paths[CPU_TO_GENI].path = devm_of_icc_get(se->dev, "qup-config");
> +	if (IS_ERR(icc_paths[CPU_TO_GENI].path))
> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[CPU_TO_GENI].path),
> +				     "Failed to get 'qup-config' ICC path\n");
> +
> +	/* The DDR path is optional, depending on protocol and hw capabilities */
> +	icc_paths[GENI_TO_DDR].path = devm_of_icc_get(se->dev, "qup-memory");
> +	if (IS_ERR(icc_paths[GENI_TO_DDR].path)) {
> +		if (PTR_ERR(icc_paths[GENI_TO_DDR].path) == -ENODATA)
> +			icc_paths[GENI_TO_DDR].path = NULL;
> +		else
> +			return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_DDR].path),
> +					     "Failed to get 'qup-memory' ICC path\n");
>  	}
>  
>  	return 0;
> -
> -err:
> -	err = PTR_ERR(se->icc_paths[i].path);
> -	if (err != -EPROBE_DEFER)
> -		dev_err_ratelimited(se->dev, "Failed to get ICC path '%s': %d\n",
> -					icc_names[i], err);
> -	return err;
> -
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_get);
>  
> -- 
> 2.34.1
> 

Re: [PATCH v1 04/12] soc: qcom: geni-se: Add geni_se_resource_state() helper

[Bjorn Andersson]

On Sat, Nov 22, 2025 at 10:30:10AM +0530, Praveen Talari wrote:
> The GENI SE protocol drivers (I2C, SPI, UART) implement similar resource
> activation/deactivation sequences independently, leading to code
> duplication.
> 
> Introduce geni_se_resource_state() to control power state of GENI SE
> resources. This function provides a unified interface that calls either
> geni_se_resources_activate() to power on resources or
> geni_se_resources_deactivate() to power off resources based on the
> power_on parameter.
> 
> The activate function enables ICC, clocks, and TLMM with proper error
> handling and cleanup paths. The deactivate function disables resources
> in reverse order including OPP rate reset, clocks, ICC and TLMM.
> 
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/qcom-geni-se.c  | 61 ++++++++++++++++++++++++++++++++
>  include/linux/soc/qcom/geni-se.h |  2 ++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 726b77650007..7aee7fd2e240 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -1013,6 +1013,67 @@ int geni_icc_disable(struct geni_se *se)
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>  
> +static int geni_se_resources_deactivate(struct geni_se *se)
> +{
> +	int ret;
> +
> +	if (se->has_opp)
> +		dev_pm_opp_set_rate(se->dev, 0);
> +
> +	ret = geni_se_resources_off(se);

Why do we end this series with two different APIs for turning (on/) off
the GENI resources? Can't there be a single geni_se_resources_"off"()?

> +	if (ret)
> +		return ret;
> +
> +	if (se->core_clk)
> +		clk_disable_unprepare(se->core_clk);
> +
> +	return geni_icc_disable(se);
> +}
> +
> +static int geni_se_resources_activate(struct geni_se *se)
> +{
> +	int ret;
> +
> +	ret = geni_icc_enable(se);
> +	if (ret)
> +		return ret;
> +
> +	if (se->core_clk) {
> +		ret = clk_prepare_enable(se->core_clk);
> +		if (ret)
> +			goto out_icc_disable;
> +	}
> +
> +	ret = geni_se_resources_on(se);
> +	if (ret)
> +		goto out_clk_disable;
> +
> +	return 0;
> +
> +out_clk_disable:
> +	if (se->core_clk)
> +		clk_disable_unprepare(se->core_clk);
> +out_icc_disable:
> +	geni_icc_disable(se);
> +	return ret;
> +}
> +
> +/**
> + * geni_se_resources_state() - Control power state of GENI SE resources
> + * @se: Pointer to the geni_se structure
> + * @power_on: Boolean flag for desired power state (true = on, false = off)
> + *
> + * Controls GENI SE resource power state by calling activate or deactivate
> + * functions based on the power_on parameter.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int geni_se_resources_state(struct geni_se *se, bool power_on)

It seems the purpose of this "helper function" is to allow replacing
geni_se_resource_on() with geni_se_resources_state(true) and
geni_se_resource_off() with geni_se_resources_state(false) in patch 10.


Naming a function "on", "activate", or "enable" provides a clear
indication of what will happen when you call the function. Calling a
function to "set state to true" is not as clear.

Further, the code paths that needs to have resources turned on should be
separate from those who signal that those resources can be turned off.
So there should not be any gain from this function, unless the same
obfuscation happens further up the stack.

Just call the activate/deactivate in the respective code path.

Regards,
Bjorn

> +{
> +	return power_on ? geni_se_resources_activate(se) : geni_se_resources_deactivate(se);
> +}
> +EXPORT_SYMBOL_GPL(geni_se_resources_state);
> +
>  /**
>   * geni_se_resources_init() - Initialize resources for a GENI SE device.
>   * @se: Pointer to the geni_se structure representing the GENI SE device.
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index c182dd0f0bde..d1ca13a4e54c 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -541,6 +541,8 @@ int geni_icc_disable(struct geni_se *se);
>  
>  int geni_se_resources_init(struct geni_se *se);
>  
> +int geni_se_resources_state(struct geni_se *se, bool power_on);
> +
>  int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
>  #endif
>  #endif
> -- 
> 2.34.1
> 

Re: [PATCH v1 08/12] i2c: qcom-geni: Isolate serial engine setup

[Bjorn Andersson]

On Sat, Nov 22, 2025 at 10:30:14AM +0530, Praveen Talari wrote:
> Move serial engine configuration from probe to geni_i2c_init().
> 
> Relocating the serial engine setup to a dedicated initialization function
> enhances code clarity and simplifies future modifications.

Please enhance commit message clarity. I don't think "code clarity" is
your most significant reason for this change, and "simplifies future
modification" is completely vague.

Be specific, the reader of this commit message hasn't implemented the
next set of commits, so they don't understand why this helps.

If the reason is that this simplifies the error handling around the
resource acquisition in the next patches, write that.

If my guess is wrong and the sole reason for you change is that you
don't like 179 lines long functions, then just say that.

Regards,
Bjorn

> 
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 148 ++++++++++++++---------------
>  1 file changed, 73 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 3a04016db2c3..4111afe2713e 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -976,10 +976,75 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
>  	return ret;
>  }
>  
> +static int geni_i2c_init(struct geni_i2c_dev *gi2c)
> +{
> +	const struct geni_i2c_desc *desc = NULL;
> +	u32 proto, tx_depth;
> +	bool fifo_disable;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(gi2c->se.dev);
> +	if (ret < 0) {
> +		dev_err(gi2c->se.dev, "error turning on device :%d\n", ret);
> +		return ret;
> +	}
> +
> +	proto = geni_se_read_proto(&gi2c->se);
> +	if (proto == GENI_SE_INVALID_PROTO) {
> +		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
> +		if (ret) {
> +			dev_err_probe(gi2c->se.dev, ret, "i2c firmware load failed ret: %d\n", ret);
> +			goto err;
> +		}
> +	} else if (proto != GENI_SE_I2C) {
> +		ret = dev_err_probe(gi2c->se.dev, -ENXIO, "Invalid proto %d\n", proto);
> +		goto err;
> +	}
> +
> +	desc = device_get_match_data(gi2c->se.dev);
> +	if (desc && desc->no_dma_support)
> +		fifo_disable = false;
> +	else
> +		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +
> +	if (fifo_disable) {
> +		/* FIFO is disabled, so we can only use GPI DMA */
> +		gi2c->gpi_mode = true;
> +		ret = setup_gpi_dma(gi2c);
> +		if (ret)
> +			goto err;
> +
> +		dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
> +	} else {
> +		gi2c->gpi_mode = false;
> +		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
> +
> +		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> +		if (!tx_depth && desc)
> +			tx_depth = desc->tx_fifo_depth;
> +
> +		if (!tx_depth) {
> +			ret = dev_err_probe(gi2c->se.dev, -EINVAL,
> +					    "Invalid TX FIFO depth\n");
> +			goto err;
> +		}
> +
> +		gi2c->tx_wm = tx_depth - 1;
> +		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> +		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
> +				       PACKING_BYTES_PW, true, true, true);
> +
> +		dev_dbg(gi2c->se.dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> +	}
> +
> +err:
> +	pm_runtime_put(gi2c->se.dev);
> +	return ret;
> +}
> +
>  static int geni_i2c_probe(struct platform_device *pdev)
>  {
>  	struct geni_i2c_dev *gi2c;
> -	u32 proto, tx_depth, fifo_disable;
>  	int ret;
>  	struct device *dev = &pdev->dev;
>  	const struct geni_i2c_desc *desc = NULL;
> @@ -1059,79 +1124,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_prepare_enable(gi2c->core_clk);
> -	if (ret)
> -		return ret;
> -
> -	ret = geni_se_resources_on(&gi2c->se);
> -	if (ret) {
> -		dev_err_probe(dev, ret, "Error turning on resources\n");
> -		goto err_clk;
> -	}
> -	proto = geni_se_read_proto(&gi2c->se);
> -	if (proto == GENI_SE_INVALID_PROTO) {
> -		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
> -		if (ret) {
> -			dev_err_probe(dev, ret, "i2c firmware load failed ret: %d\n", ret);
> -			goto err_resources;
> -		}
> -	} else if (proto != GENI_SE_I2C) {
> -		ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
> -		goto err_resources;
> -	}
> -
> -	if (desc && desc->no_dma_support)
> -		fifo_disable = false;
> -	else
> -		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> -
> -	if (fifo_disable) {
> -		/* FIFO is disabled, so we can only use GPI DMA */
> -		gi2c->gpi_mode = true;
> -		ret = setup_gpi_dma(gi2c);
> -		if (ret)
> -			goto err_resources;
> -
> -		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
> -	} else {
> -		gi2c->gpi_mode = false;
> -		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
> -
> -		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> -		if (!tx_depth && desc)
> -			tx_depth = desc->tx_fifo_depth;
> -
> -		if (!tx_depth) {
> -			ret = dev_err_probe(dev, -EINVAL,
> -					    "Invalid TX FIFO depth\n");
> -			goto err_resources;
> -		}
> -
> -		gi2c->tx_wm = tx_depth - 1;
> -		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> -		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
> -				       PACKING_BYTES_PW, true, true, true);
> -
> -		dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> -	}
> -
> -	clk_disable_unprepare(gi2c->core_clk);
> -	ret = geni_se_resources_off(&gi2c->se);
> -	if (ret) {
> -		dev_err_probe(dev, ret, "Error turning off resources\n");
> -		goto err_dma;
> -	}
> -
> -	ret = geni_icc_disable(&gi2c->se);
> -	if (ret)
> -		goto err_dma;
> -
>  	gi2c->suspended = 1;
>  	pm_runtime_set_suspended(gi2c->se.dev);
>  	pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
>  	pm_runtime_use_autosuspend(gi2c->se.dev);
>  	pm_runtime_enable(gi2c->se.dev);
>  
> +	ret =  geni_i2c_init(gi2c);
> +	if (ret < 0) {
> +		dev_err(gi2c->se.dev, "I2C init failed :%d\n", ret);
> +		pm_runtime_disable(gi2c->se.dev);
> +		goto err_dma;
> +	}
> +
>  	ret = i2c_add_adapter(&gi2c->adap);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "Error adding i2c adapter\n");
> @@ -1143,13 +1148,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  
>  	return ret;
>  
> -err_resources:
> -	geni_se_resources_off(&gi2c->se);
> -err_clk:
> -	clk_disable_unprepare(gi2c->core_clk);
> -
> -	return ret;
> -
>  err_dma:
>  	release_gpi_dma(gi2c);
>  
> -- 
> 2.34.1
> 

Re: [PATCH v1 10/12] i2c: qcom-geni: Use geni_se_resources_state() API in runtime PM functions

[Bjorn Andersson]

On Sat, Nov 22, 2025 at 10:30:16AM +0530, Praveen Talari wrote:
> To manage GENI serial engine resources during runtime power management,
> drivers currently need to call functions for ICC, clock, and
> SE resource operations in both suspend and resume paths, resulting in
> code duplication across drivers.
> 
> The new geni_se_resources_state() helper API addresses this issue by
> providing a streamlined method to enable or disable all resources
> based on a boolean parameter, thereby eliminating redundancy across
> drivers.
> 
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 28 +++++-----------------------
>  1 file changed, 5 insertions(+), 23 deletions(-)

Nice to see such stats, which I presume will also show up in the other
SE drivers later as well.

> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 790cbca2c22e..ea117a4667e0 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -1166,18 +1166,15 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
>  	disable_irq(gi2c->irq);
> -	ret = geni_se_resources_off(&gi2c->se);
> +
> +	ret = geni_se_resources_state(&gi2c->se, false);

As I said in the previous patch, there's no reason to "set state to
false", it's clearer to just have an "on" and an "off" function.

Regards,
Bjorn

>  	if (ret) {
>  		enable_irq(gi2c->irq);
>  		return ret;
> -
> -	} else {
> -		gi2c->suspended = 1;
>  	}
>  
> -	clk_disable_unprepare(gi2c->core_clk);
> -
> -	return geni_icc_disable(&gi2c->se);
> +	gi2c->suspended = 1;
> +	return ret;
>  }
>  
>  static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
> @@ -1185,28 +1182,13 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>  	int ret;
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
> -	ret = geni_icc_enable(&gi2c->se);
> +	ret = geni_se_resources_state(&gi2c->se, true);
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_prepare_enable(gi2c->core_clk);
> -	if (ret)
> -		goto out_icc_disable;
> -
> -	ret = geni_se_resources_on(&gi2c->se);
> -	if (ret)
> -		goto out_clk_disable;
> -
>  	enable_irq(gi2c->irq);
>  	gi2c->suspended = 0;
>  
> -	return 0;
> -
> -out_clk_disable:
> -	clk_disable_unprepare(gi2c->core_clk);
> -out_icc_disable:
> -	geni_icc_disable(&gi2c->se);
> -
>  	return ret;
>  }
>  
> -- 
> 2.34.1
> 

Re: [PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms

[Bjorn Andersson]

On Sat, Nov 22, 2025 at 10:30:18AM +0530, Praveen Talari wrote:
> The Qualcomm automotive SA8255p SoC relies on firmware to configure
> platform resources, including clocks, interconnects and TLMM.
> The driver requests resources operations over SCMI using power
> and performance protocols.
> 
> The SCMI power protocol enables or disables resources like clocks,
> interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
> such as resume/suspend, to control power states(on/off).
> 
> The SCMI performance protocol manages I2C frequency, with each
> frequency rate represented by a performance level. The driver uses
> geni_se_set_perf_opp() API to request the desired frequency rate..
> 
> As part of geni_se_set_perf_opp(), the OPP for the requested frequency
> is obtained using dev_pm_opp_find_freq_floor() and the performance
> level is set using dev_pm_opp_set_opp().
> 
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a0f68fdd4078..78154879f02d 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -82,6 +82,9 @@ struct geni_i2c_desc {
>  	char *icc_ddr;
>  	bool no_dma_support;
>  	unsigned int tx_fifo_depth;
> +	int (*resources_init)(struct geni_se *se);
> +	int (*set_rate)(struct geni_se *se, unsigned long freq);
> +	int (*power_state)(struct geni_se *se, bool state);

You have isolated this quite nicely now, so I'd prefer 3 (four to keep
power on/off separate) if statements, over these function pointers, at
this point.

This saves the future reader from having to remember the combination of
function pointer targets in the various cases - and allow things like
"jump to definition" in your editor to still work.

>  };
>  
>  #define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC	2
> @@ -203,8 +206,9 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>  	return -EINVAL;
>  }
>  
> -static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
> +static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)

This sounds like a qcom_geni_i2c_set_rate() now that it takes a
frequency argument.

Regards,
Bjorn

>  {
> +	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
>  	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>  	u32 val;
>  
> @@ -217,6 +221,7 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>  	val |= itr->t_low_cnt << LOW_COUNTER_SHFT;
>  	val |= itr->t_cycle_cnt;
>  	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
> +	return 0;
>  }
>  
>  static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
> @@ -908,7 +913,9 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  		return ret;
>  	}
>  
> -	qcom_geni_i2c_conf(gi2c);
> +	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
> +	if (ret)
> +		return ret;
>  
>  	if (gi2c->gpi_mode)
>  		ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
> @@ -1041,8 +1048,9 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
>  	return ret;
>  }
>  
> -static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
> +static int geni_i2c_resources_init(struct geni_se *se)
>  {
> +	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
>  	int ret;
>  
>  	ret = geni_se_resources_init(&gi2c->se);
> @@ -1095,7 +1103,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	spin_lock_init(&gi2c->lock);
>  	platform_set_drvdata(pdev, gi2c);
>  
> -	ret = geni_i2c_resources_init(gi2c);
> +	ret = gi2c->dev_data->resources_init(&gi2c->se);
>  	if (ret)
>  		return ret;
>  
> @@ -1165,10 +1173,12 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>  
>  	disable_irq(gi2c->irq);
>  
> -	ret = geni_se_resources_state(&gi2c->se, false);
> -	if (ret) {
> -		enable_irq(gi2c->irq);
> -		return ret;
> +	if (gi2c->dev_data->power_state) {
> +		ret = gi2c->dev_data->power_state(&gi2c->se, false);
> +		if (ret) {
> +			enable_irq(gi2c->irq);
> +			return ret;
> +		}
>  	}
>  
>  	gi2c->suspended = 1;
> @@ -1180,9 +1190,11 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>  	int ret;
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
> -	ret = geni_se_resources_state(&gi2c->se, true);
> -	if (ret)
> -		return ret;
> +	if (gi2c->dev_data->power_state) {
> +		ret = gi2c->dev_data->power_state(&gi2c->se, true);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	enable_irq(gi2c->irq);
>  	gi2c->suspended = 0;
> @@ -1221,6 +1233,9 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
>  
>  static const struct geni_i2c_desc geni_i2c = {
>  	.icc_ddr = "qup-memory",
> +	.resources_init = geni_i2c_resources_init,
> +	.set_rate = qcom_geni_i2c_conf,
> +	.power_state = geni_se_resources_state,
>  };
>  
>  static const struct geni_i2c_desc i2c_master_hub = {
> @@ -1228,11 +1243,20 @@ static const struct geni_i2c_desc i2c_master_hub = {
>  	.icc_ddr = NULL,
>  	.no_dma_support = true,
>  	.tx_fifo_depth = 16,
> +	.resources_init = geni_i2c_resources_init,
> +	.set_rate = qcom_geni_i2c_conf,
> +	.power_state = geni_se_resources_state,
> +};
> +
> +static const struct geni_i2c_desc sa8255p_geni_i2c = {
> +	.resources_init = geni_se_domain_attach,
> +	.set_rate = geni_se_set_perf_opp,
>  };
>  
>  static const struct of_device_id geni_i2c_dt_match[] = {
>  	{ .compatible = "qcom,geni-i2c", .data = &geni_i2c },
>  	{ .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
> +	{ .compatible = "qcom,sa8255p-geni-i2c", .data = &sa8255p_geni_i2c },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
> -- 
> 2.34.1
> 

Re: [PATCH v1 01/12] soc: qcom: geni-se: Refactor geni_icc_get() and make qup-memory ICC path optional

[Praveen Talari]

Hi Bjorn,

Thank you for review

On 11/26/2025 8:37 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:07AM +0530, Praveen Talari wrote:
>> Refactor the geni_icc_get() function to replace the loop-based ICC path
>> initialization with explicit handling of each interconnect path. This
>> improves code readability and allows for different error handling per
>> path type.
> 
> I don't think this "improves code readability", IMO you're turning a
> clean loop into a unrolled mess.
> 
> 
> But then comes the least significant portion of your "problem
> description" (i.e. the last words of it), where you indicate that this
> would allow you to have different error handling for "qup-memory".
> 
> This is actually a valid reason to make this change, so say that!
> 
> 
>>
>> The "qup-core" and "qup-config" paths remain mandatory, while "qup-memory"
>> is now optional and skipped if not defined in DT.
>>
> 
> Please rewrite this message to _start_ with the problem description.
> Make it clear on the first line/sentence why the change should be done.
> 
> E.g. compare with something like this:
> 
> """
> "qup-memory" is an optional interconnect path, unroll the geni_icc_get()
> loop in order to allow specific error handling for this path.
> """
> 
> You only need to read 4 words to understand exactly why this patch
> exists.

The "qup-memory" interconnect path is optional and may not be defined
in all device trees. Unroll the loop-based ICC path initialization to
allow specific error handling for each path type.

The "qup-core" and "qup-config" paths remain mandatory and will fail
probe if missing, while "qup-memory" is now handled as optional and
skipped when not present in the device tree.

I hope the commit text above should be appropriate

Thanks,
Praveen
> 
>> Co-developed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c | 36 +++++++++++++++++----------------
>>   1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index cd1779b6a91a..b6167b968ef6 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -899,30 +899,32 @@ EXPORT_SYMBOL_GPL(geni_se_rx_dma_unprep);
>>   
>>   int geni_icc_get(struct geni_se *se, const char *icc_ddr)
>>   {
>> -	int i, err;
>> -	const char *icc_names[] = {"qup-core", "qup-config", icc_ddr};
>> +	struct geni_icc_path *icc_paths = se->icc_paths;
>>   
>>   	if (has_acpi_companion(se->dev))
>>   		return 0;
>>   
>> -	for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) {
>> -		if (!icc_names[i])
>> -			continue;
>> -
>> -		se->icc_paths[i].path = devm_of_icc_get(se->dev, icc_names[i]);
>> -		if (IS_ERR(se->icc_paths[i].path))
>> -			goto err;
>> +	icc_paths[GENI_TO_CORE].path = devm_of_icc_get(se->dev, "qup-core");
>> +	if (IS_ERR(icc_paths[GENI_TO_CORE].path))
>> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_CORE].path),
>> +				     "Failed to get 'qup-core' ICC path\n");
> 
> To taste, but I think a local variable would be helpful to make this
> less dense.

Sure, will do it in next version.

Thanks,
Praveen
> 
> 	path = devm_of_icc_get(se->dev, "qup-core");
> 	if (IS_ERR(path))
> 		return dev_err_probe(se->dev, PTR_ERR(path), "Failed to get 'qup-core' ICC path\n");
> 	icc_paths[GENI_TO_CORE].path = path;
> 
> Regards,
> Bjorn
> 
>> +
>> +	icc_paths[CPU_TO_GENI].path = devm_of_icc_get(se->dev, "qup-config");
>> +	if (IS_ERR(icc_paths[CPU_TO_GENI].path))
>> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[CPU_TO_GENI].path),
>> +				     "Failed to get 'qup-config' ICC path\n");
>> +
>> +	/* The DDR path is optional, depending on protocol and hw capabilities */
>> +	icc_paths[GENI_TO_DDR].path = devm_of_icc_get(se->dev, "qup-memory");
>> +	if (IS_ERR(icc_paths[GENI_TO_DDR].path)) {
>> +		if (PTR_ERR(icc_paths[GENI_TO_DDR].path) == -ENODATA)
>> +			icc_paths[GENI_TO_DDR].path = NULL;
>> +		else
>> +			return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_DDR].path),
>> +					     "Failed to get 'qup-memory' ICC path\n");
>>   	}
>>   
>>   	return 0;
>> -
>> -err:
>> -	err = PTR_ERR(se->icc_paths[i].path);
>> -	if (err != -EPROBE_DEFER)
>> -		dev_err_ratelimited(se->dev, "Failed to get ICC path '%s': %d\n",
>> -					icc_names[i], err);
>> -	return err;
>> -
>>   }
>>   EXPORT_SYMBOL_GPL(geni_icc_get);
>>   
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 04/12] soc: qcom: geni-se: Add geni_se_resource_state() helper

[Praveen Talari]

H Bjorn

On 11/26/2025 8:49 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:10AM +0530, Praveen Talari wrote:
>> The GENI SE protocol drivers (I2C, SPI, UART) implement similar resource
>> activation/deactivation sequences independently, leading to code
>> duplication.
>>
>> Introduce geni_se_resource_state() to control power state of GENI SE
>> resources. This function provides a unified interface that calls either
>> geni_se_resources_activate() to power on resources or
>> geni_se_resources_deactivate() to power off resources based on the
>> power_on parameter.
>>
>> The activate function enables ICC, clocks, and TLMM with proper error
>> handling and cleanup paths. The deactivate function disables resources
>> in reverse order including OPP rate reset, clocks, ICC and TLMM.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c  | 61 ++++++++++++++++++++++++++++++++
>>   include/linux/soc/qcom/geni-se.h |  2 ++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index 726b77650007..7aee7fd2e240 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -1013,6 +1013,67 @@ int geni_icc_disable(struct geni_se *se)
>>   }
>>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>>   
>> +static int geni_se_resources_deactivate(struct geni_se *se)
>> +{
>> +	int ret;
>> +
>> +	if (se->has_opp)
>> +		dev_pm_opp_set_rate(se->dev, 0);
>> +
>> +	ret = geni_se_resources_off(se);
> 
> Why do we end this series with two different APIs for turning (on/) off

Currently, we have resources_off() which only manages clocks and 
pinctrl. I’m leveraging that in the new API.

If you agree, I can migrate the logic from resources_off() into the new 
API and remove resources_off() once support for all protocols is 
implemented.

Code snippet:

static int geni_se_resources_deactivate(struct geni_se *se)
{
         int ret;

         if (has_acpi_companion(se->dev))
                 return 0;

         if (se->has_opp)
                 dev_pm_opp_set_rate(se->dev, 0);

         ret = pinctrl_pm_select_sleep_state(se->dev);
         if (ret)
                 return ret;

         geni_se_clks_off(se);

         if (se->core_clk)
                 clk_disable_unprepare(se->core_clk);

         return geni_icc_disable(se);
}

static int geni_se_resources_activate(struct geni_se *se)
{
         int ret;

         if (has_acpi_companion(se->dev))
                 return 0;

         ret = geni_icc_enable(se);
         if (ret)
                 return ret;

         if (se->core_clk) {
                 ret = clk_prepare_enable(se->core_clk);
                 if (ret)
                         goto out_icc_disable;
         }

         ret = geni_se_clks_on(se);
         if (ret)
                 goto out_clk_disable;

         ret = pinctrl_pm_select_default_state(se->dev);
         if (ret) {
                 geni_se_clks_off(se);
                 goto out_clk_disable;
         }

         return ret;

out_clk_disable:
         if (se->core_clk)
                 clk_disable_unprepare(se->core_clk);
out_icc_disable:
         geni_icc_disable(se);
         return ret;
}

> the GENI resources? Can't there be a single geni_se_resources_"off"()?
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (se->core_clk)
>> +		clk_disable_unprepare(se->core_clk);
>> +
>> +	return geni_icc_disable(se);
>> +}
>> +
>> +static int geni_se_resources_activate(struct geni_se *se)
>> +{
>> +	int ret;
>> +
>> +	ret = geni_icc_enable(se);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (se->core_clk) {
>> +		ret = clk_prepare_enable(se->core_clk);
>> +		if (ret)
>> +			goto out_icc_disable;
>> +	}
>> +
>> +	ret = geni_se_resources_on(se);
>> +	if (ret)
>> +		goto out_clk_disable;
>> +
>> +	return 0;
>> +
>> +out_clk_disable:
>> +	if (se->core_clk)
>> +		clk_disable_unprepare(se->core_clk);
>> +out_icc_disable:
>> +	geni_icc_disable(se);
>> +	return ret;
>> +}


>> +
>> +/**
>> + * geni_se_resources_state() - Control power state of GENI SE resources
>> + * @se: Pointer to the geni_se structure
>> + * @power_on: Boolean flag for desired power state (true = on, false = off)
>> + *
>> + * Controls GENI SE resource power state by calling activate or deactivate
>> + * functions based on the power_on parameter.
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +int geni_se_resources_state(struct geni_se *se, bool power_on)
> 
> It seems the purpose of this "helper function" is to allow replacing
> geni_se_resource_on() with geni_se_resources_state(true) and
> geni_se_resource_off() with geni_se_resources_state(false) in patch 10.
> 
> 
> Naming a function "on", "activate", or "enable" provides a clear
> indication of what will happen when you call the function. Calling a
> function to "set state to true" is not as clear.
> 
> Further, the code paths that needs to have resources turned on should be
> separate from those who signal that those resources can be turned off.
> So there should not be any gain from this function, unless the same
> obfuscation happens further up the stack.
> 
> Just call the activate/deactivate in the respective code path.

Thank you for the inputs.
Sure, will review and update next patch.

Thanks,
Praveen Talari
> 
> Regards,
> Bjorn
> 
>> +{
>> +	return power_on ? geni_se_resources_activate(se) : geni_se_resources_deactivate(se);
>> +}
>> +EXPORT_SYMBOL_GPL(geni_se_resources_state);
>> +
>>   /**
>>    * geni_se_resources_init() - Initialize resources for a GENI SE device.
>>    * @se: Pointer to the geni_se structure representing the GENI SE device.
>> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
>> index c182dd0f0bde..d1ca13a4e54c 100644
>> --- a/include/linux/soc/qcom/geni-se.h
>> +++ b/include/linux/soc/qcom/geni-se.h
>> @@ -541,6 +541,8 @@ int geni_icc_disable(struct geni_se *se);
>>   
>>   int geni_se_resources_init(struct geni_se *se);
>>   
>> +int geni_se_resources_state(struct geni_se *se, bool power_on);
>> +
>>   int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
>>   #endif
>>   #endif
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 08/12] i2c: qcom-geni: Isolate serial engine setup

[Praveen Talari]

Hi Bjorn,

Thank you for review.

On 11/26/2025 9:00 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:14AM +0530, Praveen Talari wrote:
>> Move serial engine configuration from probe to geni_i2c_init().
>>
>> Relocating the serial engine setup to a dedicated initialization function
>> enhances code clarity and simplifies future modifications.
> 
> Please enhance commit message clarity. I don't think "code clarity" is
> your most significant reason for this change, and "simplifies future
> modification" is completely vague.
> 
> Be specific, the reader of this commit message hasn't implemented the
> next set of commits, so they don't understand why this helps.
> 
> If the reason is that this simplifies the error handling around the
> resource acquisition in the next patches, write that.
> 
> If my guess is wrong and the sole reason for you change is that you
> don't like 179 lines long functions, then just say that.
> 

Moving the serial engine setup to geni_i2c_init() API for a cleaner
probe function and utilizes the PM runtime API to control resources 
instead of direct clock-related APIs for better resource management.

Enables reusability of the serial engine initialization in future use 
cases like hibernation and deep sleep features where hardware context is 
lost.

I hope the commit text above should be appropriate.

Thanks,
Praveen

> Regards,
> Bjorn
> 
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 148 ++++++++++++++---------------
>>   1 file changed, 73 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 3a04016db2c3..4111afe2713e 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -976,10 +976,75 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
>>   	return ret;
>>   }
>>   
>> +static int geni_i2c_init(struct geni_i2c_dev *gi2c)
>> +{
>> +	const struct geni_i2c_desc *desc = NULL;
>> +	u32 proto, tx_depth;
>> +	bool fifo_disable;
>> +	int ret;
>> +
>> +	ret = pm_runtime_resume_and_get(gi2c->se.dev);
>> +	if (ret < 0) {
>> +		dev_err(gi2c->se.dev, "error turning on device :%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	proto = geni_se_read_proto(&gi2c->se);
>> +	if (proto == GENI_SE_INVALID_PROTO) {
>> +		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
>> +		if (ret) {
>> +			dev_err_probe(gi2c->se.dev, ret, "i2c firmware load failed ret: %d\n", ret);
>> +			goto err;
>> +		}
>> +	} else if (proto != GENI_SE_I2C) {
>> +		ret = dev_err_probe(gi2c->se.dev, -ENXIO, "Invalid proto %d\n", proto);
>> +		goto err;
>> +	}
>> +
>> +	desc = device_get_match_data(gi2c->se.dev);
>> +	if (desc && desc->no_dma_support)
>> +		fifo_disable = false;
>> +	else
>> +		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
>> +
>> +	if (fifo_disable) {
>> +		/* FIFO is disabled, so we can only use GPI DMA */
>> +		gi2c->gpi_mode = true;
>> +		ret = setup_gpi_dma(gi2c);
>> +		if (ret)
>> +			goto err;
>> +
>> +		dev_dbg(gi2c->se.dev, "Using GPI DMA mode for I2C\n");
>> +	} else {
>> +		gi2c->gpi_mode = false;
>> +		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>> +
>> +		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
>> +		if (!tx_depth && desc)
>> +			tx_depth = desc->tx_fifo_depth;
>> +
>> +		if (!tx_depth) {
>> +			ret = dev_err_probe(gi2c->se.dev, -EINVAL,
>> +					    "Invalid TX FIFO depth\n");
>> +			goto err;
>> +		}
>> +
>> +		gi2c->tx_wm = tx_depth - 1;
>> +		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
>> +		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
>> +				       PACKING_BYTES_PW, true, true, true);
>> +
>> +		dev_dbg(gi2c->se.dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
>> +	}
>> +
>> +err:
>> +	pm_runtime_put(gi2c->se.dev);
>> +	return ret;
>> +}
>> +
>>   static int geni_i2c_probe(struct platform_device *pdev)
>>   {
>>   	struct geni_i2c_dev *gi2c;
>> -	u32 proto, tx_depth, fifo_disable;
>>   	int ret;
>>   	struct device *dev = &pdev->dev;
>>   	const struct geni_i2c_desc *desc = NULL;
>> @@ -1059,79 +1124,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = clk_prepare_enable(gi2c->core_clk);
>> -	if (ret)
>> -		return ret;
>> -
>> -	ret = geni_se_resources_on(&gi2c->se);
>> -	if (ret) {
>> -		dev_err_probe(dev, ret, "Error turning on resources\n");
>> -		goto err_clk;
>> -	}
>> -	proto = geni_se_read_proto(&gi2c->se);
>> -	if (proto == GENI_SE_INVALID_PROTO) {
>> -		ret = geni_load_se_firmware(&gi2c->se, GENI_SE_I2C);
>> -		if (ret) {
>> -			dev_err_probe(dev, ret, "i2c firmware load failed ret: %d\n", ret);
>> -			goto err_resources;
>> -		}
>> -	} else if (proto != GENI_SE_I2C) {
>> -		ret = dev_err_probe(dev, -ENXIO, "Invalid proto %d\n", proto);
>> -		goto err_resources;
>> -	}
>> -
>> -	if (desc && desc->no_dma_support)
>> -		fifo_disable = false;
>> -	else
>> -		fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
>> -
>> -	if (fifo_disable) {
>> -		/* FIFO is disabled, so we can only use GPI DMA */
>> -		gi2c->gpi_mode = true;
>> -		ret = setup_gpi_dma(gi2c);
>> -		if (ret)
>> -			goto err_resources;
>> -
>> -		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>> -	} else {
>> -		gi2c->gpi_mode = false;
>> -		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>> -
>> -		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
>> -		if (!tx_depth && desc)
>> -			tx_depth = desc->tx_fifo_depth;
>> -
>> -		if (!tx_depth) {
>> -			ret = dev_err_probe(dev, -EINVAL,
>> -					    "Invalid TX FIFO depth\n");
>> -			goto err_resources;
>> -		}
>> -
>> -		gi2c->tx_wm = tx_depth - 1;
>> -		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
>> -		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
>> -				       PACKING_BYTES_PW, true, true, true);
>> -
>> -		dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
>> -	}
>> -
>> -	clk_disable_unprepare(gi2c->core_clk);
>> -	ret = geni_se_resources_off(&gi2c->se);
>> -	if (ret) {
>> -		dev_err_probe(dev, ret, "Error turning off resources\n");
>> -		goto err_dma;
>> -	}
>> -
>> -	ret = geni_icc_disable(&gi2c->se);
>> -	if (ret)
>> -		goto err_dma;
>> -
>>   	gi2c->suspended = 1;
>>   	pm_runtime_set_suspended(gi2c->se.dev);
>>   	pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
>>   	pm_runtime_use_autosuspend(gi2c->se.dev);
>>   	pm_runtime_enable(gi2c->se.dev);
>>   
>> +	ret =  geni_i2c_init(gi2c);
>> +	if (ret < 0) {
>> +		dev_err(gi2c->se.dev, "I2C init failed :%d\n", ret);
>> +		pm_runtime_disable(gi2c->se.dev);
>> +		goto err_dma;
>> +	}
>> +
>>   	ret = i2c_add_adapter(&gi2c->adap);
>>   	if (ret) {
>>   		dev_err_probe(dev, ret, "Error adding i2c adapter\n");
>> @@ -1143,13 +1148,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   
>>   	return ret;
>>   
>> -err_resources:
>> -	geni_se_resources_off(&gi2c->se);
>> -err_clk:
>> -	clk_disable_unprepare(gi2c->core_clk);
>> -
>> -	return ret;
>> -
>>   err_dma:
>>   	release_gpi_dma(gi2c);
>>   
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 10/12] i2c: qcom-geni: Use geni_se_resources_state() API in runtime PM functions

[Praveen Talari]

Hi Bjorn,

Thank you for review.

On 11/26/2025 9:08 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:16AM +0530, Praveen Talari wrote:
>> To manage GENI serial engine resources during runtime power management,
>> drivers currently need to call functions for ICC, clock, and
>> SE resource operations in both suspend and resume paths, resulting in
>> code duplication across drivers.
>>
>> The new geni_se_resources_state() helper API addresses this issue by
>> providing a streamlined method to enable or disable all resources
>> based on a boolean parameter, thereby eliminating redundancy across
>> drivers.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 28 +++++-----------------------
>>   1 file changed, 5 insertions(+), 23 deletions(-)
> 
> Nice to see such stats, which I presume will also show up in the other
> SE drivers later as well.

Sure.

> 
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 790cbca2c22e..ea117a4667e0 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -1166,18 +1166,15 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>   	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>   
>>   	disable_irq(gi2c->irq);
>> -	ret = geni_se_resources_off(&gi2c->se);
>> +
>> +	ret = geni_se_resources_state(&gi2c->se, false);
> 
> As I said in the previous patch, there's no reason to "set state to
> false", it's clearer to just have an "on" and an "off" function.

Sure. Will review and update in next patch

Thanks,
Praveen

> 
> Regards,
> Bjorn
> 
>>   	if (ret) {
>>   		enable_irq(gi2c->irq);
>>   		return ret;
>> -
>> -	} else {
>> -		gi2c->suspended = 1;
>>   	}
>>   
>> -	clk_disable_unprepare(gi2c->core_clk);
>> -
>> -	return geni_icc_disable(&gi2c->se);
>> +	gi2c->suspended = 1;
>> +	return ret;
>>   }
>>   
>>   static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>> @@ -1185,28 +1182,13 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>>   	int ret;
>>   	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>   
>> -	ret = geni_icc_enable(&gi2c->se);
>> +	ret = geni_se_resources_state(&gi2c->se, true);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = clk_prepare_enable(gi2c->core_clk);
>> -	if (ret)
>> -		goto out_icc_disable;
>> -
>> -	ret = geni_se_resources_on(&gi2c->se);
>> -	if (ret)
>> -		goto out_clk_disable;
>> -
>>   	enable_irq(gi2c->irq);
>>   	gi2c->suspended = 0;
>>   
>> -	return 0;
>> -
>> -out_clk_disable:
>> -	clk_disable_unprepare(gi2c->core_clk);
>> -out_icc_disable:
>> -	geni_icc_disable(&gi2c->se);
>> -
>>   	return ret;
>>   }
>>   
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms

[Praveen Talari]

Hi Bjorn,

On 11/26/2025 9:22 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:18AM +0530, Praveen Talari wrote:
>> The Qualcomm automotive SA8255p SoC relies on firmware to configure
>> platform resources, including clocks, interconnects and TLMM.
>> The driver requests resources operations over SCMI using power
>> and performance protocols.
>>
>> The SCMI power protocol enables or disables resources like clocks,
>> interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
>> such as resume/suspend, to control power states(on/off).
>>
>> The SCMI performance protocol manages I2C frequency, with each
>> frequency rate represented by a performance level. The driver uses
>> geni_se_set_perf_opp() API to request the desired frequency rate..
>>
>> As part of geni_se_set_perf_opp(), the OPP for the requested frequency
>> is obtained using dev_pm_opp_find_freq_floor() and the performance
>> level is set using dev_pm_opp_set_opp().
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++++++-------
>>   1 file changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index a0f68fdd4078..78154879f02d 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -82,6 +82,9 @@ struct geni_i2c_desc {
>>   	char *icc_ddr;
>>   	bool no_dma_support;
>>   	unsigned int tx_fifo_depth;
>> +	int (*resources_init)(struct geni_se *se);
>> +	int (*set_rate)(struct geni_se *se, unsigned long freq);
>> +	int (*power_state)(struct geni_se *se, bool state);
> 
> You have isolated this quite nicely now, so I'd prefer 3 (four to keep
> power on/off separate) if statements, over these function pointers, at
> this point.

Thank you for the feedback. I understand the preference for if 
statements, but function pointers offer better scalability here:

- Qualcomm has various power management schemes (Linux-driven vs 
firmware-assisted) across SoCs with more variants coming.
- If statements would require modifying the core driver logic for each 
new SoC variant, while function pointers isolate hardware-specific 
behavior to dedicated implementations.
- New SoC enablement becomes a matter of adding new function 
implementations rather than touching core logic.

Thanks,
Praveen
> 
> This saves the future reader from having to remember the combination of
> function pointer targets in the various cases - and allow things like
> "jump to definition" in your editor to still work.
> 
>>   };
>>   
>>   #define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC	2
>> @@ -203,8 +206,9 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>>   	return -EINVAL;
>>   }
>>   
>> -static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>> +static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
> 
> This sounds like a qcom_geni_i2c_set_rate() now that it takes a
> frequency argument.
Yes because of function pointer compatible

int (*set_rate)(struct geni_se *se, unsigned long freq);

Thanks,
Praveen
> 
> Regards,
> Bjorn
> 
>>   {
>> +	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
>>   	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>>   	u32 val;
>>   
>> @@ -217,6 +221,7 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>>   	val |= itr->t_low_cnt << LOW_COUNTER_SHFT;
>>   	val |= itr->t_cycle_cnt;
>>   	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
>> +	return 0;
>>   }
>>   
>>   static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
>> @@ -908,7 +913,9 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>>   		return ret;
>>   	}
>>   
>> -	qcom_geni_i2c_conf(gi2c);
>> +	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
>> +	if (ret)
>> +		return ret;
>>   
>>   	if (gi2c->gpi_mode)
>>   		ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
>> @@ -1041,8 +1048,9 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
>>   	return ret;
>>   }
>>   
>> -static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
>> +static int geni_i2c_resources_init(struct geni_se *se)
>>   {
>> +	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
>>   	int ret;
>>   
>>   	ret = geni_se_resources_init(&gi2c->se);
>> @@ -1095,7 +1103,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   	spin_lock_init(&gi2c->lock);
>>   	platform_set_drvdata(pdev, gi2c);
>>   
>> -	ret = geni_i2c_resources_init(gi2c);
>> +	ret = gi2c->dev_data->resources_init(&gi2c->se);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -1165,10 +1173,12 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>   
>>   	disable_irq(gi2c->irq);
>>   
>> -	ret = geni_se_resources_state(&gi2c->se, false);
>> -	if (ret) {
>> -		enable_irq(gi2c->irq);
>> -		return ret;
>> +	if (gi2c->dev_data->power_state) {
>> +		ret = gi2c->dev_data->power_state(&gi2c->se, false);
>> +		if (ret) {
>> +			enable_irq(gi2c->irq);
>> +			return ret;
>> +		}
>>   	}
>>   
>>   	gi2c->suspended = 1;
>> @@ -1180,9 +1190,11 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>>   	int ret;
>>   	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>   
>> -	ret = geni_se_resources_state(&gi2c->se, true);
>> -	if (ret)
>> -		return ret;
>> +	if (gi2c->dev_data->power_state) {
>> +		ret = gi2c->dev_data->power_state(&gi2c->se, true);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	enable_irq(gi2c->irq);
>>   	gi2c->suspended = 0;
>> @@ -1221,6 +1233,9 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
>>   
>>   static const struct geni_i2c_desc geni_i2c = {
>>   	.icc_ddr = "qup-memory",
>> +	.resources_init = geni_i2c_resources_init,
>> +	.set_rate = qcom_geni_i2c_conf,
>> +	.power_state = geni_se_resources_state,
>>   };
>>   
>>   static const struct geni_i2c_desc i2c_master_hub = {
>> @@ -1228,11 +1243,20 @@ static const struct geni_i2c_desc i2c_master_hub = {
>>   	.icc_ddr = NULL,
>>   	.no_dma_support = true,
>>   	.tx_fifo_depth = 16,
>> +	.resources_init = geni_i2c_resources_init,
>> +	.set_rate = qcom_geni_i2c_conf,
>> +	.power_state = geni_se_resources_state,
>> +};
>> +
>> +static const struct geni_i2c_desc sa8255p_geni_i2c = {
>> +	.resources_init = geni_se_domain_attach,
>> +	.set_rate = geni_se_set_perf_opp,
>>   };
>>   
>>   static const struct of_device_id geni_i2c_dt_match[] = {
>>   	{ .compatible = "qcom,geni-i2c", .data = &geni_i2c },
>>   	{ .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
>> +	{ .compatible = "qcom,sa8255p-geni-i2c", .data = &sa8255p_geni_i2c },
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 07/12] dt-bindings: i2c: Describe SA8255p

[Praveen Talari]

Hi Krzysztof,

On 11/26/2025 10:32 AM, Praveen Talari wrote:
> Hi Krzysztof,
> 
> On 11/22/2025 5:10 PM, Krzysztof Kozlowski wrote:
>> On Sat, Nov 22, 2025 at 10:30:13AM +0530, Praveen Talari wrote:
>>> +  dmas:
>>> +    maxItems: 2
>>> +
>>> +  dma-names:
>>> +    items:
>>> +      - const: tx
>>> +      - const: rx
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  power-domains:
>>> +    minItems: 2
>>
>> Drop
>>
>>> +    maxItems: 2
>>> +
>>> +  power-domain-names:
>>> +    items:
>>> +      - const: power
>>> +      - const: perf
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - power-domains
>>> +
>>> +allOf:
>>
>> So common SE properties are not applicable? If so explain why in the
>> commit msg.
>>
>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    i2c@a90000 {
>>> +        compatible = "qcom,sa8255p-geni-i2c";
>>> +        reg = <0xa90000 0x4000>;
>>> +        interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
>>> +        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
>>> +        power-domain-names = "power", "perf";
>>
>> dmas and dma-names
> 
> For this platform (all Auto targets), we primarily use FIFO/CPU_DMA mode 
> rather than GSI mode, and these are not defined in the Device Tree file 
> as well now. Should we still include the dmas and dma-names properties 
> in the example node?

Would you mind confirming this?

Thanks,
Praveen

> 
> Thanks,
> Praveen
> 
>>
>> Best regards,
>> Krzysztof
>>