Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization
On 08.11.18 11:25:24, Julien Thierry wrote: > On 07/11/18 22:03, Robert Richter wrote: > >-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. In a separate change the motivation of the change would not be obvious. While the change of the variable itself is trivial from the perspective of review and testing, I decided to keep it in the context of the overall change of this patch. > > Also, I would define a local variable its_base to avoid dereferencing > its every time in order to get the base address. Hmm, there is not much difference in reading the code then, while the use of a local variable just adds more code without benefit. The compiler does not care as the value is probably stored in a register anyway. There are also other struct members, should all of them being mirrored in a local variable? > > > 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? This is an intermediate definition that will be removed in a later patch. Moving the whole code here would make the change less readable. > > >+ > > 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
Re: [PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization
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 --- 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_headentry; 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_basertables[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) +{
[PATCH 07/10] irqchip/gic-v3-its: Split probing from its node initialization
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 --- 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_headentry; 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_basertables[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); 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); 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); + 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->st