[v2] riscv: spacemit: add i2c support to K1 SoC

[PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Troy Mitchell 4 weeks ago

The I2C of K1 supports fast-speed-mode and high-speed-mode,
and supports FIFO transmission.

Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
---
 .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml

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 000000000000..57af66f494e7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
@@ -0,0 +1,51 @@
+# 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: 2
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-frequency:
+    description:
+      Desired I2C bus clock frequency in Hz. As only fast and high-speed
+      modes are supported by hardware, possible values are 100000 and 400000.
+    enum: [100000, 400000]
+    default: 100000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@d4010800 {
+        compatible = "spacemit,k1-i2c";
+        reg = <0x0 0xd4010800 0x0 0x38>;
+        interrupt-parent = <&plic>;
+        interrupts = <36>;
+        clocks = <&ccu 90>;
+        clock-frequency = <100000>;
+    };
+
+...
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Samuel Holland 3 weeks, 2 days ago

Hi Troy,

On 2024-10-28 12:32 AM, Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---
>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> 
> 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 000000000000..57af66f494e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -0,0 +1,51 @@
> +# 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: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1

Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
looks to be standard across the peripherals in this SoC. Please be sure that the
binding covers all resources needed to use this peripheral.

> +
> +  clock-frequency:
> +    description:
> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
> +      modes are supported by hardware, possible values are 100000 and 400000.
> +    enum: [100000, 400000]

This looks wrong. In the manual I see:

* Supports standard-mode operation up to 100 Kbps
* Supports fast-mode operation up to 400Kbps
* Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
I2C only)
* Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
I2C only)

So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
frequencies.

Regards,
Samuel

> +    default: 100000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c@d4010800 {
> +        compatible = "spacemit,k1-i2c";
> +        reg = <0x0 0xd4010800 0x0 0x38>;
> +        interrupt-parent = <&plic>;
> +        interrupts = <36>;
> +        clocks = <&ccu 90>;
> +        clock-frequency = <100000>;
> +    };
> +
> +...

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Troy Mitchell 2 weeks, 5 days ago

On 2024/11/2 11:48, Samuel Holland wrote:
> Hi Troy,
> 
> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>> and supports FIFO transmission.
>>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>> ---
>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>
>> 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 000000000000..57af66f494e7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> @@ -0,0 +1,51 @@
>> +# 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: 2
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
> 
> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
> looks to be standard across the peripherals in this SoC. Please be sure that the
> binding covers all resources needed to use this peripheral.RCPU stands for Real-time CPU, which is typically used for low power consumption
applications.
We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
user manual. However, you can find this register referenced in the K1 clock patch:
https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/

Also, to see how to enable the I2C clock in the device tree (note that the
spacemit,apb_clock property is unused in the driver), check out the example here:
https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048

> 
>> +
>> +  clock-frequency:
>> +    description:
>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>> +      modes are supported by hardware, possible values are 100000 and 400000.
>> +    enum: [100000, 400000]
> 
> This looks wrong. In the manual I see:
> 
> * Supports standard-mode operation up to 100 Kbps
> * Supports fast-mode operation up to 400Kbps
> * Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
> I2C only)
> * Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
> I2C only)
> 
> So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
> frequencies.
okay. I will fix it in next version.
and should I keep to ignore high-speed mode here?
if not, how about this:

  clock-frequency:
    description:
      Desired I2C bus clock frequency in Hz.
      K1 supports standard, fast, high-speed modes, from 1 to 3300000.
    default: 100000
    minimum: 1
    maximum: 3300000

> 
> Regards,
> Samuel
> 
>> +    default: 100000
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c@d4010800 {
>> +        compatible = "spacemit,k1-i2c";
>> +        reg = <0x0 0xd4010800 0x0 0x38>;
>> +        interrupt-parent = <&plic>;
>> +        interrupts = <36>;
>> +        clocks = <&ccu 90>;
>> +        clock-frequency = <100000>;
>> +    };
>> +
>> +...
> 

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Samuel Holland 2 weeks, 4 days ago

Hi Troy,

On 2024-11-06 1:58 AM, Troy Mitchell wrote:
> On 2024/11/2 11:48, Samuel Holland wrote:
>> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>> and supports FIFO transmission.
>>>
>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>> ---
>>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>>>  1 file changed, 51 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>>
>>> 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 000000000000..57af66f494e7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>> @@ -0,0 +1,51 @@
>>> +# 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: 2
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>
>> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
>> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
>> looks to be standard across the peripherals in this SoC. Please be sure that the
>> binding covers all resources needed to use this peripheral.
>
> RCPU stands for Real-time CPU, which is typically used for low power consumption
> applications.
> We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
> user manual. However, you can find this register referenced in the K1 clock patch:
> https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/

Ah, well that driver is missing most of the bus clocks. For example, from a
quick comparison with the manual, the driver includes sdh_axi_aclk, but misses
all of the PWM APB clocks at APBC_PWMx_CLK_RST bit 0.

If the clock gate exists in the hardware, even if it is enabled by default, it
needs to be modeled in the devicetree.

> Also, to see how to enable the I2C clock in the device tree (note that the
> spacemit,apb_clock property is unused in the driver), check out the example here:
> https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048

The devicetree describes the hardware, irrespective of which features the driver
may or may not use.

>>> +
>>> +  clock-frequency:
>>> +    description:
>>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>>> +      modes are supported by hardware, possible values are 100000 and 400000.
>>> +    enum: [100000, 400000]
>>
>> This looks wrong. In the manual I see:
>>
>> * Supports standard-mode operation up to 100 Kbps
>> * Supports fast-mode operation up to 400Kbps
>> * Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
>> I2C only)
>> * Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
>> I2C only)
>>
>> So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
>> frequencies.
> okay. I will fix it in next version.
> and should I keep to ignore high-speed mode here?
> if not, how about this:
> 
>   clock-frequency:
>     description:
>       Desired I2C bus clock frequency in Hz.
>       K1 supports standard, fast, high-speed modes, from 1 to 3300000.
>     default: 100000
>     minimum: 1
>     maximum: 3300000

I don't know if high-speed mode should be included, since it requires some extra
negotiation to use. Assuming it should be, that looks reasonable.

Regards,
Samuel

>>
>> Regards,
>> Samuel
>>
>>> +    default: 100000
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    i2c@d4010800 {
>>> +        compatible = "spacemit,k1-i2c";
>>> +        reg = <0x0 0xd4010800 0x0 0x38>;
>>> +        interrupt-parent = <&plic>;
>>> +        interrupts = <36>;
>>> +        clocks = <&ccu 90>;
>>> +        clock-frequency = <100000>;
>>> +    };
>>> +
>>> +...
>>
>

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Haylen Chu 16 hours ago

On Wed, Nov 06, 2024 at 06:29:56PM -0600, Samuel Holland wrote:
Hi Samuel,

I'm working on the clock driver for Spacemit K1.

> On 2024-11-06 1:58 AM, Troy Mitchell wrote:
> > On 2024/11/2 11:48, Samuel Holland wrote:
> >> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
> >>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> >>> and supports FIFO transmission.
> >>>
> >>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> >>> ---
> >>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
> >>>  1 file changed, 51 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >>>
> >>> 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 000000000000..57af66f494e7
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >>> @@ -0,0 +1,51 @@
> >>> +# 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: 2
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>
> >> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
> >> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
> >> looks to be standard across the peripherals in this SoC. Please be sure that the
> >> binding covers all resources needed to use this peripheral.
> >
> > RCPU stands for Real-time CPU, which is typically used for low power consumption
> > applications.
> > We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
> > user manual.

The vendor missed the register definition of APBC_TWSIx_CLK_RST in the
documentation. There'll be an update soon.

> > However, you can find this register referenced in the K1 clock patch:
> > https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
> 
> Ah, well that driver is missing most of the bus clocks. For example, from a
> quick comparison with the manual, the driver includes sdh_axi_aclk, but misses
> all of the PWM APB clocks at APBC_PWMx_CLK_RST bit 0.

Thanks for pointing this out. Indeeded, the v2 of clock driver is
missing some bus clocks. I'm fixing them and working for a v3.

As for the I2C controller, I've confirmed with the vendor that both bus
and function clocks are required for normal operation. This applies for
all I2C controllers on the SoC, regardless of the region it belongs to
(RCPU/APBC).

> 
> If the clock gate exists in the hardware, even if it is enabled by default, it
> needs to be modeled in the devicetree.
> 
> > Also, to see how to enable the I2C clock in the device tree (note that the
> > spacemit,apb_clock property is unused in the driver), check out the example here:
> > https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048
> 
> The devicetree describes the hardware, irrespective of which features the driver
> may or may not use.

Best regards,
Haylen Chu

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Troy Mitchell 2 weeks, 4 days ago

On 2024/11/7 08:29, Samuel Holland wrote:
> Hi Troy,
> 
> On 2024-11-06 1:58 AM, Troy Mitchell wrote:
>> On 2024/11/2 11:48, Samuel Holland wrote:
>>> On 2024-10-28 12:32 AM, Troy Mitchell wrote:
>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>> and supports FIFO transmission.
>>>>
>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>> ---
>>>>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>>>>  1 file changed, 51 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>>>
>>>> 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 000000000000..57af66f494e7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>>>> @@ -0,0 +1,51 @@
>>>> +# 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: 2
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>
>>> Looking at the K1 user manual (9.1.4.77 RCPU I2C0 CLOCK RESET CONTROL
>>> REGISTER(RCPU_I2C0_CLK_RST)), I see two clocks (pclk, fclk) and a reset, which
>>> looks to be standard across the peripherals in this SoC. Please be sure that the
>>> binding covers all resources needed to use this peripheral.
>>
>> RCPU stands for Real-time CPU, which is typically used for low power consumption
>> applications.
>> We should be using the APBC_TWSIx_CLK_RST register, but it's not listed in the
>> user manual. However, you can find this register referenced in the K1 clock patch:
>> https://lore.kernel.org/all/SEYPR01MB4221AA2CA9C91A695FEFA777D7602@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
> 
> Ah, well that driver is missing most of the bus clocks. For example, from a
> quick comparison with the manual, the driver includes sdh_axi_aclk, but misses
> all of the PWM APB clocks at APBC_PWMx_CLK_RST bit 0.
> 
> If the clock gate exists in the hardware, even if it is enabled by default, it
> needs to be modeled in the devicetree.
I think you mean `APBCSCR`? It will keep enable all time.Just a difference in
frequency.

Should I add it in clockc property? If yes, how about this:
    clocks:
        maxItems: 1

and in dts example:
    i2c@d4010800 {
	...
        clocks = <&clk_apbc>, <&ccu 90>;
	clock-names = "clk_apbc", "clk_twsi";
        ...
    };
But one thing is, apbc_twsi is the sub-clock of clk_apbc, is this a duplicate
listing?
> 
>> Also, to see how to enable the I2C clock in the device tree (note that the
>> spacemit,apb_clock property is unused in the driver), check out the example here:
>> https://gitee.com/bianbu-linux/linux-6.1/blob/bl-v1.0.y/arch/riscv/boot/dts/spacemit/k1-x.dtsi#L1048
> 
> The devicetree describes the hardware, irrespective of which features the driver
> may or may not use.
> 
>>>> +
>>>> +  clock-frequency:
>>>> +    description:
>>>> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
>>>> +      modes are supported by hardware, possible values are 100000 and 400000.
>>>> +    enum: [100000, 400000]
>>>
>>> This looks wrong. In the manual I see:
>>>
>>> * Supports standard-mode operation up to 100 Kbps
>>> * Supports fast-mode operation up to 400Kbps
>>> * Supports high-speed mode (HS mode) slave operation up to 3.4Mbps(High-speed
>>> I2C only)
>>> * Supports high-speed mode (HS mode) master operation up to 3.3 Mbps (High-speed
>>> I2C only)
>>>
>>> So even ignoring HS mode, 100 kHz and 400 kHz are only the maximums, not fixed
>>> frequencies.
>> okay. I will fix it in next version.
>> and should I keep to ignore high-speed mode here?
>> if not, how about this:
>>
>>   clock-frequency:
>>     description:
>>       Desired I2C bus clock frequency in Hz.
>>       K1 supports standard, fast, high-speed modes, from 1 to 3300000.
>>     default: 100000
>>     minimum: 1
>>     maximum: 3300000
> 
> I don't know if high-speed mode should be included, since it requires some extra
> negotiation to use. Assuming it should be, that looks reasonable.
thanks.
> 
> Regards,
> Samuel
> 
>>>
>>> Regards,
>>> Samuel
>>>
>>>> +    default: 100000
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - clocks
>>>> +
>>>> +unevaluatedProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    i2c@d4010800 {
>>>> +        compatible = "spacemit,k1-i2c";
>>>> +        reg = <0x0 0xd4010800 0x0 0x38>;
>>>> +        interrupt-parent = <&plic>;
>>>> +        interrupts = <36>;
>>>> +        clocks = <&ccu 90>;
>>>> +        clock-frequency = <100000>;
>>>> +    };
>>>> +
>>>> +...
>>>
>>
> 

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Krzysztof Kozlowski 3 weeks, 4 days ago

On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---
>  .../bindings/i2c/spacemit,k1-i2c.yaml         | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> 
> 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 000000000000..57af66f494e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -0,0 +1,51 @@
> +# 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: 2

Provide changelog explaining why you are doing some changes.

List and describe items here instead.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description:
> +      Desired I2C bus clock frequency in Hz. As only fast and high-speed
> +      modes are supported by hardware, possible values are 100000 and 400000.
> +    enum: [100000, 400000]
> +    default: 100000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c@d4010800 {
> +        compatible = "spacemit,k1-i2c";
> +        reg = <0x0 0xd4010800 0x0 0x38>;

Missing second item.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Krzysztof Kozlowski 4 weeks ago

On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> ---

Where is the changelog? Nothing here, nothing in cover letter.

I asked for several changes, so now I don't know if you implemented
them.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Troy Mitchell 3 weeks, 6 days ago

On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>> and supports FIFO transmission.
>>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>> ---
> 
> Where is the changelog? Nothing here, nothing in cover letter.
> 
> I asked for several changes, so now I don't know if you implemented
> them.

I deleted the FIFO property because I believe your suggestion is correct.
this should be decided by the driver, even though the FIFO is provided
by the hardware.

Apologies for missing the changelog. To correct this, should I send a v3 
version with the changelog or resend v2?
> 
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Krzysztof Kozlowski 3 weeks, 4 days ago

On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
> > On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> >> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> >> and supports FIFO transmission.
> >>
> >> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> >> ---
> > 
> > Where is the changelog? Nothing here, nothing in cover letter.
> > 
> > I asked for several changes, so now I don't know if you implemented
> > them.
> 
> I deleted the FIFO property because I believe your suggestion is correct.
> this should be decided by the driver, even though the FIFO is provided
> by the hardware.
> 
> Apologies for missing the changelog. To correct this, should I send a v3 
> version with the changelog or resend v2?

Reply now with changelog. Your binding has some other unrelated and
incorrect changes, which I do not understand.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Troy Mitchell 2 weeks, 6 days ago

On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>> and supports FIFO transmission.
>>>>
>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>> ---
Change in v2:
 - Change the maxItems of reg from 1 to 2 in properties
 - Change 'i2c' to 'I2C' in the commit message.
 - Drop fifo-disable property
 - Drop alias in dts example
 - Move `unevaluatedProperties` after `required:` block

>>>
>>> Where is the changelog? Nothing here, nothing in cover letter.
>>>
>>> I asked for several changes, so now I don't know if you implemented
>>> them.
>>
>> I deleted the FIFO property because I believe your suggestion is correct.
>> this should be decided by the driver, even though the FIFO is provided
>> by the hardware.
>>
>> Apologies for missing the changelog. To correct this, should I send a v3 
>> version with the changelog or resend v2?
> 
> Reply now with changelog. Your binding has some other unrelated and
> incorrect changes, which I do not understand.
> 
> Best regards,
> Krzysztof
> 

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Krzysztof Kozlowski 2 weeks, 6 days ago

On 04/11/2024 14:01, Troy Mitchell wrote:
> 
> On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>> and supports FIFO transmission.
>>>>>
>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>> ---
> Change in v2:
>  - Change the maxItems of reg from 1 to 2 in properties

Why?

>  - Change 'i2c' to 'I2C' in the commit message.
>  - Drop fifo-disable property
>  - Drop alias in dts example
>  - Move `unevaluatedProperties` after `required:` block

Rest look ok.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Troy Mitchell 2 weeks, 6 days ago

On 2024/11/4 22:48, Krzysztof Kozlowski wrote:
> On 04/11/2024 14:01, Troy Mitchell wrote:
>>
>> On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
>>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>>> and supports FIFO transmission.
>>>>>>
>>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>>> ---
>> Change in v2:
>>  - Change the maxItems of reg from 1 to 2 in properties
> 
> Why?
I need the address and size. In v1, I wrote it as 1, and I got the make
dt_binding_check error.
> 
>>  - Change 'i2c' to 'I2C' in the commit message.
>>  - Drop fifo-disable property
>>  - Drop alias in dts example
>>  - Move `unevaluatedProperties` after `required:` block
> 
> Rest look ok.
> 
> Best regards,
> Krzysztof
> 

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Samuel Holland 2 weeks, 6 days ago

Hi Troy,

On 2024-11-04 7:14 PM, Troy Mitchell wrote:
> 
> On 2024/11/4 22:48, Krzysztof Kozlowski wrote:
>> On 04/11/2024 14:01, Troy Mitchell wrote:
>>>
>>> On 2024/10/31 16:00, Krzysztof Kozlowski wrote:
>>>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>>>> and supports FIFO transmission.
>>>>>>>
>>>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>>>> ---
>>> Change in v2:
>>>  - Change the maxItems of reg from 1 to 2 in properties
>>
>> Why?
> I need the address and size. In v1, I wrote it as 1, and I got the make
> dt_binding_check error.

One "<address size>" element is just one item. maxItems > 1 would be for
hardware with multiple discontiguous register ranges: <address1 size1>,
<address2 size2>.

Regards,
Samuel

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Troy Mitchell 2 weeks, 6 days ago

On 2024/11/5 10:50, Samuel Holland wrote:
> One "<address size>" element is just one item. maxItems > 1 would be for
> hardware with multiple discontiguous register ranges: <address1 size1>,
> <address2 size2>.
got it.I will fix it in next version.
> 
> Regards,
> Samuel

-- 
Troy Mitchell

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Jesse T 3 weeks, 2 days ago

On Fri, Nov 1, 2024 at 10:24 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
> > On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
> > > On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
> > >> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> > >> and supports FIFO transmission.
> > >>
> > >> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> > >> ---

Change in v2:
- drop fifo-disable property
- unevaluatedProperties goes after required: block
- drop alias

> > >
> > > Where is the changelog? Nothing here, nothing in cover letter.
It seems like it was accidentally dropped seeing there are the dashes.

> > >
> > > I asked for several changes, so now I don't know if you implemented
> > > them.
> >
> > I deleted the FIFO property because I believe your suggestion is correct.
> > this should be decided by the driver, even though the FIFO is provided
> > by the hardware.
> >
> > Apologies for missing the changelog. To correct this, should I send a v3
> > version with the changelog or resend v2?
>
> Reply now with changelog. Your binding has some other unrelated and
> incorrect changes, which I do not understand.

...

Thanks,
Jesse Taube

>
> Best regards,
> Krzysztof
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

Posted by Krzysztof Kozlowski 3 weeks, 2 days ago

On 01/11/2024 15:29, Jesse T wrote:
> On Fri, Nov 1, 2024 at 10:24 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Tue, Oct 29, 2024 at 04:36:00PM +0800, Troy Mitchell wrote:
>>> On 2024/10/28 15:38, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 28, 2024 at 01:32:19PM +0800, Troy Mitchell wrote:
>>>>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>>>>> and supports FIFO transmission.
>>>>>
>>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>>> ---
> 
> Change in v2:
> - drop fifo-disable property
> - unevaluatedProperties goes after required: block
> - drop alias

b4 diff disagrees with you :/

Best regards,
Krzysztof