| Message ID | 20240606124816.723630-1-aniketmaurya@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: dw: Fix IBI intr signal programming | expand |
Hi Aniket, > IBI_SIR_REQ_REJECT register is not present if the IP > has IC_HAS_IBI_DATA = 1 set. I don't have any access to the IP itself, but I understand there are a few different configuration settings in the IP that may affect the register interface. I think we're OK in this case (just not reading the value out of the SIR_REQ_REJECT register), but any thoughts on adding corresponding switches in the driver so we can support those configurations? These would be represented as DT config of the specific hardware instance - at the most granular, just by the specific compatible string. > dw_i3c_master_set_sir_enabled(struct dw_i3c_master *master, > > reg = readl(master->regs + IBI_SIR_REQ_REJECT); > if (enable) { > - global = reg == 0xffffffff; > + global = !master->sir_en_cnt++; > reg &= ~BIT(idx); > } else { > bool hj_rejected = !!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_HOT_JOIN_NACK); > > reg |= BIT(idx); > - global = (reg == 0xffffffff) && hj_rejected; > + global = (!--master->sir_en_cnt) && hj_rejected; > } Could we use the SIR mask for this, but just read it from a field in the struct dw_i3c_master, instead of IBI_SIR_REQ_REJECT? This would mean that there's no possibility of the counter going out of sync from the SIR settings - say, on underflow if we get a spurious disable. Cheers, Jeremy
Hi Jeremy, > I think we're OK in this case (just not reading the value out of the > SIR_REQ_REJECT register), but any thoughts on adding corresponding > switches in the driver so we can support those configurations? These > would be represented as DT config of the specific hardware instance - at > the most granular, just by the specific compatible string. We can go with some DT quirk, but I don't see the strong need to do this here. I guess optional registers like IBI_SIR_REQ_REJECT, IBI_MR_REQ_REJECT should not be relied upon to set other registers. > Could we use the SIR mask for this, but just read it from a field in the > struct dw_i3c_master, instead of IBI_SIR_REQ_REJECT? > > This would mean that there's no possibility of the counter going out of > sync from the SIR settings - say, on underflow if we get a spurious > disable. Yes, we can keep a SW SIR mask instead of a counter. It would replace all the places where we read IBI_SIR_REQ_REJECT. Both methods are okay, but if you think the mask might come in handy in some situations rather than just the count, we can go with that. Let me know your thoughts on this. Thanks, Aniket
Hi Aniket, > > I think we're OK in this case (just not reading the value out of the > > SIR_REQ_REJECT register), but any thoughts on adding corresponding > > switches in the driver so we can support those configurations? These > > would be represented as DT config of the specific hardware instance - at > > the most granular, just by the specific compatible string. > > We can go with some DT quirk, but I don't see the strong need to do this > here. Oh definitely - the behaviour here doesn't need any special handling that would warrant a quirk/etc. This is more for handling IP configuration options we may see in future. For example, I believe support for target/secondary mode is entirely optional too. > > Could we use the SIR mask for this, but just read it from a field in the > > struct dw_i3c_master, instead of IBI_SIR_REQ_REJECT? > > > > This would mean that there's no possibility of the counter going out of > > sync from the SIR settings - say, on underflow if we get a spurious > > disable. > > Yes, we can keep a SW SIR mask instead of a counter. It would replace > all the places where we read IBI_SIR_REQ_REJECT. > Both methods are okay, but if you think the mask might come in handy in > some situations rather than just the count, we can go with that. > Let me know your thoughts on this. I think keeping the mask value locally would be best. this means we 1) cannot get the counter and mask out of sync; and 2) don't need to do a read-modify-write on a register that is only updated by the driver. Cheers, Jeremy
Hi Jeremy, > This is more for handling IP configuration options we may see in future. > For example, I believe support for target/secondary mode is entirely > optional too. I was hoping to add support for target/secondary master, we might have different drivers instead, something like what's done for i2c-dw drivers. But that's a thought for another day. > I think keeping the mask value locally would be best. this means we > > 1) cannot get the counter and mask out of sync; and > 2) don't need to do a read-modify-write on a register that is only > updated by the driver. Sure, let me make the patch. Thanks, Aniket
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c index 0ec00e644bd4..08f3ef000206 100644 --- a/drivers/i3c/master/dw-i3c-master.c +++ b/drivers/i3c/master/dw-i3c-master.c @@ -1177,13 +1177,13 @@ static void dw_i3c_master_set_sir_enabled(struct dw_i3c_master *master, reg = readl(master->regs + IBI_SIR_REQ_REJECT); if (enable) { - global = reg == 0xffffffff; + global = !master->sir_en_cnt++; reg &= ~BIT(idx); } else { bool hj_rejected = !!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_HOT_JOIN_NACK); reg |= BIT(idx); - global = (reg == 0xffffffff) && hj_rejected; + global = (!--master->sir_en_cnt) && hj_rejected; } writel(reg, master->regs + IBI_SIR_REQ_REJECT); diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h index 4ab94aa72252..44a5f045f007 100644 --- a/drivers/i3c/master/dw-i3c-master.h +++ b/drivers/i3c/master/dw-i3c-master.h @@ -39,7 +39,7 @@ struct dw_i3c_master { char version[5]; char type[5]; bool ibi_capable; - + u8 sir_en_cnt; /* * Per-device hardware data, used to manage the device address table * (DAT)
IBI_SIR_REQ_REJECT register is not present if the IP has IC_HAS_IBI_DATA = 1 set. So don't rely on this register to program IBI intr signals. Instead use a simple counter to do the job. The counter is protected by spinlock. Signed-off-by: Aniket <aniketmaurya@google.com> --- drivers/i3c/master/dw-i3c-master.c | 4 ++-- drivers/i3c/master/dw-i3c-master.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)