| Message ID | 1416329088-23328-4-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Ian, On 11/18/2014 04:44 PM, Ian Campbell wrote: > Currently we only establish specific mappings for pcie0, which is > used on the Mustang platform. However at least McDivitt uses pcie3. > So wire up all the others, based on whether the corresponding DT node > is marked as available. > > This results in no change for Mustang. Hopefully, we will support PCI DT parsing in Xen 4.6! > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > +/* > + * Xen does not currently support mapping MMIO regions and interrupt > + * for bus child devices (referenced via the "ranges" and > + * "interrupt-map" properties to domain 0). Instead for now map the > + * necessary resources manually. > + */ > +static int xgene_storm_specific_mapping(struct domain *d) > +{ > + struct dt_device_node *node = NULL; NIT: const struct > + int ret; > + > + while ( (node = dt_find_compatible_node(node, "pci", "apm,xgene-pcie")) ) > + { > + u64 addr; > + > + /* Identify the bus via it's control register address */ > + ret = dt_device_get_address(node, 0, &addr, NULL); > + if ( ret < 0 ) > + return ret; > + > + if ( !dt_device_is_available(node) ) > + continue; > + > + switch ( addr ) > + { > + case 0x1f2b0000: /* PCIe0 */ > + ret = xgene_storm_pcie_specific_mapping(d, > + 0x0e000000000UL, 0x10000000000UL, 0xc2); > + break; > + case 0x1f2c0000: /* PCIe1 */ > + ret = xgene_storm_pcie_specific_mapping(d, > + 0x0d000000000UL, 0x0e000000000UL, 0xc8); > + break; > + case 0x1f2d0000: /* PCIe2 */ > + ret = xgene_storm_pcie_specific_mapping(d, > + 0x09000000000UL, 0x0a000000000UL, 0xce); > + break; > + case 0x1f500000: /* PCIe3 */ > + ret = xgene_storm_pcie_specific_mapping(d, > + 0x0a000000000UL, 0x0c000000000UL, 0xd4); > + break; > + case 0x1f510000: /* PCIe4 */ > + ret = xgene_storm_pcie_specific_mapping(d, > + 0x0c000000000UL, 0x0d000000000UL, 0xda); > + break; > + > + default: > + /* Ignore unknown PCI busses */ I would add a printk("Ignoring PCI busses %s\n", dt_node_full_name(dev)); > + ret = 0; > + break; continue? You can't assume the order of the PCI busses in the device tree. > + } > + > + if ( ret < 0 ) > + return ret; > + > + printk("Mapped additional regions for PCIe device at 0x%"PRIx64"\n", > + addr); Printing the device tree path would be more helpful than the address. Regards,
On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote: > > + default: > > + /* Ignore unknown PCI busses */ > > I would add a > printk("Ignoring PCI busses %s\n", dt_node_full_name(dev)); > > > + ret = 0; > > + break; > > continue? Yes, that makes sense (probably the ret = is then unnecessary). > You can't assume the order of the PCI busses in the device tree. But, I don't understand what this has to do with using continue. > > > + } > > + > > + if ( ret < 0 ) > > + return ret; > > + > > + printk("Mapped additional regions for PCIe device at 0x%"PRIx64"\n", > > + addr); > > Printing the device tree path would be more helpful than the address. OK.
Hi Ian, On 19/11/2014 09:56, Ian Campbell wrote: > On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote: >>> + default: >>> + /* Ignore unknown PCI busses */ >> >> I would add a >> printk("Ignoring PCI busses %s\n", dt_node_full_name(dev)); >> >>> + ret = 0; >>> + break; >> >> continue? > > Yes, that makes sense (probably the ret = is then unnecessary). > >> You can't assume the order of the PCI busses in the device tree. > > But, I don't understand what this has to do with using continue. The current xgene-storm DTS has the different PCI busses ordered. So as soon as you don't find the PCI range, it means there is no more PCI busses. Without the continue, this patch gives the impression that you rely on the node order on the device tree. Regards,
On Wed, 2014-11-19 at 10:06 +0000, Julien Grall wrote: > Hi Ian, > > On 19/11/2014 09:56, Ian Campbell wrote: > > On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote: > >>> + default: > >>> + /* Ignore unknown PCI busses */ > >> > >> I would add a > >> printk("Ignoring PCI busses %s\n", dt_node_full_name(dev)); > >> > >>> + ret = 0; > >>> + break; > >> > >> continue? > > > > Yes, that makes sense (probably the ret = is then unnecessary). > > > >> You can't assume the order of the PCI busses in the device tree. > > > > But, I don't understand what this has to do with using continue. > > The current xgene-storm DTS has the different PCI busses ordered. So as > soon as you don't find the PCI range, it means there is no more PCI busses. I don't think it does, the patch iterates over all of the buses, even ones we don't understand, we don't give up at the first one we don't grok. > Without the continue, this patch gives the impression that you rely on > the node order on the device tree. > > Regards, >
On 19/11/2014 10:18, Ian Campbell wrote: > On Wed, 2014-11-19 at 10:06 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 19/11/2014 09:56, Ian Campbell wrote: >>> On Tue, 2014-11-18 at 17:15 +0000, Julien Grall wrote: >>>>> + default: >>>>> + /* Ignore unknown PCI busses */ >>>> >>>> I would add a >>>> printk("Ignoring PCI busses %s\n", dt_node_full_name(dev)); >>>> >>>>> + ret = 0; >>>>> + break; >>>> >>>> continue? >>> >>> Yes, that makes sense (probably the ret = is then unnecessary). >>> >>>> You can't assume the order of the PCI busses in the device tree. >>> >>> But, I don't understand what this has to do with using continue. >> >> The current xgene-storm DTS has the different PCI busses ordered. So as >> soon as you don't find the PCI range, it means there is no more PCI busses. > > I don't think it does, the patch iterates over all of the buses, even > ones we don't understand, we don't give up at the first one we don't > grok. Hrmm you are right. I don't know why I though the break were bound to the loop and not the switch. Sorry for the noise. Regards,
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c index 6c432a1..926c325 100644 --- a/xen/arch/arm/platforms/xgene-storm.c +++ b/xen/arch/arm/platforms/xgene-storm.c @@ -78,35 +78,31 @@ static int map_one_spi(struct domain *d, const char *what, return ret; } -/* - * Xen does not currently support mapping MMIO regions and interrupt - * for bus child devices (referenced via the "ranges" and - * "interrupt-map" properties to domain 0). Instead for now map the - * necessary resources manually. - */ -static int xgene_storm_specific_mapping(struct domain *d) +/* Creates MMIO mappings base..end as well as 4 SPIs from the given base. */ +static int xgene_storm_pcie_specific_mapping(struct domain *d, + paddr_t base, paddr_t end, + int base_spi) { int ret; /* Map the PCIe bus resources */ - ret = map_one_mmio(d, "PCI MEMORY", paddr_to_pfn(0x0e000000000UL), - paddr_to_pfn(0x01000000000UL)); + ret = map_one_mmio(d, "PCI MEMORY", paddr_to_pfn(base), paddr_to_pfn(end)); if ( ret ) goto err; - ret = map_one_spi(d, "PCI#INTA", 0xc2, DT_IRQ_TYPE_LEVEL_HIGH); + ret = map_one_spi(d, "PCI#INTA", base_spi+0, DT_IRQ_TYPE_LEVEL_HIGH); if ( ret ) goto err; - ret = map_one_spi(d, "PCI#INTB", 0xc3, DT_IRQ_TYPE_LEVEL_HIGH); + ret = map_one_spi(d, "PCI#INTB", base_spi+1, DT_IRQ_TYPE_LEVEL_HIGH); if ( ret ) goto err; - ret = map_one_spi(d, "PCI#INTC", 0xc4, DT_IRQ_TYPE_LEVEL_HIGH); + ret = map_one_spi(d, "PCI#INTC", base_spi+2, DT_IRQ_TYPE_LEVEL_HIGH); if ( ret ) goto err; - ret = map_one_spi(d, "PCI#INTD", 0xc5, DT_IRQ_TYPE_LEVEL_HIGH); + ret = map_one_spi(d, "PCI#INTD", base_spi+3, DT_IRQ_TYPE_LEVEL_HIGH); if ( ret ) goto err; @@ -115,6 +111,68 @@ err: return ret; } +/* + * Xen does not currently support mapping MMIO regions and interrupt + * for bus child devices (referenced via the "ranges" and + * "interrupt-map" properties to domain 0). Instead for now map the + * necessary resources manually. + */ +static int xgene_storm_specific_mapping(struct domain *d) +{ + struct dt_device_node *node = NULL; + int ret; + + while ( (node = dt_find_compatible_node(node, "pci", "apm,xgene-pcie")) ) + { + u64 addr; + + /* Identify the bus via it's control register address */ + ret = dt_device_get_address(node, 0, &addr, NULL); + if ( ret < 0 ) + return ret; + + if ( !dt_device_is_available(node) ) + continue; + + switch ( addr ) + { + case 0x1f2b0000: /* PCIe0 */ + ret = xgene_storm_pcie_specific_mapping(d, + 0x0e000000000UL, 0x10000000000UL, 0xc2); + break; + case 0x1f2c0000: /* PCIe1 */ + ret = xgene_storm_pcie_specific_mapping(d, + 0x0d000000000UL, 0x0e000000000UL, 0xc8); + break; + case 0x1f2d0000: /* PCIe2 */ + ret = xgene_storm_pcie_specific_mapping(d, + 0x09000000000UL, 0x0a000000000UL, 0xce); + break; + case 0x1f500000: /* PCIe3 */ + ret = xgene_storm_pcie_specific_mapping(d, + 0x0a000000000UL, 0x0c000000000UL, 0xd4); + break; + case 0x1f510000: /* PCIe4 */ + ret = xgene_storm_pcie_specific_mapping(d, + 0x0c000000000UL, 0x0d000000000UL, 0xda); + break; + + default: + /* Ignore unknown PCI busses */ + ret = 0; + break; + } + + if ( ret < 0 ) + return ret; + + printk("Mapped additional regions for PCIe device at 0x%"PRIx64"\n", + addr); + } + + return 0; +} + static void xgene_storm_reset(void) { void __iomem *addr;
Currently we only establish specific mappings for pcie0, which is used on the Mustang platform. However at least McDivitt uses pcie3. So wire up all the others, based on whether the corresponding DT node is marked as available. This results in no change for Mustang. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- xen/arch/arm/platforms/xgene-storm.c | 84 ++++++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 13 deletions(-)