[PATCH 0/5] enhance configuring an ITS

[PATCH 0/5] enhance configuring an ITS

[Yun Wu]

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

o allocation of the ITS tables
o replacing magic numbers with sensible macros
o supporting ITS power down

This patch series is based on linux 3.19-rc6, and tested on Hisilion
ARM64 board with GICv3 ITS hardware.

Yun Wu (5):
  irqchip: gicv3-its: allocate proper size for DT
  irqchip: gicv3-its: zero itt before handling to hardware
  irqchip: gicv3-its: use 64KB page as default granule
  irqchip: gicv3-its: define macros for GITS_CTLR fields
  irqchip: gicv3-its: add support for power down

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

--
1.8.0

[PATCH 1/5] irqchip: gicv3-its: allocate proper size for DT

[Yun Wu]

A hardware implementation may be designed to search the device
table (DT) using a direct mapping between device ID and memory
address, and in this scenario a single page, currently allocated
for DT in ITS driver, will be probably not enough.

This patch will try best to get this addressed by enlarging DT
size with a limitation of MAX_ORDER pages.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index d8996bd..a391417 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -70,6 +70,7 @@ struct its_node {
 	struct its_collection	*collections;
 	struct list_head	its_device_list;
 	u64			flags;
+	u32			dev_id_bits;
 	u32			ite_size;
 };

@@ -799,6 +800,7 @@ static int its_alloc_tables(struct its_node *its)
 {
 	int err;
 	int i;
+	int size;
 	int psz = PAGE_SIZE;
 	u64 shr = GITS_BASER_InnerShareable;

@@ -812,8 +814,13 @@ static int its_alloc_tables(struct its_node *its)
 		if (type == GITS_BASER_TYPE_NONE)
 			continue;

-		/* We're lazy and only allocate a single page for now */
-		base = (void *)get_zeroed_page(GFP_KERNEL);
+		if (type == GITS_BASER_TYPE_DEVICE)
+			size = 1 << its->dev_id_bits;
+		else
+			size = PAGE_SIZE;
+
+		base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+						get_order(size / PAGE_SIZE));
 		if (!base) {
 			err = -ENOMEM;
 			goto out_free;
@@ -841,7 +848,7 @@ retry_baser:
 			break;
 		}

-		val |= (PAGE_SIZE / psz) - 1;
+		val |= (size / psz) - 1;

 		writeq_relaxed(val, its->base + GITS_BASER + i * 8);
 		tmp = readq_relaxed(its->base + GITS_BASER + i * 8);
@@ -1261,7 +1268,7 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	struct its_node *its;
 	void __iomem *its_base;
 	u32 val;
-	u64 baser, tmp;
+	u64 baser, typer, tmp;
 	int err;

 	err = of_address_to_resource(node, 0, &res);
@@ -1297,7 +1304,15 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	its->base = its_base;
 	its->phys_base = res.start;
 	its->msi_chip.of_node = node;
-	its->ite_size = ((readl_relaxed(its_base + GITS_TYPER) >> 4) & 0xf) + 1;
+
+	typer = readq_relaxed(its_base + GITS_TYPER);
+	its->ite_size = ((typer >> 4) & 0xf) + 1;
+	its->dev_id_bits = ((typer >> 13) & 0x1f) + 1;
+	if (its->dev_id_bits > KMALLOC_SHIFT_MAX) {
+		pr_warn("%s: DT size too large (%ubits -> %ubits)\n",
+			node->full_name, its->dev_id_bits, KMALLOC_SHIFT_MAX);
+		its->dev_id_bits = KMALLOC_SHIFT_MAX;
+	}

 	its->cmd_base = kzalloc(ITS_CMD_QUEUE_SZ, GFP_KERNEL);
 	if (!its->cmd_base) {
--
1.8.0

[PATCH 1/5] irqchip: gicv3-its: allocate proper size for DT

[Marc Zyngier]

On 30/01/15 07:46, Yun Wu wrote:
> A hardware implementation may be designed to search the device
> table (DT) using a direct mapping between device ID and memory
> address, and in this scenario a single page, currently allocated
> for DT in ITS driver, will be probably not enough.
> 
> This patch will try best to get this addressed by enlarging DT
> size with a limitation of MAX_ORDER pages.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>

A similar patch has been posted already (and is already in my queue):

https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-fixes&id=4be3de2af2a58476f84d678f3e8a3596f23f80d5

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

[PATCH 1/5] irqchip: gicv3-its: allocate proper size for DT

[Yun Wu (Abel)]

On 2015/1/31 3:10, Marc Zyngier wrote:

> On 30/01/15 07:46, Yun Wu wrote:
>> A hardware implementation may be designed to search the device
>> table (DT) using a direct mapping between device ID and memory
>> address, and in this scenario a single page, currently allocated
>> for DT in ITS driver, will be probably not enough.
>>
>> This patch will try best to get this addressed by enlarging DT
>> size with a limitation of MAX_ORDER pages.
>>
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> 
> A similar patch has been posted already (and is already in my queue):
> 
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-fixes&id=4be3de2af2a58476f84d678f3e8a3596f23f80d5
> 

Oh, now I see it. How about allocating a order of MAX_ORDER pages and
throwing out a warning if the number of device id bits exceeds maximum
order kernel supports, instead of letting the ITS fail in probing.

Thanks,
	Abel

[PATCH 2/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.

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 a391417..2a08d85 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1063,7 +1063,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 2/5] irqchip: gicv3-its: zero itt before handling to hardware

[Marc Zyngier]

On 30/01/15 07:46, Yun Wu wrote:
> 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.
> 
> 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 a391417..2a08d85 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1063,7 +1063,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) {

Fair enough. I suppose this cannot really hurt if we have stupid HW around.

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

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

[PATCH 3/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.

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 2a08d85..430bc92 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -801,7 +801,7 @@ static int its_alloc_tables(struct its_node *its)
 	int err;
 	int i;
 	int size;
-	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 3/5] irqchip: gicv3-its: use 64KB page as default granule

[Marc Zyngier]

On 30/01/15 07:46, Yun Wu wrote:
> 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.

Do you actually know of an implementation with such a behaviour?

> 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 2a08d85..430bc92 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -801,7 +801,7 @@ static int its_alloc_tables(struct its_node *its)
>  	int err;
>  	int i;
>  	int size;
> -	int psz = PAGE_SIZE;
> +	int psz = SZ_64K;
>  	u64 shr = GITS_BASER_InnerShareable;
> 
>  	for (i = 0; i < GITS_BASER_NR_REGS; i++) {

Assuming such an implementation exists, you'll have to rebase this on
top of the patch I mentioned in my reply to patch #1.

Thanks,

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

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

[Yun Wu (Abel)]

On 2015/1/31 3:18, Marc Zyngier wrote:

> On 30/01/15 07:46, Yun Wu wrote:
>> 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.
> 
> Do you actually know of an implementation with such a behaviour?

Yes, Hisilicon implemented a fixed page size of 16KB.

> 
>> 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 2a08d85..430bc92 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -801,7 +801,7 @@ static int its_alloc_tables(struct its_node *its)
>>  	int err;
>>  	int i;
>>  	int size;
>> -	int psz = PAGE_SIZE;
>> +	int psz = SZ_64K;
>>  	u64 shr = GITS_BASER_InnerShareable;
>>
>>  	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> 
> Assuming such an implementation exists, you'll have to rebase this on
> top of the patch I mentioned in my reply to patch #1.
> 

OK, I will.

Thanks,
	Abel

[PATCH 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 430bc92..facf6d6 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1338,7 +1338,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 1e8b0cf..8c8ce25 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_PTA			(1UL << 19)

 #define GITS_CBASER_VALID		(1UL << 63)
--
1.8.0

[PATCH 5/5] irqchip: gicv3-its: add support for power down

[Yun Wu]

Configurations of an ITS cannot be changed if the ITS is in an
active status, so it might not be safe to perform a soft reboot
with all the active ITSes un-disabled, etc. kexec.

This patch will make sure all the active ITSes disabled before
enabling them again without resetting ITS hardware.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index facf6d6..1d85471 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -29,6 +29,7 @@
 #include <linux/of_platform.h>
 #include <linux/percpu.h>
 #include <linux/slab.h>
+#include <linux/reboot.h>

 #include <linux/irqchip/arm-gic-v3.h>

@@ -1410,6 +1411,63 @@ int its_cpu_init(void)
 	return 0;
 }

+static void its_disable(struct its_node *its)
+{
+	u32 count = 1000000;	/* 1s */
+	u32 val;
+
+	/* Disable the generation of all interrupts to this ITS */
+	val = readl_relaxed(its->base + GITS_CTLR);
+	val &= ~GITS_CTLR_ENABLE;
+	writel_relaxed(val, its->base + GITS_CTLR);
+
+	/* Poll GITS_CTLR and wait until ITS becomes quiescent */
+	while (count--) {
+		val = readl_relaxed(its->base + GITS_CTLR);
+		if (val & GITS_CTLR_QUIESCENT)
+			break;
+		cpu_relax();
+		udelay(1);
+	}
+
+	if (!count)
+		pr_err("%s: failed to shutdown!\n",
+		       its->msi_chip.of_node->full_name);
+
+	/*
+	 * Release all resources of this ITS node to completely put
+	 * an end to it. Note that this step may not be necessary
+	 * in some cases, but leaving it here does no harm.
+	 */
+	irq_domain_remove(its->msi_chip.domain);
+	irq_domain_remove(its->domain);
+	its_free_tables(its);
+	kfree(its->cmd_base);
+	iounmap(its->base);
+
+	spin_lock(&its_lock);
+	list_del(&its->entry);
+	spin_unlock(&its_lock);
+	kfree(its);
+}
+
+static int its_shutdown(struct notifier_block *nfb, unsigned long val, void *v)
+{
+	struct its_node *its, *tmp;
+
+	list_for_each_entry_safe(its, tmp, &its_nodes, entry)
+		its_disable(its);
+
+	kfree(gic_rdists->prop_page);
+	kfree(lpi_bitmap);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block its_reboot_notifier = {
+	.notifier_call	= its_shutdown,
+};
+
 static struct of_device_id its_device_id[] = {
 	{	.compatible	= "arm,gic-v3-its",	},
 	{},
@@ -1435,6 +1493,7 @@ int its_init(struct device_node *node, struct rdists *rdists,

 	its_alloc_lpi_tables();
 	its_lpi_init(rdists->id_bits);
+	register_reboot_notifier(&its_reboot_notifier);

 	return 0;
 }
--
1.8.0

[PATCH 5/5] irqchip: gicv3-its: add support for power down

[Marc Zyngier]

On 30/01/15 07:46, Yun Wu wrote:
> Configurations of an ITS cannot be changed if the ITS is in an
> active status, so it might not be safe to perform a soft reboot
> with all the active ITSes un-disabled, etc. kexec.
> 
> This patch will make sure all the active ITSes disabled before
> enabling them again without resetting ITS hardware.

And what happens if the kernel crashes or gets rebooted from a watchdog?
Or if the bootloader messes things up? The ITS is in an unknown state
when we start again.

Wouldn't it be better to address this instead? Enforcing an safe initial
state seems a better solution that relying on mechanisms that may not be
relevant for all cases.

Thanks,

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

[PATCH 5/5] irqchip: gicv3-its: add support for power down

[Yun Wu (Abel)]

On 2015/1/31 3:23, Marc Zyngier wrote:

> On 30/01/15 07:46, Yun Wu wrote:
>> Configurations of an ITS cannot be changed if the ITS is in an
>> active status, so it might not be safe to perform a soft reboot
>> with all the active ITSes un-disabled, etc. kexec.
>>
>> This patch will make sure all the active ITSes disabled before
>> enabling them again without resetting ITS hardware.
> 
> And what happens if the kernel crashes or gets rebooted from a watchdog?
> Or if the bootloader messes things up? The ITS is in an unknown state
> when we start again.
> 
> Wouldn't it be better to address this instead? Enforcing an safe initial
> state seems a better solution that relying on mechanisms that may not be
> relevant for all cases.
> 

Sure, checking the ITS state before initializing it is really a better
solution, I will rewrite this patch and test again.

Thanks,
	Abel