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

[Troy Mitchell <troymitchell988@gmail.com>]

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