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

[PATCH v1 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] has been merged before it.

Best regards,
Ivaylo

[1]: https://lore.kernel.org/all/20241222145257.31451-1-krzysztof.kozlowski@linaro.org/

Ivaylo Ivanov (3):
  dt-bindings: soc: samsung: exynos-sysreg: add sysreg compatibles for
    exynos8895
  dt-bindings: soc: samsung: usi: add USIv1 and samsung,exynos8895-usi
  soc: samsung: usi: implement support for USIv1

 .../bindings/soc/samsung/exynos-usi.yaml      | 22 +++++--
 .../soc/samsung/samsung,exynos-sysreg.yaml    |  8 +++
 drivers/soc/samsung/exynos-usi.c              | 58 +++++++++++++++++--
 include/dt-bindings/soc/samsung,exynos-usi.h  |  8 +++
 4 files changed, 87 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH v1 1/3] dt-bindings: soc: samsung: exynos-sysreg: add sysreg compatibles for exynos8895

[Ivaylo Ivanov]

Exynos8895 has four different SYSREG controllers, add dedicated
compatibles for them to the documentation. They also require clocks.

Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
---
 .../bindings/soc/samsung/samsung,exynos-sysreg.yaml       | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/samsung/samsung,exynos-sysreg.yaml b/Documentation/devicetree/bindings/soc/samsung/samsung,exynos-sysreg.yaml
index 3ca220582..a75aef240 100644
--- a/Documentation/devicetree/bindings/soc/samsung/samsung,exynos-sysreg.yaml
+++ b/Documentation/devicetree/bindings/soc/samsung/samsung,exynos-sysreg.yaml
@@ -21,6 +21,10 @@ properties:
               - samsung,exynos3-sysreg
               - samsung,exynos4-sysreg
               - samsung,exynos5-sysreg
+              - samsung,exynos8895-fsys0-sysreg
+              - samsung,exynos8895-fsys1-sysreg
+              - samsung,exynos8895-peric0-sysreg
+              - samsung,exynos8895-peric1-sysreg
               - samsung,exynosautov920-peric0-sysreg
               - samsung,exynosautov920-peric1-sysreg
               - tesla,fsd-cam-sysreg
@@ -79,6 +83,10 @@ allOf:
               - samsung,exynos850-cmgp-sysreg
               - samsung,exynos850-peri-sysreg
               - samsung,exynos850-sysreg
+              - samsung,exynos8895-fsys0-sysreg
+              - samsung,exynos8895-fsys1-sysreg
+              - samsung,exynos8895-peric0-sysreg
+              - samsung,exynos8895-peric1-sysreg
     then:
       required:
         - clocks
-- 
2.43.0

Re: [PATCH v1 1/3] dt-bindings: soc: samsung: exynos-sysreg: add sysreg compatibles for exynos8895

[Krzysztof Kozlowski]

On Thu, Jan 02, 2025 at 10:40:13PM +0200, Ivaylo Ivanov wrote:
> Exynos8895 has four different SYSREG controllers, add dedicated
> compatibles for them to the documentation. They also require clocks.
> 
> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
> ---
>  .../bindings/soc/samsung/samsung,exynos-sysreg.yaml       | 8 ++++++++
>  1 file changed, 8 insertions(+)

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

I closed my tree for v6.14-rc1, so I will take the patches after merge
window.

Best regards,
Krzysztof

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

[Ivaylo Ivanov]

Add constants for choosing USIv1 configuration mode 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.

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

diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
index f80fcbc31..50db63a8a 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
@@ -106,6 +117,7 @@ if:
       contains:
         enum:
           - samsung,exynos850-usi
+          - samsung,exynos8895-usi
 
 then:
   properties:
diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
index a01af169d..5fbeb426b 100644
--- a/include/dt-bindings/soc/samsung,exynos-usi.h
+++ b/include/dt-bindings/soc/samsung,exynos-usi.h
@@ -9,6 +9,14 @@
 #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
 #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
 
+#define USI_V1_NONE		0
+#define USI_V1_I2C0		1
+#define USI_V1_I2C1		2
+#define USI_V1_I2C0_1		3
+#define USI_V1_SPI		4
+#define USI_V1_UART		5
+#define USI_V1_UART_I2C1	6
+
 #define USI_V2_NONE		0
 #define USI_V2_UART		1
 #define USI_V2_SPI		2
-- 
2.43.0

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

[Krzysztof Kozlowski]

On Thu, Jan 02, 2025 at 10:40:14PM +0200, Ivaylo Ivanov wrote:
> Add constants for choosing USIv1 configuration mode 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.
> 
> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
> ---
>  .../bindings/soc/samsung/exynos-usi.yaml      | 22 ++++++++++++++-----
>  include/dt-bindings/soc/samsung,exynos-usi.h  |  8 +++++++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
> index f80fcbc31..50db63a8a 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
> @@ -106,6 +117,7 @@ if:
>        contains:
>          enum:
>            - samsung,exynos850-usi
> +          - samsung,exynos8895-usi
>  
>  then:
>    properties:
> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
> index a01af169d..5fbeb426b 100644
> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
> @@ -9,6 +9,14 @@
>  #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
>  #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
>  
> +#define USI_V1_NONE		0
> +#define USI_V1_I2C0		1
> +#define USI_V1_I2C1		2
> +#define USI_V1_I2C0_1		3
> +#define USI_V1_SPI		4
> +#define USI_V1_UART		5
> +#define USI_V1_UART_I2C1	6

Conflicting values with USI_v2. This is the same property and the same
driver, so I expect the values to be unified.

Rename (copy) USI_V2_ to USI_ and then just add yours at the end.
Binding then in allOf:if:then should restrict with enum which values
make sense for which model.

Best regards,
Krzysztof

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

[Ivaylo Ivanov]

On 1/3/25 10:21, Krzysztof Kozlowski wrote:
> On Thu, Jan 02, 2025 at 10:40:14PM +0200, Ivaylo Ivanov wrote:
>> Add constants for choosing USIv1 configuration mode 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.
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> ---
>>  .../bindings/soc/samsung/exynos-usi.yaml      | 22 ++++++++++++++-----
>>  include/dt-bindings/soc/samsung,exynos-usi.h  |  8 +++++++
>>  2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
>> index f80fcbc31..50db63a8a 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
>> @@ -106,6 +117,7 @@ if:
>>        contains:
>>          enum:
>>            - samsung,exynos850-usi
>> +          - samsung,exynos8895-usi
>>  
>>  then:
>>    properties:
>> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
>> index a01af169d..5fbeb426b 100644
>> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
>> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
>> @@ -9,6 +9,14 @@
>>  #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
>>  #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
>>  
>> +#define USI_V1_NONE		0
>> +#define USI_V1_I2C0		1
>> +#define USI_V1_I2C1		2
>> +#define USI_V1_I2C0_1		3
>> +#define USI_V1_SPI		4
>> +#define USI_V1_UART		5
>> +#define USI_V1_UART_I2C1	6
> Conflicting values with USI_v2. This is the same property and the same
> driver, so I expect the values to be unified.
>
> Rename (copy) USI_V2_ to USI_ and then just add yours at the end.

I see, that approach could work too. Although I'd much rather keep the definition
names the same as before for USI_V2.

Best regards,
Ivaylo

> Binding then in allOf:if:then should restrict with enum which values
> make sense for which model.
>
> Best regards,
> Krzysztof
>

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

[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.

Modify the existing driver in order to allow USIv1 to probe and set
its protocol.

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

diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
index 114352695..29c3770e3 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,6 +79,16 @@ struct exynos_usi_mode {
 	unsigned int val;		/* mode register value */
 };
 
+static const struct exynos_usi_mode exynos_usi_v1_modes[] = {
+	[USI_V1_NONE]		= { .name = "none", .val = USI_V1_SW_CONF_NONE },
+	[USI_V1_I2C0]		= { .name = "i2c0", .val = USI_V1_SW_CONF_I2C0 },
+	[USI_V1_I2C1]		= { .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
+	[USI_V1_I2C0_1]		= { .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
+	[USI_V1_SPI]		= { .name = "spi", .val = USI_V1_SW_CONF_SPI },
+	[USI_V1_UART]		= { .name = "uart", .val = USI_V1_SW_CONF_UART },
+	[USI_V1_UART_I2C1]	= { .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },
+};
+
 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 },
@@ -83,11 +106,24 @@ static const struct exynos_usi_variant exynos850_usi_data = {
 	.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_V1_NONE,
+	.max_mode	= USI_V1_UART_I2C1,
+	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
+	.clk_names	= exynos850_usi_clk_names,
+};
+
 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);
@@ -105,18 +141,32 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
 {
 	unsigned int val;
 	int ret;
+	const char *name;
 
+	usi->mode = mode;
 	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
 		return -EINVAL;
 
-	val = exynos_usi_modes[mode].val;
+	switch (usi->data->ver) {
+	case USI_VER1:
+		val = exynos_usi_v1_modes[mode].val;
+		name = exynos_usi_v1_modes[usi->mode].name;
+		break;
+	case USI_VER2:
+		val = exynos_usi_modes[mode].val;
+		name = exynos_usi_modes[usi->mode].name;
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	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", name);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v1 3/3] soc: samsung: usi: implement support for USIv1

[Krzysztof Kozlowski]

On Thu, Jan 02, 2025 at 10:40:15PM +0200, Ivaylo Ivanov wrote:
>  /* 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,6 +79,16 @@ struct exynos_usi_mode {
>  	unsigned int val;		/* mode register value */
>  };
>  
> +static const struct exynos_usi_mode exynos_usi_v1_modes[] = {
> +	[USI_V1_NONE]		= { .name = "none", .val = USI_V1_SW_CONF_NONE },
> +	[USI_V1_I2C0]		= { .name = "i2c0", .val = USI_V1_SW_CONF_I2C0 },
> +	[USI_V1_I2C1]		= { .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
> +	[USI_V1_I2C0_1]		= { .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
> +	[USI_V1_SPI]		= { .name = "spi", .val = USI_V1_SW_CONF_SPI },
> +	[USI_V1_UART]		= { .name = "uart", .val = USI_V1_SW_CONF_UART },
> +	[USI_V1_UART_I2C1]	= { .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },

Now I see why you duplicated the IDs... With my approach your code here
is even simpler. Allows to drop USI_VER1 as well.


> +};
> +
>  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 },
> @@ -83,11 +106,24 @@ static const struct exynos_usi_variant exynos850_usi_data = {
>  	.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_V1_NONE,
> +	.max_mode	= USI_V1_UART_I2C1,
> +	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
> +	.clk_names	= exynos850_usi_clk_names,
> +};
> +
>  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);
> @@ -105,18 +141,32 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
>  {
>  	unsigned int val;
>  	int ret;
> +	const char *name;
>  
> +	usi->mode = mode;
>  	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
>  		return -EINVAL;
>  
> -	val = exynos_usi_modes[mode].val;
> +	switch (usi->data->ver) {
> +	case USI_VER1:
> +		val = exynos_usi_v1_modes[mode].val;
> +		name = exynos_usi_v1_modes[usi->mode].name;
> +		break;
> +	case USI_VER2:
> +		val = exynos_usi_modes[mode].val;
> +		name = exynos_usi_modes[usi->mode].name;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>  	ret = regmap_update_bits(usi->sysreg, usi->sw_conf,
>  				 usi->data->sw_conf_mask, val);
> +

No, why? Drop.

Best regards,
Krzysztof

Re: [PATCH v1 3/3] soc: samsung: usi: implement support for USIv1

[Ivaylo Ivanov]

On 1/3/25 10:29, Krzysztof Kozlowski wrote:
> On Thu, Jan 02, 2025 at 10:40:15PM +0200, Ivaylo Ivanov wrote:
>>  /* 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,6 +79,16 @@ struct exynos_usi_mode {
>>  	unsigned int val;		/* mode register value */
>>  };
>>  
>> +static const struct exynos_usi_mode exynos_usi_v1_modes[] = {
>> +	[USI_V1_NONE]		= { .name = "none", .val = USI_V1_SW_CONF_NONE },
>> +	[USI_V1_I2C0]		= { .name = "i2c0", .val = USI_V1_SW_CONF_I2C0 },
>> +	[USI_V1_I2C1]		= { .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
>> +	[USI_V1_I2C0_1]		= { .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
>> +	[USI_V1_SPI]		= { .name = "spi", .val = USI_V1_SW_CONF_SPI },
>> +	[USI_V1_UART]		= { .name = "uart", .val = USI_V1_SW_CONF_UART },
>> +	[USI_V1_UART_I2C1]	= { .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },
> Now I see why you duplicated the IDs... With my approach your code here
> is even simpler. Allows to drop USI_VER1 as well.

We can't really drop USI_VER1, as we'll fall into USIV2-specific code, like so:
if (usi->data->ver == USI_VER2) return exynos_usi_enable(usi);

Thanks for the feedback!

Best regards,
Ivaylo

>
>
>> +};
>> +
>>  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 },
>> @@ -83,11 +106,24 @@ static const struct exynos_usi_variant exynos850_usi_data = {
>>  	.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_V1_NONE,
>> +	.max_mode	= USI_V1_UART_I2C1,
>> +	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
>> +	.clk_names	= exynos850_usi_clk_names,
>> +};
>> +
>>  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);
>> @@ -105,18 +141,32 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
>>  {
>>  	unsigned int val;
>>  	int ret;
>> +	const char *name;
>>  
>> +	usi->mode = mode;
>>  	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
>>  		return -EINVAL;
>>  
>> -	val = exynos_usi_modes[mode].val;
>> +	switch (usi->data->ver) {
>> +	case USI_VER1:
>> +		val = exynos_usi_v1_modes[mode].val;
>> +		name = exynos_usi_v1_modes[usi->mode].name;
>> +		break;
>> +	case USI_VER2:
>> +		val = exynos_usi_modes[mode].val;
>> +		name = exynos_usi_modes[usi->mode].name;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>>  	ret = regmap_update_bits(usi->sysreg, usi->sw_conf,
>>  				 usi->data->sw_conf_mask, val);
>> +
> No, why? Drop.
>
> Best regards,
> Krzysztof
>

Re: [PATCH v1 3/3] soc: samsung: usi: implement support for USIv1

[Krzysztof Kozlowski]

On 04/01/2025 10:36, Ivaylo Ivanov wrote:
>>>  struct exynos_usi_variant {
>>> @@ -66,6 +79,16 @@ struct exynos_usi_mode {
>>>  	unsigned int val;		/* mode register value */
>>>  };
>>>  
>>> +static const struct exynos_usi_mode exynos_usi_v1_modes[] = {
>>> +	[USI_V1_NONE]		= { .name = "none", .val = USI_V1_SW_CONF_NONE },
>>> +	[USI_V1_I2C0]		= { .name = "i2c0", .val = USI_V1_SW_CONF_I2C0 },
>>> +	[USI_V1_I2C1]		= { .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
>>> +	[USI_V1_I2C0_1]		= { .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
>>> +	[USI_V1_SPI]		= { .name = "spi", .val = USI_V1_SW_CONF_SPI },
>>> +	[USI_V1_UART]		= { .name = "uart", .val = USI_V1_SW_CONF_UART },
>>> +	[USI_V1_UART_I2C1]	= { .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },
>> Now I see why you duplicated the IDs... With my approach your code here
>> is even simpler. Allows to drop USI_VER1 as well.
> 
> We can't really drop USI_VER1, as we'll fall into USIV2-specific code, like so:
> if (usi->data->ver == USI_VER2) return exynos_usi_enable(usi);

Yeah, indeed. You still would have just one exynos_usi_mode table.


Best regards,
Krzysztof