Re: [PATCH v1 1/3] dt-bindings: i2c: snps,dwc-i2c: Add StarFive JHB100 bindings

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5813 bytes --]

On Thu, May 21, 2026 at 11:43:38AM +0800, lianfeng.ouyang wrote:
> From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> 
> Add device tree bindings for the Synopsys DesignWare Core (DWC) I2C
> controller and its StarFive JHB100 implementation
> 
> The binding introduces a new compatible string: "snps,dwc-i2c", intended
> for the generic IP. It also defines two platform-specific compatibles
> for the StarFive JHB100 implementation:
> - "starfive,jhb100-dwc-i2c-master"
> - "starfive,jhb100-dwc-i2c-slave"

Do you have two different i2c controllers on the device, one which
implements only slave mode and one that only implements master?
Or can the same controller be both master or slave depending on how the
user wants to use it?

> 
> The controller supports standard I2C and SMBus protocols, programmable
> FIFO depths, and optional SMBus Alert routing. The binding documents
> the necessary clocks, resets, and timing properties.
> 
> Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> ---
>  .../devicetree/bindings/i2c/snps,dwc-i2c.yaml | 120 ++++++++++++++++++
>  1 file changed, 120 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/snps,dwc-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/snps,dwc-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,dwc-i2c.yaml
> new file mode 100644
> index 000000000000..7227f24f7cbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/snps,dwc-i2c.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2024 StarFive Technology Co., Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/snps,dwc-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DWC I2C Controller
> +
> +maintainers:
> +  - Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description: Generic Synopsys DWC I2C controller
> +        const: snps,dwc-i2c

I think you should delete this, we don't want to permit avoiding using
soc-specific compatibles.

Why can't this go into the existing designware i2c binding?

> +      - description: StarFive JHB100 I2C master controller
> +        items:
> +          - const: starfive,jhb100-dwc-i2c-master
> +          - const: snps,dwc-i2c

This fallback needs to be a lot more specific about what the revision
is, so that people can figure out which fallback applies to them.

> +      - description: StarFive JHB100 I2C slave controller
> +        items:
> +          - const: starfive,jhb100-dwc-i2c-slave
> +          - const: snps,dwc-i2c
> +
> +  reg:
> +    description: DWC I2C controller memory mapped registers
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    items:
> +      - description: I2C controller reference clock source
> +      - description: APB interface clock source
> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: ref
> +      - const: pclk

Please add a conditional section that sets the correct number of clocks
for your jhb100.

> +
> +  resets:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: Desired I2C bus clock frequency in Hz
> +    enum: [100000, 400000, 1000000, 3400000]
> +    default: 400000
> +
> +  i2c-sda-hold-time-ns:
> +    description: |
> +      The property should contain the SDA hold time in nanoseconds.
> +      This value is used to compute value written into DW_IC_SDA_HOLD register.

Missing a default here.

> +
> +  i2c-scl-falling-time-ns:
> +    description: |
> +      The property should contain the SCL falling time in nanoseconds.
> +      This value is used to compute the tLOW period.
> +    default: 300
> +
> +  i2c-sda-falling-time-ns:
> +    description: |
> +      The property should contain the SDA falling time in nanoseconds.
> +      This value is used to compute the tHIGH period.
> +    default: 300
> +
> +  starfive,mctp-i2c-ms:
> +    description: |
> +      The property should contain reference to the master node associated with the slave.
> +      This value is only used in slave mode, especially for MCTP application.

This property is missing a type, but I also don't understand what it is
for. You shouldn't need to know what the i2c master is.
I assume it is meant to be a phandle? Can you share an example dts
that contains this property in use?

> +
> +  dwc-i2c-tx-fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      The property describes the tx fifo depth.
> +    default: 8
> +
> +  dwc-i2c-rx-fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      The property describes the rx fifo depth.
> +    default: 8
> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts

clocks and clock-names too.

Bunch of valid complaints from sashiko on this, so
pw-bot: changes-requested

Thanks,
Conor.
> +
> +examples:
> +  - |
> +    i2c@f0000 {
> +      compatible = "snps,dwc-i2c";
> +      reg = <0xf0000 0x1000>;
> +      interrupts = <11>;
> +      clock-frequency = <400000>;
> +    };
> +  - |
> +    i2c@2000 {
> +      compatible = "snps,dwc-i2c";
> +      reg = <0x2000 0x100>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      clock-frequency = <400000>;
> +      clocks = <&i2cclk>;
> +      interrupts = <0>;
> +
> +      eeprom@64 {
> +        compatible = "atmel,24c02";
> +        reg = <0x64>;
> +      };
> +    };
> +...
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]