[PATCH v3 0/8] soc: qcom: geni: Derive SE clock configuration from OPP table on SA8255P

[PATCH v3 0/8] soc: qcom: geni: Derive SE clock configuration from OPP table on SA8255P

[Praveen Talari]

On firmware-managed platforms such as SA8255P, there is no Linux clock
handler available to determine the appropriate SE source clock, source
clock index, and divider values for a requested protocol frequency.
However, these parameters are required when programming GSI TREs, where
the hardware expects an explicit clock source selection and divider
configuration for the serial engine.

In contrast, platforms using Linux-managed clocks derive these
parameters through geni_se_clk_freq_match() using the source clock
information stored in clk_perf_tbl. Since the firmware-managed path
lacks equivalent clock information, protocol drivers cannot reuse the
existing frequency matching logic and instead rely on a direct mapping
between protocol-requested frequencies and performance levels. This
creates a separate clock configuration flow and prevents
firmware-managed platforms from deriving the actual SE clock parameters
required for GSI TRE programming.

To address this limitation, the performance-domain OPP table is treated
as the representation of SE-supported source clock frequencies. During
geni_se_domain_attach(), the OPP entries are used to populate
clk_perf_tbl and related clock performance data, allowing
firmware-managed platforms to leverage the same clock frequency matching
infrastructure used by Linux-managed platforms.

With this change, protocol drivers can use geni_se_clk_freq_match() to
select the closest supported source clock frequency for a requested
protocol rate, derive the corresponding source clock index and divider
values required for GSI TRE programming, and apply the matched clock
through the OPP framework. This removes the dependency on direct
protocol-frequency-to-performance-level mappings and provides a common
clock selection and configuration mechanism across both firmware-managed
and Linux-managed GENI deployments.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
Changes in v3:
- Added a return check for set_rate.
- Remove redundant source-frequency fields from clock tables.
- Store the validated frequency in gi2c->src_freq.
- Use it for OPP configuration and runtime resume.
- Preserve exact source-frequency validation.
- Simplify OPP level iteration in geni_se_domain_attach().
- Keep level initialisation outside the loop.
- Increment level explicitly after processing each OPP.
- Link to v2: https://patch.msgid.link/20260827-derive_clk_perf_tbl_from_perf_domain_opp_table-v2-0-091697dbeb02@oss.qualcomm.com

Changes in v2:
- Add new patch to remove OPP rate reset from resource deactivation
- Add new patch to vote source frequency via OPP for I2C
- Compress the SPI, I2C and serial commit messages per review feedback
- Drop a stray blank line before trace_geni_i2c_bus_setup() in the
  I2C resource-initialization helper commit.
- Link to v1: https://patch.msgid.link/20260805-derive_clk_perf_tbl_from_perf_domain_opp_table-v1-0-61171ab1cdce@oss.qualcomm.com

---
Praveen Talari (8):
      soc: qcom: geni-se: Populate clk_perf_tbl with SE source clock frequencies from perf OPP table
      soc: qcom: geni-se: Add helper to set SE clock rate via OPP
      soc: qcom: geni-se: Remove OPP rate reset from resource deactivation
      serial: qcom-geni: Use geni_se_set_rate() for source clock configuration
      spi: qcom-geni: Use geni_se_set_rate() for setting source clock frequency
      i2c: qcom-geni: Vote for SE clock rate using OPP
      i2c: qcom-geni: Use common GENI resource initialization helper
      i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

 drivers/i2c/busses/i2c-qcom-geni.c    | 58 ++++++++++++++++----------------
 drivers/soc/qcom/qcom-geni-se.c       | 63 +++++++++++++++++++++++++++++++----
 drivers/spi/spi-geni-qcom.c           | 24 ++++++-------
 drivers/tty/serial/qcom_geni_serial.c | 26 ++++++++-------
 include/linux/soc/qcom/geni-se.h      |  2 ++
 5 files changed, 113 insertions(+), 60 deletions(-)
---
base-commit: 1fffefb00cc0b0da1f2ae24e3d33364530ea6515
change-id: 20260805-derive_clk_perf_tbl_from_perf_domain_opp_table-2f29ad32226a

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>

[PATCH v3 1/8] soc: qcom: geni-se: Populate clk_perf_tbl with SE source clock frequencies from perf OPP table

[Praveen Talari]

Currently, on the SA8255P platform, protocol drivers attached via
geni_se_domain_attach() treat each OPP on the perf domain as directly
corresponding to a protocol value such as a baudrate or requested
frequency, and simply request that OPP via
geni_se_set_perf_level()/geni_se_set_perf_opp(). This does not allow
computing a source clock and divider combination for a protocol
requested frequency, unlike the Linux clock managed path which derives
this from se->clk_perf_tbl via geni_se_clk_freq_match(), and then
applies the matched source clock frequency with dev_pm_opp_set_rate().

Change this by treating the OPP table exposed on the perf domain
device as representing the actual SE HW supported source clock
frequencies, the same role clk_perf_tbl plays for the Linux clock
managed path. Populate se->clk_perf_tbl and se->num_clk_levels by
iterating over this OPP table in geni_se_domain_attach(), so that
protocol drivers on the firmware managed (SA8255P) path can also use
geni_se_clk_freq_match() to pick the closest supported source clock
frequency and calculate the required divider, and apply it with
dev_pm_opp_set_rate() the same way as it is done for the Linux clock
managed path, instead of relying on a direct frequency/baudrate-to-
perf-level mapping.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 074087c592cc..95eeb9438a29 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1159,7 +1159,8 @@ EXPORT_SYMBOL_GPL(geni_se_set_perf_opp);
  *
  * This function attaches the power domains ("power" and "perf") required
  * in the SCMI auto-VM environment to the GENI Serial Engine device. It
- * initializes se->pd_list with the attached domains.
+ * initializes se->pd_list with the attached domains, and populates
+ * se->clk_perf_tbl from the OPP table of the "perf" domain device.
  *
  * Return: 0 on success, or a negative error code on failure.
  */
@@ -1170,7 +1171,12 @@ int geni_se_domain_attach(struct geni_se *se)
 		.pd_names = (const char*[]) { "power", "perf" },
 		.num_pd_names = 2,
 	};
+	struct device *perf_dev;
+	struct dev_pm_opp *opp;
+	unsigned int level;
+	int num_opps;
 	int ret;
+	int i;
 
 	ret = devm_pm_domain_attach_list(se->dev,
 					 &pd_data, &se->pd_list);
@@ -1179,6 +1185,32 @@ int geni_se_domain_attach(struct geni_se *se)
 	else if (ret < 0)
 		return ret;
 
+	perf_dev = se->pd_list->pd_devs[DOMAIN_IDX_PERF];
+
+	num_opps = dev_pm_opp_get_opp_count(perf_dev);
+	if (num_opps < 0)
+		return num_opps;
+	if (num_opps == 0)
+		return -ENODEV;
+
+	se->clk_perf_tbl = devm_kcalloc(se->dev, num_opps,
+					sizeof(*se->clk_perf_tbl),
+					GFP_KERNEL);
+	if (!se->clk_perf_tbl)
+		return -ENOMEM;
+
+	level = 0;
+	for (i = 0; i < num_opps; i++) {
+		opp = dev_pm_opp_find_level_ceil(perf_dev, &level);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+
+		se->clk_perf_tbl[i] = level;
+		dev_pm_opp_put(opp);
+		level++;
+	}
+	se->num_clk_levels = num_opps;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(geni_se_domain_attach);

-- 
2.34.1

Re: [PATCH v3 1/8] soc: qcom: geni-se: Populate clk_perf_tbl with SE source clock frequencies from perf OPP table

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soc: qcom: geni-se: clk_perf_tbl populated with OPP levels instead of frequencies
--

commit 2006551310c45129142551fa77079692b29b39f8
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

soc: qcom: geni-se: Populate clk_perf_tbl with SE source clock frequencies from perf OPP table

This commit aims to populate clk_perf_tbl using the OPP table exposed on
the perf domain device. This is intended to allow protocol drivers on the
firmware managed path to calculate required dividers and pick supported
source clock frequencies similarly to the Linux clock managed path.

> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 074087c592cc3..95eeb9438a292 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
[ ... ]
> @@ -1170,7 +1171,12 @@ int geni_se_domain_attach(struct geni_se *se)
[ ... ]
> +	level = 0;
> +	for (i = 0; i < num_opps; i++) {
> +		opp = dev_pm_opp_find_level_ceil(perf_dev, &level);
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +
> +		se->clk_perf_tbl[i] = level;

[Severity: High]
Does this populate clk_perf_tbl with abstract performance levels instead of
actual clock frequencies?

The commit message states clk_perf_tbl should be populated with actual source
clock frequencies so protocol drivers can calculate dividers and set the rate
via dev_pm_opp_set_rate(). However, by using dev_pm_opp_find_level_ceil()
in geni_se_domain_attach(), it appears this loop stores the performance
level (an abstract index like 16 or 64) rather than the frequency in Hz.

For SCMI performance domains, this abstract level will break the divider math
in geni_se_clk_freq_match(), leading to massive deltas or invalid dividers.
It will also cause dev_pm_opp_set_rate() to receive tiny abstract levels
instead of Hz. Because dev_pm_opp_set_rate() searches the OPP table for a
matching frequency, passing the abstract level will incorrectly match the
lowest available frequency and likely break protocol communication.

Should this use dev_pm_opp_find_freq_ceil() and track a frequency variable
to store the actual rates in clk_perf_tbl?

> +		dev_pm_opp_put(opp);
> +		level++;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=1

[PATCH v3 2/8] soc: qcom: geni-se: Add helper to set SE clock rate via OPP

[Praveen Talari]

GENI protocol drivers need a common way to scale the SE source clock
through the OPP framework. However, the device that owns the OPP table
differs depending on how the SE resources are managed. For Linux clock
managed platforms, the OPP table is associated with the SE device,
whereas on firmware-managed platforms it is associated with the
performance power-domain device. This requires protocol drivers to be
aware of the underlying resource management model when requesting
frequency changes.

Introduce geni_se_set_rate(), a common helper that abstracts this
difference and applies the requested frequency through the appropriate
device. The helper automatically selects the performance-domain device
when power domains are attached and falls back to the SE device
otherwise.

Acked-by: Mukesh Kumar Savaliya <mukesh.savaliya@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 |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 95eeb9438a29..97905bd6d363 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1153,6 +1153,28 @@ int geni_se_set_perf_opp(struct geni_se *se, unsigned long clk_freq)
 }
 EXPORT_SYMBOL_GPL(geni_se_set_perf_opp);
 
+/**
+ * geni_se_set_rate() - Set the SE source clock rate via the OPP framework.
+ * @se: Pointer to the struct geni_se instance.
+ * @freq: The source clock frequency to set.
+ *
+ * Applies the given frequency through dev_pm_opp_set_rate(), targeting the
+ * perf domain device when the SE has power domains attached (firmware
+ * managed path), or se->dev otherwise (Linux clock managed path).
+ *
+ * Return: 0 on success, or a negative error code on failure.
+ */
+int geni_se_set_rate(struct geni_se *se, unsigned long freq)
+{
+	struct device *perf_dev = se->dev;
+
+	if (se->pd_list)
+		perf_dev = se->pd_list->pd_devs[DOMAIN_IDX_PERF];
+
+	return dev_pm_opp_set_rate(perf_dev, freq);
+}
+EXPORT_SYMBOL_GPL(geni_se_set_rate);
+
 /**
  * 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 b3372a3147a0..8c3c5b68151b 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -597,5 +597,7 @@ 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);
+
+int geni_se_set_rate(struct geni_se *se, unsigned long freq);
 #endif
 #endif

-- 
2.34.1

Re: [PATCH v3 2/8] soc: qcom: geni-se: Add helper to set SE clock rate via OPP

[sashiko-bot]

> GENI protocol drivers need a common way to scale the SE source clock
> through the OPP framework. However, the device that owns the OPP table
> differs depending on how the SE resources are managed. For Linux clock
> managed platforms, the OPP table is associated with the SE device,
> whereas on firmware-managed platforms it is associated with the
> performance power-domain device. This requires protocol drivers to be
> aware of the underlying resource management model when requesting
> frequency changes.
> 
> Introduce geni_se_set_rate(), a common helper that abstracts this
> difference and applies the requested frequency through the appropriate
> device. The helper automatically selects the performance-domain device
> when power domains are attached and falls back to the SE device
> otherwise.
> 
> Acked-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=2

[PATCH v3 3/8] soc: qcom: geni-se: Remove OPP rate reset from resource deactivation

[Praveen Talari]

Remove the dev_pm_opp_set_rate(se->dev, 0) call from
geni_se_resources_deactivate().

OPP resource management should be controlled by the consumer driver.
Additionally, clocks are already disabled by geni_se_clks_off(), making
the OPP rate reset unnecessary during resource deactivation.

Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/soc/qcom/qcom-geni-se.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 97905bd6d363..6163c19c6cf8 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1043,8 +1043,8 @@ EXPORT_SYMBOL_GPL(geni_icc_disable);
  * geni_se_resources_deactivate() - Deactivate GENI SE device resources
  * @se: Pointer to the geni_se structure
  *
- * Deactivates device resources for power saving: OPP rate to 0, pin control
- * to sleep state, turns off clocks, and disables interconnect. Skips ACPI devices.
+ * Deactivates device resources for power saving: pin control to sleep
+ * state, turns off clocks, and disables interconnect. Skips ACPI devices.
  *
  * Return: 0 on success, negative error code on failure
  */
@@ -1055,9 +1055,6 @@ int geni_se_resources_deactivate(struct geni_se *se)
 	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;

-- 
2.34.1

Re: [PATCH v3 3/8] soc: qcom: geni-se: Remove OPP rate reset from resource deactivation

[sashiko-bot]

> Remove the dev_pm_opp_set_rate(se->dev, 0) call from
> geni_se_resources_deactivate().
> 
> OPP resource management should be controlled by the consumer driver.
> Additionally, clocks are already disabled by geni_se_clks_off(), making
> the OPP rate reset unnecessary during resource deactivation.
> 
> Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=3

[PATCH v3 4/8] serial: qcom-geni: Use geni_se_set_rate() for source clock configuration

[Praveen Talari]

On the SA8255P platform there is no Linux clock handler for the SE
source clock, so the driver has no way to compute the divider needed
for a requested baud rate. To work around this, firmware instead
exposes the SE HW supported source clock frequencies as OPPs on the
perf domain device.

geni_se_domain_attach() now populates clk_perf_tbl from
the performance-domain OPP table, allowing firmware-managed
platforms to use geni_se_clk_freq_match().

Call geni_serial_set_rate() unconditionally from
qcom_geni_serial_set_termios(), and use geni_se_set_rate() instead of
dev_pm_opp_set_rate() so clock programming is handled centrally by
the GENI core, independent of the underlying clock-control mechanism.

Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/tty/serial/qcom_geni_serial.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 4a6f526340b8..23e423009255 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -113,7 +113,6 @@ struct qcom_geni_device_data {
 	bool console;
 	enum geni_se_xfer_mode mode;
 	int (*resources_init)(struct geni_se *se);
-	int (*set_rate)(struct geni_se *se, unsigned long baud);
 	int (*power_on)(struct geni_se *se);
 	int (*power_off)(struct geni_se *se);
 };
@@ -1461,7 +1460,10 @@ static int geni_serial_set_rate(struct geni_se *se, unsigned long baud)
 
 	uport->uartclk = clk_rate;
 	port->clk_rate = clk_rate;
-	dev_pm_opp_set_rate(uport->dev, clk_rate);
+	ret = geni_se_set_rate(&port->se, clk_rate);
+	if (ret)
+		return ret;
+
 	ser_clk_cfg = SER_CLK_EN;
 	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
 
@@ -1499,7 +1501,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	/* baud rate */
 	baud = uart_get_baud_rate(uport, termios, old, 300, 8000000);
 
-	ret = port->dev_data->set_rate(&port->se, baud);
+	ret = geni_serial_set_rate(&port->se, baud);
 	if (ret)
 		return;
 
@@ -2051,15 +2053,19 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
 static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
 {
 	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
+	int ret;
 
-	return port->dev_data->power_off ?
-	       port->dev_data->power_off(&port->se) : 0;
+	ret = port->dev_data->power_off ?
+	      port->dev_data->power_off(&port->se) : 0;
+	if (ret)
+		return ret;
+
+	return geni_se_set_rate(&port->se, 0);
 }
 
 static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
 {
 	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
-	struct uart_port *uport = &port->uport;
 	int ret;
 
 	if (port->dev_data->power_on) {
@@ -2068,8 +2074,8 @@ static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
 			return ret;
 	}
 
-	if (port->se.has_opp && port->clk_rate)
-		return dev_pm_opp_set_rate(uport->dev, port->clk_rate);
+	if (port->clk_rate)
+		return geni_se_set_rate(&port->se, port->clk_rate);
 
 	return 0;
 }
@@ -2130,7 +2136,6 @@ static const struct qcom_geni_device_data qcom_geni_console_data = {
 	.console = true,
 	.mode = GENI_SE_FIFO,
 	.resources_init = geni_se_resources_init,
-	.set_rate = geni_serial_set_rate,
 	.power_on = geni_se_resources_activate,
 	.power_off = geni_se_resources_deactivate,
 };
@@ -2139,7 +2144,6 @@ static const struct qcom_geni_device_data sa8255p_qcom_geni_console_data = {
 	.console = true,
 	.mode = GENI_SE_FIFO,
 	.resources_init = geni_se_domain_attach,
-	.set_rate = geni_se_set_perf_level,
 };
 #endif
 
@@ -2147,7 +2151,6 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
 	.console = false,
 	.mode = GENI_SE_DMA,
 	.resources_init = geni_se_resources_init,
-	.set_rate = geni_serial_set_rate,
 	.power_on = geni_se_resources_activate,
 	.power_off = geni_se_resources_deactivate,
 };
@@ -2156,7 +2159,6 @@ static const struct qcom_geni_device_data sa8255p_qcom_geni_uart_data = {
 	.console = false,
 	.mode = GENI_SE_DMA,
 	.resources_init = geni_se_domain_attach,
-	.set_rate = geni_se_set_perf_level,
 };
 
 static const struct dev_pm_ops qcom_geni_serial_pm_ops = {

-- 
2.34.1

Re: [PATCH v3 4/8] serial: qcom-geni: Use geni_se_set_rate() for source clock configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] serial: PM state desync in qcom_geni_serial_runtime_suspend() error path
- [High] DVFS sequencing violation: clock ungated before voltage restored
- [Medium] tty: serial: qcom-geni: missing power_off rollback on geni_se_set_rate failure
- [Medium] tty: qcom_geni_serial: Inconsistent state on geni_se_set_rate() failure
--

commit c05165e9ed1c294def3b3c9c4244bbde141fcad4
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

serial: qcom-geni: Use geni_se_set_rate() for source clock configuration

This commit transitions the QCOM GENI serial driver to use
geni_se_set_rate() for source clock configuration. This allows
firmware-managed platforms to handle clock programming centrally by the
GENI core.

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 4a6f526340b8d..23e423009255b 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
[ ... ]
> @@ -1461,7 +1460,10 @@ static int geni_serial_set_rate(struct geni_se *se, unsigned long baud)
>  
>  	uport->uartclk = clk_rate;
>  	port->clk_rate = clk_rate;
> -	dev_pm_opp_set_rate(uport->dev, clk_rate);
> +	ret = geni_se_set_rate(&port->se, clk_rate);
> +	if (ret)
> +		return ret;
> +

[Severity: Medium]
If geni_se_set_rate() fails here, the hardware clock divider remains
unchanged, but uport->uartclk and port->clk_rate are updated without being
rolled back.

Does this leave the software tracking variables advertising a new baud rate
while the hardware remains at the old rate? On a subsequent
qcom_geni_serial_runtime_resume(), the system might restore the new
port->clk_rate to the hardware, resulting in the source clock running at
the new rate while the hardware still uses the old clock divider from
before the failed termios update, permanently garbling the UART baud rate.

[ ... ]
> @@ -2051,15 +2053,19 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>  static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
>  {
>  	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> +	int ret;
>  
> -	return port->dev_data->power_off ?
> -	       port->dev_data->power_off(&port->se) : 0;
> +	ret = port->dev_data->power_off ?
> +	      port->dev_data->power_off(&port->se) : 0;
> +	if (ret)
> +		return ret;
> +
> +	return geni_se_set_rate(&port->se, 0);
>  }

[Severity: High]
If geni_se_set_rate(&port->se, 0) fails, this returns an error after the
device was successfully powered off.

When a runtime_suspend callback returns an error, the PM core assumes the
suspend failed and keeps the device's runtime state as RPM_ACTIVE. Since the
PM core considers the device logically active, will subsequent UART accesses
bypass runtime_resume() and directly access the physically powered off
hardware, resulting in a fatal bus error (synchronous external abort)?

[ ... ]
>  static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
>  {
>  	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> -	struct uart_port *uport = &port->uport;
>  	int ret;
>  
>  	if (port->dev_data->power_on) {
> @@ -2068,8 +2074,8 @@ static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
>  			return ret;
>  	}
>  
> -	if (port->se.has_opp && port->clk_rate)
> -		return dev_pm_opp_set_rate(uport->dev, port->clk_rate);
> +	if (port->clk_rate)
> +		return geni_se_set_rate(&port->se, port->clk_rate);

[Severity: High]
Does this sequence violate DVFS sequencing constraints?

During runtime_suspend, the OPP performance state drops via
geni_se_set_rate(&port->se, 0), which drops the voltage but leaves the
underlying clock frequency unchanged. Here during runtime_resume,
port->dev_data->power_on() (which invokes clk_prepare_enable()) is called
before geni_se_set_rate(&port->se, port->clk_rate) restores the voltage.

Could this result in the clock being ungated at its original high frequency
while the voltage is still dropped, leading to hardware lockups or system
reboots?


[Severity: Medium]
Also, if geni_se_set_rate() fails here, it returns an error to the PM core
without calling port->dev_data->power_off() to roll back the resources
enabled by power_on().

Since the PM core marks the device as RPM_SUSPENDED upon a failed resume
without invoking the suspend callback, does this permanently leak
interconnect bandwidth votes and clock enable counts, and leave the device
physically powered?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=4

[PATCH v3 5/8] spi: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

[Praveen Talari]

On the SA8255P platform there is no Linux clock handler for the SE
source clock, so the driver has no way to look up the source clock
index and divider needed to fill the GSI CONFIG0 TRE for a requested
transfer frequency. To work around this, firmware instead exposes the
SE HW supported source clock frequencies as OPPs on the perf domain
device.

geni_se_domain_attach() now populates clk_perf_tbl from
the performance-domain OPP table, allowing firmware-managed
platforms to use geni_se_clk_freq_match().

Use geni_se_set_rate() instead of dev_pm_opp_set_rate()
so clock programming is handled centrally by the GENI core,
independent of the underlying clock-control mechanism.

Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/spi/spi-geni-qcom.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 91e75e213f92..cbccd6d8689a 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -18,7 +18,6 @@
 #include <linux/module.h>
 #include <linux/panic_notifier.h>
 #include <linux/platform_device.h>
-#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/soc/qcom/geni-se.h>
@@ -85,7 +84,6 @@
 
 struct geni_spi_desc {
 	int (*resources_init)(struct geni_se *se);
-	int (*set_rate)(struct geni_se *se, unsigned long clk_freq);
 	int (*power_on)(struct geni_se *se);
 	int (*power_off)(struct geni_se *se);
 };
@@ -155,9 +153,9 @@ static int get_spi_clk_cfg(unsigned int speed_hz,
 
 	dev_dbg(mas->dev, "req %u=>%u sclk %lu, idx %d, div %d\n", speed_hz,
 				actual_hz, sclk_freq, *clk_idx, *clk_div);
-	ret = dev_pm_opp_set_rate(mas->dev, sclk_freq);
+	ret = geni_se_set_rate(&mas->se, sclk_freq);
 	if (ret)
-		dev_err(mas->dev, "dev_pm_opp_set_rate failed %d\n", ret);
+		dev_err(mas->dev, "geni_se_set_rate failed %d\n", ret);
 	else
 		mas->cur_sclk_hz = sclk_freq;
 
@@ -850,7 +848,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
 	}
 
 	/* Speed and bits per word can be overridden per transfer */
-	ret = mas->dev_data->set_rate(&mas->se, xfer->speed_hz);
+	ret = geni_spi_set_clock_and_bw(&mas->se, xfer->speed_hz);
 	if (ret)
 		return ret;
 
@@ -1248,9 +1246,14 @@ static int spi_geni_runtime_suspend(struct device *dev)
 {
 	struct spi_controller *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
+	int ret;
+
+	ret = mas->dev_data->power_off ?
+	      mas->dev_data->power_off(&mas->se) : 0;
+	if (ret)
+		return ret;
 
-	return mas->dev_data->power_off ?
-	       mas->dev_data->power_off(&mas->se) : 0;
+	return geni_se_set_rate(&mas->se, 0);
 }
 
 static int spi_geni_runtime_resume(struct device *dev)
@@ -1265,10 +1268,7 @@ static int spi_geni_runtime_resume(struct device *dev)
 			return ret;
 	}
 
-	if (mas->se.has_opp)
-		return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
-
-	return 0;
+	return geni_se_set_rate(&mas->se, mas->cur_sclk_hz);
 }
 
 static int spi_geni_suspend(struct device *dev)
@@ -1310,14 +1310,12 @@ static const struct dev_pm_ops spi_geni_pm_ops = {
 
 static const struct geni_spi_desc geni_spi = {
 	.resources_init = geni_se_resources_init,
-	.set_rate = geni_spi_set_clock_and_bw,
 	.power_on = geni_se_resources_activate,
 	.power_off = geni_se_resources_deactivate,
 };
 
 static const struct geni_spi_desc sa8255p_geni_spi = {
 	.resources_init = geni_se_domain_attach,
-	.set_rate = geni_se_set_perf_opp,
 };
 
 static const struct of_device_id spi_geni_dt_match[] = {

-- 
2.34.1

Re: [PATCH v3 5/8] spi: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: spi_geni_runtime_suspend() desynchronizes PM state if geni_se_set_rate() fails
- [Medium] pm: spi_geni_runtime_resume() leaks clock/power resources on geni_se_set_rate() error
--

commit 18dd7909ccc7f7c6dc80af1c7154961b4692ab24
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

On the SA8255P platform there is no Linux clock handler for the SE
source clock, so the driver has no way to look up the source clock
index and divider needed to fill the GSI CONFIG0 TRE for a requested
transfer frequency. To work around this, firmware instead exposes the
SE HW supported source clock frequencies as OPPs on the perf domain
device.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 91e75e213f929..cbccd6d8689ab 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
[ ... ]
> @@ -1248,9 +1246,14 @@ static int spi_geni_runtime_suspend(struct device *dev)
>  {
>  	struct spi_controller *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> +	int ret;
> +
> +	ret = mas->dev_data->power_off ?
> +	      mas->dev_data->power_off(&mas->se) : 0;
> +	if (ret)
> +		return ret;
>  
> -	return mas->dev_data->power_off ?
> -	       mas->dev_data->power_off(&mas->se) : 0;
> +	return geni_se_set_rate(&mas->se, 0);
>  }

[Severity: High]
Does this desynchronize the PM state if geni_se_set_rate() fails?

If mas->dev_data->power_off() successfully powers off the device (disabling
clocks, interconnects, etc.), but geni_se_set_rate() subsequently fails and
returns a negative error code, the PM core will abort the suspend transition
and leave dev->power.runtime_status as RPM_ACTIVE.

Since the PM core considers the device active, will subsequent operations
bypass the pm_runtime_get_sync() resume path and directly access hardware
registers while the clocks are actually disabled, potentially leading to an
unclocked access exception?

Could the error from geni_se_set_rate() be ignored or handled gracefully,
or should geni_se_set_rate(0) be called before power_off()?

>  static int spi_geni_runtime_resume(struct device *dev)
> @@ -1265,10 +1268,7 @@ static int spi_geni_runtime_resume(struct device *dev)
>  			return ret;
>  	}
>  
> -	if (mas->se.has_opp)
> -		return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
> -
> -	return 0;
> +	return geni_se_set_rate(&mas->se, mas->cur_sclk_hz);
>  }

[Severity: Medium]
Can this leak clock and power resources if geni_se_set_rate() fails?

If power_on() successfully enables the device clocks and interconnects, but
geni_se_set_rate() fails and returns an error, this function returns that
error directly without first calling power_off() to unwind the state.

The PM core will then mark the resume as failed and transition the device
state back to RPM_SUSPENDED, orphaning the active hardware resources.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=5

[PATCH v3 6/8] i2c: qcom-geni: Vote for SE clock rate using OPP

[Praveen Talari]

The GENI I2C driver selects a source clock based on the requested bus
frequency and programs the corresponding clock selector. However, it
does not request the selected source clock rate when OPP-based clock
scaling is used.

Store the matched source clock frequency and vote for it through
dev_pm_opp_set_rate() before programming the clock selector. Also remove
and restore the vote during runtime suspend and resume.

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

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 5f3d260a3913..a6fe89d0934f 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/soc/qcom/geni-se.h>
 #include <linux/spinlock.h>
@@ -136,6 +137,7 @@ struct geni_i2c_dev {
 	u32 clk_freq_out;
 	const struct geni_i2c_clk_fld *clk_fld;
 	u32 clk_idx;
+	unsigned long src_freq;
 	void *dma_buf;
 	size_t xfer_len;
 	dma_addr_t dma_addr;
@@ -206,7 +208,7 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map_32mhz[] = {
 static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
 {
 	const struct geni_i2c_clk_fld *itr;
-	unsigned long req_freq, res_freq;
+	unsigned long req_freq;
 	int ret;
 
 	/*
@@ -223,14 +225,14 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
 	}
 
 	ret = geni_se_clk_freq_match(&gi2c->se, req_freq, &gi2c->clk_idx,
-				     &res_freq, true);
+				     &gi2c->src_freq, true);
 	if (ret)
 		return dev_err_probe(gi2c->se.dev, ret,
 				     "Failed to find SE source clock %lu Hz\n",
 				     req_freq);
-	if (res_freq != req_freq) {
+	if (gi2c->src_freq != req_freq) {
 		dev_err(gi2c->se.dev, "Unsupported SE source clock %lu Hz\n",
-			res_freq);
+			gi2c->src_freq);
 		return -EINVAL;
 	}
 
@@ -255,6 +257,11 @@ 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;
+	int ret;
+
+	ret = dev_pm_opp_set_rate(se->dev, gi2c->src_freq);
+	if (ret)
+		return ret;
 
 	writel_relaxed(gi2c->clk_idx, gi2c->se.base + SE_GENI_CLK_SEL);
 
@@ -1286,7 +1293,7 @@ static int geni_i2c_runtime_suspend(struct device *dev)
 		}
 	}
 
-	return 0;
+	return dev_pm_opp_set_rate(dev, 0);
 }
 
 static int geni_i2c_runtime_resume(struct device *dev)
@@ -1300,6 +1307,12 @@ static int geni_i2c_runtime_resume(struct device *dev)
 			return ret;
 	}
 
+	if (gi2c->src_freq) {
+		ret = dev_pm_opp_set_rate(dev, gi2c->src_freq);
+		if (ret)
+			return ret;
+	}
+
 	enable_irq(gi2c->irq);
 
 	return 0;

-- 
2.34.1

Re: [PATCH v3 6/8] i2c: qcom-geni: Vote for SE clock rate using OPP

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: qcom-geni: missing power_off rollback on opp_set_rate failure in resume
- [High] i2c: qcom-geni: PM state desync on opp_set_rate failure in suspend
- [High] i2c: PM desync in suspend causes unclocked access crash
--

commit e5bbe927d2b6dbf7ee2cdbdeb6104c05147c9959
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

i2c: qcom-geni: Vote for SE clock rate using OPP
    
The GENI I2C driver selects a source clock based on the requested bus
frequency and programs the corresponding clock selector. However, it
does not request the selected source clock rate when OPP-based clock
scaling is used.
    
Store the matched source clock frequency and vote for it through
dev_pm_opp_set_rate() before programming the clock selector. Also remove
and restore the vote during runtime suspend and resume.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 5f3d260a3913b..a6fe89d0934f6 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -255,6 +257,11 @@ 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;
> +	int ret;
> +
> +	ret = dev_pm_opp_set_rate(se->dev, gi2c->src_freq);
> +	if (ret)
> +		return ret;
>  
>  	writel_relaxed(gi2c->clk_idx, gi2c->se.base + SE_GENI_CLK_SEL);

[Severity: High]
If dev_pm_opp_set_rate(dev, 0) fails during geni_i2c_runtime_suspend(), the
PM core will leave the device state as RPM_ACTIVE even though the physical
clocks were disabled.

On the next transfer, pm_runtime_resume_and_get() would see RPM_ACTIVE and
skip calling geni_i2c_runtime_resume(). This leads to qcom_geni_i2c_conf()
where dev_pm_opp_set_rate(dev, gi2c->src_freq) succeeds, but writel_relaxed()
is executed while the physical hardware is still unclocked.

Could this sequence trigger a fatal bus error?

[ ... ]

> @@ -1286,7 +1293,7 @@ static int geni_i2c_runtime_suspend(struct device *dev)
>  		}
>  	}
>  
> -	return 0;
> +	return dev_pm_opp_set_rate(dev, 0);

[Severity: High]
If dev_pm_opp_set_rate(dev, 0) fails here, the error is returned to the
runtime PM core. Does this leave the PM core thinking the device is RPM_ACTIVE
while the hardware clocks were already physically turned off by the preceding
gi2c->dev_data->power_off() call?

>  }
>  
>  static int geni_i2c_runtime_resume(struct device *dev)
> @@ -1300,6 +1307,12 @@ static int geni_i2c_runtime_resume(struct device *dev)
>  			return ret;
>  	}
>  
> +	if (gi2c->src_freq) {
> +		ret = dev_pm_opp_set_rate(dev, gi2c->src_freq);
> +		if (ret)
> +			return ret;

[Severity: High]
If dev_pm_opp_set_rate() fails, this returns immediately. Does this skip the
gi2c->dev_data->power_off() rollback and enable_irq() calls, leaving the
hardware resources enabled indefinitely while the PM core marks the device as
RPM_SUSPENDED?

> +	}
> +
>  	enable_irq(gi2c->irq);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=6

[PATCH v3 7/8] i2c: qcom-geni: Use common GENI resource initialization helper

[Praveen Talari]

The driver implements a custom resources_init() callback for clock
frequency validation and bandwidth vote programming. Neither operation is
required for resource initialization itself.

Move clock frequency validation to probe and program the ICC bandwidth vote
from qcom_geni_i2c_conf(), where the bus frequency is configured. This
allows the driver to use geni_se_resources_init() directly and removes the
I2C-specific resource initialization wrapper.

Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index a6fe89d0934f..dbe344719b86 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -275,7 +275,9 @@ static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
 	trace_geni_i2c_bus_setup(gi2c->se.dev, gi2c->clk_freq_out,
 				 itr->clk_div, itr->t_high_cnt,
 				 itr->t_low_cnt, itr->t_cycle_cnt);
-	return 0;
+
+	return geni_icc_set_bw_ab(&gi2c->se, GENI_DEFAULT_BW, GENI_DEFAULT_BW,
+				  Bps_to_icc(gi2c->clk_freq_out));
 }
 
 static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
@@ -1167,22 +1169,6 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
 	return ret;
 }
 
-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);
-	if (ret)
-		return ret;
-
-	ret = geni_i2c_clk_map_idx(gi2c);
-	if (ret)
-		return ret;
-
-	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)
 {
@@ -1251,6 +1237,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (ret < 0)
 		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);
+
 	ret = i2c_add_adapter(&gi2c->adap);
 	if (ret) {
 		release_gpi_dma(gi2c);
@@ -1353,7 +1344,7 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
 };
 
 static const struct geni_i2c_desc geni_i2c = {
-	.resources_init = geni_i2c_resources_init,
+	.resources_init = geni_se_resources_init,
 	.set_rate = qcom_geni_i2c_conf,
 	.power_on = geni_se_resources_activate,
 	.power_off = geni_se_resources_deactivate,
@@ -1362,7 +1353,7 @@ static const struct geni_i2c_desc geni_i2c = {
 static const struct geni_i2c_desc i2c_master_hub = {
 	.no_dma_support = true,
 	.tx_fifo_depth = 16,
-	.resources_init = geni_i2c_resources_init,
+	.resources_init = geni_se_resources_init,
 	.set_rate = qcom_geni_i2c_conf,
 	.power_on = geni_se_resources_activate,
 	.power_off = geni_se_resources_deactivate,

-- 
2.34.1

Re: [PATCH v3 7/8] i2c: qcom-geni: Use common GENI resource initialization helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: qcom-geni: uninitialized src_freq bypasses OPP vote before hw access
- [High] i2c: qcom-geni: severe lock contention by calling icc_set_bw per-transfer
- [Medium] i2c: qcom-geni: leak of GPI DMA channels on clk map error path
--

commit 78a289de1b8e5415a287115753c631aba3c4355a
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

i2c: qcom-geni: Use common GENI resource initialization helper

This commit replaces a custom resource initialization callback with
geni_se_resources_init(). It moves clock frequency validation and ICC
bandwidth vote programming to probe and qcom_geni_i2c_conf() respectively.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a6fe89d0934f6..dbe344719b86c 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -275,7 +275,9 @@ static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
>  	trace_geni_i2c_bus_setup(gi2c->se.dev, gi2c->clk_freq_out,
>  				 itr->clk_div, itr->t_high_cnt,
>  				 itr->t_low_cnt, itr->t_cycle_cnt);
> -	return 0;
> +
> +	return geni_icc_set_bw_ab(&gi2c->se, GENI_DEFAULT_BW, GENI_DEFAULT_BW,
> +				  Bps_to_icc(gi2c->clk_freq_out));
>  }

[Severity: High]
Could this introduce severe lock contention?

The geni_i2c_xfer() handler invokes this configuration function on every batch
of messages. Moving geni_icc_set_bw_ab() into qcom_geni_i2c_conf() means
icc_set_bw() is now called on every I2C transfer.

Because icc_set_bw() unconditionally acquires the global icc_bw_lock mutex
and iterates through interconnect nodes, will this rapidly acquire and
release a global interconnect lock during normal I2C operations, throttling
the rest of the system?

[ ... ]

> @@ -1251,6 +1237,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = geni_i2c_clk_map_idx(gi2c);

[Severity: High]
Does moving geni_i2c_clk_map_idx() after geni_i2c_init() leave gi2c->src_freq
uninitialized when hardware is first accessed?

When geni_i2c_init() calls pm_runtime_resume_and_get(), it triggers
geni_i2c_runtime_resume() which checks gi2c->src_freq:

	if (gi2c->src_freq) {
		ret = dev_pm_opp_set_rate(dev, gi2c->src_freq);
		if (ret)
			return ret;
	}

Because gi2c->src_freq is 0 at this point, the OPP performance state vote is
silently bypassed. geni_i2c_init() then immediately accesses hardware registers.
On Qualcomm platforms, could this un-voted access result in a NoC timeout or
system crash?

> +	if (ret)
> +		return dev_err_probe(gi2c->se.dev, ret, "Invalid clk frequency %d Hz\n",
> +				     gi2c->clk_freq_out);

[Severity: Medium]
Does this path leak the GPI DMA channels?

geni_i2c_init() allocates RX and TX channels via setup_gpi_dma() when the
device is in GPI mode. When geni_i2c_clk_map_idx() fails here, we return
the error directly without calling release_gpi_dma() to clean up the
allocated channels, unlike the i2c_add_adapter() error path directly below it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=7

[PATCH v3 8/8] i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

[Praveen Talari]

GENI resource management may be handled either through the Linux clock
framework or a firmware-managed performance domain. Source clock
programming currently depends on platform-specific callbacks.

Use geni_se_set_rate() for source clock programming and perform the
source clock configuration from qcom_geni_i2c_conf(), allowing the GENI
core to abstract the underlying resource-management mechanism from the
I2C driver. This removes the need for platform-specific rate-setting
callbacks and enables a common code path across all GENI I2C platforms.

Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index dbe344719b86..4879170a1257 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -16,7 +16,6 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/soc/qcom/geni-se.h>
 #include <linux/spinlock.h>
@@ -252,14 +251,14 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
 	return -EINVAL;
 }
 
-static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
+static int qcom_geni_i2c_conf(struct geni_se *se)
 {
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
 	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
 	u32 val;
 	int ret;
 
-	ret = dev_pm_opp_set_rate(se->dev, gi2c->src_freq);
+	ret = geni_se_set_rate(&gi2c->se, gi2c->src_freq);
 	if (ret)
 		return ret;
 
@@ -1042,7 +1041,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 		return ret;
 	}
 
-	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
+	ret = qcom_geni_i2c_conf(&gi2c->se);
 	if (ret)
 		goto out_pm;
 
@@ -1284,7 +1283,7 @@ static int geni_i2c_runtime_suspend(struct device *dev)
 		}
 	}
 
-	return dev_pm_opp_set_rate(dev, 0);
+	return geni_se_set_rate(&gi2c->se, 0);
 }
 
 static int geni_i2c_runtime_resume(struct device *dev)
@@ -1299,7 +1298,7 @@ static int geni_i2c_runtime_resume(struct device *dev)
 	}
 
 	if (gi2c->src_freq) {
-		ret = dev_pm_opp_set_rate(dev, gi2c->src_freq);
+		ret = geni_se_set_rate(&gi2c->se, gi2c->src_freq);
 		if (ret)
 			return ret;
 	}
@@ -1345,7 +1344,6 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
 
 static const struct geni_i2c_desc geni_i2c = {
 	.resources_init = geni_se_resources_init,
-	.set_rate = qcom_geni_i2c_conf,
 	.power_on = geni_se_resources_activate,
 	.power_off = geni_se_resources_deactivate,
 };
@@ -1354,14 +1352,12 @@ static const struct geni_i2c_desc i2c_master_hub = {
 	.no_dma_support = true,
 	.tx_fifo_depth = 16,
 	.resources_init = geni_se_resources_init,
-	.set_rate = qcom_geni_i2c_conf,
 	.power_on = geni_se_resources_activate,
 	.power_off = geni_se_resources_deactivate,
 };
 
 static const struct geni_i2c_desc sa8255p_geni_i2c = {
 	.resources_init = geni_se_domain_attach,
-	.set_rate = geni_se_set_perf_opp,
 };
 
 #ifdef CONFIG_ACPI

-- 
2.34.1

Re: [PATCH v3 8/8] i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: qcom-geni: severe overhead from per-transfer ICC/OPP voting
- [Medium] i2c: qcom-geni: DMA channel leak on clk_map error path
- [Low] i2c: dead code in struct geni_i2c_desc after removing set_rate usage
--

commit 180f114c4a84db9299622afd7b4abba3adcdb2b5
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
Subject: i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

This patch switches the GENI I2C driver to use geni_se_set_rate() instead of
platform-specific callbacks for source clock programming. It performs the
source clock configuration from qcom_geni_i2c_conf(), abstracting the
underlying resource management and unifying the code path across platforms.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index dbe344719b86c..4879170a12576 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -1042,7 +1041,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  		return ret;
>  	}
>  
> -	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
> +	ret = qcom_geni_i2c_conf(&gi2c->se);
>  	if (ret)
>  		goto out_pm;
>  

[Severity: High]
Is there a significant performance regression from moving the interconnect
bandwidth vote and OPP setting into qcom_geni_i2c_conf()?

Because qcom_geni_i2c_conf() is now called unconditionally on every I2C
transaction via geni_i2c_xfer(), the interconnect and OPP frameworks are
invoked per transfer. This acquires global mutexes (like icc_bw_lock) and
recalculates hardware constraints, which could introduce massive overhead on
this hot path.

[ ... ]

> @@ -1345,7 +1344,6 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
>  
>  static const struct geni_i2c_desc geni_i2c = {
>  	.resources_init = geni_se_resources_init,
> -	.set_rate = qcom_geni_i2c_conf,
>  	.power_on = geni_se_resources_activate,
>  	.power_off = geni_se_resources_deactivate,
>  };
> @@ -1354,14 +1352,12 @@ static const struct geni_i2c_desc i2c_master_hub = {
>  	.no_dma_support = true,
>  	.tx_fifo_depth = 16,
>  	.resources_init = geni_se_resources_init,
> -	.set_rate = qcom_geni_i2c_conf,
>  	.power_on = geni_se_resources_activate,
>  	.power_off = geni_se_resources_deactivate,
>  };
>  
>  static const struct geni_i2c_desc sa8255p_geni_i2c = {
>  	.resources_init = geni_se_domain_attach,
> -	.set_rate = geni_se_set_perf_opp,
>  };

[Severity: Low]
Is the set_rate function pointer now dead code?

The patch completely removes the initialization of .set_rate from all
geni_i2c_desc instances, but it leaves the function pointer inside
struct geni_i2c_desc:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_desc {
    ...
    int (*resources_init)(struct geni_se *se);
    int (*set_rate)(struct geni_se *se, unsigned long freq);
    int (*power_on)(struct geni_se *se);
    ...
}


I also noticed a resource leak in geni_i2c_probe() that appears to have been
introduced earlier in this series when geni_i2c_clk_map_idx() was moved:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_probe() {
    ...
    ret = geni_i2c_init(gi2c);
    if (ret < 0)
        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);
    ...
}

[Severity: Medium]
Are the GPI DMA channels leaked on this error path?

This problem wasn't introduced by this patch, but a previous commit in this
series moved geni_i2c_clk_map_idx() into geni_i2c_probe() after
geni_i2c_init(). 

geni_i2c_init() allocates GPI DMA channels via setup_gpi_dma() if FIFO is
disabled. If geni_i2c_clk_map_idx() then fails, the probe function returns
immediately without calling release_gpi_dma(), causing a leak of the
allocated channels.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=8