diff mbox series

[V3] irqchip/loongson-pch-pic: Update interrupt registration policy

Message ID 20240319124629.23925-1-zhangtianyang@loongson.cn (mailing list archive)
State Superseded
Headers show
Series [V3] irqchip/loongson-pch-pic: Update interrupt registration policy | expand

Commit Message

Tianyang Zhang March 19, 2024, 12:46 p.m. UTC 
From: Baoqi Zhang <zhangbaoqi@loongson.cn>

This patch remove the fixed mapping between the LS7A interrupt source
and the HT interrupt vector, and replaced it with a dynamically
allocated approach.

We introduce a mapping table in struct pch_pic, where each interrupt
source will allocate an index as a 'hwirq' from the table in the order
of application and set table value as interrupt source number. This hwirq
will be configured as its vector in the HT interrupt controller. For an
interrupt source, the validity period of the obtained hwirq will last until
the system reset.

This will be more conducive to fully utilizing existing vectors to
support more devices.

Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
Signed-off-by: Biao Dong <dongbiao@loongson.cn>
Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>
---
 drivers/irqchip/irq-loongson-pch-pic.c | 76 ++++++++++++++++++++------
 1 file changed, 59 insertions(+), 17 deletions(-)

Comments

Thomas Gleixner March 20, 2024, 10:52 a.m. UTC | #1 
On Tue, Mar 19 2024 at 20:46, Tianyang Zhang wrote:
> From: Baoqi Zhang <zhangbaoqi@loongson.cn>
>
> This patch remove the fixed mapping between the LS7A interrupt source

Please don't use 'This patch'. We already know that this is a patch.

See Documentation/process/

> and the HT interrupt vector, and replaced it with a dynamically
> allocated approach.

You explain the WHAT, but you really need to explain the WHY. 

> We introduce a mapping table in struct pch_pic, where each interrupt

s/We introduce/Introduce/ See documentation.

> source will allocate an index as a 'hwirq' from the table in the order
> of application and set table value as interrupt source number. This hwirq
> will be configured as its vector in the HT interrupt controller. For an
> interrupt source, the validity period of the obtained hwirq will last until
> the system reset.
>
> This will be more conducive to fully utilizing existing vectors to
> support more devices.
>
> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
> Signed-off-by: Biao Dong <dongbiao@loongson.cn>
> Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>

This Signed-off-by chain is wrong.

Please see Documentation/process/submitting-patches.rst

Thanks,

        tglx
Tianyang Zhang March 22, 2024, 10:14 a.m. UTC | #2 
Hi, Thomas

I will continue to revise my patch according to the document requirements.

Regarding "WHY", my understanding is that a convincing reason is needed 
to explain the necessity of this patch.

If so, can the last paragraph "This will be more conducive to fully 
utilizing existing vectors to support more devices."

be considered a simple explanation?

在 2024/3/20 下午6:52, Thomas Gleixner 写道:
> On Tue, Mar 19 2024 at 20:46, Tianyang Zhang wrote:
>> From: Baoqi Zhang <zhangbaoqi@loongson.cn>
>>
>> This patch remove the fixed mapping between the LS7A interrupt source
> Please don't use 'This patch'. We already know that this is a patch.
>
> See Documentation/process/
>
>> and the HT interrupt vector, and replaced it with a dynamically
>> allocated approach.
> You explain the WHAT, but you really need to explain the WHY.
>
>> We introduce a mapping table in struct pch_pic, where each interrupt
> s/We introduce/Introduce/ See documentation.
>
>> source will allocate an index as a 'hwirq' from the table in the order
>> of application and set table value as interrupt source number. This hwirq
>> will be configured as its vector in the HT interrupt controller. For an
>> interrupt source, the validity period of the obtained hwirq will last until
>> the system reset.
>>
>> This will be more conducive to fully utilizing existing vectors to
>> support more devices.
>>
>> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
>> Signed-off-by: Biao Dong <dongbiao@loongson.cn>
>> Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>
> This Signed-off-by chain is wrong.
>
> Please see Documentation/process/submitting-patches.rst
>
> Thanks,
>
>          tglx
Thomas Gleixner March 23, 2024, 8:05 p.m. UTC | #3 
Tianyang!

On Fri, Mar 22 2024 at 18:14, Tianyang Zhang wrote:

Please do not top-post. See the 'Top-posting' chapter in:
https://people.kernel.org/tglx/notes-about-netiquette 

> Regarding "WHY", my understanding is that a convincing reason is needed 
> to explain the necessity of this patch.

Yes.

> If so, can the last paragraph "This will be more conducive to fully 
> utilizing existing vectors to support more devices."
>
> be considered a simple explanation?

Kinda, but ideally you describe it in a way that there is context for
the reader. Like this:

  The fixed mapping between the LS7A interrupt source and the HT
  interrupt vector prevents the utilization of the full interrupt vector
  space which limits the number of devices in a system

  Replace the fixed mapping with a dynamic mapping which allocates a
  vector when an interrupt source is set up. This avoids that unused
  sources prevent vectors from being used for other devices.

See?

Thanks,

        tglx
Tianyang Zhang March 25, 2024, 1:54 a.m. UTC | #4 
Hi, Thomas

在 2024/3/24 上午4:05, Thomas Gleixner 写道:
> Tianyang!
>
> On Fri, Mar 22 2024 at 18:14, Tianyang Zhang wrote:
>
> Please do not top-post. See the 'Top-posting' chapter in:
> https://people.kernel.org/tglx/notes-about-netiquette
Sorry, I will carefully read the document
>> Regarding "WHY", my understanding is that a convincing reason is needed
>> to explain the necessity of this patch.
> Yes.
>
>> If so, can the last paragraph "This will be more conducive to fully
>> utilizing existing vectors to support more devices."
>>
>> be considered a simple explanation?
> Kinda, but ideally you describe it in a way that there is context for
> the reader. Like this:
>
>    The fixed mapping between the LS7A interrupt source and the HT
>    interrupt vector prevents the utilization of the full interrupt vector
>    space which limits the number of devices in a system
>
>    Replace the fixed mapping with a dynamic mapping which allocates a
>    vector when an interrupt source is set up. This avoids that unused
>    sources prevent vectors from being used for other devices.
>
> See?
Thank you for your demonstration. I will make the modifications 
according to the requirements
>
> Thanks,
>
>          tglx


Thanks again

             Tianyang
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
index 63db8e2172e0..1ee63fcf4b5a 100644
--- a/drivers/irqchip/irq-loongson-pch-pic.c
+++ b/drivers/irqchip/irq-loongson-pch-pic.c
@@ -33,6 +33,7 @@ 
 #define PIC_COUNT		(PIC_COUNT_PER_REG * PIC_REG_COUNT)
 #define PIC_REG_IDX(irq_id)	((irq_id) / PIC_COUNT_PER_REG)
 #define PIC_REG_BIT(irq_id)	((irq_id) % PIC_COUNT_PER_REG)
+#define PIC_UNDEF_VECTOR	255
 
 static int nr_pics;
 
@@ -46,12 +47,19 @@  struct pch_pic {
 	u32			saved_vec_en[PIC_REG_COUNT];
 	u32			saved_vec_pol[PIC_REG_COUNT];
 	u32			saved_vec_edge[PIC_REG_COUNT];
+	u8			table[PIC_COUNT];
+	int			inuse;
 };
 
 static struct pch_pic *pch_pic_priv[MAX_IO_PICS];
 
 struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
 
+static inline u8 hwirq_to_bit(struct pch_pic *priv, int hirq)
+{
+	return priv->table[hirq];
+}
+
 static void pch_pic_bitset(struct pch_pic *priv, int offset, int bit)
 {
 	u32 reg;
@@ -80,45 +88,47 @@  static void pch_pic_mask_irq(struct irq_data *d)
 {
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
 
-	pch_pic_bitset(priv, PCH_PIC_MASK, d->hwirq);
+	pch_pic_bitset(priv, PCH_PIC_MASK, hwirq_to_bit(priv, d->hwirq));
 	irq_chip_mask_parent(d);
 }
 
 static void pch_pic_unmask_irq(struct irq_data *d)
 {
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
+	int bit = hwirq_to_bit(priv, d->hwirq);
 
-	writel(BIT(PIC_REG_BIT(d->hwirq)),
-			priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
+	writel(BIT(PIC_REG_BIT(bit)),
+			priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
 
 	irq_chip_unmask_parent(d);
-	pch_pic_bitclr(priv, PCH_PIC_MASK, d->hwirq);
+	pch_pic_bitclr(priv, PCH_PIC_MASK, bit);
 }
 
 static int pch_pic_set_type(struct irq_data *d, unsigned int type)
 {
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
+	int bit = hwirq_to_bit(priv, d->hwirq);
 	int ret = 0;
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
-		pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitset(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitclr(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_edge_irq);
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitset(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitset(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_edge_irq);
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitclr(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitclr(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_level_irq);
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitclr(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitset(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_level_irq);
 		break;
 	default:
@@ -133,11 +143,12 @@  static void pch_pic_ack_irq(struct irq_data *d)
 {
 	unsigned int reg;
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
+	int bit = hwirq_to_bit(priv, d->hwirq);
 
-	reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(d->hwirq) * 4);
-	if (reg & BIT(PIC_REG_BIT(d->hwirq))) {
-		writel(BIT(PIC_REG_BIT(d->hwirq)),
-			priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
+	reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(bit) * 4);
+	if (reg & BIT(PIC_REG_BIT(bit))) {
+		writel(BIT(PIC_REG_BIT(bit)),
+			priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
 	}
 	irq_chip_ack_parent(d);
 }
@@ -159,6 +170,8 @@  static int pch_pic_domain_translate(struct irq_domain *d,
 {
 	struct pch_pic *priv = d->host_data;
 	struct device_node *of_node = to_of_node(fwspec->fwnode);
+	unsigned long flags;
+	int i;
 
 	if (of_node) {
 		if (fwspec->param_count < 2)
@@ -171,6 +184,27 @@  static int pch_pic_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		*hwirq = fwspec->param[0] - priv->gsi_base;
+
+		raw_spin_lock_irqsave(&priv->pic_lock, flags);
+		/* Check pic-table to confirm if the hwirq has been assigned */
+		for (i = 0; i < priv->inuse; i++) {
+			if (priv->table[i] == *hwirq) {
+				*hwirq = i;
+				break;
+			}
+		}
+		if (i == priv->inuse) {
+			/* Assign a new hwirq in pic-table */
+			if (priv->inuse >= PIC_COUNT) {
+				pr_err("pch-pic domain has no free vectors\n");
+				raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
+				return -EINVAL;
+			}
+			priv->table[priv->inuse] = *hwirq;
+			*hwirq = priv->inuse++;
+		}
+		raw_spin_unlock_irqrestore(&priv->pic_lock, flags);
+
 		if (fwspec->param_count > 1)
 			*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
 		else
@@ -194,6 +228,9 @@  static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
 	if (err)
 		return err;
 
+	/* Write vector ID */
+	writeb(priv->ht_vec_base + hwirq, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, hwirq)));
+
 	parent_fwspec.fwnode = domain->parent->fwnode;
 	parent_fwspec.param_count = 1;
 	parent_fwspec.param[0] = hwirq + priv->ht_vec_base;
@@ -222,7 +259,7 @@  static void pch_pic_reset(struct pch_pic *priv)
 
 	for (i = 0; i < PIC_COUNT; i++) {
 		/* Write vector ID */
-		writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i));
+		writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, i)));
 		/* Hardcode route to HT0 Lo */
 		writeb(1, priv->base + PCH_INT_ROUTE(i));
 	}
@@ -284,6 +321,7 @@  static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
 			u32 gsi_base)
 {
 	struct pch_pic *priv;
+	int i;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -294,6 +332,10 @@  static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
 	if (!priv->base)
 		goto free_priv;
 
+	priv->inuse = 0;
+	for (i = 0; i < PIC_COUNT; i++)
+		priv->table[i] = PIC_UNDEF_VECTOR;
+
 	priv->ht_vec_base = vec_base;
 	priv->vec_count = ((readq(priv->base) >> 48) & 0xff) + 1;
 	priv->gsi_base = gsi_base;