[PATCH v6 0/8] Enable QUPs and Serial on SA8255p Qualcomm platforms

[PATCH v6 0/8] Enable QUPs and Serial on SA8255p Qualcomm platforms

[Praveen Talari]

From: Praveen Talari <ptalari@qti.qualcomm.com>

The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM. The device
drivers request 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 UART baud rates, with each baud
rate represented by a performance level. Drivers use the
dev_pm_opp_set_level() API to request the desired baud rate by
specifying the performance level.

The QUP drivers are SCMI clients, with clocks, interconnects, pinctrl
and power-domains abstracted by a SCMI server.

The serial driver has a dependency on the dev_pm_opp_set_level() function,
which is applied in the OPP tree's linux-next branch.

Nikunj Kela (2):
  dt-bindings: serial: describe SA8255p
  dt-bindings: qcom: geni-se: describe SA8255p

Praveen Talari (6):
  soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms
  serial: qcom-geni: move resource initialization to separate function
  serial: qcom-geni: move resource control logic to separate functions
  serial: qcom-geni: move clock-rate logic to separate function
  serial: qcom-geni: Enable PM runtime for serial driver
  serial: qcom-geni: Enable Serial on SA8255p Qualcomm platforms
---
v3 -> v4
- removed patch "[PATCH v3 1/9] opp: add new helper API dev_pm_opp_set_level()"
  from series and serial driver has dependency of this API which is
  applied in the OPP tree's linux-next branch.
---

 .../serial/qcom,sa8255p-geni-uart.yaml        |  68 ++++
 .../soc/qcom/qcom,sa8255p-geni-se-qup.yaml    | 107 ++++++
 drivers/soc/qcom/qcom-geni-se.c               |  77 ++--
 drivers/tty/serial/qcom_geni_serial.c         | 345 ++++++++++++++----
 4 files changed, 499 insertions(+), 98 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,sa8255p-geni-se-qup.yaml


base-commit: 475c850a7fdd0915b856173186d5922899d65686
-- 
2.17.1

[PATCH v6 1/8] dt-bindings: serial: describe SA8255p

[Praveen Talari]

From: Nikunj Kela <quic_nkela@quicinc.com>

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

Add DT bindings for the QUP GENI UART controller on sa8255p platform.

The wakeup interrupt (IRQ) is treated as optional, as not all UART
instances have a wakeup-capable interrupt routed via the PDC.

Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
Co-developed-by: Praveen Talari <quic_ptalari@quicinc.com>
Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- added description for interrupt-names
- added wakeup irq as optional information in commit text and
  property description.
- removed wake irq form example node.

v4 -> v5
- added wake irq in example node

v3 -> v4
- added version log after ---

v2 -> v3
- dropped description for interrupt-names
- rebased reg property order in required option

v1 -> v2
- reorder sequence of tags in commit text
- moved reg property after compatible field
- added interrupt-names property
---
 .../serial/qcom,sa8255p-geni-uart.yaml        | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml

diff --git a/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml b/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
new file mode 100644
index 000000000000..c2e11ddcc0f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/qcom,sa8255p-geni-uart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Geni based QUP UART interface
+
+maintainers:
+  - Praveen Talari <quic_ptalari@quicinc.com>
+
+allOf:
+  - $ref: /schemas/serial/serial.yaml#
+
+properties:
+  compatible:
+    enum:
+      - qcom,sa8255p-geni-uart
+      - qcom,sa8255p-geni-debug-uart
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    items:
+      - description: UART core irq
+      - description: Wakeup irq (RX GPIO)
+
+  interrupt-names:
+    description:
+      The UART interrupt and optionally the RX in-band wakeup interrupt
+      as not all UART instances have a wakeup-capable interrupt routed
+      via the PDC.
+    items:
+      - const: uart
+      - const: wakeup
+
+  power-domains:
+    minItems: 2
+    maxItems: 2
+
+  power-domain-names:
+    items:
+      - const: power
+      - const: perf
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - power-domains
+  - power-domain-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    serial@990000 {
+        compatible = "qcom,sa8255p-geni-uart";
+        reg = <0x990000 0x4000>;
+        interrupts = <GIC_SPI 531 IRQ_TYPE_LEVEL_HIGH>;
+        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
+        power-domain-names = "power", "perf";
+    };
+...
-- 
2.17.1

[PATCH v6 2/8] dt-bindings: qcom: geni-se: describe SA8255p

[Praveen Talari]

From: Nikunj Kela <quic_nkela@quicinc.com>

SA8255p platform abstracts resources such as clocks, interconnect
configuration in Firmware.

Add DT bindings for the QUP Wrapper on sa8255p platform.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
Co-developed-by: Praveen Talari <quic_ptalari@quicinc.com>
Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- added Reviewed-by tag in commit

v3 -> v4
- reordered required:  after properties and patternproperties
- added version log after ---

v2 -> v3
- reordered required option

v1 -> v2
- reorder sequence of tags in commit text
- resolved waring errors while encountered in dt binding and dtb check.
---
 .../soc/qcom/qcom,sa8255p-geni-se-qup.yaml    | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,sa8255p-geni-se-qup.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,sa8255p-geni-se-qup.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,sa8255p-geni-se-qup.yaml
new file mode 100644
index 000000000000..352af3426d34
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,sa8255p-geni-se-qup.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,sa8255p-geni-se-qup.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GENI Serial Engine QUP Wrapper Controller
+
+maintainers:
+  - Praveen Talari <quic_ptalari@quicinc.com>
+
+description:
+  Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP) wrapper
+  is a programmable module for supporting a wide range of serial interfaces
+  like UART, SPI, I2C, I3C, etc. A single QUP module can provide up to 8 Serial
+  Interfaces, using its internal Serial Engines. The GENI Serial Engine QUP
+  Wrapper controller is modeled as a node with zero or more child nodes each
+  representing a serial engine.
+
+properties:
+  compatible:
+    const: qcom,sa8255p-geni-se-qup
+
+  reg:
+    description: QUP wrapper common register address and length.
+    maxItems: 1
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 2
+
+  ranges: true
+
+  iommus:
+    maxItems: 1
+
+  dma-coherent: true
+
+patternProperties:
+  "spi@[0-9a-f]+$":
+    type: object
+    description: GENI serial engine based SPI controller. SPI in master mode
+                 supports up to 50MHz, up to four chip selects, programmable
+                 data path from 4 bits to 32 bits and numerous protocol
+                 variants.
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: qcom,sa8255p-geni-spi
+
+  "i2c@[0-9a-f]+$":
+    type: object
+    description: GENI serial engine based I2C controller.
+    additionalProperties: true
+
+    properties:
+      compatible:
+        const: qcom,sa8255p-geni-i2c
+
+  "serial@[0-9a-f]+$":
+    type: object
+    description: GENI Serial Engine based UART Controller.
+    additionalProperties: true
+
+    properties:
+      compatible:
+        enum:
+          - qcom,sa8255p-geni-uart
+          - qcom,sa8255p-geni-debug-uart
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        geniqup@9c0000 {
+            compatible = "qcom,sa8255p-geni-se-qup";
+            reg = <0 0x9c0000 0 0x6000>;
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges;
+
+            serial@990000 {
+                compatible = "qcom,sa8255p-geni-uart";
+                reg = <0 0x990000 0 0x4000>;
+                interrupts = <GIC_SPI 531 IRQ_TYPE_LEVEL_HIGH>;
+                power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
+                power-domain-names = "power", "perf";
+            };
+        };
+    };
+...
-- 
2.17.1

[PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms

[Praveen Talari]

On the sa8255p platform, resources such as clocks,interconnects
and TLMM (GPIO) configurations are managed by firmware.

Introduce a platform data function callback to distinguish whether
resource control is performed by firmware or directly by the driver
in linux.

The refactor ensures clear differentiation of resource
management mechanisms, improving maintainability and flexibility
in handling platform-specific configurations.

Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- replaced dev_err with dev_err_probe
- added a check for desc->num_clks with MAX_CLKS, an error if
  the specified num_clks in descriptor exceeds defined MAX_CLKS.
- removed min_t which is not necessary.
- renamed callback function names to resources_init.
- resolved kernel bot warning error by documenting function
  pointer in geni_se_desc structure.

v3 -> v4
- declared an empty struct for sa8255p and added check as num clks.
- Added version log after ---

v1 -> v2
- changed datatype of i from int to unsigned int as per comment.
---
 drivers/soc/qcom/qcom-geni-se.c | 77 +++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 28 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 4cb959106efa..5c727b9a17e9 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -101,10 +101,13 @@ struct geni_wrapper {
  * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
  * @clks:		Name of the primary & optional secondary AHB clocks
  * @num_clks:		Count of clock names
+ * @resources_init:	Function pointer for initializing QUP Wrapper resources
  */
 struct geni_se_desc {
 	unsigned int num_clks;
 	const char * const *clks;
+	int (*resources_init)(struct geni_wrapper *wrapper,
+			      const struct geni_se_desc *desc);
 };
 
 static const char * const icc_path_names[] = {"qup-core", "qup-config",
@@ -891,10 +894,47 @@ int geni_icc_disable(struct geni_se *se)
 }
 EXPORT_SYMBOL_GPL(geni_icc_disable);
 
+static int geni_se_resource_init(struct geni_wrapper *wrapper,
+				 const struct geni_se_desc *desc)
+{
+	struct device *dev = wrapper->dev;
+	int ret;
+	unsigned int i;
+
+	if (desc->num_clks > MAX_CLKS)
+		return dev_err_probe(dev, -EINVAL,
+				     "Too many clocks specified in descriptor:%u (max allowed: %u)\n",
+				     desc->num_clks, MAX_CLKS);
+
+	wrapper->num_clks = desc->num_clks;
+
+	for (i = 0; i < wrapper->num_clks; ++i)
+		wrapper->clks[i].id = desc->clks[i];
+
+	ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "invalid clocks property at %pOF\n", dev->of_node);
+
+	if (ret < wrapper->num_clks) {
+		dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
+			dev->of_node, wrapper->num_clks);
+		return -EINVAL;
+	}
+
+	ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
+	if (ret) {
+		dev_err(dev, "Err getting clks %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
 static int geni_se_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct geni_wrapper *wrapper;
+	const struct geni_se_desc *desc;
 	int ret;
 
 	wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
@@ -906,36 +946,12 @@ static int geni_se_probe(struct platform_device *pdev)
 	if (IS_ERR(wrapper->base))
 		return PTR_ERR(wrapper->base);
 
-	if (!has_acpi_companion(&pdev->dev)) {
-		const struct geni_se_desc *desc;
-		int i;
-
-		desc = device_get_match_data(&pdev->dev);
-		if (!desc)
-			return -EINVAL;
-
-		wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS);
-
-		for (i = 0; i < wrapper->num_clks; ++i)
-			wrapper->clks[i].id = desc->clks[i];
-
-		ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
-		if (ret < 0) {
-			dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node);
-			return ret;
-		}
+	desc = device_get_match_data(&pdev->dev);
 
-		if (ret < wrapper->num_clks) {
-			dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
-				dev->of_node, wrapper->num_clks);
+	if (!has_acpi_companion(&pdev->dev) && desc->num_clks) {
+		ret = desc->resources_init(wrapper, desc);
+		if (ret)
 			return -EINVAL;
-		}
-
-		ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
-		if (ret) {
-			dev_err(dev, "Err getting clks %d\n", ret);
-			return ret;
-		}
 	}
 
 	dev_set_drvdata(dev, wrapper);
@@ -951,8 +967,11 @@ static const char * const qup_clks[] = {
 static const struct geni_se_desc qup_desc = {
 	.clks = qup_clks,
 	.num_clks = ARRAY_SIZE(qup_clks),
+	.resources_init = geni_se_resource_init,
 };
 
+static const struct geni_se_desc sa8255p_qup_desc;
+
 static const char * const i2c_master_hub_clks[] = {
 	"s-ahb",
 };
@@ -960,11 +979,13 @@ static const char * const i2c_master_hub_clks[] = {
 static const struct geni_se_desc i2c_master_hub_desc = {
 	.clks = i2c_master_hub_clks,
 	.num_clks = ARRAY_SIZE(i2c_master_hub_clks),
+	.resources_init = geni_se_resource_init,
 };
 
 static const struct of_device_id geni_se_dt_match[] = {
 	{ .compatible = "qcom,geni-se-qup", .data = &qup_desc },
 	{ .compatible = "qcom,geni-se-i2c-master-hub", .data = &i2c_master_hub_desc },
+	{ .compatible = "qcom,sa8255p-geni-se-qup", .data = &sa8255p_qup_desc },
 	{}
 };
 MODULE_DEVICE_TABLE(of, geni_se_dt_match);
-- 
2.17.1

[PATCH v6 4/8] serial: qcom-geni: move resource initialization to separate function

[Praveen Talari]

Enhances code readability and future modifications within the new API.

Move the code that handles the actual initialization of resources
like clock and ICC paths to a separate function, making the
probe function cleaner.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- added reviewed-by tag

v3 -> v4
- added version log after ---

v1 -> v2
- updated subject description.
- added a new line after function end
---
 drivers/tty/serial/qcom_geni_serial.c | 66 ++++++++++++++++-----------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 0293b6210aa6..6ad759146f71 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1588,6 +1588,43 @@ static struct uart_driver qcom_geni_uart_driver = {
 	.nr =  GENI_UART_PORTS,
 };
 
+static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
+{
+	int ret;
+
+	port->se.clk = devm_clk_get(port->se.dev, "se");
+	if (IS_ERR(port->se.clk)) {
+		ret = PTR_ERR(port->se.clk);
+		dev_err(port->se.dev, "Err getting SE Core clk %d\n", ret);
+		return ret;
+	}
+
+	ret = geni_icc_get(&port->se, NULL);
+	if (ret)
+		return ret;
+
+	port->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
+	port->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
+
+	/* Set BW for register access */
+	ret = geni_icc_set_bw(&port->se);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_opp_set_clkname(port->se.dev, "se");
+	if (ret)
+		return ret;
+
+	/* OPP table is optional */
+	ret = devm_pm_opp_of_add_table(port->se.dev);
+	if (ret && ret != -ENODEV) {
+		dev_err(port->se.dev, "invalid OPP table in device tree\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static void qcom_geni_serial_pm(struct uart_port *uport,
 		unsigned int new_state, unsigned int old_state)
 {
@@ -1690,12 +1727,10 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	port->dev_data = data;
 	port->se.dev = &pdev->dev;
 	port->se.wrapper = dev_get_drvdata(pdev->dev.parent);
-	port->se.clk = devm_clk_get(&pdev->dev, "se");
-	if (IS_ERR(port->se.clk)) {
-		ret = PTR_ERR(port->se.clk);
-		dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
+
+	ret = geni_serial_resource_init(port);
+	if (ret)
 		return ret;
-	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
@@ -1713,17 +1748,6 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 			return -ENOMEM;
 	}
 
-	ret = geni_icc_get(&port->se, NULL);
-	if (ret)
-		return ret;
-	port->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
-	port->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
-
-	/* Set BW for register access */
-	ret = geni_icc_set_bw(&port->se);
-	if (ret)
-		return ret;
-
 	port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
 			"qcom_geni_serial_%s%d",
 			uart_console(uport) ? "console" : "uart", uport->line);
@@ -1745,16 +1769,6 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
 		port->cts_rts_swap = true;
 
-	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
-	if (ret)
-		return ret;
-	/* OPP table is optional */
-	ret = devm_pm_opp_of_add_table(&pdev->dev);
-	if (ret && ret != -ENODEV) {
-		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
-		return ret;
-	}
-
 	port->private_data.drv = drv;
 	uport->private_data = &port->private_data;
 	platform_set_drvdata(pdev, port);
-- 
2.17.1

[PATCH v6 5/8] serial: qcom-geni: move resource control logic to separate functions

[Praveen Talari]

Supports use in PM system/runtime frameworks, helping to distinguish new
resource control mechanisms and facilitate future modifications within
the new API.

The code that handles the actual enable or disable of resources like clock
and ICC paths to a separate function (geni_serial_resources_on() and
geni_serial_resources_off()) which enhances code readability.

Introduced minor return checks in newly added function APIs to enhance
error detection and prevent silent failures.

Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- updated commit text for checks in newly added function APIs
- fiexd alignment
- reordered newly added function API definations.

v3 -> v4
- added version log after ---

v1 -> v2
- returned 0 instead of ret variable
---
 drivers/tty/serial/qcom_geni_serial.c | 54 +++++++++++++++++++++------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 6ad759146f71..715db35bab2f 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1588,6 +1588,42 @@ static struct uart_driver qcom_geni_uart_driver = {
 	.nr =  GENI_UART_PORTS,
 };
 
+static int geni_serial_resources_on(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	int ret;
+
+	ret = geni_icc_enable(&port->se);
+	if (ret)
+		return ret;
+
+	ret = geni_se_resources_on(&port->se);
+	if (ret) {
+		geni_icc_disable(&port->se);
+		return ret;
+	}
+
+	if (port->clk_rate)
+		dev_pm_opp_set_rate(uport->dev, port->clk_rate);
+
+	return 0;
+}
+
+static int geni_serial_resources_off(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	int ret;
+
+	dev_pm_opp_set_rate(uport->dev, 0);
+	ret = geni_se_resources_off(&port->se);
+	if (ret)
+		return ret;
+
+	geni_icc_disable(&port->se);
+
+	return 0;
+}
+
 static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
 {
 	int ret;
@@ -1628,23 +1664,17 @@ static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
 static void qcom_geni_serial_pm(struct uart_port *uport,
 		unsigned int new_state, unsigned int old_state)
 {
-	struct qcom_geni_serial_port *port = to_dev_port(uport);
 
 	/* If we've never been called, treat it as off */
 	if (old_state == UART_PM_STATE_UNDEFINED)
 		old_state = UART_PM_STATE_OFF;
 
-	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
-		geni_icc_enable(&port->se);
-		if (port->clk_rate)
-			dev_pm_opp_set_rate(uport->dev, port->clk_rate);
-		geni_se_resources_on(&port->se);
-	} else if (new_state == UART_PM_STATE_OFF &&
-			old_state == UART_PM_STATE_ON) {
-		geni_se_resources_off(&port->se);
-		dev_pm_opp_set_rate(uport->dev, 0);
-		geni_icc_disable(&port->se);
-	}
+	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
+		geni_serial_resources_on(uport);
+	else if (new_state == UART_PM_STATE_OFF &&
+		 old_state == UART_PM_STATE_ON)
+		geni_serial_resources_off(uport);
+
 }
 
 static const struct uart_ops qcom_geni_console_pops = {
-- 
2.17.1

[PATCH v6 6/8] serial: qcom-geni: move clock-rate logic to separate function

[Praveen Talari]

Facilitates future modifications within the new function,
leading to better readability and maintainability of the code.

Move the code that handles the actual logic of clock-rate
calculations to a separate function geni_serial_set_rate()
which enhances code readability.

Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- used "unsigned int" instead of "unsigned long" in newly
  added API function params to avoid the format specifier
  warnings.

v3 -> v4
- added version log after ---

v1 -> v2
- resolved build warnings for datatype format specifiers
- removed double spaces in log
---
 drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++----------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 715db35bab2f..b6fa7dc9b1fb 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1283,27 +1283,14 @@ static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
 	return ser_clk;
 }
 
-static void qcom_geni_serial_set_termios(struct uart_port *uport,
-					 struct ktermios *termios,
-					 const struct ktermios *old)
+static int geni_serial_set_rate(struct uart_port *uport, unsigned int baud)
 {
-	unsigned int baud;
-	u32 bits_per_char;
-	u32 tx_trans_cfg;
-	u32 tx_parity_cfg;
-	u32 rx_trans_cfg;
-	u32 rx_parity_cfg;
-	u32 stop_bit_len;
-	unsigned int clk_div;
-	u32 ser_clk_cfg;
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
 	unsigned long clk_rate;
-	u32 ver, sampling_rate;
 	unsigned int avg_bw_core;
-	unsigned long timeout;
-
-	/* baud rate */
-	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
+	unsigned int clk_div;
+	u32 ver, sampling_rate;
+	u32 ser_clk_cfg;
 
 	sampling_rate = UART_OVERSAMPLING;
 	/* Sampling rate is halved for IP versions >= 2.5 */
@@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 		dev_err(port->se.dev,
 			"Couldn't find suitable clock rate for %u\n",
 			baud * sampling_rate);
-		return;
+		return -EINVAL;
 	}
 
 	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
@@ -1339,6 +1326,37 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	port->se.icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(baud);
 	geni_icc_set_bw(&port->se);
 
+	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
+	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
+	return 0;
+}
+
+static void qcom_geni_serial_set_termios(struct uart_port *uport,
+					 struct ktermios *termios,
+					 const struct ktermios *old)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	unsigned int baud;
+	unsigned long timeout;
+	u32 bits_per_char;
+	u32 tx_trans_cfg;
+	u32 tx_parity_cfg;
+	u32 rx_trans_cfg;
+	u32 rx_parity_cfg;
+	u32 stop_bit_len;
+	int ret = 0;
+
+	/* baud rate */
+	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
+
+	ret = geni_serial_set_rate(uport, baud);
+	if (ret) {
+		dev_err(port->se.dev,
+			"%s: Failed to set baud:%u ret:%d\n",
+			__func__, baud, ret);
+		return;
+	}
+
 	/* parity */
 	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
 	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
@@ -1406,8 +1424,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
 	writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
 	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
-	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
-	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
 }
 
 #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
-- 
2.17.1

[PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Praveen Talari]

Add Power Management (PM) runtime support to Qualcomm GENI
serial driver.

Introduce necessary callbacks and updates to ensure seamless
transitions between power states, enhancing overall power
efficiency.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- added reviewed-by tag in commit
- added __maybe_unused to PM callback functions to avoid
  warnings of defined but not used
---
 drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index b6fa7dc9b1fb..3691340ce7e8 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
 		old_state = UART_PM_STATE_OFF;
 
 	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
-		geni_serial_resources_on(uport);
+		pm_runtime_resume_and_get(uport->dev);
 	else if (new_state == UART_PM_STATE_OFF &&
 		 old_state == UART_PM_STATE_ON)
-		geni_serial_resources_off(uport);
+		pm_runtime_put_sync(uport->dev);
 
 }
 
@@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	pm_runtime_enable(port->se.dev);
+
 	ret = uart_add_one_port(drv, uport);
 	if (ret)
-		return ret;
+		goto error;
 
 	if (port->wakeup_irq > 0) {
 		device_init_wakeup(&pdev->dev, true);
@@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 			device_init_wakeup(&pdev->dev, false);
 			ida_free(&port_ida, uport->line);
 			uart_remove_one_port(drv, uport);
-			return ret;
+			goto error;
 		}
 	}
 
 	return 0;
+
+error:
+	pm_runtime_disable(port->se.dev);
+	return ret;
 }
 
 static void qcom_geni_serial_remove(struct platform_device *pdev)
@@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
 	dev_pm_clear_wake_irq(&pdev->dev);
 	device_init_wakeup(&pdev->dev, false);
 	ida_free(&port_ida, uport->line);
+	pm_runtime_disable(port->se.dev);
 	uart_remove_one_port(drv, &port->uport);
 }
 
+static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
+{
+	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
+	struct uart_port *uport = &port->uport;
+
+	return geni_serial_resources_off(uport);
+}
+
+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;
+
+	return geni_serial_resources_on(uport);
+}
+
 static int qcom_geni_serial_suspend(struct device *dev)
 {
 	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
@@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
 };
 
 static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
+	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
+			   qcom_geni_serial_runtime_resume, NULL)
 	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
 };
 
-- 
2.17.1

[PATCH v6 8/8] serial: qcom-geni: Enable Serial 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 UART baud rates, with each baud
rate represented by a performance level. The driver uses the
dev_pm_opp_set_level() API to request the desired baud rate by
specifying the performance level.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v5 -> v6
- added reviewed-by tag in commit
- used "unsigned int" instead of "unsigned long" in set_rate callback
  and geni_serial_set_level to avoid build warnings.

v3 -> v4
- renamed callback function names to resources_init, set_rate and
  power_state
---
 drivers/tty/serial/qcom_geni_serial.c | 150 +++++++++++++++++++++++---
 1 file changed, 135 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3691340ce7e8..3dd3b1e1ed78 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -11,6 +11,7 @@
 #include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_opp.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -99,10 +100,16 @@
 #define DMA_RX_BUF_SIZE		2048
 
 static DEFINE_IDA(port_ida);
+#define DOMAIN_IDX_POWER	0
+#define DOMAIN_IDX_PERF		1
 
 struct qcom_geni_device_data {
 	bool console;
 	enum geni_se_xfer_mode mode;
+	struct dev_pm_domain_attach_data pd_data;
+	int (*resources_init)(struct uart_port *uport);
+	int (*set_rate)(struct uart_port *uport, unsigned int baud);
+	int (*power_state)(struct uart_port *uport, bool state);
 };
 
 struct qcom_geni_private_data {
@@ -140,6 +147,7 @@ struct qcom_geni_serial_port {
 
 	struct qcom_geni_private_data private_data;
 	const struct qcom_geni_device_data *dev_data;
+	struct dev_pm_domain_list *pd_list;
 };
 
 static const struct uart_ops qcom_geni_console_pops;
@@ -1331,6 +1339,42 @@ static int geni_serial_set_rate(struct uart_port *uport, unsigned int baud)
 	return 0;
 }
 
+static int geni_serial_set_level(struct uart_port *uport, unsigned int baud)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	struct device *perf_dev = port->pd_list->pd_devs[DOMAIN_IDX_PERF];
+
+	/*
+	 * The performance protocol sets UART communication
+	 * speeds by selecting different performance levels
+	 * through the OPP framework.
+	 *
+	 * Supported perf levels for baudrates in firmware are below
+	 * +---------------------+--------------------+
+	 * |  Perf level value   |  Baudrate values   |
+	 * +---------------------+--------------------+
+	 * |      300            |      300           |
+	 * |      1200           |      1200          |
+	 * |      2400           |      2400          |
+	 * |      4800           |      4800          |
+	 * |      9600           |      9600          |
+	 * |      19200          |      19200         |
+	 * |      38400          |      38400         |
+	 * |      57600          |      57600         |
+	 * |      115200         |      115200        |
+	 * |      230400         |      230400        |
+	 * |      460800         |      460800        |
+	 * |      921600         |      921600        |
+	 * |      2000000        |      2000000       |
+	 * |      3000000        |      3000000       |
+	 * |      3200000        |      3200000       |
+	 * |      4000000        |      4000000       |
+	 * +---------------------+--------------------+
+	 */
+
+	return dev_pm_opp_set_level(perf_dev, baud);
+}
+
 static void qcom_geni_serial_set_termios(struct uart_port *uport,
 					 struct ktermios *termios,
 					 const struct ktermios *old)
@@ -1349,7 +1393,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	/* baud rate */
 	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
 
-	ret = geni_serial_set_rate(uport, baud);
+	ret = port->dev_data->set_rate(uport, baud);
 	if (ret) {
 		dev_err(port->se.dev,
 			"%s: Failed to set baud:%u ret:%d\n",
@@ -1640,8 +1684,27 @@ static int geni_serial_resources_off(struct uart_port *uport)
 	return 0;
 }
 
-static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
+static int geni_serial_resource_state(struct uart_port *uport, bool power_on)
 {
+	return power_on ? geni_serial_resources_on(uport) : geni_serial_resources_off(uport);
+}
+
+static int geni_serial_pwr_init(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	int ret;
+
+	ret = dev_pm_domain_attach_list(port->se.dev,
+					&port->dev_data->pd_data, &port->pd_list);
+	if (ret <= 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int geni_serial_resource_init(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
 	int ret;
 
 	port->se.clk = devm_clk_get(port->se.dev, "se");
@@ -1680,7 +1743,6 @@ static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
 static void qcom_geni_serial_pm(struct uart_port *uport,
 		unsigned int new_state, unsigned int old_state)
 {
-
 	/* If we've never been called, treat it as off */
 	if (old_state == UART_PM_STATE_UNDEFINED)
 		old_state = UART_PM_STATE_OFF;
@@ -1774,13 +1836,16 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	port->se.dev = &pdev->dev;
 	port->se.wrapper = dev_get_drvdata(pdev->dev.parent);
 
-	ret = geni_serial_resource_init(port);
+	ret = port->dev_data->resources_init(uport);
 	if (ret)
 		return ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -EINVAL;
+	if (!res) {
+		ret = -EINVAL;
+		goto error;
+	}
+
 	uport->mapbase = res->start;
 
 	port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
@@ -1790,19 +1855,26 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	if (!data->console) {
 		port->rx_buf = devm_kzalloc(uport->dev,
 					    DMA_RX_BUF_SIZE, GFP_KERNEL);
-		if (!port->rx_buf)
-			return -ENOMEM;
+		if (!port->rx_buf) {
+			ret = -ENOMEM;
+			goto error;
+		}
 	}
 
 	port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
 			"qcom_geni_serial_%s%d",
 			uart_console(uport) ? "console" : "uart", uport->line);
-	if (!port->name)
-		return -ENOMEM;
+	if (!port->name) {
+		ret = -ENOMEM;
+		goto error;
+	}
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	if (irq < 0) {
+		ret = irq;
+		goto error;
+	}
+
 	uport->irq = irq;
 	uport->has_sysrq = IS_ENABLED(CONFIG_SERIAL_QCOM_GENI_CONSOLE);
 
@@ -1824,7 +1896,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 			IRQF_TRIGGER_HIGH, port->name, uport);
 	if (ret) {
 		dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
-		return ret;
+		goto error;
 	}
 
 	pm_runtime_enable(port->se.dev);
@@ -1849,6 +1921,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 
 error:
 	pm_runtime_disable(port->se.dev);
+	dev_pm_domain_detach_list(port->pd_list);
 	return ret;
 }
 
@@ -1863,22 +1936,31 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
 	ida_free(&port_ida, uport->line);
 	pm_runtime_disable(port->se.dev);
 	uart_remove_one_port(drv, &port->uport);
+	dev_pm_domain_detach_list(port->pd_list);
 }
 
 static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
 {
 	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
 	struct uart_port *uport = &port->uport;
+	int ret = 0;
 
-	return geni_serial_resources_off(uport);
+	if (port->dev_data->power_state)
+		ret = port->dev_data->power_state(uport, false);
+
+	return ret;
 }
 
 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 = 0;
+
+	if (port->dev_data->power_state)
+		ret = port->dev_data->power_state(uport, true);
 
-	return geni_serial_resources_on(uport);
+	return ret;
 }
 
 static int qcom_geni_serial_suspend(struct device *dev)
@@ -1916,11 +1998,41 @@ static int qcom_geni_serial_resume(struct device *dev)
 static const struct qcom_geni_device_data qcom_geni_console_data = {
 	.console = true,
 	.mode = GENI_SE_FIFO,
+	.resources_init = geni_serial_resource_init,
+	.set_rate = geni_serial_set_rate,
+	.power_state = geni_serial_resource_state,
 };
 
 static const struct qcom_geni_device_data qcom_geni_uart_data = {
 	.console = false,
 	.mode = GENI_SE_DMA,
+	.resources_init = geni_serial_resource_init,
+	.set_rate = geni_serial_set_rate,
+	.power_state = geni_serial_resource_state,
+};
+
+static const struct qcom_geni_device_data sa8255p_qcom_geni_console_data = {
+	.console = true,
+	.mode = GENI_SE_FIFO,
+	.pd_data = {
+		.pd_flags = PD_FLAG_DEV_LINK_ON,
+		.pd_names = (const char*[]) { "power", "perf" },
+		.num_pd_names = 2,
+	},
+	.resources_init = geni_serial_pwr_init,
+	.set_rate = geni_serial_set_level,
+};
+
+static const struct qcom_geni_device_data sa8255p_qcom_geni_uart_data = {
+	.console = false,
+	.mode = GENI_SE_DMA,
+	.pd_data = {
+		.pd_flags = PD_FLAG_DEV_LINK_ON,
+		.pd_names = (const char*[]) { "power", "perf" },
+		.num_pd_names = 2,
+	},
+	.resources_init = geni_serial_pwr_init,
+	.set_rate = geni_serial_set_level,
 };
 
 static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
@@ -1934,10 +2046,18 @@ static const struct of_device_id qcom_geni_serial_match_table[] = {
 		.compatible = "qcom,geni-debug-uart",
 		.data = &qcom_geni_console_data,
 	},
+	{
+		.compatible = "qcom,sa8255p-geni-debug-uart",
+		.data = &sa8255p_qcom_geni_console_data,
+	},
 	{
 		.compatible = "qcom,geni-uart",
 		.data = &qcom_geni_uart_data,
 	},
+	{
+		.compatible = "qcom,sa8255p-geni-uart",
+		.data = &sa8255p_qcom_geni_uart_data,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_geni_serial_match_table);
-- 
2.17.1

Re: [PATCH v6 1/8] dt-bindings: serial: describe SA8255p

[Krzysztof Kozlowski]

On Fri, Jun 06, 2025 at 10:51:07PM GMT, Praveen Talari wrote:
> From: Nikunj Kela <quic_nkela@quicinc.com>
> 
> SA8255p platform abstracts resources such as clocks, interconnect and
> GPIO pins configuration in Firmware. SCMI power and perf protocols are
> used to send request for resource configurations.
> 
> Add DT bindings for the QUP GENI UART controller on sa8255p platform.
> 
> The wakeup interrupt (IRQ) is treated as optional, as not all UART
> instances have a wakeup-capable interrupt routed via the PDC.
> 
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> Co-developed-by: Praveen Talari <quic_ptalari@quicinc.com>
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> v5 -> v6
> - added description for interrupt-names
> - added wakeup irq as optional information in commit text and
>   property description.
> - removed wake irq form example node.
> 
> v4 -> v5
> - added wake irq in example node
> 
> v3 -> v4
> - added version log after ---
> 
> v2 -> v3
> - dropped description for interrupt-names
> - rebased reg property order in required option
> 
> v1 -> v2
> - reorder sequence of tags in commit text
> - moved reg property after compatible field
> - added interrupt-names property
> ---
>  .../serial/qcom,sa8255p-geni-uart.yaml        | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
> 
> diff --git a/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml b/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
> new file mode 100644
> index 000000000000..c2e11ddcc0f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/qcom,sa8255p-geni-uart.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Geni based QUP UART interface
> +
> +maintainers:
> +  - Praveen Talari <quic_ptalari@quicinc.com>
> +
> +allOf:
> +  - $ref: /schemas/serial/serial.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sa8255p-geni-uart
> +      - qcom,sa8255p-geni-debug-uart
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1

Drop, this is not in sync with interrupt-names. You already received
comments on this. We talk about this since v4!

I am not reviewing the rest. Implement complete feedback given to you in
v4 and v5.

Best regards,
Krzysztof

Re: [PATCH v6 1/8] dt-bindings: serial: describe SA8255p

[Praveen Talari]

Hi Krzysztof

Thank you for review.

On 6/10/2025 12:34 PM, Krzysztof Kozlowski wrote:
> On Fri, Jun 06, 2025 at 10:51:07PM GMT, Praveen Talari wrote:
>> From: Nikunj Kela <quic_nkela@quicinc.com>
>>
>> SA8255p platform abstracts resources such as clocks, interconnect and
>> GPIO pins configuration in Firmware. SCMI power and perf protocols are
>> used to send request for resource configurations.
>>
>> Add DT bindings for the QUP GENI UART controller on sa8255p platform.
>>
>> The wakeup interrupt (IRQ) is treated as optional, as not all UART
>> instances have a wakeup-capable interrupt routed via the PDC.
>>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> Co-developed-by: Praveen Talari <quic_ptalari@quicinc.com>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>> v5 -> v6
>> - added description for interrupt-names
>> - added wakeup irq as optional information in commit text and
>>    property description.
>> - removed wake irq form example node.
>>
>> v4 -> v5
>> - added wake irq in example node
>>
>> v3 -> v4
>> - added version log after ---
>>
>> v2 -> v3
>> - dropped description for interrupt-names
>> - rebased reg property order in required option
>>
>> v1 -> v2
>> - reorder sequence of tags in commit text
>> - moved reg property after compatible field
>> - added interrupt-names property
>> ---
>>   .../serial/qcom,sa8255p-geni-uart.yaml        | 68 +++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml b/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
>> new file mode 100644
>> index 000000000000..c2e11ddcc0f6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/qcom,sa8255p-geni-uart.yaml
>> @@ -0,0 +1,68 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/serial/qcom,sa8255p-geni-uart.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Geni based QUP UART interface
>> +
>> +maintainers:
>> +  - Praveen Talari <quic_ptalari@quicinc.com>
>> +
>> +allOf:
>> +  - $ref: /schemas/serial/serial.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,sa8255p-geni-uart
>> +      - qcom,sa8255p-geni-debug-uart
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    minItems: 1
> 
> Drop, this is not in sync with interrupt-names. You already received
> comments on this. We talk about this since v4!
I hope you have reviewed the commit message and the description under 
interrupt-name regarding the optional wakeup IRQ. I believe that address 
your query.

I can include minItems:1 in the interrupt-name property in the next 
patch set to align/sync with interrupts property.

Thanks,
Praveen Talari
> 
> I am not reviewing the rest. Implement complete feedback given to you in
> v4 and v5.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v6 1/8] dt-bindings: serial: describe SA8255p

[Krzysztof Kozlowski]

On 10/06/2025 10:10, Praveen Talari wrote:
>>> +
>>> +  interrupts:
>>> +    minItems: 1
>>
>> Drop, this is not in sync with interrupt-names. You already received
>> comments on this. We talk about this since v4!
> I hope you have reviewed the commit message and the description under 
> interrupt-name regarding the optional wakeup IRQ. I believe that address 
> your query.
> 
> I can include minItems:1 in the interrupt-name property in the next 
> patch set to align/sync with interrupts property.
Yes, then the interrupt-names needs minItems.

Best regards,
Krzysztof

Re: [PATCH v6 1/8] dt-bindings: serial: describe SA8255p

[Praveen Talari]

Hi Krzysztof,

Thank you for review.

On 6/11/2025 12:47 PM, Krzysztof Kozlowski wrote:
> On 10/06/2025 10:10, Praveen Talari wrote:
>>>> +
>>>> +  interrupts:
>>>> +    minItems: 1
>>>
>>> Drop, this is not in sync with interrupt-names. You already received
>>> comments on this. We talk about this since v4!
>> I hope you have reviewed the commit message and the description under
>> interrupt-name regarding the optional wakeup IRQ. I believe that address
>> your query.
>>
>> I can include minItems:1 in the interrupt-name property in the next
>> patch set to align/sync with interrupts property.
> Yes, then the interrupt-names needs minItems.
Sure, will update in next patch-set.

Thanks,
Praveen Talari
> 
> Best regards,
> Krzysztof

Re: [PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms

[Praveen Talari]

Hi Bryan,

Gentle reminder!!

Thanks,
Praveen talari

On 6/6/2025 10:51 PM, Praveen Talari wrote:
> On the sa8255p platform, resources such as clocks,interconnects
> and TLMM (GPIO) configurations are managed by firmware.
> 
> Introduce a platform data function callback to distinguish whether
> resource control is performed by firmware or directly by the driver
> in linux.
> 
> The refactor ensures clear differentiation of resource
> management mechanisms, improving maintainability and flexibility
> in handling platform-specific configurations.
> 
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> v5 -> v6
> - replaced dev_err with dev_err_probe
> - added a check for desc->num_clks with MAX_CLKS, an error if
>    the specified num_clks in descriptor exceeds defined MAX_CLKS.
> - removed min_t which is not necessary.
> - renamed callback function names to resources_init.
> - resolved kernel bot warning error by documenting function
>    pointer in geni_se_desc structure.
> 
> v3 -> v4
> - declared an empty struct for sa8255p and added check as num clks.
> - Added version log after ---
> 
> v1 -> v2
> - changed datatype of i from int to unsigned int as per comment.
> ---
>   drivers/soc/qcom/qcom-geni-se.c | 77 +++++++++++++++++++++------------
>   1 file changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 4cb959106efa..5c727b9a17e9 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -101,10 +101,13 @@ struct geni_wrapper {
>    * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
>    * @clks:		Name of the primary & optional secondary AHB clocks
>    * @num_clks:		Count of clock names
> + * @resources_init:	Function pointer for initializing QUP Wrapper resources
>    */
>   struct geni_se_desc {
>   	unsigned int num_clks;
>   	const char * const *clks;
> +	int (*resources_init)(struct geni_wrapper *wrapper,
> +			      const struct geni_se_desc *desc);
>   };
>   
>   static const char * const icc_path_names[] = {"qup-core", "qup-config",
> @@ -891,10 +894,47 @@ int geni_icc_disable(struct geni_se *se)
>   }
>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>   
> +static int geni_se_resource_init(struct geni_wrapper *wrapper,
> +				 const struct geni_se_desc *desc)
> +{
> +	struct device *dev = wrapper->dev;
> +	int ret;
> +	unsigned int i;
> +
> +	if (desc->num_clks > MAX_CLKS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Too many clocks specified in descriptor:%u (max allowed: %u)\n",
> +				     desc->num_clks, MAX_CLKS);
> +
> +	wrapper->num_clks = desc->num_clks;
> +
> +	for (i = 0; i < wrapper->num_clks; ++i)
> +		wrapper->clks[i].id = desc->clks[i];
> +
> +	ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "invalid clocks property at %pOF\n", dev->of_node);
> +
> +	if (ret < wrapper->num_clks) {
> +		dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> +			dev->of_node, wrapper->num_clks);
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> +	if (ret) {
> +		dev_err(dev, "Err getting clks %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>   static int geni_se_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct geni_wrapper *wrapper;
> +	const struct geni_se_desc *desc;
>   	int ret;
>   
>   	wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
> @@ -906,36 +946,12 @@ static int geni_se_probe(struct platform_device *pdev)
>   	if (IS_ERR(wrapper->base))
>   		return PTR_ERR(wrapper->base);
>   
> -	if (!has_acpi_companion(&pdev->dev)) {
> -		const struct geni_se_desc *desc;
> -		int i;
> -
> -		desc = device_get_match_data(&pdev->dev);
> -		if (!desc)
> -			return -EINVAL;
> -
> -		wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS);
> -
> -		for (i = 0; i < wrapper->num_clks; ++i)
> -			wrapper->clks[i].id = desc->clks[i];
> -
> -		ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> -		if (ret < 0) {
> -			dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node);
> -			return ret;
> -		}
> +	desc = device_get_match_data(&pdev->dev);
>   
> -		if (ret < wrapper->num_clks) {
> -			dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> -				dev->of_node, wrapper->num_clks);
> +	if (!has_acpi_companion(&pdev->dev) && desc->num_clks) {
> +		ret = desc->resources_init(wrapper, desc);
> +		if (ret)
>   			return -EINVAL;
> -		}
> -
> -		ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> -		if (ret) {
> -			dev_err(dev, "Err getting clks %d\n", ret);
> -			return ret;
> -		}
>   	}
>   
>   	dev_set_drvdata(dev, wrapper);
> @@ -951,8 +967,11 @@ static const char * const qup_clks[] = {
>   static const struct geni_se_desc qup_desc = {
>   	.clks = qup_clks,
>   	.num_clks = ARRAY_SIZE(qup_clks),
> +	.resources_init = geni_se_resource_init,
>   };
>   
> +static const struct geni_se_desc sa8255p_qup_desc;
> +
>   static const char * const i2c_master_hub_clks[] = {
>   	"s-ahb",
>   };
> @@ -960,11 +979,13 @@ static const char * const i2c_master_hub_clks[] = {
>   static const struct geni_se_desc i2c_master_hub_desc = {
>   	.clks = i2c_master_hub_clks,
>   	.num_clks = ARRAY_SIZE(i2c_master_hub_clks),
> +	.resources_init = geni_se_resource_init,
>   };
>   
>   static const struct of_device_id geni_se_dt_match[] = {
>   	{ .compatible = "qcom,geni-se-qup", .data = &qup_desc },
>   	{ .compatible = "qcom,geni-se-i2c-master-hub", .data = &i2c_master_hub_desc },
> +	{ .compatible = "qcom,sa8255p-geni-se-qup", .data = &sa8255p_qup_desc },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, geni_se_dt_match);

Re: [PATCH v6 6/8] serial: qcom-geni: move clock-rate logic to separate function

[Praveen Talari]

Hi Bryan,

Gentle reminder!!

Thanks,
Praveen Talari

On 6/6/2025 10:51 PM, Praveen Talari wrote:
> Facilitates future modifications within the new function,
> leading to better readability and maintainability of the code.
> 
> Move the code that handles the actual logic of clock-rate
> calculations to a separate function geni_serial_set_rate()
> which enhances code readability.
> 
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> v5 -> v6
> - used "unsigned int" instead of "unsigned long" in newly
>    added API function params to avoid the format specifier
>    warnings.
> 
> v3 -> v4
> - added version log after ---
> 
> v1 -> v2
> - resolved build warnings for datatype format specifiers
> - removed double spaces in log
> ---
>   drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++----------
>   1 file changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 715db35bab2f..b6fa7dc9b1fb 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1283,27 +1283,14 @@ static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
>   	return ser_clk;
>   }
>   
> -static void qcom_geni_serial_set_termios(struct uart_port *uport,
> -					 struct ktermios *termios,
> -					 const struct ktermios *old)
> +static int geni_serial_set_rate(struct uart_port *uport, unsigned int baud)
>   {
> -	unsigned int baud;
> -	u32 bits_per_char;
> -	u32 tx_trans_cfg;
> -	u32 tx_parity_cfg;
> -	u32 rx_trans_cfg;
> -	u32 rx_parity_cfg;
> -	u32 stop_bit_len;
> -	unsigned int clk_div;
> -	u32 ser_clk_cfg;
>   	struct qcom_geni_serial_port *port = to_dev_port(uport);
>   	unsigned long clk_rate;
> -	u32 ver, sampling_rate;
>   	unsigned int avg_bw_core;
> -	unsigned long timeout;
> -
> -	/* baud rate */
> -	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> +	unsigned int clk_div;
> +	u32 ver, sampling_rate;
> +	u32 ser_clk_cfg;
>   
>   	sampling_rate = UART_OVERSAMPLING;
>   	/* Sampling rate is halved for IP versions >= 2.5 */
> @@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>   		dev_err(port->se.dev,
>   			"Couldn't find suitable clock rate for %u\n",
>   			baud * sampling_rate);
> -		return;
> +		return -EINVAL;
>   	}
>   
>   	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
> @@ -1339,6 +1326,37 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>   	port->se.icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(baud);
>   	geni_icc_set_bw(&port->se);
>   
> +	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
> +	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
> +	return 0;
> +}
> +
> +static void qcom_geni_serial_set_termios(struct uart_port *uport,
> +					 struct ktermios *termios,
> +					 const struct ktermios *old)
> +{
> +	struct qcom_geni_serial_port *port = to_dev_port(uport);
> +	unsigned int baud;
> +	unsigned long timeout;
> +	u32 bits_per_char;
> +	u32 tx_trans_cfg;
> +	u32 tx_parity_cfg;
> +	u32 rx_trans_cfg;
> +	u32 rx_parity_cfg;
> +	u32 stop_bit_len;
> +	int ret = 0;
> +
> +	/* baud rate */
> +	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> +
> +	ret = geni_serial_set_rate(uport, baud);
> +	if (ret) {
> +		dev_err(port->se.dev,
> +			"%s: Failed to set baud:%u ret:%d\n",
> +			__func__, baud, ret);
> +		return;
> +	}
> +
>   	/* parity */
>   	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
>   	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
> @@ -1406,8 +1424,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>   	writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
>   	writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
>   	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
> -	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
> -	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
>   }
>   
>   #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE

Re: [PATCH v6 6/8] serial: qcom-geni: move clock-rate logic to separate function

[Krzysztof Kozlowski]

On 16/06/2025 18:04, Praveen Talari wrote:
> Hi Bryan,
> 
> Gentle reminder!!

That's neither gentle nor needed to ping multiple times, every 8 hours.

Best regards,
Krzysztof

Re: [PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms

[Bryan O'Donoghue]

On 16/06/2025 09:40, Praveen Talari wrote:
> Hi Bryan,
> 
> Gentle reminder!!
> 
> Thanks,
> Praveen talari
> 
Small nitpick here.

1. You didn't include me in your v6 CC list so the ping
    is the first direct notification of this series I've received.
    This is no problem BTW just for your reference.

2. Again as a general recommendation to qcom folks the commit
    overview logs are fine but please include the name of the person
    whose feedback you are addressing in that log.

eg

v5 -> v6

- replaced dev_err with dev_err_probe - Bryan

etc, that way a reviewer can re-up their context quicker.

---
bod

Re: [PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms

[Bryan O'Donoghue]

On 06/06/2025 18:21, Praveen Talari wrote:
> On the sa8255p platform, resources such as clocks,interconnects
> and TLMM (GPIO) configurations are managed by firmware.
> 
> Introduce a platform data function callback to distinguish whether
> resource control is performed by firmware or directly by the driver
> in linux.
> 
> The refactor ensures clear differentiation of resource
> management mechanisms, improving maintainability and flexibility
> in handling platform-specific configurations.
> 
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> v5 -> v6
> - replaced dev_err with dev_err_probe

You've missed two opportunities for dev_err_probe() in this submission.

> - added a check for desc->num_clks with MAX_CLKS, an error if
>    the specified num_clks in descriptor exceeds defined MAX_CLKS.
> - removed min_t which is not necessary.
> - renamed callback function names to resources_init.
> - resolved kernel bot warning error by documenting function
>    pointer in geni_se_desc structure.
> 
> v3 -> v4
> - declared an empty struct for sa8255p and added check as num clks.
> - Added version log after ---
> 
> v1 -> v2
> - changed datatype of i from int to unsigned int as per comment.
> ---
>   drivers/soc/qcom/qcom-geni-se.c | 77 +++++++++++++++++++++------------
>   1 file changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 4cb959106efa..5c727b9a17e9 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -101,10 +101,13 @@ struct geni_wrapper {
>    * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
>    * @clks:		Name of the primary & optional secondary AHB clocks
>    * @num_clks:		Count of clock names
> + * @resources_init:	Function pointer for initializing QUP Wrapper resources
>    */
>   struct geni_se_desc {
>   	unsigned int num_clks;
>   	const char * const *clks;
> +	int (*resources_init)(struct geni_wrapper *wrapper,
> +			      const struct geni_se_desc *desc);
>   };
> 
>   static const char * const icc_path_names[] = {"qup-core", "qup-config",
> @@ -891,10 +894,47 @@ int geni_icc_disable(struct geni_se *se)
>   }
>   EXPORT_SYMBOL_GPL(geni_icc_disable);
> 
> +static int geni_se_resource_init(struct geni_wrapper *wrapper,
> +				 const struct geni_se_desc *desc)
> +{
> +	struct device *dev = wrapper->dev;
> +	int ret;
> +	unsigned int i;
> +
> +	if (desc->num_clks > MAX_CLKS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Too many clocks specified in descriptor:%u (max allowed: %u)\n",
> +				     desc->num_clks, MAX_CLKS);

I think this is an extraneous add, we should trust the array indexes 
inside our own driver that we control.

Actually why do we have a MAX_CLKS ? We specify a list of clk names with 
aggregate-initialisation and ARRAY_SIZE() of the aggregate.

Like so:

static const char * const qup_clks[] = {
         "m-ahb",
         "s-ahb",
};

static const struct geni_se_desc qup_desc = {
         .clks = qup_clks,
         .num_clks = ARRAY_SIZE(qup_clks),

> +
> +	wrapper->num_clks = desc->num_clks;
> +
> +	for (i = 0; i < wrapper->num_clks; ++i)
> +		wrapper->clks[i].id = desc->clks[i];
> +
> +	ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "invalid clocks property at %pOF\n", dev->of_node);
> +
> +	if (ret < wrapper->num_clks) {
> +		dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> +			dev->of_node, wrapper->num_clks);
> +		return -EINVAL;
> +	}

This code OTOH makes way more sense as we are validating our internal 
num_clks variable which we have enumerated ourselves against a DT input 
which we are consuming.

> +
> +	ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> +	if (ret) {
> +		dev_err(dev, "Err getting clks %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>   static int geni_se_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct geni_wrapper *wrapper;
> +	const struct geni_se_desc *desc;
>   	int ret;
> 
>   	wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
> @@ -906,36 +946,12 @@ static int geni_se_probe(struct platform_device *pdev)
>   	if (IS_ERR(wrapper->base))
>   		return PTR_ERR(wrapper->base);
> 
> -	if (!has_acpi_companion(&pdev->dev)) {
> -		const struct geni_se_desc *desc;
> -		int i;
> -
> -		desc = device_get_match_data(&pdev->dev);
> -		if (!desc)
> -			return -EINVAL;
> -
> -		wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS);
> -
> -		for (i = 0; i < wrapper->num_clks; ++i)
> -			wrapper->clks[i].id = desc->clks[i];
> -
> -		ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> -		if (ret < 0) {
> -			dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node);
> -			return ret;
> -		}
> +	desc = device_get_match_data(&pdev->dev);
> 
> -		if (ret < wrapper->num_clks) {
> -			dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> -				dev->of_node, wrapper->num_clks);
> +	if (!has_acpi_companion(&pdev->dev) && desc->num_clks) {
> +		ret = desc->resources_init(wrapper, desc);
> +		if (ret)
>   			return -EINVAL;
> -		}
> -
> -		ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> -		if (ret) {
> -			dev_err(dev, "Err getting clks %d\n", ret);
> -			return ret;
> -		}
>   	}
> 
>   	dev_set_drvdata(dev, wrapper);
> @@ -951,8 +967,11 @@ static const char * const qup_clks[] = {
>   static const struct geni_se_desc qup_desc = {
>   	.clks = qup_clks,
>   	.num_clks = ARRAY_SIZE(qup_clks),
> +	.resources_init = geni_se_resource_init,
>   };
> 
> +static const struct geni_se_desc sa8255p_qup_desc;
> +
>   static const char * const i2c_master_hub_clks[] = {
>   	"s-ahb",
>   };
> @@ -960,11 +979,13 @@ static const char * const i2c_master_hub_clks[] = {
>   static const struct geni_se_desc i2c_master_hub_desc = {
>   	.clks = i2c_master_hub_clks,
>   	.num_clks = ARRAY_SIZE(i2c_master_hub_clks),
> +	.resources_init = geni_se_resource_init,
>   };
> 
>   static const struct of_device_id geni_se_dt_match[] = {
>   	{ .compatible = "qcom,geni-se-qup", .data = &qup_desc },
>   	{ .compatible = "qcom,geni-se-i2c-master-hub", .data = &i2c_master_hub_desc },
> +	{ .compatible = "qcom,sa8255p-geni-se-qup", .data = &sa8255p_qup_desc },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, geni_se_dt_match);
> --
> 2.17.1
> 
> 

---
bod

Re: [PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms

[Praveen Talari]

On 6/17/2025 12:06 AM, Bryan O'Donoghue wrote:
> On 16/06/2025 09:40, Praveen Talari wrote:
>> Hi Bryan,
>>
>> Gentle reminder!!
>>
>> Thanks,
>> Praveen talari
>>
> Small nitpick here.
> 
> 1. You didn't include me in your v6 CC list so the ping
>     is the first direct notification of this series I've received.
>     This is no problem BTW just for your reference.
> 
> 2. Again as a general recommendation to qcom folks the commit
>     overview logs are fine but please include the name of the person
>     whose feedback you are addressing in that log.
> 
> eg
> 
> v5 -> v6
> 
> - replaced dev_err with dev_err_probe - Bryan
> 
> etc, that way a reviewer can re-up their context quicker.

Thanks you for your valuable inputs. I will ensure it is reflected in 
the next patch set onwards.

Thanks,
Praveen Talari

> 
> ---
> bod

Re: [PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms

[Bjorn Andersson]

On Fri, Jun 06, 2025 at 10:51:09PM +0530, Praveen Talari wrote:
> On the sa8255p platform, resources such as clocks,interconnects
> and TLMM (GPIO) configurations are managed by firmware.
> 
> Introduce a platform data function callback to distinguish whether
> resource control is performed by firmware or directly by the driver
> in linux.
> 
> The refactor ensures clear differentiation of resource
> management mechanisms, improving maintainability and flexibility
> in handling platform-specific configurations.
> 
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> v5 -> v6
> - replaced dev_err with dev_err_probe
> - added a check for desc->num_clks with MAX_CLKS, an error if
>   the specified num_clks in descriptor exceeds defined MAX_CLKS.
> - removed min_t which is not necessary.
> - renamed callback function names to resources_init.
> - resolved kernel bot warning error by documenting function
>   pointer in geni_se_desc structure.
> 
> v3 -> v4
> - declared an empty struct for sa8255p and added check as num clks.
> - Added version log after ---
> 
> v1 -> v2
> - changed datatype of i from int to unsigned int as per comment.
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 77 +++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 4cb959106efa..5c727b9a17e9 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -101,10 +101,13 @@ struct geni_wrapper {
>   * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
>   * @clks:		Name of the primary & optional secondary AHB clocks
>   * @num_clks:		Count of clock names
> + * @resources_init:	Function pointer for initializing QUP Wrapper resources
>   */
>  struct geni_se_desc {
>  	unsigned int num_clks;
>  	const char * const *clks;
> +	int (*resources_init)(struct geni_wrapper *wrapper,
> +			      const struct geni_se_desc *desc);
>  };
>  
>  static const char * const icc_path_names[] = {"qup-core", "qup-config",
> @@ -891,10 +894,47 @@ int geni_icc_disable(struct geni_se *se)
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>  
> +static int geni_se_resource_init(struct geni_wrapper *wrapper,
> +				 const struct geni_se_desc *desc)
> +{
> +	struct device *dev = wrapper->dev;
> +	int ret;
> +	unsigned int i;
> +
> +	if (desc->num_clks > MAX_CLKS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Too many clocks specified in descriptor:%u (max allowed: %u)\n",
> +				     desc->num_clks, MAX_CLKS);
> +
> +	wrapper->num_clks = desc->num_clks;
> +
> +	for (i = 0; i < wrapper->num_clks; ++i)
> +		wrapper->clks[i].id = desc->clks[i];
> +
> +	ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "invalid clocks property at %pOF\n", dev->of_node);
> +
> +	if (ret < wrapper->num_clks) {
> +		dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> +			dev->of_node, wrapper->num_clks);
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> +	if (ret) {
> +		dev_err(dev, "Err getting clks %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  static int geni_se_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct geni_wrapper *wrapper;
> +	const struct geni_se_desc *desc;
>  	int ret;
>  
>  	wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
> @@ -906,36 +946,12 @@ static int geni_se_probe(struct platform_device *pdev)
>  	if (IS_ERR(wrapper->base))
>  		return PTR_ERR(wrapper->base);
>  
> -	if (!has_acpi_companion(&pdev->dev)) {
> -		const struct geni_se_desc *desc;
> -		int i;
> -
> -		desc = device_get_match_data(&pdev->dev);
> -		if (!desc)
> -			return -EINVAL;
> -
> -		wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS);
> -
> -		for (i = 0; i < wrapper->num_clks; ++i)
> -			wrapper->clks[i].id = desc->clks[i];
> -
> -		ret = of_count_phandle_with_args(dev->of_node, "clocks", "#clock-cells");
> -		if (ret < 0) {
> -			dev_err(dev, "invalid clocks property at %pOF\n", dev->of_node);
> -			return ret;
> -		}
> +	desc = device_get_match_data(&pdev->dev);
>  
> -		if (ret < wrapper->num_clks) {
> -			dev_err(dev, "invalid clocks count at %pOF, expected %d entries\n",
> -				dev->of_node, wrapper->num_clks);
> +	if (!has_acpi_companion(&pdev->dev) && desc->num_clks) {

Reading this again, the only functional change I can spot is the
addition of this desc->num_clks check. (And the addition of the new
compatible)

The rest of the patch is just moving things out of this if statement
body, introducing the flexibility of a function pointer with a single
possible value etc.


As I've said before, function pointers are useful to create
abstractions, but they make it harder to follow the code (both for me
and the CPU) so you need to provide some value in return - and I'm
failing to see what that value is.

> +		ret = desc->resources_init(wrapper, desc);

In other words, you can replace this line with:

	ret = geni_se_resource_init();

Or, if this is all we get, then you can do nothing and just add the
additional expression to the condition and be done with it.

> +		if (ret)
>  			return -EINVAL;
> -		}
> -
> -		ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
> -		if (ret) {
> -			dev_err(dev, "Err getting clks %d\n", ret);
> -			return ret;
> -		}
>  	}
>  
>  	dev_set_drvdata(dev, wrapper);
> @@ -951,8 +967,11 @@ static const char * const qup_clks[] = {
>  static const struct geni_se_desc qup_desc = {
>  	.clks = qup_clks,
>  	.num_clks = ARRAY_SIZE(qup_clks),
> +	.resources_init = geni_se_resource_init,
>  };
>  
> +static const struct geni_se_desc sa8255p_qup_desc;

This looks like a forward declaration, it took me a while to realize
that this is giving you the actual all-zero geni_se_desc.

Add a = {}; to make it clear that this is where you declare the
variable.

Thanks,
Bjorn

> +
>  static const char * const i2c_master_hub_clks[] = {
>  	"s-ahb",
>  };
> @@ -960,11 +979,13 @@ static const char * const i2c_master_hub_clks[] = {
>  static const struct geni_se_desc i2c_master_hub_desc = {
>  	.clks = i2c_master_hub_clks,
>  	.num_clks = ARRAY_SIZE(i2c_master_hub_clks),
> +	.resources_init = geni_se_resource_init,
>  };
>  
>  static const struct of_device_id geni_se_dt_match[] = {
>  	{ .compatible = "qcom,geni-se-qup", .data = &qup_desc },
>  	{ .compatible = "qcom,geni-se-i2c-master-hub", .data = &i2c_master_hub_desc },
> +	{ .compatible = "qcom,sa8255p-geni-se-qup", .data = &sa8255p_qup_desc },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, geni_se_dt_match);
> -- 
> 2.17.1
> 

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Bjorn Andersson]

On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
> Add Power Management (PM) runtime support to Qualcomm GENI
> serial driver.
> 

Doesn't this have impact on the behavior outside of your
project? Or is the transition from qcom_geni_serial_pm() to explicit
RPM merely moving code around?

Seems like this deserves to not be hidden in a middle of a patch series.

> Introduce necessary callbacks and updates to ensure seamless
> transitions between power states, enhancing overall power
> efficiency.
> 

This commit message fails to state why we need runtime PM support in the
driver.

Also, start your commit message with a problem description, per
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> v5 -> v6
> - added reviewed-by tag in commit
> - added __maybe_unused to PM callback functions to avoid
>   warnings of defined but not used
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index b6fa7dc9b1fb..3691340ce7e8 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>  		old_state = UART_PM_STATE_OFF;
>  
>  	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
> -		geni_serial_resources_on(uport);
> +		pm_runtime_resume_and_get(uport->dev);
>  	else if (new_state == UART_PM_STATE_OFF &&
>  		 old_state == UART_PM_STATE_ON)
> -		geni_serial_resources_off(uport);
> +		pm_runtime_put_sync(uport->dev);
>  
>  }
>  
> @@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	pm_runtime_enable(port->se.dev);

Any reason not to use devm_pm_runtime_enable() and avoid the
two pm_runtime_disable() below?

Regards,
Bjorn

> +
>  	ret = uart_add_one_port(drv, uport);
>  	if (ret)
> -		return ret;
> +		goto error;
>  
>  	if (port->wakeup_irq > 0) {
>  		device_init_wakeup(&pdev->dev, true);
> @@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  			device_init_wakeup(&pdev->dev, false);
>  			ida_free(&port_ida, uport->line);
>  			uart_remove_one_port(drv, uport);
> -			return ret;
> +			goto error;
>  		}
>  	}
>  
>  	return 0;
> +
> +error:
> +	pm_runtime_disable(port->se.dev);
> +	return ret;
>  }
>  
>  static void qcom_geni_serial_remove(struct platform_device *pdev)
> @@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>  	dev_pm_clear_wake_irq(&pdev->dev);
>  	device_init_wakeup(&pdev->dev, false);
>  	ida_free(&port_ida, uport->line);
> +	pm_runtime_disable(port->se.dev);
>  	uart_remove_one_port(drv, &port->uport);
>  }
>  
> +static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
> +{
> +	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> +	struct uart_port *uport = &port->uport;
> +
> +	return geni_serial_resources_off(uport);
> +}
> +
> +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;
> +
> +	return geni_serial_resources_on(uport);
> +}
> +
>  static int qcom_geni_serial_suspend(struct device *dev)
>  {
>  	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> @@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
>  };
>  
>  static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
> +	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
> +			   qcom_geni_serial_runtime_resume, NULL)
>  	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
>  };
>  
> -- 
> 2.17.1
> 

Re: [PATCH v6 6/8] serial: qcom-geni: move clock-rate logic to separate function

[Bjorn Andersson]

On Mon, Jun 16, 2025 at 09:34:27PM +0530, Praveen Talari wrote:
> Hi Bryan,
> 
> Gentle reminder!!
> 

As I've told you all countless times, if you want attention to your
patchset review each others patches! For some reason you're the only one
showing interest in getting this series merged.

> Thanks,
> Praveen Talari
> 
> On 6/6/2025 10:51 PM, Praveen Talari wrote:
> > Facilitates future modifications within the new function,
> > leading to better readability and maintainability of the code.
> > 
> > Move the code that handles the actual logic of clock-rate
> > calculations to a separate function geni_serial_set_rate()
> > which enhances code readability.
> > 
> > Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> > ---
> > v5 -> v6
> > - used "unsigned int" instead of "unsigned long" in newly
> >    added API function params to avoid the format specifier
> >    warnings.
> > 
> > v3 -> v4
> > - added version log after ---
> > 
> > v1 -> v2
> > - resolved build warnings for datatype format specifiers
> > - removed double spaces in log
> > ---
> >   drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++----------
> >   1 file changed, 36 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 715db35bab2f..b6fa7dc9b1fb 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1283,27 +1283,14 @@ static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
> >   	return ser_clk;
> >   }
> > -static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > -					 struct ktermios *termios,
> > -					 const struct ktermios *old)
> > +static int geni_serial_set_rate(struct uart_port *uport, unsigned int baud)
> >   {
> > -	unsigned int baud;
> > -	u32 bits_per_char;
> > -	u32 tx_trans_cfg;
> > -	u32 tx_parity_cfg;
> > -	u32 rx_trans_cfg;
> > -	u32 rx_parity_cfg;
> > -	u32 stop_bit_len;
> > -	unsigned int clk_div;
> > -	u32 ser_clk_cfg;
> >   	struct qcom_geni_serial_port *port = to_dev_port(uport);
> >   	unsigned long clk_rate;
> > -	u32 ver, sampling_rate;
> >   	unsigned int avg_bw_core;
> > -	unsigned long timeout;
> > -
> > -	/* baud rate */
> > -	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> > +	unsigned int clk_div;
> > +	u32 ver, sampling_rate;
> > +	u32 ser_clk_cfg;
> >   	sampling_rate = UART_OVERSAMPLING;
> >   	/* Sampling rate is halved for IP versions >= 2.5 */
> > @@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> >   		dev_err(port->se.dev,
> >   			"Couldn't find suitable clock rate for %u\n",
> >   			baud * sampling_rate);
> > -		return;
> > +		return -EINVAL;
> >   	}
> >   	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
> > @@ -1339,6 +1326,37 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> >   	port->se.icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(baud);
> >   	geni_icc_set_bw(&port->se);
> > +	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
> > +	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
> > +	return 0;
> > +}
> > +
> > +static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > +					 struct ktermios *termios,
> > +					 const struct ktermios *old)
> > +{
> > +	struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +	unsigned int baud;
> > +	unsigned long timeout;
> > +	u32 bits_per_char;
> > +	u32 tx_trans_cfg;
> > +	u32 tx_parity_cfg;
> > +	u32 rx_trans_cfg;
> > +	u32 rx_parity_cfg;
> > +	u32 stop_bit_len;
> > +	int ret = 0;
> > +
> > +	/* baud rate */
> > +	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> > +
> > +	ret = geni_serial_set_rate(uport, baud);
> > +	if (ret) {
> > +		dev_err(port->se.dev,
> > +			"%s: Failed to set baud:%u ret:%d\n",
> > +			__func__, baud, ret);

As far as I can tell there's one error path in geni_serial_set_rate()
and there you already printed a more specific error message, as such
this doesn't add any value.

PS. In general, please don't use __func__, write helpful error messages
instead.

Regards,
Bjorn

> > +		return;
> > +	}
> > +
> >   	/* parity */
> >   	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
> >   	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
> > @@ -1406,8 +1424,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> >   	writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
> >   	writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
> >   	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
> > -	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
> > -	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
> >   }
> >   #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Praveen Talari]

Hi Bjorn,

Thank you for review.

On 6/17/2025 9:23 PM, Bjorn Andersson wrote:
> On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
>> Add Power Management (PM) runtime support to Qualcomm GENI
>> serial driver.
>>
> 
> Doesn't this have impact on the behavior outside of your
> project? Or is the transition from qcom_geni_serial_pm() to explicit
> RPM merely moving code around?
There is no impact on functionality and behavior with this change.

The transition is purely refactor.

Using PM runtime APIs such as
pm_runtime_resume_and_get() and pm_runtime_put_sync() to manage resource 
both locally and firmware.

> 
> Seems like this deserves to not be hidden in a middle of a patch series.
This depends on refactored patches.
> 
>> Introduce necessary callbacks and updates to ensure seamless
>> transitions between power states, enhancing overall power
>> efficiency.
>>
> 
> This commit message fails to state why we need runtime PM support in the
> driver.
Sure, will update commit text.
> 
> Also, start your commit message with a problem description, per
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>> v5 -> v6
>> - added reviewed-by tag in commit
>> - added __maybe_unused to PM callback functions to avoid
>>    warnings of defined but not used
>> ---
>>   drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index b6fa7dc9b1fb..3691340ce7e8 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>>   		old_state = UART_PM_STATE_OFF;
>>   
>>   	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>> -		geni_serial_resources_on(uport);
>> +		pm_runtime_resume_and_get(uport->dev);
>>   	else if (new_state == UART_PM_STATE_OFF &&
>>   		 old_state == UART_PM_STATE_ON)
>> -		geni_serial_resources_off(uport);
>> +		pm_runtime_put_sync(uport->dev);
>>   
>>   }
>>   
>> @@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>   		return ret;
>>   	}
>>   
>> +	pm_runtime_enable(port->se.dev);
> 
> Any reason not to use devm_pm_runtime_enable() and avoid the
> two pm_runtime_disable() below?

I agree, will update in next patch-set.
> 
> Regards,
> Bjorn
> 
>> +
>>   	ret = uart_add_one_port(drv, uport);
>>   	if (ret)
>> -		return ret;
>> +		goto error;
>>   
>>   	if (port->wakeup_irq > 0) {
>>   		device_init_wakeup(&pdev->dev, true);
>> @@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>   			device_init_wakeup(&pdev->dev, false);
>>   			ida_free(&port_ida, uport->line);
>>   			uart_remove_one_port(drv, uport);
>> -			return ret;
>> +			goto error;
>>   		}
>>   	}
>>   
>>   	return 0;
>> +
>> +error:
>> +	pm_runtime_disable(port->se.dev);
>> +	return ret;
>>   }
>>   
>>   static void qcom_geni_serial_remove(struct platform_device *pdev)
>> @@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>>   	dev_pm_clear_wake_irq(&pdev->dev);
>>   	device_init_wakeup(&pdev->dev, false);
>>   	ida_free(&port_ida, uport->line);
>> +	pm_runtime_disable(port->se.dev);
>>   	uart_remove_one_port(drv, &port->uport);
>>   }
>>   
>> +static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
>> +{
>> +	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>> +	struct uart_port *uport = &port->uport;
>> +
>> +	return geni_serial_resources_off(uport);
>> +}
>> +
>> +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;
>> +
>> +	return geni_serial_resources_on(uport);
>> +}
>> +
>>   static int qcom_geni_serial_suspend(struct device *dev)
>>   {
>>   	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>> @@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
>>   };
>>   
>>   static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
>> +			   qcom_geni_serial_runtime_resume, NULL)
>>   	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
>>   };
>>   
>> -- 
>> 2.17.1
>>

Re: [PATCH v6 3/8] soc: qcom: geni-se: Enable QUPs on SA8255p Qualcomm platforms

[Praveen Talari]

HI Bryan,

Thank you for review.

On 6/17/2025 12:23 AM, Bryan O'Donoghue wrote:
> On 06/06/2025 18:21, Praveen Talari wrote:
>> On the sa8255p platform, resources such as clocks,interconnects
>> and TLMM (GPIO) configurations are managed by firmware.
>>
>> Introduce a platform data function callback to distinguish whether
>> resource control is performed by firmware or directly by the driver
>> in linux.
>>
>> The refactor ensures clear differentiation of resource
>> management mechanisms, improving maintainability and flexibility
>> in handling platform-specific configurations.
>>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>> v5 -> v6
>> - replaced dev_err with dev_err_probe
> 
> You've missed two opportunities for dev_err_probe() in this submission.

Thank you for pointing out. Yes i can see two more dev_err in new API.
Will address in next patch.

> 
>> - added a check for desc->num_clks with MAX_CLKS, an error if
>>    the specified num_clks in descriptor exceeds defined MAX_CLKS.
>> - removed min_t which is not necessary.
>> - renamed callback function names to resources_init.
>> - resolved kernel bot warning error by documenting function
>>    pointer in geni_se_desc structure.
>>
>> v3 -> v4
>> - declared an empty struct for sa8255p and added check as num clks.
>> - Added version log after ---
>>
>> v1 -> v2
>> - changed datatype of i from int to unsigned int as per comment.
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c | 77 +++++++++++++++++++++------------
>>   1 file changed, 49 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c 
>> b/drivers/soc/qcom/qcom-geni-se.c
>> index 4cb959106efa..5c727b9a17e9 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -101,10 +101,13 @@ struct geni_wrapper {
>>    * struct geni_se_desc - Data structure to represent the QUP Wrapper 
>> resources
>>    * @clks:        Name of the primary & optional secondary AHB clocks
>>    * @num_clks:        Count of clock names
>> + * @resources_init:    Function pointer for initializing QUP Wrapper 
>> resources
>>    */
>>   struct geni_se_desc {
>>       unsigned int num_clks;
>>       const char * const *clks;
>> +    int (*resources_init)(struct geni_wrapper *wrapper,
>> +                  const struct geni_se_desc *desc);
>>   };
>>
>>   static const char * const icc_path_names[] = {"qup-core", "qup-config",
>> @@ -891,10 +894,47 @@ int geni_icc_disable(struct geni_se *se)
>>   }
>>   EXPORT_SYMBOL_GPL(geni_icc_disable);
>>
>> +static int geni_se_resource_init(struct geni_wrapper *wrapper,
>> +                 const struct geni_se_desc *desc)
>> +{
>> +    struct device *dev = wrapper->dev;
>> +    int ret;
>> +    unsigned int i;
>> +
>> +    if (desc->num_clks > MAX_CLKS)
>> +        return dev_err_probe(dev, -EINVAL,
>> +                     "Too many clocks specified in descriptor:%u (max 
>> allowed: %u)\n",
>> +                     desc->num_clks, MAX_CLKS);
> 
> I think this is an extraneous add, we should trust the array indexes 
> inside our own driver that we control.
> 
> Actually why do we have a MAX_CLKS ? We specify a list of clk names with 

MAX_CLKS is needed for static declaration of "struct clk_bulk_data 
clK[MAX_CLKS]" which is need to save clk related info.

otherwise we allocate dynamic memory instead of static.

> aggregate-initialisation and ARRAY_SIZE() of the aggregate.
> 
> Like so:
> 
> static const char * const qup_clks[] = {
>          "m-ahb",
>          "s-ahb",
> };
> 
> static const struct geni_se_desc qup_desc = {
>          .clks = qup_clks,
>          .num_clks = ARRAY_SIZE(qup_clks),
> 
>> +
>> +    wrapper->num_clks = desc->num_clks;
>> +
>> +    for (i = 0; i < wrapper->num_clks; ++i)
>> +        wrapper->clks[i].id = desc->clks[i];
>> +
>> +    ret = of_count_phandle_with_args(dev->of_node, "clocks", 
>> "#clock-cells");
>> +    if (ret < 0)
>> +        return dev_err_probe(dev, ret, "invalid clocks property at 
>> %pOF\n", dev->of_node);
>> +
>> +    if (ret < wrapper->num_clks) {
>> +        dev_err(dev, "invalid clocks count at %pOF, expected %d 
>> entries\n",
>> +            dev->of_node, wrapper->num_clks);
>> +        return -EINVAL;
>> +    }
> 
> This code OTOH makes way more sense as we are validating our internal 
> num_clks variable which we have enumerated ourselves against a DT input 
> which we are consuming.

Yes, we have fixed clks which are enumerated in desc wrt DT.

> 
>> +
>> +    ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
>> +    if (ret) {
>> +        dev_err(dev, "Err getting clks %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static int geni_se_probe(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>>       struct geni_wrapper *wrapper;
>> +    const struct geni_se_desc *desc;
>>       int ret;
>>
>>       wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
>> @@ -906,36 +946,12 @@ static int geni_se_probe(struct platform_device 
>> *pdev)
>>       if (IS_ERR(wrapper->base))
>>           return PTR_ERR(wrapper->base);
>>
>> -    if (!has_acpi_companion(&pdev->dev)) {
>> -        const struct geni_se_desc *desc;
>> -        int i;
>> -
>> -        desc = device_get_match_data(&pdev->dev);
>> -        if (!desc)
>> -            return -EINVAL;
>> -
>> -        wrapper->num_clks = min_t(unsigned int, desc->num_clks, 
>> MAX_CLKS);
>> -
>> -        for (i = 0; i < wrapper->num_clks; ++i)
>> -            wrapper->clks[i].id = desc->clks[i];
>> -
>> -        ret = of_count_phandle_with_args(dev->of_node, "clocks", 
>> "#clock-cells");
>> -        if (ret < 0) {
>> -            dev_err(dev, "invalid clocks property at %pOF\n", 
>> dev->of_node);
>> -            return ret;
>> -        }
>> +    desc = device_get_match_data(&pdev->dev);
>>
>> -        if (ret < wrapper->num_clks) {
>> -            dev_err(dev, "invalid clocks count at %pOF, expected %d 
>> entries\n",
>> -                dev->of_node, wrapper->num_clks);
>> +    if (!has_acpi_companion(&pdev->dev) && desc->num_clks) {
>> +        ret = desc->resources_init(wrapper, desc);
>> +        if (ret)
>>               return -EINVAL;
>> -        }
>> -
>> -        ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
>> -        if (ret) {
>> -            dev_err(dev, "Err getting clks %d\n", ret);
>> -            return ret;
>> -        }
>>       }
>>
>>       dev_set_drvdata(dev, wrapper);
>> @@ -951,8 +967,11 @@ static const char * const qup_clks[] = {
>>   static const struct geni_se_desc qup_desc = {
>>       .clks = qup_clks,
>>       .num_clks = ARRAY_SIZE(qup_clks),
>> +    .resources_init = geni_se_resource_init,
>>   };
>>
>> +static const struct geni_se_desc sa8255p_qup_desc;
>> +
>>   static const char * const i2c_master_hub_clks[] = {
>>       "s-ahb",
>>   };
>> @@ -960,11 +979,13 @@ static const char * const i2c_master_hub_clks[] = {
>>   static const struct geni_se_desc i2c_master_hub_desc = {
>>       .clks = i2c_master_hub_clks,
>>       .num_clks = ARRAY_SIZE(i2c_master_hub_clks),
>> +    .resources_init = geni_se_resource_init,
>>   };
>>
>>   static const struct of_device_id geni_se_dt_match[] = {
>>       { .compatible = "qcom,geni-se-qup", .data = &qup_desc },
>>       { .compatible = "qcom,geni-se-i2c-master-hub", .data = 
>> &i2c_master_hub_desc },
>> +    { .compatible = "qcom,sa8255p-geni-se-qup", .data = 
>> &sa8255p_qup_desc },
>>       {}
>>   };
>>   MODULE_DEVICE_TABLE(of, geni_se_dt_match);
>> -- 
>> 2.17.1
>>
>>
> 
> ---
> bod

Re: [PATCH v6 6/8] serial: qcom-geni: move clock-rate logic to separate function

[Praveen Talari]

HI Bjorn,

Thank you for review.

On 6/17/2025 9:37 PM, Bjorn Andersson wrote:
> On Mon, Jun 16, 2025 at 09:34:27PM +0530, Praveen Talari wrote:
>> Hi Bryan,
>>
>> Gentle reminder!!
>>
> 
> As I've told you all countless times, if you want attention to your
> patchset review each others patches! For some reason you're the only one
> showing interest in getting this series merged.

My intention is to CC Bryan with a polite reminder, one week after the 
initial post.

> 
>> Thanks,
>> Praveen Talari
>>
>> On 6/6/2025 10:51 PM, Praveen Talari wrote:
>>> Facilitates future modifications within the new function,
>>> leading to better readability and maintainability of the code.
>>>
>>> Move the code that handles the actual logic of clock-rate
>>> calculations to a separate function geni_serial_set_rate()
>>> which enhances code readability.
>>>
>>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>>> ---
>>> v5 -> v6
>>> - used "unsigned int" instead of "unsigned long" in newly
>>>     added API function params to avoid the format specifier
>>>     warnings.
>>>
>>> v3 -> v4
>>> - added version log after ---
>>>
>>> v1 -> v2
>>> - resolved build warnings for datatype format specifiers
>>> - removed double spaces in log
>>> ---
>>>    drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++----------
>>>    1 file changed, 36 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>> index 715db35bab2f..b6fa7dc9b1fb 100644
>>> --- a/drivers/tty/serial/qcom_geni_serial.c
>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>> @@ -1283,27 +1283,14 @@ static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
>>>    	return ser_clk;
>>>    }
>>> -static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>> -					 struct ktermios *termios,
>>> -					 const struct ktermios *old)
>>> +static int geni_serial_set_rate(struct uart_port *uport, unsigned int baud)
>>>    {
>>> -	unsigned int baud;
>>> -	u32 bits_per_char;
>>> -	u32 tx_trans_cfg;
>>> -	u32 tx_parity_cfg;
>>> -	u32 rx_trans_cfg;
>>> -	u32 rx_parity_cfg;
>>> -	u32 stop_bit_len;
>>> -	unsigned int clk_div;
>>> -	u32 ser_clk_cfg;
>>>    	struct qcom_geni_serial_port *port = to_dev_port(uport);
>>>    	unsigned long clk_rate;
>>> -	u32 ver, sampling_rate;
>>>    	unsigned int avg_bw_core;
>>> -	unsigned long timeout;
>>> -
>>> -	/* baud rate */
>>> -	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
>>> +	unsigned int clk_div;
>>> +	u32 ver, sampling_rate;
>>> +	u32 ser_clk_cfg;
>>>    	sampling_rate = UART_OVERSAMPLING;
>>>    	/* Sampling rate is halved for IP versions >= 2.5 */
>>> @@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>    		dev_err(port->se.dev,
>>>    			"Couldn't find suitable clock rate for %u\n",
>>>    			baud * sampling_rate);
>>> -		return;
>>> +		return -EINVAL;
>>>    	}
>>>    	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
>>> @@ -1339,6 +1326,37 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>    	port->se.icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(baud);
>>>    	geni_icc_set_bw(&port->se);
>>> +	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
>>> +	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
>>> +	return 0;
>>> +}
>>> +
>>> +static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>> +					 struct ktermios *termios,
>>> +					 const struct ktermios *old)
>>> +{
>>> +	struct qcom_geni_serial_port *port = to_dev_port(uport);
>>> +	unsigned int baud;
>>> +	unsigned long timeout;
>>> +	u32 bits_per_char;
>>> +	u32 tx_trans_cfg;
>>> +	u32 tx_parity_cfg;
>>> +	u32 rx_trans_cfg;
>>> +	u32 rx_parity_cfg;
>>> +	u32 stop_bit_len;
>>> +	int ret = 0;
>>> +
>>> +	/* baud rate */
>>> +	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
>>> +
>>> +	ret = geni_serial_set_rate(uport, baud);
>>> +	if (ret) {
>>> +		dev_err(port->se.dev,
>>> +			"%s: Failed to set baud:%u ret:%d\n",
>>> +			__func__, baud, ret);
> 
> As far as I can tell there's one error path in geni_serial_set_rate()
> and there you already printed a more specific error message, as such
> this doesn't add any value.
Sure, will review and update in next patch-set.

Thanks,
Praveen Talari
> 
> PS. In general, please don't use __func__, write helpful error messages
> instead.
> 
> Regards,
> Bjorn
> 
>>> +		return;
>>> +	}
>>> +
>>>    	/* parity */
>>>    	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
>>>    	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
>>> @@ -1406,8 +1424,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>    	writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
>>>    	writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
>>>    	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
>>> -	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
>>> -	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
>>>    }
>>>    #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Praveen Talari]

Hi Bjorn,

Thank you for review.

On 6/17/2025 9:23 PM, Bjorn Andersson wrote:
> On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
>> Add Power Management (PM) runtime support to Qualcomm GENI
>> serial driver.
>>
> 
> Doesn't this have impact on the behavior outside of your
> project? Or is the transition from qcom_geni_serial_pm() to explicit
> RPM merely moving code around?
> 
> Seems like this deserves to not be hidden in a middle of a patch series.
> 
>> Introduce necessary callbacks and updates to ensure seamless
>> transitions between power states, enhancing overall power
>> efficiency.
>>
> 
> This commit message fails to state why we need runtime PM support in the
> driver.

Introduce PM runtime support to the Qualcomm GENI serial driver to enable
better power efficiency and modularity across diverse resource control
mechanisms, including Linux and firmware-managed systems.

As part of this enhancement, the existing qcom_geni_serial_pm() logic to
use standard PM runtime APIs such as pm_runtime_resume_and_get() and
pm_runtime_put_sync(). Power state transitions are now handled through
the geni_serial_resources_on() and geni_serial_resources_off() functions.

Is it fine?
Please guide me/correct me if needed

Thanks,
Praveen Talari
> 
> Also, start your commit message with a problem description, per
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>> v5 -> v6
>> - added reviewed-by tag in commit
>> - added __maybe_unused to PM callback functions to avoid
>>    warnings of defined but not used
>> ---
>>   drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index b6fa7dc9b1fb..3691340ce7e8 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>>   		old_state = UART_PM_STATE_OFF;
>>   
>>   	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>> -		geni_serial_resources_on(uport);
>> +		pm_runtime_resume_and_get(uport->dev);
>>   	else if (new_state == UART_PM_STATE_OFF &&
>>   		 old_state == UART_PM_STATE_ON)
>> -		geni_serial_resources_off(uport);
>> +		pm_runtime_put_sync(uport->dev);
>>   
>>   }
>>   
>> @@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>   		return ret;
>>   	}
>>   
>> +	pm_runtime_enable(port->se.dev);
> 
> Any reason not to use devm_pm_runtime_enable() and avoid the
> two pm_runtime_disable() below?
> 
> Regards,
> Bjorn
> 
>> +
>>   	ret = uart_add_one_port(drv, uport);
>>   	if (ret)
>> -		return ret;
>> +		goto error;
>>   
>>   	if (port->wakeup_irq > 0) {
>>   		device_init_wakeup(&pdev->dev, true);
>> @@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>   			device_init_wakeup(&pdev->dev, false);
>>   			ida_free(&port_ida, uport->line);
>>   			uart_remove_one_port(drv, uport);
>> -			return ret;
>> +			goto error;
>>   		}
>>   	}
>>   
>>   	return 0;
>> +
>> +error:
>> +	pm_runtime_disable(port->se.dev);
>> +	return ret;
>>   }
>>   
>>   static void qcom_geni_serial_remove(struct platform_device *pdev)
>> @@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>>   	dev_pm_clear_wake_irq(&pdev->dev);
>>   	device_init_wakeup(&pdev->dev, false);
>>   	ida_free(&port_ida, uport->line);
>> +	pm_runtime_disable(port->se.dev);
>>   	uart_remove_one_port(drv, &port->uport);
>>   }
>>   
>> +static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
>> +{
>> +	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>> +	struct uart_port *uport = &port->uport;
>> +
>> +	return geni_serial_resources_off(uport);
>> +}
>> +
>> +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;
>> +
>> +	return geni_serial_resources_on(uport);
>> +}
>> +
>>   static int qcom_geni_serial_suspend(struct device *dev)
>>   {
>>   	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>> @@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
>>   };
>>   
>>   static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
>> +			   qcom_geni_serial_runtime_resume, NULL)
>>   	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
>>   };
>>   
>> -- 
>> 2.17.1
>>

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Krzysztof Kozlowski]

On 30/06/2025 07:10, Praveen Talari wrote:
> Hi Bjorn,
> 
> Thank you for review.
> 
> On 6/17/2025 9:23 PM, Bjorn Andersson wrote:
>> On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
>>> Add Power Management (PM) runtime support to Qualcomm GENI
>>> serial driver.
>>>
>>
>> Doesn't this have impact on the behavior outside of your
>> project? Or is the transition from qcom_geni_serial_pm() to explicit
>> RPM merely moving code around?
>>
>> Seems like this deserves to not be hidden in a middle of a patch series.
>>
>>> Introduce necessary callbacks and updates to ensure seamless
>>> transitions between power states, enhancing overall power
>>> efficiency.
>>>
>>
>> This commit message fails to state why we need runtime PM support in the
>> driver.
> 
> Introduce PM runtime support to the Qualcomm GENI serial driver to enable
> better power efficiency and modularity across diverse resource control
> mechanisms, including Linux and firmware-managed systems.


No, not better, that's some marketing blob without saying how this power
efficiency is visible/observable.

Best regards,
Krzysztof

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Dmitry Baryshkov]

On Mon, Jun 30, 2025 at 10:40:25AM +0530, Praveen Talari wrote:
> Hi Bjorn,
> 
> Thank you for review.
> 
> On 6/17/2025 9:23 PM, Bjorn Andersson wrote:
> > On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
> > > Add Power Management (PM) runtime support to Qualcomm GENI
> > > serial driver.
> > > 
> > 
> > Doesn't this have impact on the behavior outside of your
> > project? Or is the transition from qcom_geni_serial_pm() to explicit
> > RPM merely moving code around?
> > 
> > Seems like this deserves to not be hidden in a middle of a patch series.
> > 
> > > Introduce necessary callbacks and updates to ensure seamless
> > > transitions between power states, enhancing overall power
> > > efficiency.
> > > 
> > 
> > This commit message fails to state why we need runtime PM support in the
> > driver.
> 
> Introduce PM runtime support to the Qualcomm GENI serial driver to enable
> better power efficiency and modularity across diverse resource control
> mechanisms, including Linux and firmware-managed systems.
> 
> As part of this enhancement, the existing qcom_geni_serial_pm() logic to
> use standard PM runtime APIs such as pm_runtime_resume_and_get() and
> pm_runtime_put_sync(). Power state transitions are now handled through
> the geni_serial_resources_on() and geni_serial_resources_off() functions.
> 
> Is it fine?
> Please guide me/correct me if needed

Please start by stating out the problem that you are trying to solve.
There is no actual issue description in your patch.

> 
> Thanks,
> Praveen Talari
> > 
> > Also, start your commit message with a problem description, per
> > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> > 
> > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > > Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> > > ---
> > > v5 -> v6
> > > - added reviewed-by tag in commit
> > > - added __maybe_unused to PM callback functions to avoid
> > >    warnings of defined but not used
> > > ---
> > >   drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
> > >   1 file changed, 29 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > index b6fa7dc9b1fb..3691340ce7e8 100644
> > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > @@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
> > >   		old_state = UART_PM_STATE_OFF;
> > >   	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
> > > -		geni_serial_resources_on(uport);
> > > +		pm_runtime_resume_and_get(uport->dev);
> > >   	else if (new_state == UART_PM_STATE_OFF &&
> > >   		 old_state == UART_PM_STATE_ON)
> > > -		geni_serial_resources_off(uport);
> > > +		pm_runtime_put_sync(uport->dev);
> > >   }
> > > @@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> > >   		return ret;
> > >   	}
> > > +	pm_runtime_enable(port->se.dev);
> > 
> > Any reason not to use devm_pm_runtime_enable() and avoid the
> > two pm_runtime_disable() below?
> > 
> > Regards,
> > Bjorn
> > 
> > > +
> > >   	ret = uart_add_one_port(drv, uport);
> > >   	if (ret)
> > > -		return ret;
> > > +		goto error;
> > >   	if (port->wakeup_irq > 0) {
> > >   		device_init_wakeup(&pdev->dev, true);
> > > @@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> > >   			device_init_wakeup(&pdev->dev, false);
> > >   			ida_free(&port_ida, uport->line);
> > >   			uart_remove_one_port(drv, uport);
> > > -			return ret;
> > > +			goto error;
> > >   		}
> > >   	}
> > >   	return 0;
> > > +
> > > +error:
> > > +	pm_runtime_disable(port->se.dev);
> > > +	return ret;
> > >   }
> > >   static void qcom_geni_serial_remove(struct platform_device *pdev)
> > > @@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
> > >   	dev_pm_clear_wake_irq(&pdev->dev);
> > >   	device_init_wakeup(&pdev->dev, false);
> > >   	ida_free(&port_ida, uport->line);
> > > +	pm_runtime_disable(port->se.dev);
> > >   	uart_remove_one_port(drv, &port->uport);
> > >   }
> > > +static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
> > > +{
> > > +	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> > > +	struct uart_port *uport = &port->uport;
> > > +
> > > +	return geni_serial_resources_off(uport);
> > > +}
> > > +
> > > +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;
> > > +
> > > +	return geni_serial_resources_on(uport);
> > > +}
> > > +
> > >   static int qcom_geni_serial_suspend(struct device *dev)
> > >   {
> > >   	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> > > @@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
> > >   };
> > >   static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
> > > +	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
> > > +			   qcom_geni_serial_runtime_resume, NULL)
> > >   	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
> > >   };
> > > -- 
> > > 2.17.1
> > > 

-- 
With best wishes
Dmitry

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Praveen Talari]

Hi Dmitry/Bjorn/Krzysztof,

Thank you for review.

On 7/1/2025 4:42 PM, Dmitry Baryshkov wrote:
> On Mon, Jun 30, 2025 at 10:40:25AM +0530, Praveen Talari wrote:
>> Hi Bjorn,
>>
>> Thank you for review.
>>
>> On 6/17/2025 9:23 PM, Bjorn Andersson wrote:
>>> On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
>>>> Add Power Management (PM) runtime support to Qualcomm GENI
>>>> serial driver.
>>>>
>>>
>>> Doesn't this have impact on the behavior outside of your
>>> project? Or is the transition from qcom_geni_serial_pm() to explicit
>>> RPM merely moving code around?
>>>
>>> Seems like this deserves to not be hidden in a middle of a patch series.
>>>
>>>> Introduce necessary callbacks and updates to ensure seamless
>>>> transitions between power states, enhancing overall power
>>>> efficiency.
>>>>
>>>
>>> This commit message fails to state why we need runtime PM support in the
>>> driver.
>>
>> Introduce PM runtime support to the Qualcomm GENI serial driver to enable
>> better power efficiency and modularity across diverse resource control
>> mechanisms, including Linux and firmware-managed systems.
>>
>> As part of this enhancement, the existing qcom_geni_serial_pm() logic to
>> use standard PM runtime APIs such as pm_runtime_resume_and_get() and
>> pm_runtime_put_sync(). Power state transitions are now handled through
>> the geni_serial_resources_on() and geni_serial_resources_off() functions.
>>
>> Is it fine?
>> Please guide me/correct me if needed
> 
> Please start by stating out the problem that you are trying to solve.
> There is no actual issue description in your patch.

I hope this commit describes the actual problem.

The GENI serial driver currently handles power resource management
through calls to the statically defined geni_serial_resources_on() and
geni_serial_resources_off() functions. This approach reduces modularity
and limits support for platforms with diverse power management 
mechanisms, including resource managed by firmware.

Improve modularity and enable better integration with platform-specific
power management, introduce support for runtime PM. Use
pm_runtime_resume_and_get() and pm_runtime_put_sync() within the
qcom_geni_serial_pm() callback to control resource power state 
transitions based on UART power state changes.

Thanks,
Praveen Talari
> 
>>
>> Thanks,
>> Praveen Talari
>>>
>>> Also, start your commit message with a problem description, per
>>> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
>>>
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>>>> ---
>>>> v5 -> v6
>>>> - added reviewed-by tag in commit
>>>> - added __maybe_unused to PM callback functions to avoid
>>>>     warnings of defined but not used
>>>> ---
>>>>    drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
>>>>    1 file changed, 29 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>>> index b6fa7dc9b1fb..3691340ce7e8 100644
>>>> --- a/drivers/tty/serial/qcom_geni_serial.c
>>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>>> @@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>>>>    		old_state = UART_PM_STATE_OFF;
>>>>    	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>>>> -		geni_serial_resources_on(uport);
>>>> +		pm_runtime_resume_and_get(uport->dev);
>>>>    	else if (new_state == UART_PM_STATE_OFF &&
>>>>    		 old_state == UART_PM_STATE_ON)
>>>> -		geni_serial_resources_off(uport);
>>>> +		pm_runtime_put_sync(uport->dev);
>>>>    }
>>>> @@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>>>    		return ret;
>>>>    	}
>>>> +	pm_runtime_enable(port->se.dev);
>>>
>>> Any reason not to use devm_pm_runtime_enable() and avoid the
>>> two pm_runtime_disable() below?
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> +
>>>>    	ret = uart_add_one_port(drv, uport);
>>>>    	if (ret)
>>>> -		return ret;
>>>> +		goto error;
>>>>    	if (port->wakeup_irq > 0) {
>>>>    		device_init_wakeup(&pdev->dev, true);
>>>> @@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>>>    			device_init_wakeup(&pdev->dev, false);
>>>>    			ida_free(&port_ida, uport->line);
>>>>    			uart_remove_one_port(drv, uport);
>>>> -			return ret;
>>>> +			goto error;
>>>>    		}
>>>>    	}
>>>>    	return 0;
>>>> +
>>>> +error:
>>>> +	pm_runtime_disable(port->se.dev);
>>>> +	return ret;
>>>>    }
>>>>    static void qcom_geni_serial_remove(struct platform_device *pdev)
>>>> @@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>>>>    	dev_pm_clear_wake_irq(&pdev->dev);
>>>>    	device_init_wakeup(&pdev->dev, false);
>>>>    	ida_free(&port_ida, uport->line);
>>>> +	pm_runtime_disable(port->se.dev);
>>>>    	uart_remove_one_port(drv, &port->uport);
>>>>    }
>>>> +static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
>>>> +{
>>>> +	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>>>> +	struct uart_port *uport = &port->uport;
>>>> +
>>>> +	return geni_serial_resources_off(uport);
>>>> +}
>>>> +
>>>> +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;
>>>> +
>>>> +	return geni_serial_resources_on(uport);
>>>> +}
>>>> +
>>>>    static int qcom_geni_serial_suspend(struct device *dev)
>>>>    {
>>>>    	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>>>> @@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
>>>>    };
>>>>    static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
>>>> +	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
>>>> +			   qcom_geni_serial_runtime_resume, NULL)
>>>>    	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
>>>>    };
>>>> -- 
>>>> 2.17.1
>>>>
> 

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

[Dmitry Baryshkov]

On Mon, 14 Jul 2025 at 19:21, Praveen Talari <quic_ptalari@quicinc.com> wrote:
>
> Hi Dmitry/Bjorn/Krzysztof,
>
> Thank you for review.
>
> On 7/1/2025 4:42 PM, Dmitry Baryshkov wrote:
> > On Mon, Jun 30, 2025 at 10:40:25AM +0530, Praveen Talari wrote:
> >> Hi Bjorn,
> >>
> >> Thank you for review.
> >>
> >> On 6/17/2025 9:23 PM, Bjorn Andersson wrote:
> >>> On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
> >>>> Add Power Management (PM) runtime support to Qualcomm GENI
> >>>> serial driver.
> >>>>
> >>>
> >>> Doesn't this have impact on the behavior outside of your
> >>> project? Or is the transition from qcom_geni_serial_pm() to explicit
> >>> RPM merely moving code around?
> >>>
> >>> Seems like this deserves to not be hidden in a middle of a patch series.
> >>>
> >>>> Introduce necessary callbacks and updates to ensure seamless
> >>>> transitions between power states, enhancing overall power
> >>>> efficiency.
> >>>>
> >>>
> >>> This commit message fails to state why we need runtime PM support in the
> >>> driver.
> >>
> >> Introduce PM runtime support to the Qualcomm GENI serial driver to enable
> >> better power efficiency and modularity across diverse resource control
> >> mechanisms, including Linux and firmware-managed systems.
> >>
> >> As part of this enhancement, the existing qcom_geni_serial_pm() logic to
> >> use standard PM runtime APIs such as pm_runtime_resume_and_get() and
> >> pm_runtime_put_sync(). Power state transitions are now handled through
> >> the geni_serial_resources_on() and geni_serial_resources_off() functions.
> >>
> >> Is it fine?
> >> Please guide me/correct me if needed
> >
> > Please start by stating out the problem that you are trying to solve.
> > There is no actual issue description in your patch.
>
> I hope this commit describes the actual problem.
>
> The GENI serial driver currently handles power resource management
> through calls to the statically defined geni_serial_resources_on() and
> geni_serial_resources_off() functions. This approach reduces modularity
> and limits support for platforms with diverse power management
> mechanisms, including resource managed by firmware.
>
> Improve modularity and enable better integration with platform-specific
> power management, introduce support for runtime PM. Use
> pm_runtime_resume_and_get() and pm_runtime_put_sync() within the
> qcom_geni_serial_pm() callback to control resource power state
> transitions based on UART power state changes.

LGTM

>
> Thanks,
> Praveen Talari
> >
> >>
> >> Thanks,
> >> Praveen Talari
> >>>
> >>> Also, start your commit message with a problem description, per
> >>> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> >>>
> >>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >>>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> >>>> ---
> >>>> v5 -> v6
> >>>> - added reviewed-by tag in commit
> >>>> - added __maybe_unused to PM callback functions to avoid
> >>>>     warnings of defined but not used
> >>>> ---
> >>>>    drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
> >>>>    1 file changed, 29 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> >>>> index b6fa7dc9b1fb..3691340ce7e8 100644
> >>>> --- a/drivers/tty/serial/qcom_geni_serial.c
> >>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
> >>>> @@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
> >>>>                    old_state = UART_PM_STATE_OFF;
> >>>>            if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
> >>>> -          geni_serial_resources_on(uport);
> >>>> +          pm_runtime_resume_and_get(uport->dev);
> >>>>            else if (new_state == UART_PM_STATE_OFF &&
> >>>>                     old_state == UART_PM_STATE_ON)
> >>>> -          geni_serial_resources_off(uport);
> >>>> +          pm_runtime_put_sync(uport->dev);
> >>>>    }
> >>>> @@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> >>>>                    return ret;
> >>>>            }
> >>>> +  pm_runtime_enable(port->se.dev);
> >>>
> >>> Any reason not to use devm_pm_runtime_enable() and avoid the
> >>> two pm_runtime_disable() below?
> >>>
> >>> Regards,
> >>> Bjorn
> >>>
> >>>> +
> >>>>            ret = uart_add_one_port(drv, uport);
> >>>>            if (ret)
> >>>> -          return ret;
> >>>> +          goto error;
> >>>>            if (port->wakeup_irq > 0) {
> >>>>                    device_init_wakeup(&pdev->dev, true);
> >>>> @@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> >>>>                            device_init_wakeup(&pdev->dev, false);
> >>>>                            ida_free(&port_ida, uport->line);
> >>>>                            uart_remove_one_port(drv, uport);
> >>>> -                  return ret;
> >>>> +                  goto error;
> >>>>                    }
> >>>>            }
> >>>>            return 0;
> >>>> +
> >>>> +error:
> >>>> +  pm_runtime_disable(port->se.dev);
> >>>> +  return ret;
> >>>>    }
> >>>>    static void qcom_geni_serial_remove(struct platform_device *pdev)
> >>>> @@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
> >>>>            dev_pm_clear_wake_irq(&pdev->dev);
> >>>>            device_init_wakeup(&pdev->dev, false);
> >>>>            ida_free(&port_ida, uport->line);
> >>>> +  pm_runtime_disable(port->se.dev);
> >>>>            uart_remove_one_port(drv, &port->uport);
> >>>>    }
> >>>> +static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
> >>>> +{
> >>>> +  struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> >>>> +  struct uart_port *uport = &port->uport;
> >>>> +
> >>>> +  return geni_serial_resources_off(uport);
> >>>> +}
> >>>> +
> >>>> +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;
> >>>> +
> >>>> +  return geni_serial_resources_on(uport);
> >>>> +}
> >>>> +
> >>>>    static int qcom_geni_serial_suspend(struct device *dev)
> >>>>    {
> >>>>            struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> >>>> @@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
> >>>>    };
> >>>>    static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
> >>>> +  SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
> >>>> +                     qcom_geni_serial_runtime_resume, NULL)
> >>>>            SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
> >>>>    };
> >>>> --
> >>>> 2.17.1
> >>>>
> >



-- 
With best wishes
Dmitry