Re: [RFC PATCH v2 0/6] hw/i2c: i2c slave mode support

From: Klaus Jensen <its@irrelevant.dk>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>,
	qemu-devel@nongnu.org,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	qemu-arm@nongnu.org, Peter Delevoryas <pdel@fb.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	Corey Minyard <cminyard@mvista.com>,
	Padmakar Kalghatgi <p.kalghatgi@samsung.com>,
	Damien Hedde <damien.hedde@greensocs.com>,
	Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>,
	Arun Kumar Kashinath Agasar <arun.kka@samsung.com>,
	Klaus Jensen <k.jensen@samsung.com>,
	Zev Weiss <zev@bewilderbeest.net>
Subject: Re: [RFC PATCH v2 0/6] hw/i2c: i2c slave mode support
Date: Thu, 2 Jun 2022 21:19:57 +0200	[thread overview]
Message-ID: <YpkNXUkafYQ3ZSpT@apples> (raw)
In-Reply-To: <c796202d-2e57-32b3-3007-343d37dcdb84@kaod.org>

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

On Jun  2 17:40, Cédric Le Goater wrote:
> On 6/2/22 16:29, Jae Hyun Yoo wrote:
> > Hi Klaus,
> > 
> > On 6/2/2022 6:50 AM, Cédric Le Goater wrote:
> > > On 6/2/22 10:21, Klaus Jensen wrote:
> > > > 
> > > > There is an outstanding issue with the SLAVE_ADDR_RX_MATCH interrupt bit
> > > > (bit 7). Remember from my first series I had a workaround to make sure
> > > > it wasnt masked.
> > > > 
> > > > I posted this upstream to linux
> > > > 
> > > > https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
> > > > 
> > > > Not sure if that is the right way to fix it.
> > > 
> > > That's weird. I would have thought it was already enabled [ Adding Jae ]
> > 
> > Slave mode support in Aspeed I2C driver is already enabled and it has
> > worked well so far. The fix Klaus made in the link is incorrect.
> > 
> > https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
> > 
> > The patch is adding ASPEED_I2CD_INTR_SLAVE_MATCH as a mask bit for
> > I2CD0C (Interrupt Control Register) but actually this bit is part of
> > I2CD10 (Interrupt Status Register). Means that the slave match interrupt
> > can be enabled without enabling any mask bit in I2CD0C.
> 
> Thanks Jae.
> 
> So we should enable this interrupt always independently of the
> Interrupt Control Register value.
> 
> I would simply extend the mask value (bus->regs[intr_ctrl_reg])
> with the SLAVE_ADDR_RX_MATCH bit when interrupts are raised in
> aspeed_i2c_bus_raise_interrupt().
> 

Alright, so my "workaround" from v1 was actually the right fix - I'll
re-add it ;)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]