diff mbox series

[RESEND,v5,1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Message ID 20250303-k1-i2c-master-v5-1-21dfc7adfe37@gmail.com
State Superseded
Headers show
Series riscv: spacemit: add i2c support to K1 SoC | expand

Commit Message

Troy Mitchell March 3, 2025, 5:30 a.m. UTC 
The I2C of K1 supports fast-speed-mode and high-speed-mode,
and supports FIFO transmission.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
---
 .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)

Comments

Yixun Lan March 3, 2025, 9:35 a.m. UTC | #1 
On 13:30 Mon 03 Mar     , Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> ---
>  .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C controller embedded in SpacemiT's K1 SoC
> +
> +maintainers:
> +  - Troy Mitchell <troymitchell988@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-i2c
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
..
> +  clocks:
> +    minItems: 2
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 2
I'd suggest to give a brief description and explicit clock name here,
you can consult marvell,mv64xxx-i2c.yaml for example

> +
> +  clock-frequency:
> +    description: |
> +      K1 support three different modes which running different frequencies
> +      standard speed mode: up to 100000 (100Hz)
> +      fast speed mode    : up to 400000 (400Hz)
> +      high speed mode    : up to 3300000 (3.3Mhz)
> +    default: 400000
> +    maximum: 3300000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c@d4010800 {
> +        compatible = "spacemit,k1-i2c";
> +        reg = <0xd4010800 0x38>;
> +        interrupt-parent = <&plic>;
> +        interrupts = <36>;
> +        clocks = <&ccu 176>, <&ccu 90>;
> +        clock-names = "apb", "twsi";
9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
from above docs, there are two clocks 
bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable

I'd suggest to name it according to the functionality, thus 'func', 'bus'
clock, not its source.. which would make it more system wide consistent

> +        clock-frequency = <100000>;
> +    };
> +
> +...
> 
> -- 
> 2.34.1
>
Samuel Holland March 5, 2025, 2:11 a.m. UTC | #2 
Hi Troy,

On 2025-03-03 3:35 AM, Yixun Lan wrote:
> On 13:30 Mon 03 Mar     , Troy Mitchell wrote:
>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>> and supports FIFO transmission.
>>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
>> ---
>>  .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> @@ -0,0 +1,59 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C controller embedded in SpacemiT's K1 SoC
>> +
>> +maintainers:
>> +  - Troy Mitchell <troymitchell988@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: spacemit,k1-i2c
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
> ..
>> +  clocks:
>> +    minItems: 2
>> +    maxItems: 2
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    maxItems: 2
> I'd suggest to give a brief description and explicit clock name here,
> you can consult marvell,mv64xxx-i2c.yaml for example
> 
>> +
>> +  clock-frequency:
>> +    description: |
>> +      K1 support three different modes which running different frequencies
>> +      standard speed mode: up to 100000 (100Hz)
>> +      fast speed mode    : up to 400000 (400Hz)
>> +      high speed mode    : up to 3300000 (3.3Mhz)
>> +    default: 400000
>> +    maximum: 3300000
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c@d4010800 {
>> +        compatible = "spacemit,k1-i2c";
>> +        reg = <0xd4010800 0x38>;
>> +        interrupt-parent = <&plic>;
>> +        interrupts = <36>;
>> +        clocks = <&ccu 176>, <&ccu 90>;
>> +        clock-names = "apb", "twsi";
> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
> from above docs, there are two clocks 
> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
> 
> I'd suggest to name it according to the functionality, thus 'func', 'bus'
> clock, not its source.. which would make it more system wide consistent

Also in that same register is:

2	RST	RW	0x1	TWSI0 Reset Generation
This field resets both the APB and functional domain.
- 0: No Reset
- 1: Reset

Which means you need a 'resets' property in the binding as well.

Regards,
Samuel

>> +        clock-frequency = <100000>;
>> +    };
>> +
>> +...
>>
>> -- 
>> 2.34.1
>>
>
Yixun Lan March 5, 2025, 3:05 a.m. UTC | #3 
Hi Samuel Holland:

On 20:11 Tue 04 Mar     , Samuel Holland wrote:
> Hi Troy,
> 
> On 2025-03-03 3:35 AM, Yixun Lan wrote:
> > On 13:30 Mon 03 Mar     , Troy Mitchell wrote:
> >> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> >> and supports FIFO transmission.
> >>
> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> >> ---
> >>  .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
> >>  1 file changed, 59 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >> @@ -0,0 +1,59 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: I2C controller embedded in SpacemiT's K1 SoC
> >> +
> >> +maintainers:
> >> +  - Troy Mitchell <troymitchell988@gmail.com>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: spacemit,k1-i2c
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> > ..
> >> +  clocks:
> >> +    minItems: 2
> >> +    maxItems: 2
> >> +
> >> +  clock-names:
> >> +    minItems: 2
> >> +    maxItems: 2
> > I'd suggest to give a brief description and explicit clock name here,
> > you can consult marvell,mv64xxx-i2c.yaml for example
> > 
> >> +
> >> +  clock-frequency:
> >> +    description: |
> >> +      K1 support three different modes which running different frequencies
> >> +      standard speed mode: up to 100000 (100Hz)
> >> +      fast speed mode    : up to 400000 (400Hz)
> >> +      high speed mode    : up to 3300000 (3.3Mhz)
> >> +    default: 400000
> >> +    maximum: 3300000
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - interrupts
> >> +  - clocks
> >> +
> >> +unevaluatedProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    i2c@d4010800 {
> >> +        compatible = "spacemit,k1-i2c";
> >> +        reg = <0xd4010800 0x38>;
> >> +        interrupt-parent = <&plic>;
> >> +        interrupts = <36>;
> >> +        clocks = <&ccu 176>, <&ccu 90>;
> >> +        clock-names = "apb", "twsi";
> > 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
> > https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
> > from above docs, there are two clocks 
> > bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
> > bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
> > 
> > I'd suggest to name it according to the functionality, thus 'func', 'bus'
> > clock, not its source.. which would make it more system wide consistent
> 
> Also in that same register is:
> 
> 2	RST	RW	0x1	TWSI0 Reset Generation
> This field resets both the APB and functional domain.
> - 0: No Reset
> - 1: Reset
> 
> Which means you need a 'resets' property in the binding as well.
> 
right, there is reset needed

I'd suggest to add it as an incremental patch later, when we
implement real reset driver, and also complete the calling reset
consumer API in i2c driver

but, let me know if this is not the right way to go

> Regards,
> Samuel
> 
> >> +        clock-frequency = <100000>;
> >> +    };
> >> +
> >> +...
> >>
> >> -- 
> >> 2.34.1
> >>
> > 
> 
>
Samuel Holland March 5, 2025, 4:01 a.m. UTC | #4 
On 2025-03-04 9:05 PM, Yixun Lan wrote:
>>>> +        clocks = <&ccu 176>, <&ccu 90>;
>>>> +        clock-names = "apb", "twsi";
>>> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
>>> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
>>> from above docs, there are two clocks 
>>> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
>>> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
>>>
>>> I'd suggest to name it according to the functionality, thus 'func', 'bus'
>>> clock, not its source.. which would make it more system wide consistent
>>
>> Also in that same register is:
>>
>> 2	RST	RW	0x1	TWSI0 Reset Generation
>> This field resets both the APB and functional domain.
>> - 0: No Reset
>> - 1: Reset
>>
>> Which means you need a 'resets' property in the binding as well.
>>
> right, there is reset needed
> 
> I'd suggest to add it as an incremental patch later, when we
> implement real reset driver, and also complete the calling reset
> consumer API in i2c driver
> 
> but, let me know if this is not the right way to go

If you add the resets property later, that's a breaking change to the DT,
because existing devicetrees will not have that property. So you would have to
make the reset consumer in the driver optional, even if it's not really
optional, to work with older DTs. So it is _possible_ to add incrementally, but
not recommended because it adds "legacy" code that never really goes away.

It's okay to define the binding as requiring the resets property now, even
before the reset controller driver is merged. You just won't be able to add the
I2C controller to the DTS until the reset controller binding is merged. But
since the reset controller is the same IP block as the clock controller, its
binding should be available soon anyway.

Regards,
Samuel
Yixun Lan March 5, 2025, 2:10 p.m. UTC | #5 
Hi Samuel:

On 22:01 Tue 04 Mar     , Samuel Holland wrote:
> On 2025-03-04 9:05 PM, Yixun Lan wrote:
> >>>> +        clocks = <&ccu 176>, <&ccu 90>;
> >>>> +        clock-names = "apb", "twsi";
> >>> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
> >>> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
> >>> from above docs, there are two clocks 
> >>> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
> >>> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
> >>>
> >>> I'd suggest to name it according to the functionality, thus 'func', 'bus'
> >>> clock, not its source.. which would make it more system wide consistent
> >>
> >> Also in that same register is:
> >>
> >> 2	RST	RW	0x1	TWSI0 Reset Generation
> >> This field resets both the APB and functional domain.
> >> - 0: No Reset
> >> - 1: Reset
> >>
> >> Which means you need a 'resets' property in the binding as well.
> >>
> > right, there is reset needed
> > 
> > I'd suggest to add it as an incremental patch later, when we
> > implement real reset driver, and also complete the calling reset
> > consumer API in i2c driver
> > 
> > but, let me know if this is not the right way to go
> 
> If you add the resets property later, that's a breaking change to the DT,
> because existing devicetrees will not have that property. So you would have to
> make the reset consumer in the driver optional, even if it's not really
> optional, to work with older DTs. So it is _possible_ to add incrementally, but
> not recommended because it adds "legacy" code that never really goes away.
> 
Ok, that's fair if we want to keep DT backward compatible..

> It's okay to define the binding as requiring the resets property now, even
> before the reset controller driver is merged. You just won't be able to add the
> I2C controller to the DTS until the reset controller binding is merged. But
> since the reset controller is the same IP block as the clock controller, its
> binding should be available soon anyway.
> 
yes, this should work, thanks
I think we will wait for reset driver
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
@@ -0,0 +1,59 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C controller embedded in SpacemiT's K1 SoC
+
+maintainers:
+  - Troy Mitchell <troymitchell988@gmail.com>
+
+properties:
+  compatible:
+    const: spacemit,k1-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    minItems: 2
+    maxItems: 2
+
+  clock-frequency:
+    description: |
+      K1 support three different modes which running different frequencies
+      standard speed mode: up to 100000 (100Hz)
+      fast speed mode    : up to 400000 (400Hz)
+      high speed mode    : up to 3300000 (3.3Mhz)
+    default: 400000
+    maximum: 3300000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@d4010800 {
+        compatible = "spacemit,k1-i2c";
+        reg = <0xd4010800 0x38>;
+        interrupt-parent = <&plic>;
+        interrupts = <36>;
+        clocks = <&ccu 176>, <&ccu 90>;
+        clock-names = "apb", "twsi";
+        clock-frequency = <100000>;
+    };
+
+...