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

[Haylen Chu <heylenay@4d2.org>]

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