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
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 <[email protected]>
Signed-off-by: Yun Wu <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Define macros for GITS_CTLR fields to avoid using magic numbers.
Signed-off-by: Yun Wu <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
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 <[email protected]>
>> ---
>> 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
>>
On Sun, 15 Feb 2015 09:32:00 +0000
Yun Wu <[email protected]> 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 <[email protected]>
> ---
> 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.
On Sun, 15 Feb 2015 09:32:02 +0000
Yun Wu <[email protected]> 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 <[email protected]>
> ---
> 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.
On Sun, 15 Feb 2015 09:31:59 +0000
Yun Wu <[email protected]> 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 <[email protected]>
Acked-by: Marc Zyngier <[email protected]>
> ---
> 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.
On 2015/2/17 17:19, Marc Zyngier wrote:
> On Sun, 15 Feb 2015 09:32:00 +0000
> Yun Wu <[email protected]> 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 <[email protected]>
>> ---
>> 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
On 2015/2/17 17:29, Marc Zyngier wrote:
> On Sun, 15 Feb 2015 09:32:02 +0000
> Yun Wu <[email protected]> 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 <[email protected]>
>> ---
>> 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
On Tue, 17 Feb 2015 10:15:15 +0000
"Yun Wu (Abel)" <[email protected]> wrote:
> On 2015/2/17 17:29, Marc Zyngier wrote:
>
> > On Sun, 15 Feb 2015 09:32:02 +0000
> > Yun Wu <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.
On 2015/2/17 19:11, Marc Zyngier wrote:
> On Tue, 17 Feb 2015 10:15:15 +0000
> "Yun Wu (Abel)" <[email protected]> wrote:
>
>> On 2015/2/17 17:29, Marc Zyngier wrote:
>>
>>> On Sun, 15 Feb 2015 09:32:02 +0000
>>> Yun Wu <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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