[PATCH v2 0/6] enhance configuring an ITS

[PATCH v2 0/6] enhance configuring an ITS

[Yun Wu]

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

o allocation of the ITS tables
o replacing magic numbers with sensible macros
o guarantees a safe quiescent status before initializing an ITS
o judging enabling status of LPI feature

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

v1 -> v2:
o rebase to Marc's GIC fix branch
o drop DT size calculation part 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 (6):
  irqchip: gicv3-its: zero itt before handling to hardware
  irqchip: gicv3-its: use 64KB page as default granule
  irqchip: gicv3-its: limit order of DT size to MAX_ORDER
  irqchip: gicv3-its: define macros for GITS_CTLR fields
  irqchip: gicv3-its: add support for power down
  irqchip: gicv3: skip ITS init when no ITS available

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

--
1.8.0

[PATCH v2 0/6] enhance configuring an ITS

[Yun Wu]

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

o allocation of the ITS tables
o replacing magic numbers with sensible macros
o guarantees a safe quiescent status before initializing an ITS
o judging enabling status of LPI feature

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

v1 -> v2:
o rebase to Marc's GIC fix branch
o drop DT size calculation part 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 (6):
  irqchip: gicv3-its: zero itt before handling to hardware
  irqchip: gicv3-its: use 64KB page as default granule
  irqchip: gicv3-its: limit order of DT size to MAX_ORDER
  irqchip: gicv3-its: define macros for GITS_CTLR fields
  irqchip: gicv3-its: add support for power down
  irqchip: gicv3: skip ITS init when no ITS available

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

--
1.8.0

[PATCH v2 1/6] 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 v2 1/6] 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 v2 2/6] 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 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 v2 2/6] 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 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 v2 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER

[Yun Wu]

When required DT size is out of the kmalloc()'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 memory allocator. This patch
will keep ITS going on to the next initializaion step with an explicit
warning.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index f5bfa42..de36606 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -828,6 +828,12 @@ 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) {
+				pr_warn("%s: DT size too large, reduce to %u pages\n",
+					its->msi_chip.of_node->full_name,
+					1 << order);
+				order = MAX_ORDER;
+			}
 		}

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

[PATCH v2 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER

[Yun Wu]

When required DT size is out of the kmalloc()'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 memory allocator. This patch
will keep ITS going on to the next initializaion step with an explicit
warning.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index f5bfa42..de36606 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -828,6 +828,12 @@ 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) {
+				pr_warn("%s: DT size too large, reduce to %u pages\n",
+					its->msi_chip.of_node->full_name,
+					1 << order);
+				order = MAX_ORDER;
+			}
 		}

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

[PATCH v2 4/6] 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 de36606..42c03b2 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1389,7 +1389,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 v2 4/6] 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 de36606..42c03b2 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1389,7 +1389,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 v2 5/6] irqchip: gicv3-its: add support for power down

[Yun Wu]

It's unsafe to change the configurations of an activated ITS directly
since this will lead to unpredictable results. This patch guarantees
a safe quiescent status before initializing an ITS.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 42c03b2..29eb665 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1321,6 +1321,31 @@ 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 (count--) {
+		val = readl_relaxed(base + GITS_CTLR);
+		if (val & GITS_CTLR_QUIESCENT)
+			return 0;
+		cpu_relax();
+		udelay(1);
+	}
+
+	return -EBUSY;
+}
+
 static int its_probe(struct device_node *node, struct irq_domain *parent)
 {
 	struct resource res;
@@ -1349,6 +1374,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, behaviour unpredictable\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 v2 5/6] irqchip: gicv3-its: add support for power down

[Yun Wu]

It's unsafe to change the configurations of an activated ITS directly
since this will lead to unpredictable results. This patch guarantees
a safe quiescent status before initializing an ITS.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 42c03b2..29eb665 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1321,6 +1321,31 @@ 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 (count--) {
+		val = readl_relaxed(base + GITS_CTLR);
+		if (val & GITS_CTLR_QUIESCENT)
+			return 0;
+		cpu_relax();
+		udelay(1);
+	}
+
+	return -EBUSY;
+}
+
 static int its_probe(struct device_node *node, struct irq_domain *parent)
 {
 	struct resource res;
@@ -1349,6 +1374,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, behaviour unpredictable\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 v2 6/6] irqchip: gicv3: skip ITS init when no ITS available

[Yun Wu]

There is one more condition that needs to be considered when judging
whether LPI feature is enabled or not, which is whether there is any
ITS available and correctly enabled.

This patch will fix this by caching ITS enabling status in the GIC
chip data structure.

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

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 1a146cc..e17faca 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -47,6 +47,7 @@ struct gic_chip_data {
 	u64			redist_stride;
 	u32			nr_redist_regions;
 	unsigned int		irq_nr;
+	int			lpi_enabled;
 };

 static struct gic_chip_data gic_data __read_mostly;
@@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
 	gic_write_grpen1(1);
 }

-static int gic_dist_supports_lpis(void)
-{
-	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS);
-}
-
 static void gic_cpu_init(void)
 {
 	void __iomem *rbase;
@@ -410,7 +406,7 @@ static void gic_cpu_init(void)
 	gic_cpu_config(rbase, gic_redist_wait_for_rwp);

 	/* Give LPIs a spin */
-	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
+	if (gic_data.lpi_enabled)
 		its_cpu_init();

 	/* initialise system registers */
@@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 	}
 	/* LPIs */
 	if (hw >= 8192 && hw < GIC_ID_NR) {
-		if (!gic_dist_supports_lpis())
+		if (!gic_data.lpi_enabled)
 			return -EPERM;
 		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
@@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare

 	set_handle_irq(gic_handle_irq);

-	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
-		its_init(node, &gic_data.rdists, gic_data.domain);
+	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
+	    !!(readl_relaxed(dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
+	    !its_init(node, &gic_data.rdists, gic_data.domain))
+		gic_data.lpi_enabled = 1;
+	else
+		gic_data.lpi_enabled = 0;

 	gic_smp_init();
 	gic_dist_init();
--
1.8.0

[PATCH v2 6/6] irqchip: gicv3: skip ITS init when no ITS available

[Yun Wu]

There is one more condition that needs to be considered when judging
whether LPI feature is enabled or not, which is whether there is any
ITS available and correctly enabled.

This patch will fix this by caching ITS enabling status in the GIC
chip data structure.

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

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 1a146cc..e17faca 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -47,6 +47,7 @@ struct gic_chip_data {
 	u64			redist_stride;
 	u32			nr_redist_regions;
 	unsigned int		irq_nr;
+	int			lpi_enabled;
 };

 static struct gic_chip_data gic_data __read_mostly;
@@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
 	gic_write_grpen1(1);
 }

-static int gic_dist_supports_lpis(void)
-{
-	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS);
-}
-
 static void gic_cpu_init(void)
 {
 	void __iomem *rbase;
@@ -410,7 +406,7 @@ static void gic_cpu_init(void)
 	gic_cpu_config(rbase, gic_redist_wait_for_rwp);

 	/* Give LPIs a spin */
-	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
+	if (gic_data.lpi_enabled)
 		its_cpu_init();

 	/* initialise system registers */
@@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 	}
 	/* LPIs */
 	if (hw >= 8192 && hw < GIC_ID_NR) {
-		if (!gic_dist_supports_lpis())
+		if (!gic_data.lpi_enabled)
 			return -EPERM;
 		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
@@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare

 	set_handle_irq(gic_handle_irq);

-	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
-		its_init(node, &gic_data.rdists, gic_data.domain);
+	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
+	    !!(readl_relaxed(dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
+	    !its_init(node, &gic_data.rdists, gic_data.domain))
+		gic_data.lpi_enabled = 1;
+	else
+		gic_data.lpi_enabled = 0;

 	gic_smp_init();
 	gic_dist_init();
--
1.8.0

Re: [PATCH v2 6/6] irqchip: gicv3: skip ITS init when no ITS available

[Vladimir Murzin]

Hi Yun,

On 15/02/15 09:32, Yun Wu wrote:
> There is one more condition that needs to be considered when judging
> whether LPI feature is enabled or not, which is whether there is any
> ITS available and correctly enabled.
> 
> This patch will fix this by caching ITS enabling status in the GIC
> chip data structure.

I posted patch for that before [1] and it landed in Marc's tree
(irq/gic-fixes). It is not clear from the commit message what the "one
more condition" is, but I guess it is the same dts stuff. Do you see
issue without your patch applied?

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314752.html

Thanks
Vladimir

> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 1a146cc..e17faca 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -47,6 +47,7 @@ struct gic_chip_data {
>  	u64			redist_stride;
>  	u32			nr_redist_regions;
>  	unsigned int		irq_nr;
> +	int			lpi_enabled;
>  };
> 
>  static struct gic_chip_data gic_data __read_mostly;
> @@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
>  	gic_write_grpen1(1);
>  }
> 
> -static int gic_dist_supports_lpis(void)
> -{
> -	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS);
> -}
> -
>  static void gic_cpu_init(void)
>  {
>  	void __iomem *rbase;
> @@ -410,7 +406,7 @@ static void gic_cpu_init(void)
>  	gic_cpu_config(rbase, gic_redist_wait_for_rwp);
> 
>  	/* Give LPIs a spin */
> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
> +	if (gic_data.lpi_enabled)
>  		its_cpu_init();
> 
>  	/* initialise system registers */
> @@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>  	}
>  	/* LPIs */
>  	if (hw >= 8192 && hw < GIC_ID_NR) {
> -		if (!gic_dist_supports_lpis())
> +		if (!gic_data.lpi_enabled)
>  			return -EPERM;
>  		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>  				    handle_fasteoi_irq, NULL, NULL);
> @@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
> 
>  	set_handle_irq(gic_handle_irq);
> 
> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
> -		its_init(node, &gic_data.rdists, gic_data.domain);
> +	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
> +	    !!(readl_relaxed(dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
> +	    !its_init(node, &gic_data.rdists, gic_data.domain))
> +		gic_data.lpi_enabled = 1;
> +	else
> +		gic_data.lpi_enabled = 0;
> 
>  	gic_smp_init();
>  	gic_dist_init();
> --
> 1.8.0
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 

[PATCH v2 6/6] irqchip: gicv3: skip ITS init when no ITS available

[Vladimir Murzin]

Hi Yun,

On 15/02/15 09:32, Yun Wu wrote:
> There is one more condition that needs to be considered when judging
> whether LPI feature is enabled or not, which is whether there is any
> ITS available and correctly enabled.
> 
> This patch will fix this by caching ITS enabling status in the GIC
> chip data structure.

I posted patch for that before [1] and it landed in Marc's tree
(irq/gic-fixes). It is not clear from the commit message what the "one
more condition" is, but I guess it is the same dts stuff. Do you see
issue without your patch applied?

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314752.html

Thanks
Vladimir

> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 1a146cc..e17faca 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -47,6 +47,7 @@ struct gic_chip_data {
>  	u64			redist_stride;
>  	u32			nr_redist_regions;
>  	unsigned int		irq_nr;
> +	int			lpi_enabled;
>  };
> 
>  static struct gic_chip_data gic_data __read_mostly;
> @@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
>  	gic_write_grpen1(1);
>  }
> 
> -static int gic_dist_supports_lpis(void)
> -{
> -	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS);
> -}
> -
>  static void gic_cpu_init(void)
>  {
>  	void __iomem *rbase;
> @@ -410,7 +406,7 @@ static void gic_cpu_init(void)
>  	gic_cpu_config(rbase, gic_redist_wait_for_rwp);
> 
>  	/* Give LPIs a spin */
> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
> +	if (gic_data.lpi_enabled)
>  		its_cpu_init();
> 
>  	/* initialise system registers */
> @@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>  	}
>  	/* LPIs */
>  	if (hw >= 8192 && hw < GIC_ID_NR) {
> -		if (!gic_dist_supports_lpis())
> +		if (!gic_data.lpi_enabled)
>  			return -EPERM;
>  		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>  				    handle_fasteoi_irq, NULL, NULL);
> @@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
> 
>  	set_handle_irq(gic_handle_irq);
> 
> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
> -		its_init(node, &gic_data.rdists, gic_data.domain);
> +	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
> +	    !!(readl_relaxed(dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
> +	    !its_init(node, &gic_data.rdists, gic_data.domain))
> +		gic_data.lpi_enabled = 1;
> +	else
> +		gic_data.lpi_enabled = 0;
> 
>  	gic_smp_init();
>  	gic_dist_init();
> --
> 1.8.0
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 

Re: [PATCH v2 6/6] irqchip: gicv3: skip ITS init when no ITS available

[Yun Wu (Abel)]

Hi Murzin,
On 2015/2/16 18:05, Vladimir Murzin wrote:

> Hi Yun,
> 
> On 15/02/15 09:32, Yun Wu wrote:
>> There is one more condition that needs to be considered when judging
>> whether LPI feature is enabled or not, which is whether there is any
>> ITS available and correctly enabled.
>>
>> This patch will fix this by caching ITS enabling status in the GIC
>> chip data structure.
> 
> I posted patch for that before [1] and it landed in Marc's tree
> (irq/gic-fixes). It is not clear from the commit message what the "one
> more condition" is, but I guess it is the same dts stuff. Do you see
> issue without your patch applied?

Oh yes, your patch perfectly solved this problem, so this one is no longer
needed. And sorry for not noticing your patch. :)

Thanks,
	Abel

> 
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314752.html
> 
> Thanks
> Vladimir
> 
>>
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 1a146cc..e17faca 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -47,6 +47,7 @@ struct gic_chip_data {
>>  	u64			redist_stride;
>>  	u32			nr_redist_regions;
>>  	unsigned int		irq_nr;
>> +	int			lpi_enabled;
>>  };
>>
>>  static struct gic_chip_data gic_data __read_mostly;
>> @@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
>>  	gic_write_grpen1(1);
>>  }
>>
>> -static int gic_dist_supports_lpis(void)
>> -{
>> -	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS);
>> -}
>> -
>>  static void gic_cpu_init(void)
>>  {
>>  	void __iomem *rbase;
>> @@ -410,7 +406,7 @@ static void gic_cpu_init(void)
>>  	gic_cpu_config(rbase, gic_redist_wait_for_rwp);
>>
>>  	/* Give LPIs a spin */
>> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>> +	if (gic_data.lpi_enabled)
>>  		its_cpu_init();
>>
>>  	/* initialise system registers */
>> @@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>>  	}
>>  	/* LPIs */
>>  	if (hw >= 8192 && hw < GIC_ID_NR) {
>> -		if (!gic_dist_supports_lpis())
>> +		if (!gic_data.lpi_enabled)
>>  			return -EPERM;
>>  		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>>  				    handle_fasteoi_irq, NULL, NULL);
>> @@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>
>>  	set_handle_irq(gic_handle_irq);
>>
>> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>> -		its_init(node, &gic_data.rdists, gic_data.domain);
>> +	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
>> +	    !!(readl_relaxed(dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
>> +	    !its_init(node, &gic_data.rdists, gic_data.domain))
>> +		gic_data.lpi_enabled = 1;
>> +	else
>> +		gic_data.lpi_enabled = 0;
>>
>>  	gic_smp_init();
>>  	gic_dist_init();
>> --
>> 1.8.0
>>

[PATCH v2 6/6] irqchip: gicv3: skip ITS init when no ITS available

[Yun Wu (Abel)]

Hi Murzin,
On 2015/2/16 18:05, Vladimir Murzin wrote:

> Hi Yun,
> 
> On 15/02/15 09:32, Yun Wu wrote:
>> There is one more condition that needs to be considered when judging
>> whether LPI feature is enabled or not, which is whether there is any
>> ITS available and correctly enabled.
>>
>> This patch will fix this by caching ITS enabling status in the GIC
>> chip data structure.
> 
> I posted patch for that before [1] and it landed in Marc's tree
> (irq/gic-fixes). It is not clear from the commit message what the "one
> more condition" is, but I guess it is the same dts stuff. Do you see
> issue without your patch applied?

Oh yes, your patch perfectly solved this problem, so this one is no longer
needed. And sorry for not noticing your patch. :)

Thanks,
	Abel

> 
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314752.html
> 
> Thanks
> Vladimir
> 
>>
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 1a146cc..e17faca 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -47,6 +47,7 @@ struct gic_chip_data {
>>  	u64			redist_stride;
>>  	u32			nr_redist_regions;
>>  	unsigned int		irq_nr;
>> +	int			lpi_enabled;
>>  };
>>
>>  static struct gic_chip_data gic_data __read_mostly;
>> @@ -390,11 +391,6 @@ static void gic_cpu_sys_reg_init(void)
>>  	gic_write_grpen1(1);
>>  }
>>
>> -static int gic_dist_supports_lpis(void)
>> -{
>> -	return !!(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_LPIS);
>> -}
>> -
>>  static void gic_cpu_init(void)
>>  {
>>  	void __iomem *rbase;
>> @@ -410,7 +406,7 @@ static void gic_cpu_init(void)
>>  	gic_cpu_config(rbase, gic_redist_wait_for_rwp);
>>
>>  	/* Give LPIs a spin */
>> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>> +	if (gic_data.lpi_enabled)
>>  		its_cpu_init();
>>
>>  	/* initialise system registers */
>> @@ -629,7 +625,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>>  	}
>>  	/* LPIs */
>>  	if (hw >= 8192 && hw < GIC_ID_NR) {
>> -		if (!gic_dist_supports_lpis())
>> +		if (!gic_data.lpi_enabled)
>>  			return -EPERM;
>>  		irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>>  				    handle_fasteoi_irq, NULL, NULL);
>> @@ -785,8 +781,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>
>>  	set_handle_irq(gic_handle_irq);
>>
>> -	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>> -		its_init(node, &gic_data.rdists, gic_data.domain);
>> +	if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) &&
>> +	    !!(readl_relaxed(dist_base + GICD_TYPER) & GICD_TYPER_LPIS) &&
>> +	    !its_init(node, &gic_data.rdists, gic_data.domain))
>> +		gic_data.lpi_enabled = 1;
>> +	else
>> +		gic_data.lpi_enabled = 0;
>>
>>  	gic_smp_init();
>>  	gic_dist_init();
>> --
>> 1.8.0
>>

Re: [PATCH v2 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER

[Marc Zyngier]

On Sun, 15 Feb 2015 09:32:00 +0000
Yun Wu <wuyun.wu@huawei.com> wrote:

> When required DT size is out of the kmalloc()'s capability, the whole

Nit: Using the DT acronym is very confusing here, as it means "Device
Tree" to most people, including me. Please use "Device Table" instead.

Also, the reference to kmalloc is wrong, as we're really using the page
allocator.

> ITS will fail in probing. This actually is not the hardware's problem
> and is mainly a limitation of the kernel memory allocator. This patch
> will keep ITS going on to the next initializaion step with an explicit
> warning.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c index f5bfa42..de36606 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -828,6 +828,12 @@ 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) {
> +				pr_warn("%s: DT size too large,
> reduce to %u pages\n",
> +
> its->msi_chip.of_node->full_name,
> +					1 << order);
> +				order = MAX_ORDER;
> +			}
>  		}

Something is wrong here. Either (order == MAX_ORDER) is acceptable, or
it is not. Also, your warning says that you're reducing the size to a
new value, but you're displaying the size you can't handle. That
message should be fixed.

Thanks,

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

[PATCH v2 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER

[Marc Zyngier]

On Sun, 15 Feb 2015 09:32:00 +0000
Yun Wu <wuyun.wu@huawei.com> wrote:

> When required DT size is out of the kmalloc()'s capability, the whole

Nit: Using the DT acronym is very confusing here, as it means "Device
Tree" to most people, including me. Please use "Device Table" instead.

Also, the reference to kmalloc is wrong, as we're really using the page
allocator.

> ITS will fail in probing. This actually is not the hardware's problem
> and is mainly a limitation of the kernel memory allocator. This patch
> will keep ITS going on to the next initializaion step with an explicit
> warning.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c index f5bfa42..de36606 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -828,6 +828,12 @@ 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) {
> +				pr_warn("%s: DT size too large,
> reduce to %u pages\n",
> +
> its->msi_chip.of_node->full_name,
> +					1 << order);
> +				order = MAX_ORDER;
> +			}
>  		}

Something is wrong here. Either (order == MAX_ORDER) is acceptable, or
it is not. Also, your warning says that you're reducing the size to a
new value, but you're displaying the size you can't handle. That
message should be fixed.

Thanks,

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

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

[Marc Zyngier]

On Sun, 15 Feb 2015 09:32:02 +0000
Yun Wu <wuyun.wu@huawei.com> wrote:

> It's unsafe to change the configurations of an activated ITS directly
> since this will lead to unpredictable results. This patch guarantees
> a safe quiescent status before initializing an ITS.

Please change the title of this patch to reflect what it actually
does. Nothing here is about powering down anything.

> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 32
> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1321,6 +1321,31 @@ 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 (count--) {
> +		val = readl_relaxed(base + GITS_CTLR);
> +		if (val & GITS_CTLR_QUIESCENT)
> +			return 0;
> +		cpu_relax();
> +		udelay(1);
> +	}

You're now introducing a third variant of a 1s timeout loop. Notice a
pattern?

Thanks,

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

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

[Marc Zyngier]

On Sun, 15 Feb 2015 09:32:02 +0000
Yun Wu <wuyun.wu@huawei.com> wrote:

> It's unsafe to change the configurations of an activated ITS directly
> since this will lead to unpredictable results. This patch guarantees
> a safe quiescent status before initializing an ITS.

Please change the title of this patch to reflect what it actually
does. Nothing here is about powering down anything.

> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 32
> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1321,6 +1321,31 @@ 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 (count--) {
> +		val = readl_relaxed(base + GITS_CTLR);
> +		if (val & GITS_CTLR_QUIESCENT)
> +			return 0;
> +		cpu_relax();
> +		udelay(1);
> +	}

You're now introducing a third variant of a 1s timeout loop. Notice a
pattern?

Thanks,

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

Re: [PATCH v2 2/6] irqchip: gicv3-its: use 64KB page as default granule

[Marc Zyngier]

On Sun, 15 Feb 2015 09:31:59 +0000
Yun Wu <wuyun.wu@huawei.com> 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.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.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
> 
> 
> 



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

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

[Marc Zyngier]

On Sun, 15 Feb 2015 09:31:59 +0000
Yun Wu <wuyun.wu@huawei.com> 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.
> 
> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.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
> 
> 
> 



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

Re: [PATCH v2 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER

[Yun Wu (Abel)]

On 2015/2/17 17:19, Marc Zyngier wrote:

> On Sun, 15 Feb 2015 09:32:00 +0000
> Yun Wu <wuyun.wu@huawei.com> wrote:
> 
>> When required DT size is out of the kmalloc()'s capability, the whole
> 
> Nit: Using the DT acronym is very confusing here, as it means "Device
> Tree" to most people, including me. Please use "Device Table" instead.
> 
> Also, the reference to kmalloc is wrong, as we're really using the page
> allocator.

OK, I will get the description fixed in the next version.

> 
>> ITS will fail in probing. This actually is not the hardware's problem
>> and is mainly a limitation of the kernel memory allocator. This patch
>> will keep ITS going on to the next initializaion step with an explicit
>> warning.
>>
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c index f5bfa42..de36606 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -828,6 +828,12 @@ 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) {
>> +				pr_warn("%s: DT size too large,
>> reduce to %u pages\n",
>> +
>> its->msi_chip.of_node->full_name,
>> +					1 << order);
>> +				order = MAX_ORDER;
>> +			}
>>  		}
> 
> Something is wrong here. Either (order == MAX_ORDER) is acceptable, or
> it is not. Also, your warning says that you're reducing the size to a
> new value, but you're displaying the size you can't handle. That
> message should be fixed.
> 

Yes, my fault, I will fix them.

Thanks,
	Abel

[PATCH v2 3/6] irqchip: gicv3-its: limit order of DT size to MAX_ORDER

[Yun Wu (Abel)]

On 2015/2/17 17:19, Marc Zyngier wrote:

> On Sun, 15 Feb 2015 09:32:00 +0000
> Yun Wu <wuyun.wu@huawei.com> wrote:
> 
>> When required DT size is out of the kmalloc()'s capability, the whole
> 
> Nit: Using the DT acronym is very confusing here, as it means "Device
> Tree" to most people, including me. Please use "Device Table" instead.
> 
> Also, the reference to kmalloc is wrong, as we're really using the page
> allocator.

OK, I will get the description fixed in the next version.

> 
>> ITS will fail in probing. This actually is not the hardware's problem
>> and is mainly a limitation of the kernel memory allocator. This patch
>> will keep ITS going on to the next initializaion step with an explicit
>> warning.
>>
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c index f5bfa42..de36606 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -828,6 +828,12 @@ 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) {
>> +				pr_warn("%s: DT size too large,
>> reduce to %u pages\n",
>> +
>> its->msi_chip.of_node->full_name,
>> +					1 << order);
>> +				order = MAX_ORDER;
>> +			}
>>  		}
> 
> Something is wrong here. Either (order == MAX_ORDER) is acceptable, or
> it is not. Also, your warning says that you're reducing the size to a
> new value, but you're displaying the size you can't handle. That
> message should be fixed.
> 

Yes, my fault, I will fix them.

Thanks,
	Abel

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

[Yun Wu (Abel)]

On 2015/2/17 17:29, Marc Zyngier wrote:

> On Sun, 15 Feb 2015 09:32:02 +0000
> Yun Wu <wuyun.wu@huawei.com> wrote:
> 
>> It's unsafe to change the configurations of an activated ITS directly
>> since this will lead to unpredictable results. This patch guarantees
>> a safe quiescent status before initializing an ITS.
> 
> Please change the title of this patch to reflect what it actually
> does. Nothing here is about powering down anything.

My miss, I will fix this in next version.

> 
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 32
>> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1321,6 +1321,31 @@ 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 (count--) {
>> +		val = readl_relaxed(base + GITS_CTLR);
>> +		if (val & GITS_CTLR_QUIESCENT)
>> +			return 0;
>> +		cpu_relax();
>> +		udelay(1);
>> +	}
> 
> You're now introducing a third variant of a 1s timeout loop. Notice a
> pattern?
> 

I am not sure I know exactly what you suggest. Do you mean I should code
like below to keep the coding style same as the other 2 loops?

	while (1) {
		val = readl_relaxed(base + GITS_CTLR);
		if (val & GITS_CTLR_QUIESCENT)
			return 0;

		count--;
		if (!count)
			return -EBUSY;

		cpu_relax();
		udelay(1);
	}

Thanks,
	Abel

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

[Yun Wu (Abel)]

On 2015/2/17 17:29, Marc Zyngier wrote:

> On Sun, 15 Feb 2015 09:32:02 +0000
> Yun Wu <wuyun.wu@huawei.com> wrote:
> 
>> It's unsafe to change the configurations of an activated ITS directly
>> since this will lead to unpredictable results. This patch guarantees
>> a safe quiescent status before initializing an ITS.
> 
> Please change the title of this patch to reflect what it actually
> does. Nothing here is about powering down anything.

My miss, I will fix this in next version.

> 
>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 32
>> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1321,6 +1321,31 @@ 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 (count--) {
>> +		val = readl_relaxed(base + GITS_CTLR);
>> +		if (val & GITS_CTLR_QUIESCENT)
>> +			return 0;
>> +		cpu_relax();
>> +		udelay(1);
>> +	}
> 
> You're now introducing a third variant of a 1s timeout loop. Notice a
> pattern?
> 

I am not sure I know exactly what you suggest. Do you mean I should code
like below to keep the coding style same as the other 2 loops?

	while (1) {
		val = readl_relaxed(base + GITS_CTLR);
		if (val & GITS_CTLR_QUIESCENT)
			return 0;

		count--;
		if (!count)
			return -EBUSY;

		cpu_relax();
		udelay(1);
	}

Thanks,
	Abel

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

[Marc Zyngier]

On Tue, 17 Feb 2015 10:15:15 +0000
"Yun Wu (Abel)" <wuyun.wu@huawei.com> wrote:

> On 2015/2/17 17:29, Marc Zyngier wrote:
> 
> > On Sun, 15 Feb 2015 09:32:02 +0000
> > Yun Wu <wuyun.wu@huawei.com> wrote:
> > 
> >> It's unsafe to change the configurations of an activated ITS
> >> directly since this will lead to unpredictable results. This patch
> >> guarantees a safe quiescent status before initializing an ITS.
> > 
> > Please change the title of this patch to reflect what it actually
> > does. Nothing here is about powering down anything.
> 
> My miss, I will fix this in next version.
> 
> > 
> >> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> >> ---
> >>  drivers/irqchip/irq-gic-v3-its.c | 32
> >> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> >> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >> @@ -1321,6 +1321,31 @@ 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 (count--) {
> >> +		val = readl_relaxed(base + GITS_CTLR);
> >> +		if (val & GITS_CTLR_QUIESCENT)
> >> +			return 0;
> >> +		cpu_relax();
> >> +		udelay(1);
> >> +	}
> > 
> > You're now introducing a third variant of a 1s timeout loop. Notice
> > a pattern?
> > 
> 
> I am not sure I know exactly what you suggest. Do you mean I should
> code like below to keep the coding style same as the other 2 loops?
> 
> 	while (1) {
> 		val = readl_relaxed(base + GITS_CTLR);
> 		if (val & GITS_CTLR_QUIESCENT)
> 			return 0;
> 
> 		count--;
> 		if (!count)
> 			return -EBUSY;
> 
> 		cpu_relax();
> 		udelay(1);
> 	}

That'd be a good start. But given that this is starting to be a common
construct, it could probably be rewritten as:

static int its_poll_timeout(struct its_node *its, void *data,
                            int (*fn)(struct its_node *its,
                                      void *data))
{
	while (1) {
		if (!fn(its, data))
			return 0;

		...
	}
}

and have the call sites to provide the right utility function. We also
have two similar timeout loops in the main GICv3 driver, so there
should be room for improvement.

Thoughts?

Thanks,

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

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

[Marc Zyngier]

On Tue, 17 Feb 2015 10:15:15 +0000
"Yun Wu (Abel)" <wuyun.wu@huawei.com> wrote:

> On 2015/2/17 17:29, Marc Zyngier wrote:
> 
> > On Sun, 15 Feb 2015 09:32:02 +0000
> > Yun Wu <wuyun.wu@huawei.com> wrote:
> > 
> >> It's unsafe to change the configurations of an activated ITS
> >> directly since this will lead to unpredictable results. This patch
> >> guarantees a safe quiescent status before initializing an ITS.
> > 
> > Please change the title of this patch to reflect what it actually
> > does. Nothing here is about powering down anything.
> 
> My miss, I will fix this in next version.
> 
> > 
> >> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
> >> ---
> >>  drivers/irqchip/irq-gic-v3-its.c | 32
> >> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> >> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >> @@ -1321,6 +1321,31 @@ 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 (count--) {
> >> +		val = readl_relaxed(base + GITS_CTLR);
> >> +		if (val & GITS_CTLR_QUIESCENT)
> >> +			return 0;
> >> +		cpu_relax();
> >> +		udelay(1);
> >> +	}
> > 
> > You're now introducing a third variant of a 1s timeout loop. Notice
> > a pattern?
> > 
> 
> I am not sure I know exactly what you suggest. Do you mean I should
> code like below to keep the coding style same as the other 2 loops?
> 
> 	while (1) {
> 		val = readl_relaxed(base + GITS_CTLR);
> 		if (val & GITS_CTLR_QUIESCENT)
> 			return 0;
> 
> 		count--;
> 		if (!count)
> 			return -EBUSY;
> 
> 		cpu_relax();
> 		udelay(1);
> 	}

That'd be a good start. But given that this is starting to be a common
construct, it could probably be rewritten as:

static int its_poll_timeout(struct its_node *its, void *data,
                            int (*fn)(struct its_node *its,
                                      void *data))
{
	while (1) {
		if (!fn(its, data))
			return 0;

		...
	}
}

and have the call sites to provide the right utility function. We also
have two similar timeout loops in the main GICv3 driver, so there
should be room for improvement.

Thoughts?

Thanks,

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

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

[Yun Wu (Abel)]

On 2015/2/17 19:11, Marc Zyngier wrote:

> On Tue, 17 Feb 2015 10:15:15 +0000
> "Yun Wu (Abel)" <wuyun.wu@huawei.com> wrote:
> 
>> On 2015/2/17 17:29, Marc Zyngier wrote:
>>
>>> On Sun, 15 Feb 2015 09:32:02 +0000
>>> Yun Wu <wuyun.wu@huawei.com> wrote:
>>>
>>>> It's unsafe to change the configurations of an activated ITS
>>>> directly since this will lead to unpredictable results. This patch
>>>> guarantees a safe quiescent status before initializing an ITS.
>>>
>>> Please change the title of this patch to reflect what it actually
>>> does. Nothing here is about powering down anything.
>>
>> My miss, I will fix this in next version.
>>
>>>
>>>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3-its.c | 32
>>>> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -1321,6 +1321,31 @@ 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 (count--) {
>>>> +		val = readl_relaxed(base + GITS_CTLR);
>>>> +		if (val & GITS_CTLR_QUIESCENT)
>>>> +			return 0;
>>>> +		cpu_relax();
>>>> +		udelay(1);
>>>> +	}
>>>
>>> You're now introducing a third variant of a 1s timeout loop. Notice
>>> a pattern?
>>>
>>
>> I am not sure I know exactly what you suggest. Do you mean I should
>> code like below to keep the coding style same as the other 2 loops?
>>
>> 	while (1) {
>> 		val = readl_relaxed(base + GITS_CTLR);
>> 		if (val & GITS_CTLR_QUIESCENT)
>> 			return 0;
>>
>> 		count--;
>> 		if (!count)
>> 			return -EBUSY;
>>
>> 		cpu_relax();
>> 		udelay(1);
>> 	}
> 
> That'd be a good start. But given that this is starting to be a common
> construct, it could probably be rewritten as:
> 
> static int its_poll_timeout(struct its_node *its, void *data,
>                             int (*fn)(struct its_node *its,
>                                       void *data))
> {
> 	while (1) {
> 		if (!fn(its, data))
> 			return 0;
> 
> 		...
> 	}
> }
> 
> and have the call sites to provide the right utility function. We also
> have two similar timeout loops in the main GICv3 driver, so there
> should be room for improvement.
> 
> Thoughts?
> 

It looks fine to me. I will make some improvement in the next version after
Chinese Spring Festival. :)

Thanks,
	Abel

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

[Yun Wu (Abel)]

On 2015/2/17 19:11, Marc Zyngier wrote:

> On Tue, 17 Feb 2015 10:15:15 +0000
> "Yun Wu (Abel)" <wuyun.wu@huawei.com> wrote:
> 
>> On 2015/2/17 17:29, Marc Zyngier wrote:
>>
>>> On Sun, 15 Feb 2015 09:32:02 +0000
>>> Yun Wu <wuyun.wu@huawei.com> wrote:
>>>
>>>> It's unsafe to change the configurations of an activated ITS
>>>> directly since this will lead to unpredictable results. This patch
>>>> guarantees a safe quiescent status before initializing an ITS.
>>>
>>> Please change the title of this patch to reflect what it actually
>>> does. Nothing here is about powering down anything.
>>
>> My miss, I will fix this in next version.
>>
>>>
>>>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3-its.c | 32
>>>> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -1321,6 +1321,31 @@ 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 (count--) {
>>>> +		val = readl_relaxed(base + GITS_CTLR);
>>>> +		if (val & GITS_CTLR_QUIESCENT)
>>>> +			return 0;
>>>> +		cpu_relax();
>>>> +		udelay(1);
>>>> +	}
>>>
>>> You're now introducing a third variant of a 1s timeout loop. Notice
>>> a pattern?
>>>
>>
>> I am not sure I know exactly what you suggest. Do you mean I should
>> code like below to keep the coding style same as the other 2 loops?
>>
>> 	while (1) {
>> 		val = readl_relaxed(base + GITS_CTLR);
>> 		if (val & GITS_CTLR_QUIESCENT)
>> 			return 0;
>>
>> 		count--;
>> 		if (!count)
>> 			return -EBUSY;
>>
>> 		cpu_relax();
>> 		udelay(1);
>> 	}
> 
> That'd be a good start. But given that this is starting to be a common
> construct, it could probably be rewritten as:
> 
> static int its_poll_timeout(struct its_node *its, void *data,
>                             int (*fn)(struct its_node *its,
>                                       void *data))
> {
> 	while (1) {
> 		if (!fn(its, data))
> 			return 0;
> 
> 		...
> 	}
> }
> 
> and have the call sites to provide the right utility function. We also
> have two similar timeout loops in the main GICv3 driver, so there
> should be room for improvement.
> 
> Thoughts?
> 

It looks fine to me. I will make some improvement in the next version after
Chinese Spring Festival. :)

Thanks,
	Abel

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

[Yun Wu (Abel)]

On 2015/2/17 20:27, Yun Wu (Abel) wrote:

> On 2015/2/17 19:11, Marc Zyngier wrote:
> 
>> On Tue, 17 Feb 2015 10:15:15 +0000
>> "Yun Wu (Abel)" <wuyun.wu@huawei.com> wrote:
>>
>>> On 2015/2/17 17:29, Marc Zyngier wrote:
>>>
>>>> On Sun, 15 Feb 2015 09:32:02 +0000
>>>> Yun Wu <wuyun.wu@huawei.com> wrote:
>>>>
>>>>> It's unsafe to change the configurations of an activated ITS
>>>>> directly since this will lead to unpredictable results. This patch
>>>>> guarantees a safe quiescent status before initializing an ITS.
>>>>
>>>> Please change the title of this patch to reflect what it actually
>>>> does. Nothing here is about powering down anything.
>>>
>>> My miss, I will fix this in next version.
>>>
>>>>
>>>>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>>>>> ---
>>>>>  drivers/irqchip/irq-gic-v3-its.c | 32
>>>>> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>>>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>>> @@ -1321,6 +1321,31 @@ 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 (count--) {
>>>>> +		val = readl_relaxed(base + GITS_CTLR);
>>>>> +		if (val & GITS_CTLR_QUIESCENT)
>>>>> +			return 0;
>>>>> +		cpu_relax();
>>>>> +		udelay(1);
>>>>> +	}
>>>>
>>>> You're now introducing a third variant of a 1s timeout loop. Notice
>>>> a pattern?
>>>>
>>>
>>> I am not sure I know exactly what you suggest. Do you mean I should
>>> code like below to keep the coding style same as the other 2 loops?
>>>
>>> 	while (1) {
>>> 		val = readl_relaxed(base + GITS_CTLR);
>>> 		if (val & GITS_CTLR_QUIESCENT)
>>> 			return 0;
>>>
>>> 		count--;
>>> 		if (!count)
>>> 			return -EBUSY;
>>>
>>> 		cpu_relax();
>>> 		udelay(1);
>>> 	}
>>
>> That'd be a good start. But given that this is starting to be a common
>> construct, it could probably be rewritten as:
>>
>> static int its_poll_timeout(struct its_node *its, void *data,
>>                             int (*fn)(struct its_node *its,
>>                                       void *data))
>> {
>> 	while (1) {
>> 		if (!fn(its, data))
>> 			return 0;
>>
>> 		...
>> 	}
>> }
>>
>> and have the call sites to provide the right utility function. We also
>> have two similar timeout loops in the main GICv3 driver, so there
>> should be room for improvement.
>>
>> Thoughts?
>>


Hi Marc,

Currently I didn't make any improvement on providing a unified utility
function to do the timeout loops, because I haven't found a proper way
yet. And to achieve this, at least three aspects I can imagine are
needed to be considered.

Refactoring the existing loop functions comes first. A prototype is
showed below.

	static T1 func_prototype(T2 node, char *msg, void **args)
	{
		u32 count = 1000000;

		do_something_here(node, args);

		while (1) {
			if (condition_satisfied(node, args))
				return (T1)SUCCESS;

			count--;
			if (!count) {
				print_err_msg(msg);
				return (T1)FAIL;
			}

			cpu_relax();
			udelay(1);
		}
	}

Obviously it will make things complicated to move do_something_here() and
print_err_msg() outside of func_prototype(), because func_prototype() could
be called sereval places. But the two functions are different from each
loop function, so...

	static T1 func_prototype(T2 node, char *msg, void **args)
	{
		u32 count = 1000000;

		do_something_here(node, args);

		while (count--) {
			if (condition_satisfied(node, args))
				return (T1)SUCCESS;

			cpu_relax();
			udelay(1);
		}

		print_err_msg(msg);
		return (T1)FAIL;
	}

Now we can unify the loop part,

	static bool condition_satisfied(T2 node, void **args);

	static u32 poll_timeout(T2 node, void **args,
				bool (*fn)(T2, void **))
	{
		u32 count = 1000000;

		while (count--) {
			if (fn(node, args))
				break;

			cpu_relax();
			udelay(1);
		}

		return count;
	}

	static T1 func_prototype(T2 node, char *msg, void **args)
	{
		do_something_here(node, args);

		if (poll_timeout(node, args, condition_satisfied)) {
			return (T1)SUCCESS;
		} else {
			print_err_msg(msg);
			return (T1)FAIL;
		}
	}

Look at what I have done, turn the original N loop functions to 2*N+1 functions.
It can hardly be called improvement.. :(

The 2nd and 3rd aspects are return value and list of parameters respectively.
Using (void *) may help a lot, I think.

Thoughts?

Thanks,
	Abel

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

[Yun Wu (Abel)]

On 2015/2/17 20:27, Yun Wu (Abel) wrote:

> On 2015/2/17 19:11, Marc Zyngier wrote:
> 
>> On Tue, 17 Feb 2015 10:15:15 +0000
>> "Yun Wu (Abel)" <wuyun.wu@huawei.com> wrote:
>>
>>> On 2015/2/17 17:29, Marc Zyngier wrote:
>>>
>>>> On Sun, 15 Feb 2015 09:32:02 +0000
>>>> Yun Wu <wuyun.wu@huawei.com> wrote:
>>>>
>>>>> It's unsafe to change the configurations of an activated ITS
>>>>> directly since this will lead to unpredictable results. This patch
>>>>> guarantees a safe quiescent status before initializing an ITS.
>>>>
>>>> Please change the title of this patch to reflect what it actually
>>>> does. Nothing here is about powering down anything.
>>>
>>> My miss, I will fix this in next version.
>>>
>>>>
>>>>> Signed-off-by: Yun Wu <wuyun.wu@huawei.com>
>>>>> ---
>>>>>  drivers/irqchip/irq-gic-v3-its.c | 32
>>>>> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>>>> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>>> @@ -1321,6 +1321,31 @@ 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 (count--) {
>>>>> +		val = readl_relaxed(base + GITS_CTLR);
>>>>> +		if (val & GITS_CTLR_QUIESCENT)
>>>>> +			return 0;
>>>>> +		cpu_relax();
>>>>> +		udelay(1);
>>>>> +	}
>>>>
>>>> You're now introducing a third variant of a 1s timeout loop. Notice
>>>> a pattern?
>>>>
>>>
>>> I am not sure I know exactly what you suggest. Do you mean I should
>>> code like below to keep the coding style same as the other 2 loops?
>>>
>>> 	while (1) {
>>> 		val = readl_relaxed(base + GITS_CTLR);
>>> 		if (val & GITS_CTLR_QUIESCENT)
>>> 			return 0;
>>>
>>> 		count--;
>>> 		if (!count)
>>> 			return -EBUSY;
>>>
>>> 		cpu_relax();
>>> 		udelay(1);
>>> 	}
>>
>> That'd be a good start. But given that this is starting to be a common
>> construct, it could probably be rewritten as:
>>
>> static int its_poll_timeout(struct its_node *its, void *data,
>>                             int (*fn)(struct its_node *its,
>>                                       void *data))
>> {
>> 	while (1) {
>> 		if (!fn(its, data))
>> 			return 0;
>>
>> 		...
>> 	}
>> }
>>
>> and have the call sites to provide the right utility function. We also
>> have two similar timeout loops in the main GICv3 driver, so there
>> should be room for improvement.
>>
>> Thoughts?
>>


Hi Marc,

Currently I didn't make any improvement on providing a unified utility
function to do the timeout loops, because I haven't found a proper way
yet. And to achieve this, at least three aspects I can imagine are
needed to be considered.

Refactoring the existing loop functions comes first. A prototype is
showed below.

	static T1 func_prototype(T2 node, char *msg, void **args)
	{
		u32 count = 1000000;

		do_something_here(node, args);

		while (1) {
			if (condition_satisfied(node, args))
				return (T1)SUCCESS;

			count--;
			if (!count) {
				print_err_msg(msg);
				return (T1)FAIL;
			}

			cpu_relax();
			udelay(1);
		}
	}

Obviously it will make things complicated to move do_something_here() and
print_err_msg() outside of func_prototype(), because func_prototype() could
be called sereval places. But the two functions are different from each
loop function, so...

	static T1 func_prototype(T2 node, char *msg, void **args)
	{
		u32 count = 1000000;

		do_something_here(node, args);

		while (count--) {
			if (condition_satisfied(node, args))
				return (T1)SUCCESS;

			cpu_relax();
			udelay(1);
		}

		print_err_msg(msg);
		return (T1)FAIL;
	}

Now we can unify the loop part,

	static bool condition_satisfied(T2 node, void **args);

	static u32 poll_timeout(T2 node, void **args,
				bool (*fn)(T2, void **))
	{
		u32 count = 1000000;

		while (count--) {
			if (fn(node, args))
				break;

			cpu_relax();
			udelay(1);
		}

		return count;
	}

	static T1 func_prototype(T2 node, char *msg, void **args)
	{
		do_something_here(node, args);

		if (poll_timeout(node, args, condition_satisfied)) {
			return (T1)SUCCESS;
		} else {
			print_err_msg(msg);
			return (T1)FAIL;
		}
	}

Look at what I have done, turn the original N loop functions to 2*N+1 functions.
It can hardly be called improvement.. :(

The 2nd and 3rd aspects are return value and list of parameters respectively.
Using (void *) may help a lot, I think.

Thoughts?

Thanks,
	Abel