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

Troy Mitchell
2024-11-07T01:58:02Z

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