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

[Lianfeng Ouyang]

Best Regards,
Lianfeng Ouyang

> -----邮件原件-----
> 发件人: Conor Dooley <conor@xxxxxxxxxx>
> 发送时间: 2026年5月22日 4:23
> 收件人: Lianfeng Ouyang <lianfeng.ouyang@xxxxxxxxxxxxxxxx>
> 抄送: Andi Shyti <andi.shyti@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>;
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Jan Dabros
> <jsd@xxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 主题: Re: [PATCH v1 1/3] dt-bindings: i2c: snps,dwc-i2c: Add StarFive JHB100
> bindings
> 
> On Thu, May 21, 2026 at 11:43:38AM +0800, lianfeng.ouyang wrote:
> > From: Lianfeng Ouyang <lianfeng.ouyang@xxxxxxxxxxxxxxxx>
> >
> > 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?
> 

There are two different i2c controllers on the device, one which
implements only slave mode and one that only implements master?

> >
> > 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@xxxxxxxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxxxxxx>
> > +
> > +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?

According to Mika's suggestion, it has now been changed to i2c starry - *. 
Cannot go into the existing designware i2c binding because this controller is
a variant of designware i2c, and their register addresses and bit layouts 
differ greatly, so it cannot directly reuse existing designware i2c

> 
> > +      - 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.

Thank you for the reminder. I will add some explanations

> 
> > +      - 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.

OK, thanks

> 
> > +
> > +  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.

really

> 
> > +
> > +  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?

Sorry, I will add examples of types in the next version

> 
> > +
> > +  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.

the next version will be revised according to your suggestions. Thank you for your correction

> > +
> > +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
> >