| Message ID | 20240119054547.983693-1-dylan_hung@aspeedtech.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i3c: dw: Disable IBI IRQ depends on hot-join and SIR enabling | expand |
On Fri, 19 Jan 2024 13:45:47 +0800, Dylan Hung wrote: > Disable IBI IRQ signal and status only when hot-join and SIR enabling of > all target devices attached to the bus are disabled. > > Fixes: e389b1d72a62 ("i3c: dw: Add support for in-band interrupts") > > Applied, thanks! [1/1] i3c: dw: Disable IBI IRQ depends on hot-join and SIR enabling https://git.kernel.org/abelloni/c/10201396ef64 Best regards,
Hi Dylan, Just a question on a prior patch you sent: > Disable IBI IRQ signal and status only when hot-join and SIR enabling > of all target devices attached to the bus are disabled. > > Fixes: e389b1d72a62 ("i3c: dw: Add support for in-band interrupts") [...] > --- a/drivers/i3c/master/dw-i3c-master.c > +++ b/drivers/i3c/master/dw-i3c-master.c > @@ -1163,8 +1163,10 @@ static void dw_i3c_master_set_sir_enabled(struct dw_i3c_master *master, > global = reg == 0xffffffff; > reg &= ~BIT(idx); > } else { > - global = reg == 0; > + bool hj_rejected = !!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_HOT_JOIN_NACK); > + > reg |= BIT(idx); > + global = (reg == 0xffffffff) && hj_rejected; > } > writel(reg, master->regs + IBI_SIR_REQ_REJECT); > My interpretation of this change is that we keep the "global" IBI irq enabled if hot-join-nack is set (ie, always, because we don't support hot join, and configure the hardware to nack all hot join requests). However, we never enable the hot-join NACK interrupt - IBI_QUEUE_CTRL bit 0 is never set. So I can't see how we would ever get an interrupt for the hot join NACK case anyway. Because of that, this patch is just keeping the IBI threshold interrupt always enabled for no reason. Or is something else happening here? Is there another cause for the IBI threshold IRQs? Cheers, Jeremy
Hi Jeremy, > -----Original Message----- > From: Jeremy Kerr <jk@codeconstruct.com.au> > Sent: Wednesday, May 1, 2024 2:22 PM > To: Dylan Hung <dylan_hung@aspeedtech.com>; > alexandre.belloni@bootlin.com; joel@jms.id.au; u.kleine- > koenig@pengutronix.de; gustavoars@kernel.org; > krzysztof.kozlowski@linaro.org; zenghuchen@google.com; > matt@codeconstruct.com.au; linux-i3c@lists.infradead.org; linux- > kernel@vger.kernel.org > Cc: BMC-SW <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH] i3c: dw: Disable IBI IRQ depends on hot-join and SIR > enabling > > Hi Dylan, > > Just a question on a prior patch you sent: > > > Disable IBI IRQ signal and status only when hot-join and SIR enabling > > of all target devices attached to the bus are disabled. > > > > Fixes: e389b1d72a62 ("i3c: dw: Add support for in-band interrupts") > > [...] > > > --- a/drivers/i3c/master/dw-i3c-master.c > > +++ b/drivers/i3c/master/dw-i3c-master.c > > @@ -1163,8 +1163,10 @@ static void > > dw_i3c_master_set_sir_enabled(struct dw_i3c_master *master, > > global = reg == 0xffffffff; > > reg &= ~BIT(idx); > > } else { > > - global = reg == 0; > > + bool hj_rejected = !!(readl(master->regs + > > +DEVICE_CTRL) & DEV_CTRL_HOT_JOIN_NACK); > > + > > reg |= BIT(idx); > > + global = (reg == 0xffffffff) && hj_rejected; > > } > > writel(reg, master->regs + IBI_SIR_REQ_REJECT); > > > ``` if (global) dw_i3c_master_enable_sir_signal(master, enable); ``` > My interpretation of this change is that we keep the "global" IBI irq enabled if > hot-join-nack is set (ie, always, because we don't support hot join, and > configure the hardware to nack all hot join requests). > I would like to clarify the control logic, incorporating the principle of disabling the SIR interrupt signal: Case 1: When `DEV_CTRL_HOT_JOIN_NACK` is set, indicating `hj_rejected` is true, it signifies the controller's non-receptiveness to the hot-join event. Consequently, we can safely disable the SIR interrupt signal if none of the target devices request SIR (reg == 0xffffffff). Case 2: When `DEV_CTRL_HOT_JOIN_NACK` is unset, indicating `hj_rejected` is false, this indicates the controller's readiness to engage with the hot-join event. Therefore, it's imperative to keep the SIR interrupt signal enabled, even if not all target devices request SIR. In this case, `global` is false and `enable` is false. Here's a summary table to illustrate the logic: | enable | reg==0xffffffff? | hj_rejected? | global | result | |--- |--- |--- |--- |--- | | false | true | true | true | disable SIR interrupt | | false | false | true | false | keep SIR interrupt | | false | true | false | false | keep SIR interrupt | | false | false | false | false | keep SIR interrupt | > However, we never enable the hot-join NACK interrupt - IBI_QUEUE_CTRL bit > 0 is never set. So I can't see how we would ever get an interrupt for the hot > join NACK case anyway. Because of that, this patch is just keeping the IBI > threshold interrupt always enabled for no reason. > Billy recently submitted a change to implement the hot-join enabling/disabling. Therefore, it is timely to consider the hot-join functionality. https://patchwork.kernel.org/project/linux-i3c/patch/20240429073624.256830-1-billy_tsai@aspeedtech.com/ > Or is something else happening here? Is there another cause for the IBI > threshold IRQs? > > Cheers, > > > Jeremy
Hi Dylan, Thanks for the response! I have a couple of follow-up things though: > > My interpretation of this change is that we keep the "global" IBI irq enabled if > > hot-join-nack is set (ie, always, because we don't support hot join, and > > configure the hardware to nack all hot join requests). > > > I would like to clarify the control logic, incorporating the principle > of disabling the SIR interrupt signal: > > Case 1: > When `DEV_CTRL_HOT_JOIN_NACK` is set, indicating `hj_rejected` is > true, it signifies the controller's non-receptiveness to the hot-join > event. Consequently, we can safely disable the SIR interrupt signal if > none of the target devices request SIR (reg == 0xffffffff). > > Case 2: > When `DEV_CTRL_HOT_JOIN_NACK` is unset, indicating `hj_rejected` is > false, this indicates the controller's readiness to engage with the > hot-join event. Therefore, it's imperative to keep the SIR interrupt > signal enabled, even if not all target devices request SIR. In this > case, `global` is false and `enable` is false. Yep, I see what you're doing there, but it looks like the correct state would never be set if we're not enabling/disabling the IBIs separately; with this code, we would only ever enable the SIR for the HJ if we *also* happen to enable IBIs. The initial state would be to have all SIRs masked. > Billy recently submitted a change to implement the hot-join enabling/disabling. Therefore, it is timely to consider the hot-join functionality. > https://patchwork.kernel.org/project/linux-i3c/patch/20240429073624.256830-1-billy_tsai@aspeedtech.com/ Yep, I saw that, excellent! It's next on my list to take a look at. It's just a little unusual that we're enabling the HJ interrupt before actually having the HJ support though. Cheers, Jeremy
Hi Jeremy, > -----Original Message----- > From: Jeremy Kerr <jk@codeconstruct.com.au> > Sent: Monday, May 6, 2024 1:10 PM > To: Dylan Hung <dylan_hung@aspeedtech.com>; > alexandre.belloni@bootlin.com; joel@jms.id.au; u.kleine- > koenig@pengutronix.de; gustavoars@kernel.org; > krzysztof.kozlowski@linaro.org; zenghuchen@google.com; > matt@codeconstruct.com.au; linux-i3c@lists.infradead.org; linux- > kernel@vger.kernel.org > Cc: BMC-SW <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH] i3c: dw: Disable IBI IRQ depends on hot-join and SIR > enabling > > Hi Dylan, > > Thanks for the response! I have a couple of follow-up things though: > > > > My interpretation of this change is that we keep the "global" IBI > > > irq enabled if hot-join-nack is set (ie, always, because we don't > > > support hot join, and configure the hardware to nack all hot join > requests). > > > > > I would like to clarify the control logic, incorporating the principle > > of disabling the SIR interrupt signal: > > > > Case 1: > > When `DEV_CTRL_HOT_JOIN_NACK` is set, indicating `hj_rejected` is > > true, it signifies the controller's non-receptiveness to the hot-join > > event. Consequently, we can safely disable the SIR interrupt signal if > > none of the target devices request SIR (reg == 0xffffffff). > > > > Case 2: > > When `DEV_CTRL_HOT_JOIN_NACK` is unset, indicating `hj_rejected` is > > false, this indicates the controller's readiness to engage with the > > hot-join event. Therefore, it's imperative to keep the SIR interrupt > > signal enabled, even if not all target devices request SIR. In this > > case, `global` is false and `enable` is false. > > Yep, I see what you're doing there, but it looks like the correct state would > never be set if we're not enabling/disabling the IBIs separately; with this code, > we would only ever enable the SIR for the HJ if we > *also* happen to enable IBIs. When we want to enable the SIR, the current code doesn't check whether the hot-join is enable or not. The modification I made pertains to disabling the SIR interrupt. It doesn't alter the logic for enabling the SIR. ``` reg = readl(master->regs + IBI_SIR_REQ_REJECT); if (enable) { global = reg == 0xffffffff; reg &= ~BIT(idx); } ``` > > The initial state would be to have all SIRs masked. > Yes, indeed. The "global" variable is also true because "reg == 0xffffffff" is true. Therefore, the INTR_IBI_THLD_STAT bit will be set in the following code. > > Billy recently submitted a change to implement the hot-join > enabling/disabling. Therefore, it is timely to consider the hot-join > functionality. > > https://patchwork.kernel.org/project/linux-i3c/patch/20240429073624.25 > > 6830-1-billy_tsai@aspeedtech.com/ > > Yep, I saw that, excellent! It's next on my list to take a look at. > > It's just a little unusual that we're enabling the HJ interrupt before actually > having the HJ support though. > > Cheers, > > > Jeremy
Hi Dylan, > > The initial state would be to have all SIRs masked. > > > > Yes, indeed. The "global" variable is also true because "reg == > 0xffffffff" is true. > Therefore, the INTR_IBI_THLD_STAT bit will be set in the following > code. That's mainly my point - none of this code is ever run unless the ->enable_ibi or ->disable_ibi controller callback is invoked. So we'll end up with the HJ interrupt only being enabled if some i3c device driver enables IBIs, which is a bit of a weird side-effect. It probably makes more sense when the rest of the HJ code is added, but not so much as a standalone patch. Cheers, Jeremy
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c index ef5751e91cc9..276153e10f5a 100644 --- a/drivers/i3c/master/dw-i3c-master.c +++ b/drivers/i3c/master/dw-i3c-master.c @@ -1163,8 +1163,10 @@ static void dw_i3c_master_set_sir_enabled(struct dw_i3c_master *master, global = reg == 0xffffffff; reg &= ~BIT(idx); } else { - global = reg == 0; + bool hj_rejected = !!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_HOT_JOIN_NACK); + reg |= BIT(idx); + global = (reg == 0xffffffff) && hj_rejected; } writel(reg, master->regs + IBI_SIR_REQ_REJECT);
Disable IBI IRQ signal and status only when hot-join and SIR enabling of all target devices attached to the bus are disabled. Fixes: e389b1d72a62 ("i3c: dw: Add support for in-band interrupts") Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com> --- drivers/i3c/master/dw-i3c-master.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)