[PATCH] i3c: dw: Fix IBI intr signal programming

[PATCH] i3c: dw: Fix IBI intr signal programming

[Aniket]

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(-)

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


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: dw: Fix IBI intr signal programming

[Jeremy Kerr]

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
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: dw: Fix IBI intr signal programming

[Aniket .]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: dw: Fix IBI intr signal programming

[Jeremy Kerr]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: dw: Fix IBI intr signal programming

[Aniket .]

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v2] i3c: dw: Fix IBI intr programming

[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 <aniketmaurya@google.com>
---
 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, &reg);
 	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


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v2] i3c: dw: Fix IBI intr programming

[Jeremy Kerr]

Hi 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.

Looks good, thanks!

Reviewed-by: Jeremy Kerr <jk@codeconstruct.com.au>

Cheers,


Jeremy

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v2] i3c: dw: Fix IBI intr programming

[Alexandre Belloni]

On Fri, 07 Jun 2024 07:20:30 +0000, Aniket wrote:
> 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.
> 
> 
> [...]

Applied, thanks!

[1/1] i3c: dw: Fix IBI intr programming
      https://git.kernel.org/abelloni/c/76fb85b86b40

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c