| Message ID | 1449766530-16935-4-git-send-email-Suravee.Suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 10, 2015 at 08:55:29AM -0800, Suravee Suthikulpanit wrote: > This patch replaces the struct device_node with struct fwnode_handle > since this structure is common between DT and ACPI. > > It also refactors gicv2m_init_one() to prepare for ACPI support. > The only functional change is removing the node name from pr_info. > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > @@ -359,10 +355,10 @@ static int __init gicv2m_init_one(struct device_node *node, > } > > list_add_tail(&v2m->entry, &v2m_nodes); > - pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name, > - (unsigned long)v2m->res.start, (unsigned long)v2m->res.end, > - v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); > > + pr_info("range[%#lx:%#lx], SPI[%d:%d]\n", > + (unsigned long)res->start, (unsigned long)res->end, > + v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); You didn't change this, but I don't think this message has enough context. It's pretty cryptic all by itself. It'd be nice if it could at least include a device name, e.g., if you could use dev_info(). If that's possible, I guess a separate patch would be appropriate, since there are other similar things in this file. Bjorn
Hi Bjorn, Thanks for your review. Please see my comments below. On 12/16/2015 4:12 PM, Bjorn Helgaas wrote: > On Thu, Dec 10, 2015 at 08:55:29AM -0800, Suravee Suthikulpanit wrote: >> This patch replaces the struct device_node with struct fwnode_handle >> since this structure is common between DT and ACPI. >> >> It also refactors gicv2m_init_one() to prepare for ACPI support. >> The only functional change is removing the node name from pr_info. >> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > >> @@ -359,10 +355,10 @@ static int __init gicv2m_init_one(struct device_node *node, >> } >> >> list_add_tail(&v2m->entry, &v2m_nodes); >> - pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name, >> - (unsigned long)v2m->res.start, (unsigned long)v2m->res.end, >> - v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); >> >> + pr_info("range[%#lx:%#lx], SPI[%d:%d]\n", >> + (unsigned long)res->start, (unsigned long)res->end, >> + v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); > > You didn't change this, but I don't think this message has enough > context. It's pretty cryptic all by itself. It'd be nice if it could > at least include a device name, e.g., if you could use dev_info(). Here is the example of the information printed: [ 0.000000] GICv2m: range[0xe1180000:0xe1181000], SPI[64:320] Basically, the v2m is just an extension of the GIC. Here, we are printing the memory range that it is covering, which can be used to identify different V2m frame and the associate interrupt range (SPI). The node name is not really providing any values. So, we are removing it. Thanks, Suravee > If that's possible, I guess a separate patch would be appropriate, > since there are other similar things in this file. > j > Bjorn
On Wed, Dec 16, 2015 at 06:23:49PM -0600, Suravee Suthikulanit wrote: > Hi Bjorn, > > Thanks for your review. Please see my comments below. > > On 12/16/2015 4:12 PM, Bjorn Helgaas wrote: > >On Thu, Dec 10, 2015 at 08:55:29AM -0800, Suravee Suthikulpanit wrote: > >>This patch replaces the struct device_node with struct fwnode_handle > >>since this structure is common between DT and ACPI. > >> > >>It also refactors gicv2m_init_one() to prepare for ACPI support. > >>The only functional change is removing the node name from pr_info. > >> > >>Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > >>Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > > > >>@@ -359,10 +355,10 @@ static int __init gicv2m_init_one(struct device_node *node, > >> } > >> > >> list_add_tail(&v2m->entry, &v2m_nodes); > >>- pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name, > >>- (unsigned long)v2m->res.start, (unsigned long)v2m->res.end, > >>- v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); > >> > >>+ pr_info("range[%#lx:%#lx], SPI[%d:%d]\n", > >>+ (unsigned long)res->start, (unsigned long)res->end, > >>+ v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); > > > >You didn't change this, but I don't think this message has enough > >context. It's pretty cryptic all by itself. It'd be nice if it could > >at least include a device name, e.g., if you could use dev_info(). > > Here is the example of the information printed: > [ 0.000000] GICv2m: range[0xe1180000:0xe1181000], SPI[64:320] > > Basically, the v2m is just an extension of the GIC. Here, we are > printing the memory range that it is covering, which can be used to > identify different V2m frame and the associate interrupt range > (SPI). The node name is not really providing any values. So, we are > removing it. I noticed the pr_fmt definition later; that adds some useful context I didn't know about. I guess there's no struct device for the GIC? I don't see one in struct device_node. Seems like this piece of hardware that apparently responds to a memory range *could* have a struct device, but I'm a little fuzzy on how we handle ACPI and OF device descriptions in that regard. I hadn't noticed the memory range part; maybe you could use %pR there? Just to double-check, there's no off-by-one error in the SPI range, is there? The pattern I usually expect is "start, start + nr_items - 1". I'm just kibbitzing here; this isn't PCI code, and you don't need my ack, so just consider these as random observations. Bjorn
On 12/17/2015 10:57 AM, Bjorn Helgaas wrote: > On Wed, Dec 16, 2015 at 06:23:49PM -0600, Suravee Suthikulanit wrote: >> Hi Bjorn, >> >> Thanks for your review. Please see my comments below. >> >> On 12/16/2015 4:12 PM, Bjorn Helgaas wrote: >>> On Thu, Dec 10, 2015 at 08:55:29AM -0800, Suravee Suthikulpanit wrote: >>>> This patch replaces the struct device_node with struct fwnode_handle >>>> since this structure is common between DT and ACPI. >>>> >>>> It also refactors gicv2m_init_one() to prepare for ACPI support. >>>> The only functional change is removing the node name from pr_info. >>>> >>>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >>>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >>> >>>> @@ -359,10 +355,10 @@ static int __init gicv2m_init_one(struct device_node *node, >>>> } >>>> >>>> list_add_tail(&v2m->entry, &v2m_nodes); >>>> - pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name, >>>> - (unsigned long)v2m->res.start, (unsigned long)v2m->res.end, >>>> - v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); >>>> >>>> + pr_info("range[%#lx:%#lx], SPI[%d:%d]\n", >>>> + (unsigned long)res->start, (unsigned long)res->end, >>>> + v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); >>> >>> You didn't change this, but I don't think this message has enough >>> context. It's pretty cryptic all by itself. It'd be nice if it could >>> at least include a device name, e.g., if you could use dev_info(). >> >> Here is the example of the information printed: >> [ 0.000000] GICv2m: range[0xe1180000:0xe1181000], SPI[64:320] >> >> Basically, the v2m is just an extension of the GIC. Here, we are >> printing the memory range that it is covering, which can be used to >> identify different V2m frame and the associate interrupt range >> (SPI). The node name is not really providing any values. So, we are >> removing it. > > I noticed the pr_fmt definition later; that adds some useful context I > didn't know about. I guess there's no struct device for the GIC? I > don't see one in struct device_node. Seems like this piece of > hardware that apparently responds to a memory range *could* have a > struct device,but I'm a little fuzzy on how we handle ACPI and OF > device descriptions in that regard. For DT, v2m is advertised as a sub-node inside GIC. So, both of them has the struct device_node references. IIUC, GIC node is match as irqchip, and not as a traditional platform bus device. Similarly, for ACPI, v2m is advertised as a sub-table inside MADT, and we are using the fwnode_handle to reference to. > I hadn't noticed the memory range part; maybe you could use %pR there? I guess we could have :) I can send a separate patch to clean this up. > Just to double-check, there's no off-by-one error in the SPI range, is > there? The pattern I usually expect is "start, start + nr_items - 1". In that case, this should have been [64:319]. I'll send a small patch to clean this up. > I'm just kibbitzing here; this isn't PCI code, and you don't need my > ack, so just consider these as random observations. > > Bjorn > Thanks for sharing your observation. It's always been good ones :) Suravee
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c index 87f8d10..779c390 100644 --- a/drivers/irqchip/irq-gic-v2m.c +++ b/drivers/irqchip/irq-gic-v2m.c @@ -55,7 +55,7 @@ static DEFINE_SPINLOCK(v2m_lock); struct v2m_data { struct list_head entry; - struct device_node *node; + struct fwnode_handle *fwnode; struct resource res; /* GICv2m resource */ void __iomem *base; /* GICv2m virt address */ u32 spi_start; /* The SPI number that MSIs start */ @@ -254,7 +254,7 @@ static void gicv2m_teardown(void) list_del(&v2m->entry); kfree(v2m->bm); iounmap(v2m->base); - of_node_put(v2m->node); + of_node_put(to_of_node(v2m->fwnode)); kfree(v2m); } } @@ -268,7 +268,7 @@ static int gicv2m_allocate_domains(struct irq_domain *parent) if (!v2m) return 0; - inner_domain = irq_domain_create_tree(of_node_to_fwnode(v2m->node), + inner_domain = irq_domain_create_tree(v2m->fwnode, &gicv2m_domain_ops, v2m); if (!inner_domain) { pr_err("Failed to create GICv2m domain\n"); @@ -277,10 +277,10 @@ static int gicv2m_allocate_domains(struct irq_domain *parent) inner_domain->bus_token = DOMAIN_BUS_NEXUS; inner_domain->parent = parent; - pci_domain = pci_msi_create_irq_domain(of_node_to_fwnode(v2m->node), + pci_domain = pci_msi_create_irq_domain(v2m->fwnode, &gicv2m_msi_domain_info, inner_domain); - plat_domain = platform_msi_create_irq_domain(of_node_to_fwnode(v2m->node), + plat_domain = platform_msi_create_irq_domain(v2m->fwnode, &gicv2m_pmsi_domain_info, inner_domain); if (!pci_domain || !plat_domain) { @@ -296,8 +296,9 @@ static int gicv2m_allocate_domains(struct irq_domain *parent) return 0; } -static int __init gicv2m_init_one(struct device_node *node, - struct irq_domain *parent) +static int __init gicv2m_init_one(struct fwnode_handle *fwnode, + u32 spi_start, u32 nr_spis, + struct resource *res) { int ret; struct v2m_data *v2m; @@ -309,13 +310,9 @@ static int __init gicv2m_init_one(struct device_node *node, } INIT_LIST_HEAD(&v2m->entry); - v2m->node = node; + v2m->fwnode = fwnode; - ret = of_address_to_resource(node, 0, &v2m->res); - if (ret) { - pr_err("Failed to allocate v2m resource.\n"); - goto err_free_v2m; - } + memcpy(&v2m->res, res, sizeof(struct resource)); v2m->base = ioremap(v2m->res.start, resource_size(&v2m->res)); if (!v2m->base) { @@ -324,10 +321,9 @@ static int __init gicv2m_init_one(struct device_node *node, goto err_free_v2m; } - if (!of_property_read_u32(node, "arm,msi-base-spi", &v2m->spi_start) && - !of_property_read_u32(node, "arm,msi-num-spis", &v2m->nr_spis)) { - pr_info("Overriding V2M MSI_TYPER (base:%u, num:%u)\n", - v2m->spi_start, v2m->nr_spis); + if (spi_start && nr_spis) { + v2m->spi_start = spi_start; + v2m->nr_spis = nr_spis; } else { u32 typer = readl_relaxed(v2m->base + V2M_MSI_TYPER); @@ -359,10 +355,10 @@ static int __init gicv2m_init_one(struct device_node *node, } list_add_tail(&v2m->entry, &v2m_nodes); - pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name, - (unsigned long)v2m->res.start, (unsigned long)v2m->res.end, - v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); + pr_info("range[%#lx:%#lx], SPI[%d:%d]\n", + (unsigned long)res->start, (unsigned long)res->end, + v2m->spi_start, (v2m->spi_start + v2m->nr_spis)); return 0; err_iounmap: @@ -384,10 +380,25 @@ int __init gicv2m_of_init(struct device_node *node, struct irq_domain *parent) for (child = of_find_matching_node(node, gicv2m_device_id); child; child = of_find_matching_node(child, gicv2m_device_id)) { + u32 spi_start = 0, nr_spis = 0; + struct resource res; + if (!of_find_property(child, "msi-controller", NULL)) continue; - ret = gicv2m_init_one(child, parent); + ret = of_address_to_resource(child, 0, &res); + if (ret) { + pr_err("Failed to allocate v2m resource.\n"); + break; + } + + if (!of_property_read_u32(child, "arm,msi-base-spi", + &spi_start) && + !of_property_read_u32(child, "arm,msi-num-spis", &nr_spis)) + pr_info("DT overriding V2M MSI_TYPER (base:%u, num:%u)\n", + spi_start, nr_spis); + + ret = gicv2m_init_one(&child->fwnode, spi_start, nr_spis, &res); if (ret) { of_node_put(node); break;