[PATCH v1 0/2] i2c: exynos5: Add support for Exynos8895 SoC

[PATCH v1 0/2] i2c: exynos5: Add support for Exynos8895 SoC

[Ivaylo Ivanov]

Hey folks,

This series adds HSI2C support for Exynos8895 to i2c-exynos5. HSI2C
buses here are mostly available implemented in USIv1 blocks, with 5
available externally - hsi2c_0 to 4. hsi2c_0 is available for PMIC
(although I've never seen it used) and it uses BUSC clocks, which are
still not implemented in the current clock driver, so I've decided to
leave it out for now.

In the next few patchsets support for USIv1 will be added to the
exynos-usi driver and with that the rest of the I2C buses will be made
available.

Kind regards,
Ivo

Ivaylo Ivanov (2):
  dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible
  i2c: exynos5: Add support for Exynos8895 SoC

 .../devicetree/bindings/i2c/i2c-exynos5.yaml  | 26 ++++++++++++++--
 drivers/i2c/busses/i2c-exynos5.c              | 31 +++++++++++++++++--
 2 files changed, 52 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Ivaylo Ivanov]

Add samsung,exynos8895-hsi2c dedicated compatible for representing
I2C of Exynos8895 SoC. Since there are I2C buses that aren't implemented
as a part of USIv1 blocks, they only require a single clock.

Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
---
 .../devicetree/bindings/i2c/i2c-exynos5.yaml  | 26 ++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
index cc8bba553..b029be88e 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
@@ -25,6 +25,7 @@ properties:
           - samsung,exynos5250-hsi2c    # Exynos5250 and Exynos5420
           - samsung,exynos5260-hsi2c    # Exynos5260
           - samsung,exynos7-hsi2c       # Exynos7
+          - samsung,exynos8895-hsi2c
           - samsung,exynosautov9-hsi2c
       - items:
           - enum:
@@ -94,9 +95,28 @@ allOf:
         - clock-names
 
     else:
-      properties:
-        clocks:
-          maxItems: 1
+      if:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - samsung,exynos8895-hsi2c
+
+      then:
+        properties:
+          clocks:
+            maxItems: 2
+
+          clock-names:
+            maxItems: 2
+
+        required:
+          - clock-names
+
+      else:
+        properties:
+          clocks:
+            maxItems: 1
 
 unevaluatedProperties: false
 
-- 
2.43.0

[PATCH v1 2/2] i2c: exynos5: Add support for Exynos8895 SoC

[Ivaylo Ivanov]

Exynos8895 functioning logic mostly follows I2C_TYPE_EXYNOS7, but timing
and temp calculations are slightly changed according to the following
logic:

FPCLK / FI2C = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 2 *
((FLT_CYCLE + 3) - (FLT_CYCLE + 3) % (CLK_DIV + 1))

temp := (FPCLK / FI2C) - (FLT_CYCLE + 3) * 2

Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index e33001508..8d8ee42ba 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -168,6 +168,7 @@ enum i2c_type_exynos {
 	I2C_TYPE_EXYNOS5,
 	I2C_TYPE_EXYNOS7,
 	I2C_TYPE_EXYNOSAUTOV9,
+	I2C_TYPE_EXYNOS8895,
 };
 
 struct exynos5_i2c {
@@ -240,6 +241,11 @@ static const struct exynos_hsi2c_variant exynosautov9_hsi2c_data = {
 	.hw		= I2C_TYPE_EXYNOSAUTOV9,
 };
 
+static const struct exynos_hsi2c_variant exynos8895_hsi2c_data = {
+	.fifo_depth	= 64,
+	.hw		= I2C_TYPE_EXYNOS8895,
+};
+
 static const struct of_device_id exynos5_i2c_match[] = {
 	{
 		.compatible = "samsung,exynos5-hsi2c",
@@ -256,6 +262,9 @@ static const struct of_device_id exynos5_i2c_match[] = {
 	}, {
 		.compatible = "samsung,exynosautov9-hsi2c",
 		.data = &exynosautov9_hsi2c_data
+	}, {
+		.compatible = "samsung,exynos8895-hsi2c",
+		.data = &exynos8895_hsi2c_data
 	}, {},
 };
 MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
@@ -331,6 +340,14 @@ static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, bool hs_timings)
 	 * clk_cycle := TSCLK_L + TSCLK_H
 	 * temp := (CLK_DIV + 1) * (clk_cycle + 2)
 	 *
+	 * In case of HSI2C controllers in Exynos8895
+	 * FPCLK / FI2C =
+	 * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) +
+	 * 2 * ((FLT_CYCLE + 3) - (FLT_CYCLE + 3) % (CLK_DIV + 1))
+	 *
+	 * clk_cycle := TSCLK_L + TSCLK_H
+	 * temp := (FPCLK / FI2C) - (FLT_CYCLE + 3) * 2
+	 *
 	 * Constraints: 4 <= temp, 0 <= CLK_DIV < 256, 2 <= clk_cycle <= 510
 	 *
 	 * To split SCL clock into low, high periods appropriately, one
@@ -352,11 +369,19 @@ static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, bool hs_timings)
 	 *
 	 */
 	t_ftl_cycle = (readl(i2c->regs + HSI2C_CONF) >> 16) & 0x7;
-	temp = clkin / op_clk - 8 - t_ftl_cycle;
+	if (i2c->variant->hw == I2C_TYPE_EXYNOS8895)
+		temp = clkin / op_clk - (t_ftl_cycle + 3) * 2;
+	else
+		temp = clkin / op_clk - 8 - t_ftl_cycle;
 	if (i2c->variant->hw != I2C_TYPE_EXYNOS7)
 		temp -= t_ftl_cycle;
 	div = temp / 512;
-	clk_cycle = temp / (div + 1) - 2;
+
+	if (i2c->variant->hw == I2C_TYPE_EXYNOS8895)
+		clk_cycle = (temp + ((t_ftl_cycle + 3) % (div + 1)) * 2) /
+			    (div + 1) - 2;
+	else
+		clk_cycle = temp / (div + 1) - 2;
 	if (temp < 4 || div >= 256 || clk_cycle < 2) {
 		dev_err(i2c->dev, "%s clock set-up failed\n",
 			hs_timings ? "HS" : "FS");
@@ -491,6 +516,8 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 	switch (i2c->variant->hw) {
 	case I2C_TYPE_EXYNOSAUTOV9:
 		fallthrough;
+	case I2C_TYPE_EXYNOS8895:
+		fallthrough;
 	case I2C_TYPE_EXYNOS7:
 		if (int_status & HSI2C_INT_TRANS_DONE) {
 			i2c->trans_done = 1;
-- 
2.43.0

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Krzysztof Kozlowski]

On 14/12/2024 23:04, Ivaylo Ivanov wrote:
> Add samsung,exynos8895-hsi2c dedicated compatible for representing
> I2C of Exynos8895 SoC. Since there are I2C buses that aren't implemented
> as a part of USIv1 blocks, they only require a single clock.
> 
> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
> ---
>  .../devicetree/bindings/i2c/i2c-exynos5.yaml  | 26 ++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
> index cc8bba553..b029be88e 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
> @@ -25,6 +25,7 @@ properties:
>            - samsung,exynos5250-hsi2c    # Exynos5250 and Exynos5420
>            - samsung,exynos5260-hsi2c    # Exynos5260
>            - samsung,exynos7-hsi2c       # Exynos7
> +          - samsung,exynos8895-hsi2c
>            - samsung,exynosautov9-hsi2c
>        - items:
>            - enum:
> @@ -94,9 +95,28 @@ allOf:
>          - clock-names
>  
>      else:
> -      properties:
> -        clocks:
> -          maxItems: 1
> +      if:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - samsung,exynos8895-hsi2c
> +
> +      then:
> +        properties:
> +          clocks:

Missing minItems

> +            maxItems: 2
> +
> +          clock-names:

Ditto

> +            maxItems: 2
> +
> +        required:
> +          - clock-names
I don't understand why do you need second, same branch in if, basically
duplicating previous. But regardless of that, no nesting of ifs. Define
clocks for all variants explicitly.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Ivaylo Ivanov]

On 12/16/24 10:44, Krzysztof Kozlowski wrote:
> On 14/12/2024 23:04, Ivaylo Ivanov wrote:
>> Add samsung,exynos8895-hsi2c dedicated compatible for representing
>> I2C of Exynos8895 SoC. Since there are I2C buses that aren't implemented
>> as a part of USIv1 blocks, they only require a single clock.
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> ---
>>  .../devicetree/bindings/i2c/i2c-exynos5.yaml  | 26 ++++++++++++++++---
>>  1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>> index cc8bba553..b029be88e 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>> @@ -25,6 +25,7 @@ properties:
>>            - samsung,exynos5250-hsi2c    # Exynos5250 and Exynos5420
>>            - samsung,exynos5260-hsi2c    # Exynos5260
>>            - samsung,exynos7-hsi2c       # Exynos7
>> +          - samsung,exynos8895-hsi2c
>>            - samsung,exynosautov9-hsi2c
>>        - items:
>>            - enum:
>> @@ -94,9 +95,28 @@ allOf:
>>          - clock-names
>>  
>>      else:
>> -      properties:
>> -        clocks:
>> -          maxItems: 1
>> +      if:
>> +        properties:
>> +          compatible:
>> +            contains:
>> +              enum:
>> +                - samsung,exynos8895-hsi2c
>> +
>> +      then:
>> +        properties:
>> +          clocks:
> Missing minItems
>
>> +            maxItems: 2
>> +
>> +          clock-names:
> Ditto
>
>> +            maxItems: 2
>> +
>> +        required:
>> +          - clock-names
> I don't understand why do you need second, same branch in if, basically

Because, as I stated in the commit message, we have HSI2C controllers
both implemented in USIv1 blocks and outside. These that are a part of
USIv1 need 2 clocks, and those that aren't have only one. So it's not
a duplicate for the previous - autov9 sets a minitems of 2 and the
others have a maxitems of 1.

Best regards,
Ivo

> duplicating previous. But regardless of that, no nesting of ifs. Define
> clocks for all variants explicitly.
>
> Best regards,
> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Krzysztof Kozlowski]

On 16/12/2024 21:59, Ivaylo Ivanov wrote:
> On 12/16/24 10:44, Krzysztof Kozlowski wrote:
>> On 14/12/2024 23:04, Ivaylo Ivanov wrote:
>>> Add samsung,exynos8895-hsi2c dedicated compatible for representing
>>> I2C of Exynos8895 SoC. Since there are I2C buses that aren't implemented
>>> as a part of USIv1 blocks, they only require a single clock.
>>>
>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>>> ---
>>>  .../devicetree/bindings/i2c/i2c-exynos5.yaml  | 26 ++++++++++++++++---
>>>  1 file changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>>> index cc8bba553..b029be88e 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>>> @@ -25,6 +25,7 @@ properties:
>>>            - samsung,exynos5250-hsi2c    # Exynos5250 and Exynos5420
>>>            - samsung,exynos5260-hsi2c    # Exynos5260
>>>            - samsung,exynos7-hsi2c       # Exynos7
>>> +          - samsung,exynos8895-hsi2c
>>>            - samsung,exynosautov9-hsi2c
>>>        - items:
>>>            - enum:
>>> @@ -94,9 +95,28 @@ allOf:
>>>          - clock-names
>>>  
>>>      else:
>>> -      properties:
>>> -        clocks:
>>> -          maxItems: 1
>>> +      if:
>>> +        properties:
>>> +          compatible:
>>> +            contains:
>>> +              enum:
>>> +                - samsung,exynos8895-hsi2c
>>> +
>>> +      then:
>>> +        properties:
>>> +          clocks:
>> Missing minItems
>>
>>> +            maxItems: 2
>>> +
>>> +          clock-names:
>> Ditto
>>
>>> +            maxItems: 2
>>> +
>>> +        required:
>>> +          - clock-names
>> I don't understand why do you need second, same branch in if, basically
> 
> Because, as I stated in the commit message, we have HSI2C controllers
> both implemented in USIv1 blocks and outside. These that are a part of

On Exynos8895? Where? With the same compatible?

> USIv1 need 2 clocks, and those that aren't have only one. So it's not
> a duplicate for the previous - autov9 sets a minitems of 2 and the
> others have a maxitems of 1.

We talk here about branch that says 2 items. You duplicate that one.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Ivaylo Ivanov]

On 12/17/24 07:24, Krzysztof Kozlowski wrote:
> On 16/12/2024 21:59, Ivaylo Ivanov wrote:
>> On 12/16/24 10:44, Krzysztof Kozlowski wrote:
>>> On 14/12/2024 23:04, Ivaylo Ivanov wrote:
>>>> Add samsung,exynos8895-hsi2c dedicated compatible for representing
>>>> I2C of Exynos8895 SoC. Since there are I2C buses that aren't implemented
>>>> as a part of USIv1 blocks, they only require a single clock.
>>>>
>>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>>>> ---
>>>>  .../devicetree/bindings/i2c/i2c-exynos5.yaml  | 26 ++++++++++++++++---
>>>>  1 file changed, 23 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>>>> index cc8bba553..b029be88e 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-exynos5.yaml
>>>> @@ -25,6 +25,7 @@ properties:
>>>>            - samsung,exynos5250-hsi2c    # Exynos5250 and Exynos5420
>>>>            - samsung,exynos5260-hsi2c    # Exynos5260
>>>>            - samsung,exynos7-hsi2c       # Exynos7
>>>> +          - samsung,exynos8895-hsi2c
>>>>            - samsung,exynosautov9-hsi2c
>>>>        - items:
>>>>            - enum:
>>>> @@ -94,9 +95,28 @@ allOf:
>>>>          - clock-names
>>>>  
>>>>      else:
>>>> -      properties:
>>>> -        clocks:
>>>> -          maxItems: 1
>>>> +      if:
>>>> +        properties:
>>>> +          compatible:
>>>> +            contains:
>>>> +              enum:
>>>> +                - samsung,exynos8895-hsi2c
>>>> +
>>>> +      then:
>>>> +        properties:
>>>> +          clocks:
>>> Missing minItems
>>>
>>>> +            maxItems: 2
>>>> +
>>>> +          clock-names:
>>> Ditto
>>>
>>>> +            maxItems: 2
>>>> +
>>>> +        required:
>>>> +          - clock-names
>>> I don't understand why do you need second, same branch in if, basically
>> Because, as I stated in the commit message, we have HSI2C controllers
>> both implemented in USIv1 blocks and outside. These that are a part of
> On Exynos8895? Where? With the same compatible?

hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
they need a different compatible though? It's functionally the same i2c design
as the one implemented in USIv1 blocks.

>
>> USIv1 need 2 clocks, and those that aren't have only one. So it's not
>> a duplicate for the previous - autov9 sets a minitems of 2 and the
>> others have a maxitems of 1.
> We talk here about branch that says 2 items. You duplicate that one.

I see. I can just define the clocks for all at this point.

Best regards,
Ivo.

>
> Best regards,
> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Krzysztof Kozlowski]

On 17/12/2024 10:08, Ivaylo Ivanov wrote:
>>>>>        - items:
>>>>>            - enum:
>>>>> @@ -94,9 +95,28 @@ allOf:
>>>>>          - clock-names
>>>>>  
>>>>>      else:
>>>>> -      properties:
>>>>> -        clocks:
>>>>> -          maxItems: 1
>>>>> +      if:
>>>>> +        properties:
>>>>> +          compatible:
>>>>> +            contains:
>>>>> +              enum:
>>>>> +                - samsung,exynos8895-hsi2c
>>>>> +
>>>>> +      then:
>>>>> +        properties:
>>>>> +          clocks:
>>>> Missing minItems
>>>>
>>>>> +            maxItems: 2
>>>>> +
>>>>> +          clock-names:
>>>> Ditto
>>>>
>>>>> +            maxItems: 2
>>>>> +
>>>>> +        required:
>>>>> +          - clock-names
>>>> I don't understand why do you need second, same branch in if, basically
>>> Because, as I stated in the commit message, we have HSI2C controllers
>>> both implemented in USIv1 blocks and outside. These that are a part of
>> On Exynos8895? Where? With the same compatible?
> 
> hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
> from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
> they need a different compatible though? It's functionally the same i2c design
> as the one implemented in USIv1 blocks.
If one block is part of USI and other not, they might not be the same
I2C blocks, even if interface is similar. If they were the same or even
functionally the same, they would have the same clock inputs. However
user manual also suggests that there is only one clock, not two (for
both cases), so they could be functionally equivalent but then number of
clocks looks incorrect.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Ivaylo Ivanov]

On 12/17/24 11:26, Krzysztof Kozlowski wrote:
> On 17/12/2024 10:08, Ivaylo Ivanov wrote:
>>>>>>        - items:
>>>>>>            - enum:
>>>>>> @@ -94,9 +95,28 @@ allOf:
>>>>>>          - clock-names
>>>>>>  
>>>>>>      else:
>>>>>> -      properties:
>>>>>> -        clocks:
>>>>>> -          maxItems: 1
>>>>>> +      if:
>>>>>> +        properties:
>>>>>> +          compatible:
>>>>>> +            contains:
>>>>>> +              enum:
>>>>>> +                - samsung,exynos8895-hsi2c
>>>>>> +
>>>>>> +      then:
>>>>>> +        properties:
>>>>>> +          clocks:
>>>>> Missing minItems
>>>>>
>>>>>> +            maxItems: 2
>>>>>> +
>>>>>> +          clock-names:
>>>>> Ditto
>>>>>
>>>>>> +            maxItems: 2
>>>>>> +
>>>>>> +        required:
>>>>>> +          - clock-names
>>>>> I don't understand why do you need second, same branch in if, basically
>>>> Because, as I stated in the commit message, we have HSI2C controllers
>>>> both implemented in USIv1 blocks and outside. These that are a part of
>>> On Exynos8895? Where? With the same compatible?
>> hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
>> from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
>> they need a different compatible though? It's functionally the same i2c design
>> as the one implemented in USIv1 blocks.
> If one block is part of USI and other not, they might not be the same
> I2C blocks, even if interface is similar. If they were the same or even
> functionally the same, they would have the same clock inputs. However

I see, so in such case I should make samsung,exynos8895-hsi2c-nonusi or
something like that?

> user manual also suggests that there is only one clock, not two (for
> both cases), so they could be functionally equivalent but then number of
> clocks looks incorrect.

That'd be weird. Both according to downstream and upstream clk driver,
for the USI-implemented i2cs we have a pclk and an sclk_usi.

Best regards,
Ivo.

>
> Best regards,
> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Krzysztof Kozlowski]

On 17/12/2024 10:31, Ivaylo Ivanov wrote:
> On 12/17/24 11:26, Krzysztof Kozlowski wrote:
>> On 17/12/2024 10:08, Ivaylo Ivanov wrote:
>>>>>>>        - items:
>>>>>>>            - enum:
>>>>>>> @@ -94,9 +95,28 @@ allOf:
>>>>>>>          - clock-names
>>>>>>>  
>>>>>>>      else:
>>>>>>> -      properties:
>>>>>>> -        clocks:
>>>>>>> -          maxItems: 1
>>>>>>> +      if:
>>>>>>> +        properties:
>>>>>>> +          compatible:
>>>>>>> +            contains:
>>>>>>> +              enum:
>>>>>>> +                - samsung,exynos8895-hsi2c
>>>>>>> +
>>>>>>> +      then:
>>>>>>> +        properties:
>>>>>>> +          clocks:
>>>>>> Missing minItems
>>>>>>
>>>>>>> +            maxItems: 2
>>>>>>> +
>>>>>>> +          clock-names:
>>>>>> Ditto
>>>>>>
>>>>>>> +            maxItems: 2
>>>>>>> +
>>>>>>> +        required:
>>>>>>> +          - clock-names
>>>>>> I don't understand why do you need second, same branch in if, basically
>>>>> Because, as I stated in the commit message, we have HSI2C controllers
>>>>> both implemented in USIv1 blocks and outside. These that are a part of
>>>> On Exynos8895? Where? With the same compatible?
>>> hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
>>> from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
>>> they need a different compatible though? It's functionally the same i2c design
>>> as the one implemented in USIv1 blocks.
>> If one block is part of USI and other not, they might not be the same
>> I2C blocks, even if interface is similar. If they were the same or even
>> functionally the same, they would have the same clock inputs. However
> 
> I see, so in such case I should make samsung,exynos8895-hsi2c-nonusi or
> something like that?
> 
>> user manual also suggests that there is only one clock, not two (for
>> both cases), so they could be functionally equivalent but then number of
>> clocks looks incorrect.
> 
> That'd be weird. Both according to downstream and upstream clk driver,
> for the USI-implemented i2cs we have a pclk and an sclk_usi.
Something is not precise here, as usually with Samsung clock topology.

First, the non-USI instances have the IPCLK as well, e.g. things like
PERIC1_UID_HSI2C_CAM1_IPCLKPORT_iPCLK

USI have BLK_PERIC0_UID_USI03_IPCLKPORT_i_SCLK_USI, but that's USI
clock, not HSI2C in USI. Datasheet mentions this is UART and SPI special
clock, but not I2C. The PCLK is used for HSI2C iPCLK.


Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Ivaylo Ivanov]

On 12/17/24 11:43, Krzysztof Kozlowski wrote:
> On 17/12/2024 10:31, Ivaylo Ivanov wrote:
>> On 12/17/24 11:26, Krzysztof Kozlowski wrote:
>>> On 17/12/2024 10:08, Ivaylo Ivanov wrote:
>>>>>>>>        - items:
>>>>>>>>            - enum:
>>>>>>>> @@ -94,9 +95,28 @@ allOf:
>>>>>>>>          - clock-names
>>>>>>>>  
>>>>>>>>      else:
>>>>>>>> -      properties:
>>>>>>>> -        clocks:
>>>>>>>> -          maxItems: 1
>>>>>>>> +      if:
>>>>>>>> +        properties:
>>>>>>>> +          compatible:
>>>>>>>> +            contains:
>>>>>>>> +              enum:
>>>>>>>> +                - samsung,exynos8895-hsi2c
>>>>>>>> +
>>>>>>>> +      then:
>>>>>>>> +        properties:
>>>>>>>> +          clocks:
>>>>>>> Missing minItems
>>>>>>>
>>>>>>>> +            maxItems: 2
>>>>>>>> +
>>>>>>>> +          clock-names:
>>>>>>> Ditto
>>>>>>>
>>>>>>>> +            maxItems: 2
>>>>>>>> +
>>>>>>>> +        required:
>>>>>>>> +          - clock-names
>>>>>>> I don't understand why do you need second, same branch in if, basically
>>>>>> Because, as I stated in the commit message, we have HSI2C controllers
>>>>>> both implemented in USIv1 blocks and outside. These that are a part of
>>>>> On Exynos8895? Where? With the same compatible?
>>>> hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
>>>> from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
>>>> they need a different compatible though? It's functionally the same i2c design
>>>> as the one implemented in USIv1 blocks.
>>> If one block is part of USI and other not, they might not be the same
>>> I2C blocks, even if interface is similar. If they were the same or even
>>> functionally the same, they would have the same clock inputs. However
>> I see, so in such case I should make samsung,exynos8895-hsi2c-nonusi or
>> something like that?
>>
>>> user manual also suggests that there is only one clock, not two (for
>>> both cases), so they could be functionally equivalent but then number of
>>> clocks looks incorrect.
>> That'd be weird. Both according to downstream and upstream clk driver,
>> for the USI-implemented i2cs we have a pclk and an sclk_usi.
> Something is not precise here, as usually with Samsung clock topology.
>
> First, the non-USI instances have the IPCLK as well, e.g. things like
> PERIC1_UID_HSI2C_CAM1_IPCLKPORT_iPCLK
>
> USI have BLK_PERIC0_UID_USI03_IPCLKPORT_i_SCLK_USI, but that's USI
> clock, not HSI2C in USI. Datasheet mentions this is UART and SPI special
> clock, but not I2C.

That's weird. Don't we need the clock enabled in order for the
USIv1's HSI2C to work?

Best regards,
Ivo

>  The PCLK is used for HSI2C iPCLK.
>
>
> Best regards,
> Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Markuss Broks]

Hi Krzysztof,

On 12/17/24 11:43 AM, Krzysztof Kozlowski wrote:
> On 17/12/2024 10:31, Ivaylo Ivanov wrote:
>> On 12/17/24 11:26, Krzysztof Kozlowski wrote:
>>> On 17/12/2024 10:08, Ivaylo Ivanov wrote:
>>>>>>>>         - items:
>>>>>>>>             - enum:
>>>>>>>> @@ -94,9 +95,28 @@ allOf:
>>>>>>>>           - clock-names
>>>>>>>>   
>>>>>>>>       else:
>>>>>>>> -      properties:
>>>>>>>> -        clocks:
>>>>>>>> -          maxItems: 1
>>>>>>>> +      if:
>>>>>>>> +        properties:
>>>>>>>> +          compatible:
>>>>>>>> +            contains:
>>>>>>>> +              enum:
>>>>>>>> +                - samsung,exynos8895-hsi2c
>>>>>>>> +
>>>>>>>> +      then:
>>>>>>>> +        properties:
>>>>>>>> +          clocks:
>>>>>>> Missing minItems
>>>>>>>
>>>>>>>> +            maxItems: 2
>>>>>>>> +
>>>>>>>> +          clock-names:
>>>>>>> Ditto
>>>>>>>
>>>>>>>> +            maxItems: 2
>>>>>>>> +
>>>>>>>> +        required:
>>>>>>>> +          - clock-names
>>>>>>> I don't understand why do you need second, same branch in if, basically
>>>>>> Because, as I stated in the commit message, we have HSI2C controllers
>>>>>> both implemented in USIv1 blocks and outside. These that are a part of
>>>>> On Exynos8895? Where? With the same compatible?
>>>> hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
>>>> from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
>>>> they need a different compatible though? It's functionally the same i2c design
>>>> as the one implemented in USIv1 blocks.
>>> If one block is part of USI and other not, they might not be the same
>>> I2C blocks, even if interface is similar. If they were the same or even
>>> functionally the same, they would have the same clock inputs. However
>> I see, so in such case I should make samsung,exynos8895-hsi2c-nonusi or
>> something like that?
>>
>>> user manual also suggests that there is only one clock, not two (for
>>> both cases), so they could be functionally equivalent but then number of
>>> clocks looks incorrect.
>> That'd be weird. Both according to downstream and upstream clk driver,
>> for the USI-implemented i2cs we have a pclk and an sclk_usi.
> Something is not precise here, as usually with Samsung clock topology.
>
> First, the non-USI instances have the IPCLK as well, e.g. things like
> PERIC1_UID_HSI2C_CAM1_IPCLKPORT_iPCLK
>
> USI have BLK_PERIC0_UID_USI03_IPCLKPORT_i_SCLK_USI, but that's USI
> clock, not HSI2C in USI. Datasheet mentions this is UART and SPI special
> clock, but not I2C. The PCLK is used for HSI2C iPCLK.

In USI, USI PCLK is used for the internal AMBA APB bus clock and the 
IPCLK signal is used for the peripheral controller blocks (i2c/spi/uart).

So perhaps we have it described incorrectly, and the hsi2c controllers 
(at least on E8895) should take only one clock input (IPCLK), and USI 
block should take the PCLK input.


>
>
> Best regards,
> Krzysztof
>

- Markuss

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Krzysztof Kozlowski]

On 17/12/2024 11:04, Ivaylo Ivanov wrote:
> On 12/17/24 11:43, Krzysztof Kozlowski wrote:
>> On 17/12/2024 10:31, Ivaylo Ivanov wrote:
>>> On 12/17/24 11:26, Krzysztof Kozlowski wrote:
>>>> On 17/12/2024 10:08, Ivaylo Ivanov wrote:
>>>>>>>>>        - items:
>>>>>>>>>            - enum:
>>>>>>>>> @@ -94,9 +95,28 @@ allOf:
>>>>>>>>>          - clock-names
>>>>>>>>>  
>>>>>>>>>      else:
>>>>>>>>> -      properties:
>>>>>>>>> -        clocks:
>>>>>>>>> -          maxItems: 1
>>>>>>>>> +      if:
>>>>>>>>> +        properties:
>>>>>>>>> +          compatible:
>>>>>>>>> +            contains:
>>>>>>>>> +              enum:
>>>>>>>>> +                - samsung,exynos8895-hsi2c
>>>>>>>>> +
>>>>>>>>> +      then:
>>>>>>>>> +        properties:
>>>>>>>>> +          clocks:
>>>>>>>> Missing minItems
>>>>>>>>
>>>>>>>>> +            maxItems: 2
>>>>>>>>> +
>>>>>>>>> +          clock-names:
>>>>>>>> Ditto
>>>>>>>>
>>>>>>>>> +            maxItems: 2
>>>>>>>>> +
>>>>>>>>> +        required:
>>>>>>>>> +          - clock-names
>>>>>>>> I don't understand why do you need second, same branch in if, basically
>>>>>>> Because, as I stated in the commit message, we have HSI2C controllers
>>>>>>> both implemented in USIv1 blocks and outside. These that are a part of
>>>>>> On Exynos8895? Where? With the same compatible?
>>>>> hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
>>>>> from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
>>>>> they need a different compatible though? It's functionally the same i2c design
>>>>> as the one implemented in USIv1 blocks.
>>>> If one block is part of USI and other not, they might not be the same
>>>> I2C blocks, even if interface is similar. If they were the same or even
>>>> functionally the same, they would have the same clock inputs. However
>>> I see, so in such case I should make samsung,exynos8895-hsi2c-nonusi or
>>> something like that?
>>>
>>>> user manual also suggests that there is only one clock, not two (for
>>>> both cases), so they could be functionally equivalent but then number of
>>>> clocks looks incorrect.
>>> That'd be weird. Both according to downstream and upstream clk driver,
>>> for the USI-implemented i2cs we have a pclk and an sclk_usi.
>> Something is not precise here, as usually with Samsung clock topology.
>>
>> First, the non-USI instances have the IPCLK as well, e.g. things like
>> PERIC1_UID_HSI2C_CAM1_IPCLKPORT_iPCLK
>>
>> USI have BLK_PERIC0_UID_USI03_IPCLKPORT_i_SCLK_USI, but that's USI
>> clock, not HSI2C in USI. Datasheet mentions this is UART and SPI special
>> clock, but not I2C.
> 
> That's weird. Don't we need the clock enabled in order for the
> USIv1's HSI2C to work?
The clock goes to USI, so it is enabled, no?

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: i2c: exynos5: Add samsung,exynos8895-hsi2c compatible

[Ivaylo Ivanov]

On 12/18/24 11:22, Krzysztof Kozlowski wrote:
> On 17/12/2024 11:04, Ivaylo Ivanov wrote:
>> On 12/17/24 11:43, Krzysztof Kozlowski wrote:
>>> On 17/12/2024 10:31, Ivaylo Ivanov wrote:
>>>> On 12/17/24 11:26, Krzysztof Kozlowski wrote:
>>>>> On 17/12/2024 10:08, Ivaylo Ivanov wrote:
>>>>>>>>>>        - items:
>>>>>>>>>>            - enum:
>>>>>>>>>> @@ -94,9 +95,28 @@ allOf:
>>>>>>>>>>          - clock-names
>>>>>>>>>>  
>>>>>>>>>>      else:
>>>>>>>>>> -      properties:
>>>>>>>>>> -        clocks:
>>>>>>>>>> -          maxItems: 1
>>>>>>>>>> +      if:
>>>>>>>>>> +        properties:
>>>>>>>>>> +          compatible:
>>>>>>>>>> +            contains:
>>>>>>>>>> +              enum:
>>>>>>>>>> +                - samsung,exynos8895-hsi2c
>>>>>>>>>> +
>>>>>>>>>> +      then:
>>>>>>>>>> +        properties:
>>>>>>>>>> +          clocks:
>>>>>>>>> Missing minItems
>>>>>>>>>
>>>>>>>>>> +            maxItems: 2
>>>>>>>>>> +
>>>>>>>>>> +          clock-names:
>>>>>>>>> Ditto
>>>>>>>>>
>>>>>>>>>> +            maxItems: 2
>>>>>>>>>> +
>>>>>>>>>> +        required:
>>>>>>>>>> +          - clock-names
>>>>>>>>> I don't understand why do you need second, same branch in if, basically
>>>>>>>> Because, as I stated in the commit message, we have HSI2C controllers
>>>>>>>> both implemented in USIv1 blocks and outside. These that are a part of
>>>>>>> On Exynos8895? Where? With the same compatible?
>>>>>> hsi2c_0 which has a clock from BUSC and hsi2c_1 to hsi2c_4 which use clocks
>>>>>> from PERIC1 (CLK_GOUT_PERIC1_HSI2C_CAM{0,1,2,3}_IPCLK). Why would
>>>>>> they need a different compatible though? It's functionally the same i2c design
>>>>>> as the one implemented in USIv1 blocks.
>>>>> If one block is part of USI and other not, they might not be the same
>>>>> I2C blocks, even if interface is similar. If they were the same or even
>>>>> functionally the same, they would have the same clock inputs. However
>>>> I see, so in such case I should make samsung,exynos8895-hsi2c-nonusi or
>>>> something like that?
>>>>
>>>>> user manual also suggests that there is only one clock, not two (for
>>>>> both cases), so they could be functionally equivalent but then number of
>>>>> clocks looks incorrect.
>>>> That'd be weird. Both according to downstream and upstream clk driver,
>>>> for the USI-implemented i2cs we have a pclk and an sclk_usi.
>>> Something is not precise here, as usually with Samsung clock topology.
>>>
>>> First, the non-USI instances have the IPCLK as well, e.g. things like
>>> PERIC1_UID_HSI2C_CAM1_IPCLKPORT_iPCLK
>>>
>>> USI have BLK_PERIC0_UID_USI03_IPCLKPORT_i_SCLK_USI, but that's USI
>>> clock, not HSI2C in USI. Datasheet mentions this is UART and SPI special
>>> clock, but not I2C.
>> That's weird. Don't we need the clock enabled in order for the
>> USIv1's HSI2C to work?
> The clock goes to USI, so it is enabled, no?

Yes, and as Markuss said:
"USI PCLK is used for the internal AMBA APB bus clock and the IPCLK
signal is used for the peripheral controller blocks (i2c/spi/uart)."

So perhaps referencing the USI PCLK in the hsi2c driver for USIv2, as
well as USIv1, is a wrong approach and should be dropped/fixed?

Best regards,
Ivo

>
> Best regards,
> Krzysztof