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

Troy Mitchell
2024-11-06T08:03:49Z

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