回复: 回复: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support

[Lianfeng Ouyang]

> -----邮件原件-----
> 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 发送时间: 2026年6月3日 14:40
> 收件人: Lianfeng Ouyang <lianfeng.ouyang@xxxxxxxxxxxxxxxx>
> 抄送: Andi Shyti <andi.shyti@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>;
> linux-i2c@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 主题: 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@xxxxxxxxxxxxxxx>
> > > 发送时间: 2026年6月3日 13:49
> > > On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote:
> > > > > -----邮件原件-----
> > > > > 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > > > 发送时间: 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