diff mbox series

[v2,01/12] irqchip/gic: Rework gic_configure_irq to take the full ICFGR base

Message ID 20190806100121.240767-2-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series irqchip/gic-v3: Add support for GICv3.1 extended PPI/SPI ranges | expand

Commit Message

Marc Zyngier Aug. 6, 2019, 10:01 a.m. UTC 
gic_configure_irq is currently passed the (re)distributor address,
to which it applies an a fixed offset to get to the configuration
registers. This offset is constant across all GICs, or rather it was
until to v3.1...

An easy way out is for the individual drivers to pass the base
address of the configuration register for the considered interrupt.
At the same time, move part of the error handling back to the
individual drivers, as things are about to change on that front.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-common.c | 14 +++++---------
 drivers/irqchip/irq-gic-v3.c     | 11 ++++++++++-
 drivers/irqchip/irq-gic.c        | 10 +++++++++-
 drivers/irqchip/irq-hip04.c      |  7 ++++++-
 4 files changed, 30 insertions(+), 12 deletions(-)

Comments

Zenghui Yu Aug. 19, 2019, 2:26 p.m. UTC | #1 
Hi Marc,

On 2019/8/6 18:01, Marc Zyngier wrote:
> gic_configure_irq is currently passed the (re)distributor address,
> to which it applies an a fixed offset to get to the configuration
> registers. This offset is constant across all GICs, or rather it was
> until to v3.1...
> 
> An easy way out is for the individual drivers to pass the base
> address of the configuration register for the considered interrupt.
> At the same time, move part of the error handling back to the
> individual drivers, as things are about to change on that front.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>   drivers/irqchip/irq-gic-common.c | 14 +++++---------
>   drivers/irqchip/irq-gic-v3.c     | 11 ++++++++++-
>   drivers/irqchip/irq-gic.c        | 10 +++++++++-
>   drivers/irqchip/irq-hip04.c      |  7 ++++++-
>   4 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index b0a8215a13fc..6900b6f0921c 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -63,7 +63,7 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
>   	 * for "irq", depending on "type".
>   	 */
>   	raw_spin_lock_irqsave(&irq_controller_lock, flags);
> -	val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
> +	val = oldval = readl_relaxed(base + confoff);
>   	if (type & IRQ_TYPE_LEVEL_MASK)
>   		val &= ~confmask;
>   	else if (type & IRQ_TYPE_EDGE_BOTH)
> @@ -83,14 +83,10 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
>   	 * does not allow us to set the configuration or we are in a
>   	 * non-secure mode, and hence it may not be catastrophic.
>   	 */
> -	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
> -	if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val) {
> -		if (WARN_ON(irq >= 32))
> -			ret = -EINVAL;

Since this WARN_ON is dropped, the comment above should also be updated.
But what is the reason for deleting it?  (It may give us some points
when we fail to set type for SPIs.)


Thanks,
zenghui

> -		else
> -			pr_warn("GIC: PPI%d is secure or misconfigured\n",
> -				irq - 16);
> -	}
> +	writel_relaxed(val, base + confoff);
> +	if (readl_relaxed(base + confoff) != val)
> +		ret = -EINVAL;
> +
>   	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
>   
>   	if (sync_access)
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 96d927f0f91a..b250e69908f8 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -407,6 +407,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>   	unsigned int irq = gic_irq(d);
>   	void (*rwp_wait)(void);
>   	void __iomem *base;
> +	int ret;
>   
>   	/* Interrupt configuration for SGIs can't be changed */
>   	if (irq < 16)
> @@ -425,7 +426,15 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>   		rwp_wait = gic_dist_wait_for_rwp;
>   	}
>   
> -	return gic_configure_irq(irq, type, base, rwp_wait);
> +
> +	ret = gic_configure_irq(irq, type, base + GICD_ICFGR, rwp_wait);
> +	if (ret && irq < 32) {
> +		/* Misconfigured PPIs are usually not fatal */
> +		pr_warn("GIC: PPI%d is secure or misconfigured\n", irq - 16);
> +		ret = 0;
> +	}
> +
> +	return ret;
>   }
>   
>   static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index e45f45e68720..ab48760acabb 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -291,6 +291,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>   {
>   	void __iomem *base = gic_dist_base(d);
>   	unsigned int gicirq = gic_irq(d);
> +	int ret;
>   
>   	/* Interrupt configuration for SGIs can't be changed */
>   	if (gicirq < 16)
> @@ -301,7 +302,14 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>   			    type != IRQ_TYPE_EDGE_RISING)
>   		return -EINVAL;
>   
> -	return gic_configure_irq(gicirq, type, base, NULL);
> +	ret = gic_configure_irq(gicirq, type, base + GIC_DIST_CONFIG, NULL);
> +	if (ret && gicirq < 32) {
> +		/* Misconfigured PPIs are usually not fatal */
> +		pr_warn("GIC: PPI%d is secure or misconfigured\n", gicirq - 16);
> +		ret = 0;
> +	}
> +
> +	return ret;
>   }
>   
>   static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
> diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c
> index cf705827599c..1626131834a6 100644
> --- a/drivers/irqchip/irq-hip04.c
> +++ b/drivers/irqchip/irq-hip04.c
> @@ -130,7 +130,12 @@ static int hip04_irq_set_type(struct irq_data *d, unsigned int type)
>   
>   	raw_spin_lock(&irq_controller_lock);
>   
> -	ret = gic_configure_irq(irq, type, base, NULL);
> +	ret = gic_configure_irq(irq, type, base + GIC_DIST_CONFIG, NULL);
> +	if (ret && irq < 32) {
> +		/* Misconfigured PPIs are usually not fatal */
> +		pr_warn("GIC: PPI%d is secure or misconfigured\n", irq - 16);
> +		ret = 0;
> +	}
>   
>   	raw_spin_unlock(&irq_controller_lock);
>   
>
Marc Zyngier Aug. 19, 2019, 2:53 p.m. UTC | #2 
On 19/08/2019 15:26, Zenghui Yu wrote:
> Hi Marc,
> 
> On 2019/8/6 18:01, Marc Zyngier wrote:
>> gic_configure_irq is currently passed the (re)distributor address,
>> to which it applies an a fixed offset to get to the configuration
>> registers. This offset is constant across all GICs, or rather it was
>> until to v3.1...
>>
>> An easy way out is for the individual drivers to pass the base
>> address of the configuration register for the considered interrupt.
>> At the same time, move part of the error handling back to the
>> individual drivers, as things are about to change on that front.
>>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>   drivers/irqchip/irq-gic-common.c | 14 +++++---------
>>   drivers/irqchip/irq-gic-v3.c     | 11 ++++++++++-
>>   drivers/irqchip/irq-gic.c        | 10 +++++++++-
>>   drivers/irqchip/irq-hip04.c      |  7 ++++++-
>>   4 files changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
>> index b0a8215a13fc..6900b6f0921c 100644
>> --- a/drivers/irqchip/irq-gic-common.c
>> +++ b/drivers/irqchip/irq-gic-common.c
>> @@ -63,7 +63,7 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
>>   	 * for "irq", depending on "type".
>>   	 */
>>   	raw_spin_lock_irqsave(&irq_controller_lock, flags);
>> -	val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
>> +	val = oldval = readl_relaxed(base + confoff);
>>   	if (type & IRQ_TYPE_LEVEL_MASK)
>>   		val &= ~confmask;
>>   	else if (type & IRQ_TYPE_EDGE_BOTH)
>> @@ -83,14 +83,10 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
>>   	 * does not allow us to set the configuration or we are in a
>>   	 * non-secure mode, and hence it may not be catastrophic.
>>   	 */
>> -	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
>> -	if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val) {
>> -		if (WARN_ON(irq >= 32))
>> -			ret = -EINVAL;
> 
> Since this WARN_ON is dropped, the comment above should also be updated.
> But what is the reason for deleting it?  (It may give us some points
> when we fail to set type for SPIs.)

The core code already warns in the case where irq_set_type() fails, and
the duplication of warnings is pretty superfluous.

Thanks,

	M.
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index b0a8215a13fc..6900b6f0921c 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -63,7 +63,7 @@  int gic_configure_irq(unsigned int irq, unsigned int type,
 	 * for "irq", depending on "type".
 	 */
 	raw_spin_lock_irqsave(&irq_controller_lock, flags);
-	val = oldval = readl_relaxed(base + GIC_DIST_CONFIG + confoff);
+	val = oldval = readl_relaxed(base + confoff);
 	if (type & IRQ_TYPE_LEVEL_MASK)
 		val &= ~confmask;
 	else if (type & IRQ_TYPE_EDGE_BOTH)
@@ -83,14 +83,10 @@  int gic_configure_irq(unsigned int irq, unsigned int type,
 	 * does not allow us to set the configuration or we are in a
 	 * non-secure mode, and hence it may not be catastrophic.
 	 */
-	writel_relaxed(val, base + GIC_DIST_CONFIG + confoff);
-	if (readl_relaxed(base + GIC_DIST_CONFIG + confoff) != val) {
-		if (WARN_ON(irq >= 32))
-			ret = -EINVAL;
-		else
-			pr_warn("GIC: PPI%d is secure or misconfigured\n",
-				irq - 16);
-	}
+	writel_relaxed(val, base + confoff);
+	if (readl_relaxed(base + confoff) != val)
+		ret = -EINVAL;
+
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 
 	if (sync_access)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 96d927f0f91a..b250e69908f8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -407,6 +407,7 @@  static int gic_set_type(struct irq_data *d, unsigned int type)
 	unsigned int irq = gic_irq(d);
 	void (*rwp_wait)(void);
 	void __iomem *base;
+	int ret;
 
 	/* Interrupt configuration for SGIs can't be changed */
 	if (irq < 16)
@@ -425,7 +426,15 @@  static int gic_set_type(struct irq_data *d, unsigned int type)
 		rwp_wait = gic_dist_wait_for_rwp;
 	}
 
-	return gic_configure_irq(irq, type, base, rwp_wait);
+
+	ret = gic_configure_irq(irq, type, base + GICD_ICFGR, rwp_wait);
+	if (ret && irq < 32) {
+		/* Misconfigured PPIs are usually not fatal */
+		pr_warn("GIC: PPI%d is secure or misconfigured\n", irq - 16);
+		ret = 0;
+	}
+
+	return ret;
 }
 
 static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index e45f45e68720..ab48760acabb 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -291,6 +291,7 @@  static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	void __iomem *base = gic_dist_base(d);
 	unsigned int gicirq = gic_irq(d);
+	int ret;
 
 	/* Interrupt configuration for SGIs can't be changed */
 	if (gicirq < 16)
@@ -301,7 +302,14 @@  static int gic_set_type(struct irq_data *d, unsigned int type)
 			    type != IRQ_TYPE_EDGE_RISING)
 		return -EINVAL;
 
-	return gic_configure_irq(gicirq, type, base, NULL);
+	ret = gic_configure_irq(gicirq, type, base + GIC_DIST_CONFIG, NULL);
+	if (ret && gicirq < 32) {
+		/* Misconfigured PPIs are usually not fatal */
+		pr_warn("GIC: PPI%d is secure or misconfigured\n", gicirq - 16);
+		ret = 0;
+	}
+
+	return ret;
 }
 
 static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c
index cf705827599c..1626131834a6 100644
--- a/drivers/irqchip/irq-hip04.c
+++ b/drivers/irqchip/irq-hip04.c
@@ -130,7 +130,12 @@  static int hip04_irq_set_type(struct irq_data *d, unsigned int type)
 
 	raw_spin_lock(&irq_controller_lock);
 
-	ret = gic_configure_irq(irq, type, base, NULL);
+	ret = gic_configure_irq(irq, type, base + GIC_DIST_CONFIG, NULL);
+	if (ret && irq < 32) {
+		/* Misconfigured PPIs are usually not fatal */
+		pr_warn("GIC: PPI%d is secure or misconfigured\n", irq - 16);
+		ret = 0;
+	}
 
 	raw_spin_unlock(&irq_controller_lock);