[PATCH 0/4] add support for MBIGEN generating message based SPIs

[PATCH 0/4] add support for MBIGEN generating message based SPIs

[Yang Yingliang]

Now MBIGENs have pins that used to generate SPIs,
with
5052875 ("irqchip/gic-v3: Add support for Message Based Interrupts as an MSI controller"),
we can support MBIGEN to generate message based SPIs by writing
GICD_SETSPIR. This patchset add support for MBIGEN generating
message based SPIs and a bugfix for MBI driver.


Patch #1 is a bugfix for MBI driver.
Pathc #2 ~ #4 is support for MBIGEN generating message based SPIs.

Yang Yingliang (4):
  irqchip/gic-v3-mbi: fix uninitialized mbi_lock
  irqchip/mbigen: rename register marcros
  irqchip/mbigen: add support for a MBIGEN generating SPIs
  dt-bindings/irqchip/mbigen: add example of MBIGEN generate SPIs

 .../interrupt-controller/hisilicon,mbigen-v2.txt   | 17 +++++++++-
 drivers/irqchip/irq-gic-v3-mbi.c                   |  2 +-
 drivers/irqchip/irq-mbigen.c                       | 39 ++++++++++++++++------
 3 files changed, 45 insertions(+), 13 deletions(-)

-- 
1.8.3

[PATCH 1/4] irqchip/gic-v3-mbi: fix uninitialized mbi_lock

[Yang Yingliang]

mbi_lock is uninitialized, use marco DEFINE_MUTEX
to initialize it.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/irqchip/irq-gic-v3-mbi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-mbi.c b/drivers/irqchip/irq-gic-v3-mbi.c
index ad70e7c..fbfa7ff 100644
--- a/drivers/irqchip/irq-gic-v3-mbi.c
+++ b/drivers/irqchip/irq-gic-v3-mbi.c
@@ -24,7 +24,7 @@ struct mbi_range {
 	unsigned long		*bm;
 };
 
-static struct mutex		mbi_lock;
+static DEFINE_MUTEX(mbi_lock);
 static phys_addr_t		mbi_phys_base;
 static struct mbi_range		*mbi_ranges;
 static unsigned int		mbi_range_nr;
-- 
1.8.3

[PATCH 2/4] irqchip/mbigen: rename register marcros

[Yang Yingliang]

A MBIGEN can also be used for generating SPIs, so let's
rename register macros to make them more resonable.

The first 64-pins of MBIGEN is used by SPIs, so rename
RESERVED_IRQ_PER_MBIGEN_CHIP to SPI_NUM_PER_MBIGEN_CHIP
and change the comment for this marcro.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/irqchip/irq-mbigen.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 567b29c..f05998f 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -30,14 +30,14 @@
 /* Interrupt numbers per mbigen node supported */
 #define IRQS_PER_MBIGEN_NODE		128
 
-/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */
-#define RESERVED_IRQ_PER_MBIGEN_CHIP	64
+/* 64 irqs (Pin0-pin63) are used for SPIs on each mbigen chip */
+#define SPI_NUM_PER_MBIGEN_CHIP	64
 
 /* The maximum IRQ pin number of mbigen chip(start from 0) */
 #define MAXIMUM_IRQ_PIN_NUM		1407
 
 /**
- * In mbigen vector register
+ * In mbigen lpi vector register
  * bit[21:12]:	event id value
  * bit[11:0]:	device id
  */
@@ -48,7 +48,7 @@
 #define MBIGEN_NODE_OFFSET		0x1000
 
 /* offset of vector register in mbigen node */
-#define REG_MBIGEN_VEC_OFFSET		0x200
+#define REG_MBIGEN_LPI_VEC_OFFSET	0x200
 
 /**
  * offset of clear register in mbigen node
@@ -62,7 +62,7 @@
  * This register is used to configure interrupt
  * trigger type
  */
-#define REG_MBIGEN_TYPE_OFFSET		0x0
+#define REG_MBIGEN_LPI_TYPE_OFFSET	0x0
 
 /**
  * struct mbigen_device - holds the information of mbigen device.
@@ -79,12 +79,12 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
 {
 	unsigned int nid, pin;
 
-	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
+	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
 	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
 	pin = hwirq % IRQS_PER_MBIGEN_NODE;
 
 	return pin * 4 + nid * MBIGEN_NODE_OFFSET
-			+ REG_MBIGEN_VEC_OFFSET;
+			+ REG_MBIGEN_LPI_VEC_OFFSET;
 }
 
 static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
@@ -92,7 +92,7 @@ static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
 {
 	unsigned int nid, irq_ofst, ofst;
 
-	hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP;
+	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
 	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
 	irq_ofst = hwirq % IRQS_PER_MBIGEN_NODE;
 
@@ -100,7 +100,7 @@ static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
 	ofst = irq_ofst / 32 * 4;
 
 	*addr = ofst + nid * MBIGEN_NODE_OFFSET
-		+ REG_MBIGEN_TYPE_OFFSET;
+		+ REG_MBIGEN_LPI_TYPE_OFFSET;
 }
 
 static inline void get_mbigen_clear_reg(irq_hw_number_t hwirq,
@@ -183,7 +183,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
-			(fwspec->param[0] < RESERVED_IRQ_PER_MBIGEN_CHIP))
+			(fwspec->param[0] < SPI_NUM_PER_MBIGEN_CHIP))
 			return -EINVAL;
 		else
 			*hwirq = fwspec->param[0];
-- 
1.8.3

[PATCH 3/4] irqchip/mbigen: add support for a MBIGEN generating SPIs

[Yang Yingliang]

Now with
5052875 ("irqchip/gic-v3: Add support for Message Based Interrupts as an MSI controller"),
we can support MBIGEN to generate message based SPIs by writing GICD_SETSPIR.

The first 64-pins of each MBIGEN chip is used to generate SPIs, and each
MBIGEN chip has several MBIGEN nodes, every node has 128 pins for generating
LPIs. The total pins are: 64(SPIs) + 128 * node_nr(LPIs). So we can translate
the pin index in a unified way in mbigen_domain_translate().

Also Add TYPE and VEC registers that used by generating SPIs, the driver can
access them when MBIGEN is used to generate SPIs.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/irqchip/irq-mbigen.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index f05998f..37c1932 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -48,6 +48,7 @@
 #define MBIGEN_NODE_OFFSET		0x1000
 
 /* offset of vector register in mbigen node */
+#define REG_MBIGEN_SPI_VEC_OFFSET	0x500
 #define REG_MBIGEN_LPI_VEC_OFFSET	0x200
 
 /**
@@ -62,6 +63,7 @@
  * This register is used to configure interrupt
  * trigger type
  */
+#define REG_MBIGEN_SPI_TYPE_OFFSET	0x400
 #define REG_MBIGEN_LPI_TYPE_OFFSET	0x0
 
 /**
@@ -79,6 +81,9 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
 {
 	unsigned int nid, pin;
 
+	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP)
+		return (hwirq * 4 + REG_MBIGEN_SPI_VEC_OFFSET);
+
 	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
 	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
 	pin = hwirq % IRQS_PER_MBIGEN_NODE;
@@ -92,6 +97,13 @@ static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
 {
 	unsigned int nid, irq_ofst, ofst;
 
+	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
+		*mask = 1 << (hwirq % 32);
+		ofst = hwirq / 32 * 4;
+		*addr = ofst + REG_MBIGEN_SPI_TYPE_OFFSET;
+		return;
+	}
+
 	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
 	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
 	irq_ofst = hwirq % IRQS_PER_MBIGEN_NODE;
@@ -162,6 +174,12 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	u32 val;
 
 	base += get_mbigen_vec_reg(d->hwirq);
+
+	if (d->hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
+		writel_relaxed(msg->data, base);
+		return;
+	}
+
 	val = readl_relaxed(base);
 
 	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
@@ -182,8 +200,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
 		if (fwspec->param_count != 2)
 			return -EINVAL;
 
-		if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
-			(fwspec->param[0] < SPI_NUM_PER_MBIGEN_CHIP))
+		if (fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM)
 			return -EINVAL;
 		else
 			*hwirq = fwspec->param[0];
-- 
1.8.3

Re: [PATCH 3/4] irqchip/mbigen: add support for a MBIGEN generating SPIs

[Marc Zyngier]

On 16/10/18 10:15, Yang Yingliang wrote:
> Now with
> 5052875 ("irqchip/gic-v3: Add support for Message Based Interrupts as an MSI controller"),
> we can support MBIGEN to generate message based SPIs by writing GICD_SETSPIR.
> 
> The first 64-pins of each MBIGEN chip is used to generate SPIs, and each
> MBIGEN chip has several MBIGEN nodes, every node has 128 pins for generating
> LPIs. The total pins are: 64(SPIs) + 128 * node_nr(LPIs). So we can translate
> the pin index in a unified way in mbigen_domain_translate().
> 
> Also Add TYPE and VEC registers that used by generating SPIs, the driver can
> access them when MBIGEN is used to generate SPIs.
> 
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  drivers/irqchip/irq-mbigen.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index f05998f..37c1932 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -48,6 +48,7 @@
>  #define MBIGEN_NODE_OFFSET		0x1000
>  
>  /* offset of vector register in mbigen node */
> +#define REG_MBIGEN_SPI_VEC_OFFSET	0x500
>  #define REG_MBIGEN_LPI_VEC_OFFSET	0x200
>  
>  /**
> @@ -62,6 +63,7 @@
>   * This register is used to configure interrupt
>   * trigger type
>   */
> +#define REG_MBIGEN_SPI_TYPE_OFFSET	0x400
>  #define REG_MBIGEN_LPI_TYPE_OFFSET	0x0
>  
>  /**
> @@ -79,6 +81,9 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>  {
>  	unsigned int nid, pin;
>  
> +	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP)
> +		return (hwirq * 4 + REG_MBIGEN_SPI_VEC_OFFSET);
> +
>  	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
>  	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>  	pin = hwirq % IRQS_PER_MBIGEN_NODE;
> @@ -92,6 +97,13 @@ static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
>  {
>  	unsigned int nid, irq_ofst, ofst;
>  
> +	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
> +		*mask = 1 << (hwirq % 32);
> +		ofst = hwirq / 32 * 4;
> +		*addr = ofst + REG_MBIGEN_SPI_TYPE_OFFSET;
> +		return;
> +	}
> +
>  	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
>  	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>  	irq_ofst = hwirq % IRQS_PER_MBIGEN_NODE;
> @@ -162,6 +174,12 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>  	u32 val;
>  
>  	base += get_mbigen_vec_reg(d->hwirq);
> +
> +	if (d->hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
> +		writel_relaxed(msg->data, base);
> +		return;
> +	}

How is the GICD_SETSPI_NSR base address programmed if you're ignoring
the address stored in the message?

How do you deal with level interrupts, as you don't seem to use the
level MSI framework either?

> +
>  	val = readl_relaxed(base);
>  
>  	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
> @@ -182,8 +200,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
>  		if (fwspec->param_count != 2)
>  			return -EINVAL;
>  
> -		if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
> -			(fwspec->param[0] < SPI_NUM_PER_MBIGEN_CHIP))
> +		if (fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM)
>  			return -EINVAL;
>  		else
>  			*hwirq = fwspec->param[0];
> 

Now, the biggest question of them all: how does it work with ACPI? Last
time I checked, there was no DT support for platforms using the MBIGEN.

My gut feeling is that it would be so much better if the SPIs generated
by the MBIGEN were configured in firmware, and the devices behind it
would just describe the corresponding SPI...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 3/4] irqchip/mbigen: add support for a MBIGEN generating SPIs

[Yang Yingliang]

Hi, Marc

On 2018/10/18 0:30, Marc Zyngier wrote:
> On 16/10/18 10:15, Yang Yingliang wrote:
>> Now with
>> 5052875 ("irqchip/gic-v3: Add support for Message Based Interrupts as an MSI controller"),
>> we can support MBIGEN to generate message based SPIs by writing GICD_SETSPIR.
>>
>> The first 64-pins of each MBIGEN chip is used to generate SPIs, and each
>> MBIGEN chip has several MBIGEN nodes, every node has 128 pins for generating
>> LPIs. The total pins are: 64(SPIs) + 128 * node_nr(LPIs). So we can translate
>> the pin index in a unified way in mbigen_domain_translate().
>>
>> Also Add TYPE and VEC registers that used by generating SPIs, the driver can
>> access them when MBIGEN is used to generate SPIs.
>>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>>   drivers/irqchip/irq-mbigen.c | 21 +++++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>> index f05998f..37c1932 100644
>> --- a/drivers/irqchip/irq-mbigen.c
>> +++ b/drivers/irqchip/irq-mbigen.c
>> @@ -48,6 +48,7 @@
>>   #define MBIGEN_NODE_OFFSET		0x1000
>>   
>>   /* offset of vector register in mbigen node */
>> +#define REG_MBIGEN_SPI_VEC_OFFSET	0x500
>>   #define REG_MBIGEN_LPI_VEC_OFFSET	0x200
>>   
>>   /**
>> @@ -62,6 +63,7 @@
>>    * This register is used to configure interrupt
>>    * trigger type
>>    */
>> +#define REG_MBIGEN_SPI_TYPE_OFFSET	0x400
>>   #define REG_MBIGEN_LPI_TYPE_OFFSET	0x0
>>   
>>   /**
>> @@ -79,6 +81,9 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>>   {
>>   	unsigned int nid, pin;
>>   
>> +	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP)
>> +		return (hwirq * 4 + REG_MBIGEN_SPI_VEC_OFFSET);
>> +
>>   	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
>>   	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>>   	pin = hwirq % IRQS_PER_MBIGEN_NODE;
>> @@ -92,6 +97,13 @@ static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
>>   {
>>   	unsigned int nid, irq_ofst, ofst;
>>   
>> +	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
>> +		*mask = 1 << (hwirq % 32);
>> +		ofst = hwirq / 32 * 4;
>> +		*addr = ofst + REG_MBIGEN_SPI_TYPE_OFFSET;
>> +		return;
>> +	}
>> +
>>   	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
>>   	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>>   	irq_ofst = hwirq % IRQS_PER_MBIGEN_NODE;
>> @@ -162,6 +174,12 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>>   	u32 val;
>>   
>>   	base += get_mbigen_vec_reg(d->hwirq);
>> +
>> +	if (d->hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
>> +		writel_relaxed(msg->data, base);
>> +		return;
>> +	}
> How is the GICD_SETSPI_NSR base address programmed if you're ignoring
> the address stored in the message?
Now, this base address is encoded in MBIGEN register by default.

In case this address isn't set to register in later SoCs, I think the
MBIGEN driver set this base address would be better. I will add
relative codes to handle both GICD_SETSPI_NSR and ITS Translate
address.

>
> How do you deal with level interrupts, as you don't seem to use the
> level MSI framework either?
The MBIGEN driver need to clear active state in irq_eoi hook, when it
dealing with level SPIs.

>
>> +
>>   	val = readl_relaxed(base);
>>   
>>   	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
>> @@ -182,8 +200,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
>>   		if (fwspec->param_count != 2)
>>   			return -EINVAL;
>>   
>> -		if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
>> -			(fwspec->param[0] < SPI_NUM_PER_MBIGEN_CHIP))
>> +		if (fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM)
>>   			return -EINVAL;
>>   		else
>>   			*hwirq = fwspec->param[0];
>>
> Now, the biggest question of them all: how does it work with ACPI? Last
> time I checked, there was no DT support for platforms using the MBIGEN.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/hisilicon/hip07.dtsi
This DT describes how platform devices using the MBIGEN.

>
> My gut feeling is that it would be so much better if the SPIs generated
> by the MBIGEN were configured in firmware, and the devices behind it
> would just describe the corresponding SPI...
We need use this framework to clear active state in MBIGEN register, so 
configuring
in firmware is not enough...

Regards,
Yang
>
> Thanks,
>
> 	M.

Re: [PATCH 3/4] irqchip/mbigen: add support for a MBIGEN generating SPIs

[Marc Zyngier]

Hi Yang,

On 18/10/18 04:41, Yang Yingliang wrote:
> Hi, Marc
> 
> On 2018/10/18 0:30, Marc Zyngier wrote:
>> On 16/10/18 10:15, Yang Yingliang wrote:
>>> Now with
>>> 5052875 ("irqchip/gic-v3: Add support for Message Based Interrupts as an MSI controller"),
>>> we can support MBIGEN to generate message based SPIs by writing GICD_SETSPIR.
>>>
>>> The first 64-pins of each MBIGEN chip is used to generate SPIs, and each
>>> MBIGEN chip has several MBIGEN nodes, every node has 128 pins for generating
>>> LPIs. The total pins are: 64(SPIs) + 128 * node_nr(LPIs). So we can translate
>>> the pin index in a unified way in mbigen_domain_translate().
>>>
>>> Also Add TYPE and VEC registers that used by generating SPIs, the driver can
>>> access them when MBIGEN is used to generate SPIs.
>>>
>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>>> ---
>>>   drivers/irqchip/irq-mbigen.c | 21 +++++++++++++++++++--
>>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>>> index f05998f..37c1932 100644
>>> --- a/drivers/irqchip/irq-mbigen.c
>>> +++ b/drivers/irqchip/irq-mbigen.c
>>> @@ -48,6 +48,7 @@
>>>   #define MBIGEN_NODE_OFFSET		0x1000
>>>   
>>>   /* offset of vector register in mbigen node */
>>> +#define REG_MBIGEN_SPI_VEC_OFFSET	0x500
>>>   #define REG_MBIGEN_LPI_VEC_OFFSET	0x200
>>>   
>>>   /**
>>> @@ -62,6 +63,7 @@
>>>    * This register is used to configure interrupt
>>>    * trigger type
>>>    */
>>> +#define REG_MBIGEN_SPI_TYPE_OFFSET	0x400
>>>   #define REG_MBIGEN_LPI_TYPE_OFFSET	0x0
>>>   
>>>   /**
>>> @@ -79,6 +81,9 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>>>   {
>>>   	unsigned int nid, pin;
>>>   
>>> +	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP)
>>> +		return (hwirq * 4 + REG_MBIGEN_SPI_VEC_OFFSET);
>>> +
>>>   	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
>>>   	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>>>   	pin = hwirq % IRQS_PER_MBIGEN_NODE;
>>> @@ -92,6 +97,13 @@ static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
>>>   {
>>>   	unsigned int nid, irq_ofst, ofst;
>>>   
>>> +	if (hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
>>> +		*mask = 1 << (hwirq % 32);
>>> +		ofst = hwirq / 32 * 4;
>>> +		*addr = ofst + REG_MBIGEN_SPI_TYPE_OFFSET;
>>> +		return;
>>> +	}
>>> +
>>>   	hwirq -= SPI_NUM_PER_MBIGEN_CHIP;
>>>   	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>>>   	irq_ofst = hwirq % IRQS_PER_MBIGEN_NODE;
>>> @@ -162,6 +174,12 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>>>   	u32 val;
>>>   
>>>   	base += get_mbigen_vec_reg(d->hwirq);
>>> +
>>> +	if (d->hwirq < SPI_NUM_PER_MBIGEN_CHIP) {
>>> +		writel_relaxed(msg->data, base);
>>> +		return;
>>> +	}
>> How is the GICD_SETSPI_NSR base address programmed if you're ignoring
>> the address stored in the message?
> Now, this base address is encoded in MBIGEN register by default.

OK. So let's move the comment in that function to the top, and indicate
that this also applies to the GICD_SETSPI_NSR register.

> In case this address isn't set to register in later SoCs, I think the
> MBIGEN driver set this base address would be better. I will add
> relative codes to handle both GICD_SETSPI_NSR and ITS Translate
> address.
> 
>>
>> How do you deal with level interrupts, as you don't seem to use the
>> level MSI framework either?
> The MBIGEN driver need to clear active state in irq_eoi hook, when it
> dealing with level SPIs.

OK, so you're not taking advantage of the the CLEAR register here? Sad.

> 
>>
>>> +
>>>   	val = readl_relaxed(base);
>>>   
>>>   	val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT);
>>> @@ -182,8 +200,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
>>>   		if (fwspec->param_count != 2)
>>>   			return -EINVAL;
>>>   
>>> -		if ((fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM) ||
>>> -			(fwspec->param[0] < SPI_NUM_PER_MBIGEN_CHIP))
>>> +		if (fwspec->param[0] > MAXIMUM_IRQ_PIN_NUM)
>>>   			return -EINVAL;
>>>   		else
>>>   			*hwirq = fwspec->param[0];
>>>
>> Now, the biggest question of them all: how does it work with ACPI? Last
>> time I checked, there was no DT support for platforms using the MBIGEN.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/hisilicon/hip07.dtsi
> This DT describes how platform devices using the MBIGEN.

That's not how my own D05 boots. It is ACPI only. How is that going to
work on this platform?

> 
>>
>> My gut feeling is that it would be so much better if the SPIs generated
>> by the MBIGEN were configured in firmware, and the devices behind it
>> would just describe the corresponding SPI...
> We need use this framework to clear active state in MBIGEN register, so 
> configuring
> in firmware is not enough...

Fair enough.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 3/4] irqchip/mbigen: add support for a MBIGEN generating SPIs

[Hanjun Guo]

Hi Marc,

>>>>
>>> Now, the biggest question of them all: how does it work with ACPI? Last
>>> time I checked, there was no DT support for platforms using the MBIGEN.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/hisilicon/hip07.dtsi
>> This DT describes how platform devices using the MBIGEN.
> 
> That's not how my own D05 boots. It is ACPI only. How is that going to
> work on this platform?

D05 is ACPI only so it has no dtb in the firmware, that's why we can
boot D05 without acpi=on in the boot cmdline, but if you want to
boot D05 with dtb, you could try to specify the dtb in the grub
(seems centos based):

menuentry "example" --id example{
        linux /Image-kernel root=... rdinit=...
        initrd /example.img
        devicetree /d05.dtb
}

Hope it helps.

Thanks
Hanjun

Re: [PATCH 3/4] irqchip/mbigen: add support for a MBIGEN generating SPIs

[Marc Zyngier]

Hi Hanjun,

On 18/10/18 12:20, Hanjun Guo wrote:
> Hi Marc,
> 
>>>>>
>>>> Now, the biggest question of them all: how does it work with ACPI? Last
>>>> time I checked, there was no DT support for platforms using the MBIGEN.
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/hisilicon/hip07.dtsi
>>> This DT describes how platform devices using the MBIGEN.
>>
>> That's not how my own D05 boots. It is ACPI only. How is that going to
>> work on this platform?
> 
> D05 is ACPI only so it has no dtb in the firmware, that's why we can
> boot D05 without acpi=on in the boot cmdline, but if you want to
> boot D05 with dtb, you could try to specify the dtb in the grub
> (seems centos based):
> 
> menuentry "example" --id example{
>         linux /Image-kernel root=... rdinit=...
>         initrd /example.img
>         devicetree /d05.dtb
> }

Sure. But what I'm asking here is how this change in the MBIGEN driver
can be beneficial to people who need ACPI, for better or worse? For
example, you cannot get the PCIe SMMU without ACPI. Good-bye VFIO.

If it is DT only, I seriously doubt anyone will be able to use it.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 3/4] irqchip/mbigen: add support for a MBIGEN generating SPIs

[Hanjun Guo]

On 2018/10/18 19:56, Marc Zyngier wrote:
> Hi Hanjun,
> 
> On 18/10/18 12:20, Hanjun Guo wrote:
>> Hi Marc,
>>
>>>>>>
>>>>> Now, the biggest question of them all: how does it work with ACPI? Last
>>>>> time I checked, there was no DT support for platforms using the MBIGEN.
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/hisilicon/hip07.dtsi
>>>> This DT describes how platform devices using the MBIGEN.
>>>
>>> That's not how my own D05 boots. It is ACPI only. How is that going to
>>> work on this platform?
>>
>> D05 is ACPI only so it has no dtb in the firmware, that's why we can
>> boot D05 without acpi=on in the boot cmdline, but if you want to
>> boot D05 with dtb, you could try to specify the dtb in the grub
>> (seems centos based):
>>
>> menuentry "example" --id example{
>>         linux /Image-kernel root=... rdinit=...
>>         initrd /example.img
>>         devicetree /d05.dtb
>> }
> 
> Sure. But what I'm asking here is how this change in the MBIGEN driver
> can be beneficial to people who need ACPI, for better or worse? For
> example, you cannot get the PCIe SMMU without ACPI. Good-bye VFIO.
> 
> If it is DT only, I seriously doubt anyone will be able to use it.

We have another ARM64 SoC for wireless base station which with the
same CPU DIE but different IO DIE, they share the same mbigen controller.

Since we use big-endian on wireless base station so only DT is available
(EFI stub is little-endian), we need to reuse this mbigen driver
on this SoC (not the one for D05) as well, not sure if this makes sense
to you, please let us know.

Thanks
Hanjun

[PATCH 4/4] dt-bindings/irqchip/mbigen: add example of MBIGEN generate SPIs

[Yang Yingliang]

Now MBIGEN can support to generate SPIs by writing
GICD_SETSPIR. Add dt example to help document.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 .../interrupt-controller/hisilicon,mbigen-v2.txt        | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt b/Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt
index a6813a0..298c033 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/hisilicon,mbigen-v2.txt
@@ -10,7 +10,7 @@ Hisilicon designed mbigen to collect and generate interrupt.
 
 
 Non-pci devices can connect to mbigen and generate the
-interrupt by writing ITS register.
+interrupt by writing GICD or ITS register.
 
 The mbigen chip and devices connect to mbigen have the following properties:
 
@@ -64,6 +64,13 @@ Examples:
 				num-pins = <2>;
 				#interrupt-cells = <2>;
 			};
+
+			mbigen_spi_example:spi_example {
+				interrupt-controller;
+				msi-parent = <&gic>;
+				num-pins = <2>;
+				#interrupt-cells = <2>;
+			};
 	};
 
 Devices connect to mbigen required properties:
@@ -82,3 +89,11 @@ Examples:
 		interrupts =	<656 1>,
 				<657 1>;
 	};
+
+	spi_example: spi0@0 {
+		compatible = "spi,example";
+		reg = <0 0 0 0>;
+		interrupt-parent = <&mbigen_spi_example>;
+		interrupts = <13 4>,
+			     <14 4>;
+	};
-- 
1.8.3