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 <[email protected]>
---
drivers/i3c/master/dw-i3c-master.c | 4 ++--
drivers/i3c/master/dw-i3c-master.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
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)
--
2.45.1.467.gbab1589fc0-goog
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
IBI_SIR_REQ_REJECT register is not present if the IP has
IC_HAS_IBI_DATA = 1 set. So don't rely on doing read-
modify-write op on this register.
Instead maintain a variable to store the sir reject mask
and use it to set IBI_SIR_REQ_REJECT.
Signed-off-by: Aniket <[email protected]>
---
changed from v1 to v2
- replaced counter with the mask
drivers/i3c/master/dw-i3c-master.c | 15 ++++++++-------
drivers/i3c/master/dw-i3c-master.h | 2 +-
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 0ec00e644bd4..03911a5c0264 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -658,7 +658,9 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
if (ret)
return ret;
- writel(IBI_REQ_REJECT_ALL, master->regs + IBI_SIR_REQ_REJECT);
+ master->sir_rej_mask = IBI_REQ_REJECT_ALL;
+ writel(master->sir_rej_mask, master->regs + IBI_SIR_REQ_REJECT);
+
writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
/* For now don't support Hot-Join */
@@ -1175,17 +1177,16 @@ static void dw_i3c_master_set_sir_enabled(struct dw_i3c_master *master,
master->platform_ops->set_dat_ibi(master, dev, enable, ®);
writel(reg, master->regs + dat_entry);
- reg = readl(master->regs + IBI_SIR_REQ_REJECT);
if (enable) {
- global = reg == 0xffffffff;
- reg &= ~BIT(idx);
+ global = (master->sir_rej_mask == IBI_REQ_REJECT_ALL);
+ master->sir_rej_mask &= ~BIT(idx);
} else {
bool hj_rejected = !!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_HOT_JOIN_NACK);
- reg |= BIT(idx);
- global = (reg == 0xffffffff) && hj_rejected;
+ master->sir_rej_mask |= BIT(idx);
+ global = (master->sir_rej_mask == IBI_REQ_REJECT_ALL) && hj_rejected;
}
- writel(reg, master->regs + IBI_SIR_REQ_REJECT);
+ writel(master->sir_rej_mask, master->regs + IBI_SIR_REQ_REJECT);
if (global)
dw_i3c_master_enable_sir_signal(master, enable);
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index 4ab94aa72252..8cb617b8147e 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;
-
+ u32 sir_rej_mask;
/*
* Per-device hardware data, used to manage the device address table
* (DAT)
--
2.45.2.505.gda0bf45e8d-goog