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@xxxxxxxxx> >>> --- >>> .../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@xxxxxxxxx> >>> + >>> +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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ 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>; >>> + }; >>> + >>> +... >> >
