[PATCHv3,3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts

Commit Message

Grzegorz Jaszczyk July 2, 2020, 2:17 p.m. UTC

From: Suman Anna <[e-mail]>

The PRUSS INTC has a fixed number of output interrupt lines that are
connected to a number of processors or other PRUSS instances or other
devices (like DMA) on the SoC. The output interrupt lines 2 through 9
are usually connected to the main Arm host processor and are referred
to as host interrupts 0 through 7 from ARM/MPU perspective.

All of these 8 host interrupts are not always exclusively connected
to the Arm interrupt controller. Some SoCs have some interrupt lines
not connected to the Arm interrupt controller at all, while a few others
have the interrupt lines connected to multiple processors in which they
need to be partitioned as per SoC integration needs. For example, AM437x
and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
connected to the other PRUSS, while AM335x has host interrupt 0 shared
between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
a DMA controller.

Add support to the PRUSS INTC driver to allow both these shared and
invalid interrupts by not returning a failure if any of these interrupts
are skipped from the corresponding INTC DT node.

Signed-off-by: Suman Anna <[e-mail]>
Signed-off-by: Grzegorz Jaszczyk <[e-mail]>
---
v2->v3:
- Extra checks for (intc->irqs[i]) in error/remove path was moved from
  "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
  interrupts" to this patch
v1->v2:
- https://patchwork.kernel.org/patch/11069757/
---
 drivers/irqchip/irq-pruss-intc.c | 73 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 5 deletions(-)

Comments

Marc Zyngier July 2, 2020, 5:44 p.m. UTC | #1

On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
> From: Suman Anna <[e-mail]>
> 
> The PRUSS INTC has a fixed number of output interrupt lines that are
> connected to a number of processors or other PRUSS instances or other
> devices (like DMA) on the SoC. The output interrupt lines 2 through 9
> are usually connected to the main Arm host processor and are referred
> to as host interrupts 0 through 7 from ARM/MPU perspective.
> 
> All of these 8 host interrupts are not always exclusively connected
> to the Arm interrupt controller. Some SoCs have some interrupt lines
> not connected to the Arm interrupt controller at all, while a few 
> others
> have the interrupt lines connected to multiple processors in which they
> need to be partitioned as per SoC integration needs. For example, 
> AM437x
> and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 
> 5
> connected to the other PRUSS, while AM335x has host interrupt 0 shared
> between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU 
> and
> a DMA controller.
> 
> Add support to the PRUSS INTC driver to allow both these shared and
> invalid interrupts by not returning a failure if any of these 
> interrupts
> are skipped from the corresponding INTC DT node.

That's not exactly "adding support", is it? It really is "ignore these
interrupts because they are useless from the main CPU's perspective",
right?

> 
> Signed-off-by: Suman Anna <[e-mail]>
> Signed-off-by: Grzegorz Jaszczyk <[e-mail]>
> ---
> v2->v3:
> - Extra checks for (intc->irqs[i]) in error/remove path was moved from
>   "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
>   interrupts" to this patch
> v1->v2:
> - https://patchwork.kernel.org/patch/11069757/
> ---
>  drivers/irqchip/irq-pruss-intc.c | 73 
> +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-pruss-intc.c 
> b/drivers/irqchip/irq-pruss-intc.c
> index fb3dda3..49c936f 100644
> --- a/drivers/irqchip/irq-pruss-intc.c
> +++ b/drivers/irqchip/irq-pruss-intc.c
> @@ -65,11 +65,15 @@
>   * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
>   * @base: base virtual address of INTC register space
>   * @domain: irq domain for this interrupt controller
> + * @shared_intr: bit-map denoting if the MPU host interrupt is shared

nit: bitmap

> + * @invalid_intr: bit-map denoting if host interrupt is not connected 
> to MPU
>   */
>  struct pruss_intc {
>  	unsigned int irqs[MAX_NUM_HOST_IRQS];
>  	void __iomem *base;
>  	struct irq_domain *domain;
> +	u16 shared_intr;
> +	u16 invalid_intr;

Please represent bitmaps as an unsigned long.

>  };
> 
>  static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
> unsigned int reg)
> @@ -222,7 +226,8 @@ static int pruss_intc_probe(struct platform_device 
> *pdev)
>  		"host_intr4", "host_intr5", "host_intr6", "host_intr7", };
>  	struct device *dev = &pdev->dev;
>  	struct pruss_intc *intc;
> -	int i, irq;
> +	int i, irq, count;
> +	u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
> 
>  	intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
>  	if (!intc)
> @@ -235,6 +240,52 @@ static int pruss_intc_probe(struct platform_device 
> *pdev)
>  		return PTR_ERR(intc->base);
>  	}
> 
> +	count = of_property_read_variable_u8_array(dev->of_node,
> +						   "ti,irqs-reserved",
> +						   temp_intr, 0,
> +						   MAX_NUM_HOST_IRQS);
> +	/*
> +	 * The irqs-reserved is used only for some SoC's therefore not having
> +	 * this property is still valid
> +	 */
> +	if (count == -EINVAL)
> +		count = 0;
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
> +			dev_warn(dev, "ignoring invalid reserved irq %d\n",
> +				 temp_intr[i]);
> +			continue;
> +		}
> +
> +		intc->invalid_intr |= BIT(temp_intr[i]);
> +	}
> +
> +	count = of_property_read_variable_u8_array(dev->of_node,
> +						   "ti,irqs-shared",
> +						   temp_intr, 0,
> +						   MAX_NUM_HOST_IRQS);
> +	/*
> +	 * The irqs-shared is used only for some SoC's therefore not having
> +	 * this property is still valid
> +	 */
> +	if (count == -EINVAL)
> +		count = 0;
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
> +			dev_warn(dev, "ignoring invalid shared irq %d\n",
> +				 temp_intr[i]);
> +			continue;
> +		}
> +
> +		intc->shared_intr |= BIT(temp_intr[i]);
> +	}
> +

You probably want to move this in a separate function, since you 
populate a
common structure.

>  	pruss_intc_init(intc);
> 
>  	/* always 64 events */
> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct platform_device 
> *pdev)
>  		return -ENOMEM;
> 
>  	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
> +		if (intc->invalid_intr & BIT(i))
> +			continue;
> +
>  		irq = platform_get_irq_byname(pdev, irq_names[i]);
>  		if (irq <= 0) {
> +			if (intc->shared_intr & BIT(i))
> +				continue;

I don't really understand why you are treating these "shared" interrupts
differently from the invalid ones. In all cases, they shouldn't be used.

> +
>  			dev_err(dev, "platform_get_irq_byname failed for %s : %d\n",
>  				irq_names[i], irq);
>  			goto fail_irq;
> @@ -259,8 +316,11 @@ static int pruss_intc_probe(struct platform_device 
> *pdev)
>  	return 0;
> 
>  fail_irq:
> -	while (--i >= 0)
> -		irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
> +	while (--i >= 0) {
> +		if (intc->irqs[i])
> +			irq_set_chained_handler_and_data(intc->irqs[i], NULL,
> +							 NULL);
> +	}
> 
>  	irq_domain_remove(intc->domain);
> 
> @@ -273,8 +333,11 @@ static int pruss_intc_remove(struct 
> platform_device *pdev)
>  	unsigned int hwirq;
>  	int i;
> 
> -	for (i = 0; i < MAX_NUM_HOST_IRQS; i++)
> -		irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
> +	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
> +		if (intc->irqs[i])
> +			irq_set_chained_handler_and_data(intc->irqs[i], NULL,
> +							 NULL);
> +	}
> 
>  	for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)
>  		irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));

Thanks,

         M.

Suman Anna July 10, 2020, 8:59 p.m. UTC | #2

Hi Marc,

On 7/2/20 12:44 PM, Marc Zyngier wrote:
> On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
>> From: Suman Anna <[e-mail]>
>>
>> The PRUSS INTC has a fixed number of output interrupt lines that are
>> connected to a number of processors or other PRUSS instances or other
>> devices (like DMA) on the SoC. The output interrupt lines 2 through 9
>> are usually connected to the main Arm host processor and are referred
>> to as host interrupts 0 through 7 from ARM/MPU perspective.
>>
>> All of these 8 host interrupts are not always exclusively connected
>> to the Arm interrupt controller. Some SoCs have some interrupt lines
>> not connected to the Arm interrupt controller at all, while a few others
>> have the interrupt lines connected to multiple processors in which they
>> need to be partitioned as per SoC integration needs. For example, AM437x
>> and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
>> connected to the other PRUSS, while AM335x has host interrupt 0 shared
>> between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
>> a DMA controller.
>>
>> Add support to the PRUSS INTC driver to allow both these shared and
>> invalid interrupts by not returning a failure if any of these interrupts
>> are skipped from the corresponding INTC DT node.
> 
> That's not exactly "adding support", is it? It really is "ignore these
> interrupts because they are useless from the main CPU's perspective",
> right?

Correct. We can rephrase this to something like
"Add logic to the PRUSS INTC driver to ignore.."

> 
>>
>> Signed-off-by: Suman Anna <[e-mail]>
>> Signed-off-by: Grzegorz Jaszczyk <[e-mail]>
>> ---
>> v2->v3:
>> - Extra checks for (intc->irqs[i]) in error/remove path was moved from
>>   "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
>>   interrupts" to this patch
>> v1->v2:
>> - https://patchwork.kernel.org/patch/11069757/
>> ---
>>  drivers/irqchip/irq-pruss-intc.c | 73 
>> +++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 68 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-pruss-intc.c 
>> b/drivers/irqchip/irq-pruss-intc.c
>> index fb3dda3..49c936f 100644
>> --- a/drivers/irqchip/irq-pruss-intc.c
>> +++ b/drivers/irqchip/irq-pruss-intc.c
>> @@ -65,11 +65,15 @@
>>   * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
>>   * @base: base virtual address of INTC register space
>>   * @domain: irq domain for this interrupt controller
>> + * @shared_intr: bit-map denoting if the MPU host interrupt is shared
> 
> nit: bitmap

ok

> 
>> + * @invalid_intr: bit-map denoting if host interrupt is not connected 
>> to MPU
>>   */
>>  struct pruss_intc {
>>      unsigned int irqs[MAX_NUM_HOST_IRQS];
>>      void __iomem *base;
>>      struct irq_domain *domain;
>> +    u16 shared_intr;
>> +    u16 invalid_intr;
> 
> Please represent bitmaps as an unsigned long.

ok. We have atmost 8 interrupts coming in, but agree on the change since 
we are using the BIT() macro below.

> 
>>  };
>>
>>  static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
>> unsigned int reg)
>> @@ -222,7 +226,8 @@ static int pruss_intc_probe(struct platform_device 
>> *pdev)
>>          "host_intr4", "host_intr5", "host_intr6", "host_intr7", };
>>      struct device *dev = &pdev->dev;
>>      struct pruss_intc *intc;
>> -    int i, irq;
>> +    int i, irq, count;
>> +    u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
>>
>>      intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
>>      if (!intc)
>> @@ -235,6 +240,52 @@ static int pruss_intc_probe(struct 
>> platform_device *pdev)
>>          return PTR_ERR(intc->base);
>>      }
>>
>> +    count = of_property_read_variable_u8_array(dev->of_node,
>> +                           "ti,irqs-reserved",
>> +                           temp_intr, 0,
>> +                           MAX_NUM_HOST_IRQS);
>> +    /*
>> +     * The irqs-reserved is used only for some SoC's therefore not 
>> having
>> +     * this property is still valid
>> +     */
>> +    if (count == -EINVAL)
>> +        count = 0;
>> +    if (count < 0)
>> +        return count;
>> +
>> +    for (i = 0; i < count; i++) {
>> +        if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
>> +            dev_warn(dev, "ignoring invalid reserved irq %d\n",
>> +                 temp_intr[i]);
>> +            continue;
>> +        }
>> +
>> +        intc->invalid_intr |= BIT(temp_intr[i]);
>> +    }
>> +
>> +    count = of_property_read_variable_u8_array(dev->of_node,
>> +                           "ti,irqs-shared",
>> +                           temp_intr, 0,
>> +                           MAX_NUM_HOST_IRQS);
>> +    /*
>> +     * The irqs-shared is used only for some SoC's therefore not having
>> +     * this property is still valid
>> +     */
>> +    if (count == -EINVAL)
>> +        count = 0;
>> +    if (count < 0)
>> +        return count;
>> +
>> +    for (i = 0; i < count; i++) {
>> +        if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
>> +            dev_warn(dev, "ignoring invalid shared irq %d\n",
>> +                 temp_intr[i]);
>> +            continue;
>> +        }
>> +
>> +        intc->shared_intr |= BIT(temp_intr[i]);
>> +    }
>> +
> 
> You probably want to move this in a separate function, since you populate a
> common structure.
> 
>>      pruss_intc_init(intc);
>>
>>      /* always 64 events */
>> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct 
>> platform_device *pdev)
>>          return -ENOMEM;
>>
>>      for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>> +        if (intc->invalid_intr & BIT(i))
>> +            continue;
>> +
>>          irq = platform_get_irq_byname(pdev, irq_names[i]);
>>          if (irq <= 0) {
>> +            if (intc->shared_intr & BIT(i))
>> +                continue;
> 
> I don't really understand why you are treating these "shared" interrupts
> differently from the invalid ones. In all cases, they shouldn't be used.

The behavior is the same in how we handle it, but the difference is that 
an "invalid" one is never even connected to the ARM interrupt 
controller, while the "shared" one is a choice. So, unless this 
interrupt is being used/handled by a different processor/entity, you 
would not see this skipped from the dts node.

regards
Suman


> 
>> +
>>              dev_err(dev, "platform_get_irq_byname failed for %s : %d\n",
>>                  irq_names[i], irq);
>>              goto fail_irq;
>> @@ -259,8 +316,11 @@ static int pruss_intc_probe(struct 
>> platform_device *pdev)
>>      return 0;
>>
>>  fail_irq:
>> -    while (--i >= 0)
>> -        irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
>> +    while (--i >= 0) {
>> +        if (intc->irqs[i])
>> +            irq_set_chained_handler_and_data(intc->irqs[i], NULL,
>> +                             NULL);
>> +    }
>>
>>      irq_domain_remove(intc->domain);
>>
>> @@ -273,8 +333,11 @@ static int pruss_intc_remove(struct 
>> platform_device *pdev)
>>      unsigned int hwirq;
>>      int i;
>>
>> -    for (i = 0; i < MAX_NUM_HOST_IRQS; i++)
>> -        irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
>> +    for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>> +        if (intc->irqs[i])
>> +            irq_set_chained_handler_and_data(intc->irqs[i], NULL,
>> +                             NULL);
>> +    }
>>
>>      for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)
>>          irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));
> 
> Thanks,
> 
>          M.

Marc Zyngier July 17, 2020, 11:02 a.m. UTC | #3

On Fri, 10 Jul 2020 21:59:17 +0100,
Suman Anna <[e-mail]> wrote:

Hi Suman,

[...]

>
> Hi Marc,
> 
> On 7/2/20 12:44 PM, Marc Zyngier wrote:
> > On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
> >> From: Suman Anna <[e-mail]>
> >> 
> >> The PRUSS INTC has a fixed number of output interrupt lines that are
> >> connected to a number of processors or other PRUSS instances or other
> >> devices (like DMA) on the SoC. The output interrupt lines 2 through 9
> >> are usually connected to the main Arm host processor and are referred
> >> to as host interrupts 0 through 7 from ARM/MPU perspective.
> >> 
> >> All of these 8 host interrupts are not always exclusively connected
> >> to the Arm interrupt controller. Some SoCs have some interrupt lines
> >> not connected to the Arm interrupt controller at all, while a few others
> >> have the interrupt lines connected to multiple processors in which they
> >> need to be partitioned as per SoC integration needs. For example, AM437x
> >> and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
> >> connected to the other PRUSS, while AM335x has host interrupt 0 shared
> >> between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
> >> a DMA controller.
> >> 
> >> Add support to the PRUSS INTC driver to allow both these shared and
> >> invalid interrupts by not returning a failure if any of these interrupts
> >> are skipped from the corresponding INTC DT node.
> > 
> > That's not exactly "adding support", is it? It really is "ignore these
> > interrupts because they are useless from the main CPU's perspective",
> > right?
> 
> Correct. We can rephrase this to something like
> "Add logic to the PRUSS INTC driver to ignore.."
> 
> > 
> >> 
> >> Signed-off-by: Suman Anna <[e-mail]>
> >> Signed-off-by: Grzegorz Jaszczyk <[e-mail]>
> >> ---
> >> v2->v3:
> >> - Extra checks for (intc->irqs[i]) in error/remove path was moved from
> >>   "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
> >>   interrupts" to this patch
> >> v1->v2:
> >> - https://patchwork.kernel.org/patch/11069757/
> >> ---
> >>  drivers/irqchip/irq-pruss-intc.c | 73
> >> +++++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 68 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/irqchip/irq-pruss-intc.c
> >> b/drivers/irqchip/irq-pruss-intc.c
> >> index fb3dda3..49c936f 100644
> >> --- a/drivers/irqchip/irq-pruss-intc.c
> >> +++ b/drivers/irqchip/irq-pruss-intc.c
> >> @@ -65,11 +65,15 @@
> >>   * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
> >>   * @base: base virtual address of INTC register space
> >>   * @domain: irq domain for this interrupt controller
> >> + * @shared_intr: bit-map denoting if the MPU host interrupt is shared
> > 
> > nit: bitmap
> 
> ok
> 
> > 
> >> + * @invalid_intr: bit-map denoting if host interrupt is not
> >> connected to MPU
> >>   */
> >>  struct pruss_intc {
> >>      unsigned int irqs[MAX_NUM_HOST_IRQS];
> >>      void __iomem *base;
> >>      struct irq_domain *domain;
> >> +    u16 shared_intr;
> >> +    u16 invalid_intr;
> > 
> > Please represent bitmaps as an unsigned long.
> 
> ok. We have atmost 8 interrupts coming in, but agree on the change
> since we are using the BIT() macro below.
> 
> > 
> >>  };
> >> 
> >>  static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
> >> unsigned int reg)
> >> @@ -222,7 +226,8 @@ static int pruss_intc_probe(struct
> >> platform_device *pdev)
> >>          "host_intr4", "host_intr5", "host_intr6", "host_intr7", };
> >>      struct device *dev = &pdev->dev;
> >>      struct pruss_intc *intc;
> >> -    int i, irq;
> >> +    int i, irq, count;
> >> +    u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
> >> 
> >>      intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
> >>      if (!intc)
> >> @@ -235,6 +240,52 @@ static int pruss_intc_probe(struct
> >> platform_device *pdev)
> >>          return PTR_ERR(intc->base);
> >>      }
> >> 
> >> +    count = of_property_read_variable_u8_array(dev->of_node,
> >> +                           "ti,irqs-reserved",
> >> +                           temp_intr, 0,
> >> +                           MAX_NUM_HOST_IRQS);
> >> +    /*
> >> +     * The irqs-reserved is used only for some SoC's therefore not
> >> having
> >> +     * this property is still valid
> >> +     */
> >> +    if (count == -EINVAL)
> >> +        count = 0;
> >> +    if (count < 0)
> >> +        return count;
> >> +
> >> +    for (i = 0; i < count; i++) {
> >> +        if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
> >> +            dev_warn(dev, "ignoring invalid reserved irq %d\n",
> >> +                 temp_intr[i]);
> >> +            continue;
> >> +        }
> >> +
> >> +        intc->invalid_intr |= BIT(temp_intr[i]);
> >> +    }
> >> +
> >> +    count = of_property_read_variable_u8_array(dev->of_node,
> >> +                           "ti,irqs-shared",
> >> +                           temp_intr, 0,
> >> +                           MAX_NUM_HOST_IRQS);
> >> +    /*
> >> +     * The irqs-shared is used only for some SoC's therefore not having
> >> +     * this property is still valid
> >> +     */
> >> +    if (count == -EINVAL)
> >> +        count = 0;
> >> +    if (count < 0)
> >> +        return count;
> >> +
> >> +    for (i = 0; i < count; i++) {
> >> +        if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
> >> +            dev_warn(dev, "ignoring invalid shared irq %d\n",
> >> +                 temp_intr[i]);
> >> +            continue;
> >> +        }
> >> +
> >> +        intc->shared_intr |= BIT(temp_intr[i]);
> >> +    }
> >> +
> > 
> > You probably want to move this in a separate function, since you populate a
> > common structure.
> > 
> >>      pruss_intc_init(intc);
> >> 
> >>      /* always 64 events */
> >> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
> >> platform_device *pdev)
> >>          return -ENOMEM;
> >> 
> >>      for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
> >> +        if (intc->invalid_intr & BIT(i))
> >> +            continue;
> >> +
> >>          irq = platform_get_irq_byname(pdev, irq_names[i]);
> >>          if (irq <= 0) {
> >> +            if (intc->shared_intr & BIT(i))
> >> +                continue;
> > 
> > I don't really understand why you are treating these "shared" interrupts
> > differently from the invalid ones. In all cases, they shouldn't be used.
> 
> The behavior is the same in how we handle it, but the difference is
> that an "invalid" one is never even connected to the ARM interrupt
> controller, while the "shared" one is a choice. So, unless this
> interrupt is being used/handled by a different processor/entity, you
> would not see this skipped from the dts node.

And I'm saying that all that matters is that you are discarding these
interrupts. Whether they are flagged invalid or shared, they are not
available to Linux. So the difference in handling is pointless and
only makes it harder to understand what you are doing.

Thanks,

	M.

Suman Anna July 25, 2020, 3:57 p.m. UTC | #4

Hi Marc,

On 7/17/20 6:02 AM, Marc Zyngier wrote:
> On Fri, 10 Jul 2020 21:59:17 +0100,
> Suman Anna <[e-mail]> wrote:
> 
> Hi Suman,
> 
> [...]
> 
>>
>> Hi Marc,
>>
>> On 7/2/20 12:44 PM, Marc Zyngier wrote:
>>> On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
>>>> From: Suman Anna <[e-mail]>
>>>>
>>>> The PRUSS INTC has a fixed number of output interrupt lines that are
>>>> connected to a number of processors or other PRUSS instances or other
>>>> devices (like DMA) on the SoC. The output interrupt lines 2 through 9
>>>> are usually connected to the main Arm host processor and are referred
>>>> to as host interrupts 0 through 7 from ARM/MPU perspective.
>>>>
>>>> All of these 8 host interrupts are not always exclusively connected
>>>> to the Arm interrupt controller. Some SoCs have some interrupt lines
>>>> not connected to the Arm interrupt controller at all, while a few others
>>>> have the interrupt lines connected to multiple processors in which they
>>>> need to be partitioned as per SoC integration needs. For example, AM437x
>>>> and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
>>>> connected to the other PRUSS, while AM335x has host interrupt 0 shared
>>>> between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
>>>> a DMA controller.
>>>>
>>>> Add support to the PRUSS INTC driver to allow both these shared and
>>>> invalid interrupts by not returning a failure if any of these interrupts
>>>> are skipped from the corresponding INTC DT node.
>>>
>>> That's not exactly "adding support", is it? It really is "ignore these
>>> interrupts because they are useless from the main CPU's perspective",
>>> right?
>>
>> Correct. We can rephrase this to something like
>> "Add logic to the PRUSS INTC driver to ignore.."
>>
>>>
>>>>
>>>> Signed-off-by: Suman Anna <[e-mail]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[e-mail]>
>>>> ---
>>>> v2->v3:
>>>> - Extra checks for (intc->irqs[i]) in error/remove path was moved from
>>>>    "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
>>>>    interrupts" to this patch
>>>> v1->v2:
>>>> - https://patchwork.kernel.org/patch/11069757/
>>>> ---
>>>>   drivers/irqchip/irq-pruss-intc.c | 73
>>>> +++++++++++++++++++++++++++++++++++++---
>>>>   1 file changed, 68 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-pruss-intc.c
>>>> b/drivers/irqchip/irq-pruss-intc.c
>>>> index fb3dda3..49c936f 100644
>>>> --- a/drivers/irqchip/irq-pruss-intc.c
>>>> +++ b/drivers/irqchip/irq-pruss-intc.c
>>>> @@ -65,11 +65,15 @@
>>>>    * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
>>>>    * @base: base virtual address of INTC register space
>>>>    * @domain: irq domain for this interrupt controller
>>>> + * @shared_intr: bit-map denoting if the MPU host interrupt is shared
>>>
>>> nit: bitmap
>>
>> ok
>>
>>>
>>>> + * @invalid_intr: bit-map denoting if host interrupt is not
>>>> connected to MPU
>>>>    */
>>>>   struct pruss_intc {
>>>>       unsigned int irqs[MAX_NUM_HOST_IRQS];
>>>>       void __iomem *base;
>>>>       struct irq_domain *domain;
>>>> +    u16 shared_intr;
>>>> +    u16 invalid_intr;
>>>
>>> Please represent bitmaps as an unsigned long.
>>
>> ok. We have atmost 8 interrupts coming in, but agree on the change
>> since we are using the BIT() macro below.
>>
>>>
>>>>   };
>>>>
>>>>   static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
>>>> unsigned int reg)
>>>> @@ -222,7 +226,8 @@ static int pruss_intc_probe(struct
>>>> platform_device *pdev)
>>>>           "host_intr4", "host_intr5", "host_intr6", "host_intr7", };
>>>>       struct device *dev = &pdev->dev;
>>>>       struct pruss_intc *intc;
>>>> -    int i, irq;
>>>> +    int i, irq, count;
>>>> +    u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
>>>>
>>>>       intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
>>>>       if (!intc)
>>>> @@ -235,6 +240,52 @@ static int pruss_intc_probe(struct
>>>> platform_device *pdev)
>>>>           return PTR_ERR(intc->base);
>>>>       }
>>>>
>>>> +    count = of_property_read_variable_u8_array(dev->of_node,
>>>> +                           "ti,irqs-reserved",
>>>> +                           temp_intr, 0,
>>>> +                           MAX_NUM_HOST_IRQS);
>>>> +    /*
>>>> +     * The irqs-reserved is used only for some SoC's therefore not
>>>> having
>>>> +     * this property is still valid
>>>> +     */
>>>> +    if (count == -EINVAL)
>>>> +        count = 0;
>>>> +    if (count < 0)
>>>> +        return count;
>>>> +
>>>> +    for (i = 0; i < count; i++) {
>>>> +        if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
>>>> +            dev_warn(dev, "ignoring invalid reserved irq %d\n",
>>>> +                 temp_intr[i]);
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        intc->invalid_intr |= BIT(temp_intr[i]);
>>>> +    }
>>>> +
>>>> +    count = of_property_read_variable_u8_array(dev->of_node,
>>>> +                           "ti,irqs-shared",
>>>> +                           temp_intr, 0,
>>>> +                           MAX_NUM_HOST_IRQS);
>>>> +    /*
>>>> +     * The irqs-shared is used only for some SoC's therefore not having
>>>> +     * this property is still valid
>>>> +     */
>>>> +    if (count == -EINVAL)
>>>> +        count = 0;
>>>> +    if (count < 0)
>>>> +        return count;
>>>> +
>>>> +    for (i = 0; i < count; i++) {
>>>> +        if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
>>>> +            dev_warn(dev, "ignoring invalid shared irq %d\n",
>>>> +                 temp_intr[i]);
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        intc->shared_intr |= BIT(temp_intr[i]);
>>>> +    }
>>>> +
>>>
>>> You probably want to move this in a separate function, since you populate a
>>> common structure.
>>>
>>>>       pruss_intc_init(intc);
>>>>
>>>>       /* always 64 events */
>>>> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
>>>> platform_device *pdev)
>>>>           return -ENOMEM;
>>>>
>>>>       for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>>>> +        if (intc->invalid_intr & BIT(i))
>>>> +            continue;
>>>> +
>>>>           irq = platform_get_irq_byname(pdev, irq_names[i]);
>>>>           if (irq <= 0) {
>>>> +            if (intc->shared_intr & BIT(i))
>>>> +                continue;
>>>
>>> I don't really understand why you are treating these "shared" interrupts
>>> differently from the invalid ones. In all cases, they shouldn't be used.
>>
>> The behavior is the same in how we handle it, but the difference is
>> that an "invalid" one is never even connected to the ARM interrupt
>> controller, while the "shared" one is a choice. So, unless this
>> interrupt is being used/handled by a different processor/entity, you
>> would not see this skipped from the dts node.
> 
> And I'm saying that all that matters is that you are discarding these
> interrupts. Whether they are flagged invalid or shared, they are not
> available to Linux. So the difference in handling is pointless and
> only makes it harder to understand what you are doing.

The primary reason for using two properties and this logic was to 
accurately describe the h/w and usage of these in the DT bindings to 
distinguish the "never connected" vs the "optionally can be skipped" 
interrupts rather than go by how these are handled in the driver. I feel 
we will loose this description and make it confusing for SoC product 
integration developers.

Greg is planning to consolidate these for the next version. It would 
have been nice if we could have retained them.

regards
Suman

Marc Zyngier July 25, 2020, 4:27 p.m. UTC | #5

On 2020-07-25 16:57, Suman Anna wrote:

Suman,

> Hi Marc,

[...]

>>>>> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
>>>>> platform_device *pdev)
>>>>>           return -ENOMEM;
>>>>> 
>>>>>       for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>>>>> +        if (intc->invalid_intr & BIT(i))
>>>>> +            continue;
>>>>> +
>>>>>           irq = platform_get_irq_byname(pdev, irq_names[i]);
>>>>>           if (irq <= 0) {
>>>>> +            if (intc->shared_intr & BIT(i))
>>>>> +                continue;
>>>> 
>>>> I don't really understand why you are treating these "shared" 
>>>> interrupts
>>>> differently from the invalid ones. In all cases, they shouldn't be 
>>>> used.
>>> 
>>> The behavior is the same in how we handle it, but the difference is
>>> that an "invalid" one is never even connected to the ARM interrupt
>>> controller, while the "shared" one is a choice. So, unless this
>>> interrupt is being used/handled by a different processor/entity, you
>>> would not see this skipped from the dts node.
>> 
>> And I'm saying that all that matters is that you are discarding these
>> interrupts. Whether they are flagged invalid or shared, they are not
>> available to Linux. So the difference in handling is pointless and
>> only makes it harder to understand what you are doing.
> 
> The primary reason for using two properties and this logic was to
> accurately describe the h/w and usage of these in the DT bindings to
> distinguish the "never connected" vs the "optionally can be skipped"
> interrupts rather than go by how these are handled in the driver. I
> feel we will loose this description and make it confusing for SoC
> product integration developers.

This logic makes zero difference to Linux, and I do not see what
you gain by having two code paths with separate list of unusable
interrupts. And why on Earth would a "Soc product integration
developer" have any business to mess with this driver code?
They should very much stay away from it and deal with their
precious value add.

If you want two properties or even twenty, go for it, and have fun.
Just don't make this driver even more unreadable than it already is.
Merge all these interrupts in *one* list of unusable interrupts,
and be done with it.

Thanks,

         M.

Suman Anna July 25, 2020, 4:39 p.m. UTC | #6

Hi Marc,

On 7/25/20 11:27 AM, Marc Zyngier wrote:
> On 2020-07-25 16:57, Suman Anna wrote:
> 
> Suman,
> 
>> Hi Marc,
> 
> [...]
> 
>>>>>> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
>>>>>> platform_device *pdev)
>>>>>>           return -ENOMEM;
>>>>>>
>>>>>>       for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>>>>>> +        if (intc->invalid_intr & BIT(i))
>>>>>> +            continue;
>>>>>> +
>>>>>>           irq = platform_get_irq_byname(pdev, irq_names[i]);
>>>>>>           if (irq <= 0) {
>>>>>> +            if (intc->shared_intr & BIT(i))
>>>>>> +                continue;
>>>>>
>>>>> I don't really understand why you are treating these "shared" 
>>>>> interrupts
>>>>> differently from the invalid ones. In all cases, they shouldn't be 
>>>>> used.
>>>>
>>>> The behavior is the same in how we handle it, but the difference is
>>>> that an "invalid" one is never even connected to the ARM interrupt
>>>> controller, while the "shared" one is a choice. So, unless this
>>>> interrupt is being used/handled by a different processor/entity, you
>>>> would not see this skipped from the dts node.
>>>
>>> And I'm saying that all that matters is that you are discarding these
>>> interrupts. Whether they are flagged invalid or shared, they are not
>>> available to Linux. So the difference in handling is pointless and
>>> only makes it harder to understand what you are doing.
>>
>> The primary reason for using two properties and this logic was to
>> accurately describe the h/w and usage of these in the DT bindings to
>> distinguish the "never connected" vs the "optionally can be skipped"
>> interrupts rather than go by how these are handled in the driver. I
>> feel we will loose this description and make it confusing for SoC
>> product integration developers.
> 
> This logic makes zero difference to Linux, and I do not see what
> you gain by having two code paths with separate list of unusable
> interrupts. 

OK, I understand your stance on this.

And why on Earth would a "Soc product integration
> developer" have any business to mess with this driver code?
> They should very much stay away from it and deal with their
> precious value add.

It really depends on how they put together the system and exercise the 
PRUs and the number of processors interacting with them. We have had 
customers put together usecases where both the ARM core running Linux 
and a remote processor like an M4 or R5 talk to the PRU at the same 
time, or even inter PRUSS instances. They would have to adjust the DT in 
their board dts files in general.

> 
> If you want two properties or even twenty, go for it, and have fun.
> Just don't make this driver even more unreadable than it already is.
> Merge all these interrupts in *one* list of unusable interrupts,
> and be done with it.

Yes, we are merging this for the next version.

regards
Suman

Patch

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index fb3dda3..49c936f 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -65,11 +65,15 @@ 
  * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
  * @base: base virtual address of INTC register space
  * @domain: irq domain for this interrupt controller
+ * @shared_intr: bit-map denoting if the MPU host interrupt is shared
+ * @invalid_intr: bit-map denoting if host interrupt is not connected to MPU
  */
 struct pruss_intc {
 	unsigned int irqs[MAX_NUM_HOST_IRQS];
 	void __iomem *base;
 	struct irq_domain *domain;
+	u16 shared_intr;
+	u16 invalid_intr;
 };
 
 static inline u32 pruss_intc_read_reg(struct pruss_intc *intc, unsigned int reg)
@@ -222,7 +226,8 @@  static int pruss_intc_probe(struct platform_device *pdev)
 		"host_intr4", "host_intr5", "host_intr6", "host_intr7", };
 	struct device *dev = &pdev->dev;
 	struct pruss_intc *intc;
-	int i, irq;
+	int i, irq, count;
+	u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
 
 	intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
 	if (!intc)
@@ -235,6 +240,52 @@  static int pruss_intc_probe(struct platform_device *pdev)
 		return PTR_ERR(intc->base);
 	}
 
+	count = of_property_read_variable_u8_array(dev->of_node,
+						   "ti,irqs-reserved",
+						   temp_intr, 0,
+						   MAX_NUM_HOST_IRQS);
+	/*
+	 * The irqs-reserved is used only for some SoC's therefore not having
+	 * this property is still valid
+	 */
+	if (count == -EINVAL)
+		count = 0;
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+			dev_warn(dev, "ignoring invalid reserved irq %d\n",
+				 temp_intr[i]);
+			continue;
+		}
+
+		intc->invalid_intr |= BIT(temp_intr[i]);
+	}
+
+	count = of_property_read_variable_u8_array(dev->of_node,
+						   "ti,irqs-shared",
+						   temp_intr, 0,
+						   MAX_NUM_HOST_IRQS);
+	/*
+	 * The irqs-shared is used only for some SoC's therefore not having
+	 * this property is still valid
+	 */
+	if (count == -EINVAL)
+		count = 0;
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+			dev_warn(dev, "ignoring invalid shared irq %d\n",
+				 temp_intr[i]);
+			continue;
+		}
+
+		intc->shared_intr |= BIT(temp_intr[i]);
+	}
+
 	pruss_intc_init(intc);
 
 	/* always 64 events */
@@ -244,8 +295,14 @@  static int pruss_intc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+		if (intc->invalid_intr & BIT(i))
+			continue;
+
 		irq = platform_get_irq_byname(pdev, irq_names[i]);
 		if (irq <= 0) {
+			if (intc->shared_intr & BIT(i))
+				continue;
+
 			dev_err(dev, "platform_get_irq_byname failed for %s : %d\n",
 				irq_names[i], irq);
 			goto fail_irq;
@@ -259,8 +316,11 @@  static int pruss_intc_probe(struct platform_device *pdev)
 	return 0;
 
 fail_irq:
-	while (--i >= 0)
-		irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
+	while (--i >= 0) {
+		if (intc->irqs[i])
+			irq_set_chained_handler_and_data(intc->irqs[i], NULL,
+							 NULL);
+	}
 
 	irq_domain_remove(intc->domain);
 
@@ -273,8 +333,11 @@  static int pruss_intc_remove(struct platform_device *pdev)
 	unsigned int hwirq;
 	int i;
 
-	for (i = 0; i < MAX_NUM_HOST_IRQS; i++)
-		irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
+	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+		if (intc->irqs[i])
+			irq_set_chained_handler_and_data(intc->irqs[i], NULL,
+							 NULL);
+	}
 
 	for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)
 		irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));