Re: [PATCH v4 01/13] hw/i2c: Add designware i2c controller

[Nicholas Piggin <npiggin@gmail.com>]

On Tue, May 05, 2026 at 09:36:57AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/5/26 08:20, Nicholas Piggin wrote:
> > On Thu, Apr 30, 2026 at 01:53:21PM +1000, Alistair Francis wrote:
> > > On Sat, Apr 25, 2026 at 11:20 PM Joel Stanley <joel@jms.id.au> wrote:
> > > > 
> > > > From: Chris Rauer <crauer@google.com>
> > > > 
> > > > In the past this model has been submitted for use with the arm virt
> > > > machine, however in this case it will be used by the upcoming
> > > > Tenstorrent Atlantis RISC-V machine.
> > > > 
> > > > This is a re-submission of the model with Chris' permission, with a
> > > > light touch of updates to make it build with qemu master.
> > > > 
> > > > Reviewed-by: Hao Wu <wuhaotsh@google.com>
> > > > Signed-off-by: Chris Rauer <crauer@google.com>
> > > > Link: https://lore.kernel.org/qemu-devel/20220110214755.810343-2-venture@google.com
> > > > [jms: rebase and minor build fixes for class_init and reset callback]
> > > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > > ---
> 
> 
> > > > +#define DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS   BIT(7)
> > > > +#define DW_IC_COMP_PARAM_1_HAS_DMA              0 /* bit 6 - DMA disabled. */
> > > > +#define DW_IC_COMP_PARAM_1_INTR_IO              BIT(5)
> > > > +#define DW_IC_COMP_PARAM_1_HC_COUNT_VAL         0 /* bit 4 - disabled */
> > > > +#define DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE      (BIT(2) | BIT(3))
> > > > +#define DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32    BIT(1) /* bits 0, 1 */
> > > > +#define DW_IC_COMP_PARAM_1_INIT_VAL             \
> > > > +    (DW_IC_COMP_PARAM_1_APB_DATA_WIDTH_32 | \
> > > > +    DW_IC_COMP_PARAM_1_HIGH_SPEED_MODE | \
> > > > +    DW_IC_COMP_PARAM_1_HC_COUNT_VAL | \
> > > > +    DW_IC_COMP_PARAM_1_INTR_IO | \
> > > > +    DW_IC_COMP_PARAM_1_HAS_DMA | \
> > > > +    DW_IC_COMP_PARAM_1_HAS_ENCODED_PARAMS | \
> > > > +    ((DESIGNWARE_I2C_RX_FIFO_SIZE - 1) << 8) | \
> > > > +    ((DESIGNWARE_I2C_TX_FIFO_SIZE - 1) << 16))
> > > > +#define DW_IC_COMP_VERSION_INIT_VAL             0x3132302a
> > > > +#define DW_IC_COMP_TYPE_INIT_VAL                0x44570140
> > > 
> > > I would prefer these to sue the QEMU register API
> > 
> > That's probably a good idea. It's a lot of churn to change it, but
> > I'm having a try. Might do it as a follow on patch.
> 
> Good suggestion but please as a follow up, this patch is already
> big enough, is reviewed and tested.

I agree... however I implemented it and it is a large amount of chrun
which is also not easy to review. Also it breaks migration
compatibility. So I don't want to get this patch bogged down but if we
agree to take the register API conversion then maybe we should squash
before upstreaming?

Not sure. I will post the patch as a separate optional addition to the
series to begin with.

Thanks,
Nick