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

[Troy Mitchell <troymitchell988@gmail.com>]

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