* [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support
@ 2026-05-27 8:50 lianfeng.ouyang
2026-05-27 8:50 ` [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C lianfeng.ouyang
` (4 more replies)
0 siblings, 5 replies; 25+ messages in thread
From: lianfeng.ouyang @ 2026-05-27 8:50 UTC (permalink / raw)
To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Mika Westerberg, Andy Shevchenko
Cc: linux-i2c, devicetree, linux-kernel, Lianfeng Ouyang
From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
The Starfive JHB100 I2C controller is a variant of the widely-used
DesignWare I2C IP, with a distinct register layout and enhanced features
such as SMBus Alert and programmable FIFO depths.
The series is structured as follows:
1. Adds the device tree binding document for the starfive,jhb100-i2c
compatible.
2. Prepares the existing i2c-designware-core by exporting and making
certain key functions overridable, allowing code reuse.
3. Introduces the new i2c-starfive-* driver, with separate modules for
master and slave functionality, based on the 2023-07 revision of
the Synopsys IP manual.
Currently, due to the following differences, i2c designware cannot be
fully reused
1. For high and low level counting settings at different rates, i2c
starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM
2. Interrupt clearing is achieved by writing 1 to the corresponding
bit of INTR_CLR, while designware reads different clearing
registers
3. Master and slave require separate probe callbacks and cannot rely
solely on the runtime mode switching provided by i2c_dw_set_mode()
4. The value of FIFO depth is not obtained through registers, but
written through DTS
I have written some poorly styled code to reduce changes to i2c designware
and reuse its functions by keeping aa always true, for example
1. the implementation of i2c-d w_probe_master() differs only for the two
IPs in i2c_dw_set_timits_master(). In order to reuse
i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as
__weak. A better approach is to use a callback function, but using
a callback function requires changing more i2c designware files.
I don't know what the attitude of the community is
2. For the operation of clearing interrupt flags, i2c designware reads
and i2c starfive writes. Therefore, in order not to modify the
relevant logic of i2c designware, I added a write operation to
sf_reg_read()
So I think this version of the code is not allowed to merge, but I don't
know how to handle this situation because if i2c designware is not changed
at all, we will have to write code that is similar to i2c designware.
Will this type of IP not be allowed to merge?
Lianfeng Ouyang (3):
dt-bindings: i2c: Add StarFive JHB100 I2C
i2c: designware: Export symbols and add __weak for Starfive I2C driver
i2c: starfive: Add StarFive JHB100 I2C master/slave support
.../bindings/i2c/starfive,jhb100-i2c.yaml | 128 +++++
MAINTAINERS | 7 +
drivers/i2c/busses/Kconfig | 24 +
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-designware-common.c | 72 ++-
drivers/i2c/busses/i2c-designware-core.h | 20 +
drivers/i2c/busses/i2c-designware-master.c | 4 +-
drivers/i2c/busses/i2c-designware-platdrv.c | 6 +
drivers/i2c/busses/i2c-designware-slave.c | 2 +-
drivers/i2c/busses/i2c-starfive-core.h | 164 ++++++
drivers/i2c/busses/i2c-starfive-master.c | 470 ++++++++++++++++++
drivers/i2c/busses/i2c-starfive-slave.c | 180 +++++++
12 files changed, 1060 insertions(+), 20 deletions(-)
create mode 100644 Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml
create mode 100644 drivers/i2c/busses/i2c-starfive-core.h
create mode 100644 drivers/i2c/busses/i2c-starfive-master.c
create mode 100644 drivers/i2c/busses/i2c-starfive-slave.c
--
2.43.0
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C 2026-05-27 8:50 [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support lianfeng.ouyang @ 2026-05-27 8:50 ` lianfeng.ouyang 2026-05-27 13:04 ` Krzysztof Kozlowski 2026-05-27 8:50 ` [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver lianfeng.ouyang ` (3 subsequent siblings) 4 siblings, 1 reply; 25+ messages in thread From: lianfeng.ouyang @ 2026-05-27 8:50 UTC (permalink / raw) To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c, devicetree, linux-kernel, Lianfeng Ouyang From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> Add device tree bindings for the Starfive I2C controller and its implementation The binding defines two platform-specific compatibles for the StarFive JHB100 implementation: - "starfive,jhb100-i2c-master" - "starfive,jhb100-i2c-slave" 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> --- .../bindings/i2c/starfive,jhb100-i2c.yaml | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml diff --git a/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml new file mode 100644 index 000000000000..c8631348121c --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml @@ -0,0 +1,128 @@ +# 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/starfive,jhb100-i2c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: StarFive jhb100 I2C Controller + +maintainers: + - Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + description: | + Must be one of: + - "starfive,jhb100-i2c-master" for master mode controller + - "starfive,jhb100-i2c-slave" for slave mode controller + enum: + - starfive,jhb100-i2c-master + - starfive,jhb100-i2c-slave + + reg: + maxItems: 1 + description: StarFive I2C controller memory mapped registers + + interrupts: + maxItems: 1 + + clocks: + minItems: 2 + maxItems: 2 + items: + - description: I2C controller reference clock source + - description: APB interface clock source + + clock-names: + minItems: 2 + maxItems: 2 + items: + - const: ref + - const: pclk + + resets: + maxItems: 1 + description: Phandle to the reset controller for the I2C controller + + clock-frequency: + description: Desired I2C bus clock frequency in Hz + enum: [100000, 400000, 1000000, 3400000] + default: 400000 + + i2c-sda-hold-time-ns: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + SDA hold time in nanoseconds, used to compute the value written into + the DW_IC_SDA_HOLD register. + default: 0 + + i2c-scl-falling-time-ns: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + SCL falling time in nanoseconds, used to compute the tLOW period. + default: 300 + + i2c-sda-falling-time-ns: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + SDA falling time in nanoseconds, used to compute the tHIGH period. + default: 300 + + starfive,mctp-i2c-ms: + $ref: /schemas/types.yaml#/definitions/phandle + description: | + Phandle to the master I2C node associated with this slave controller. + Only used in slave mode, typically for MCTP applications. + + starfive,i2c-tx-fifo-depth: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + TX FIFO depth of the I2C controller. + default: 8 + + starfive,i2c-rx-fifo-depth: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + RX FIFO depth of the I2C controller. + default: 8 + +unevaluatedProperties: false + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts + +examples: + - | + i2c@2000 { + compatible = "starfive,jhb100-i2c-master"; + reg = <0x2000 0x100>; + #address-cells = <1>; + #size-cells = <0>; + clock-frequency = <400000>; + clocks = <&i2c_refclk>, <&apb_pclk>; + clock-names = "ref", "pclk"; + interrupts = <0>; + + eeprom@64 { + compatible = "atmel,24c02"; + reg = <0x64>; + }; + }; + | + i2c@3000 { + compatible = "starfive,jhb100-i2c-slave"; + reg = <0x3000 0x100>; + clocks = <&i2c_refclk>, <&apb_pclk>; + clock-names = "ref", "pclk"; + interrupts = <1>; + starfive,mctp-i2c-ms = <&i2c_master>; + }; +... -- 2.43.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C 2026-05-27 8:50 ` [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C lianfeng.ouyang @ 2026-05-27 13:04 ` Krzysztof Kozlowski 2026-05-28 6:57 ` 回复: " Lianfeng Ouyang 0 siblings, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2026-05-27 13:04 UTC (permalink / raw) To: lianfeng.ouyang, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c, devicetree, linux-kernel On 27/05/2026 10:50, lianfeng.ouyang wrote: > From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > > Add device tree bindings for the Starfive I2C controller > and its implementation > > The binding defines two platform-specific compatibles for the StarFive > JHB100 implementation: > - "starfive,jhb100-i2c-master" > - "starfive,jhb100-i2c-slave" You might get the same questions as v1, till you solve them. Why do you add device role to the compatible? Do not explain WHAT you did here. Explain the background and why you did that way. Also, use undeprecated naming, not master/slave. > > 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> > --- > .../bindings/i2c/starfive,jhb100-i2c.yaml | 128 ++++++++++++++++++ > 1 file changed, 128 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml > > diff --git a/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml > new file mode 100644 > index 000000000000..c8631348121c > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml > @@ -0,0 +1,128 @@ > +# 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/starfive,jhb100-i2c.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: StarFive jhb100 I2C Controller > + > +maintainers: > + - Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > + > +allOf: > + - $ref: /schemas/i2c/i2c-controller.yaml# > + > +properties: > + compatible: > + description: | > + Must be one of: > + - "starfive,jhb100-i2c-master" for master mode controller > + - "starfive,jhb100-i2c-slave" for slave mode controller Drop description, redundant. Schema tells that. > + enum: > + - starfive,jhb100-i2c-master > + - starfive,jhb100-i2c-slave > + > + reg: > + maxItems: 1 > + description: StarFive I2C controller memory mapped registers Drop description, redundant. > + > + interrupts: > + maxItems: 1 > + > + clocks: > + minItems: 2 > + maxItems: 2 Drop both, redundant. > + items: > + - description: I2C controller reference clock source > + - description: APB interface clock source > + > + clock-names: > + minItems: 2 > + maxItems: 2 Same. From where did you take such syntax... > + items: > + - const: ref > + - const: pclk > + > + resets: > + maxItems: 1 > + description: Phandle to the reset controller for the I2C controller Drop description. Or say something useful. > + > + clock-frequency: > + description: Desired I2C bus clock frequency in Hz > + enum: [100000, 400000, 1000000, 3400000] Aren't you duplicating constraints from dtschema? > + default: 400000 > + > + i2c-sda-hold-time-ns: So you added a generic property - where is it documented? Generic properties must be in common schema or dtschema. And please prove that none of the generic properties are suitable. > + $ref: /schemas/types.yaml#/definitions/uint32 I don't think you tested it. And this concludes my review. I finished here. Please do not send untested bindings. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C 2026-05-27 13:04 ` Krzysztof Kozlowski @ 2026-05-28 6:57 ` Lianfeng Ouyang 2026-05-28 7:47 ` Krzysztof Kozlowski 0 siblings, 1 reply; 25+ messages in thread From: Lianfeng Ouyang @ 2026-05-28 6:57 UTC (permalink / raw) To: Krzysztof Kozlowski, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Hi, Krzysztof Thanks for the review. > -----邮件原件----- > 发件人: Krzysztof Kozlowski <krzk@kernel.org> > 发送时间: 2026年5月27日 21:05 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>; Andi Shyti > <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mika > Westerberg <mika.westerberg@linux.intel.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com> > 抄送: linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C > > On 27/05/2026 10:50, lianfeng.ouyang wrote: > > From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > > > > Add device tree bindings for the Starfive I2C controller > > and its implementation > > > > The binding defines two platform-specific compatibles for the StarFive > > JHB100 implementation: > > - "starfive,jhb100-i2c-master" > > - "starfive,jhb100-i2c-slave" > > You might get the same questions as v1, till you solve them. Why do you > add device role to the compatible? > > Do not explain WHAT you did here. Explain the background and why you > did that way. > > Also, use undeprecated naming, not master/slave. > > > > > 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> > > --- > > .../bindings/i2c/starfive,jhb100-i2c.yaml | 128 ++++++++++++++++++ > > 1 file changed, 128 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml > > > > diff --git a/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml > b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml > > new file mode 100644 > > index 000000000000..c8631348121c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml > > @@ -0,0 +1,128 @@ > > +# 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/starfive,jhb100-i2c.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: StarFive jhb100 I2C Controller > > + > > +maintainers: > > + - Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > > + > > +allOf: > > + - $ref: /schemas/i2c/i2c-controller.yaml# > > + > > +properties: > > + compatible: > > + description: | > > + Must be one of: > > + - "starfive,jhb100-i2c-master" for master mode controller > > + - "starfive,jhb100-i2c-slave" for slave mode controller > > Drop description, redundant. Schema tells that. > > > + enum: > > + - starfive,jhb100-i2c-master > > + - starfive,jhb100-i2c-slave > > + > > + reg: > > + maxItems: 1 > > + description: StarFive I2C controller memory mapped registers > > Drop description, redundant. > > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + minItems: 2 > > + maxItems: 2 > > Drop both, redundant. > > > + items: > > + - description: I2C controller reference clock source > > + - description: APB interface clock source > > + > > + clock-names: > > + minItems: 2 > > + maxItems: 2 > > Same. From where did you take such syntax... > > > + items: > > + - const: ref > > + - const: pclk > > + > > + resets: > > + maxItems: 1 > > + description: Phandle to the reset controller for the I2C controller > > Drop description. Or say something useful. > > > + > > + clock-frequency: > > + description: Desired I2C bus clock frequency in Hz > > + enum: [100000, 400000, 1000000, 3400000] > > Aren't you duplicating constraints from dtschema? > > > + default: 400000 > > + > > + i2c-sda-hold-time-ns: > > So you added a generic property - where is it documented? Generic > properties must be in common schema or dtschema. > > And please prove that none of the generic properties are suitable. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > I don't think you tested it. And this concludes my review. I finished > here. Please do not send untested bindings. > > > Best regards, > Krzysztof Sorry, I didn't know I needed to run dt-binding_check before, but now I have set up the environment. Starting from the next version, I will run dt-binding_check and modify it according to your reply. Then, I will remove the description of the required system properties and the common i2c properties Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 回复: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C 2026-05-28 6:57 ` 回复: " Lianfeng Ouyang @ 2026-05-28 7:47 ` Krzysztof Kozlowski 2026-05-28 8:30 ` 回复: " Lianfeng Ouyang 0 siblings, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2026-05-28 7:47 UTC (permalink / raw) To: Lianfeng Ouyang, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org On 28/05/2026 08:57, Lianfeng Ouyang wrote: >>> + default: 400000 >>> + >>> + i2c-sda-hold-time-ns: >> >> So you added a generic property - where is it documented? Generic >> properties must be in common schema or dtschema. >> >> And please prove that none of the generic properties are suitable. >> >>> + $ref: /schemas/types.yaml#/definitions/uint32 >> >> I don't think you tested it. And this concludes my review. I finished >> here. Please do not send untested bindings. >> >> >> Best regards, >> Krzysztof > > Sorry, I didn't know I needed to run dt-binding_check before, but now I have Every doc asks that. DTS101 slides or my earlier talks as well... Other people talks as well... and you received that request last time at v1. Please do not repeat the same mistakes because then we feel like our feedback is wasted. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: 回复: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C 2026-05-28 7:47 ` Krzysztof Kozlowski @ 2026-05-28 8:30 ` Lianfeng Ouyang 0 siblings, 0 replies; 25+ messages in thread From: Lianfeng Ouyang @ 2026-05-28 8:30 UTC (permalink / raw) To: Krzysztof Kozlowski, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Krzysztof Kozlowski <krzk@kernel.org> > 发送时间: 2026年5月28日 15:48 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>; Andi Shyti > <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mika > Westerberg <mika.westerberg@linux.intel.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com> > 抄送: linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: 回复: [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C > > On 28/05/2026 08:57, Lianfeng Ouyang wrote: > >>> + default: 400000 > >>> + > >>> + i2c-sda-hold-time-ns: > >> > >> So you added a generic property - where is it documented? Generic > >> properties must be in common schema or dtschema. > >> > >> And please prove that none of the generic properties are suitable. > >> > >>> + $ref: /schemas/types.yaml#/definitions/uint32 > >> > >> I don't think you tested it. And this concludes my review. I finished > >> here. Please do not send untested bindings. > >> > >> > >> Best regards, > >> Krzysztof > > > > Sorry, I didn't know I needed to run dt-binding_check before, but now I have > > Every doc asks that. DTS101 slides or my earlier talks as well... Other > people talks as well... and you received that request last time at v1. > Please do not repeat the same mistakes because then we feel like our > feedback is wasted. > > Best regards, > Krzysztof I'm very sorry, I know that language is not strong at this moment. In order to prevent your feedback from being wasted, I will correct the relevant errors and run tests in the next version. Thank you Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-05-27 8:50 [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support lianfeng.ouyang 2026-05-27 8:50 ` [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C lianfeng.ouyang @ 2026-05-27 8:50 ` lianfeng.ouyang 2026-05-27 13:07 ` Krzysztof Kozlowski 2026-06-02 22:24 ` Andy Shevchenko 2026-05-27 8:50 ` [PATCH v2 3/3] i2c: starfive: Add StarFive JHB100 I2C master/slave support lianfeng.ouyang ` (2 subsequent siblings) 4 siblings, 2 replies; 25+ messages in thread From: lianfeng.ouyang @ 2026-05-27 8:50 UTC (permalink / raw) To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c, devicetree, linux-kernel, Lianfeng Ouyang From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> Export several key functions (i2c_dw_probe_master, i2c_dw_init, i2c_dw_xfer_init, i2c_dw_read_clear_intrbits, etc.) and mark them as __weak. This allows the i2c starfive driver to reuse the common infrastructure while overriding the implementations where needed. Additionally, extend the register map configuration and introduce the MODEL_STARFIVE flag to accommodate the starfive i2c IP's different register space. Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> --- drivers/i2c/busses/i2c-designware-common.c | 72 ++++++++++++++++----- drivers/i2c/busses/i2c-designware-core.h | 20 ++++++ drivers/i2c/busses/i2c-designware-master.c | 4 +- drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++ drivers/i2c/busses/i2c-designware-slave.c | 2 +- 5 files changed, 84 insertions(+), 20 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 4dc57fd56170..034f6a446ab6 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -70,6 +70,7 @@ static const char *const abort_sources[] = { "incorrect slave-transmitter mode configuration", }; +#if !IS_ENABLED(CONFIG_I2C_STARFIVE) static int dw_reg_read(void *context, unsigned int reg, unsigned int *val) { struct dw_i2c_dev *dev = context; @@ -87,6 +88,7 @@ static int dw_reg_write(void *context, unsigned int reg, unsigned int val) return 0; } +#endif static int dw_reg_read_swab(void *context, unsigned int reg, unsigned int *val) { @@ -143,9 +145,15 @@ static int i2c_dw_init_regmap(struct dw_i2c_dev *dev) .val_bits = 32, .reg_stride = 4, .disable_locking = true, +#if IS_ENABLED(CONFIG_I2C_STARFIVE) + .reg_read = sf_reg_read, + .reg_write = sf_reg_write, + .max_register = SF_IC_SMBUS_INTR_CLR, +#else .reg_read = dw_reg_read, .reg_write = dw_reg_write, .max_register = DW_IC_COMP_TYPE, +#endif }; u32 reg; int ret; @@ -162,6 +170,10 @@ static int i2c_dw_init_regmap(struct dw_i2c_dev *dev) return ret; reg = readl(dev->base + DW_IC_COMP_TYPE); + + if ((dev->flags & MODEL_MASK) == MODEL_STARFIVE) + reg = readl(dev->base + (SF_IC_COMP_TYPE & (~SF_REG_FLAG))); + i2c_dw_release_lock(dev); if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) @@ -359,7 +371,7 @@ static inline u32 i2c_dw_acpi_round_bus_speed(struct device *device) { return 0; #endif /* CONFIG_ACPI */ -static void i2c_dw_configure_mode(struct dw_i2c_dev *dev, int mode) +__weak void i2c_dw_configure_mode(struct dw_i2c_dev *dev, int mode) { switch (mode) { case DW_IC_MASTER: @@ -382,7 +394,7 @@ static void i2c_dw_configure_mode(struct dw_i2c_dev *dev, int mode) } } -static void i2c_dw_write_timings(struct dw_i2c_dev *dev) +__weak void i2c_dw_write_timings(struct dw_i2c_dev *dev) { /* Write standard speed timing parameters */ regmap_write(dev->map, DW_IC_SS_SCL_HCNT, dev->ss_hcnt); @@ -411,7 +423,7 @@ static void i2c_dw_write_timings(struct dw_i2c_dev *dev) * * The controller must be disabled before this function is called. */ -void i2c_dw_set_mode(struct dw_i2c_dev *dev, int mode) +__weak void i2c_dw_set_mode(struct dw_i2c_dev *dev, int mode) { if (mode == DW_IC_SLAVE && !dev->slave) mode = DW_IC_MASTER; @@ -806,10 +818,25 @@ static int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev) if (ret) return ret; +#if IS_ENABLED(CONFIG_I2C_STARFIVE) + u32 tx_fifo_cfg = 8, rx_fifo_cfg = 8; + +#ifdef CONFIG_OF + ret = of_property_read_u32(dev->dev->of_node, "starfive,i2c-tx-fifo-depth", &tx_fifo_cfg); + if (!ret && (tx_fifo_cfg < 2 || tx_fifo_cfg > 256)) + tx_fifo_cfg = 8; + + ret = of_property_read_u32(dev->dev->of_node, "starfive,i2c-rx-fifo-depth", &rx_fifo_cfg); + if (!ret && (rx_fifo_cfg < 2 || rx_fifo_cfg > 256)) + rx_fifo_cfg = 8; +#endif + param = rx_fifo_cfg << 8 | tx_fifo_cfg << 16; +#else ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, ¶m); i2c_dw_release_lock(dev); if (ret) return ret; +#endif tx_fifo_depth = FIELD_GET(DW_IC_FIFO_TX_FIELD, param) + 1; rx_fifo_depth = FIELD_GET(DW_IC_FIFO_RX_FIELD, param) + 1; @@ -896,20 +923,29 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) if (ret) return ret; - ret = i2c_dw_probe_master(dev); - if (ret) - return ret; + if (dev->mode == DW_IC_SLAVE) { + ret = i2c_sf_probe_slave(dev); + } else { + ret = i2c_dw_probe_master(dev); + if (ret) + return ret; - ret = i2c_dw_init(dev); - if (ret) - return ret; + ret = i2c_dw_init(dev); + if (ret) + return ret; - if (!adap->name[0]) - strscpy(adap->name, "Synopsys DesignWare I2C adapter"); + if (!adap->name[0]) + strscpy(adap->name, "Synopsys DesignWare I2C adapter"); + } adap->retries = 3; adap->algo = &i2c_dw_algo; +#if IS_ENABLED(CONFIG_I2C_STARFIVE_SLAVE) + if (dev->mode == DW_IC_SLAVE) + adap->algo = &i2c_dw_slave_algo; +#else adap->quirks = &i2c_dw_quirks; +#endif adap->dev.parent = dev->dev; i2c_set_adapdata(adap, dev); @@ -938,16 +974,18 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) if (!dev->emptyfifo_hold_master) irq_flags |= IRQF_NO_THREAD; - ret = i2c_dw_acquire_lock(dev); - if (ret) - return ret; + if (!IS_ENABLED(CONFIG_I2C_STARFIVE) || dev->mode == DW_IC_MASTER) { + ret = i2c_dw_acquire_lock(dev); + if (ret) + return ret; - __i2c_dw_write_intr_mask(dev, 0); - i2c_dw_release_lock(dev); + __i2c_dw_write_intr_mask(dev, 0); + i2c_dw_release_lock(dev); + } if (!(dev->flags & ACCESS_POLLING)) { ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, - irq_flags, dev_name(dev->dev), dev); + irq_flags, dev_name(dev->dev), dev); if (ret) return ret; } diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 9d8d104cc391..87223f7e28aa 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -321,6 +321,11 @@ struct dw_i2c_dev { u32 bus_capacitance_pF; bool clk_freq_optimized; bool emptyfifo_hold_master; +#if IS_ENABLED(CONFIG_I2C_STARFIVE) + u16 scl_hcnt; + u16 scl_lcnt; + struct i2c_adapter *ms_adapter; /* Bind another I2C master controller */ +#endif }; #define ACCESS_INTR_MASK BIT(0) @@ -328,6 +333,7 @@ struct dw_i2c_dev { #define ARBITRATION_SEMAPHORE BIT(2) #define ACCESS_POLLING BIT(3) +#define MODEL_STARFIVE BIT(9) #define MODEL_AMD_NAVI_GPU BIT(10) #define MODEL_WANGXUN_SP BIT(11) #define MODEL_MASK GENMASK(11, 8) @@ -359,8 +365,14 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev); u32 i2c_dw_func(struct i2c_adapter *adap); irqreturn_t i2c_dw_isr_master(struct dw_i2c_dev *dev); +u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev); +u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev); + extern const struct dev_pm_ops i2c_dw_dev_pm_ops; +#if IS_ENABLED(CONFIG_I2C_STARFIVE) +#include "i2c-starfive-core.h" +#else static inline void __i2c_dw_enable(struct dw_i2c_dev *dev) { dev->status |= STATUS_ACTIVE; @@ -372,6 +384,7 @@ static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev) regmap_write(dev->map, DW_IC_ENABLE, 0); dev->status &= ~STATUS_ACTIVE; } +#endif static inline void __i2c_dw_write_intr_mask(struct dw_i2c_dev *dev, unsigned int intr_mask) @@ -411,8 +424,15 @@ static inline irqreturn_t i2c_dw_isr_slave(struct dw_i2c_dev *dev) { return IRQ_ static inline void i2c_dw_configure(struct dw_i2c_dev *dev) { +#if IS_ENABLED(CONFIG_I2C_STARFIVE) + if (device_is_compatible(dev->dev, "starfive,jhb100-i2c-slave")) + i2c_sf_configure_slave(dev); + else + i2c_sf_configure_master(dev); +#else i2c_dw_configure_slave(dev); i2c_dw_configure_master(dev); +#endif } int i2c_dw_probe(struct dw_i2c_dev *dev); diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index de929b91d5ea..f27c0fc761da 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -31,7 +31,7 @@ #define AMD_TIMEOUT_MAX_US 250 #define AMD_MASTERCFG_MASK GENMASK(15, 0) -static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev) +__weak int i2c_dw_set_timings_master(struct dw_i2c_dev *dev) { unsigned int comp_param1; u32 sda_falling_time, scl_falling_time; @@ -570,7 +570,7 @@ i2c_dw_read(struct dw_i2c_dev *dev) } } -static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev) +__weak u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev) { unsigned int stat, dummy; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 426ffec06e22..8c0f5c39e4ff 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -151,6 +151,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) if (device_property_present(device, "wx,i2c-snps-model")) flags = MODEL_WANGXUN_SP | ACCESS_POLLING; + if (device_is_compatible(device, "starfive,jhb100-i2c-master") || + device_is_compatible(device, "starfive,jhb100-i2c-slave")) + flags |= MODEL_STARFIVE; + dev->dev = device; dev->irq = irq; dev->flags = flags; @@ -255,6 +259,8 @@ static const struct of_device_id dw_i2c_of_match[] = { { .compatible = "mobileye,eyeq6lplus-i2c" }, { .compatible = "mscc,ocelot-i2c" }, { .compatible = "snps,designware-i2c" }, + { .compatible = "starfive,jhb100-i2c-master" }, + { .compatible = "starfive,jhb100-i2c-slave" }, {} }; MODULE_DEVICE_TABLE(of, dw_i2c_of_match); diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c index ad0d5fbfa6d5..1b94e4fbaad1 100644 --- a/drivers/i2c/busses/i2c-designware-slave.c +++ b/drivers/i2c/busses/i2c-designware-slave.c @@ -61,7 +61,7 @@ int i2c_dw_unreg_slave(struct i2c_client *slave) return 0; } -static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev) +__weak u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev) { unsigned int stat, dummy; -- 2.43.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-05-27 8:50 ` [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver lianfeng.ouyang @ 2026-05-27 13:07 ` Krzysztof Kozlowski 2026-05-28 2:36 ` 回复: " Lianfeng Ouyang 2026-06-02 22:24 ` Andy Shevchenko 1 sibling, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2026-05-27 13:07 UTC (permalink / raw) To: lianfeng.ouyang, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c, devicetree, linux-kernel On 27/05/2026 10:50, lianfeng.ouyang wrote: > dev->dev = device; > dev->irq = irq; > dev->flags = flags; > @@ -255,6 +259,8 @@ static const struct of_device_id dw_i2c_of_match[] = { > { .compatible = "mobileye,eyeq6lplus-i2c" }, > { .compatible = "mscc,ocelot-i2c" }, > { .compatible = "snps,designware-i2c" }, > + { .compatible = "starfive,jhb100-i2c-master" }, > + { .compatible = "starfive,jhb100-i2c-slave" }, So both devices are the same... Or you miss proper entry in ID table. Don't sprinkle compatibles around the code - ID table match data is for variant customization. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-05-27 13:07 ` Krzysztof Kozlowski @ 2026-05-28 2:36 ` Lianfeng Ouyang 2026-05-28 7:46 ` Krzysztof Kozlowski 0 siblings, 1 reply; 25+ messages in thread From: Lianfeng Ouyang @ 2026-05-28 2:36 UTC (permalink / raw) To: Krzysztof Kozlowski, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Hi, Krzysztof Thanks for the review. > -----邮件原件----- > 发件人: Krzysztof Kozlowski <krzk@kernel.org> > 发送时间: 2026年5月27日 21:07 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>; Andi Shyti > <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mika > Westerberg <mika.westerberg@linux.intel.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com> > 抄送: linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for > Starfive I2C driver > > On 27/05/2026 10:50, lianfeng.ouyang wrote: > > dev->dev = device; > > dev->irq = irq; > > dev->flags = flags; > > @@ -255,6 +259,8 @@ static const struct of_device_id dw_i2c_of_match[] = > { > > { .compatible = "mobileye,eyeq6lplus-i2c" }, > > { .compatible = "mscc,ocelot-i2c" }, > > { .compatible = "snps,designware-i2c" }, > > + { .compatible = "starfive,jhb100-i2c-master" }, > > + { .compatible = "starfive,jhb100-i2c-slave" }, > > So both devices are the same... Or you miss proper entry in ID table. > Don't sprinkle compatibles around the code - ID table match data is for > variant customization. > > Best regards, > Krzysztof Ok, Master and slave have different base addresses and interrupt numbers, but the register layout is the same, so in the next version, I will only use the same compatible ”starfive, jhb100-i2c“, and then add starfive, i2c-slave to the dts node of the slave to distinguish them Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-05-28 2:36 ` 回复: " Lianfeng Ouyang @ 2026-05-28 7:46 ` Krzysztof Kozlowski 2026-05-28 8:28 ` 回复: " Lianfeng Ouyang 0 siblings, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2026-05-28 7:46 UTC (permalink / raw) To: Lianfeng Ouyang, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org On 28/05/2026 04:36, Lianfeng Ouyang wrote: >>> { .compatible = "mobileye,eyeq6lplus-i2c" }, >>> { .compatible = "mscc,ocelot-i2c" }, >>> { .compatible = "snps,designware-i2c" }, >>> + { .compatible = "starfive,jhb100-i2c-master" }, >>> + { .compatible = "starfive,jhb100-i2c-slave" }, >> >> So both devices are the same... Or you miss proper entry in ID table. >> Don't sprinkle compatibles around the code - ID table match data is for >> variant customization. >> >> Best regards, >> Krzysztof > > Ok, Master and slave have different base addresses and interrupt numbers, but > the register layout is the same, so in the next version, I will only use the same > compatible ”starfive, jhb100-i2c“, and then add starfive, i2c-slave to the dts node > of the slave to distinguish them You do not need to distinguish same devices usually, but I still do not understand why do you need all these different compatibles and your commit msgs or binding text do not help, so I don't know what to advise. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-05-28 7:46 ` Krzysztof Kozlowski @ 2026-05-28 8:28 ` Lianfeng Ouyang 2026-06-03 14:34 ` Krzysztof Kozlowski 0 siblings, 1 reply; 25+ messages in thread From: Lianfeng Ouyang @ 2026-05-28 8:28 UTC (permalink / raw) To: Krzysztof Kozlowski, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Krzysztof Kozlowski <krzk@kernel.org> > 发送时间: 2026年5月28日 15:46 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>; Andi Shyti > <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mika > Westerberg <mika.westerberg@linux.intel.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com> > 抄送: linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add > __weak for Starfive I2C driver > > On 28/05/2026 04:36, Lianfeng Ouyang wrote: > >>> { .compatible = "mobileye,eyeq6lplus-i2c" }, > >>> { .compatible = "mscc,ocelot-i2c" }, > >>> { .compatible = "snps,designware-i2c" }, > >>> + { .compatible = "starfive,jhb100-i2c-master" }, > >>> + { .compatible = "starfive,jhb100-i2c-slave" }, > >> > >> So both devices are the same... Or you miss proper entry in ID table. > >> Don't sprinkle compatibles around the code - ID table match data is for > >> variant customization. > >> > >> Best regards, > >> Krzysztof > > > > Ok, Master and slave have different base addresses and interrupt numbers, but > > the register layout is the same, so in the next version, I will only use the same > > compatible ”starfive, jhb100-i2c“, and then add starfive, i2c-slave to the dts > node > > of the slave to distinguish them > > You do not need to distinguish same devices usually, but I still do not > understand why do you need all these different compatibles and your > commit msgs or binding text do not help, so I don't know what to advise. > > > Best regards, > Krzysztof The IP corresponding to the i2c driver submitted this time is quite unique, so it should be distinguished. The master-slave of i2c designware are at the same base address and switch according to the scene, so only one is initialized in probe(). But i2c starfive has master-slave at different base addresses, and it can work simultaneously. Therefore, both roles need to be initialized in their respective probes. If we don't distinguish, we don't know if this probe is initializing the master or the slave Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 回复: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-05-28 8:28 ` 回复: " Lianfeng Ouyang @ 2026-06-03 14:34 ` Krzysztof Kozlowski 2026-06-04 2:11 ` 回复: " Lianfeng Ouyang 0 siblings, 1 reply; 25+ messages in thread From: Krzysztof Kozlowski @ 2026-06-03 14:34 UTC (permalink / raw) To: Lianfeng Ouyang, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org On 28/05/2026 10:28, Lianfeng Ouyang wrote: > > >> -----邮件原件----- >> 发件人: Krzysztof Kozlowski <krzk@kernel.org> >> 发送时间: 2026年5月28日 15:46 >> 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>; Andi Shyti >> <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski >> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mika >> Westerberg <mika.westerberg@linux.intel.com>; Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> >> 抄送: linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; >> linux-kernel@vger.kernel.org >> 主题: Re: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add >> __weak for Starfive I2C driver >> >> On 28/05/2026 04:36, Lianfeng Ouyang wrote: >>>>> { .compatible = "mobileye,eyeq6lplus-i2c" }, >>>>> { .compatible = "mscc,ocelot-i2c" }, >>>>> { .compatible = "snps,designware-i2c" }, >>>>> + { .compatible = "starfive,jhb100-i2c-master" }, >>>>> + { .compatible = "starfive,jhb100-i2c-slave" }, >>>> >>>> So both devices are the same... Or you miss proper entry in ID table. >>>> Don't sprinkle compatibles around the code - ID table match data is for >>>> variant customization. >>>> >>>> Best regards, >>>> Krzysztof >>> >>> Ok, Master and slave have different base addresses and interrupt numbers, but >>> the register layout is the same, so in the next version, I will only use the same >>> compatible ”starfive, jhb100-i2c“, and then add starfive, i2c-slave to the dts >> node >>> of the slave to distinguish them >> >> You do not need to distinguish same devices usually, but I still do not >> understand why do you need all these different compatibles and your >> commit msgs or binding text do not help, so I don't know what to advise. >> >> >> Best regards, >> Krzysztof > > The IP corresponding to the i2c driver submitted this time is quite unique, > so it should be distinguished. The master-slave of i2c designware are at the > same base address and switch according to the scene, so only one is initialized > in probe(). But i2c starfive has master-slave at different base addresses, > and it can work simultaneously. Therefore, both roles need to be initialized in > their respective probes. If we don't distinguish, we don't know if this probe is > initializing the master or the slave > I don't think you read the comments. Again, you claim devices are incompatible but have exactly the same programming interface. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: 回复: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-06-03 14:34 ` Krzysztof Kozlowski @ 2026-06-04 2:11 ` Lianfeng Ouyang 0 siblings, 0 replies; 25+ messages in thread From: Lianfeng Ouyang @ 2026-06-04 2:11 UTC (permalink / raw) To: Krzysztof Kozlowski, Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Krzysztof Kozlowski <krzk@kernel.org> > 发送时间: 2026年6月3日 22:34 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>; Andi Shyti > <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mika > Westerberg <mika.westerberg@linux.intel.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com> > 抄送: linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: 回复: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add > __weak for Starfive I2C driver > > On 28/05/2026 10:28, Lianfeng Ouyang wrote: > > > > > >> -----邮件原件----- > >> 发件人: Krzysztof Kozlowski <krzk@kernel.org> > >> 发送时间: 2026年5月28日 15:46 > >> 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>; Andi Shyti > >> <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof > Kozlowski > >> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Mika > >> Westerberg <mika.westerberg@linux.intel.com>; Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> > >> 抄送: linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > >> linux-kernel@vger.kernel.org > >> 主题: Re: 回复: [PATCH v2 2/3] i2c: designware: Export symbols and add > >> __weak for Starfive I2C driver > >> > >> On 28/05/2026 04:36, Lianfeng Ouyang wrote: > >>>>> { .compatible = "mobileye,eyeq6lplus-i2c" }, > >>>>> { .compatible = "mscc,ocelot-i2c" }, > >>>>> { .compatible = "snps,designware-i2c" }, > >>>>> + { .compatible = "starfive,jhb100-i2c-master" }, > >>>>> + { .compatible = "starfive,jhb100-i2c-slave" }, > >>>> > >>>> So both devices are the same... Or you miss proper entry in ID table. > >>>> Don't sprinkle compatibles around the code - ID table match data is for > >>>> variant customization. > >>>> > >>>> Best regards, > >>>> Krzysztof > >>> > >>> Ok, Master and slave have different base addresses and interrupt numbers, > but > >>> the register layout is the same, so in the next version, I will only use the > same > >>> compatible ”starfive, jhb100-i2c“, and then add starfive, i2c-slave to the dts > >> node > >>> of the slave to distinguish them > >> > >> You do not need to distinguish same devices usually, but I still do not > >> understand why do you need all these different compatibles and your > >> commit msgs or binding text do not help, so I don't know what to advise. > >> > >> > >> Best regards, > >> Krzysztof > > > > The IP corresponding to the i2c driver submitted this time is quite unique, > > so it should be distinguished. The master-slave of i2c designware are at the > > same base address and switch according to the scene, so only one is initialized > > in probe(). But i2c starfive has master-slave at different base addresses, > > and it can work simultaneously. Therefore, both roles need to be initialized in > > their respective probes. If we don't distinguish, we don't know if this probe is > > initializing the master or the slave > > > > I don't think you read the comments. Again, you claim devices are > incompatible but have exactly the same programming interface. > > Best regards, > Krzysztof Hi Krzysztof , Thanks for your review. Now i2c has been changed to use the same compatible and use .data to distinguish between master and slave Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver 2026-05-27 8:50 ` [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver lianfeng.ouyang 2026-05-27 13:07 ` Krzysztof Kozlowski @ 2026-06-02 22:24 ` Andy Shevchenko 1 sibling, 0 replies; 25+ messages in thread From: Andy Shevchenko @ 2026-06-02 22:24 UTC (permalink / raw) To: lianfeng.ouyang Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c, devicetree, linux-kernel On Wed, May 27, 2026 at 04:50:38PM +0800, lianfeng.ouyang wrote: > Export several key functions (i2c_dw_probe_master, i2c_dw_init, > i2c_dw_xfer_init, i2c_dw_read_clear_intrbits, etc.) and mark them as > __weak. This allows the i2c starfive driver to reuse the common > infrastructure while overriding the implementations where needed. > > Additionally, extend the register map configuration and introduce the > MODEL_STARFIVE flag to accommodate the starfive i2c IP's different > register space. ... > +#endif My gosh, no. > .val_bits = 32, > .reg_stride = 4, > .disable_locking = true, > +#if IS_ENABLED(CONFIG_I2C_STARFIVE) > + .reg_read = sf_reg_read, > + .reg_write = sf_reg_write, > + .max_register = SF_IC_SMBUS_INTR_CLR, > +#else No. Just find a different way. See how Baikal T1 support was implemented (in v6.18, for example). > .reg_read = dw_reg_read, > .reg_write = dw_reg_write, > .max_register = DW_IC_COMP_TYPE, > +#endif ... > +#ifdef CONFIG_OF Definitely no. > + ret = of_property_read_u32(dev->dev->of_node, "starfive,i2c-tx-fifo-depth", &tx_fifo_cfg); No. Why?! Also do not use OF-centric APIs in the driver that uses fwnode. > + if (!ret && (tx_fifo_cfg < 2 || tx_fifo_cfg > 256)) > + tx_fifo_cfg = 8; > + > + ret = of_property_read_u32(dev->dev->of_node, "starfive,i2c-rx-fifo-depth", &rx_fifo_cfg); > + if (!ret && (rx_fifo_cfg < 2 || rx_fifo_cfg > 256)) > + rx_fifo_cfg = 8; > +#endif > + param = rx_fifo_cfg << 8 | tx_fifo_cfg << 16; > +#else > ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, ¶m); > i2c_dw_release_lock(dev); > if (ret) > return ret; > +#endif -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 3/3] i2c: starfive: Add StarFive JHB100 I2C master/slave support 2026-05-27 8:50 [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support lianfeng.ouyang 2026-05-27 8:50 ` [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C lianfeng.ouyang 2026-05-27 8:50 ` [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver lianfeng.ouyang @ 2026-05-27 8:50 ` lianfeng.ouyang 2026-05-27 12:36 ` [PATCH v2 0/3] i2c: Add Starfive " Mika Westerberg 2026-06-02 22:26 ` Andy Shevchenko 4 siblings, 0 replies; 25+ messages in thread From: lianfeng.ouyang @ 2026-05-27 8:50 UTC (permalink / raw) To: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, Andy Shevchenko Cc: linux-i2c, devicetree, linux-kernel, Lianfeng Ouyang From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> Add support for the Starfive I2C controller found on the StarFive JHB100 platform. The controller supports both master and slave modes and uses a different register layout compared to the standard DesignWare I2C IP. Currently, due to the following differences, i2c designware cannot be fully reused 1. For high and low level counting settings at different rates, i2c starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM 2. Interrupt clearing is achieved by writing 1 to the corresponding bit of INTR_CLR, while designware reads different clearing registers 3. Master and slave require separate probe callbacks and cannot rely solely on the runtime mode switching provided by i2c_dw_set_mode(). 4. The value of FIFO depth is not obtained through registers, but written through DTS The driver leverages the common i2c-designware-core infrastructure exported by the previous patch, while implementing its own timing calculation, FIFO configuration, and interrupt handling to accommodate the register offsets and bit definitions. Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> --- MAINTAINERS | 7 + drivers/i2c/busses/Kconfig | 24 ++ drivers/i2c/busses/Makefile | 3 + drivers/i2c/busses/i2c-starfive-core.h | 164 ++++++++ drivers/i2c/busses/i2c-starfive-master.c | 470 +++++++++++++++++++++++ drivers/i2c/busses/i2c-starfive-slave.c | 180 +++++++++ 6 files changed, 848 insertions(+) create mode 100644 drivers/i2c/busses/i2c-starfive-core.h create mode 100644 drivers/i2c/busses/i2c-starfive-master.c create mode 100644 drivers/i2c/busses/i2c-starfive-slave.c diff --git a/MAINTAINERS b/MAINTAINERS index d3a6b3f6b6a0..7a6bc33b1118 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -25319,6 +25319,13 @@ L: linux-riscv@lists.infradead.org S: Maintained F: arch/riscv/boot/dts/starfive/jhb100* +Starfive JHB100 I2C DRIVER +M: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> +L: linux-i2c@vger.kernel.org +S: Supported +F: Documentation/devicetree/bindings/i2c/starfive,jhb100-i2c.yaml +F: drivers/i2c/busses/i2c-starfive-* + STARFIVE JHB100 PINCTRL DRIVERS M: Changhuang Liang <changhuang.liang@starfivetech.com> M: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 7cb6b9b864a7..7630b19e706c 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -640,6 +640,30 @@ config I2C_DESIGNWARE_PCI endif +config I2C_STARFIVE + tristate "Starfive i2c adapter" + select REGMAP + help + This is a driver for the Starfive I2C controller, + a variant of the DesignWare I2C IP with a different register layout. + It shares common infrastructure with the standard DesignWare I2C + driver (I2C_DESIGNWARE_CORE) + +if I2C_STARFIVE + +config I2C_STARFIVE_SLAVE + bool "Starfive i2c slave" + default I2C_STARFIVE + select I2C_SLAVE + help + If you say yes to this option, support will be included for the + Starfive i2c slave adapter. + + This is not a standalone module, this module compiles together with + i2c-starfive-core. + +endif + config I2C_DIGICOLOR tristate "Conexant Digicolor I2C driver" depends on ARCH_DIGICOLOR || COMPILE_TEST diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 547123ab351f..1f6f56394bc9 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -61,6 +61,9 @@ i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-bayt obj-$(CONFIG_I2C_DESIGNWARE_AMDISP) += i2c-designware-amdisp.o obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o i2c-designware-pci-y := i2c-designware-pcidrv.o +obj-$(CONFIG_I2C_STARFIVE) += i2c-starfive.o +i2c-starfive-y += i2c-starfive-master.o +i2c-starfive-$(CONFIG_I2C_STARFIVE_SLAVE) += i2c-starfive-slave.o obj-$(CONFIG_I2C_DIGICOLOR) += i2c-digicolor.o obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o obj-$(CONFIG_I2C_EMEV2) += i2c-emev2.o diff --git a/drivers/i2c/busses/i2c-starfive-core.h b/drivers/i2c/busses/i2c-starfive-core.h new file mode 100644 index 000000000000..eb38d7215208 --- /dev/null +++ b/drivers/i2c/busses/i2c-starfive-core.h @@ -0,0 +1,164 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Starfive JHB100 I2C adapter driver. + * + * Based on the Starfive JHB100 I2C adapter driver. + * + * Copyright (C) 2006 Texas Instruments. + * Copyright (C) 2007 MontaVista Software Inc. + * Copyright (C) 2009 Provigent Ltd. + */ + +#include <linux/bits.h> +#include <linux/compiler_types.h> +#include <linux/completion.h> +#include <linux/dev_printk.h> +#include <linux/errno.h> +#include <linux/i2c.h> +#include <linux/regmap.h> +#include <linux/types.h> + +#define SF_REG_FLAG BIT(31) + +#define IC_DFLT_OPERATION_REG_OFFSET (SF_REG_FLAG | 0x00) +#define IC_DFLT_I2C_REG_OFFSET (SF_REG_FLAG | 0x20) +#define IC_DFLT_SMBUS_REG_OFFSET (SF_REG_FLAG | 0xFC) + +#define DW_IC_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C | \ + I2C_FUNC_SMBUS_BYTE | \ + I2C_FUNC_SMBUS_BYTE_DATA | \ + I2C_FUNC_SMBUS_WORD_DATA | \ + I2C_FUNC_SMBUS_BLOCK_DATA | \ + I2C_FUNC_SMBUS_I2C_BLOCK) + +/* + * Registers offset + */ +#define SF_IC_ENABLE (IC_DFLT_OPERATION_REG_OFFSET + 0x04) +#define DW_IC_ENABLE_TX_CMD_BLOCK BIT(2) +#define SF_IC_ENABLE_DAR_EN BIT(16) +#define SF_IC_ENABLE_DAR2_EN BIT(17) +#define SF_IC_ENABLE_DAR3_EN BIT(18) +#define SF_IC_ENABLE_DAR4_EN BIT(19) +#define SF_IC_CAPABILITIES (IC_DFLT_OPERATION_REG_OFFSET + 0x0c) +#define SF_IC_CAPABILITIES_IC_SMBUS BIT(10) +#define SF_IC_SMBUS_CAPABILITIES (IC_DFLT_OPERATION_REG_OFFSET + 0x18) +#define SF_IC_SMBUS_CAPABILITIES_SMBUS_ARP BIT(2) + +#define SF_IC_CTRL (IC_DFLT_I2C_REG_OFFSET + 0x04) +#define SF_IC_CTRL_OP_MODE BIT(0) +#define SF_IC_CTRL_SPEED_STD BIT(4) +#define SF_IC_CTRL_SPEED_FAST (2 << 4) +#define SF_IC_CTRL_SPEED_HIGH (3 << 4) +#define SF_IC_CTRL_SPEED_MASK GENMASK(5, 4) +#define SF_IC_CTRL_10BITADDR_TGT BIT(8) +#define SF_IC_CTRL_10BITADDR_CTRLR BIT(9) +#undef DW_IC_CON_10BITADDR_MASTER +#define DW_IC_CON_10BITADDR_MASTER BIT(9) +#define SF_IC_CTRL_STOP_DET_IFADDRESSED BIT(10) +#define SF_IC_CTRL_TX_EMPTY_CTRL BIT(11) +#define SF_IC_CTRL_RX_FIFO_FULL_HLD_CTRL BIT(12) +#define SF_IC_CTRL_BUS_CLEAR_CTRL BIT(14) +#undef DW_IC_CON_BUS_CLEAR_CTRL +#define DW_IC_CON_BUS_CLEAR_CTRL BIT(14) +#define SF_IC_TAR (IC_DFLT_I2C_REG_OFFSET + 0x08) +#define SF_IC_TAR_SPECIAL BIT(11) +#define SF_IC_TAR_SMBUS_QUICK_CMD BIT(16) +#define SF_IC_DAR (IC_DFLT_I2C_REG_OFFSET + 0x0C) +#define SF_IC_SCL_HCNT (IC_DFLT_I2C_REG_OFFSET + 0x24) +#define SF_IC_SCL_LCNT (IC_DFLT_I2C_REG_OFFSET + 0x28) +#define SF_IC_HS_SCL_HCNT (IC_DFLT_I2C_REG_OFFSET + 0x2c) +#define SF_IC_HS_SCL_LCNT (IC_DFLT_I2C_REG_OFFSET + 0x30) +#define SF_IC_SDA_HOLD (IC_DFLT_I2C_REG_OFFSET + 0x34) +#define DW_IC_SDA_HOLD_TX_SHIFT 0 +#define DW_IC_SDA_HOLD_TX_MASK GENMASK(15, 0) +#define SF_IC_SPKLEN (IC_DFLT_I2C_REG_OFFSET + 0x3c) +#define SF_IC_HS_SPKLEN (IC_DFLT_I2C_REG_OFFSET + 0x40) +#define SF_IC_SCL_STUCK_AT_LOW_TIMEOUT (IC_DFLT_I2C_REG_OFFSET + 0x44) +#define SF_IC_SCL_STUCK_AT_LOW_TIMEOUT_MAX (IC_DFLT_I2C_REG_OFFSET + 0x48) +#define SF_IC_SDA_STUCK_AT_LOW_TIMEOUT (IC_DFLT_I2C_REG_OFFSET + 0x4c) +#define SF_IC_DATA_CMD (IC_DFLT_I2C_REG_OFFSET + 0x58) +#define SF_IC_DATA_CMD_DAT GENMASK(7, 0) +#define SF_IC_DATA_CMD_CMD BIT(8) +#define SF_IC_DATA_CMD_STOP BIT(9) +#define SF_IC_DATA_CMD_FIRST_DATA_BYTE BIT(11) +#define SF_IC_RX_TL (IC_DFLT_I2C_REG_OFFSET + 0x5c) +#define SF_IC_TX_TL (IC_DFLT_I2C_REG_OFFSET + 0x60) +#define SF_IC_INTR_STAT (IC_DFLT_I2C_REG_OFFSET + 0x74) +#define SF_IC_INTR_SCL_STUCK_AT_LOW BIT(14) +#define SF_IC_INTR_MASK (IC_DFLT_I2C_REG_OFFSET + 0x78) +#define SF_IC_RAW_INTR_STAT (IC_DFLT_I2C_REG_OFFSET + 0x7c) +#define SF_IC_INTR_CLR (IC_DFLT_I2C_REG_OFFSET + 0x80) +#define SF_IC_CLR_INTR BIT(0) +#define SF_IC_CLR_RX_UNDER BIT(1) +#define SF_IC_CLR_RX_OVER BIT(2) +#define SF_IC_CLR_TX_OVER BIT(3) +#define SF_IC_CLR_RD_REQ BIT(4) +#define SF_IC_CLR_TX_ABRT BIT(5) +#define SF_IC_CLR_RX_DONE BIT(6) +#define SF_IC_CLR_ACTIVITY BIT(7) +#define SF_IC_CLR_STOP_DET BIT(8) +#define SF_IC_CLR_START_DET BIT(9) +#define SF_IC_CLR_GEN_CALL BIT(10) +#define SF_IC_CLR_RESTART_DET BIT(11) +#define SF_IC_CLR_SCL_STUCK_DET BIT(12) +#define SF_IC_ENABLE_STATUS (IC_DFLT_I2C_REG_OFFSET + 0x84) +#define SF_IC_TX_TRMNT_SOURCE (IC_DFLT_I2C_REG_OFFSET + 0x88) +#define SF_IC_STATUS (IC_DFLT_I2C_REG_OFFSET + 0x8c) +#define SF_IC_STATUS_ACTIVITY BIT(0) +#define SF_IC_STATUS_TFE BIT(2) +#define SF_IC_STATUS_RFNE BIT(3) +#define SF_IC_STATUS_CTRLR_ACTIVITY BIT(5) +#define SF_IC_STATUS_TGT_ACTIVITY BIT(6) +#define SF_IC_TXFLR (IC_DFLT_I2C_REG_OFFSET + 0x90) +#define SF_IC_RXFLR (IC_DFLT_I2C_REG_OFFSET + 0x94) +#define SF_IC_COMP_VERSION (IC_DFLT_I2C_REG_OFFSET + 0xa4) +#define SF_IC_COMP_TYPE (IC_DFLT_I2C_REG_OFFSET + 0xa8) + +#define SF_IC_SMBUS_ARP_CTRL (IC_DFLT_SMBUS_REG_OFFSET + 0x8) +#define SF_IC_SMBUS_ARP_CTRL_NARP_DEVICE_TYPE 0 +#define SF_IC_SMBUS_INTR_STAT (IC_DFLT_SMBUS_REG_OFFSET + 0x28) +#define SF_IC_R_SMBUS_ALERT_DET BIT(10) +#define SF_IC_SMBUS_INTR_MASK (IC_DFLT_SMBUS_REG_OFFSET + 0x2c) +#define SF_IC_SMBUS_INTR_CLR (IC_DFLT_SMBUS_REG_OFFSET + 0x34) +#define SF_IC_CLR_SMBUS_ALERT_DET BIT(10) + +/* error code */ +#define DW_IC_ERR_TX_ABRT 0x1 + +#undef TXGBE_RX_FIFO_DEPTH +#define TXGBE_RX_FIFO_DEPTH 0 + +/* make (dev->master_cfg & DW_IC_CON_RESTART_EN) always true*/ +#undef DW_IC_CON_RESTART_EN +#define DW_IC_CON_RESTART_EN 0xffffffff + +extern const struct i2c_algorithm i2c_dw_slave_algo; +int sf_reg_read(void *context, unsigned int reg, unsigned int *val); +int sf_reg_write(void *context, unsigned int reg, unsigned int val); +int i2c_sf_probe_slave(struct dw_i2c_dev *dev); +void i2c_dw_read_clear_intrbits_common(struct dw_i2c_dev *dev); +void i2c_dw_write_timings(struct dw_i2c_dev *dev); +void i2c_dw_configure_mode(struct dw_i2c_dev *dev, int mode); +int i2c_dw_set_timings_master(struct dw_i2c_dev *dev); +void i2c_sf_configure_master(struct dw_i2c_dev *dev); +void i2c_sf_configure_slave(struct dw_i2c_dev *dev); + +static inline void __i2c_dw_enable(struct dw_i2c_dev *dev) +{ + int val; + + dev->status |= STATUS_ACTIVE; + regmap_read(dev->map, DW_IC_ENABLE, &val); + regmap_write(dev->map, DW_IC_ENABLE, + ((val & ~DW_IC_ENABLE_TX_CMD_BLOCK) | DW_IC_ENABLE_ENABLE)); +} + +static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev) +{ + int val; + + regmap_read(dev->map, DW_IC_ENABLE, &val); + regmap_write(dev->map, DW_IC_ENABLE, val & ~DW_IC_ENABLE_ENABLE); + dev->status &= ~STATUS_ACTIVE; +} diff --git a/drivers/i2c/busses/i2c-starfive-master.c b/drivers/i2c/busses/i2c-starfive-master.c new file mode 100644 index 000000000000..94fe041cf46d --- /dev/null +++ b/drivers/i2c/busses/i2c-starfive-master.c @@ -0,0 +1,470 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Starfive JHB100 I2C adapter driver (master only). + * * + * Copyright (C) 2006 Texas Instruments. + * Copyright (C) 2007 MontaVista Software Inc. + * Copyright (C) 2009 Provigent Ltd. + */ +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/export.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/pinctrl/consumer.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/reset.h> +#include <linux/units.h> + +#include "i2c-designware-core.h" + +#define AMD_TIMEOUT_MIN_US 25 +#define AMD_TIMEOUT_MAX_US 250 +#define AMD_MASTERCFG_MASK GENMASK(15, 0) + +#define REG_MAP_NONE 0xffff + +struct starfive_reg_desc { + u32 sf_addr; + u32 dw_addr; +}; + +static const struct starfive_reg_desc sf_dw_reg_map[] = { + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_CTRL}, /* 0x0 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_TAR}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_CAPABILITIES, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_DATA_CMD}, /* 0x10 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_SMBUS_CAPABILITIES, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0x20 */ + { .sf_addr = SF_IC_CTRL, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_DAR, .dw_addr = SF_IC_INTR_STAT}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_INTR_MASK}, /* 0x30 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_RAW_INTR_STAT}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_INTR_CLR}, /* 0x40 */ + { .sf_addr = SF_IC_SCL_HCNT, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_SCL_LCNT, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_HS_SCL_HCNT, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_HS_SCL_LCNT, .dw_addr = REG_MAP_NONE}, /* 0x50 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_SPKLEN, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_HS_SPKLEN, .dw_addr = REG_MAP_NONE}, /* 0x60 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_ENABLE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_STATUS}, /* 0x70 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_TXFLR}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_RXFLR}, + { .sf_addr = SF_IC_RX_TL, .dw_addr = SF_IC_SDA_HOLD}, + { .sf_addr = SF_IC_TX_TL, .dw_addr = SF_IC_TX_TRMNT_SOURCE}, /* 0x80 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0x90 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_ENABLE_STATUS}, + { .sf_addr = SF_IC_INTR_CLR, .dw_addr = REG_MAP_NONE}, /* 0xA0 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0xB0 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0xC0 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_SMBUS_INTR_MASK}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0xD0 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0xE0 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0xF0 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = SF_IC_COMP_VERSION}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0x100 */ + { .sf_addr = SF_IC_SMBUS_ARP_CTRL, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0x110 */ + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, /* 0x120 */ + { .sf_addr = SF_IC_SMBUS_INTR_STAT, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = REG_MAP_NONE, .dw_addr = REG_MAP_NONE}, + { .sf_addr = SF_IC_SMBUS_INTR_CLR, .dw_addr = REG_MAP_NONE}, /* 0x130 */ +}; + +int sf_reg_read(void *context, unsigned int reg, unsigned int *val) +{ + struct dw_i2c_dev *dev = context; + unsigned int reg_v, reg_nof, idx; + + reg_nof = reg & ~SF_REG_FLAG; + idx = reg_nof >> 2; + + if (idx >= ARRAY_SIZE(sf_dw_reg_map)) + return -EINVAL; + + const struct starfive_reg_desc *desc = &sf_dw_reg_map[idx]; + + if (reg & SF_REG_FLAG) + reg_v = desc->sf_addr; + else + reg_v = desc->dw_addr; + + if (reg_v == 0xff) { + dev_err(dev->dev, "sf reg read 0x%x error!\n", reg_nof); + return -EINVAL; + } + + if (reg_v == SF_IC_INTR_CLR) + writel(SF_IC_CLR_INTR, dev->base + (reg_v & ~SF_REG_FLAG)); + else + *val = readl(dev->base + (reg_v & ~SF_REG_FLAG)); + + return 0; +} + +int sf_reg_write(void *context, unsigned int reg, unsigned int val) +{ + struct dw_i2c_dev *dev = context; + unsigned int reg_v, reg_nof, idx; + + reg_nof = reg & ~SF_REG_FLAG; + idx = reg_nof >> 2; + + if (idx >= ARRAY_SIZE(sf_dw_reg_map)) + return -EINVAL; + + const struct starfive_reg_desc *desc = &sf_dw_reg_map[idx]; + + if (reg & SF_REG_FLAG) + reg_v = desc->sf_addr; + else + reg_v = desc->dw_addr; + + if (reg_v == 0xff) { + dev_err(dev->dev, "sf reg read 0x%x error!\n", reg_nof); + return -EINVAL; + } + + writel(val, dev->base + (reg_v & ~SF_REG_FLAG)); + + return 0; +} + +/** + * i2c_sf_scl_hcnt() - Calculate SCL HCNT + * @ic_clk: Input clock in kHz + * @thigh: Duration in ns of logic 1 to generate + * @tr: SCL rise time in ns + * @spk_cnt: Spike count + */ +static u32 i2c_sf_scl_hcnt(u32 ic_clk, u32 thigh, u32 tr, u32 spk_cnt) +{ + u64 min_thigh_cnt, rise_cnt; + + /* Formula: cnt = f_kHz * t_ns * 10^(-6) */ + min_thigh_cnt = DIV_ROUND_CLOSEST_ULL((u64)ic_clk * thigh, MICRO); + rise_cnt = DIV_ROUND_CLOSEST_ULL((u64)ic_clk * tr, MICRO); + + return max(5, min_thigh_cnt + rise_cnt - spk_cnt - 3); +} + +/** + * i2c_sf_scl_lcnt() - Calculate SCL LCNT + * @ic_clk: Input clock in kHz + * @tlow: Duration in ns of logic 0 to generate + * @tf: SCL fall time in ns + */ +static u32 i2c_sf_scl_lcnt(u32 ic_clk, u32 tlow, u32 tf) +{ + u64 min_tlow_cnt, fall_cnt; + + /* Formula: cnt = f_kHz * t_ns * 10^(-6) */ + min_tlow_cnt = DIV_ROUND_CLOSEST_ULL((u64)ic_clk * tlow, MICRO); + fall_cnt = DIV_ROUND_CLOSEST_ULL((u64)ic_clk * tf, MICRO); + + return max(6, min_tlow_cnt + fall_cnt); +} + +int i2c_dw_set_timings_master(struct dw_i2c_dev *dev) +{ + u32 scl_falling_time = 0, scl_rising_time = 0; + u32 scl_high_time = 0, scl_low_time = 0; + struct i2c_timings *t = &dev->timings; + unsigned int comp_param1; + u32 ic_clk, spk_cnt; + int ret; + + ret = i2c_dw_acquire_lock(dev); + if (ret) + return ret; + + ret = regmap_read(dev->map, SF_IC_CTRL, &comp_param1); + i2c_dw_release_lock(dev); + if (ret) + return ret; + + ic_clk = i2c_dw_clk_rate(dev); + + /* 50ns maximum spike */ + spk_cnt = DIV_ROUND_CLOSEST_ULL((u64)ic_clk * 50, MICRO); + + regmap_write(dev->map, SF_IC_HS_SPKLEN, spk_cnt); + regmap_write(dev->map, SF_IC_SPKLEN, spk_cnt); + + /* Parse user defined rise time and fall time*/ + if (t->scl_rise_ns) + scl_rising_time = t->scl_rise_ns; + + if (t->scl_fall_ns) + scl_falling_time = t->scl_fall_ns; + + /* Ensure the rise time and fall time should not lower than t_rise_max + * and t_fall_max specification, else it would run faster than expected + * frequency + */ + switch (t->bus_freq_hz) { + case I2C_MAX_STANDARD_MODE_FREQ: + scl_rising_time = max(scl_rising_time, 1000); + scl_falling_time = max(scl_falling_time, 300); + scl_high_time = 4000; /* tHIGH_min = 4.0 us */ + scl_low_time = 4700; /* tLOW_min = 4.7 us */ + break; + case I2C_MAX_FAST_MODE_FREQ: + scl_rising_time = max(scl_rising_time, 300); + scl_falling_time = max(scl_falling_time, 300); + scl_high_time = 600; /* tHIGH_min = 600 ns */ + scl_low_time = 1300; /* tLOW_min = 1.3 us */ + break; + case I2C_MAX_FAST_MODE_PLUS_FREQ: + scl_rising_time = max(scl_rising_time, 120); + scl_falling_time = max(scl_falling_time, 120); + scl_high_time = 260; /* tHIGH_min = 260 ns */ + scl_low_time = 500; /* tLOW_min = 500 ns */ + break; + case I2C_MAX_HIGH_SPEED_MODE_FREQ: + scl_rising_time = max(scl_rising_time, 40); + scl_falling_time = max(scl_falling_time, 40); + scl_high_time = 60; /* tHIGH_min = 60 ns */ + scl_low_time = 160; /* tLOW_min = 160 ns */ + break; + default: + scl_rising_time = max(scl_rising_time, 1000); + scl_falling_time = max(scl_falling_time, 300); + scl_high_time = 4000; /* tHIGH_min = 4.0 us */ + scl_low_time = 4700; /* tLOW_min = 4.7 us */ + break; + } + + ic_clk = i2c_dw_clk_rate(dev); + + if (!dev->scl_hcnt || !dev->scl_lcnt) { + dev->scl_hcnt = i2c_sf_scl_hcnt(ic_clk, scl_high_time, scl_rising_time, spk_cnt); + dev->scl_lcnt = i2c_sf_scl_lcnt(ic_clk, scl_low_time, scl_falling_time); + } + + dev_dbg(dev->dev, "Bus speed: %s\n", i2c_freq_mode_string(t->bus_freq_hz)); + + /* Check is high speed possible and fall back to fast mode if not */ + if ((dev->master_cfg & SF_IC_CTRL_SPEED_MASK) == SF_IC_CTRL_SPEED_HIGH) { + if ((comp_param1 & SF_IC_CTRL_SPEED_MASK) != SF_IC_CTRL_SPEED_HIGH) { + dev_err(dev->dev, "High Speed not supported!\n"); + t->bus_freq_hz = I2C_MAX_FAST_MODE_FREQ; + dev->master_cfg &= ~SF_IC_CTRL_SPEED_MASK; + dev->master_cfg |= SF_IC_CTRL_SPEED_FAST; + dev->hs_hcnt = 0; + dev->hs_lcnt = 0; + + /* Replace with I2C_MAX_FAST_MODE_PLUS_FREQ */ + scl_rising_time = max(scl_rising_time, 120); + scl_falling_time = max(scl_falling_time, 120); + scl_high_time = 260; /* tHIGH_min = 260 ns */ + scl_low_time = 500; /* tLOW_min = 500 ns */ + + dev->scl_hcnt = i2c_sf_scl_hcnt(ic_clk, scl_high_time, + scl_rising_time, spk_cnt); + dev->scl_lcnt = i2c_sf_scl_lcnt(ic_clk, scl_low_time, + scl_falling_time); + } else if (!dev->hs_hcnt || !dev->hs_lcnt) { + dev->hs_hcnt = dev->scl_hcnt; + dev->hs_lcnt = dev->scl_lcnt; + } + dev_dbg(dev->dev, "High Speed Mode HCNT:LCNT = %d:%d\n", + dev->hs_hcnt, dev->hs_lcnt); + } else { + dev_dbg(dev->dev, "HCNT:LCNT: %d:%d\n", dev->scl_hcnt, dev->scl_lcnt); + } + + return 0; +} + +void i2c_dw_write_timings(struct dw_i2c_dev *dev) +{ + /* Write standard speed timing parameters */ + regmap_write(dev->map, SF_IC_SCL_HCNT, dev->scl_hcnt); + regmap_write(dev->map, SF_IC_SCL_LCNT, dev->scl_lcnt); + + /* Write high speed timing parameters if supported */ + if (dev->hs_hcnt && dev->hs_lcnt) { + regmap_write(dev->map, SF_IC_HS_SCL_HCNT, dev->hs_hcnt); + regmap_write(dev->map, SF_IC_HS_SCL_LCNT, dev->hs_lcnt); + } +} + +void i2c_dw_configure_mode(struct dw_i2c_dev *dev, int mode) +{ + /* Configure Tx/Rx FIFO threshold levels */ + regmap_write(dev->map, SF_IC_TX_TL, dev->tx_fifo_depth / 2); + regmap_write(dev->map, SF_IC_RX_TL, 0); + + /* Configure the I2C master */ + regmap_write(dev->map, SF_IC_CTRL, dev->master_cfg); +} + +static int i2c_sf_set_sda_hold_time(struct dw_i2c_dev *dev) +{ + int ret = regmap_write_bits(dev->map, + DW_IC_SDA_HOLD, + DW_IC_SDA_HOLD_TX_MASK, + dev->sda_hold_time); + return ret; +} + +void i2c_dw_read_clear_intrbits_common(struct dw_i2c_dev *dev) +{ + unsigned int stat; + + /* + * The IC_INTR_STAT register just indicates "enabled" interrupts. + * The unmasked raw version of interrupt status bits is available + * in the IC_RAW_INTR_STAT register. + * + * That is, + * stat = readl(IC_INTR_STAT); + * equals to, + * stat = readl(IC_RAW_INTR_STAT) & readl(IC_INTR_MASK); + * + * The raw version might be useful for debugging purposes. + */ + regmap_read(dev->map, DW_IC_INTR_STAT, &stat); + /* + * Do not use the IC_CLR_INTR register to clear interrupts, or + * you'll miss some interrupts, triggered during the period from + * readl(IC_INTR_STAT) to readl(IC_CLR_INTR). + * + * Instead, use the separately-prepared IC_CLR_* registers. + */ + if (stat & DW_IC_INTR_RX_UNDER) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_RX_UNDER); + if (stat & DW_IC_INTR_RX_OVER) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_RX_OVER); + if (stat & DW_IC_INTR_TX_OVER) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_TX_OVER); + if (stat & DW_IC_INTR_RD_REQ) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_RD_REQ); + if (stat & DW_IC_INTR_RX_DONE) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_RX_DONE); + if (stat & DW_IC_INTR_ACTIVITY) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_ACTIVITY); + if (stat & DW_IC_INTR_START_DET) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_START_DET); + if (stat & DW_IC_INTR_GEN_CALL) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_GEN_CALL); +} +EXPORT_SYMBOL_GPL(i2c_dw_read_clear_intrbits_common); + +u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev) +{ + unsigned int stat; + + /* + * The IC_INTR_STAT register just indicates "enabled" interrupts. + * The unmasked raw version of interrupt status bits is available + * in the IC_RAW_INTR_STAT register. + * + * That is, + * stat = readl(IC_INTR_STAT); + * equals to, + * stat = readl(IC_RAW_INTR_STAT) & readl(IC_INTR_MASK); + * + * The raw version might be useful for debugging purposes. + */ + regmap_read(dev->map, DW_IC_INTR_STAT, &stat); + + if (stat & DW_IC_INTR_TX_ABRT) { + /* + * The IC_TX_ABRT_SOURCE register is cleared whenever + * the IC_CLR_TX_ABRT is read. Preserve it beforehand. + */ + regmap_read(dev->map, DW_IC_TX_ABRT_SOURCE, &dev->abort_source); + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_TX_ABRT); + } + if ((stat & DW_IC_INTR_STOP_DET) && + (dev->rx_outstanding == 0 || (stat & DW_IC_INTR_RX_FULL))) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_STOP_DET); + if (stat & DW_IC_INTR_RESTART_DET) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_RESTART_DET); + if (stat & SF_IC_INTR_SCL_STUCK_AT_LOW) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_SCL_STUCK_DET); + + i2c_dw_read_clear_intrbits_common(dev); + + return stat; +} + +void i2c_sf_configure_master(struct dw_i2c_dev *dev) +{ + struct i2c_timings *t = &dev->timings; + + dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY | + I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_PEC; + + dev->master_cfg = SF_IC_CTRL_OP_MODE; + + dev->mode = DW_IC_MASTER; + + switch (t->bus_freq_hz) { + case I2C_MAX_STANDARD_MODE_FREQ: + dev->master_cfg |= SF_IC_CTRL_SPEED_STD; + break; + case I2C_MAX_FAST_MODE_FREQ: + dev->master_cfg |= SF_IC_CTRL_SPEED_FAST; + break; + case I2C_MAX_FAST_MODE_PLUS_FREQ: + dev->master_cfg |= SF_IC_CTRL_SPEED_FAST; + break; + case I2C_MAX_HIGH_SPEED_MODE_FREQ: + dev->master_cfg |= SF_IC_CTRL_SPEED_HIGH; + break; + default: + dev->master_cfg |= SF_IC_CTRL_SPEED_FAST; + } + + dev->set_sda_hold_time = i2c_sf_set_sda_hold_time; +} + +MODULE_AUTHOR("Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>"); +MODULE_DESCRIPTION("Starfive JHB100 I2C bus master adapter"); +MODULE_LICENSE("GPL"); diff --git a/drivers/i2c/busses/i2c-starfive-slave.c b/drivers/i2c/busses/i2c-starfive-slave.c new file mode 100644 index 000000000000..06c3ead35841 --- /dev/null +++ b/drivers/i2c/busses/i2c-starfive-slave.c @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Starfive JHB100 I2C adapter driver (slave only). + * + * Based on the Starfive JHB100 I2C adapter driver (master). + * + * Copyright (C) 2016 Synopsys Inc. + */ +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of_platform.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> + +#include "i2c-designware-core.h" + +void i2c_dw_set_mode(struct dw_i2c_dev *dev, int mode) +{ + if (dev->mode == DW_IC_SLAVE && mode == DW_IC_SLAVE) { + regmap_write(dev->map, SF_IC_DAR, dev->slave->addr); + __i2c_dw_enable(dev); + dev->status = 0; + } +} + +static void i2c_sf_configure_fifo_slave(struct dw_i2c_dev *dev) +{ + /* Configure the I2C slave. */ + regmap_write(dev->map, SF_IC_CTRL, 0); + regmap_write(dev->map, SF_IC_CTRL, dev->slave_cfg); + regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_SLAVE_MASK); +} + +/** + * i2c_sf_init_slave() - Initialize the Starfive i2c slave hardware + * @dev: device private data + * + * This function configures and enables the I2C in slave mode. + * This function is called during I2C init function, and in case of timeout at + * run time. + */ +static int i2c_sf_init_slave(struct dw_i2c_dev *dev) +{ + int ret; + int r_value; + + ret = i2c_dw_acquire_lock(dev); + if (ret) + return ret; + + /* Disable the adapter. */ + __i2c_dw_disable(dev); + + /* Write SDA hold time if supported */ + if (dev->sda_hold_time) + regmap_write(dev->map, DW_IC_SDA_HOLD, dev->sda_hold_time); + + regmap_read(dev->map, SF_IC_CAPABILITIES, &r_value); + if (r_value & SF_IC_CAPABILITIES_IC_SMBUS) { + regmap_read(dev->map, SF_IC_SMBUS_CAPABILITIES, &r_value); + if (r_value & SF_IC_SMBUS_CAPABILITIES_SMBUS_ARP) { + regmap_write(dev->map, DW_IC_ENABLE, SF_IC_ENABLE_DAR_EN); + regmap_write(dev->map, SF_IC_SMBUS_ARP_CTRL, + SF_IC_SMBUS_ARP_CTRL_NARP_DEVICE_TYPE); + } + } + + i2c_sf_configure_fifo_slave(dev); + i2c_dw_release_lock(dev); + + return 0; +} + +u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev) +{ + unsigned int stat; + unsigned int smbus_stat; + + /* + * The IC_INTR_STAT register just indicates "enabled" interrupts. + * The unmasked raw version of interrupt status bits is available + * in the IC_RAW_INTR_STAT register. + * + * That is, + * stat = readl(IC_INTR_STAT); + * equals to, + * stat = readl(IC_RAW_INTR_STAT) & readl(IC_INTR_MASK); + * + * The raw version might be useful for debugging purposes. + */ + regmap_read(dev->map, DW_IC_INTR_STAT, &stat); + regmap_read(dev->map, SF_IC_SMBUS_INTR_STAT, &smbus_stat); + + /* + * Do not use the IC_CLR_INTR register to clear interrupts, or + * you'll miss some interrupts, triggered during the period from + * readl(IC_INTR_STAT) to readl(IC_CLR_INTR). + * + * Instead, use the separately-prepared IC_CLR_* registers. + */ + + if (stat & DW_IC_INTR_TX_ABRT) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_TX_ABRT); + if (stat & DW_IC_INTR_STOP_DET) + regmap_write(dev->map, SF_IC_INTR_CLR, SF_IC_CLR_STOP_DET); + if (smbus_stat & SF_IC_R_SMBUS_ALERT_DET) + regmap_write(dev->map, SF_IC_SMBUS_INTR_CLR, SF_IC_CLR_SMBUS_ALERT_DET); + + i2c_dw_read_clear_intrbits_common(dev); + + return stat; +} + +#if IS_ENABLED(CONFIG_I2C_STARFIVE_SLAVE) +static int i2c_sf_xfer_ms(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) +{ + struct dw_i2c_dev *dev = i2c_get_adapdata(adap); + + if (!dev->ms_adapter) + return -ENXIO; + + return i2c_dw_xfer(dev->ms_adapter, msgs, num); +} +#endif + +const struct i2c_algorithm i2c_dw_slave_algo = { + .master_xfer = i2c_sf_xfer_ms, + .functionality = i2c_dw_func, + .reg_slave = i2c_dw_reg_slave, + .unreg_slave = i2c_dw_unreg_slave, +}; + +void i2c_sf_configure_slave(struct dw_i2c_dev *dev) +{ + dev->functionality = I2C_FUNC_SLAVE | DW_IC_DEFAULT_FUNCTIONALITY; + + dev->slave_cfg = SF_IC_CTRL_RX_FIFO_FULL_HLD_CTRL | + SF_IC_CTRL_STOP_DET_IFADDRESSED; + + dev->mode = DW_IC_SLAVE; +} + +int i2c_sf_probe_slave(struct dw_i2c_dev *dev) +{ + struct i2c_adapter *adap = &dev->adapter; + struct device_node *ms_node; + int ret; + + ret = i2c_sf_init_slave(dev); + if (ret) + return ret; + + snprintf(adap->name, sizeof(adap->name), "Starfive JHB100 I2C Slave adapter"); + + /* Get the specified I2C master adapter from DTS for MCTP */ + ms_node = of_parse_phandle(dev->dev->of_node, "starfive,mctp-i2c-ms", 0); + if (ms_node) { + struct platform_device *ms_pdev = of_find_device_by_node(ms_node); + + if (ms_pdev) { + struct dw_i2c_dev *ms_dev = platform_get_drvdata(ms_pdev); + + if (ms_dev) + dev->ms_adapter = &ms_dev->adapter; + } + } + + return ret; +} +EXPORT_SYMBOL_GPL(i2c_sf_probe_slave); + +MODULE_AUTHOR("Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>"); +MODULE_DESCRIPTION("Starfive JHB100 I2C bus slave adapter"); +MODULE_LICENSE("GPL"); -- 2.43.0 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-05-27 8:50 [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support lianfeng.ouyang ` (2 preceding siblings ...) 2026-05-27 8:50 ` [PATCH v2 3/3] i2c: starfive: Add StarFive JHB100 I2C master/slave support lianfeng.ouyang @ 2026-05-27 12:36 ` Mika Westerberg 2026-05-28 2:41 ` 回复: " Lianfeng Ouyang 2026-06-02 22:26 ` Andy Shevchenko 4 siblings, 1 reply; 25+ messages in thread From: Mika Westerberg @ 2026-05-27 12:36 UTC (permalink / raw) To: lianfeng.ouyang Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Shevchenko, linux-i2c, devicetree, linux-kernel Hi, On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > > The Starfive JHB100 I2C controller is a variant of the widely-used > DesignWare I2C IP, with a distinct register layout and enhanced features > such as SMBus Alert and programmable FIFO depths. > > The series is structured as follows: > 1. Adds the device tree binding document for the starfive,jhb100-i2c > compatible. > 2. Prepares the existing i2c-designware-core by exporting and making > certain key functions overridable, allowing code reuse. > 3. Introduces the new i2c-starfive-* driver, with separate modules for > master and slave functionality, based on the 2023-07 revision of > the Synopsys IP manual. > > Currently, due to the following differences, i2c designware cannot be > fully reused > 1. For high and low level counting settings at different rates, i2c > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > 2. Interrupt clearing is achieved by writing 1 to the corresponding > bit of INTR_CLR, while designware reads different clearing > registers > 3. Master and slave require separate probe callbacks and cannot rely > solely on the runtime mode switching provided by i2c_dw_set_mode() > 4. The value of FIFO depth is not obtained through registers, but > written through DTS > > I have written some poorly styled code to reduce changes to i2c designware > and reuse its functions by keeping aa always true, for example > 1. the implementation of i2c-d w_probe_master() differs only for the two > IPs in i2c_dw_set_timits_master(). In order to reuse > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > __weak. A better approach is to use a callback function, but using > a callback function requires changing more i2c designware files. > I don't know what the attitude of the community is > 2. For the operation of clearing interrupt flags, i2c designware reads > and i2c starfive writes. Therefore, in order not to modify the > relevant logic of i2c designware, I added a write operation to > sf_reg_read() > So I think this version of the code is not allowed to merge, but I don't > know how to handle this situation because if i2c designware is not changed > at all, we will have to write code that is similar to i2c designware. > Will this type of IP not be allowed to merge? It's not that you are not allowed to change the existing driver. It is definitely encouraged to avoid unnecessary duplication. If possible think of the core parts as library that your glue driver can call and if something is missing you can always add (or modify). Also while there, let's start by adding support for the master operations first and then later on you can add the slave bits. Hope this helps. ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-05-27 12:36 ` [PATCH v2 0/3] i2c: Add Starfive " Mika Westerberg @ 2026-05-28 2:41 ` Lianfeng Ouyang 0 siblings, 0 replies; 25+ messages in thread From: Lianfeng Ouyang @ 2026-05-28 2:41 UTC (permalink / raw) To: Mika Westerberg Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Shevchenko, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Mika Westerberg <mika.westerberg@linux.intel.com> > 发送时间: 2026年5月27日 20:36 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > 抄送: Andi Shyti <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; linux-i2c@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > 主题: Re: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support > > Hi, > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > DesignWare I2C IP, with a distinct register layout and enhanced features > > such as SMBus Alert and programmable FIFO depths. > > > > The series is structured as follows: > > 1. Adds the device tree binding document for the starfive,jhb100-i2c > > compatible. > > 2. Prepares the existing i2c-designware-core by exporting and making > > certain key functions overridable, allowing code reuse. > > 3. Introduces the new i2c-starfive-* driver, with separate modules for > > master and slave functionality, based on the 2023-07 revision of > > the Synopsys IP manual. > > > > Currently, due to the following differences, i2c designware cannot be > > fully reused > > 1. For high and low level counting settings at different rates, i2c > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > bit of INTR_CLR, while designware reads different clearing > > registers > > 3. Master and slave require separate probe callbacks and cannot rely > > solely on the runtime mode switching provided by > i2c_dw_set_mode() > > 4. The value of FIFO depth is not obtained through registers, but > > written through DTS > > > > I have written some poorly styled code to reduce changes to i2c designware > > and reuse its functions by keeping aa always true, for example > > 1. the implementation of i2c-d w_probe_master() differs only for the two > > IPs in i2c_dw_set_timits_master(). In order to reuse > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > > __weak. A better approach is to use a callback function, but using > > a callback function requires changing more i2c designware files. > > I don't know what the attitude of the community is > > 2. For the operation of clearing interrupt flags, i2c designware reads > > and i2c starfive writes. Therefore, in order not to modify the > > relevant logic of i2c designware, I added a write operation to > > sf_reg_read() > > So I think this version of the code is not allowed to merge, but I don't > > know how to handle this situation because if i2c designware is not changed > > at all, we will have to write code that is similar to i2c designware. > > Will this type of IP not be allowed to merge? > > It's not that you are not allowed to change the existing driver. It is > definitely encouraged to avoid unnecessary duplication. If possible think > of the core parts as library that your glue driver can call and if > something is missing you can always add (or modify). > > Also while there, let's start by adding support for the master operations > first and then later on you can add the slave bits. > > Hope this helps. Thanks for your reply. I have a general understanding. First, I will complete the i2c Starfive Master driver and appropriately modify the i2c designware related functions into callback form Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-05-27 8:50 [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support lianfeng.ouyang ` (3 preceding siblings ...) 2026-05-27 12:36 ` [PATCH v2 0/3] i2c: Add Starfive " Mika Westerberg @ 2026-06-02 22:26 ` Andy Shevchenko 2026-06-03 5:31 ` 回复: " Lianfeng Ouyang 4 siblings, 1 reply; 25+ messages in thread From: Andy Shevchenko @ 2026-06-02 22:26 UTC (permalink / raw) To: lianfeng.ouyang Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c, devicetree, linux-kernel On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > The Starfive JHB100 I2C controller is a variant of the widely-used > DesignWare I2C IP, with a distinct register layout and enhanced features > such as SMBus Alert and programmable FIFO depths. > > The series is structured as follows: > 1. Adds the device tree binding document for the starfive,jhb100-i2c > compatible. > 2. Prepares the existing i2c-designware-core by exporting and making > certain key functions overridable, allowing code reuse. > 3. Introduces the new i2c-starfive-* driver, with separate modules for > master and slave functionality, based on the 2023-07 revision of > the Synopsys IP manual. > > Currently, due to the following differences, i2c designware cannot be > fully reused > 1. For high and low level counting settings at different rates, i2c > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > 2. Interrupt clearing is achieved by writing 1 to the corresponding > bit of INTR_CLR, while designware reads different clearing > registers > 3. Master and slave require separate probe callbacks and cannot rely > solely on the runtime mode switching provided by i2c_dw_set_mode() > 4. The value of FIFO depth is not obtained through registers, but > written through DTS NAK in this form. We well discourage code duplication and ugly ifdeffery with full of __weak annotations that may not be present in the regular driver. There is not even a tiny bit of justification for this nonsense. TL;DR: this series needs much more work. > I have written some poorly styled code to reduce changes to i2c designware > and reuse its functions by keeping aa always true, for example > 1. the implementation of i2c-d w_probe_master() differs only for the two > IPs in i2c_dw_set_timits_master(). In order to reuse > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > __weak. A better approach is to use a callback function, but using > a callback function requires changing more i2c designware files. > I don't know what the attitude of the community is > 2. For the operation of clearing interrupt flags, i2c designware reads > and i2c starfive writes. Therefore, in order not to modify the > relevant logic of i2c designware, I added a write operation to > sf_reg_read() > So I think this version of the code is not allowed to merge, but I don't > know how to handle this situation because if i2c designware is not changed > at all, we will have to write code that is similar to i2c designware. > Will this type of IP not be allowed to merge? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-06-02 22:26 ` Andy Shevchenko @ 2026-06-03 5:31 ` Lianfeng Ouyang 2026-06-03 5:48 ` Andy Shevchenko 0 siblings, 1 reply; 25+ messages in thread From: Lianfeng Ouyang @ 2026-06-03 5:31 UTC (permalink / raw) To: Andy Shevchenko Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > 发送时间: 2026年6月3日 6:27 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > 抄送: Andi Shyti <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; > linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > DesignWare I2C IP, with a distinct register layout and enhanced features > > such as SMBus Alert and programmable FIFO depths. > > > > The series is structured as follows: > > 1. Adds the device tree binding document for the starfive,jhb100-i2c > > compatible. > > 2. Prepares the existing i2c-designware-core by exporting and making > > certain key functions overridable, allowing code reuse. > > 3. Introduces the new i2c-starfive-* driver, with separate modules for > > master and slave functionality, based on the 2023-07 revision of > > the Synopsys IP manual. > > > > Currently, due to the following differences, i2c designware cannot be > > fully reused > > 1. For high and low level counting settings at different rates, i2c > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > bit of INTR_CLR, while designware reads different clearing > > registers > > 3. Master and slave require separate probe callbacks and cannot rely > > solely on the runtime mode switching provided by > i2c_dw_set_mode() > > 4. The value of FIFO depth is not obtained through registers, but > > written through DTS > > NAK in this form. We well discourage code duplication and ugly ifdeffery with > full of __weak annotations that may not be present in the regular driver. There > is not even a tiny bit of justification for this nonsense. > > TL;DR: this series needs much more work. > > > I have written some poorly styled code to reduce changes to i2c designware > > and reuse its functions by keeping aa always true, for example > > 1. the implementation of i2c-d w_probe_master() differs only for the two > > IPs in i2c_dw_set_timits_master(). In order to reuse > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > > __weak. A better approach is to use a callback function, but using > > a callback function requires changing more i2c designware files. > > I don't know what the attitude of the community is > > 2. For the operation of clearing interrupt flags, i2c designware reads > > and i2c starfive writes. Therefore, in order not to modify the > > relevant logic of i2c designware, I added a write operation to > > sf_reg_read() > > So I think this version of the code is not allowed to merge, but I don't > > know how to handle this situation because if i2c designware is not changed > > at all, we will have to write code that is similar to i2c designware. > > Will this type of IP not be allowed to merge? > > -- > With Best Regards, > Andy Shevchenko > Thanks for the review. In the future, the designware will be changed to the form of callback functions, and then callback functions will be passed in i2c starry - * and implemented using designware as a library Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-06-03 5:31 ` 回复: " Lianfeng Ouyang @ 2026-06-03 5:48 ` Andy Shevchenko 2026-06-03 6:09 ` 回复: " Lianfeng Ouyang 0 siblings, 1 reply; 25+ messages in thread From: Andy Shevchenko @ 2026-06-03 5:48 UTC (permalink / raw) To: Lianfeng Ouyang Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote: > > -----邮件原件----- > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > 发送时间: 2026年6月3日 6:27 > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > > DesignWare I2C IP, with a distinct register layout and enhanced features > > > such as SMBus Alert and programmable FIFO depths. > > > > > > The series is structured as follows: > > > 1. Adds the device tree binding document for the starfive,jhb100-i2c > > > compatible. > > > 2. Prepares the existing i2c-designware-core by exporting and making > > > certain key functions overridable, allowing code reuse. > > > 3. Introduces the new i2c-starfive-* driver, with separate modules for > > > master and slave functionality, based on the 2023-07 revision of > > > the Synopsys IP manual. > > > > > > Currently, due to the following differences, i2c designware cannot be > > > fully reused > > > 1. For high and low level counting settings at different rates, i2c > > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > bit of INTR_CLR, while designware reads different clearing > > > registers > > > 3. Master and slave require separate probe callbacks and cannot rely > > > solely on the runtime mode switching provided by > > i2c_dw_set_mode() > > > 4. The value of FIFO depth is not obtained through registers, but > > > written through DTS > > > > NAK in this form. We well discourage code duplication and ugly ifdeffery with > > full of __weak annotations that may not be present in the regular driver. There > > is not even a tiny bit of justification for this nonsense. > > > > TL;DR: this series needs much more work. > > > > > I have written some poorly styled code to reduce changes to i2c designware > > > and reuse its functions by keeping aa always true, for example > > > 1. the implementation of i2c-d w_probe_master() differs only for the two > > > IPs in i2c_dw_set_timits_master(). In order to reuse > > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > > > __weak. A better approach is to use a callback function, but using > > > a callback function requires changing more i2c designware files. > > > I don't know what the attitude of the community is > > > 2. For the operation of clearing interrupt flags, i2c designware reads > > > and i2c starfive writes. Therefore, in order not to modify the > > > relevant logic of i2c designware, I added a write operation to > > > sf_reg_read() > > > So I think this version of the code is not allowed to merge, but I don't > > > know how to handle this situation because if i2c designware is not changed > > > at all, we will have to write code that is similar to i2c designware. > > > Will this type of IP not be allowed to merge? > > Thanks for the review. > > In the future, the designware will be changed to the form of callback functions, > and then callback functions will be passed in i2c starry - * and implemented > using designware as a library Why you can't specify your own regmap as it was done in Baikal case? What are the obstacles to achieve that? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-06-03 5:48 ` Andy Shevchenko @ 2026-06-03 6:09 ` Lianfeng Ouyang 2026-06-03 6:39 ` Andy Shevchenko 0 siblings, 1 reply; 25+ messages in thread From: Lianfeng Ouyang @ 2026-06-03 6:09 UTC (permalink / raw) To: Andy Shevchenko Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > 发送时间: 2026年6月3日 13:49 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > 抄送: Andi Shyti <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; > linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave > support > > On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote: > > > -----邮件原件----- > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > 发送时间: 2026年6月3日 6:27 > > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > > > DesignWare I2C IP, with a distinct register layout and enhanced features > > > > such as SMBus Alert and programmable FIFO depths. > > > > > > > > The series is structured as follows: > > > > 1. Adds the device tree binding document for the starfive,jhb100-i2c > > > > compatible. > > > > 2. Prepares the existing i2c-designware-core by exporting and making > > > > certain key functions overridable, allowing code reuse. > > > > 3. Introduces the new i2c-starfive-* driver, with separate modules for > > > > master and slave functionality, based on the 2023-07 revision of > > > > the Synopsys IP manual. > > > > > > > > Currently, due to the following differences, i2c designware cannot be > > > > fully reused > > > > 1. For high and low level counting settings at different rates, i2c > > > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > > bit of INTR_CLR, while designware reads different clearing > > > > registers > > > > 3. Master and slave require separate probe callbacks and cannot rely > > > > solely on the runtime mode switching provided by > > > i2c_dw_set_mode() > > > > 4. The value of FIFO depth is not obtained through registers, but > > > > written through DTS > > > > > > NAK in this form. We well discourage code duplication and ugly ifdeffery with > > > full of __weak annotations that may not be present in the regular driver. > There > > > is not even a tiny bit of justification for this nonsense. > > > > > > TL;DR: this series needs much more work. > > > > > > > I have written some poorly styled code to reduce changes to i2c > designware > > > > and reuse its functions by keeping aa always true, for example > > > > 1. the implementation of i2c-d w_probe_master() differs only for the two > > > > IPs in i2c_dw_set_timits_master(). In order to reuse > > > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > > > > __weak. A better approach is to use a callback function, but using > > > > a callback function requires changing more i2c designware files. > > > > I don't know what the attitude of the community is > > > > 2. For the operation of clearing interrupt flags, i2c designware reads > > > > and i2c starfive writes. Therefore, in order not to modify the > > > > relevant logic of i2c designware, I added a write operation to > > > > sf_reg_read() > > > > So I think this version of the code is not allowed to merge, but I don't > > > > know how to handle this situation because if i2c designware is not changed > > > > at all, we will have to write code that is similar to i2c designware. > > > > Will this type of IP not be allowed to merge? > > > > Thanks for the review. > > > > In the future, the designware will be changed to the form of callback functions, > > and then callback functions will be passed in i2c starry - * and implemented > > using designware as a library > > Why you can't specify your own regmap as it was done in Baikal case? What are > the obstacles to achieve that? > > -- > With Best Regards, > Andy Shevchenko > Hi Andy, The main reasons are as follows 1. For high and low level counting settings at different rates, i2c starfive just use IC_SCL_H/LCNT to set SS, FM, FM+, UFM, ====> Therefore, it is not possible to directly use the i2c_dew_set_timits_master() of designware, Because the definition of registers has changed 2. Interrupt clearing is achieved by writing 1 to the corresponding bit of INTR_CLR, while designware reads different clearing registers ====> The way of operating registers is different, so it cannot be distinguished solely by address or offset, and can only be adapted to operations belonging to i2c starfive through callback functions 3. Master and slave require separate probe callbacks and cannot rely solely on the runtime mode switching provided by i2c_dw_set_mode() ====> There are host and slave IP addresses separately, unlike designware where one IP supports two roles, because the logic of switching roles cannot be distinguished by address and offset 4. The value of FIFO depth is not obtained through registers, but written through DTS ====> The register does not have information on the depth of FIFO, and manual writing of register settings is required In addition to the above four points, the initialization, transmission, and interrupts of i2c Starfive are basically the same as i2c Desirware. Therefore, we hope to reuse i2c designware instead of re implementing the i2c Starfive driver, otherwise the code repetition will be high Hope I can answer your question, thank you ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-06-03 6:09 ` 回复: " Lianfeng Ouyang @ 2026-06-03 6:39 ` Andy Shevchenko 2026-06-04 3:00 ` 回复: " Lianfeng Ouyang 0 siblings, 1 reply; 25+ messages in thread From: Andy Shevchenko @ 2026-06-03 6:39 UTC (permalink / raw) To: Lianfeng Ouyang Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org On Wed, Jun 03, 2026 at 06:09:24AM +0000, Lianfeng Ouyang wrote: > > -----邮件原件----- > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > 发送时间: 2026年6月3日 13:49 > > On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote: > > > > -----邮件原件----- > > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > 发送时间: 2026年6月3日 6:27 > > > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > > > > DesignWare I2C IP, with a distinct register layout and enhanced features > > > > > such as SMBus Alert and programmable FIFO depths. > > > > > > > > > > The series is structured as follows: > > > > > 1. Adds the device tree binding document for the starfive,jhb100-i2c > > > > > compatible. > > > > > 2. Prepares the existing i2c-designware-core by exporting and making > > > > > certain key functions overridable, allowing code reuse. > > > > > 3. Introduces the new i2c-starfive-* driver, with separate modules for > > > > > master and slave functionality, based on the 2023-07 revision of > > > > > the Synopsys IP manual. > > > > > > > > > > Currently, due to the following differences, i2c designware cannot be > > > > > fully reused > > > > > 1. For high and low level counting settings at different rates, i2c > > > > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > > > bit of INTR_CLR, while designware reads different clearing > > > > > registers > > > > > 3. Master and slave require separate probe callbacks and cannot rely > > > > > solely on the runtime mode switching provided by > > > > i2c_dw_set_mode() > > > > > 4. The value of FIFO depth is not obtained through registers, but > > > > > written through DTS > > > > > > > > NAK in this form. We well discourage code duplication and ugly ifdeffery with > > > > full of __weak annotations that may not be present in the regular driver. > > There > > > > is not even a tiny bit of justification for this nonsense. > > > > > > > > TL;DR: this series needs much more work. > > > > > > > > > I have written some poorly styled code to reduce changes to i2c > > designware > > > > > and reuse its functions by keeping aa always true, for example > > > > > 1. the implementation of i2c-d w_probe_master() differs only for the two > > > > > IPs in i2c_dw_set_timits_master(). In order to reuse > > > > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > > > > > __weak. A better approach is to use a callback function, but using > > > > > a callback function requires changing more i2c designware files. > > > > > I don't know what the attitude of the community is > > > > > 2. For the operation of clearing interrupt flags, i2c designware reads > > > > > and i2c starfive writes. Therefore, in order not to modify the > > > > > relevant logic of i2c designware, I added a write operation to > > > > > sf_reg_read() > > > > > So I think this version of the code is not allowed to merge, but I don't > > > > > know how to handle this situation because if i2c designware is not changed > > > > > at all, we will have to write code that is similar to i2c designware. > > > > > Will this type of IP not be allowed to merge? > > > > > > Thanks for the review. > > > > > > In the future, the designware will be changed to the form of callback functions, > > > and then callback functions will be passed in i2c starry - * and implemented > > > using designware as a library > > > > Why you can't specify your own regmap as it was done in Baikal case? What are > > the obstacles to achieve that? > > The main reasons are as follows > 1. For high and low level counting settings at different rates, i2c > starfive just use IC_SCL_H/LCNT to set SS, FM, FM+, UFM, > ====> Therefore, it is not possible to directly use the i2c_dew_set_timits_master() > of designware, Because the definition of registers has changed So, it's not a DW per se? What the DW databook reflects the register layout and other bits? I.o.w. which version of DW i2c IP is this? > 2. Interrupt clearing is achieved by writing 1 to the corresponding > bit of INTR_CLR, while designware reads different clearing > registers > ====> The way of operating registers is different, so it cannot be > distinguished solely by address or offset, and can only be adapted > to operations belonging to i2c starfive through callback functions This can be done in the custom regmap functions. You know that you may redefine the regmap IO accessors and do whatever you want there based on the registers. Yes, what you are probably talking about is a workflow. But we basically need to see a patch-per-workflow change for each case to understand the differences better. I.o.w. if you need, for instance, separate or special IRQ handling part, create a patch to move existing code to a callback with explanation why it's needed. All the same for the timings and so on. It would be nice to have a datasheet at hand to actually read and see the differences. Without that it's even hard to propose better solutions. But current state is for sure no go. > 3. Master and slave require separate probe callbacks and cannot rely > solely on the runtime mode switching provided by i2c_dw_set_mode() > ====> There are host and slave IP addresses separately, unlike designware > where one IP supports two roles, because the logic of switching roles > cannot be distinguished by address and offset This feature should be left at last, but altogether it smells like either a completely new i2c DW design (like they did for DMA and SSI at some point) and most likely will need a brand new driver explaining all of this with a link to a datasheet. > 4. The value of FIFO depth is not obtained through registers, but > written through DTS > ====> The register does not have information on the depth of FIFO, > and manual writing of register settings is required > > In addition to the above four points, the initialization, transmission, and interrupts > of i2c Starfive are basically the same as i2c Desirware. Therefore, we hope to reuse > i2c designware instead of re implementing the i2c Starfive driver, otherwise the code repetition will be high These sounds like candidates for i2c-designware-lib.c, indeed. But the rest most likely shouldn't be intervened at all. I.o.w. please provide a detailed analysis (something like [1]) in the cover letter, or just send an RFC on the driver design before even starting considering actual coding. > Hope I can answer your question, thank you [1]: e539f435cb9c ("spi: dw: Add support for DesignWare DWC_ssi") -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-06-03 6:39 ` Andy Shevchenko @ 2026-06-04 3:00 ` Lianfeng Ouyang 2026-06-04 18:09 ` Andy Shevchenko 0 siblings, 1 reply; 25+ messages in thread From: Lianfeng Ouyang @ 2026-06-04 3:00 UTC (permalink / raw) To: Andy Shevchenko Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > 发送时间: 2026年6月3日 14:40 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > 抄送: Andi Shyti <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; > linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C > master/slave support > > On Wed, Jun 03, 2026 at 06:09:24AM +0000, Lianfeng Ouyang wrote: > > > -----邮件原件----- > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > 发送时间: 2026年6月3日 13:49 > > > On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote: > > > > > -----邮件原件----- > > > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > 发送时间: 2026年6月3日 6:27 > > > > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > > > > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > > > > > DesignWare I2C IP, with a distinct register layout and enhanced > features > > > > > > such as SMBus Alert and programmable FIFO depths. > > > > > > > > > > > > The series is structured as follows: > > > > > > 1. Adds the device tree binding document for the starfive,jhb100-i2c > > > > > > compatible. > > > > > > 2. Prepares the existing i2c-designware-core by exporting and > making > > > > > > certain key functions overridable, allowing code reuse. > > > > > > 3. Introduces the new i2c-starfive-* driver, with separate modules for > > > > > > master and slave functionality, based on the 2023-07 revision > of > > > > > > the Synopsys IP manual. > > > > > > > > > > > > Currently, due to the following differences, i2c designware cannot be > > > > > > fully reused > > > > > > 1. For high and low level counting settings at different rates, i2c > > > > > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > > > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > > > > bit of INTR_CLR, while designware reads different clearing > > > > > > registers > > > > > > 3. Master and slave require separate probe callbacks and cannot rely > > > > > > solely on the runtime mode switching provided by > > > > > i2c_dw_set_mode() > > > > > > 4. The value of FIFO depth is not obtained through registers, but > > > > > > written through DTS > > > > > > > > > > NAK in this form. We well discourage code duplication and ugly ifdeffery > with > > > > > full of __weak annotations that may not be present in the regular driver. > > > There > > > > > is not even a tiny bit of justification for this nonsense. > > > > > > > > > > TL;DR: this series needs much more work. > > > > > > > > > > > I have written some poorly styled code to reduce changes to i2c > > > designware > > > > > > and reuse its functions by keeping aa always true, for example > > > > > > 1. the implementation of i2c-d w_probe_master() differs only for the > two > > > > > > IPs in i2c_dw_set_timits_master(). In order to reuse > > > > > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > > > > > > __weak. A better approach is to use a callback function, but using > > > > > > a callback function requires changing more i2c designware files. > > > > > > I don't know what the attitude of the community is > > > > > > 2. For the operation of clearing interrupt flags, i2c designware reads > > > > > > and i2c starfive writes. Therefore, in order not to modify the > > > > > > relevant logic of i2c designware, I added a write operation to > > > > > > sf_reg_read() > > > > > > So I think this version of the code is not allowed to merge, but I don't > > > > > > know how to handle this situation because if i2c designware is not > changed > > > > > > at all, we will have to write code that is similar to i2c designware. > > > > > > Will this type of IP not be allowed to merge? > > > > > > > > Thanks for the review. > > > > > > > > In the future, the designware will be changed to the form of callback > functions, > > > > and then callback functions will be passed in i2c starry - * and implemented > > > > using designware as a library > > > > > > Why you can't specify your own regmap as it was done in Baikal case? What > are > > > the obstacles to achieve that? > > > > The main reasons are as follows > > 1. For high and low level counting settings at different rates, i2c > > starfive just use IC_SCL_H/LCNT to set SS, FM, FM+, UFM, > > ====> Therefore, it is not possible to directly use the > i2c_dew_set_timits_master() > > of designware, Because the definition of registers has changed > > So, it's not a DW per se? What the DW databook reflects the register layout and > other bits? I.o.w. which version of DW i2c IP is this? > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > bit of INTR_CLR, while designware reads different clearing > > registers > > ====> The way of operating registers is different, so it cannot be > > distinguished solely by address or offset, and can only be adapted > > to operations belonging to i2c starfive through callback functions > > This can be done in the custom regmap functions. You know that you may > redefine the regmap IO accessors and do whatever you want there based on > the registers. Yes, what you are probably talking about is a workflow. > > But we basically need to see a patch-per-workflow change for each case > to understand the differences better. I.o.w. if you need, for instance, > separate or special IRQ handling part, create a patch to move existing > code to a callback with explanation why it's needed. All the same for > the timings and so on. It would be nice to have a datasheet at hand to > actually read and see the differences. Without that it's even hard to > propose better solutions. But current state is for sure no go. > > > 3. Master and slave require separate probe callbacks and cannot rely > > solely on the runtime mode switching provided by i2c_dw_set_mode() > > ====> There are host and slave IP addresses separately, unlike designware > > where one IP supports two roles, because the logic of switching roles > > cannot be distinguished by address and offset > > This feature should be left at last, but altogether it smells like either > a completely new i2c DW design (like they did for DMA and SSI at some > point) and most likely will need a brand new driver explaining all of this > with a link to a datasheet. > > > 4. The value of FIFO depth is not obtained through registers, but > > written through DTS > > ====> The register does not have information on the depth of FIFO, > > and manual writing of register settings is required > > > > In addition to the above four points, the initialization, transmission, and > interrupts > > of i2c Starfive are basically the same as i2c Desirware. Therefore, we hope to > reuse > > i2c designware instead of re implementing the i2c Starfive driver, otherwise > the code repetition will be high > > These sounds like candidates for i2c-designware-lib.c, indeed. > But the rest most likely shouldn't be intervened at all. I.o.w. > please provide a detailed analysis (something like [1]) in > the cover letter, or just send an RFC on the driver design > before even starting considering actual coding. > > > Hope I can answer your question, thank you > > [1]: e539f435cb9c ("spi: dw: Add support for DesignWare DWC_ssi") > > > -- > With Best Regards, > Andy Shevchenko > Okay, based on the format of e539f435cb9c, I have compiled the differences in register offsets and the differences between registers IC_CTRL and IC-INTR_CLR. Looking forward to your reply DWC_ADV_i2c is the enhanced version of DW_apb_i2c (corresponding to existing i2c designware driver), Most of their registers are similar, but the bit fields of key registers (e.g. IC_CTRL/IC_CON, IC_TAR, IC_ENABLE) are different, and DWC_ADV_i2c has additional registers for mission critical features and newer protocol version support. DWC_ADV_i2c has additional features compared to DW_apb_i2c. Major enhancements in DWC_ADV_i2c are: - Support for I2C Bus Specification V7.0 (October 2021) and SMBus Specification V3.2 (January 2022), while DW_apb_i2c supports I2C V6.0 (April 2014) and SMBus V3.1 (March 2018) - Support for SMBus Non-ARP Command Code Decoding and Advanced Block Read/Write - Support for dynamic target address update (IC_DYNAMIC_TAR_UPDATE=1) - Support for Ultra-Fast Mode (up to 5 MB/s) with dedicated timing registers - Both DWC_ADV_i2c and DW_apb_i2c are APB slave devices, supporting APB2/APB3/APB4 interfaces with configurable data width (8/16/32 bits). Register offset (base address 0x0 for both, DWC_ADV_i2c uses separate address blocks for operational, I2C and SMBus registers, DW_apb_i2c uses a unified register map) DWC_ADV_i2c DW_apb_i2c IC_HCI_VERSION 0x00 N/A IC_ENABLE 0x04 0x6c IC_RESET_CTRL 0x08 N/A IC_CAPABILITIES 0x0c N/A IC_I2C_CAPABILITIES 0x10 N/A IC_SMBUS_CAPABILITIES 0x18 0x9c IC_SMBUS_BLOCK_OFFSET 0x1c 0xa0 IC_CTRL 0x04 (I2C block) 0x00 (IC_CON) IC_TAR 0x08 (I2C block) 0x04 IC_DAR 0x0c (I2C block) 0x08 (IC_SAR) IC_HS_CADDR 0x10 (I2C block) 0x0c (IC_HS_MADDR) IC_DATA_CMD 0x58 (I2C block) 0x10 IC_UFM_TBUF_CNT 0x20 (I2C block) N/A IC_SCL_HCNT 0x24 (I2C block) 0x1c (IC_FS_SCL_HCNT) IC_SCL_LCNT 0x28 (I2C block) 0x20 (IC_FS_SCL_LCNT) IC_HS_SCL_HCNT 0x2c (I2C block) 0x24 IC_HS_SCL_LCNT 0x30 (I2C block) 0x28 IC_SDA_HOLD 0x34 (I2C block) 0x7c IC_SDA_SETUP 0x38 (I2C block) 0x94 IC_SPKLEN 0x3c (I2C block) 0xa0 (IC_FS_SPKLEN) IC_HS_SPKLEN 0x40 (I2C block) 0xa4 IC_DMA_RDLR 0x6c (I2C block) 0x90 IC_INTR_STAT 0x74 (I2C block) 0x2c IC_INTR_MASK 0x78 (I2C block) 0x30 IC_RAW_INTR_STAT 0x7c (I2C block) 0x34 IC_DAR4 0xb8 (I2C block) 0x108 (IC_SAR4) IC_SFTY_CTRL 0xc0 (I2C block) N/A IC_SMBUS_CTRL 0x04 (SMBus block) N/A IC_SMBUS_ARP_CTRL 0x08 (SMBus block) N/A IC_SMBUS_INTR_STAT 0x28 (SMBus block) 0xc8 IC_SMBUS_INTR_MASK 0x2c (SMBus block) 0xcc IC_SMBUS_INTR_RAW_STAT 0x30 (SMBus block) 0xd0 Register configuration - IC_CTRL (DWC_ADV_i2c) vs IC_CON (DW_apb_i2c) DWC_Adv_i2c DW_apb_i2c IC_OP_MODE bit[0] bit[0] (MASTER_MODE) IC_SPEED bit[5:4] bit[2:1] (SPEED) IC_10BITADDR_TGT bit[8] bit[3] (IC_10BITADDR_SLAVE) IC_10BITADDR_CTRLR bit[9] bit[4] (IC_10BITADDR_MASTER) BUS_CLEAR_FEATURE_CTRL bit[14] bit[11] Register configuration - IC_INTR_CLR DWC_ADV_i2c DW_apb_i2c CLR_INTR bit[0] (write) IC_INTR_CLR bit[0] (read) CLR_RX_UNDER bit[1] (write) IC_CLR_RX_UNDER bit[0] (read) CLR_RX_OVER bit[2] (write) IC_CLR_RX_OVER bit[0] (read) CLR_TX_OVER bit[3] (write) IC_CLR_TX_OVER bit[0] (read) CLR_RD_REQ bit[4] (write) IC_CLR_RD_REQ bit[0] (read) ...... The documents used are [1] DesignWare ® Cores Advanced I2C/SMBus Controller and Target Device Databook, Product Code H464-0, I2C Bus Spec V7.0, [2] DesignWare ® DW_apb_i2c Databook, Version 2.03a, December 2020, I2C Bus Spec V6.0 Best Regards, Lianfeng Ouyang ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 回复: 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-06-04 3:00 ` 回复: " Lianfeng Ouyang @ 2026-06-04 18:09 ` Andy Shevchenko 2026-06-08 9:48 ` 回复: " Lianfeng Ouyang 0 siblings, 1 reply; 25+ messages in thread From: Andy Shevchenko @ 2026-06-04 18:09 UTC (permalink / raw) To: Lianfeng Ouyang Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org On Thu, Jun 04, 2026 at 03:00:13AM +0000, Lianfeng Ouyang wrote: > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > 发送时间: 2026年6月3日 14:40 > > On Wed, Jun 03, 2026 at 06:09:24AM +0000, Lianfeng Ouyang wrote: > > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > 发送时间: 2026年6月3日 13:49 > > > > On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote: > > > > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > 发送时间: 2026年6月3日 6:27 > > > > > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > > > > > > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > > > > > > DesignWare I2C IP, with a distinct register layout and enhanced > > features > > > > > > > such as SMBus Alert and programmable FIFO depths. > > > > > > > > > > > > > > The series is structured as follows: > > > > > > > 1. Adds the device tree binding document for the starfive,jhb100-i2c > > > > > > > compatible. > > > > > > > 2. Prepares the existing i2c-designware-core by exporting and > > making > > > > > > > certain key functions overridable, allowing code reuse. > > > > > > > 3. Introduces the new i2c-starfive-* driver, with separate modules for > > > > > > > master and slave functionality, based on the 2023-07 revision > > of > > > > > > > the Synopsys IP manual. > > > > > > > > > > > > > > Currently, due to the following differences, i2c designware cannot be > > > > > > > fully reused > > > > > > > 1. For high and low level counting settings at different rates, i2c > > > > > > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > > > > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > > > > > bit of INTR_CLR, while designware reads different clearing > > > > > > > registers > > > > > > > 3. Master and slave require separate probe callbacks and cannot rely > > > > > > > solely on the runtime mode switching provided by > > > > > > i2c_dw_set_mode() > > > > > > > 4. The value of FIFO depth is not obtained through registers, but > > > > > > > written through DTS > > > > > > > > > > > > NAK in this form. We well discourage code duplication and ugly ifdeffery > > with > > > > > > full of __weak annotations that may not be present in the regular driver. > > > > There > > > > > > is not even a tiny bit of justification for this nonsense. > > > > > > > > > > > > TL;DR: this series needs much more work. > > > > > > > > > > > > > I have written some poorly styled code to reduce changes to i2c > > > > designware > > > > > > > and reuse its functions by keeping aa always true, for example > > > > > > > 1. the implementation of i2c-d w_probe_master() differs only for the > > two > > > > > > > IPs in i2c_dw_set_timits_master(). In order to reuse > > > > > > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as > > > > > > > __weak. A better approach is to use a callback function, but using > > > > > > > a callback function requires changing more i2c designware files. > > > > > > > I don't know what the attitude of the community is > > > > > > > 2. For the operation of clearing interrupt flags, i2c designware reads > > > > > > > and i2c starfive writes. Therefore, in order not to modify the > > > > > > > relevant logic of i2c designware, I added a write operation to > > > > > > > sf_reg_read() > > > > > > > So I think this version of the code is not allowed to merge, but I don't > > > > > > > know how to handle this situation because if i2c designware is not > > changed > > > > > > > at all, we will have to write code that is similar to i2c designware. > > > > > > > Will this type of IP not be allowed to merge? > > > > > > > > > > Thanks for the review. > > > > > > > > > > In the future, the designware will be changed to the form of callback > > functions, > > > > > and then callback functions will be passed in i2c starry - * and implemented > > > > > using designware as a library > > > > > > > > Why you can't specify your own regmap as it was done in Baikal case? What > > are > > > > the obstacles to achieve that? > > > > > > The main reasons are as follows > > > 1. For high and low level counting settings at different rates, i2c > > > starfive just use IC_SCL_H/LCNT to set SS, FM, FM+, UFM, > > > ====> Therefore, it is not possible to directly use the > > i2c_dew_set_timits_master() > > > of designware, Because the definition of registers has changed > > > > So, it's not a DW per se? What the DW databook reflects the register layout and > > other bits? I.o.w. which version of DW i2c IP is this? > > > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > bit of INTR_CLR, while designware reads different clearing > > > registers > > > ====> The way of operating registers is different, so it cannot be > > > distinguished solely by address or offset, and can only be adapted > > > to operations belonging to i2c starfive through callback functions > > > > This can be done in the custom regmap functions. You know that you may > > redefine the regmap IO accessors and do whatever you want there based on > > the registers. Yes, what you are probably talking about is a workflow. > > > > But we basically need to see a patch-per-workflow change for each case > > to understand the differences better. I.o.w. if you need, for instance, > > separate or special IRQ handling part, create a patch to move existing > > code to a callback with explanation why it's needed. All the same for > > the timings and so on. It would be nice to have a datasheet at hand to > > actually read and see the differences. Without that it's even hard to > > propose better solutions. But current state is for sure no go. > > > > > 3. Master and slave require separate probe callbacks and cannot rely > > > solely on the runtime mode switching provided by i2c_dw_set_mode() > > > ====> There are host and slave IP addresses separately, unlike designware > > > where one IP supports two roles, because the logic of switching roles > > > cannot be distinguished by address and offset > > > > This feature should be left at last, but altogether it smells like either > > a completely new i2c DW design (like they did for DMA and SSI at some > > point) and most likely will need a brand new driver explaining all of this > > with a link to a datasheet. > > > > > 4. The value of FIFO depth is not obtained through registers, but > > > written through DTS > > > ====> The register does not have information on the depth of FIFO, > > > and manual writing of register settings is required > > > > > > In addition to the above four points, the initialization, transmission, and > > interrupts > > > of i2c Starfive are basically the same as i2c Desirware. Therefore, we hope to > > reuse > > > i2c designware instead of re implementing the i2c Starfive driver, otherwise > > the code repetition will be high > > > > These sounds like candidates for i2c-designware-lib.c, indeed. > > But the rest most likely shouldn't be intervened at all. I.o.w. > > please provide a detailed analysis (something like [1]) in > > the cover letter, or just send an RFC on the driver design > > before even starting considering actual coding. > > > > > Hope I can answer your question, thank you > > > > [1]: e539f435cb9c ("spi: dw: Add support for DesignWare DWC_ssi") > > Okay, based on the format of e539f435cb9c, I have compiled the differences in > register offsets and the differences between registers IC_CTRL and IC-INTR_CLR. > Looking forward to your reply Thank you for this information, it makes much more easier to see the big picture. With the given information I think you most likely need a brand new driver. No callbacks for sure. The only thing that may be done is to split really common code and data structures in i2c-designware-lib.[ch] between two. This will require quite a bit of refactoring (as you most likely don't want to inherit all quirked and hacked fields from struct dw_i2c_dev, meaning you want to have something just very basic and common between two there. means that the current struct dw_i2c_dev will be morphed to something like drivers/i2c/busses/i2c-designware-lib.h: struct dw_i2c_common { ... }; drivers/i2c/busses/i2c-designware-core.h: struct dw_i2c_dev { struct dw_i2c_common icom; ... }; and in your new driver struct dw_adv_i2c_dev { struct dw_i2c_common icom; ... }; And in the drivers/i2c/busses/i2c-designware-lib.c: ... dw_i2c_common_...(struct dw_i2c_common *icom, ...) { ... } for the shared APIs. With that I predict a series out of ~10 patches (or more). The brand new driver is also a solution, but to go there you need to at least have a PoC of the above to justify that the shared code makes little sense. > DWC_ADV_i2c is the enhanced version of DW_apb_i2c (corresponding to existing i2c designware driver), > Most of their registers are similar, but the bit fields of key registers > (e.g. IC_CTRL/IC_CON, IC_TAR, IC_ENABLE) are different, and DWC_ADV_i2c has > additional registers for mission critical features and newer protocol version support. > > DWC_ADV_i2c has additional features compared to DW_apb_i2c. Major enhancements > in DWC_ADV_i2c are: > - Support for I2C Bus Specification V7.0 (October 2021) and SMBus Specification V3.2 (January 2022), > while DW_apb_i2c supports I2C V6.0 (April 2014) and SMBus V3.1 (March 2018) > - Support for SMBus Non-ARP Command Code Decoding and Advanced Block Read/Write > - Support for dynamic target address update (IC_DYNAMIC_TAR_UPDATE=1) > - Support for Ultra-Fast Mode (up to 5 MB/s) with dedicated timing registers > - Both DWC_ADV_i2c and DW_apb_i2c are APB slave devices, supporting APB2/APB3/APB4 > interfaces with configurable data width (8/16/32 bits). > > Register offset (base address 0x0 for both, DWC_ADV_i2c uses separate address blocks for > operational, I2C and SMBus registers, DW_apb_i2c uses a unified register map) > DWC_ADV_i2c DW_apb_i2c > IC_HCI_VERSION 0x00 N/A > IC_ENABLE 0x04 0x6c > IC_RESET_CTRL 0x08 N/A > IC_CAPABILITIES 0x0c N/A > IC_I2C_CAPABILITIES 0x10 N/A > IC_SMBUS_CAPABILITIES 0x18 0x9c > IC_SMBUS_BLOCK_OFFSET 0x1c 0xa0 > IC_CTRL 0x04 (I2C block) 0x00 (IC_CON) > IC_TAR 0x08 (I2C block) 0x04 > IC_DAR 0x0c (I2C block) 0x08 (IC_SAR) > IC_HS_CADDR 0x10 (I2C block) 0x0c (IC_HS_MADDR) > IC_DATA_CMD 0x58 (I2C block) 0x10 > IC_UFM_TBUF_CNT 0x20 (I2C block) N/A > IC_SCL_HCNT 0x24 (I2C block) 0x1c (IC_FS_SCL_HCNT) > IC_SCL_LCNT 0x28 (I2C block) 0x20 (IC_FS_SCL_LCNT) > IC_HS_SCL_HCNT 0x2c (I2C block) 0x24 > IC_HS_SCL_LCNT 0x30 (I2C block) 0x28 > IC_SDA_HOLD 0x34 (I2C block) 0x7c > IC_SDA_SETUP 0x38 (I2C block) 0x94 > IC_SPKLEN 0x3c (I2C block) 0xa0 (IC_FS_SPKLEN) > IC_HS_SPKLEN 0x40 (I2C block) 0xa4 > IC_DMA_RDLR 0x6c (I2C block) 0x90 > IC_INTR_STAT 0x74 (I2C block) 0x2c > IC_INTR_MASK 0x78 (I2C block) 0x30 > IC_RAW_INTR_STAT 0x7c (I2C block) 0x34 > IC_DAR4 0xb8 (I2C block) 0x108 (IC_SAR4) > IC_SFTY_CTRL 0xc0 (I2C block) N/A > IC_SMBUS_CTRL 0x04 (SMBus block) N/A > IC_SMBUS_ARP_CTRL 0x08 (SMBus block) N/A > IC_SMBUS_INTR_STAT 0x28 (SMBus block) 0xc8 > IC_SMBUS_INTR_MASK 0x2c (SMBus block) 0xcc > IC_SMBUS_INTR_RAW_STAT 0x30 (SMBus block) 0xd0 > > Register configuration - IC_CTRL (DWC_ADV_i2c) vs IC_CON (DW_apb_i2c) > DWC_Adv_i2c DW_apb_i2c > IC_OP_MODE bit[0] bit[0] (MASTER_MODE) > IC_SPEED bit[5:4] bit[2:1] (SPEED) > IC_10BITADDR_TGT bit[8] bit[3] (IC_10BITADDR_SLAVE) > IC_10BITADDR_CTRLR bit[9] bit[4] (IC_10BITADDR_MASTER) > BUS_CLEAR_FEATURE_CTRL bit[14] bit[11] > > Register configuration - IC_INTR_CLR > DWC_ADV_i2c DW_apb_i2c > CLR_INTR bit[0] (write) IC_INTR_CLR bit[0] (read) > CLR_RX_UNDER bit[1] (write) IC_CLR_RX_UNDER bit[0] (read) > CLR_RX_OVER bit[2] (write) IC_CLR_RX_OVER bit[0] (read) > CLR_TX_OVER bit[3] (write) IC_CLR_TX_OVER bit[0] (read) > CLR_RD_REQ bit[4] (write) IC_CLR_RD_REQ bit[0] (read) > ...... > > The documents used are > [1] DesignWare ® Cores Advanced I2C/SMBus Controller and Target Device Databook, Product Code H464-0, I2C Bus Spec V7.0, > [2] DesignWare ® DW_apb_i2c Databook, Version 2.03a, December 2020, I2C Bus Spec V6.0 -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 25+ messages in thread
* 回复: 回复: 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support 2026-06-04 18:09 ` Andy Shevchenko @ 2026-06-08 9:48 ` Lianfeng Ouyang 0 siblings, 0 replies; 25+ messages in thread From: Lianfeng Ouyang @ 2026-06-08 9:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Andi Shyti, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > -----邮件原件----- > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > 发送时间: 2026年6月5日 2:09 > 收件人: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com> > 抄送: Andi Shyti <andi.shyti@kernel.org>; Rob Herring <robh@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Mika Westerberg <mika.westerberg@linux.intel.com>; > linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 主题: Re: 回复: 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C > master/slave support > > On Thu, Jun 04, 2026 at 03:00:13AM +0000, Lianfeng Ouyang wrote: > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > 发送时间: 2026年6月3日 14:40 > > > On Wed, Jun 03, 2026 at 06:09:24AM +0000, Lianfeng Ouyang wrote: > > > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > 发送时间: 2026年6月3日 13:49 > > > > > On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote: > > > > > > > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > > 发送时间: 2026年6月3日 6:27 > > > > > > > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote: > > > > > > > > > > > The Starfive JHB100 I2C controller is a variant of the widely-used > > > > > > > > DesignWare I2C IP, with a distinct register layout and enhanced > > > features > > > > > > > > such as SMBus Alert and programmable FIFO depths. > > > > > > > > > > > > > > > > The series is structured as follows: > > > > > > > > 1. Adds the device tree binding document for the > starfive,jhb100-i2c > > > > > > > > compatible. > > > > > > > > 2. Prepares the existing i2c-designware-core by exporting and > > > making > > > > > > > > certain key functions overridable, allowing code reuse. > > > > > > > > 3. Introduces the new i2c-starfive-* driver, with separate modules > for > > > > > > > > master and slave functionality, based on the 2023-07 > revision > > > of > > > > > > > > the Synopsys IP manual. > > > > > > > > > > > > > > > > Currently, due to the following differences, i2c designware cannot > be > > > > > > > > fully reused > > > > > > > > 1. For high and low level counting settings at different rates, i2c > > > > > > > > starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM > > > > > > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > > > > > > bit of INTR_CLR, while designware reads different > clearing > > > > > > > > registers > > > > > > > > 3. Master and slave require separate probe callbacks and cannot > rely > > > > > > > > solely on the runtime mode switching provided by > > > > > > > i2c_dw_set_mode() > > > > > > > > 4. The value of FIFO depth is not obtained through registers, but > > > > > > > > written through DTS > > > > > > > > > > > > > > NAK in this form. We well discourage code duplication and ugly > ifdeffery > > > with > > > > > > > full of __weak annotations that may not be present in the regular > driver. > > > > > There > > > > > > > is not even a tiny bit of justification for this nonsense. > > > > > > > > > > > > > > TL;DR: this series needs much more work. > > > > > > > > > > > > > > > I have written some poorly styled code to reduce changes to i2c > > > > > designware > > > > > > > > and reuse its functions by keeping aa always true, for example > > > > > > > > 1. the implementation of i2c-d w_probe_master() differs only for > the > > > two > > > > > > > > IPs in i2c_dw_set_timits_master(). In order to reuse > > > > > > > > i2c_dw_probe_master(), i2c_dw_set_timits_master is declared > as > > > > > > > > __weak. A better approach is to use a callback function, but using > > > > > > > > a callback function requires changing more i2c designware files. > > > > > > > > I don't know what the attitude of the community is > > > > > > > > 2. For the operation of clearing interrupt flags, i2c designware > reads > > > > > > > > and i2c starfive writes. Therefore, in order not to modify > the > > > > > > > > relevant logic of i2c designware, I added a write operation to > > > > > > > > sf_reg_read() > > > > > > > > So I think this version of the code is not allowed to merge, but I > don't > > > > > > > > know how to handle this situation because if i2c designware is not > > > changed > > > > > > > > at all, we will have to write code that is similar to i2c designware. > > > > > > > > Will this type of IP not be allowed to merge? > > > > > > > > > > > > Thanks for the review. > > > > > > > > > > > > In the future, the designware will be changed to the form of callback > > > functions, > > > > > > and then callback functions will be passed in i2c starry - * and > implemented > > > > > > using designware as a library > > > > > > > > > > Why you can't specify your own regmap as it was done in Baikal case? > What > > > are > > > > > the obstacles to achieve that? > > > > > > > > The main reasons are as follows > > > > 1. For high and low level counting settings at different rates, i2c > > > > starfive just use IC_SCL_H/LCNT to set SS, FM, FM+, UFM, > > > > ====> Therefore, it is not possible to directly use the > > > i2c_dew_set_timits_master() > > > > of designware, Because the definition of registers has changed > > > > > > So, it's not a DW per se? What the DW databook reflects the register layout > and > > > other bits? I.o.w. which version of DW i2c IP is this? > > > > > > > 2. Interrupt clearing is achieved by writing 1 to the corresponding > > > > bit of INTR_CLR, while designware reads different clearing > > > > registers > > > > ====> The way of operating registers is different, so it cannot be > > > > distinguished solely by address or offset, and can only be adapted > > > > to operations belonging to i2c starfive through callback functions > > > > > > This can be done in the custom regmap functions. You know that you may > > > redefine the regmap IO accessors and do whatever you want there based on > > > the registers. Yes, what you are probably talking about is a workflow. > > > > > > But we basically need to see a patch-per-workflow change for each case > > > to understand the differences better. I.o.w. if you need, for instance, > > > separate or special IRQ handling part, create a patch to move existing > > > code to a callback with explanation why it's needed. All the same for > > > the timings and so on. It would be nice to have a datasheet at hand to > > > actually read and see the differences. Without that it's even hard to > > > propose better solutions. But current state is for sure no go. > > > > > > > 3. Master and slave require separate probe callbacks and cannot rely > > > > solely on the runtime mode switching provided by i2c_dw_set_mode() > > > > ====> There are host and slave IP addresses separately, unlike designware > > > > where one IP supports two roles, because the logic of switching > roles > > > > cannot be distinguished by address and offset > > > > > > This feature should be left at last, but altogether it smells like either > > > a completely new i2c DW design (like they did for DMA and SSI at some > > > point) and most likely will need a brand new driver explaining all of this > > > with a link to a datasheet. > > > > > > > 4. The value of FIFO depth is not obtained through registers, but > > > > written through DTS > > > > ====> The register does not have information on the depth of FIFO, > > > > and manual writing of register settings is required > > > > > > > > In addition to the above four points, the initialization, transmission, and > > > interrupts > > > > of i2c Starfive are basically the same as i2c Desirware. Therefore, we hope > to > > > reuse > > > > i2c designware instead of re implementing the i2c Starfive driver, > otherwise > > > the code repetition will be high > > > > > > These sounds like candidates for i2c-designware-lib.c, indeed. > > > But the rest most likely shouldn't be intervened at all. I.o.w. > > > please provide a detailed analysis (something like [1]) in > > > the cover letter, or just send an RFC on the driver design > > > before even starting considering actual coding. > > > > > > > Hope I can answer your question, thank you > > > > > > [1]: e539f435cb9c ("spi: dw: Add support for DesignWare DWC_ssi") > > > > Okay, based on the format of e539f435cb9c, I have compiled the differences in > > register offsets and the differences between registers IC_CTRL and > IC-INTR_CLR. > > Looking forward to your reply > > Thank you for this information, it makes much more easier to see the big > picture. > With the given information I think you most likely need a brand new driver. > No callbacks for sure. The only thing that may be done is to split really common > code and data structures in i2c-designware-lib.[ch] between two. This will > require quite a bit of refactoring (as you most likely don't want to inherit > all quirked and hacked fields from struct dw_i2c_dev, meaning you want to > have something just very basic and common between two there. means that > the current struct dw_i2c_dev will be morphed to something like > > drivers/i2c/busses/i2c-designware-lib.h: > > struct dw_i2c_common { > ... > }; > > drivers/i2c/busses/i2c-designware-core.h: > > struct dw_i2c_dev { > struct dw_i2c_common icom; > ... > }; > > and in your new driver > > struct dw_adv_i2c_dev { > struct dw_i2c_common icom; > ... > }; > > And in the drivers/i2c/busses/i2c-designware-lib.c: > > ... dw_i2c_common_...(struct dw_i2c_common *icom, ...) > { > ... > } > > for the shared APIs. > > With that I predict a series out of ~10 patches (or more). > > The brand new driver is also a solution, but to go there you need to at least > have a PoC of the above to justify that the shared code makes little sense. > Thank you for your idea. I will modify my code in this direction Best Regards, Lianfeng Ouyang > > DWC_ADV_i2c is the enhanced version of DW_apb_i2c (corresponding to > existing i2c designware driver), > > Most of their registers are similar, but the bit fields of key registers > > (e.g. IC_CTRL/IC_CON, IC_TAR, IC_ENABLE) are different, and DWC_ADV_i2c > has > > additional registers for mission critical features and newer protocol version > support. > > > > DWC_ADV_i2c has additional features compared to DW_apb_i2c. Major > enhancements > > in DWC_ADV_i2c are: > > - Support for I2C Bus Specification V7.0 (October 2021) and SMBus > Specification V3.2 (January 2022), > > while DW_apb_i2c supports I2C V6.0 (April 2014) and SMBus V3.1 (March > 2018) > > - Support for SMBus Non-ARP Command Code Decoding and Advanced Block > Read/Write > > - Support for dynamic target address update (IC_DYNAMIC_TAR_UPDATE=1) > > - Support for Ultra-Fast Mode (up to 5 MB/s) with dedicated timing registers > > - Both DWC_ADV_i2c and DW_apb_i2c are APB slave devices, supporting > APB2/APB3/APB4 > > interfaces with configurable data width (8/16/32 bits). > > > > Register offset (base address 0x0 for both, DWC_ADV_i2c uses separate > address blocks for > > operational, I2C and SMBus registers, DW_apb_i2c uses a unified register > map) > > DWC_ADV_i2c DW_apb_i2c > > IC_HCI_VERSION 0x00 N/A > > IC_ENABLE 0x04 0x6c > > IC_RESET_CTRL 0x08 N/A > > IC_CAPABILITIES 0x0c N/A > > IC_I2C_CAPABILITIES 0x10 N/A > > IC_SMBUS_CAPABILITIES 0x18 0x9c > > IC_SMBUS_BLOCK_OFFSET 0x1c 0xa0 > > IC_CTRL 0x04 (I2C block) 0x00 (IC_CON) > > IC_TAR 0x08 (I2C block) 0x04 > > IC_DAR 0x0c (I2C block) 0x08 (IC_SAR) > > IC_HS_CADDR 0x10 (I2C block) 0x0c > (IC_HS_MADDR) > > IC_DATA_CMD 0x58 (I2C block) 0x10 > > IC_UFM_TBUF_CNT 0x20 (I2C block) N/A > > IC_SCL_HCNT 0x24 (I2C block) 0x1c > (IC_FS_SCL_HCNT) > > IC_SCL_LCNT 0x28 (I2C block) 0x20 > (IC_FS_SCL_LCNT) > > IC_HS_SCL_HCNT 0x2c (I2C block) 0x24 > > IC_HS_SCL_LCNT 0x30 (I2C block) 0x28 > > IC_SDA_HOLD 0x34 (I2C block) 0x7c > > IC_SDA_SETUP 0x38 (I2C block) 0x94 > > IC_SPKLEN 0x3c (I2C block) 0xa0 (IC_FS_SPKLEN) > > IC_HS_SPKLEN 0x40 (I2C block) 0xa4 > > IC_DMA_RDLR 0x6c (I2C block) 0x90 > > IC_INTR_STAT 0x74 (I2C block) 0x2c > > IC_INTR_MASK 0x78 (I2C block) 0x30 > > IC_RAW_INTR_STAT 0x7c (I2C block) 0x34 > > IC_DAR4 0xb8 (I2C block) 0x108 (IC_SAR4) > > IC_SFTY_CTRL 0xc0 (I2C block) N/A > > IC_SMBUS_CTRL 0x04 (SMBus block) N/A > > IC_SMBUS_ARP_CTRL 0x08 (SMBus block) N/A > > IC_SMBUS_INTR_STAT 0x28 (SMBus block) 0xc8 > > IC_SMBUS_INTR_MASK 0x2c (SMBus block) 0xcc > > IC_SMBUS_INTR_RAW_STAT 0x30 (SMBus block) 0xd0 > > > > Register configuration - IC_CTRL (DWC_ADV_i2c) vs IC_CON (DW_apb_i2c) > > DWC_Adv_i2c DW_apb_i2c > > IC_OP_MODE bit[0] bit[0] > (MASTER_MODE) > > IC_SPEED bit[5:4] bit[2:1] (SPEED) > > IC_10BITADDR_TGT bit[8] bit[3] > (IC_10BITADDR_SLAVE) > > IC_10BITADDR_CTRLR bit[9] bit[4] > (IC_10BITADDR_MASTER) > > BUS_CLEAR_FEATURE_CTRL bit[14] bit[11] > > > > Register configuration - IC_INTR_CLR > > DWC_ADV_i2c DW_apb_i2c > > CLR_INTR bit[0] (write) IC_INTR_CLR > bit[0] (read) > > CLR_RX_UNDER bit[1] (write) IC_CLR_RX_UNDER > bit[0] (read) > > CLR_RX_OVER bit[2] (write) IC_CLR_RX_OVER > bit[0] (read) > > CLR_TX_OVER bit[3] (write) IC_CLR_TX_OVER > bit[0] (read) > > CLR_RD_REQ bit[4] (write) IC_CLR_RD_REQ > bit[0] (read) > > ...... > > > > The documents used are > > [1] DesignWare ® Cores Advanced I2C/SMBus Controller and Target Device > Databook, Product Code H464-0, I2C Bus Spec V7.0, > > [2] DesignWare ® DW_apb_i2c Databook, Version 2.03a, December 2020, I2C > Bus Spec V6.0 > > -- > With Best Regards, > Andy Shevchenko > ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2026-06-08 12:20 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-27 8:50 [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support lianfeng.ouyang 2026-05-27 8:50 ` [PATCH v2 1/3] dt-bindings: i2c: Add StarFive JHB100 I2C lianfeng.ouyang 2026-05-27 13:04 ` Krzysztof Kozlowski 2026-05-28 6:57 ` 回复: " Lianfeng Ouyang 2026-05-28 7:47 ` Krzysztof Kozlowski 2026-05-28 8:30 ` 回复: " Lianfeng Ouyang 2026-05-27 8:50 ` [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver lianfeng.ouyang 2026-05-27 13:07 ` Krzysztof Kozlowski 2026-05-28 2:36 ` 回复: " Lianfeng Ouyang 2026-05-28 7:46 ` Krzysztof Kozlowski 2026-05-28 8:28 ` 回复: " Lianfeng Ouyang 2026-06-03 14:34 ` Krzysztof Kozlowski 2026-06-04 2:11 ` 回复: " Lianfeng Ouyang 2026-06-02 22:24 ` Andy Shevchenko 2026-05-27 8:50 ` [PATCH v2 3/3] i2c: starfive: Add StarFive JHB100 I2C master/slave support lianfeng.ouyang 2026-05-27 12:36 ` [PATCH v2 0/3] i2c: Add Starfive " Mika Westerberg 2026-05-28 2:41 ` 回复: " Lianfeng Ouyang 2026-06-02 22:26 ` Andy Shevchenko 2026-06-03 5:31 ` 回复: " Lianfeng Ouyang 2026-06-03 5:48 ` Andy Shevchenko 2026-06-03 6:09 ` 回复: " Lianfeng Ouyang 2026-06-03 6:39 ` Andy Shevchenko 2026-06-04 3:00 ` 回复: " Lianfeng Ouyang 2026-06-04 18:09 ` Andy Shevchenko 2026-06-08 9:48 ` 回复: " Lianfeng Ouyang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox