From: Baoqi Zhang <[email protected]>
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 <[email protected]>
Signed-off-by: Biao Dong <[email protected]>
Signed-off-by: Tianyang Zhang <[email protected]>
---
drivers/irqchip/irq-loongson-pch-pic.c | 76 ++++++++++++++++++++------
1 file changed, 59 insertions(+), 17 deletions(-)
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;
--
2.20.1
On Tue, Mar 19 2024 at 20:46, Tianyang Zhang wrote:
> From: Baoqi Zhang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Biao Dong <[email protected]>
> Signed-off-by: Tianyang Zhang <[email protected]>
This Signed-off-by chain is wrong.
Please see Documentation/process/submitting-patches.rst
Thanks,
tglx
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Biao Dong <[email protected]>
>> Signed-off-by: Tianyang Zhang <[email protected]>
> This Signed-off-by chain is wrong.
>
> Please see Documentation/process/submitting-patches.rst
>
> Thanks,
>
> tglx
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
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