Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization

From: Julien Thierry
Date: Thu Nov 08 2018 - 06:25:31 EST

Next message: Fabrizio Castro: "RE: [PATCH v2 2/3] dt-bindings: can: rcar_can: Add r8a774a1 support"
Previous message: Petr Mladek: "Re: [PATCH v6 1/3] printk: Add line-buffered printk() API."
Next in thread: Richter, Robert: "Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 07/11/18 22:03, Robert Richter wrote:

To initialize the its nodes at a later point during boot, we need to
split probing from initialization. Collect all information required
for initialization in struct its_node. We can then use the its node
list for initialization.

Signed-off-by: Robert Richter <rrichter@xxxxxxxxxx>
---
drivers/irqchip/irq-gic-v3-its.c | 135 +++++++++++++++++++++++--------------
drivers/irqchip/irq-gic-v3.c | 2 +-
include/linux/irqchip/arm-gic-v3.h | 4 +-
3 files changed, 87 insertions(+), 54 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 4033f71f5181..c28f4158ff70 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -103,6 +103,7 @@ struct its_node {
struct list_head entry;
void __iomem *base;
phys_addr_t phys_base;
+ phys_addr_t phys_size;
struct its_cmd_block *cmd_base;
struct its_cmd_block *cmd_write;
struct its_baser tables[GITS_BASER_NR_REGS];
@@ -3375,7 +3376,7 @@ static struct syscore_ops its_syscore_ops = {
.resume = its_restore_enable,
};
-static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
+static int its_init_domain(struct its_node *its)
{
struct irq_domain *inner_domain;
struct msi_domain_info *info;
@@ -3384,7 +3385,8 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
if (!info)
return -ENOMEM;
- inner_domain = irq_domain_create_tree(handle, &its_domain_ops, its);
+ inner_domain = irq_domain_create_tree(its->fwnode_handle,
+ &its_domain_ops, its);

Separate change?

if (!inner_domain) {
kfree(info);
return -ENOMEM;
@@ -3441,8 +3443,7 @@ static int its_init_vpe_domain(void)
return 0;
}
-static int __init its_compute_its_list_map(struct resource *res,
- void __iomem *its_base)
+static int __init its_compute_its_list_map(struct its_node *its)
{
int its_number;
u32 ctlr;
@@ -3456,15 +3457,15 @@ static int __init its_compute_its_list_map(struct resource *res,
its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
if (its_number >= GICv4_ITS_LIST_MAX) {
pr_err("ITS@%pa: No ITSList entry available!\n",
- &res->start);
+ &its->phys_base);
return -EINVAL;
}
- ctlr = readl_relaxed(its_base + GITS_CTLR);
+ ctlr = readl_relaxed(its->base + GITS_CTLR);
ctlr &= ~GITS_CTLR_ITS_NUMBER;
ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
- writel_relaxed(ctlr, its_base + GITS_CTLR);
- ctlr = readl_relaxed(its_base + GITS_CTLR);
+ writel_relaxed(ctlr, its->base + GITS_CTLR);
+ ctlr = readl_relaxed(its->base + GITS_CTLR);

This (removal of its_base parameter) also feel like a separate change.

Also, I would define a local variable its_base to avoid dereferencing its every time in order to get the base address.

if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
its_number = ctlr & GITS_CTLR_ITS_NUMBER;
its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
@@ -3472,83 +3473,110 @@ static int __init its_compute_its_list_map(struct resource *res,
if (test_and_set_bit(its_number, &its_list_map)) {
pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
- &res->start, its_number);
+ &its->phys_base, its_number);
return -EINVAL;
}
return its_number;
}
+static void its_free(struct its_node *its)
+{
+ raw_spin_lock(&its_lock);
+ list_del(&its->entry);
+ raw_spin_unlock(&its_lock);
+
+ kfree(its);
+}
+
+static int __init its_init_one(struct its_node *its);

You might as well define its_init_one here, no?

+
static int __init its_probe_one(struct resource *res,
struct fwnode_handle *handle, int numa_node)
{
struct its_node *its;
+ int err;
+
+ its = kzalloc(sizeof(*its), GFP_KERNEL);
+ if (!its)
+ return -ENOMEM;
+
+ raw_spin_lock_init(&its->lock);
+ INIT_LIST_HEAD(&its->entry);
+ INIT_LIST_HEAD(&its->its_device_list);
+ its->fwnode_handle = handle;
+ its->phys_base = res->start;
+ its->phys_size = resource_size(res);
+ its->numa_node = numa_node;
+
+ raw_spin_lock(&its_lock);
+ list_add_tail(&its->entry, &its_nodes);
+ raw_spin_unlock(&its_lock);
+
+ pr_info("ITS %pR\n", res);
+
+ err = its_init_one(its);
+ if (err)
+ its_free(its);
+
+ return err;
+}
+
+static int __init its_init_one(struct its_node *its)
+{
void __iomem *its_base;
u32 val, ctlr;
u64 baser, tmp, typer;
int err;
- its_base = ioremap(res->start, resource_size(res));
+ its_base = ioremap(its->phys_base, its->phys_size);
if (!its_base) {
- pr_warn("ITS@%pa: Unable to map ITS registers\n", &res->start);
- return -ENOMEM;
+ pr_warn("ITS@%pa: Unable to map ITS registers\n", &its->phys_base);
+ err = -ENOMEM;
+ goto fail;
}
val = readl_relaxed(its_base + GITS_PIDR2) & GIC_PIDR2_ARCH_MASK;
if (val != 0x30 && val != 0x40) {
- pr_warn("ITS@%pa: No ITS detected, giving up\n", &res->start);
+ pr_warn("ITS@%pa: No ITS detected, giving up\n", &its->phys_base);
err = -ENODEV;
goto out_unmap;
}
err = its_force_quiescent(its_base);
if (err) {
- pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &res->start);
+ pr_warn("ITS@%pa: Failed to quiesce, giving up\n", &its->phys_base);
goto out_unmap;
}
- pr_info("ITS %pR\n", res);
-
- its = kzalloc(sizeof(*its), GFP_KERNEL);
- if (!its) {
- err = -ENOMEM;
- goto out_unmap;
- }
-
- raw_spin_lock_init(&its->lock);
- INIT_LIST_HEAD(&its->entry);
- INIT_LIST_HEAD(&its->its_device_list);
typer = gic_read_typer(its_base + GITS_TYPER);
its->base = its_base;
- its->phys_base = res->start;
its->ite_size = GITS_TYPER_ITT_ENTRY_SIZE(typer);
its->device_ids = GITS_TYPER_DEVBITS(typer);
its->is_v4 = !!(typer & GITS_TYPER_VLPIS);
if (its->is_v4) {
if (!(typer & GITS_TYPER_VMOVP)) {
- err = its_compute_its_list_map(res, its_base);
+ err = its_compute_its_list_map(its);
if (err < 0)
- goto out_free_its;
+ goto out_unmap;
its->list_nr = err;
pr_info("ITS@%pa: Using ITS number %d\n",
- &res->start, err);
+ &its->phys_base, err);
} else {
- pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
+ pr_info("ITS@%pa: Single VMOVP capable\n",
+ &its->phys_base);
}
}
- its->numa_node = numa_node;
-
its->cmd_base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
get_order(ITS_CMD_QUEUE_SZ));
if (!its->cmd_base) {
err = -ENOMEM;
- goto out_free_its;
+ goto out_unmap;
}
its->cmd_write = its->cmd_base;
- its->fwnode_handle = handle;
its->get_msi_base = its_irq_get_msi_base;
its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP;
@@ -3597,13 +3625,11 @@ static int __init its_probe_one(struct resource *res,
if (GITS_TYPER_HCC(typer))
its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
- err = its_init_domain(handle, its);
+ err = its_init_domain(its);

I'm not sure what is the logic for "this goes in probe, this goes in init?".

if (err)
goto out_free_tables;
- raw_spin_lock(&its_lock);
- list_add_tail(&its->entry, &its_nodes);
- raw_spin_unlock(&its_lock);
+ pr_info("ITS@%pa: ITS node added\n", &its->phys_base);
return 0;
@@ -3611,11 +3637,10 @@ static int __init its_probe_one(struct resource *res,
its_free_tables(its);
out_free_cmd:
free_pages((unsigned long)its->cmd_base, get_order(ITS_CMD_QUEUE_SZ));
-out_free_its:
- kfree(its);
out_unmap:
iounmap(its_base);
- pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
+fail:
+ pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
return err;
}
@@ -3888,13 +3913,12 @@ static void __init its_acpi_probe(void)
static void __init its_acpi_probe(void) { }
#endif
-int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
- struct irq_domain *parent_domain)
+static int __init its_init(void);
+
+int __init its_probe(struct fwnode_handle *handle, struct rdists *rdists,
+ struct irq_domain *parent_domain)
{
struct device_node *of_node;
- struct its_node *its;
- bool has_v4 = false;
- int err;
its_parent = parent_domain;
of_node = to_of_node(handle);
@@ -3903,13 +3927,22 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
else
its_acpi_probe();
+ gic_rdists = rdists;
+
+ return its_init();
+}
+
+static int __init its_init(void)
+{
+ struct its_node *its;
+ bool has_v4 = false;
+ int err;
+
if (list_empty(&its_nodes)) {
pr_warn("ITS: No ITS available, not enabling LPIs\n");
return -ENXIO;
}
- gic_rdists = rdists;
-
err = allocate_lpi_tables();
if (err)
return err;
@@ -3917,10 +3950,10 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
list_for_each_entry(its, &its_nodes, entry)
has_v4 |= its->is_v4;
- if (has_v4 & rdists->has_vlpis) {
+ if (has_v4 & gic_rdists->has_vlpis) {
if (its_init_vpe_domain() ||
- its_init_v4(parent_domain, &its_vpe_domain_ops)) {
- rdists->has_vlpis = false;
+ its_init_v4(its_parent, &its_vpe_domain_ops)) {
+ gic_rdists->has_vlpis = false;
pr_err("ITS: Disabling GICv4 support\n");
}
}
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 8f87f40c9460..e04108b7c6b7 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1132,7 +1132,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
gic_cpu_pm_init();
if (gic_dist_supports_lpis()) {
- its_init(handle, &gic_data.rdists, gic_data.domain);
+ its_probe(handle, &gic_data.rdists, gic_data.domain);
its_cpu_init();
}
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 071b4cbdf010..a6fdb2910f73 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -603,8 +603,8 @@ struct rdists {
struct irq_domain;
struct fwnode_handle;
int its_cpu_init(void);
-int its_init(struct fwnode_handle *handle, struct rdists *rdists,
- struct irq_domain *domain);
+int its_probe(struct fwnode_handle *handle, struct rdists *rdists,
+ struct irq_domain *domain);
int mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent);
static inline bool gic_enable_sre(void)

--
Julien Thierry

Next message: Fabrizio Castro: "RE: [PATCH v2 2/3] dt-bindings: can: rcar_can: Add r8a774a1 support"
Previous message: Petr Mladek: "Re: [PATCH v6 1/3] printk: Add line-buffered printk() API."
Next in thread: Richter, Robert: "Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]