[PATCH v3 0/5] enhance configuring an ITS

[PATCH v3 0/5] enhance configuring an ITS

[Yun Wu]

This patch series makes some enhancement to ITS configuration in the
following aspects:

o make allocation of the ITS tables more sensible
o replace magic numbers with sensible macros
o guarantees a safe quiescent status before initializing an ITS

This patch series is based on Marc's branch[1], and tested on Hisilion
ARM64 board with GICv3 ITS hardware.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/gic-fixes

v2 -> v3:
o drop the patch of tracing LPI enabling status since Vladimir Murzin
  had already posted a similar patch
o fix several improper description issues

v1 -> v2:
o rebase to Marc's GIC fix branch
o drop size calculation for Device Table since Marc had already posted one
o guarantees a safe quiescent status before initializing an ITS as
  Marc suggested, rather than register a reboot notifier
o fix an issue about the enabling status of LPI feature

Yun Wu (5):
  irqchip: gicv3-its: zero itt before handling to hardware
  irqchip: gicv3-its: use 64KB page as default granule
  irqchip: gicv3-its: add limitation to page order
  irqchip: gicv3-its: define macros for GITS_CTLR fields
  irqchip: gicv3-its: support safe initialization

 drivers/irqchip/irq-gic-v3-its.c   | 46 +++++++++++++++++++++++++++++++++++---
 include/linux/irqchip/arm-gic-v3.h |  3 +++
 2 files changed, 46 insertions(+), 3 deletions(-)

--
1.8.0

[PATCH v3 1/5] irqchip: gicv3-its: zero itt before handling to hardware

[Yun Wu]

Some kind of brain-dead implementations chooses to insert ITEes in
rapid sequence of disabled ITEes, and an un-zeroed ITT will confuse
ITS on judging whether an ITE is really enabled or not. Considering
the implementations are still supported by the GICv3 architecture,
in which ITT is not required to be zeroed before being handled to
hardware, we do the favor in ITS driver.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6850141..69eeea3 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1076,7 +1076,7 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
 	nr_ites = max(2UL, roundup_pow_of_two(nvecs));
 	sz = nr_ites * its->ite_size;
 	sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
-	itt = kmalloc(sz, GFP_KERNEL);
+	itt = kzalloc(sz, GFP_KERNEL);
 	lpi_map = its_lpi_alloc_chunks(nvecs, &lpi_base, &nr_lpis);

 	if (!dev || !itt || !lpi_map) {
--
1.8.0

[PATCH v3 2/5] irqchip: gicv3-its: use 64KB page as default granule

[Yun Wu]

The field of page size in register GITS_BASERn might be read-only
if an implementation only supports a single, fixed page size. But
currently the ITS driver will throw out an error when PAGE_SIZE
is less than the minimum size supported by an ITS. So addressing
this problem by using 64KB pages as default granule for all the
ITS base tables.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 69eeea3..f5bfa42 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -800,7 +800,7 @@ static int its_alloc_tables(struct its_node *its)
 {
 	int err;
 	int i;
-	int psz = PAGE_SIZE;
+	int psz = SZ_64K;
 	u64 shr = GITS_BASER_InnerShareable;

 	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
--
1.8.0

[PATCH v3 3/5] irqchip: gicv3-its: add limitation to page order

[Yun Wu]

When required size of Device Table is out of the page allocator's
capability, the whole ITS will fail in probing. This actually is
not the hardware's problem and is mainly a limitation of the kernel
page allocator. This patch will keep ITS going on to the next
initializaion stage with an explicit warning.

Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index f5bfa42..e8bda0b 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -828,6 +828,11 @@ static int its_alloc_tables(struct its_node *its)
 			u32 ids = GITS_TYPER_DEVBITS(typer);

 			order = get_order((1UL << ids) * entry_size);
+			if (order >= MAX_ORDER) {
+				order = MAX_ORDER - 1;
+				pr_warn("%s: DT size too large, reduce page order to %u\n",
+					its->msi_chip.of_node->full_name, order);
+			}
 		}

 		alloc_size = (1 << order) * PAGE_SIZE;
--
1.8.0

[PATCH v3 3/5] irqchip: gicv3-its: add limitation to page order

[Marc Zyngier]

On 04/03/15 03:18, Yun Wu wrote:
> When required size of Device Table is out of the page allocator's
> capability, the whole ITS will fail in probing. This actually is
> not the hardware's problem and is mainly a limitation of the kernel
> page allocator. This patch will keep ITS going on to the next
> initializaion stage with an explicit warning.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index f5bfa42..e8bda0b 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -828,6 +828,11 @@ static int its_alloc_tables(struct its_node *its)
>  			u32 ids = GITS_TYPER_DEVBITS(typer);
> 
>  			order = get_order((1UL << ids) * entry_size);
> +			if (order >= MAX_ORDER) {
> +				order = MAX_ORDER - 1;
> +				pr_warn("%s: DT size too large, reduce page order to %u\n",

Please spell out device table instead of DT. Most people will interpret
DT as Device Tree, and won't immediately get what is happening.

Thanks,

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

[PATCH v3 4/5] irqchip: gicv3-its: define macros for GITS_CTLR fields

[Yun Wu]

Define macros for GITS_CTLR fields to avoid using magic numbers.

Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 2 +-
 include/linux/irqchip/arm-gic-v3.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e8bda0b..d13c24e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1388,7 +1388,7 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	writeq_relaxed(baser, its->base + GITS_CBASER);
 	tmp = readq_relaxed(its->base + GITS_CBASER);
 	writeq_relaxed(0, its->base + GITS_CWRITER);
-	writel_relaxed(1, its->base + GITS_CTLR);
+	writel_relaxed(GITS_CTLR_ENABLE, its->base + GITS_CTLR);

 	if ((tmp ^ baser) & GITS_BASER_SHAREABILITY_MASK) {
 		pr_info("ITS: using cache flushing for cmd queue\n");
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 3459b43..c9d3002 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -134,6 +134,9 @@

 #define GITS_TRANSLATER			0x10040

+#define GITS_CTLR_ENABLE		(1U << 0)
+#define GITS_CTLR_QUIESCENT		(1U << 31)
+
 #define GITS_TYPER_DEVBITS_SHIFT	13
 #define GITS_TYPER_DEVBITS(r)		((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
 #define GITS_TYPER_PTA			(1UL << 19)
--
1.8.0

[PATCH v3 4/5] irqchip: gicv3-its: define macros for GITS_CTLR fields

[Marc Zyngier]

On 04/03/15 03:18, Yun Wu wrote:
> Define macros for GITS_CTLR fields to avoid using magic numbers.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

> ---
>  drivers/irqchip/irq-gic-v3-its.c   | 2 +-
>  include/linux/irqchip/arm-gic-v3.h | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e8bda0b..d13c24e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1388,7 +1388,7 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
>  	writeq_relaxed(baser, its->base + GITS_CBASER);
>  	tmp = readq_relaxed(its->base + GITS_CBASER);
>  	writeq_relaxed(0, its->base + GITS_CWRITER);
> -	writel_relaxed(1, its->base + GITS_CTLR);
> +	writel_relaxed(GITS_CTLR_ENABLE, its->base + GITS_CTLR);
> 
>  	if ((tmp ^ baser) & GITS_BASER_SHAREABILITY_MASK) {
>  		pr_info("ITS: using cache flushing for cmd queue\n");
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 3459b43..c9d3002 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -134,6 +134,9 @@
> 
>  #define GITS_TRANSLATER			0x10040
> 
> +#define GITS_CTLR_ENABLE		(1U << 0)
> +#define GITS_CTLR_QUIESCENT		(1U << 31)
> +
>  #define GITS_TYPER_DEVBITS_SHIFT	13
>  #define GITS_TYPER_DEVBITS(r)		((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
>  #define GITS_TYPER_PTA			(1UL << 19)
> --
> 1.8.0
> 
> 
> 


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

[PATCH v3 5/5] irqchip: gicv3-its: support safe initialization

[Yun Wu]

It's unsafe to change the configurations of an activated ITS directly
since this will lead to unpredictable results. This patch guarantees
the ITSes being initialized are quiescent.

Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index d13c24e..9e09aa0 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1320,6 +1320,34 @@ static const struct irq_domain_ops its_domain_ops = {
 	.deactivate		= its_irq_domain_deactivate,
 };

+static int its_check_quiesced(void __iomem *base)
+{
+	u32 count = 1000000;	/* 1s */
+	u32 val;
+
+	val = readl_relaxed(base + GITS_CTLR);
+	if (val & GITS_CTLR_QUIESCENT)
+		return 0;
+
+	/* Disable the generation of all interrupts to this ITS */
+	val &= ~GITS_CTLR_ENABLE;
+	writel_relaxed(val, base + GITS_CTLR);
+
+	/* Poll GITS_CTLR and wait until ITS becomes quiescent */
+	while (1) {
+		val = readl_relaxed(base + GITS_CTLR);
+		if (val & GITS_CTLR_QUIESCENT)
+			return 0;
+
+		count--;
+		if (!count)
+			return -EBUSY;
+
+		cpu_relax();
+		udelay(1);
+	}
+}
+
 static int its_probe(struct device_node *node, struct irq_domain *parent)
 {
 	struct resource res;
@@ -1348,6 +1376,13 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 		goto out_unmap;
 	}

+	err = its_check_quiesced(its_base);
+	if (err) {
+		pr_warn("%s: failed to quiesce, giving up\n",
+			node->full_name);
+		goto out_unmap;
+	}
+
 	pr_info("ITS: %s\n", node->full_name);

 	its = kzalloc(sizeof(*its), GFP_KERNEL);
--
1.8.0

[PATCH v3 5/5] irqchip: gicv3-its: support safe initialization

[Marc Zyngier]

On 04/03/15 03:18, Yun Wu wrote:
> It's unsafe to change the configurations of an activated ITS directly
> since this will lead to unpredictable results. This patch guarantees
> the ITSes being initialized are quiescent.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index d13c24e..9e09aa0 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1320,6 +1320,34 @@ static const struct irq_domain_ops its_domain_ops = {
>  	.deactivate		= its_irq_domain_deactivate,
>  };
> 
> +static int its_check_quiesced(void __iomem *base)

Another nitpick: Rather than "its_check_quiesced", how about
"its_force_quiescent" instead? Because this does a lot more than just
checking.

> +{
> +	u32 count = 1000000;	/* 1s */
> +	u32 val;
> +
> +	val = readl_relaxed(base + GITS_CTLR);
> +	if (val & GITS_CTLR_QUIESCENT)
> +		return 0;
> +
> +	/* Disable the generation of all interrupts to this ITS */
> +	val &= ~GITS_CTLR_ENABLE;
> +	writel_relaxed(val, base + GITS_CTLR);
> +
> +	/* Poll GITS_CTLR and wait until ITS becomes quiescent */
> +	while (1) {
> +		val = readl_relaxed(base + GITS_CTLR);
> +		if (val & GITS_CTLR_QUIESCENT)
> +			return 0;
> +
> +		count--;
> +		if (!count)
> +			return -EBUSY;
> +
> +		cpu_relax();
> +		udelay(1);
> +	}
> +}
> +

I still dislike this repeated pattern, but I don't have a good solution
so far.

>  static int its_probe(struct device_node *node, struct irq_domain *parent)
>  {
>  	struct resource res;
> @@ -1348,6 +1376,13 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
>  		goto out_unmap;
>  	}
> 
> +	err = its_check_quiesced(its_base);
> +	if (err) {
> +		pr_warn("%s: failed to quiesce, giving up\n",
> +			node->full_name);
> +		goto out_unmap;
> +	}
> +
>  	pr_info("ITS: %s\n", node->full_name);
> 
>  	its = kzalloc(sizeof(*its), GFP_KERNEL);
> --
> 1.8.0
> 
> 
> 

Assuming you fix the above nitpick:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

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

[PATCH v3 5/5] irqchip: gicv3-its: support safe initialization

[Yun Wu (Abel)]

On 2015/3/5 20:05, Marc Zyngier wrote:

> On 04/03/15 03:18, Yun Wu wrote:
>> It's unsafe to change the configurations of an activated ITS directly
>> since this will lead to unpredictable results. This patch guarantees
>> the ITSes being initialized are quiescent.
>>
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index d13c24e..9e09aa0 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1320,6 +1320,34 @@ static const struct irq_domain_ops its_domain_ops = {
>>  	.deactivate		= its_irq_domain_deactivate,
>>  };
>>
>> +static int its_check_quiesced(void __iomem *base)
> 
> Another nitpick: Rather than "its_check_quiesced", how about
> "its_force_quiescent" instead? Because this does a lot more than just
> checking.

Yes, indeed.

> 
>> +{
>> +	u32 count = 1000000;	/* 1s */
>> +	u32 val;
>> +
>> +	val = readl_relaxed(base + GITS_CTLR);
>> +	if (val & GITS_CTLR_QUIESCENT)
>> +		return 0;
>> +
>> +	/* Disable the generation of all interrupts to this ITS */
>> +	val &= ~GITS_CTLR_ENABLE;
>> +	writel_relaxed(val, base + GITS_CTLR);
>> +
>> +	/* Poll GITS_CTLR and wait until ITS becomes quiescent */
>> +	while (1) {
>> +		val = readl_relaxed(base + GITS_CTLR);
>> +		if (val & GITS_CTLR_QUIESCENT)
>> +			return 0;
>> +
>> +		count--;
>> +		if (!count)
>> +			return -EBUSY;
>> +
>> +		cpu_relax();
>> +		udelay(1);
>> +	}
>> +}
>> +
> 
> I still dislike this repeated pattern, but I don't have a good solution
> so far.

Me too.

> 
>>  static int its_probe(struct device_node *node, struct irq_domain *parent)
>>  {
>>  	struct resource res;
>> @@ -1348,6 +1376,13 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
>>  		goto out_unmap;
>>  	}
>>
>> +	err = its_check_quiesced(its_base);
>> +	if (err) {
>> +		pr_warn("%s: failed to quiesce, giving up\n",
>> +			node->full_name);
>> +		goto out_unmap;
>> +	}
>> +
>>  	pr_info("ITS: %s\n", node->full_name);
>>
>>  	its = kzalloc(sizeof(*its), GFP_KERNEL);
>> --
>> 1.8.0
>>
>>
>>
> 
> Assuming you fix the above nitpick:
> 
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> 

Thanks,
	Abel

[PATCH v3 0/5] enhance configuring an ITS

[Marc Zyngier]

On 04/03/15 03:18, Yun Wu wrote:
> This patch series makes some enhancement to ITS configuration in the
> following aspects:
> 
> o make allocation of the ITS tables more sensible
> o replace magic numbers with sensible macros
> o guarantees a safe quiescent status before initializing an ITS
> 
> This patch series is based on Marc's branch[1], and tested on Hisilion
> ARM64 board with GICv3 ITS hardware.

So this now looks pretty good (assuming you fix the couple of nits I
mentioned).

As this relies on my branch, shall I take it and ask Jason to look at
the branch as a whole?

Thanks,

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

[PATCH v3 0/5] enhance configuring an ITS

[Yun Wu (Abel)]

On 2015/3/5 20:12, Marc Zyngier wrote:

> On 04/03/15 03:18, Yun Wu wrote:
>> This patch series makes some enhancement to ITS configuration in the
>> following aspects:
>>
>> o make allocation of the ITS tables more sensible
>> o replace magic numbers with sensible macros
>> o guarantees a safe quiescent status before initializing an ITS
>>
>> This patch series is based on Marc's branch[1], and tested on Hisilion
>> ARM64 board with GICv3 ITS hardware.
> 
> So this now looks pretty good (assuming you fix the couple of nits I
> mentioned).
> 
> As this relies on my branch, shall I take it and ask Jason to look at
> the branch as a whole?
> 

Yes, please.

Thanks,
	Abel