[PATCH v4 0/3] soc: samsung: usi: implement support for USIv1

[PATCH v4 0/3] soc: samsung: usi: implement support for USIv1

[Ivaylo Ivanov]

Hey folks,

This patch series adds support for USIv1 in the existing exynos-usi driver,
as well as dedicated sysreg compatibles for exynos8895.

The USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895).
It provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
SPI, UART, UART_HSI2C1). It's a bit different from USIv2 as it doesn't
have any known MMIO register map and the serial protocols that it
implements share the same register base, hence why the way of modelling it
in device trees will be with ranges, like so:

usi1: usi@10460000 {
  compatible = "samsung,exynos8895-usi";
  ranges = <0x0 0x10460000 0x11000>;
  clocks = <1>, <2>;
  clock-names = "pclk", "ipclk";
  #address-cells = <1>;
  #size-cells = <1>;
  samsung,sysreg = <&syscon_peric0 0x1004>;
  status = "disabled";

  hsi2c_5: i2c@0 {
    compatible = "samsung,exynos8895-hsi2c";
    reg = <0x0 0x1000>;
    ...
  };
};

This patchset also assumes that [1] and [2] have been merged before it.
This has to be applied before the device tree changes [3].

Best regards,
Ivaylo

[1]: https://lore.kernel.org/all/20241222145257.31451-1-krzysztof.kozlowski@linaro.org/
[2]: https://lore.kernel.org/all/20250103082549.19419-1-krzysztof.kozlowski@linaro.org/
[3]: https://lore.kernel.org/all/20250105161344.420749-1-ivo.ivanov.ivanov1@gmail.com/

Changes in v4:
  - merge the first and second patch and don't break compilation
  - add exynos8895 compatible in the binding's oneOf:
  - keep exynos850's compatible in the first allOf: if
  - add exynos_usi_remove callback and error path at the end of probe,
    making sure to also have a removal routine for usiv2 and not just v1

Changes in v3:
  - drop the sysreg patch as it was applied
  - add a patch at the beginning of the series for renaming all USI_V2
    constants to USI_MODE_ and a patch at the end to rename them in dt
  - redo the usi binding support for 8895 to hopefully match all
    feedback from Krzysztof
  - change the description of the usiv1 and 8895 binding patch in order
    to account for the constants changes
  - change the subject and description of the usiv1 driver support patch
    because we're adding support for exynos8895 in the first place
  - make exynos_usi_modes a two dimensional array while also accounting
    for the merged usi mode constants

Changes in v2:
  - add r-b from Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
  - restrict the possible ids of samsung,mode with an allOf:if:then
  - set the properties samsung,clkreq-on and reg to false for non-usiv2
  - only make use of exynos_usi_modes
  - make sure pclk and ipclk are enabled

Ivaylo Ivanov (3):
  dt-bindings: soc: samsung: usi: add USIv1 and samsung,exynos8895-usi
  soc: samsung: usi: implement support for USIv1 and exynos8895
  arm64: dts: exynos: update all samsung,mode constants

 .../bindings/soc/samsung/exynos-usi.yaml      |  99 ++++++++++------
 arch/arm64/boot/dts/exynos/exynos850.dtsi     |  14 +--
 arch/arm64/boot/dts/exynos/exynosautov9.dtsi  |  48 ++++----
 .../arm64/boot/dts/exynos/exynosautov920.dtsi |   2 +-
 .../boot/dts/exynos/google/gs101-oriole.dts   |   4 +-
 arch/arm64/boot/dts/exynos/google/gs101.dtsi  |   2 +-
 drivers/soc/samsung/exynos-usi.c              | 108 +++++++++++++++---
 include/dt-bindings/soc/samsung,exynos-usi.h  |  17 ++-
 8 files changed, 209 insertions(+), 85 deletions(-)

-- 
2.43.0

[PATCH v4 1/3] dt-bindings: soc: samsung: usi: add USIv1 and samsung,exynos8895-usi

[Ivaylo Ivanov]

Add new constants for choosing the additional USIv1 configuration modes
in device tree. Those are further used in the USI driver to figure out
which value to write into SW_CONF register. Modify the current USI IP-core
bindings to include information about USIv1 and a compatible for
exynos8895.

In the original bindings commit, protocol mode definitions were named
with the version of the supported USI (in this case, V2) with the idea of
leaving enough room in the future for other versions of this block. This,
however, is not how the modes should be modelled. The modes are not
version specific and you should not be able to tell USI which version of
a mode to use - that has to be handled in the driver - thus encoding this
information in the binding is meaningless. Only one constant per mode is
needed, so while we're at it, add new constants with the prefix USI_MODE
and mark the old ones as depracated.

Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
---
 .../bindings/soc/samsung/exynos-usi.yaml      | 99 ++++++++++++-------
 include/dt-bindings/soc/samsung,exynos-usi.h  | 17 +++-
 2 files changed, 79 insertions(+), 37 deletions(-)

diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
index 5b046932f..f711e23c0 100644
--- a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
+++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
@@ -11,11 +11,21 @@ maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
 
 description: |
-  USI IP-core provides selectable serial protocol (UART, SPI or High-Speed I2C).
-  USI shares almost all internal circuits within each protocol, so only one
-  protocol can be chosen at a time. USI is modeled as a node with zero or more
-  child nodes, each representing a serial sub-node device. The mode setting
-  selects which particular function will be used.
+  The USI IP-core provides configurable support for serial protocols, enabling
+  different serial communication modes depending on the version.
+
+  In USIv1, configurations are available to enable either one or two protocols
+  simultaneously in select combinations - High-Speed I2C0, High-Speed
+  I2C1, SPI, UART, High-Speed I2C0 and I2C1 or both High-Speed
+  I2C1 and UART.
+
+  In USIv2, only one protocol can be active at a time, either UART, SPI, or
+  High-Speed I2C.
+
+  The USI core shares internal circuits across protocols, meaning only the
+  selected configuration is active at any given time. USI is modeled as a node
+  with zero or more child nodes, each representing a serial sub-node device. The
+  mode setting selects which particular function will be used.
 
 properties:
   $nodename:
@@ -31,6 +41,7 @@ properties:
           - const: samsung,exynos850-usi
       - enum:
           - samsung,exynos850-usi
+          - samsung,exynos8895-usi
 
   reg:
     maxItems: 1
@@ -64,7 +75,7 @@ properties:
 
   samsung,mode:
     $ref: /schemas/types.yaml#/definitions/uint32
-    enum: [0, 1, 2, 3]
+    enum: [0, 1, 2, 3, 4, 5, 6]
     description:
       Selects USI function (which serial protocol to use). Refer to
       <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values.
@@ -101,37 +112,59 @@ required:
   - samsung,sysreg
   - samsung,mode
 
-if:
-  properties:
-    compatible:
-      contains:
-        enum:
-          - samsung,exynos850-usi
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - samsung,exynos850-usi
+
+    then:
+      properties:
+        reg:
+          maxItems: 1
+
+        clocks:
+          items:
+            - description: Bus (APB) clock
+            - description: Operating clock for UART/SPI/I2C protocol
+
+        clock-names:
+          maxItems: 2
+
+        samsung,mode:
+          enum: [0, 1, 2, 3]
+
+      required:
+        - reg
+        - clocks
+        - clock-names
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - samsung,exynos8895-usi
 
-then:
-  properties:
-    reg:
-      maxItems: 1
+    then:
+      properties:
+        reg: false
 
-    clocks:
-      items:
-        - description: Bus (APB) clock
-        - description: Operating clock for UART/SPI/I2C protocol
+        clocks:
+          items:
+            - description: Bus (APB) clock
+            - description: Operating clock for UART/SPI protocol
 
-    clock-names:
-      maxItems: 2
+        clock-names:
+          maxItems: 2
 
-  required:
-    - reg
-    - clocks
-    - clock-names
+        samsung,clkreq-on: false
 
-else:
-  properties:
-    reg: false
-    clocks: false
-    clock-names: false
-    samsung,clkreq-on: false
+      required:
+        - clocks
+        - clock-names
 
 additionalProperties: false
 
@@ -144,7 +177,7 @@ examples:
         compatible = "samsung,exynos850-usi";
         reg = <0x138200c0 0x20>;
         samsung,sysreg = <&sysreg_peri 0x1010>;
-        samsung,mode = <USI_V2_UART>;
+        samsung,mode = <USI_MODE_UART>;
         samsung,clkreq-on; /* needed for UART mode */
         #address-cells = <1>;
         #size-cells = <1>;
diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
index a01af169d..b46de214d 100644
--- a/include/dt-bindings/soc/samsung,exynos-usi.h
+++ b/include/dt-bindings/soc/samsung,exynos-usi.h
@@ -9,9 +9,18 @@
 #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
 #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
 
-#define USI_V2_NONE		0
-#define USI_V2_UART		1
-#define USI_V2_SPI		2
-#define USI_V2_I2C		3
+#define USI_MODE_NONE		0
+#define USI_MODE_UART		1
+#define USI_MODE_SPI		2
+#define USI_MODE_I2C		3
+#define USI_MODE_I2C1		4
+#define USI_MODE_I2C0_1		5
+#define USI_MODE_UART_I2C1	6
+
+/* Deprecated */
+#define USI_V2_NONE		USI_MODE_NONE
+#define USI_V2_UART		USI_MODE_UART
+#define USI_V2_SPI		USI_MODE_SPI
+#define USI_V2_I2C		USI_MODE_I2C
 
 #endif /* __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H */
-- 
2.43.0

[PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and exynos8895

[Ivaylo Ivanov]

USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895) and
provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
SPI, UART, UART_HSI2C1).

USIv1, unlike USIv2, doesn't have any known register map. Underlying
protocols that it implements have no offset, like with Exynos850.
Desired protocol can be chosen via SW_CONF register from System
Register block of the same domain as USI.

In order to select a particular protocol, the protocol has to be
selected via the System Register. Unlike USIv2, there's no need for
any setup before the given protocol becomes accessible apart from
enabling the APB clock and the protocol operating clock.

Modify the existing driver in order to allow USIv1 instances in
Exynos8895 to probe and set their protocol. While we're at it,
make use of the new mode constants in place of the old ones
and add a removal routine.

Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
---
 drivers/soc/samsung/exynos-usi.c | 108 +++++++++++++++++++++++++++----
 1 file changed, 95 insertions(+), 13 deletions(-)

diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
index 114352695..43c17b100 100644
--- a/drivers/soc/samsung/exynos-usi.c
+++ b/drivers/soc/samsung/exynos-usi.c
@@ -16,6 +16,18 @@
 
 #include <dt-bindings/soc/samsung,exynos-usi.h>
 
+/* USIv1: System Register: SW_CONF register bits */
+#define USI_V1_SW_CONF_NONE		0x0
+#define USI_V1_SW_CONF_I2C0		0x1
+#define USI_V1_SW_CONF_I2C1		0x2
+#define USI_V1_SW_CONF_I2C0_1		0x3
+#define USI_V1_SW_CONF_SPI		0x4
+#define USI_V1_SW_CONF_UART		0x8
+#define USI_V1_SW_CONF_UART_I2C1	0xa
+#define USI_V1_SW_CONF_MASK		(USI_V1_SW_CONF_I2C0 | USI_V1_SW_CONF_I2C1 | \
+					 USI_V1_SW_CONF_I2C0_1 | USI_V1_SW_CONF_SPI | \
+					 USI_V1_SW_CONF_UART | USI_V1_SW_CONF_UART_I2C1)
+
 /* USIv2: System Register: SW_CONF register bits */
 #define USI_V2_SW_CONF_NONE	0x0
 #define USI_V2_SW_CONF_UART	BIT(0)
@@ -34,7 +46,8 @@
 #define USI_OPTION_CLKSTOP_ON	BIT(2)
 
 enum exynos_usi_ver {
-	USI_VER2 = 2,
+	USI_VER1 = 1,
+	USI_VER2,
 };
 
 struct exynos_usi_variant {
@@ -66,19 +79,39 @@ struct exynos_usi_mode {
 	unsigned int val;		/* mode register value */
 };
 
-static const struct exynos_usi_mode exynos_usi_modes[] = {
-	[USI_V2_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
-	[USI_V2_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
-	[USI_V2_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
-	[USI_V2_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
+#define USI_MODES_MAX (USI_MODE_UART_I2C1 + 1)
+static const struct exynos_usi_mode exynos_usi_modes[][USI_MODES_MAX] = {
+	[USI_VER1] = {
+		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V1_SW_CONF_NONE },
+		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V1_SW_CONF_UART },
+		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V1_SW_CONF_SPI },
+		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V1_SW_CONF_I2C0 },
+		[USI_MODE_I2C1] =	{ .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
+		[USI_MODE_I2C0_1] =	{ .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
+		[USI_MODE_UART_I2C1] =	{ .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },
+	}, [USI_VER2] = {
+		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
+		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
+		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
+		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
+	},
 };
 
 static const char * const exynos850_usi_clk_names[] = { "pclk", "ipclk" };
 static const struct exynos_usi_variant exynos850_usi_data = {
 	.ver		= USI_VER2,
 	.sw_conf_mask	= USI_V2_SW_CONF_MASK,
-	.min_mode	= USI_V2_NONE,
-	.max_mode	= USI_V2_I2C,
+	.min_mode	= USI_MODE_NONE,
+	.max_mode	= USI_MODE_I2C,
+	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
+	.clk_names	= exynos850_usi_clk_names,
+};
+
+static const struct exynos_usi_variant exynos8895_usi_data = {
+	.ver		= USI_VER1,
+	.sw_conf_mask	= USI_V1_SW_CONF_MASK,
+	.min_mode	= USI_MODE_NONE,
+	.max_mode	= USI_MODE_UART_I2C1,
 	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
 	.clk_names	= exynos850_usi_clk_names,
 };
@@ -88,6 +121,10 @@ static const struct of_device_id exynos_usi_dt_match[] = {
 		.compatible = "samsung,exynos850-usi",
 		.data = &exynos850_usi_data,
 	},
+	{
+		.compatible = "samsung,exynos8895-usi",
+		.data = &exynos8895_usi_data,
+	},
 	{ } /* sentinel */
 };
 MODULE_DEVICE_TABLE(of, exynos_usi_dt_match);
@@ -109,14 +146,15 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
 	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
 		return -EINVAL;
 
-	val = exynos_usi_modes[mode].val;
+	val = exynos_usi_modes[usi->data->ver][mode].val;
 	ret = regmap_update_bits(usi->sysreg, usi->sw_conf,
 				 usi->data->sw_conf_mask, val);
 	if (ret)
 		return ret;
 
 	usi->mode = mode;
-	dev_dbg(usi->dev, "protocol: %s\n", exynos_usi_modes[usi->mode].name);
+	dev_dbg(usi->dev, "protocol: %s\n",
+		exynos_usi_modes[usi->data->ver][usi->mode].name);
 
 	return 0;
 }
@@ -160,6 +198,30 @@ static int exynos_usi_enable(const struct exynos_usi *usi)
 	return ret;
 }
 
+/**
+ * exynos_usi_disable - Disable USI block
+ * @usi: USI driver object
+ *
+ * USI IP-core needs the reset flag cleared in order to function. This
+ * routine disables the USI block by setting the reset flag. It also disables
+ * HWACG behavior. It should be performed on removal of the device.
+ */
+static void exynos_usi_disable(const struct exynos_usi *usi)
+{
+	u32 val;
+
+	/* Make sure that we've stopped providing the clock to USI IP */
+	val = readl(usi->regs + USI_OPTION);
+	val &= ~USI_OPTION_CLKREQ_ON;
+	val |= ~USI_OPTION_CLKSTOP_ON;
+	writel(val, usi->regs + USI_OPTION);
+
+	/* Set USI block state to reset */
+	val = readl(usi->regs + USI_CON);
+	val |= USI_CON_RESET;
+	writel(val, usi->regs + USI_CON);
+}
+
 static int exynos_usi_configure(struct exynos_usi *usi)
 {
 	int ret;
@@ -169,9 +231,12 @@ static int exynos_usi_configure(struct exynos_usi *usi)
 		return ret;
 
 	if (usi->data->ver == USI_VER2)
-		return exynos_usi_enable(usi);
+		ret = exynos_usi_enable(usi);
+	else
+		ret = clk_bulk_prepare_enable(usi->data->num_clks,
+					      usi->clks);
 
-	return 0;
+	return ret;
 }
 
 static int exynos_usi_parse_dt(struct device_node *np, struct exynos_usi *usi)
@@ -253,10 +318,26 @@ static int exynos_usi_probe(struct platform_device *pdev)
 
 	ret = exynos_usi_configure(usi);
 	if (ret)
-		return ret;
+		goto fail_probe;
 
 	/* Make it possible to embed protocol nodes into USI np */
 	return of_platform_populate(np, NULL, NULL, dev);
+
+fail_probe:
+	if (usi->data->ver != USI_VER2)
+		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);
+
+	return ret;
+}
+
+static void exynos_usi_remove(struct platform_device *pdev)
+{
+	struct exynos_usi *usi = platform_get_drvdata(pdev);
+
+	if (usi->data->ver == USI_VER2)
+		exynos_usi_disable(usi);
+	else
+		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);
 }
 
 static int __maybe_unused exynos_usi_resume_noirq(struct device *dev)
@@ -277,6 +358,7 @@ static struct platform_driver exynos_usi_driver = {
 		.of_match_table	= exynos_usi_dt_match,
 	},
 	.probe = exynos_usi_probe,
+	.remove = exynos_usi_remove,
 };
 module_platform_driver(exynos_usi_driver);
 
-- 
2.43.0

[PATCH v4 3/3] arm64: dts: exynos: update all samsung,mode constants

[Ivaylo Ivanov]

Update all samsung,mode property values to account for renaming USI_V2
constants to USI_MODE in the bindings.

Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
---
 arch/arm64/boot/dts/exynos/exynos850.dtsi     | 14 +++---
 arch/arm64/boot/dts/exynos/exynosautov9.dtsi  | 48 +++++++++----------
 .../arm64/boot/dts/exynos/exynosautov920.dtsi |  2 +-
 .../boot/dts/exynos/google/gs101-oriole.dts   |  4 +-
 arch/arm64/boot/dts/exynos/google/gs101.dtsi  |  2 +-
 5 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
index f1c8b4613..cb55015c8 100644
--- a/arch/arm64/boot/dts/exynos/exynos850.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
@@ -651,7 +651,7 @@ usi_uart: usi@138200c0 {
 			compatible = "samsung,exynos850-usi";
 			reg = <0x138200c0 0x20>;
 			samsung,sysreg = <&sysreg_peri 0x1010>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -677,7 +677,7 @@ usi_hsi2c_0: usi@138a00c0 {
 			compatible = "samsung,exynos850-usi";
 			reg = <0x138a00c0 0x20>;
 			samsung,sysreg = <&sysreg_peri 0x1020>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -706,7 +706,7 @@ usi_hsi2c_1: usi@138b00c0 {
 			compatible = "samsung,exynos850-usi";
 			reg = <0x138b00c0 0x20>;
 			samsung,sysreg = <&sysreg_peri 0x1030>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -735,7 +735,7 @@ usi_hsi2c_2: usi@138c00c0 {
 			compatible = "samsung,exynos850-usi";
 			reg = <0x138c00c0 0x20>;
 			samsung,sysreg = <&sysreg_peri 0x1040>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -764,7 +764,7 @@ usi_spi_0: usi@139400c0 {
 			compatible = "samsung,exynos850-usi";
 			reg = <0x139400c0 0x20>;
 			samsung,sysreg = <&sysreg_peri 0x1050>;
-			samsung,mode = <USI_V2_SPI>;
+			samsung,mode = <USI_MODE_SPI>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -796,7 +796,7 @@ usi_cmgp0: usi@11d000c0 {
 			compatible = "samsung,exynos850-usi";
 			reg = <0x11d000c0 0x20>;
 			samsung,sysreg = <&sysreg_cmgp 0x2000>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -855,7 +855,7 @@ usi_cmgp1: usi@11d200c0 {
 			compatible = "samsung,exynos850-usi";
 			reg = <0x11d200c0 0x20>;
 			samsung,sysreg = <&sysreg_cmgp 0x2010>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
diff --git a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
index b36292a7d..66628cb32 100644
--- a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
@@ -442,7 +442,7 @@ usi_0: usi@103000c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103000c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1000>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -505,7 +505,7 @@ usi_i2c_0: usi@103100c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103100c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1004>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -534,7 +534,7 @@ usi_1: usi@103200c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103200c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1008>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -597,7 +597,7 @@ usi_i2c_1: usi@103300c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103300c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x100c>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -626,7 +626,7 @@ usi_2: usi@103400c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103400c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1010>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -689,7 +689,7 @@ usi_i2c_2: usi@103500c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103500c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1014>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -718,7 +718,7 @@ usi_3: usi@103600c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103600c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1018>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -781,7 +781,7 @@ usi_i2c_3: usi@103700c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103700c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x101c>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -810,7 +810,7 @@ usi_4: usi@103800c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103800c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1020>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -873,7 +873,7 @@ usi_i2c_4: usi@103900c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103900c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1024>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -902,7 +902,7 @@ usi_5: usi@103a00c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103a00c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1028>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -965,7 +965,7 @@ usi_i2c_5: usi@103b00c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x103b00c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x102c>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -994,7 +994,7 @@ usi_6: usi@109000c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109000c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1000>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1057,7 +1057,7 @@ usi_i2c_6: usi@109100c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109100c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1004>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1086,7 +1086,7 @@ usi_7: usi@109200c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109200c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1008>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1149,7 +1149,7 @@ usi_i2c_7: usi@109300c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109300c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x100c>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1178,7 +1178,7 @@ usi_8: usi@109400c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109400c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1010>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1241,7 +1241,7 @@ usi_i2c_8: usi@109500c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109500c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1014>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1270,7 +1270,7 @@ usi_9: usi@109600c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109600c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1018>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1333,7 +1333,7 @@ usi_i2c_9: usi@109700c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109700c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x101c>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1362,7 +1362,7 @@ usi_10: usi@109800c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109800c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1020>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1425,7 +1425,7 @@ usi_i2c_10: usi@109900c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109900c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1024>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1454,7 +1454,7 @@ usi_11: usi@109a00c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109a00c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x1028>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
@@ -1515,7 +1515,7 @@ usi_i2c_11: usi@109b00c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x109b00c0 0x20>;
 			samsung,sysreg = <&syscon_peric1 0x102c>;
-			samsung,mode = <USI_V2_I2C>;
+			samsung,mode = <USI_MODE_I2C>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
diff --git a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
index c759134c9..6e9007518 100644
--- a/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynosautov920.dtsi
@@ -223,7 +223,7 @@ usi_0: usi@108800c0 {
 				     "samsung,exynos850-usi";
 			reg = <0x108800c0 0x20>;
 			samsung,sysreg = <&syscon_peric0 0x1000>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
diff --git a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
index 387fb779b..b73c152c7 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
+++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
@@ -161,12 +161,12 @@ &usi_uart {
 };
 
 &usi8 {
-	samsung,mode = <USI_V2_I2C>;
+	samsung,mode = <USI_MODE_I2C>;
 	status = "okay";
 };
 
 &usi12 {
-	samsung,mode = <USI_V2_I2C>;
+	samsung,mode = <USI_MODE_I2C>;
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index 302c5beb2..473db46aa 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -825,7 +825,7 @@ usi_uart: usi@10a000c0 {
 				 <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_0>;
 			clock-names = "pclk", "ipclk";
 			samsung,sysreg = <&sysreg_peric0 0x1020>;
-			samsung,mode = <USI_V2_UART>;
+			samsung,mode = <USI_MODE_UART>;
 			status = "disabled";
 
 			serial_0: serial@10a00000 {
-- 
2.43.0

Re: [PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and exynos8895

[Krzysztof Kozlowski]

On Tue, Jan 07, 2025 at 01:35:11PM +0200, Ivaylo Ivanov wrote:
> USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895) and
> provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
> SPI, UART, UART_HSI2C1).
> 
> USIv1, unlike USIv2, doesn't have any known register map. Underlying
> protocols that it implements have no offset, like with Exynos850.
> Desired protocol can be chosen via SW_CONF register from System
> Register block of the same domain as USI.
> 
> In order to select a particular protocol, the protocol has to be
> selected via the System Register. Unlike USIv2, there's no need for
> any setup before the given protocol becomes accessible apart from
> enabling the APB clock and the protocol operating clock.
> 
> Modify the existing driver in order to allow USIv1 instances in
> Exynos8895 to probe and set their protocol. While we're at it,
> make use of the new mode constants in place of the old ones
> and add a removal routine.
> 
> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
> ---
>  drivers/soc/samsung/exynos-usi.c | 108 +++++++++++++++++++++++++++----
>  1 file changed, 95 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
> index 114352695..43c17b100 100644
> --- a/drivers/soc/samsung/exynos-usi.c
> +++ b/drivers/soc/samsung/exynos-usi.c
> @@ -16,6 +16,18 @@
>  
>  #include <dt-bindings/soc/samsung,exynos-usi.h>
>  
> +/* USIv1: System Register: SW_CONF register bits */
> +#define USI_V1_SW_CONF_NONE		0x0
> +#define USI_V1_SW_CONF_I2C0		0x1
> +#define USI_V1_SW_CONF_I2C1		0x2
> +#define USI_V1_SW_CONF_I2C0_1		0x3
> +#define USI_V1_SW_CONF_SPI		0x4
> +#define USI_V1_SW_CONF_UART		0x8
> +#define USI_V1_SW_CONF_UART_I2C1	0xa
> +#define USI_V1_SW_CONF_MASK		(USI_V1_SW_CONF_I2C0 | USI_V1_SW_CONF_I2C1 | \
> +					 USI_V1_SW_CONF_I2C0_1 | USI_V1_SW_CONF_SPI | \
> +					 USI_V1_SW_CONF_UART | USI_V1_SW_CONF_UART_I2C1)
> +
>  /* USIv2: System Register: SW_CONF register bits */
>  #define USI_V2_SW_CONF_NONE	0x0
>  #define USI_V2_SW_CONF_UART	BIT(0)
> @@ -34,7 +46,8 @@
>  #define USI_OPTION_CLKSTOP_ON	BIT(2)
>  
>  enum exynos_usi_ver {
> -	USI_VER2 = 2,
> +	USI_VER1 = 1,

Is this assignment=1 actually now helping? Isn't it creating empty item
in exynos_usi_modes array? Basically it wastes space in the array for
no benefits.

> +	USI_VER2,
>  };
>  
>  struct exynos_usi_variant {
> @@ -66,19 +79,39 @@ struct exynos_usi_mode {
>  	unsigned int val;		/* mode register value */
>  };
>  
> -static const struct exynos_usi_mode exynos_usi_modes[] = {
> -	[USI_V2_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
> -	[USI_V2_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
> -	[USI_V2_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
> -	[USI_V2_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
> +#define USI_MODES_MAX (USI_MODE_UART_I2C1 + 1)
> +static const struct exynos_usi_mode exynos_usi_modes[][USI_MODES_MAX] = {
> +	[USI_VER1] = {
> +		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V1_SW_CONF_NONE },
> +		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V1_SW_CONF_UART },
> +		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V1_SW_CONF_SPI },
> +		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V1_SW_CONF_I2C0 },
> +		[USI_MODE_I2C1] =	{ .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
> +		[USI_MODE_I2C0_1] =	{ .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
> +		[USI_MODE_UART_I2C1] =	{ .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },
> +	}, [USI_VER2] = {
> +		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
> +		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
> +		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
> +		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
> +	},
>  };
>  
>  static const char * const exynos850_usi_clk_names[] = { "pclk", "ipclk" };
>  static const struct exynos_usi_variant exynos850_usi_data = {
>  	.ver		= USI_VER2,
>  	.sw_conf_mask	= USI_V2_SW_CONF_MASK,
> -	.min_mode	= USI_V2_NONE,
> -	.max_mode	= USI_V2_I2C,
> +	.min_mode	= USI_MODE_NONE,
> +	.max_mode	= USI_MODE_I2C,
> +	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
> +	.clk_names	= exynos850_usi_clk_names,
> +};
> +
> +static const struct exynos_usi_variant exynos8895_usi_data = {
> +	.ver		= USI_VER1,
> +	.sw_conf_mask	= USI_V1_SW_CONF_MASK,
> +	.min_mode	= USI_MODE_NONE,
> +	.max_mode	= USI_MODE_UART_I2C1,
>  	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
>  	.clk_names	= exynos850_usi_clk_names,
>  };
> @@ -88,6 +121,10 @@ static const struct of_device_id exynos_usi_dt_match[] = {
>  		.compatible = "samsung,exynos850-usi",
>  		.data = &exynos850_usi_data,
>  	},
> +	{

These two are in oone line.

> +		.compatible = "samsung,exynos8895-usi",
> +		.data = &exynos8895_usi_data,
> +	},
>  	{ } /* sentinel */
>  };
>  MODULE_DEVICE_TABLE(of, exynos_usi_dt_match);
> @@ -109,14 +146,15 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
>  	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
>  		return -EINVAL;
>  
> -	val = exynos_usi_modes[mode].val;
> +	val = exynos_usi_modes[usi->data->ver][mode].val;
>  	ret = regmap_update_bits(usi->sysreg, usi->sw_conf,
>  				 usi->data->sw_conf_mask, val);
>  	if (ret)
>  		return ret;
>  
>  	usi->mode = mode;
> -	dev_dbg(usi->dev, "protocol: %s\n", exynos_usi_modes[usi->mode].name);
> +	dev_dbg(usi->dev, "protocol: %s\n",
> +		exynos_usi_modes[usi->data->ver][usi->mode].name);
>  
>  	return 0;
>  }
> @@ -160,6 +198,30 @@ static int exynos_usi_enable(const struct exynos_usi *usi)
>  	return ret;
>  }
>  
> +/**
> + * exynos_usi_disable - Disable USI block
> + * @usi: USI driver object
> + *
> + * USI IP-core needs the reset flag cleared in order to function. This
> + * routine disables the USI block by setting the reset flag. It also disables
> + * HWACG behavior. It should be performed on removal of the device.
> + */
> +static void exynos_usi_disable(const struct exynos_usi *usi)
> +{
> +	u32 val;
> +
> +	/* Make sure that we've stopped providing the clock to USI IP */
> +	val = readl(usi->regs + USI_OPTION);
> +	val &= ~USI_OPTION_CLKREQ_ON;
> +	val |= ~USI_OPTION_CLKSTOP_ON;
> +	writel(val, usi->regs + USI_OPTION);
> +
> +	/* Set USI block state to reset */
> +	val = readl(usi->regs + USI_CON);
> +	val |= USI_CON_RESET;
> +	writel(val, usi->regs + USI_CON);
> +}
> +
>  static int exynos_usi_configure(struct exynos_usi *usi)
>  {
>  	int ret;
> @@ -169,9 +231,12 @@ static int exynos_usi_configure(struct exynos_usi *usi)
>  		return ret;
>  
>  	if (usi->data->ver == USI_VER2)
> -		return exynos_usi_enable(usi);
> +		ret = exynos_usi_enable(usi);
> +	else
> +		ret = clk_bulk_prepare_enable(usi->data->num_clks,
> +					      usi->clks);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int exynos_usi_parse_dt(struct device_node *np, struct exynos_usi *usi)
> @@ -253,10 +318,26 @@ static int exynos_usi_probe(struct platform_device *pdev)
>  
>  	ret = exynos_usi_configure(usi);
>  	if (ret)
> -		return ret;
> +		goto fail_probe;
>  
>  	/* Make it possible to embed protocol nodes into USI np */
>  	return of_platform_populate(np, NULL, NULL, dev);

This also needs error handling.

> +
> +fail_probe:

err_unconfigure:

> +	if (usi->data->ver != USI_VER2)
> +		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);

Move it to its own callback exynos_usi_unconfigure(), so naming will be
symmetric. The probe does not prepare clocks directly, so above code is
not that readable. The most readable is to have symmetrics calls -
configure+unconfigure (or whatever we name it).

> +
> +	return ret;
> +}
> +
> +static void exynos_usi_remove(struct platform_device *pdev)
> +{
> +	struct exynos_usi *usi = platform_get_drvdata(pdev);
> +
> +	if (usi->data->ver == USI_VER2)
> +		exynos_usi_disable(usi);

This is not related to the patch and should be separate patch, if at
all.

> +	else
> +		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);

So the easiest would be to add devm reset action and then no need for
goto-err handling and remove() callback.

Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: soc: samsung: usi: add USIv1 and samsung,exynos8895-usi

[Krzysztof Kozlowski]

On Tue, Jan 07, 2025 at 01:35:10PM +0200, Ivaylo Ivanov wrote:
> Add new constants for choosing the additional USIv1 configuration modes
> in device tree. Those are further used in the USI driver to figure out
> which value to write into SW_CONF register. Modify the current USI IP-core
> bindings to include information about USIv1 and a compatible for
> exynos8895.
> 
> In the original bindings commit, protocol mode definitions were named
> with the version of the supported USI (in this case, V2) with the idea of
> leaving enough room in the future for other versions of this block. This,
> however, is not how the modes should be modelled. The modes are not
> version specific and you should not be able to tell USI which version of
> a mode to use - that has to be handled in the driver - thus encoding this
> information in the binding is meaningless. Only one constant per mode is
> needed, so while we're at it, add new constants with the prefix USI_MODE
> and mark the old ones as depracated.
> 
> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
> ---
>  .../bindings/soc/samsung/exynos-usi.yaml      | 99 ++++++++++++-------
>  include/dt-bindings/soc/samsung,exynos-usi.h  | 17 +++-
>  2 files changed, 79 insertions(+), 37 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and exynos8895

[Ivaylo Ivanov]

On 1/8/25 10:30, Krzysztof Kozlowski wrote:
> On Tue, Jan 07, 2025 at 01:35:11PM +0200, Ivaylo Ivanov wrote:
>> USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895) and
>> provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
>> SPI, UART, UART_HSI2C1).
>>
>> USIv1, unlike USIv2, doesn't have any known register map. Underlying
>> protocols that it implements have no offset, like with Exynos850.
>> Desired protocol can be chosen via SW_CONF register from System
>> Register block of the same domain as USI.
>>
>> In order to select a particular protocol, the protocol has to be
>> selected via the System Register. Unlike USIv2, there's no need for
>> any setup before the given protocol becomes accessible apart from
>> enabling the APB clock and the protocol operating clock.
>>
>> Modify the existing driver in order to allow USIv1 instances in
>> Exynos8895 to probe and set their protocol. While we're at it,
>> make use of the new mode constants in place of the old ones
>> and add a removal routine.
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> ---
>>  drivers/soc/samsung/exynos-usi.c | 108 +++++++++++++++++++++++++++----
>>  1 file changed, 95 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
>> index 114352695..43c17b100 100644
>> --- a/drivers/soc/samsung/exynos-usi.c
>> +++ b/drivers/soc/samsung/exynos-usi.c
>> @@ -16,6 +16,18 @@
>>  
>>  #include <dt-bindings/soc/samsung,exynos-usi.h>
>>  
>> +/* USIv1: System Register: SW_CONF register bits */
>> +#define USI_V1_SW_CONF_NONE		0x0
>> +#define USI_V1_SW_CONF_I2C0		0x1
>> +#define USI_V1_SW_CONF_I2C1		0x2
>> +#define USI_V1_SW_CONF_I2C0_1		0x3
>> +#define USI_V1_SW_CONF_SPI		0x4
>> +#define USI_V1_SW_CONF_UART		0x8
>> +#define USI_V1_SW_CONF_UART_I2C1	0xa
>> +#define USI_V1_SW_CONF_MASK		(USI_V1_SW_CONF_I2C0 | USI_V1_SW_CONF_I2C1 | \
>> +					 USI_V1_SW_CONF_I2C0_1 | USI_V1_SW_CONF_SPI | \
>> +					 USI_V1_SW_CONF_UART | USI_V1_SW_CONF_UART_I2C1)
>> +
>>  /* USIv2: System Register: SW_CONF register bits */
>>  #define USI_V2_SW_CONF_NONE	0x0
>>  #define USI_V2_SW_CONF_UART	BIT(0)
>> @@ -34,7 +46,8 @@
>>  #define USI_OPTION_CLKSTOP_ON	BIT(2)
>>  
>>  enum exynos_usi_ver {
>> -	USI_VER2 = 2,
>> +	USI_VER1 = 1,
> Is this assignment=1 actually now helping? Isn't it creating empty item
> in exynos_usi_modes array? Basically it wastes space in the array for
> no benefits.

I wanted to keep the USIv2 enum the same.

>
>> +	USI_VER2,
>>  };
>>  
>>  struct exynos_usi_variant {
>> @@ -66,19 +79,39 @@ struct exynos_usi_mode {
>>  	unsigned int val;		/* mode register value */
>>  };
>>  
>> -static const struct exynos_usi_mode exynos_usi_modes[] = {
>> -	[USI_V2_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
>> -	[USI_V2_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
>> -	[USI_V2_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
>> -	[USI_V2_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
>> +#define USI_MODES_MAX (USI_MODE_UART_I2C1 + 1)
>> +static const struct exynos_usi_mode exynos_usi_modes[][USI_MODES_MAX] = {
>> +	[USI_VER1] = {
>> +		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V1_SW_CONF_NONE },
>> +		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V1_SW_CONF_UART },
>> +		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V1_SW_CONF_SPI },
>> +		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V1_SW_CONF_I2C0 },
>> +		[USI_MODE_I2C1] =	{ .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
>> +		[USI_MODE_I2C0_1] =	{ .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
>> +		[USI_MODE_UART_I2C1] =	{ .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },
>> +	}, [USI_VER2] = {
>> +		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
>> +		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
>> +		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
>> +		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
>> +	},
>>  };
>>  
>>  static const char * const exynos850_usi_clk_names[] = { "pclk", "ipclk" };
>>  static const struct exynos_usi_variant exynos850_usi_data = {
>>  	.ver		= USI_VER2,
>>  	.sw_conf_mask	= USI_V2_SW_CONF_MASK,
>> -	.min_mode	= USI_V2_NONE,
>> -	.max_mode	= USI_V2_I2C,
>> +	.min_mode	= USI_MODE_NONE,
>> +	.max_mode	= USI_MODE_I2C,
>> +	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
>> +	.clk_names	= exynos850_usi_clk_names,
>> +};
>> +
>> +static const struct exynos_usi_variant exynos8895_usi_data = {
>> +	.ver		= USI_VER1,
>> +	.sw_conf_mask	= USI_V1_SW_CONF_MASK,
>> +	.min_mode	= USI_MODE_NONE,
>> +	.max_mode	= USI_MODE_UART_I2C1,
>>  	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
>>  	.clk_names	= exynos850_usi_clk_names,
>>  };
>> @@ -88,6 +121,10 @@ static const struct of_device_id exynos_usi_dt_match[] = {
>>  		.compatible = "samsung,exynos850-usi",
>>  		.data = &exynos850_usi_data,
>>  	},
>> +	{
> These two are in oone line.
>
>> +		.compatible = "samsung,exynos8895-usi",
>> +		.data = &exynos8895_usi_data,
>> +	},
>>  	{ } /* sentinel */
>>  };
>>  MODULE_DEVICE_TABLE(of, exynos_usi_dt_match);
>> @@ -109,14 +146,15 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
>>  	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
>>  		return -EINVAL;
>>  
>> -	val = exynos_usi_modes[mode].val;
>> +	val = exynos_usi_modes[usi->data->ver][mode].val;
>>  	ret = regmap_update_bits(usi->sysreg, usi->sw_conf,
>>  				 usi->data->sw_conf_mask, val);
>>  	if (ret)
>>  		return ret;
>>  
>>  	usi->mode = mode;
>> -	dev_dbg(usi->dev, "protocol: %s\n", exynos_usi_modes[usi->mode].name);
>> +	dev_dbg(usi->dev, "protocol: %s\n",
>> +		exynos_usi_modes[usi->data->ver][usi->mode].name);
>>  
>>  	return 0;
>>  }
>> @@ -160,6 +198,30 @@ static int exynos_usi_enable(const struct exynos_usi *usi)
>>  	return ret;
>>  }
>>  
>> +/**
>> + * exynos_usi_disable - Disable USI block
>> + * @usi: USI driver object
>> + *
>> + * USI IP-core needs the reset flag cleared in order to function. This
>> + * routine disables the USI block by setting the reset flag. It also disables
>> + * HWACG behavior. It should be performed on removal of the device.
>> + */
>> +static void exynos_usi_disable(const struct exynos_usi *usi)
>> +{
>> +	u32 val;
>> +
>> +	/* Make sure that we've stopped providing the clock to USI IP */
>> +	val = readl(usi->regs + USI_OPTION);
>> +	val &= ~USI_OPTION_CLKREQ_ON;
>> +	val |= ~USI_OPTION_CLKSTOP_ON;
>> +	writel(val, usi->regs + USI_OPTION);
>> +
>> +	/* Set USI block state to reset */
>> +	val = readl(usi->regs + USI_CON);
>> +	val |= USI_CON_RESET;
>> +	writel(val, usi->regs + USI_CON);
>> +}
>> +
>>  static int exynos_usi_configure(struct exynos_usi *usi)
>>  {
>>  	int ret;
>> @@ -169,9 +231,12 @@ static int exynos_usi_configure(struct exynos_usi *usi)
>>  		return ret;
>>  
>>  	if (usi->data->ver == USI_VER2)
>> -		return exynos_usi_enable(usi);
>> +		ret = exynos_usi_enable(usi);
>> +	else
>> +		ret = clk_bulk_prepare_enable(usi->data->num_clks,
>> +					      usi->clks);
>>  
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  static int exynos_usi_parse_dt(struct device_node *np, struct exynos_usi *usi)
>> @@ -253,10 +318,26 @@ static int exynos_usi_probe(struct platform_device *pdev)
>>  
>>  	ret = exynos_usi_configure(usi);
>>  	if (ret)
>> -		return ret;
>> +		goto fail_probe;
>>  
>>  	/* Make it possible to embed protocol nodes into USI np */
>>  	return of_platform_populate(np, NULL, NULL, dev);
> This also needs error handling.
>
>> +
>> +fail_probe:
> err_unconfigure:
>
>> +	if (usi->data->ver != USI_VER2)
>> +		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);
> Move it to its own callback exynos_usi_unconfigure(), so naming will be
> symmetric. The probe does not prepare clocks directly, so above code is
> not that readable. The most readable is to have symmetrics calls -
> configure+unconfigure (or whatever we name it).

Alright.

>
>> +
>> +	return ret;
>> +}
>> +
>> +static void exynos_usi_remove(struct platform_device *pdev)
>> +{
>> +	struct exynos_usi *usi = platform_get_drvdata(pdev);
>> +
>> +	if (usi->data->ver == USI_VER2)
>> +		exynos_usi_disable(usi);
> This is not related to the patch and should be separate patch, if at
> all.

Well I though that since didn't have any removal routine before it'd be good
to introduce that and not leave USIv2 with hwacg set.

Best regards,
Ivaylo

>> +	else
>> +		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);
> So the easiest would be to add devm reset action and then no need for
> goto-err handling and remove() callback.
>
> Best regards,
> Krzysztof
>

Re: [PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and exynos8895

[Krzysztof Kozlowski]

On 08/01/2025 10:17, Ivaylo Ivanov wrote:
> On 1/8/25 10:30, Krzysztof Kozlowski wrote:
>> On Tue, Jan 07, 2025 at 01:35:11PM +0200, Ivaylo Ivanov wrote:
>>> USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895) and
>>> provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
>>> SPI, UART, UART_HSI2C1).
>>>
>>> USIv1, unlike USIv2, doesn't have any known register map. Underlying
>>> protocols that it implements have no offset, like with Exynos850.
>>> Desired protocol can be chosen via SW_CONF register from System
>>> Register block of the same domain as USI.
>>>
>>> In order to select a particular protocol, the protocol has to be
>>> selected via the System Register. Unlike USIv2, there's no need for
>>> any setup before the given protocol becomes accessible apart from
>>> enabling the APB clock and the protocol operating clock.
>>>
>>> Modify the existing driver in order to allow USIv1 instances in
>>> Exynos8895 to probe and set their protocol. While we're at it,
>>> make use of the new mode constants in place of the old ones
>>> and add a removal routine.
>>>
>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>>> ---
>>>  drivers/soc/samsung/exynos-usi.c | 108 +++++++++++++++++++++++++++----
>>>  1 file changed, 95 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
>>> index 114352695..43c17b100 100644
>>> --- a/drivers/soc/samsung/exynos-usi.c
>>> +++ b/drivers/soc/samsung/exynos-usi.c
>>> @@ -16,6 +16,18 @@
>>>  
>>>  #include <dt-bindings/soc/samsung,exynos-usi.h>
>>>  
>>> +/* USIv1: System Register: SW_CONF register bits */
>>> +#define USI_V1_SW_CONF_NONE		0x0
>>> +#define USI_V1_SW_CONF_I2C0		0x1
>>> +#define USI_V1_SW_CONF_I2C1		0x2
>>> +#define USI_V1_SW_CONF_I2C0_1		0x3
>>> +#define USI_V1_SW_CONF_SPI		0x4
>>> +#define USI_V1_SW_CONF_UART		0x8
>>> +#define USI_V1_SW_CONF_UART_I2C1	0xa
>>> +#define USI_V1_SW_CONF_MASK		(USI_V1_SW_CONF_I2C0 | USI_V1_SW_CONF_I2C1 | \
>>> +					 USI_V1_SW_CONF_I2C0_1 | USI_V1_SW_CONF_SPI | \
>>> +					 USI_V1_SW_CONF_UART | USI_V1_SW_CONF_UART_I2C1)
>>> +
>>>  /* USIv2: System Register: SW_CONF register bits */
>>>  #define USI_V2_SW_CONF_NONE	0x0
>>>  #define USI_V2_SW_CONF_UART	BIT(0)
>>> @@ -34,7 +46,8 @@
>>>  #define USI_OPTION_CLKSTOP_ON	BIT(2)
>>>  
>>>  enum exynos_usi_ver {
>>> -	USI_VER2 = 2,
>>> +	USI_VER1 = 1,
>> Is this assignment=1 actually now helping? Isn't it creating empty item
>> in exynos_usi_modes array? Basically it wastes space in the array for
>> no benefits.
> 
> I wanted to keep the USIv2 enum the same.

Is there any need for keeping it the same?

> 
>>
>>> +	USI_VER2,
>>>  };


...

>>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void exynos_usi_remove(struct platform_device *pdev)
>>> +{
>>> +	struct exynos_usi *usi = platform_get_drvdata(pdev);
>>> +
>>> +	if (usi->data->ver == USI_VER2)
>>> +		exynos_usi_disable(usi);
>> This is not related to the patch and should be separate patch, if at
>> all.
> 
> Well I though that since didn't have any removal routine before it'd be good
> to introduce that and not leave USIv2 with hwacg set.

Sure, but separate commit, please. Can be preceeding the USIv1 support.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and exynos8895

[Ivaylo Ivanov]

On 1/8/25 11:26, Krzysztof Kozlowski wrote:
> On 08/01/2025 10:17, Ivaylo Ivanov wrote:
>> On 1/8/25 10:30, Krzysztof Kozlowski wrote:
>>> On Tue, Jan 07, 2025 at 01:35:11PM +0200, Ivaylo Ivanov wrote:
>>>> USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895) and
>>>> provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
>>>> SPI, UART, UART_HSI2C1).
>>>>
>>>> USIv1, unlike USIv2, doesn't have any known register map. Underlying
>>>> protocols that it implements have no offset, like with Exynos850.
>>>> Desired protocol can be chosen via SW_CONF register from System
>>>> Register block of the same domain as USI.
>>>>
>>>> In order to select a particular protocol, the protocol has to be
>>>> selected via the System Register. Unlike USIv2, there's no need for
>>>> any setup before the given protocol becomes accessible apart from
>>>> enabling the APB clock and the protocol operating clock.
>>>>
>>>> Modify the existing driver in order to allow USIv1 instances in
>>>> Exynos8895 to probe and set their protocol. While we're at it,
>>>> make use of the new mode constants in place of the old ones
>>>> and add a removal routine.
>>>>
>>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>>>> ---
>>>>  drivers/soc/samsung/exynos-usi.c | 108 +++++++++++++++++++++++++++----
>>>>  1 file changed, 95 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
>>>> index 114352695..43c17b100 100644
>>>> --- a/drivers/soc/samsung/exynos-usi.c
>>>> +++ b/drivers/soc/samsung/exynos-usi.c
>>>> @@ -16,6 +16,18 @@
>>>>  
>>>>  #include <dt-bindings/soc/samsung,exynos-usi.h>
>>>>  
>>>> +/* USIv1: System Register: SW_CONF register bits */
>>>> +#define USI_V1_SW_CONF_NONE		0x0
>>>> +#define USI_V1_SW_CONF_I2C0		0x1
>>>> +#define USI_V1_SW_CONF_I2C1		0x2
>>>> +#define USI_V1_SW_CONF_I2C0_1		0x3
>>>> +#define USI_V1_SW_CONF_SPI		0x4
>>>> +#define USI_V1_SW_CONF_UART		0x8
>>>> +#define USI_V1_SW_CONF_UART_I2C1	0xa
>>>> +#define USI_V1_SW_CONF_MASK		(USI_V1_SW_CONF_I2C0 | USI_V1_SW_CONF_I2C1 | \
>>>> +					 USI_V1_SW_CONF_I2C0_1 | USI_V1_SW_CONF_SPI | \
>>>> +					 USI_V1_SW_CONF_UART | USI_V1_SW_CONF_UART_I2C1)
>>>> +
>>>>  /* USIv2: System Register: SW_CONF register bits */
>>>>  #define USI_V2_SW_CONF_NONE	0x0
>>>>  #define USI_V2_SW_CONF_UART	BIT(0)
>>>> @@ -34,7 +46,8 @@
>>>>  #define USI_OPTION_CLKSTOP_ON	BIT(2)
>>>>  
>>>>  enum exynos_usi_ver {
>>>> -	USI_VER2 = 2,
>>>> +	USI_VER1 = 1,
>>> Is this assignment=1 actually now helping? Isn't it creating empty item
>>> in exynos_usi_modes array? Basically it wastes space in the array for
>>> no benefits.
>> I wanted to keep the USIv2 enum the same.
> Is there any need for keeping it the same?

No, not really.

>
>>>> +	USI_VER2,
>>>>  };
>
> ...
>
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void exynos_usi_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct exynos_usi *usi = platform_get_drvdata(pdev);
>>>> +
>>>> +	if (usi->data->ver == USI_VER2)
>>>> +		exynos_usi_disable(usi);
>>> This is not related to the patch and should be separate patch, if at
>>> all.
>> Well I though that since didn't have any removal routine before it'd be good
>> to introduce that and not leave USIv2 with hwacg set.
> Sure, but separate commit, please. Can be preceeding the USIv1 support.

What about right after the USIv1 support? It would be less messy in my
opinion.

>
> Best regards,
> Krzysztof

Re: [PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and exynos8895

[Krzysztof Kozlowski]

On 08/01/2025 10:45, Ivaylo Ivanov wrote:
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static void exynos_usi_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct exynos_usi *usi = platform_get_drvdata(pdev);
>>>>> +
>>>>> +	if (usi->data->ver == USI_VER2)
>>>>> +		exynos_usi_disable(usi);
>>>> This is not related to the patch and should be separate patch, if at
>>>> all.
>>> Well I though that since didn't have any removal routine before it'd be good
>>> to introduce that and not leave USIv2 with hwacg set.
>> Sure, but separate commit, please. Can be preceeding the USIv1 support.
> 
> What about right after the USIv1 support? It would be less messy in my
> opinion.
Fixes should be before new features and why messy? You add devm cleanup
handler for unconfigure and in next commit you grow it with clk disable
for USIv1?

Best regards,
Krzysztof