RE: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
> -Original Message- > From: Tao Ren [mailto:rentao.b...@gmail.com] > Sent: Wednesday, August 26, 2020 4:05 AM > To: Eddie James > Cc: Joel Stanley ; devicetree ; > linux-aspeed ; dmitry.torok...@gmail.com; > Brendan Higgins ; Linux Kernel Mailing List > ; Rob Herring ; > linux-...@vger.kernel.org; linux-in...@vger.kernel.org; Ryan Chen > > Subject: Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits > > On Tue, Aug 25, 2020 at 02:47:51PM -0500, Eddie James wrote: > > > > On 8/25/20 1:38 AM, Joel Stanley wrote: > > > On Thu, 20 Aug 2020 at 16:12, Eddie James > wrote: > > > > Mask the IRQ status to only the bits that the driver checks. This > > > > prevents excessive driver warnings when operating in slave mode > > > > when additional bits are set that the driver doesn't handle. > > > > > > > > Signed-off-by: Eddie James > > > > --- > > > > drivers/i2c/busses/i2c-aspeed.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c > > > > b/drivers/i2c/busses/i2c-aspeed.c index 31268074c422..abf40f2af8b4 > > > > 100644 > > > > --- a/drivers/i2c/busses/i2c-aspeed.c > > > > +++ b/drivers/i2c/busses/i2c-aspeed.c > > > > @@ -604,6 +604,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, > void *dev_id) > > > > writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, > > > > bus->base + ASPEED_I2C_INTR_STS_REG); > > > > readl(bus->base + ASPEED_I2C_INTR_STS_REG); > > > > + irq_received &= 0xf000; > > > > irq_remaining = irq_received; > > > This would defeat the check for irq_remaining. I don't think we want to do > this. > > > > > > Can you explain why these bits are being set in slave mode? > > > > > > No, I don't have any documentation for the bits that are masked off > > here, so I don't know why they would get set. > > > > The check for irq_remaining is still useful for detecting that the > > driver state machine might be out of sync with what the master is doing. > > I have a similar patch in my local tree, and the reason being: AST2600 I2C > Controller may set I2CD10[25:24] to report Current Slave Parking Status > (defined in new register I2CS24) even though the new register mode is off. The > 2 bits can be ignored in legacy mode, and Ryan from ASPEED could confirm it. Yes, in AST2600 i2cd10[25:24] will be the same with new mode register i2cs24[25:24] Thanks Tao. > > > Cheers, > > Tao
Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
On Tue, Aug 25, 2020 at 02:47:51PM -0500, Eddie James wrote: > > On 8/25/20 1:38 AM, Joel Stanley wrote: > > On Thu, 20 Aug 2020 at 16:12, Eddie James wrote: > > > Mask the IRQ status to only the bits that the driver checks. This > > > prevents excessive driver warnings when operating in slave mode > > > when additional bits are set that the driver doesn't handle. > > > > > > Signed-off-by: Eddie James > > > --- > > > drivers/i2c/busses/i2c-aspeed.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c > > > b/drivers/i2c/busses/i2c-aspeed.c > > > index 31268074c422..abf40f2af8b4 100644 > > > --- a/drivers/i2c/busses/i2c-aspeed.c > > > +++ b/drivers/i2c/busses/i2c-aspeed.c > > > @@ -604,6 +604,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > > > *dev_id) > > > writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, > > > bus->base + ASPEED_I2C_INTR_STS_REG); > > > readl(bus->base + ASPEED_I2C_INTR_STS_REG); > > > + irq_received &= 0xf000; > > > irq_remaining = irq_received; > > This would defeat the check for irq_remaining. I don't think we want to do > > this. > > > > Can you explain why these bits are being set in slave mode? > > > No, I don't have any documentation for the bits that are masked off here, so > I don't know why they would get set. > > The check for irq_remaining is still useful for detecting that the driver > state machine might be out of sync with what the master is doing. I have a similar patch in my local tree, and the reason being: AST2600 I2C Controller may set I2CD10[25:24] to report Current Slave Parking Status (defined in new register I2CS24) even though the new register mode is off. The 2 bits can be ignored in legacy mode, and Ryan from ASPEED could confirm it. Cheers, Tao
Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
On 8/25/20 1:38 AM, Joel Stanley wrote: On Thu, 20 Aug 2020 at 16:12, Eddie James wrote: Mask the IRQ status to only the bits that the driver checks. This prevents excessive driver warnings when operating in slave mode when additional bits are set that the driver doesn't handle. Signed-off-by: Eddie James --- drivers/i2c/busses/i2c-aspeed.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 31268074c422..abf40f2af8b4 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -604,6 +604,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id) writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, bus->base + ASPEED_I2C_INTR_STS_REG); readl(bus->base + ASPEED_I2C_INTR_STS_REG); + irq_received &= 0xf000; irq_remaining = irq_received; This would defeat the check for irq_remaining. I don't think we want to do this. Can you explain why these bits are being set in slave mode? No, I don't have any documentation for the bits that are masked off here, so I don't know why they would get set. The check for irq_remaining is still useful for detecting that the driver state machine might be out of sync with what the master is doing. Thanks, Eddie
Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
On Thu, 20 Aug 2020 at 16:12, Eddie James wrote: > > Mask the IRQ status to only the bits that the driver checks. This > prevents excessive driver warnings when operating in slave mode > when additional bits are set that the driver doesn't handle. > > Signed-off-by: Eddie James > --- > drivers/i2c/busses/i2c-aspeed.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index 31268074c422..abf40f2af8b4 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -604,6 +604,7 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void > *dev_id) > writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE, >bus->base + ASPEED_I2C_INTR_STS_REG); > readl(bus->base + ASPEED_I2C_INTR_STS_REG); > + irq_received &= 0xf000; > irq_remaining = irq_received; This would defeat the check for irq_remaining. I don't think we want to do this. Can you explain why these bits are being set in slave mode?
Re: [PATCH 3/5] i2c: aspeed: Mask IRQ status to relevant bits
> + irq_received &= 0xf000; Can we have a define for this? Like ASPEED_I2CD_INTR_MASTER_IRQS or something? signature.asc Description: PGP signature
