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

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

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/

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: 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      | 55 +++++++++++++++----
 .../soc/samsung/samsung,exynos-sysreg.yaml    |  8 +++
 drivers/soc/samsung/exynos-usi.c              | 42 +++++++++++++-
 include/dt-bindings/soc/samsung,exynos-usi.h  |  7 +++
 4 files changed, 97 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v2 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../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

[PATCH v2 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      | 55 +++++++++++++++----
 include/dt-bindings/soc/samsung,exynos-usi.h  |  7 +++
 2 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml
index 5b046932f..6e32daa45 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,6 @@ properties:
 
   samsung,mode:
     $ref: /schemas/types.yaml#/definitions/uint32
-    enum: [0, 1, 2, 3]
     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,18 +111,42 @@ required:
   - samsung,sysreg
   - samsung,mode
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - samsung,exynos850-usi
+    then:
+      properties:
+        reg:
+          maxItems: 1
+
+        samsung,mode:
+          enum: [0, 1, 2, 3]
+
+      required:
+        - reg
+
+    else:
+      properties:
+        reg: false
+        samsung,clkreq-on: false
+
+        samsung,mode:
+          enum: [4, 5, 6, 7, 8, 9, 10]
+
 if:
   properties:
     compatible:
       contains:
         enum:
           - samsung,exynos850-usi
+          - samsung,exynos8895-usi
 
 then:
   properties:
-    reg:
-      maxItems: 1
-
     clocks:
       items:
         - description: Bus (APB) clock
@@ -122,16 +156,13 @@ then:
       maxItems: 2
 
   required:
-    - reg
     - clocks
     - clock-names
 
 else:
   properties:
-    reg: false
     clocks: false
     clock-names: false
-    samsung,clkreq-on: false
 
 additionalProperties: false
 
diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
index a01af169d..4c077c9a8 100644
--- a/include/dt-bindings/soc/samsung,exynos-usi.h
+++ b/include/dt-bindings/soc/samsung,exynos-usi.h
@@ -13,5 +13,12 @@
 #define USI_V2_UART		1
 #define USI_V2_SPI		2
 #define USI_V2_I2C		3
+#define USI_V1_NONE		4
+#define USI_V1_I2C0		5
+#define USI_V1_I2C1		6
+#define USI_V1_I2C0_1		7
+#define USI_V1_SPI		8
+#define USI_V1_UART		9
+#define USI_V1_UART_I2C1	10
 
 #endif /* __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H */
-- 
2.43.0

[PATCH v2 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 apart from
enabling the APB clock and the protocol operating clock.

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 | 42 +++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
index 114352695..e57d2c274 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 {
@@ -71,6 +84,13 @@ static const struct exynos_usi_mode exynos_usi_modes[] = {
 	[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 },
+	[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 char * const exynos850_usi_clk_names[] = { "pclk", "ipclk" };
@@ -83,11 +103,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);
@@ -169,9 +202,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)
-- 
2.43.0

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

[Krzysztof Kozlowski]

On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote:
>  
>    reg:
>      maxItems: 1
> @@ -64,7 +75,6 @@ properties:
>  
>    samsung,mode:
>      $ref: /schemas/types.yaml#/definitions/uint32
> -    enum: [0, 1, 2, 3]

Widest constraints stay here, so minimum/maximum.

>      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,18 +111,42 @@ required:
>    - samsung,sysreg
>    - samsung,mode
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - samsung,exynos850-usi
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 1
> +
> +        samsung,mode:
> +          enum: [0, 1, 2, 3]
> +
> +      required:
> +        - reg
> +
> +    else:
> +      properties:
> +        reg: false
> +        samsung,clkreq-on: false
> +
> +        samsung,mode:
> +          enum: [4, 5, 6, 7, 8, 9, 10]

Is it really true? Previously for example GS101 had values 0-3, now you
claim has values 4-10, so an ABI change without explanation.

> +
>  if:

Missing allOf:

>    properties:
>      compatible:
>        contains:
>          enum:
>            - samsung,exynos850-usi
> +          - samsung,exynos8895-usi

Effect is not readable and not correct. You have two if:then:else.
Usually it is easier to just have separate if: for each group of
variants and define/constrain complete for such group within its if:.

>  
>  then:
>    properties:
> -    reg:
> -      maxItems: 1
> -
>      clocks:
>        items:
>          - description: Bus (APB) clock
> @@ -122,16 +156,13 @@ then:
>        maxItems: 2
>  
>    required:
> -    - reg
>      - clocks
>      - clock-names
>  
>  else:
>    properties:
> -    reg: false
>      clocks: false
>      clock-names: false
> -    samsung,clkreq-on: false
>  
>  additionalProperties: false
>  
> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
> index a01af169d..4c077c9a8 100644
> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
> @@ -13,5 +13,12 @@
>  #define USI_V2_UART		1
>  #define USI_V2_SPI		2
>  #define USI_V2_I2C		3
> +#define USI_V1_NONE		4

Drop, it is already there.

> +#define USI_V1_I2C0		5
> +#define USI_V1_I2C1		6
> +#define USI_V1_I2C0_1		7
> +#define USI_V1_SPI		8

Drop

> +#define USI_V1_UART		9

Drop

> +#define USI_V1_UART_I2C1	10


Best regards,
Krzysztof

Re: (subset) [PATCH v2 1/3] dt-bindings: soc: samsung: exynos-sysreg: add sysreg compatibles for exynos8895

[Krzysztof Kozlowski]

On Sat, 04 Jan 2025 18:29:13 +0200, Ivaylo Ivanov wrote:
> Exynos8895 has four different SYSREG controllers, add dedicated
> compatibles for them to the documentation. They also require clocks.
> 
> 

Applied, thanks!

[1/3] dt-bindings: soc: samsung: exynos-sysreg: add sysreg compatibles for exynos8895
      https://git.kernel.org/krzk/linux/c/38405d3825d883b9e6ae680c14b530f79709533e

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

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

[Ivaylo Ivanov]

On 1/5/25 11:18, Krzysztof Kozlowski wrote:
> On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote:
>>  
>>    reg:
>>      maxItems: 1
>> @@ -64,7 +75,6 @@ properties:
>>  
>>    samsung,mode:
>>      $ref: /schemas/types.yaml#/definitions/uint32
>> -    enum: [0, 1, 2, 3]
> Widest constraints stay here, so minimum/maximum.

Why? If we are going to add new enum values specific for each USI version,
isn't it better to selectively constrain them in the binding?

>
>>      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,18 +111,42 @@ required:
>>    - samsung,sysreg
>>    - samsung,mode
>>  
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - samsung,exynos850-usi
>> +    then:
>> +      properties:
>> +        reg:
>> +          maxItems: 1
>> +
>> +        samsung,mode:
>> +          enum: [0, 1, 2, 3]
>> +
>> +      required:
>> +        - reg
>> +
>> +    else:
>> +      properties:
>> +        reg: false
>> +        samsung,clkreq-on: false
>> +
>> +        samsung,mode:
>> +          enum: [4, 5, 6, 7, 8, 9, 10]
> Is it really true? Previously for example GS101 had values 0-3, now you
> claim has values 4-10, so an ABI change without explanation.

How is it unexplained? Citing the commit message:
"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."

USIv1 and v2 write different values to the SW_CONF register. For example:

#define USI_V1_SW_CONF_UART        0x8
#define USI_V2_SW_CONF_UART    BIT(0)

..
 [USI_V2_UART] =    { .name = "uart", .val = USI_V2_SW_CONF_UART },
 [USI_V1_UART] =    { .name = "uart", .val = USI_V1_SW_CONF_UART },
..

Hence the decision to have separate constants for different USI versions,
which in my opinion is cleaner than meshing the enums together and
choosing what to use with IFs in the driver code.

>
>> +
>>  if:
> Missing allOf:
>
>>    properties:
>>      compatible:
>>        contains:
>>          enum:
>>            - samsung,exynos850-usi
>> +          - samsung,exynos8895-usi
> Effect is not readable and not correct. You have two if:then:else.
> Usually it is easier to just have separate if: for each group of
> variants and define/constrain complete for such group within its if:.
>
>>  
>>  then:
>>    properties:
>> -    reg:
>> -      maxItems: 1
>> -
>>      clocks:
>>        items:
>>          - description: Bus (APB) clock
>> @@ -122,16 +156,13 @@ then:
>>        maxItems: 2
>>  
>>    required:
>> -    - reg
>>      - clocks
>>      - clock-names
>>  
>>  else:
>>    properties:
>> -    reg: false
>>      clocks: false
>>      clock-names: false
>> -    samsung,clkreq-on: false
>>  
>>  additionalProperties: false
>>  
>> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
>> index a01af169d..4c077c9a8 100644
>> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
>> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
>> @@ -13,5 +13,12 @@
>>  #define USI_V2_UART		1
>>  #define USI_V2_SPI		2
>>  #define USI_V2_I2C		3
>> +#define USI_V1_NONE		4
> Drop, it is already there.
>
>> +#define USI_V1_I2C0		5
>> +#define USI_V1_I2C1		6
>> +#define USI_V1_I2C0_1		7
>> +#define USI_V1_SPI		8
> Drop
>
>> +#define USI_V1_UART		9
> Drop

How so? These bring different configuration values. Could you specify how
exactly you want me to implement these in the driver?

Thanks for the feedback!

Best regards,
Ivaylo

>
>> +#define USI_V1_UART_I2C1	10
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 05/01/2025 10:51, Ivaylo Ivanov wrote:
> On 1/5/25 11:18, Krzysztof Kozlowski wrote:
>> On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote:
>>>  
>>>    reg:
>>>      maxItems: 1
>>> @@ -64,7 +75,6 @@ properties:
>>>  
>>>    samsung,mode:
>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>> -    enum: [0, 1, 2, 3]
>> Widest constraints stay here, so minimum/maximum.
> 
> Why?

Because that's the coding style and that's how you define the field,
considering you might miss a variant in multiple if:then: .


> If we are going to add new enum values specific for each USI version,
> isn't it better to selectively constrain them in the binding?

You are supposed to constrained them.

Again: widest constrains always stay in top level property. This applies
to all bindings, all fields. Repeated multiple times, so here is
standard example:

https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127


> 
>>
>>>      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,18 +111,42 @@ required:
>>>    - samsung,sysreg
>>>    - samsung,mode
>>>  
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - samsung,exynos850-usi
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          maxItems: 1
>>> +
>>> +        samsung,mode:
>>> +          enum: [0, 1, 2, 3]
>>> +
>>> +      required:
>>> +        - reg
>>> +
>>> +    else:
>>> +      properties:
>>> +        reg: false
>>> +        samsung,clkreq-on: false
>>> +
>>> +        samsung,mode:
>>> +          enum: [4, 5, 6, 7, 8, 9, 10]
>> Is it really true? Previously for example GS101 had values 0-3, now you
>> claim has values 4-10, so an ABI change without explanation.
> 
> How is it unexplained? Citing the commit message:
> "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."
> 

I don't see reference here about GS101 and others.

> USIv1 and v2 write different values to the SW_CONF register. For example:
> 
> #define USI_V1_SW_CONF_UART        0x8
> #define USI_V2_SW_CONF_UART    BIT(0)
> 
> ..
>  [USI_V2_UART] =    { .name = "uart", .val = USI_V2_SW_CONF_UART },
>  [USI_V1_UART] =    { .name = "uart", .val = USI_V1_SW_CONF_UART },
> ..
> 
> Hence the decision to have separate constants for different USI versions,
> which in my opinion is cleaner than meshing the enums together and
> choosing what to use with IFs in the driver code.

This does not answer at all why GS101 receives now different values than
before.

Explain why you are changing ABI.

> 
>>
>>> +
>>>  if:
>> Missing allOf:
>>
>>>    properties:
>>>      compatible:
>>>        contains:
>>>          enum:
>>>            - samsung,exynos850-usi
>>> +          - samsung,exynos8895-usi
>> Effect is not readable and not correct. You have two if:then:else.
>> Usually it is easier to just have separate if: for each group of
>> variants and define/constrain complete for such group within its if:.
>>
>>>  
>>>  then:
>>>    properties:
>>> -    reg:
>>> -      maxItems: 1
>>> -
>>>      clocks:
>>>        items:
>>>          - description: Bus (APB) clock
>>> @@ -122,16 +156,13 @@ then:
>>>        maxItems: 2
>>>  
>>>    required:
>>> -    - reg
>>>      - clocks
>>>      - clock-names
>>>  
>>>  else:
>>>    properties:
>>> -    reg: false
>>>      clocks: false
>>>      clock-names: false
>>> -    samsung,clkreq-on: false
>>>  
>>>  additionalProperties: false
>>>  
>>> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
>>> index a01af169d..4c077c9a8 100644
>>> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
>>> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
>>> @@ -13,5 +13,12 @@
>>>  #define USI_V2_UART		1
>>>  #define USI_V2_SPI		2
>>>  #define USI_V2_I2C		3
>>> +#define USI_V1_NONE		4
>> Drop, it is already there.
>>
>>> +#define USI_V1_I2C0		5
>>> +#define USI_V1_I2C1		6
>>> +#define USI_V1_I2C0_1		7
>>> +#define USI_V1_SPI		8
>> Drop
>>
>>> +#define USI_V1_UART		9
>> Drop
> 
> How so? These bring different configuration values. Could you specify how
> exactly you want me to implement these in the driver?

Heh, so the binding was made poorly for the driver and driver was
developed in a matching way, so now this became an argument. Binding and
drivers are independent, so whatever argument was in the driver does not
matter really.

What I don't understand is downstream for USIv1 and USIv2 has it correct
- proper string values without mentioning any version. So, surprisingly
proper downstream binding, really rare case, was converted to something
tied to current driver implementation.

You have only one sort of property - the mode how you configure the USI
engine. The mode can be: I2C, SPI, I2C0, I2C1 for special cases with two
I2C etc.

The mode is not "USI_V1_I2C" because it is redundant. USI V1 cannot be
USI V2. You cannot tell USI to be v1 or v2. It is either v1 or v2. Only
one of these, thus encoding this information in the binding is meaningless.

I even mentioned this in original binding review:
"so please drop everywhere v2 (bindings, symbols, Kconfig,
functions) except the compatible."
Well, then I missed to check on that, so now this mess has to be fixed.

Best regards,
Krzysztof

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

[Ivaylo Ivanov]

On 1/5/25 12:39, Krzysztof Kozlowski wrote:
> On 05/01/2025 10:51, Ivaylo Ivanov wrote:
>> On 1/5/25 11:18, Krzysztof Kozlowski wrote:
>>> On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote:
>>>>  
>>>>    reg:
>>>>      maxItems: 1
>>>> @@ -64,7 +75,6 @@ properties:
>>>>  
>>>>    samsung,mode:
>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>> -    enum: [0, 1, 2, 3]
>>> Widest constraints stay here, so minimum/maximum.
>> Why?
> Because that's the coding style and that's how you define the field,
> considering you might miss a variant in multiple if:then: .
>
>
>> If we are going to add new enum values specific for each USI version,
>> isn't it better to selectively constrain them in the binding?
> You are supposed to constrained them.
>
> Again: widest constrains always stay in top level property. This applies
> to all bindings, all fields. Repeated multiple times, so here is
> standard example:
>
> https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127

Ah, I see now. Will get that fixed.

>
>
>>>>      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,18 +111,42 @@ required:
>>>>    - samsung,sysreg
>>>>    - samsung,mode
>>>>  
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - samsung,exynos850-usi
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          maxItems: 1
>>>> +
>>>> +        samsung,mode:
>>>> +          enum: [0, 1, 2, 3]
>>>> +
>>>> +      required:
>>>> +        - reg
>>>> +
>>>> +    else:
>>>> +      properties:
>>>> +        reg: false
>>>> +        samsung,clkreq-on: false
>>>> +
>>>> +        samsung,mode:
>>>> +          enum: [4, 5, 6, 7, 8, 9, 10]
>>> Is it really true? Previously for example GS101 had values 0-3, now you
>>> claim has values 4-10, so an ABI change without explanation.
>> How is it unexplained? Citing the commit message:
>> "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."
>>
> I don't see reference here about GS101 and others.
>
>> USIv1 and v2 write different values to the SW_CONF register. For example:
>>
>> #define USI_V1_SW_CONF_UART        0x8
>> #define USI_V2_SW_CONF_UART    BIT(0)
>>
>> ..
>>  [USI_V2_UART] =    { .name = "uart", .val = USI_V2_SW_CONF_UART },
>>  [USI_V1_UART] =    { .name = "uart", .val = USI_V1_SW_CONF_UART },
>> ..
>>
>> Hence the decision to have separate constants for different USI versions,
>> which in my opinion is cleaner than meshing the enums together and
>> choosing what to use with IFs in the driver code.
> This does not answer at all why GS101 receives now different values than
> before.
>
> Explain why you are changing ABI.

Oh I see what I've missed, it should be everything non-8895 having 0-3,
not just the top-level compatible samsung,exynos850-usi.

>
>>>> +
>>>>  if:
>>> Missing allOf:
>>>
>>>>    properties:
>>>>      compatible:
>>>>        contains:
>>>>          enum:
>>>>            - samsung,exynos850-usi
>>>> +          - samsung,exynos8895-usi
>>> Effect is not readable and not correct. You have two if:then:else.
>>> Usually it is easier to just have separate if: for each group of
>>> variants and define/constrain complete for such group within its if:.
>>>
>>>>  
>>>>  then:
>>>>    properties:
>>>> -    reg:
>>>> -      maxItems: 1
>>>> -
>>>>      clocks:
>>>>        items:
>>>>          - description: Bus (APB) clock
>>>> @@ -122,16 +156,13 @@ then:
>>>>        maxItems: 2
>>>>  
>>>>    required:
>>>> -    - reg
>>>>      - clocks
>>>>      - clock-names
>>>>  
>>>>  else:
>>>>    properties:
>>>> -    reg: false
>>>>      clocks: false
>>>>      clock-names: false
>>>> -    samsung,clkreq-on: false
>>>>  
>>>>  additionalProperties: false
>>>>  
>>>> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
>>>> index a01af169d..4c077c9a8 100644
>>>> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
>>>> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
>>>> @@ -13,5 +13,12 @@
>>>>  #define USI_V2_UART		1
>>>>  #define USI_V2_SPI		2
>>>>  #define USI_V2_I2C		3
>>>> +#define USI_V1_NONE		4
>>> Drop, it is already there.
>>>
>>>> +#define USI_V1_I2C0		5
>>>> +#define USI_V1_I2C1		6
>>>> +#define USI_V1_I2C0_1		7
>>>> +#define USI_V1_SPI		8
>>> Drop
>>>
>>>> +#define USI_V1_UART		9
>>> Drop
>> How so? These bring different configuration values. Could you specify how
>> exactly you want me to implement these in the driver?
> Heh, so the binding was made poorly for the driver and driver was
> developed in a matching way, so now this became an argument. Binding and
> drivers are independent, so whatever argument was in the driver does not
> matter really.
>
> What I don't understand is downstream for USIv1 and USIv2 has it correct
> - proper string values without mentioning any version. So, surprisingly
> proper downstream binding, really rare case, was converted to something
> tied to current driver implementation.
>
> You have only one sort of property - the mode how you configure the USI
> engine. The mode can be: I2C, SPI, I2C0, I2C1 for special cases with two
> I2C etc.
>
> The mode is not "USI_V1_I2C" because it is redundant. USI V1 cannot be
> USI V2. You cannot tell USI to be v1 or v2. It is either v1 or v2. Only
> one of these, thus encoding this information in the binding is meaningless.
>
> I even mentioned this in original binding review:
> "so please drop everywhere v2 (bindings, symbols, Kconfig,
> functions) except the compatible."
> Well, then I missed to check on that, so now this mess has to be fixed.

Yeah I get it now. Alright, I'll look into what I can do to unmangle this
mess.

Thanks again!

Best regards,
Ivaylo

>
> Best regards,
> Krzysztof