[RESEND PATCH v1 0/9] A bunch of fix and optimization patches in spmi-pmic-arb.c

[RESEND PATCH v1 0/9] A bunch of fix and optimization patches in spmi-pmic-arb.c

[Fenglin Wu]

Just want to resend the series of the changes again and see if anyone can
help to review them and give any comments. Thanks!

This change series includes some fixes and optimizations in spmi-pmic-arb.c.
Please see change detail and description in each of the patch. Thanks!

Abhijeet Dharmapurikar (1):
  spmi: pmic-arb: add a print in cleanup_irq

Ashay Jaiswal (1):
  spmi: pmic-arb: add support to dispatch interrupt based on IRQ status

David Collins (5):
  spmi: pmic-arb: check apid against limits before calling irq handler
  spmi: pmic-arb: correct duplicate APID to PPID mapping logic
  spmi: pmic-arb: block access for invalid PMIC arbiter v5 SPMI writes
  spmi: pmic-arb: make interrupt support optional
  spmi: pmic-arb: increase SPMI transaction timeout delay

Subbaraman Narayanamurthy (1):
  spmi: pmic-arb: do not ack and clear peripheral interrupts in
    cleanup_irq

Yimin Peng (1):
  spmi: pmic-arb: support updating interrupt type flags

 drivers/spmi/spmi-pmic-arb.c | 127 +++++++++++++++++++++++++++++++------------
 1 file changed, 91 insertions(+), 36 deletions(-)

-- 
2.7.4

[RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Fenglin Wu]

From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>

The cleanup_irq() was meant to clear and mask interrupts that were
left enabled in the hardware but there was no interrupt handler
registered for it. Add an error print when it gets invoked.

Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index bbbd311..295e19f 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -489,6 +489,8 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
 	u8 per = ppid & 0xFF;
 	u8 irq_mask = BIT(id);
 
+	dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d sid=0x%x per=0x%x irq=%d\n",
+			__func__, apid, sid, per, id);
 	writel_relaxed(irq_mask, pmic_arb->ver_ops->irq_clear(pmic_arb, apid));
 
 	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
-- 
2.7.4

Re: [RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Stephen Boyd]

Quoting Fenglin Wu (2021-09-16 23:32:56)
> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> 
> The cleanup_irq() was meant to clear and mask interrupts that were
> left enabled in the hardware but there was no interrupt handler
> registered for it. Add an error print when it gets invoked.

Why? Don't we get the genirq spurious irq message in this scenario?

> 
> Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>

Re: [RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Fenglin Wu]

On 10/13/2021 1:46 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:32:56)
>> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>>
>> The cleanup_irq() was meant to clear and mask interrupts that were
>> left enabled in the hardware but there was no interrupt handler
>> registered for it. Add an error print when it gets invoked.
> Why? Don't we get the genirq spurious irq message in this scenario?

Thanks for reviewing the change.

No, there is no existing message printed out in this special case ( IRQ 
fired for not registered interrupt).

>> Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>

Re: [RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Stephen Boyd]

Quoting Fenglin Wu (2021-10-12 21:15:42)
> 
> On 10/13/2021 1:46 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-09-16 23:32:56)
> >> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> >>
> >> The cleanup_irq() was meant to clear and mask interrupts that were
> >> left enabled in the hardware but there was no interrupt handler
> >> registered for it. Add an error print when it gets invoked.
> > Why? Don't we get the genirq spurious irq message in this scenario?
> 
> Thanks for reviewing the change.
> 
> No, there is no existing message printed out in this special case ( IRQ 
> fired for not registered interrupt).

Ah I see so the irq doesn't have a flow handler? Shouldn't you call
handle_bad_irq() in this case so we get a irq descriptor print?

Re: [RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Fenglin Wu]

On 10/14/2021 3:35 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-12 21:15:42)
>> On 10/13/2021 1:46 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-09-16 23:32:56)
>>>> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>>>>
>>>> The cleanup_irq() was meant to clear and mask interrupts that were
>>>> left enabled in the hardware but there was no interrupt handler
>>>> registered for it. Add an error print when it gets invoked.
>>> Why? Don't we get the genirq spurious irq message in this scenario?
>> Thanks for reviewing the change.
>>
>> No, there is no existing message printed out in this special case ( IRQ
>> fired for not registered interrupt).
> Ah I see so the irq doesn't have a flow handler? Shouldn't you call
> handle_bad_irq() in this case so we get a irq descriptor print?
In such case, the irq number is not valid and there won't be a valid
irq_desc, hence it's not possible to call handle_bad_irq() here.

Re: [RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Stephen Boyd]

Quoting Fenglin Wu (2021-10-13 19:26:55)
> 
> On 10/14/2021 3:35 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-10-12 21:15:42)
> >> On 10/13/2021 1:46 AM, Stephen Boyd wrote:
> >>> Quoting Fenglin Wu (2021-09-16 23:32:56)
> >>>> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> >>>>
> >>>> The cleanup_irq() was meant to clear and mask interrupts that were
> >>>> left enabled in the hardware but there was no interrupt handler
> >>>> registered for it. Add an error print when it gets invoked.
> >>> Why? Don't we get the genirq spurious irq message in this scenario?
> >> Thanks for reviewing the change.
> >>
> >> No, there is no existing message printed out in this special case ( IRQ
> >> fired for not registered interrupt).
> > Ah I see so the irq doesn't have a flow handler? Shouldn't you call
> > handle_bad_irq() in this case so we get a irq descriptor print?
> In such case, the irq number is not valid and there won't be a valid
> irq_desc, hence it's not possible to call handle_bad_irq() here.

I mean handle_bad_irq() on the irqdesc for the spmi pmic arb chained
irq. Because things are not good with the chained irq.

Re: [RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Fenglin Wu]

On 10/15/2021 9:09 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-13 19:26:55)
>> On 10/14/2021 3:35 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-10-12 21:15:42)
>>>> On 10/13/2021 1:46 AM, Stephen Boyd wrote:
>>>>> Quoting Fenglin Wu (2021-09-16 23:32:56)
>>>>>> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>>>>>>
>>>>>> The cleanup_irq() was meant to clear and mask interrupts that were
>>>>>> left enabled in the hardware but there was no interrupt handler
>>>>>> registered for it. Add an error print when it gets invoked.
>>>>> Why? Don't we get the genirq spurious irq message in this scenario?
>>>> Thanks for reviewing the change.
>>>>
>>>> No, there is no existing message printed out in this special case ( IRQ
>>>> fired for not registered interrupt).
>>> Ah I see so the irq doesn't have a flow handler? Shouldn't you call
>>> handle_bad_irq() in this case so we get a irq descriptor print?
>> In such case, the irq number is not valid and there won't be a valid
>> irq_desc, hence it's not possible to call handle_bad_irq() here.
> I mean handle_bad_irq() on the irqdesc for the spmi pmic arb chained
> irq. Because things are not good with the chained irq.
Okay, how about this, Update periph_interrupt() function with a return
value, and return -EINVAL once an invalid IRQ is detected. In
pmic_arb_chained_irq(), call handle_bad_irq() if periph_interrupt()
returned -EINVAL.

Re: [RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Fenglin Wu]

On 10/15/2021 9:27 AM, Fenglin Wu wrote:
>
> On 10/15/2021 9:09 AM, Stephen Boyd wrote:
>> Quoting Fenglin Wu (2021-10-13 19:26:55)
>>> On 10/14/2021 3:35 AM, Stephen Boyd wrote:
>>>> Quoting Fenglin Wu (2021-10-12 21:15:42)
>>>>> On 10/13/2021 1:46 AM, Stephen Boyd wrote:
>>>>>> Quoting Fenglin Wu (2021-09-16 23:32:56)
>>>>>>> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>>>>>>>
>>>>>>> The cleanup_irq() was meant to clear and mask interrupts that were
>>>>>>> left enabled in the hardware but there was no interrupt handler
>>>>>>> registered for it. Add an error print when it gets invoked.
>>>>>> Why? Don't we get the genirq spurious irq message in this scenario?
>>>>> Thanks for reviewing the change.
>>>>>
>>>>> No, there is no existing message printed out in this special case 
>>>>> ( IRQ
>>>>> fired for not registered interrupt).
>>>> Ah I see so the irq doesn't have a flow handler? Shouldn't you call
>>>> handle_bad_irq() in this case so we get a irq descriptor print?
>>> In such case, the irq number is not valid and there won't be a valid
>>> irq_desc, hence it's not possible to call handle_bad_irq() here.
>> I mean handle_bad_irq() on the irqdesc for the spmi pmic arb chained
>> irq. Because things are not good with the chained irq.
> Okay, how about this, Update periph_interrupt() function with a return
> value, and return -EINVAL once an invalid IRQ is detected. In
> pmic_arb_chained_irq(), call handle_bad_irq() if periph_interrupt()
> returned -EINVAL.
Combined with your comments in "[PATCH v1 3/9] spmi: pmic-arb:check apid
againstlimits before calling irq handler",it seemslike that it can be
a independentpatch for handling spuriousinterrupt, something like this
in my mind:

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 295e19f..bd01ad4 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -504,10 +504,10 @@ static void cleanup_irq(struct spmi_pmic_arb 
*pmic_arb, u16 apid, int id)
                                 irq_mask, ppid);
  }

-static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
+static int periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
  {
         unsigned int irq;
-       u32 status, id;
+       u32 status, id, handled = 0;
         u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
         u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;

@@ -522,7 +522,10 @@ static void periph_interrupt(struct spmi_pmic_arb 
*pmic_arb, u16 apid)
                         continue;
                 }
                 generic_handle_irq(irq);
+               handled++;
         }
+
+       return (handled == 0) ? -EINVAL : 0;
  }

  static void pmic_arb_chained_irq(struct irq_desc *desc)
@@ -533,7 +536,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
         int first = pmic_arb->min_apid >> 5;
         int last = pmic_arb->max_apid >> 5;
         u8 ee = pmic_arb->ee;
-       u32 status, enable;
+       u32 status, enable, handled = 0;
         int i, id, apid;

         chained_irq_enter(chip, desc);
@@ -548,10 +551,14 @@ static void pmic_arb_chained_irq(struct irq_desc 
*desc)
                         enable = readl_relaxed(
ver_ops->acc_enable(pmic_arb, apid));
                         if (enable & SPMI_PIC_ACC_ENABLE_BIT)
- periph_interrupt(pmic_arb, apid);
+                               if (periph_interrupt(pmic_arb, apid) == 0)
+ handled++;
                 }
         }

+       if (handled == 0)
+               handle_bad_irq(desc);
+
         chained_irq_exit(chip, desc);
  }

Is this what you expected? The original patch is only for printing a 
debug message when any
sub-irq is detected as enabled but not registered, some other sub-IRQ 
maybe still valid and
be handled after that, which means the chained-irq may still be a good 
one.Should I keep
the original patch unchanged and submit a separate one to handle the 
spuriousinterrupt?

[RESEND PATCH v1 2/9] spmi: pmic-arb: do not ack and clear peripheral interrupts in cleanup_irq

[Fenglin Wu]

From: Subbaraman Narayanamurthy <subbaram@codeaurora.org>

Currently, cleanup_irq() is invoked when a peripheral's interrupt
fires and there is no mapping present in the interrupt domain of
spmi interrupt controller.

The cleanup_irq clears the arbiter bit, clears the pmic interrupt
and disables it at the pmic in that order. The last disable in
cleanup_irq races with request_irq() in that it stomps over the
enable issued by request_irq. Fix this by not writing to the pmic
in cleanup_irq. The latched bit will be left set in the pmic,
which will not send us more interrupts even if the enable bit
stays enabled.

When a client wants to request an interrupt, use the activate
callback on the irq_domain to clear latched bit. This ensures
that the latched, if set due to the above changes in cleanup_irq
or when the bootloader leaves it set, gets cleaned up, paving way
for upcoming interrupts to trigger.

With this, there is a possibility of unwanted triggering of
interrupt right after the latched bit is cleared - the interrupt
may be left enabled too. To avoid that, clear the enable first
followed by clearing the latched bit in the activate callback.

Signed-off-by: Subbaraman Narayanamurthy <subbaram@codeaurora.org>
[collinsd@codeaurora.org: fix merge conflict]
Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 295e19f..4d7ad004 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -492,16 +492,6 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
 	dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d sid=0x%x per=0x%x irq=%d\n",
 			__func__, apid, sid, per, id);
 	writel_relaxed(irq_mask, pmic_arb->ver_ops->irq_clear(pmic_arb, apid));
-
-	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
-			(per << 8) + QPNPINT_REG_LATCHED_CLR, &irq_mask, 1))
-		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed to ack irq_mask = 0x%x for ppid = %x\n",
-				irq_mask, ppid);
-
-	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
-			       (per << 8) + QPNPINT_REG_EN_CLR, &irq_mask, 1))
-		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed to ack irq_mask = 0x%x for ppid = %x\n",
-				irq_mask, ppid);
 }
 
 static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
@@ -667,6 +657,7 @@ static int qpnpint_irq_domain_activate(struct irq_domain *domain,
 	u16 apid = hwirq_to_apid(d->hwirq);
 	u16 sid = hwirq_to_sid(d->hwirq);
 	u16 irq = hwirq_to_irq(d->hwirq);
+	u8 buf;
 
 	if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
 		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u: ee=%u but owner=%u\n",
@@ -675,6 +666,10 @@ static int qpnpint_irq_domain_activate(struct irq_domain *domain,
 		return -ENODEV;
 	}
 
+	buf = BIT(irq);
+	qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &buf, 1);
+	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &buf, 1);
+
 	return 0;
 }
 
-- 
2.7.4

[RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

[Fenglin Wu]

From: David Collins <collinsd@codeaurora.org>

Check that the apid for an SPMI interrupt falls between the
min_apid and max_apid that can be handled by the APPS processor
before invoking the per-apid interrupt handler:
periph_interrupt().

This avoids an access violation in rare cases where the status
bit is set for an interrupt that is not owned by the APPS
processor.

Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 4d7ad004..c4adc06 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 			id = ffs(status) - 1;
 			status &= ~BIT(id);
 			apid = id + i * 32;
+			if (apid < pmic_arb->min_apid
+			    || apid > pmic_arb->max_apid) {
+				WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
+					apid);
+				continue;
+			}
 			enable = readl_relaxed(
 					ver_ops->acc_enable(pmic_arb, apid));
 			if (enable & SPMI_PIC_ACC_ENABLE_BIT)
-- 
2.7.4

Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

[Stephen Boyd]

Quoting Fenglin Wu (2021-09-16 23:32:58)
> From: David Collins <collinsd@codeaurora.org>
> 
> Check that the apid for an SPMI interrupt falls between the
> min_apid and max_apid that can be handled by the APPS processor
> before invoking the per-apid interrupt handler:
> periph_interrupt().
> 
> This avoids an access violation in rare cases where the status
> bit is set for an interrupt that is not owned by the APPS
> processor.
> 
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---

Fixes? BTW, a lot of these patches are irqchip specific. It would be
good to get review from irqchip maintainers. Maybe we should split the
irqchip driver off via the auxiliary bus so that irqchip maintainers can
review. Please Cc them on irqchip related patches.

IRQCHIP DRIVERS
M:      Thomas Gleixner <tglx@linutronix.de>
M:      Marc Zyngier <maz@kernel.org>

>  drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 4d7ad004..c4adc06 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>                         id = ffs(status) - 1;
>                         status &= ~BIT(id);
>                         apid = id + i * 32;
> +                       if (apid < pmic_arb->min_apid
> +                           || apid > pmic_arb->max_apid) {

The || goes on the line above. What about making a local variable for
first and last and then shifting by 5 in the loop?

int first = pmic_arb->min_apid;
int last = pmic_arb->max_apid;

for (i = first >> 5; i <= last >> 5; i++)

	if (apid < first || apid > last)

> +                               WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
> +                                       apid);

Is there any way to recover from this? Or once the mapping is wrong
we're going to get interrupts that we don't know what to do with
forever?

> +                               continue;
> +                       }
>                         enable = readl_relaxed(
>                                         ver_ops->acc_enable(pmic_arb, apid));
>                         if (enable & SPMI_PIC_ACC_ENABLE_BIT)

Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

[Fenglin Wu]

On 10/13/2021 2:02 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:32:58)
>> From: David Collins <collinsd@codeaurora.org>
>>
>> Check that the apid for an SPMI interrupt falls between the
>> min_apid and max_apid that can be handled by the APPS processor
>> before invoking the per-apid interrupt handler:
>> periph_interrupt().
>>
>> This avoids an access violation in rare cases where the status
>> bit is set for an interrupt that is not owned by the APPS
>> processor.
>>
>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
> Fixes? BTW, a lot of these patches are irqchip specific. It would be
> good to get review from irqchip maintainers. Maybe we should split the
> irqchip driver off via the auxiliary bus so that irqchip maintainers can
> review. Please Cc them on irqchip related patches.
>
> IRQCHIP DRIVERS
> M:      Thomas Gleixner <tglx@linutronix.de>
> M:      Marc Zyngier <maz@kernel.org>
Sure, copied Thomas and Marc for code review.
This is a fix to avoid the register access violation in a case that an
interrupt is fired in a PMIC module which is not owned by APPS
processor.
>>   drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 4d7ad004..c4adc06 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>>                          id = ffs(status) - 1;
>>                          status &= ~BIT(id);
>>                          apid = id + i * 32;
>> +                       if (apid < pmic_arb->min_apid
>> +                           || apid > pmic_arb->max_apid) {
> The || goes on the line above. What about making a local variable for
> first and last and then shifting by 5 in the loop?
>
> int first = pmic_arb->min_apid;
> int last = pmic_arb->max_apid;
>
> for (i = first >> 5; i <= last >> 5; i++)
>
> 	if (apid < first || apid > last)
ACK, will update it following this.
>> +                               WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
>> +                                       apid);
> Is there any way to recover from this? Or once the mapping is wrong
> we're going to get interrupts that we don't know what to do with
> forever?
This is a rare case that the unexpected interrupt is fired in a module
not owned by APPS process, so the interrupt itself is not expected hence
no need to recover from this but just bail out to avoid following register
access violation.
>> +                               continue;
>> +                       }
>>                          enable = readl_relaxed(
>>                                          ver_ops->acc_enable(pmic_arb, apid));
>>                          if (enable & SPMI_PIC_ACC_ENABLE_BIT)

Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

[Stephen Boyd]

Quoting Fenglin Wu (2021-10-12 22:31:22)
> 
> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-09-16 23:32:58)
> >> From: David Collins <collinsd@codeaurora.org>
> >>
> >> Check that the apid for an SPMI interrupt falls between the
> >> min_apid and max_apid that can be handled by the APPS processor
> >> before invoking the per-apid interrupt handler:
> >> periph_interrupt().
> >>
> >> This avoids an access violation in rare cases where the status
> >> bit is set for an interrupt that is not owned by the APPS
> >> processor.
> >>
> >> Signed-off-by: David Collins <collinsd@codeaurora.org>
> >> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >> ---
> > Fixes? BTW, a lot of these patches are irqchip specific. It would be
> > good to get review from irqchip maintainers. Maybe we should split the
> > irqchip driver off via the auxiliary bus so that irqchip maintainers can
> > review. Please Cc them on irqchip related patches.
> >
> > IRQCHIP DRIVERS
> > M:      Thomas Gleixner <tglx@linutronix.de>
> > M:      Marc Zyngier <maz@kernel.org>
> Sure, copied Thomas and Marc for code review.
> This is a fix to avoid the register access violation in a case that an
> interrupt is fired in a PMIC module which is not owned by APPS
> processor.

Got it.

> >>   drivers/spmi/spmi-pmic-arb.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> >> index 4d7ad004..c4adc06 100644
> >> --- a/drivers/spmi/spmi-pmic-arb.c
> >> +++ b/drivers/spmi/spmi-pmic-arb.c
> >> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
> >>                          id = ffs(status) - 1;
> >>                          status &= ~BIT(id);
> >>                          apid = id + i * 32;
> >> +                       if (apid < pmic_arb->min_apid
> >> +                           || apid > pmic_arb->max_apid) {
> > The || goes on the line above. What about making a local variable for
> > first and last and then shifting by 5 in the loop?
> >
> > int first = pmic_arb->min_apid;
> > int last = pmic_arb->max_apid;
> >
> > for (i = first >> 5; i <= last >> 5; i++)
> >
> >       if (apid < first || apid > last)
> ACK, will update it following this.
> >> +                               WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
> >> +                                       apid);
> > Is there any way to recover from this? Or once the mapping is wrong
> > we're going to get interrupts that we don't know what to do with
> > forever?
> This is a rare case that the unexpected interrupt is fired in a module
> not owned by APPS process, so the interrupt itself is not expected hence
> no need to recover from this but just bail out to avoid following register
> access violation.

And then the irq stops coming? It feels like a misconfiguration in the
firmware that we're trying to hide, hence the WARN_ONCE(). Can we
somehow silence irqs that aren't owned by the APPS when this driver
probes so that they can't even happen after probe?

Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

[Fenglin Wu]

On 10/14/2021 3:25 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-12 22:31:22)
>> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-09-16 23:32:58)
>>>> From: David Collins <collinsd@codeaurora.org>
>>>>
>>>> Check that the apid for an SPMI interrupt falls between the
>>>> min_apid and max_apid that can be handled by the APPS processor
>>>> before invoking the per-apid interrupt handler:
>>>> periph_interrupt().
>>>>
>>>> This avoids an access violation in rare cases where the status
>>>> bit is set for an interrupt that is not owned by the APPS
>>>> processor.
>>>>
>>>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>> ---
>>> Fixes? BTW, a lot of these patches are irqchip specific. It would be
>>> good to get review from irqchip maintainers. Maybe we should split the
>>> irqchip driver off via the auxiliary bus so that irqchip maintainers can
>>> review. Please Cc them on irqchip related patches.
>>>
>>> IRQCHIP DRIVERS
>>> M:      Thomas Gleixner <tglx@linutronix.de>
>>> M:      Marc Zyngier <maz@kernel.org>
>> Sure, copied Thomas and Marc for code review.
>> This is a fix to avoid the register access violation in a case that an
>> interrupt is fired in a PMIC module which is not owned by APPS
>> processor.
> Got it.
>
>>>>    drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>>>> index 4d7ad004..c4adc06 100644
>>>> --- a/drivers/spmi/spmi-pmic-arb.c
>>>> +++ b/drivers/spmi/spmi-pmic-arb.c
>>>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>>>>                           id = ffs(status) - 1;
>>>>                           status &= ~BIT(id);
>>>>                           apid = id + i * 32;
>>>> +                       if (apid < pmic_arb->min_apid
>>>> +                           || apid > pmic_arb->max_apid) {
>>> The || goes on the line above. What about making a local variable for
>>> first and last and then shifting by 5 in the loop?
>>>
>>> int first = pmic_arb->min_apid;
>>> int last = pmic_arb->max_apid;
>>>
>>> for (i = first >> 5; i <= last >> 5; i++)
>>>
>>>        if (apid < first || apid > last)
>> ACK, will update it following this.
>>>> +                               WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
>>>> +                                       apid);
>>> Is there any way to recover from this? Or once the mapping is wrong
>>> we're going to get interrupts that we don't know what to do with
>>> forever?
>> This is a rare case that the unexpected interrupt is fired in a module
>> not owned by APPS process, so the interrupt itself is not expected hence
>> no need to recover from this but just bail out to avoid following register
>> access violation.
> And then the irq stops coming? It feels like a misconfiguration in the
> firmware that we're trying to hide, hence the WARN_ONCE(). Can we
> somehow silence irqs that aren't owned by the APPS when this driver
> probes so that they can't even happen after probe?
Actually this is a rarely happened case that couldn't be reproduced easily
and consistently for further debug. I agreed this should be caused by HW
misconfiguration or even some unknown HW bug that it would send out SPMI
interrupt messages with incorrect APID, but we have never had any chance
to find out the root cause. The patch here simply checked the APID and
bail out if it's not in the valid range, it won't cause anything bad but
improves the SW robustness. After that, the IRQ won't be triggered again
because the latched status in PMIC is not cleared. Also, because of the
access restriction to the registers corresponding to this APID, there is
nothing we can do from APPS processor side to keep it silent.

Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

[Stephen Boyd]

Quoting Fenglin Wu (2021-10-13 20:11:40)
> 
> On 10/14/2021 3:25 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-10-12 22:31:22)
> >> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
> >>> Quoting Fenglin Wu (2021-09-16 23:32:58)
> >>>> From: David Collins <collinsd@codeaurora.org>
> >>>>
> >>>> Check that the apid for an SPMI interrupt falls between the
> >>>> min_apid and max_apid that can be handled by the APPS processor
> >>>> before invoking the per-apid interrupt handler:
> >>>> periph_interrupt().
> >>>>
> >>>> This avoids an access violation in rare cases where the status
> >>>> bit is set for an interrupt that is not owned by the APPS
> >>>> processor.
> >>>>
> >>>> Signed-off-by: David Collins <collinsd@codeaurora.org>
> >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >>>> ---
> >>> Fixes? BTW, a lot of these patches are irqchip specific. It would be
> >>> good to get review from irqchip maintainers. Maybe we should split the
> >>> irqchip driver off via the auxiliary bus so that irqchip maintainers can
> >>> review. Please Cc them on irqchip related patches.
> >>>
> >>> IRQCHIP DRIVERS
> >>> M:      Thomas Gleixner <tglx@linutronix.de>
> >>> M:      Marc Zyngier <maz@kernel.org>
> >> Sure, copied Thomas and Marc for code review.
> >> This is a fix to avoid the register access violation in a case that an
> >> interrupt is fired in a PMIC module which is not owned by APPS
> >> processor.
> > Got it.
> >
> >>>>    drivers/spmi/spmi-pmic-arb.c | 6 ++++++
> >>>>    1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> >>>> index 4d7ad004..c4adc06 100644
> >>>> --- a/drivers/spmi/spmi-pmic-arb.c
> >>>> +++ b/drivers/spmi/spmi-pmic-arb.c
> >>>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
> >>>>                           id = ffs(status) - 1;
> >>>>                           status &= ~BIT(id);
> >>>>                           apid = id + i * 32;
> >>>> +                       if (apid < pmic_arb->min_apid
> >>>> +                           || apid > pmic_arb->max_apid) {
> >>> The || goes on the line above. What about making a local variable for
> >>> first and last and then shifting by 5 in the loop?
> >>>
> >>> int first = pmic_arb->min_apid;
> >>> int last = pmic_arb->max_apid;
> >>>
> >>> for (i = first >> 5; i <= last >> 5; i++)
> >>>
> >>>        if (apid < first || apid > last)
> >> ACK, will update it following this.
> >>>> +                               WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
> >>>> +                                       apid);
> >>> Is there any way to recover from this? Or once the mapping is wrong
> >>> we're going to get interrupts that we don't know what to do with
> >>> forever?
> >> This is a rare case that the unexpected interrupt is fired in a module
> >> not owned by APPS process, so the interrupt itself is not expected hence
> >> no need to recover from this but just bail out to avoid following register
> >> access violation.
> > And then the irq stops coming? It feels like a misconfiguration in the
> > firmware that we're trying to hide, hence the WARN_ONCE(). Can we
> > somehow silence irqs that aren't owned by the APPS when this driver
> > probes so that they can't even happen after probe?
> Actually this is a rarely happened case that couldn't be reproduced easily
> and consistently for further debug. I agreed this should be caused by HW
> misconfiguration or even some unknown HW bug that it would send out SPMI
> interrupt messages with incorrect APID, but we have never had any chance
> to find out the root cause. The patch here simply checked the APID and
> bail out if it's not in the valid range, it won't cause anything bad but
> improves the SW robustness. After that, the IRQ won't be triggered again
> because the latched status in PMIC is not cleared. Also, because of the
> access restriction to the registers corresponding to this APID, there is
> nothing we can do from APPS processor side to keep it silent.

This patch seems like a band-aid for an issue that isn't fully
understood. I suppose it's good that the irq will stay asserted forever
and then it won't happen again until it gets cleared by some other
processor in the SoC. Instead of the WARN_ONCE() can we track if any irq
is handled when the chained irq is raised, and if nothing is handled
then call handle_bad_irq() on the chained descriptor? Take a look at
pinctrl-msm.c to see how they handled spurious irqs that aren't actually
directed at the APPS processor. We should do something similar here.

Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

[Fenglin Wu]

On 10/15/2021 9:15 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-13 20:11:40)
>> On 10/14/2021 3:25 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-10-12 22:31:22)
>>>> On 10/13/2021 2:02 AM, Stephen Boyd wrote:
>>>>> Quoting Fenglin Wu (2021-09-16 23:32:58)
>>>>>> From: David Collins <collinsd@codeaurora.org>
>>>>>>
>>>>>> Check that the apid for an SPMI interrupt falls between the
>>>>>> min_apid and max_apid that can be handled by the APPS processor
>>>>>> before invoking the per-apid interrupt handler:
>>>>>> periph_interrupt().
>>>>>>
>>>>>> This avoids an access violation in rare cases where the status
>>>>>> bit is set for an interrupt that is not owned by the APPS
>>>>>> processor.
>>>>>>
>>>>>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>>>> ---
>>>>> Fixes? BTW, a lot of these patches are irqchip specific. It would be
>>>>> good to get review from irqchip maintainers. Maybe we should split the
>>>>> irqchip driver off via the auxiliary bus so that irqchip maintainers can
>>>>> review. Please Cc them on irqchip related patches.
>>>>>
>>>>> IRQCHIP DRIVERS
>>>>> M:      Thomas Gleixner <tglx@linutronix.de>
>>>>> M:      Marc Zyngier <maz@kernel.org>
>>>> Sure, copied Thomas and Marc for code review.
>>>> This is a fix to avoid the register access violation in a case that an
>>>> interrupt is fired in a PMIC module which is not owned by APPS
>>>> processor.
>>> Got it.
>>>
>>>>>>     drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>>>>>>     1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>>>>>> index 4d7ad004..c4adc06 100644
>>>>>> --- a/drivers/spmi/spmi-pmic-arb.c
>>>>>> +++ b/drivers/spmi/spmi-pmic-arb.c
>>>>>> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>>>>>>                            id = ffs(status) - 1;
>>>>>>                            status &= ~BIT(id);
>>>>>>                            apid = id + i * 32;
>>>>>> +                       if (apid < pmic_arb->min_apid
>>>>>> +                           || apid > pmic_arb->max_apid) {
>>>>> The || goes on the line above. What about making a local variable for
>>>>> first and last and then shifting by 5 in the loop?
>>>>>
>>>>> int first = pmic_arb->min_apid;
>>>>> int last = pmic_arb->max_apid;
>>>>>
>>>>> for (i = first >> 5; i <= last >> 5; i++)
>>>>>
>>>>>         if (apid < first || apid > last)
>>>> ACK, will update it following this.
>>>>>> +                               WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
>>>>>> +                                       apid);
>>>>> Is there any way to recover from this? Or once the mapping is wrong
>>>>> we're going to get interrupts that we don't know what to do with
>>>>> forever?
>>>> This is a rare case that the unexpected interrupt is fired in a module
>>>> not owned by APPS process, so the interrupt itself is not expected hence
>>>> no need to recover from this but just bail out to avoid following register
>>>> access violation.
>>> And then the irq stops coming? It feels like a misconfiguration in the
>>> firmware that we're trying to hide, hence the WARN_ONCE(). Can we
>>> somehow silence irqs that aren't owned by the APPS when this driver
>>> probes so that they can't even happen after probe?
>> Actually this is a rarely happened case that couldn't be reproduced easily
>> and consistently for further debug. I agreed this should be caused by HW
>> misconfiguration or even some unknown HW bug that it would send out SPMI
>> interrupt messages with incorrect APID, but we have never had any chance
>> to find out the root cause. The patch here simply checked the APID and
>> bail out if it's not in the valid range, it won't cause anything bad but
>> improves the SW robustness. After that, the IRQ won't be triggered again
>> because the latched status in PMIC is not cleared. Also, because of the
>> access restriction to the registers corresponding to this APID, there is
>> nothing we can do from APPS processor side to keep it silent.
> This patch seems like a band-aid for an issue that isn't fully
> understood. I suppose it's good that the irq will stay asserted forever
> and then it won't happen again until it gets cleared by some other
> processor in the SoC. Instead of the WARN_ONCE() can we track if any irq
> is handled when the chained irq is raised, and if nothing is handled
> then call handle_bad_irq() on the chained descriptor? Take a look at
> pinctrl-msm.c to see how they handled spurious irqs that aren't actually
> directed at the APPS processor. We should do something similar here.
Sure, I will do it that way.

[RESEND PATCH v1 4/9] spmi: pmic-arb: add support to dispatch interrupt based on IRQ status

[Fenglin Wu]

From: Ashay Jaiswal <ashayj@codeaurora.org>

Current implementation of SPMI arbiter dispatches interrupt based on the
Arbiter's accumulator status, in some cases the accumulator status may
remain zero and the interrupt remains un-handled. Add logic to dispatch
interrupts based Arbiter's IRQ status if the accumulator status is zero.

Signed-off-by: Ashay Jaiswal <ashayj@codeaurora.org>
Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index c4adc06..59c445b 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -525,12 +525,18 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 	u8 ee = pmic_arb->ee;
 	u32 status, enable;
 	int i, id, apid;
+	/* status based dispatch */
+	bool acc_valid = false;
+	u32 irq_status = 0;
 
 	chained_irq_enter(chip, desc);
 
 	for (i = first; i <= last; ++i) {
 		status = readl_relaxed(
 				ver_ops->owner_acc_status(pmic_arb, ee, i));
+		if (status)
+			acc_valid = true;
+
 		while (status) {
 			id = ffs(status) - 1;
 			status &= ~BIT(id);
@@ -548,6 +554,28 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 		}
 	}
 
+	/* ACC_STATUS is empty but IRQ fired check IRQ_STATUS */
+	if (!acc_valid) {
+		for (i = pmic_arb->min_apid; i <= pmic_arb->max_apid; i++) {
+			/* skip if APPS is not irq owner */
+			if (pmic_arb->apid_data[i].irq_ee != pmic_arb->ee)
+				continue;
+
+			irq_status = readl_relaxed(
+					     ver_ops->irq_status(pmic_arb, i));
+			if (irq_status) {
+				enable = readl_relaxed(
+					     ver_ops->acc_enable(pmic_arb, i));
+				if (enable & SPMI_PIC_ACC_ENABLE_BIT) {
+					dev_dbg(&pmic_arb->spmic->dev,
+						"Dispatching IRQ for apid=%d status=%x\n",
+						i, irq_status);
+					periph_interrupt(pmic_arb, i);
+				}
+			}
+		}
+	}
+
 	chained_irq_exit(chip, desc);
 }
 
-- 
2.7.4

[RESEND PATCH v1 5/9] spmi: pmic-arb: correct duplicate APID to PPID mapping logic

[Fenglin Wu]

From: David Collins <collinsd@codeaurora.org>

Correct the way that duplicate PPID mappings are handled for PMIC
arbiter v5.  The final APID mapped to a given PPID should be the
one which has write owner = APPS EE, if it exists, or if not
that, then the first APID mapped to the PPID, if it exists.

Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 59c445b..f1b72d8 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -918,7 +918,8 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
 	 * version 5, there is more than one APID mapped to each PPID.
 	 * The owner field for each of these mappings specifies the EE which is
 	 * allowed to write to the APID.  The owner of the last (highest) APID
-	 * for a given PPID will receive interrupts from the PPID.
+	 * which has the IRQ owner bit set for a given PPID will receive
+	 * interrupts from the PPID.
 	 */
 	for (i = 0; ; i++, apidd++) {
 		offset = pmic_arb->ver_ops->apid_map_offset(i);
@@ -941,16 +942,16 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
 		apid = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
 		prev_apidd = &pmic_arb->apid_data[apid];
 
-		if (valid && is_irq_ee &&
-				prev_apidd->write_ee == pmic_arb->ee) {
+		if (!valid || apidd->write_ee == pmic_arb->ee) {
+			/* First PPID mapping or one for this EE */
+			pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
+		} else if (valid && is_irq_ee &&
+			   prev_apidd->write_ee == pmic_arb->ee) {
 			/*
 			 * Duplicate PPID mapping after the one for this EE;
 			 * override the irq owner
 			 */
 			prev_apidd->irq_ee = apidd->irq_ee;
-		} else if (!valid || is_irq_ee) {
-			/* First PPID mapping or duplicate for another EE */
-			pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
 		}
 
 		apidd->ppid = ppid;
-- 
2.7.4

Re: [RESEND PATCH v1 5/9] spmi: pmic-arb: correct duplicate APID to PPID mapping logic

[Stephen Boyd]

Quoting Fenglin Wu (2021-09-16 23:33:00)
> From: David Collins <collinsd@codeaurora.org>
> 
> Correct the way that duplicate PPID mappings are handled for PMIC
> arbiter v5.  The final APID mapped to a given PPID should be the
> one which has write owner = APPS EE, if it exists, or if not
> that, then the first APID mapped to the PPID, if it exists.
> 
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---

Does this need a Fixes tag?

>  drivers/spmi/spmi-pmic-arb.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 59c445b..f1b72d8 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -918,7 +918,8 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>          * version 5, there is more than one APID mapped to each PPID.
>          * The owner field for each of these mappings specifies the EE which is
>          * allowed to write to the APID.  The owner of the last (highest) APID
> -        * for a given PPID will receive interrupts from the PPID.
> +        * which has the IRQ owner bit set for a given PPID will receive
> +        * interrupts from the PPID.
>          */
>         for (i = 0; ; i++, apidd++) {
>                 offset = pmic_arb->ver_ops->apid_map_offset(i);
> @@ -941,16 +942,16 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>                 apid = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
>                 prev_apidd = &pmic_arb->apid_data[apid];
>  
> -               if (valid && is_irq_ee &&
> -                               prev_apidd->write_ee == pmic_arb->ee) {
> +               if (!valid || apidd->write_ee == pmic_arb->ee) {
> +                       /* First PPID mapping or one for this EE */
> +                       pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
> +               } else if (valid && is_irq_ee &&
> +                          prev_apidd->write_ee == pmic_arb->ee) {

This can be one line please.

>                         /*
>                          * Duplicate PPID mapping after the one for this EE;
>                          * override the irq owner
>                          */
>                         prev_apidd->irq_ee = apidd->irq_ee;
> -               } else if (!valid || is_irq_ee) {
> -                       /* First PPID mapping or duplicate for another EE */
> -                       pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
>                 }
>  
>                 apidd->ppid = ppid;
> -- 
> 2.7.4
>

Re: [RESEND PATCH v1 5/9] spmi: pmic-arb: correct duplicate APID to PPID mapping logic

[Fenglin Wu]

On 10/13/2021 1:44 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:33:00)
>> From: David Collins <collinsd@codeaurora.org>
>>
>> Correct the way that duplicate PPID mappings are handled for PMIC
>> arbiter v5.  The final APID mapped to a given PPID should be the
>> one which has write owner = APPS EE, if it exists, or if not
>> that, then the first APID mapped to the PPID, if it exists.
>>
>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
> Does this need a Fixes tag?
ACK, will add a Fixes tag
>
>>   drivers/spmi/spmi-pmic-arb.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 59c445b..f1b72d8 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -918,7 +918,8 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>>           * version 5, there is more than one APID mapped to each PPID.
>>           * The owner field for each of these mappings specifies the EE which is
>>           * allowed to write to the APID.  The owner of the last (highest) APID
>> -        * for a given PPID will receive interrupts from the PPID.
>> +        * which has the IRQ owner bit set for a given PPID will receive
>> +        * interrupts from the PPID.
>>           */
>>          for (i = 0; ; i++, apidd++) {
>>                  offset = pmic_arb->ver_ops->apid_map_offset(i);
>> @@ -941,16 +942,16 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>>                  apid = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
>>                  prev_apidd = &pmic_arb->apid_data[apid];
>>   
>> -               if (valid && is_irq_ee &&
>> -                               prev_apidd->write_ee == pmic_arb->ee) {
>> +               if (!valid || apidd->write_ee == pmic_arb->ee) {
>> +                       /* First PPID mapping or one for this EE */
>> +                       pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
>> +               } else if (valid && is_irq_ee &&
>> +                          prev_apidd->write_ee == pmic_arb->ee) {
> This can be one line please.
ACK.
>>                          /*
>>                           * Duplicate PPID mapping after the one for this EE;
>>                           * override the irq owner
>>                           */
>>                          prev_apidd->irq_ee = apidd->irq_ee;
>> -               } else if (!valid || is_irq_ee) {
>> -                       /* First PPID mapping or duplicate for another EE */
>> -                       pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
>>                  }
>>   
>>                  apidd->ppid = ppid;
>> -- 
>> 2.7.4
>>

[RESEND PATCH v1 6/9] spmi: pmic-arb: block access for invalid PMIC arbiter v5 SPMI writes

[Fenglin Wu]

From: David Collins <collinsd@codeaurora.org>

The system crashes due to an access permission violation when
writing to a PMIC peripheral which is not owned by the current
ee.  Add a check for PMIC arbiter version 5 for such invalid
write requests and return an error instead of crashing the
system.

Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index f1b72d8..9239830 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1020,6 +1020,11 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
 		offset = 0x10000 * pmic_arb->ee + 0x80 * apid;
 		break;
 	case PMIC_ARB_CHANNEL_RW:
+		if (pmic_arb->apid_data[apid].write_ee != pmic_arb->ee) {
+			dev_err(&pmic_arb->spmic->dev, "disallowed SPMI write to sid=%u, addr=0x%04X\n",
+				sid, addr);
+			return -EPERM;
+		}
 		offset = 0x10000 * apid;
 		break;
 	}
-- 
2.7.4

[RESEND PATCH v1 7/9] spmi: pmic-arb: support updating interrupt type flags

[Fenglin Wu]

From: Yimin Peng <yiminp@codeaurora.org>

Have the qpnpint_irq_set_type function clear unwanted high/low
trigger bits when updating the interrupt flags.

Signed-off-by: Yimin Peng <yiminp@codeaurora.org>
Signed-off-by: Subbaraman Narayanamurthy <subbaram@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 9239830..988204c 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -636,8 +636,12 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
 		type.type |= BIT(irq);
 		if (flow_type & IRQF_TRIGGER_RISING)
 			type.polarity_high |= BIT(irq);
+		else
+			type.polarity_high &= ~BIT(irq);
 		if (flow_type & IRQF_TRIGGER_FALLING)
 			type.polarity_low  |= BIT(irq);
+		else
+			type.polarity_low  &= ~BIT(irq);
 
 		flow_handler = handle_edge_irq;
 	} else {
@@ -646,10 +650,13 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
 			return -EINVAL;
 
 		type.type &= ~BIT(irq); /* level trig */
-		if (flow_type & IRQF_TRIGGER_HIGH)
+		if (flow_type & IRQF_TRIGGER_HIGH) {
 			type.polarity_high |= BIT(irq);
-		else
+			type.polarity_low  &= ~BIT(irq);
+		} else {
 			type.polarity_low  |= BIT(irq);
+			type.polarity_high &= ~BIT(irq);
+		}
 
 		flow_handler = handle_level_irq;
 	}
-- 
2.7.4

Re: [RESEND PATCH v1 7/9] spmi: pmic-arb: support updating interrupt type flags

[Stephen Boyd]

Quoting Fenglin Wu (2021-09-16 23:33:02)
> From: Yimin Peng <yiminp@codeaurora.org>
> 
> Have the qpnpint_irq_set_type function clear unwanted high/low
> trigger bits when updating the interrupt flags.

Why?

> 
> Signed-off-by: Yimin Peng <yiminp@codeaurora.org>
> Signed-off-by: Subbaraman Narayanamurthy <subbaram@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---

Does this need a Fixes tag?

Re: [RESEND PATCH v1 7/9] spmi: pmic-arb: support updating interrupt type flags

[Fenglin Wu]

copy IRQCHIP driver maintainers as requested in another patch.

On 10/13/2021 1:42 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:33:02)
>> From: Yimin Peng <yiminp@codeaurora.org>
>>
>> Have the qpnpint_irq_set_type function clear unwanted high/low
>> trigger bits when updating the interrupt flags.
> Why?
There was a requirement to update the PMIC module interrupt type 
dynamically
(such as from low level trigger to high level trigger), hence it's required
to clear the unnecessary trigger type when setting it.
>> Signed-off-by: Yimin Peng <yiminp@codeaurora.org>
>> Signed-off-by: Subbaraman Narayanamurthy <subbaram@codeaurora.org>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
> Does this need a Fixes tag?
Maybe no need to a Fixes tag because this is part of the initial code when
interrupt handling is added?

Re: [RESEND PATCH v1 7/9] spmi: pmic-arb: support updating interrupt type flags

[Stephen Boyd]

Quoting Fenglin Wu (2021-10-12 23:27:22)
> copy IRQCHIP driver maintainers as requested in another patch.
> 
> On 10/13/2021 1:42 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-09-16 23:33:02)
> >> From: Yimin Peng <yiminp@codeaurora.org>
> >>
> >> Have the qpnpint_irq_set_type function clear unwanted high/low
> >> trigger bits when updating the interrupt flags.
> > Why?
> There was a requirement to update the PMIC module interrupt type 
> dynamically
> (such as from low level trigger to high level trigger), hence it's required
> to clear the unnecessary trigger type when setting it.

Can you clearly describe the problem in the commit text? Is this a bug
fix?

> >> Signed-off-by: Yimin Peng <yiminp@codeaurora.org>
> >> Signed-off-by: Subbaraman Narayanamurthy <subbaram@codeaurora.org>
> >> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >> ---
> > Does this need a Fixes tag?
> Maybe no need to a Fixes tag because this is part of the initial code when
> interrupt handling is added?

Was it always broken? Or trigger types haven't been changing at runtime
because most users are requesting irqs and forgetting about it? Are you
using gpio-keys or something like that now? Adding a Fixes tag doesn't
hurt.

Re: [RESEND PATCH v1 7/9] spmi: pmic-arb: support updating interrupt type flags

[Fenglin Wu]

On 10/14/2021 3:37 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-12 23:27:22)
>> copy IRQCHIP driver maintainers as requested in another patch.
>>
>> On 10/13/2021 1:42 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-09-16 23:33:02)
>>>> From: Yimin Peng <yiminp@codeaurora.org>
>>>>
>>>> Have the qpnpint_irq_set_type function clear unwanted high/low
>>>> trigger bits when updating the interrupt flags.
>>> Why?
>> There was a requirement to update the PMIC module interrupt type
>> dynamically
>> (such as from low level trigger to high level trigger), hence it's required
>> to clear the unnecessary trigger type when setting it.
> Can you clearly describe the problem in the commit text? Is this a bug
> fix?
sure, will do.
>>>> Signed-off-by: Yimin Peng <yiminp@codeaurora.org>
>>>> Signed-off-by: Subbaraman Narayanamurthy <subbaram@codeaurora.org>
>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>> ---
>>> Does this need a Fixes tag?
>> Maybe no need to a Fixes tag because this is part of the initial code when
>> interrupt handling is added?
> Was it always broken? Or trigger types haven't been changing at runtime
> because most users are requesting irqs and forgetting about it? Are you
> using gpio-keys or something like that now? Adding a Fixes tag doesn't
> hurt.
You are right, it was reported by someone when using a PMIC GPIO and update
the interrupt at runtime, I am not sure if it's gpio-keys but that can be a
realistic case.
I will add a Fixes tag for it.

[RESEND PATCH v1 8/9] spmi: pmic-arb: make interrupt support optional

[Fenglin Wu]

From: David Collins <collinsd@codeaurora.org>

Make the support of PMIC peripheral interrupts optional for
spmi-pmic-arb devices.  This is useful in situations where
SPMI address mapping is required without the need for IRQ
support.

Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 988204c..55fa981 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}
 
-	pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
-	if (pmic_arb->irq < 0) {
-		err = pmic_arb->irq;
-		goto err_put_ctrl;
+	if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
+		pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
+		if (pmic_arb->irq < 0) {
+			err = pmic_arb->irq;
+			goto err_put_ctrl;
+		}
 	}
 
 	err = of_property_read_u32(pdev->dev.of_node, "qcom,channel", &channel);
@@ -1343,17 +1345,22 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		}
 	}
 
-	dev_dbg(&pdev->dev, "adding irq domain\n");
-	pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
-					 &pmic_arb_irq_domain_ops, pmic_arb);
-	if (!pmic_arb->domain) {
-		dev_err(&pdev->dev, "unable to create irq_domain\n");
-		err = -ENOMEM;
-		goto err_put_ctrl;
+	if (pmic_arb->irq > 0) {
+		dev_dbg(&pdev->dev, "adding irq domain\n");
+		pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
+					    &pmic_arb_irq_domain_ops, pmic_arb);
+		if (!pmic_arb->domain) {
+			dev_err(&pdev->dev, "unable to create irq_domain\n");
+			err = -ENOMEM;
+			goto err_put_ctrl;
+		}
+
+		irq_set_chained_handler_and_data(pmic_arb->irq,
+						pmic_arb_chained_irq, pmic_arb);
+	} else {
+		dev_dbg(&pdev->dev, "not supporting PMIC interrupts\n");
 	}
 
-	irq_set_chained_handler_and_data(pmic_arb->irq, pmic_arb_chained_irq,
-					pmic_arb);
 	err = spmi_controller_add(ctrl);
 	if (err)
 		goto err_domain_remove;
@@ -1361,8 +1368,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	return 0;
 
 err_domain_remove:
-	irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
-	irq_domain_remove(pmic_arb->domain);
+	if (pmic_arb->irq > 0) {
+		irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+		irq_domain_remove(pmic_arb->domain);
+	}
 err_put_ctrl:
 	spmi_controller_put(ctrl);
 	return err;
@@ -1373,8 +1382,10 @@ static int spmi_pmic_arb_remove(struct platform_device *pdev)
 	struct spmi_controller *ctrl = platform_get_drvdata(pdev);
 	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	spmi_controller_remove(ctrl);
-	irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
-	irq_domain_remove(pmic_arb->domain);
+	if (pmic_arb->irq > 0) {
+		irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+		irq_domain_remove(pmic_arb->domain);
+	}
 	spmi_controller_put(ctrl);
 	return 0;
 }
-- 
2.7.4

Re: [RESEND PATCH v1 8/9] spmi: pmic-arb: make interrupt support optional

[Stephen Boyd]

Quoting Fenglin Wu (2021-09-16 23:33:03)
> From: David Collins <collinsd@codeaurora.org>
> 
> Make the support of PMIC peripheral interrupts optional for
> spmi-pmic-arb devices.  This is useful in situations where
> SPMI address mapping is required without the need for IRQ
> support.
> 
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------

Is there a binding update? Can the binding be converted to YAML as well?

>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 988204c..55fa981 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>                 goto err_put_ctrl;
>         }
>  
> -       pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> -       if (pmic_arb->irq < 0) {
> -               err = pmic_arb->irq;
> -               goto err_put_ctrl;
> +       if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {

I don't think we should be keying off of interrupt-names. Instead we
should be checking for something else. Maybe interrupt-controller
property?

> +               pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> +               if (pmic_arb->irq < 0) {
> +                       err = pmic_arb->irq;
> +                       goto err_put_ctrl;

Re: [RESEND PATCH v1 8/9] spmi: pmic-arb: make interrupt support optional

[Fenglin Wu]

On 10/13/2021 1:41 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-09-16 23:33:03)
>> From: David Collins <collinsd@codeaurora.org>
>>
>> Make the support of PMIC peripheral interrupts optional for
>> spmi-pmic-arb devices.  This is useful in situations where
>> SPMI address mapping is required without the need for IRQ
>> support.
>>
>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>> ---
>>   drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
> Is there a binding update? Can the binding be converted to YAML as well?
This change doesn't add/update any dtsi properties but just checking if an
existing property is present to decide if IRQ support is required, so no
binding change is needed.
>
>>   1 file changed, 28 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 988204c..55fa981 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>>                  goto err_put_ctrl;
>>          }
>>   
>> -       pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
>> -       if (pmic_arb->irq < 0) {
>> -               err = pmic_arb->irq;
>> -               goto err_put_ctrl;
>> +       if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
> I don't think we should be keying off of interrupt-names. Instead we
> should be checking for something else. Maybe interrupt-controller
> property?
Sure, I can update it to check the presence of "interrupt-controller" 
property.
>> +               pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
>> +               if (pmic_arb->irq < 0) {
>> +                       err = pmic_arb->irq;
>> +                       goto err_put_ctrl;

Re: [RESEND PATCH v1 8/9] spmi: pmic-arb: make interrupt support optional

[Stephen Boyd]

Quoting Fenglin Wu (2021-10-13 01:36:54)
> 
> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-09-16 23:33:03)
> >> From: David Collins <collinsd@codeaurora.org>
> >>
> >> Make the support of PMIC peripheral interrupts optional for
> >> spmi-pmic-arb devices.  This is useful in situations where
> >> SPMI address mapping is required without the need for IRQ
> >> support.
> >>
> >> Signed-off-by: David Collins <collinsd@codeaurora.org>
> >> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >> ---
> >>   drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
> > Is there a binding update? Can the binding be converted to YAML as well?
> This change doesn't add/update any dtsi properties but just checking if an
> existing property is present to decide if IRQ support is required, so no
> binding change is needed.

The property is now optional in the binding. Please update the binding.

> >
> >>   1 file changed, 28 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> >> index 988204c..55fa981 100644
> >> --- a/drivers/spmi/spmi-pmic-arb.c
> >> +++ b/drivers/spmi/spmi-pmic-arb.c
> >> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >>                  goto err_put_ctrl;
> >>          }
> >>   
> >> -       pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> >> -       if (pmic_arb->irq < 0) {
> >> -               err = pmic_arb->irq;
> >> -               goto err_put_ctrl;
> >> +       if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
> > I don't think we should be keying off of interrupt-names. Instead we
> > should be checking for something else. Maybe interrupt-controller
> > property?
> Sure, I can update it to check the presence of "interrupt-controller" 
> property.

Ok.

Re: [RESEND PATCH v1 8/9] spmi: pmic-arb: make interrupt support optional

[Fenglin Wu]

On 10/14/2021 3:38 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-13 01:36:54)
>> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-09-16 23:33:03)
>>>> From: David Collins <collinsd@codeaurora.org>
>>>>
>>>> Make the support of PMIC peripheral interrupts optional for
>>>> spmi-pmic-arb devices.  This is useful in situations where
>>>> SPMI address mapping is required without the need for IRQ
>>>> support.
>>>>
>>>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>> ---
>>>>    drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
>>> Is there a binding update? Can the binding be converted to YAML as well?
>> This change doesn't add/update any dtsi properties but just checking if an
>> existing property is present to decide if IRQ support is required, so no
>> binding change is needed.
> The property is now optional in the binding. Please update the binding.
Right, thanks for pointing it out. I forgot that part.
I will update the binding. How about only update the interrupt properties as
optional in this series then I can come up with following patch to convert
the binding to YAML format?
>
>>>>    1 file changed, 28 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>>>> index 988204c..55fa981 100644
>>>> --- a/drivers/spmi/spmi-pmic-arb.c
>>>> +++ b/drivers/spmi/spmi-pmic-arb.c
>>>> @@ -1280,10 +1280,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>>>>                   goto err_put_ctrl;
>>>>           }
>>>>    
>>>> -       pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
>>>> -       if (pmic_arb->irq < 0) {
>>>> -               err = pmic_arb->irq;
>>>> -               goto err_put_ctrl;
>>>> +       if (of_find_property(pdev->dev.of_node, "interrupt-names", NULL)) {
>>> I don't think we should be keying off of interrupt-names. Instead we
>>> should be checking for something else. Maybe interrupt-controller
>>> property?
>> Sure, I can update it to check the presence of "interrupt-controller"
>> property.
> Ok.

Re: [RESEND PATCH v1 8/9] spmi: pmic-arb: make interrupt support optional

[Stephen Boyd]

Quoting Fenglin Wu (2021-10-13 20:20:57)
> 
> On 10/14/2021 3:38 AM, Stephen Boyd wrote:
> > Quoting Fenglin Wu (2021-10-13 01:36:54)
> >> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
> >>> Quoting Fenglin Wu (2021-09-16 23:33:03)
> >>>> From: David Collins <collinsd@codeaurora.org>
> >>>>
> >>>> Make the support of PMIC peripheral interrupts optional for
> >>>> spmi-pmic-arb devices.  This is useful in situations where
> >>>> SPMI address mapping is required without the need for IRQ
> >>>> support.
> >>>>
> >>>> Signed-off-by: David Collins <collinsd@codeaurora.org>
> >>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> >>>> ---
> >>>>    drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
> >>> Is there a binding update? Can the binding be converted to YAML as well?
> >> This change doesn't add/update any dtsi properties but just checking if an
> >> existing property is present to decide if IRQ support is required, so no
> >> binding change is needed.
> > The property is now optional in the binding. Please update the binding.
> Right, thanks for pointing it out. I forgot that part.
> I will update the binding. How about only update the interrupt properties as
> optional in this series then I can come up with following patch to convert
> the binding to YAML format?

Sure. The benefit of converting it to YAML is that we can use the
checker to quickly validate the binding vs. having to read the whole
thing to understand that it's correct. Converting an existing binding to
YAML shouldn't be that hard.

Re: [RESEND PATCH v1 8/9] spmi: pmic-arb: make interrupt support optional

[Fenglin Wu]

On 10/15/2021 9:17 AM, Stephen Boyd wrote:
> Quoting Fenglin Wu (2021-10-13 20:20:57)
>> On 10/14/2021 3:38 AM, Stephen Boyd wrote:
>>> Quoting Fenglin Wu (2021-10-13 01:36:54)
>>>> On 10/13/2021 1:41 AM, Stephen Boyd wrote:
>>>>> Quoting Fenglin Wu (2021-09-16 23:33:03)
>>>>>> From: David Collins <collinsd@codeaurora.org>
>>>>>>
>>>>>> Make the support of PMIC peripheral interrupts optional for
>>>>>> spmi-pmic-arb devices.  This is useful in situations where
>>>>>> SPMI address mapping is required without the need for IRQ
>>>>>> support.
>>>>>>
>>>>>> Signed-off-by: David Collins <collinsd@codeaurora.org>
>>>>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>>>>> ---
>>>>>>     drivers/spmi/spmi-pmic-arb.c | 45 +++++++++++++++++++++++++++-----------------
>>>>> Is there a binding update? Can the binding be converted to YAML as well?
>>>> This change doesn't add/update any dtsi properties but just checking if an
>>>> existing property is present to decide if IRQ support is required, so no
>>>> binding change is needed.
>>> The property is now optional in the binding. Please update the binding.
>> Right, thanks for pointing it out. I forgot that part.
>> I will update the binding. How about only update the interrupt properties as
>> optional in this series then I can come up with following patch to convert
>> the binding to YAML format?
> Sure. The benefit of converting it to YAML is that we can use the
> checker to quickly validate the binding vs. having to read the whole
> thing to understand that it's correct. Converting an existing binding to
> YAML shouldn't be that hard.
Thanks, will do that for sure after this series of the changes.

[RESEND PATCH v1 9/9] spmi: pmic-arb: increase SPMI transaction timeout delay

[Fenglin Wu]

From: David Collins <collinsd@codeaurora.org>

Increase the SPMI transaction timeout delay from 100 us to
1000 us in order to account for the slower execution time
found on some simulator targets.

Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 55fa981..08c2566 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -91,7 +91,7 @@ enum pmic_arb_channel {
 
 /* Maximum number of support PMIC peripherals */
 #define PMIC_ARB_MAX_PERIPHS		512
-#define PMIC_ARB_TIMEOUT_US		100
+#define PMIC_ARB_TIMEOUT_US		1000
 #define PMIC_ARB_MAX_TRANS_BYTES	(8)
 
 #define PMIC_ARB_APID_MASK		0xFF
-- 
2.7.4

[RESEND PATCH v1 0/9] A bunch of fix and optimization patches in spmi-pmic-arb.c

[Fenglin Wu]

This change series includes some fixes and optimizations in spmi-pmic-arb.c.
Please see change detail and description in each of the patch. Thanks!

Abhijeet Dharmapurikar (1):
  spmi: pmic-arb: add a print in cleanup_irq

Ashay Jaiswal (1):
  spmi: pmic-arb: add support to dispatch interrupt based on IRQ status

David Collins (5):
  spmi: pmic-arb: check apid against limits before calling irq handler
  spmi: pmic-arb: correct duplicate APID to PPID mapping logic
  spmi: pmic-arb: block access for invalid PMIC arbiter v5 SPMI writes
  spmi: pmic-arb: make interrupt support optional
  spmi: pmic-arb: increase SPMI transaction timeout delay

Subbaraman Narayanamurthy (1):
  spmi: pmic-arb: do not ack and clear peripheral interrupts in
    cleanup_irq

Yimin Peng (1):
  spmi: pmic-arb: support updating interrupt type flags

 drivers/spmi/spmi-pmic-arb.c | 127 +++++++++++++++++++++++++++++++------------
 1 file changed, 91 insertions(+), 36 deletions(-)

-- 
2.7.4

[RESEND PATCH v1 1/9] spmi: pmic-arb: add a print in cleanup_irq

[Fenglin Wu]

From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>

The cleanup_irq() was meant to clear and mask interrupts that were
left enabled in the hardware but there was no interrupt handler
registered for it. Add an error print when it gets invoked.

Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index bbbd311..295e19f 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -489,6 +489,8 @@ static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
 	u8 per = ppid & 0xFF;
 	u8 irq_mask = BIT(id);
 
+	dev_err_ratelimited(&pmic_arb->spmic->dev, "%s apid=%d sid=0x%x per=0x%x irq=%d\n",
+			__func__, apid, sid, per, id);
 	writel_relaxed(irq_mask, pmic_arb->ver_ops->irq_clear(pmic_arb, apid));
 
 	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
-- 
2.7.4