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

[Lianfeng Ouyang]

> -----邮件原件-----
> 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 发送时间: 2026年6月5日 2:09
> 收件人: 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 Thu, Jun 04, 2026 at 03:00:13AM +0000, Lianfeng Ouyang wrote:
> > > 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > 发送时间: 2026年6月3日 14:40
> > > 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")
> >
> > 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
>