| Message ID | 20200521130008.8266-1-lorenzo.pieralisi@arm.com |
|---|---|
| Headers | show |
| Series | ACPI/OF: Upgrade MSI/IOMMU ID mapping APIs | expand |
Hi Lorenzo, On 5/21/2020 4:00 PM, Lorenzo Pieralisi wrote: > From: Diana Craciun <diana.craciun@oss.nxp.com> > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to > extract memory and other resources. > > Interrupt (GIC ITS) information is extracted from the MADT table > by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > > IORT table is parsed to configure DMA. > > Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > --- The author of this patch should be Makarand. I think I accidentaly broke it when we exchanged the patches. Very sorry about it. --- Best Regards, Laurentiu > drivers/bus/fsl-mc/fsl-mc-bus.c | 73 +++++++++++++++----- > drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++----- > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 75 ++++++++++++++++++++- > 3 files changed, 150 insertions(+), 35 deletions(-) > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c > index 824ff77bbe86..324d49d6df89 100644 > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c > @@ -18,6 +18,8 @@ > #include <linux/bitops.h> > #include <linux/msi.h> > #include <linux/dma-mapping.h> > +#include <linux/acpi.h> > +#include <linux/iommu.h> > > #include "fsl-mc-private.h" > > @@ -38,6 +40,7 @@ struct fsl_mc { > struct fsl_mc_device *root_mc_bus_dev; > u8 num_translation_ranges; > struct fsl_mc_addr_translation_range *translation_ranges; > + void *fsl_mc_regs; > }; > > /** > @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { > phys_addr_t start_phys_addr; > }; > > +#define FSL_MC_FAPR 0x28 > +#define MC_FAPR_PL BIT(18) > +#define MC_FAPR_BMT BIT(17) > + > /** > * fsl_mc_bus_match - device to driver matching callback > * @dev: the fsl-mc device to match against > @@ -124,7 +131,10 @@ static int fsl_mc_dma_configure(struct device *dev) > while (dev_is_fsl_mc(dma_dev)) > dma_dev = dma_dev->parent; > > - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > + if (dev_of_node(dma_dev)) > + return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > + > + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); > } > > static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, > @@ -865,8 +875,11 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > struct fsl_mc_io *mc_io = NULL; > int container_id; > phys_addr_t mc_portal_phys_addr; > - u32 mc_portal_size; > - struct resource res; > + u32 mc_portal_size, mc_stream_id; > + struct resource *plat_res; > + > + if (!iommu_present(&fsl_mc_bus_type)) > + return -EPROBE_DEFER; > > mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > if (!mc) > @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, mc); > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > + if (IS_ERR(mc->fsl_mc_regs)) > + return PTR_ERR(mc->fsl_mc_regs); > + > + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > + /* > + * HW ORs the PL and BMT bit, places the result in bit 15 of > + * the StreamID and ORs in the ICID. Calculate it accordingly. > + */ > + mc_stream_id = (mc_stream_id & 0xffff) | > + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > + 0x4000 : 0); > + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, > + &mc_stream_id); > + if (error) > + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > + error); > + } > + > /* > * Get physical address of MC portal for the root DPRC: > */ > - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > - if (error < 0) { > - dev_err(&pdev->dev, > - "of_address_to_resource() failed for %pOF\n", > - pdev->dev.of_node); > - return error; > - } > - > - mc_portal_phys_addr = res.start; > - mc_portal_size = resource_size(&res); > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mc_portal_phys_addr = plat_res->start; > + mc_portal_size = resource_size(plat_res); > error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > mc_portal_size, NULL, > FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io); > @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > mc_version.major, mc_version.minor, mc_version.revision); > > - error = get_mc_addr_translation_ranges(&pdev->dev, > - &mc->translation_ranges, > - &mc->num_translation_ranges); > - if (error < 0) > - goto error_cleanup_mc_io; > + if (dev_of_node(&pdev->dev)) { > + error = get_mc_addr_translation_ranges(&pdev->dev, > + &mc->translation_ranges, > + &mc->num_translation_ranges); > + if (error < 0) > + goto error_cleanup_mc_io; > + } > > error = dprc_get_container_id(mc_io, 0, &container_id); > if (error < 0) { > @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > goto error_cleanup_mc_io; > > mc->root_mc_bus_dev = mc_bus_dev; > + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > return 0; > > error_cleanup_mc_io: > @@ -967,11 +997,18 @@ static const struct of_device_id fsl_mc_bus_match_table[] = { > > MODULE_DEVICE_TABLE(of, fsl_mc_bus_match_table); > > +static const struct acpi_device_id fsl_mc_bus_acpi_match_table[] = { > + {"NXP0008", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, fsl_mc_bus_acpi_match_table); > + > static struct platform_driver fsl_mc_bus_driver = { > .driver = { > .name = "fsl_mc_bus", > .pm = NULL, > .of_match_table = fsl_mc_bus_match_table, > + .acpi_match_table = fsl_mc_bus_acpi_match_table, > }, > .probe = fsl_mc_bus_probe, > .remove = fsl_mc_bus_remove, > diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c b/drivers/bus/fsl-mc/fsl-mc-msi.c > index e7bbff445a83..8edadf05cbb7 100644 > --- a/drivers/bus/fsl-mc/fsl-mc-msi.c > +++ b/drivers/bus/fsl-mc/fsl-mc-msi.c > @@ -13,6 +13,7 @@ > #include <linux/irq.h> > #include <linux/irqdomain.h> > #include <linux/msi.h> > +#include <linux/acpi_iort.h> > > #include "fsl-mc-private.h" > > @@ -179,25 +180,31 @@ struct irq_domain *fsl_mc_msi_create_irq_domain(struct fwnode_handle *fwnode, > > struct irq_domain *fsl_mc_find_msi_domain(struct device *dev) > { > - struct irq_domain *msi_domain = NULL; > + struct device *root_dprc_dev; > + struct device *bus_dev; > + struct irq_domain *msi_domain; > struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > > - msi_domain = of_msi_map_get_device_domain(dev, mc_dev->icid, > + fsl_mc_get_root_dprc(dev, &root_dprc_dev); > + bus_dev = root_dprc_dev->parent; > + > + if (bus_dev->of_node) { > + msi_domain = of_msi_map_get_device_domain(dev, > + mc_dev->icid, > DOMAIN_BUS_FSL_MC_MSI); > > - /* > - * if the msi-map property is missing assume that all the > - * child containers inherit the domain from the parent > - */ > - if (!msi_domain) { > - struct device *root_dprc_dev; > - struct device *bus_dev; > - > - fsl_mc_get_root_dprc(dev, &root_dprc_dev); > - bus_dev = root_dprc_dev->parent; > - msi_domain = of_msi_get_domain(bus_dev, > - bus_dev->of_node, > - DOMAIN_BUS_FSL_MC_MSI); > + /* > + * if the msi-map property is missing assume that all the > + * child containers inherit the domain from the parent > + */ > + if (!msi_domain) > + > + msi_domain = of_msi_get_domain(bus_dev, > + bus_dev->of_node, > + DOMAIN_BUS_FSL_MC_MSI); > + } else { > + msi_domain = iort_get_device_domain(dev, mc_dev->icid, > + DOMAIN_BUS_FSL_MC_MSI); > } > > return msi_domain; > diff --git a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c > index a5c8d577e424..b8b948fb6b2d 100644 > --- a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c > +++ b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c > @@ -7,6 +7,8 @@ > * > */ > > +#include <linux/acpi.h> > +#include <linux/acpi_iort.h> > #include <linux/of_device.h> > #include <linux/of_address.h> > #include <linux/irq.h> > @@ -30,7 +32,8 @@ static u32 fsl_mc_msi_domain_get_msi_id(struct irq_domain *domain, > u32 out_id; > > of_node = irq_domain_get_of_node(domain); > - out_id = of_msi_map_id(&mc_dev->dev, of_node, mc_dev->icid); > + out_id = of_node ? of_msi_map_id(&mc_dev->dev, of_node, mc_dev->icid) : > + iort_msi_map_id(&mc_dev->dev, mc_dev->icid); > > return out_id; > } > @@ -79,7 +82,67 @@ static const struct of_device_id its_device_id[] = { > {}, > }; > > -static int __init its_fsl_mc_msi_init(void) > +static int __init its_fsl_mc_msi_init_one(struct fwnode_handle *handle, > + const char *name) > +{ > + struct irq_domain *parent; > + struct irq_domain *mc_msi_domain; > + > + parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS); > + if (!parent || !msi_get_domain_info(parent)) { > + pr_err("%s: Unable to locate ITS domain\n", name); > + return -ENXIO; > + } > + > + mc_msi_domain = fsl_mc_msi_create_irq_domain(handle, > + &its_fsl_mc_msi_domain_info, > + parent); > + if (!mc_msi_domain) > + pr_err("ACPIF: unable to create fsl-mc domain\n"); > + > + pr_info("fsl-mc MSI: domain created\n"); > + > + return 0; > +} > + > +static int __init > +its_fsl_mc_msi_parse_madt(union acpi_subtable_headers *header, > + const unsigned long end) > +{ > + struct acpi_madt_generic_translator *its_entry; > + struct fwnode_handle *dom_handle; > + const char *node_name; > + int err = -ENXIO; > + > + its_entry = (struct acpi_madt_generic_translator *)header; > + node_name = kasprintf(GFP_KERNEL, "ITS@0x%lx", > + (long)its_entry->base_address); > + > + dom_handle = iort_find_domain_token(its_entry->translation_id); > + if (!dom_handle) { > + pr_err("%s: Unable to locate ITS domain handle\n", node_name); > + goto out; > + } > + > + err = its_fsl_mc_msi_init_one(dom_handle, node_name); > + if (!err) > + pr_info("fsl-mc MSI: %s domain created\n", node_name); > + > +out: > + kfree(node_name); > + return err; > +} > + > + > +static int __init its_fsl_mc_acpi_msi_init(void) > +{ > + acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > + its_fsl_mc_msi_parse_madt, 0); > + > + return 0; > +} > + > +static int __init its_fsl_mc_of_msi_init(void) > { > struct device_node *np; > struct irq_domain *parent; > @@ -113,4 +176,12 @@ static int __init its_fsl_mc_msi_init(void) > return 0; > } > > +static int __init its_fsl_mc_msi_init(void) > +{ > + its_fsl_mc_of_msi_init(); > + its_fsl_mc_acpi_msi_init(); > + > + return 0; > +} > + > early_initcall(its_fsl_mc_msi_init); >
Hi Lorenzo, > -----Original Message----- > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > Sent: Thursday, May 21, 2020 8:33 PM > To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; linux-arm- > kernel@lists.infradead.org > Cc: Diana Madalina Craciun (OSS) <diana.craciun@oss.nxp.com>; Makarand > Pawagi <makarand.pawagi@nxp.com>; iommu@lists.linux-foundation.org; > linux-acpi@vger.kernel.org; devicetree@vger.kernel.org; linux- > pci@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Rafael J. Wysocki > <rjw@rjwysocki.net>; Joerg Roedel <joro@8bytes.org>; Hanjun Guo > <guohanjun@huawei.com>; Bjorn Helgaas <bhelgaas@google.com>; Sudeep > Holla <sudeep.holla@arm.com>; Robin Murphy <robin.murphy@arm.com>; > Catalin Marinas <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; > Marc Zyngier <maz@kernel.org> > Subject: Re: [PATCH 12/12] bus: fsl-mc: Add ACPI support for fsl-mc > > Hi Lorenzo, > > On 5/21/2020 4:00 PM, Lorenzo Pieralisi wrote: > > From: Diana Craciun <diana.craciun@oss.nxp.com> > > > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to > > extract memory and other resources. > > > > Interrupt (GIC ITS) information is extracted from the MADT table by > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > > > > IORT table is parsed to configure DMA. > > > > Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > --- > > The author of this patch should be Makarand. I think I accidentaly broke it when > we exchanged the patches. Very sorry about it. > Will you be able to correct this or should I post another patch? > --- > Best Regards, Laurentiu > > > > drivers/bus/fsl-mc/fsl-mc-bus.c | 73 +++++++++++++++----- > > drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++----- > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 75 > > ++++++++++++++++++++- > > 3 files changed, 150 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c > > b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89 > > 100644 > > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c > > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c > > @@ -18,6 +18,8 @@ > > #include <linux/bitops.h> > > #include <linux/msi.h> > > #include <linux/dma-mapping.h> > > +#include <linux/acpi.h> > > +#include <linux/iommu.h> > > > > #include "fsl-mc-private.h" > > > > @@ -38,6 +40,7 @@ struct fsl_mc { > > struct fsl_mc_device *root_mc_bus_dev; > > u8 num_translation_ranges; > > struct fsl_mc_addr_translation_range *translation_ranges; > > + void *fsl_mc_regs; > > }; > > > > /** > > @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { > > phys_addr_t start_phys_addr; > > }; > > > > +#define FSL_MC_FAPR 0x28 > > +#define MC_FAPR_PL BIT(18) > > +#define MC_FAPR_BMT BIT(17) > > + > > /** > > * fsl_mc_bus_match - device to driver matching callback > > * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@ > > static int fsl_mc_dma_configure(struct device *dev) > > while (dev_is_fsl_mc(dma_dev)) > > dma_dev = dma_dev->parent; > > > > - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > > + if (dev_of_node(dma_dev)) > > + return of_dma_configure_id(dev, dma_dev->of_node, 0, > &input_id); > > + > > + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); > > } > > > > static ssize_t modalias_show(struct device *dev, struct > > device_attribute *attr, @@ -865,8 +875,11 @@ static int > fsl_mc_bus_probe(struct platform_device *pdev) > > struct fsl_mc_io *mc_io = NULL; > > int container_id; > > phys_addr_t mc_portal_phys_addr; > > - u32 mc_portal_size; > > - struct resource res; > > + u32 mc_portal_size, mc_stream_id; > > + struct resource *plat_res; > > + > > + if (!iommu_present(&fsl_mc_bus_type)) > > + return -EPROBE_DEFER; > > > > mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > > if (!mc) > > @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct > > platform_device *pdev) > > > > platform_set_drvdata(pdev, mc); > > > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > > + if (IS_ERR(mc->fsl_mc_regs)) > > + return PTR_ERR(mc->fsl_mc_regs); > > + > > + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > > + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > > + /* > > + * HW ORs the PL and BMT bit, places the result in bit 15 of > > + * the StreamID and ORs in the ICID. Calculate it accordingly. > > + */ > > + mc_stream_id = (mc_stream_id & 0xffff) | > > + ((mc_stream_id & (MC_FAPR_PL | > MC_FAPR_BMT)) ? > > + 0x4000 : 0); > > + error = acpi_dma_configure_id(&pdev->dev, > DEV_DMA_COHERENT, > > + &mc_stream_id); > > + if (error) > > + dev_warn(&pdev->dev, "failed to configure > dma: %d.\n", > > + error); > > + } > > + > > /* > > * Get physical address of MC portal for the root DPRC: > > */ > > - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > > - if (error < 0) { > > - dev_err(&pdev->dev, > > - "of_address_to_resource() failed for %pOF\n", > > - pdev->dev.of_node); > > - return error; > > - } > > - > > - mc_portal_phys_addr = res.start; > > - mc_portal_size = resource_size(&res); > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + mc_portal_phys_addr = plat_res->start; > > + mc_portal_size = resource_size(plat_res); > > error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > > mc_portal_size, NULL, > > FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, > &mc_io); @@ -903,11 +930,13 @@ > > static int fsl_mc_bus_probe(struct platform_device *pdev) > > dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > > mc_version.major, mc_version.minor, mc_version.revision); > > > > - error = get_mc_addr_translation_ranges(&pdev->dev, > > - &mc->translation_ranges, > > - &mc->num_translation_ranges); > > - if (error < 0) > > - goto error_cleanup_mc_io; > > + if (dev_of_node(&pdev->dev)) { > > + error = get_mc_addr_translation_ranges(&pdev->dev, > > + &mc->translation_ranges, > > + &mc- > >num_translation_ranges); > > + if (error < 0) > > + goto error_cleanup_mc_io; > > + } > > > > error = dprc_get_container_id(mc_io, 0, &container_id); > > if (error < 0) { > > @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device > *pdev) > > goto error_cleanup_mc_io; > > > > mc->root_mc_bus_dev = mc_bus_dev; > > + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > > return 0; > > > > error_cleanup_mc_io: > > @@ -967,11 +997,18 @@ static const struct of_device_id > > fsl_mc_bus_match_table[] = { > > > > MODULE_DEVICE_TABLE(of, fsl_mc_bus_match_table); > > > > +static const struct acpi_device_id fsl_mc_bus_acpi_match_table[] = { > > + {"NXP0008", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(acpi, fsl_mc_bus_acpi_match_table); > > + > > static struct platform_driver fsl_mc_bus_driver = { > > .driver = { > > .name = "fsl_mc_bus", > > .pm = NULL, > > .of_match_table = fsl_mc_bus_match_table, > > + .acpi_match_table = fsl_mc_bus_acpi_match_table, > > }, > > .probe = fsl_mc_bus_probe, > > .remove = fsl_mc_bus_remove, > > diff --git a/drivers/bus/fsl-mc/fsl-mc-msi.c > > b/drivers/bus/fsl-mc/fsl-mc-msi.c index e7bbff445a83..8edadf05cbb7 > > 100644 > > --- a/drivers/bus/fsl-mc/fsl-mc-msi.c > > +++ b/drivers/bus/fsl-mc/fsl-mc-msi.c > > @@ -13,6 +13,7 @@ > > #include <linux/irq.h> > > #include <linux/irqdomain.h> > > #include <linux/msi.h> > > +#include <linux/acpi_iort.h> > > > > #include "fsl-mc-private.h" > > > > @@ -179,25 +180,31 @@ struct irq_domain > > *fsl_mc_msi_create_irq_domain(struct fwnode_handle *fwnode, > > > > struct irq_domain *fsl_mc_find_msi_domain(struct device *dev) { > > - struct irq_domain *msi_domain = NULL; > > + struct device *root_dprc_dev; > > + struct device *bus_dev; > > + struct irq_domain *msi_domain; > > struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > > > > - msi_domain = of_msi_map_get_device_domain(dev, mc_dev->icid, > > + fsl_mc_get_root_dprc(dev, &root_dprc_dev); > > + bus_dev = root_dprc_dev->parent; > > + > > + if (bus_dev->of_node) { > > + msi_domain = of_msi_map_get_device_domain(dev, > > + mc_dev->icid, > > DOMAIN_BUS_FSL_MC_MSI); > > > > - /* > > - * if the msi-map property is missing assume that all the > > - * child containers inherit the domain from the parent > > - */ > > - if (!msi_domain) { > > - struct device *root_dprc_dev; > > - struct device *bus_dev; > > - > > - fsl_mc_get_root_dprc(dev, &root_dprc_dev); > > - bus_dev = root_dprc_dev->parent; > > - msi_domain = of_msi_get_domain(bus_dev, > > - bus_dev->of_node, > > - DOMAIN_BUS_FSL_MC_MSI); > > + /* > > + * if the msi-map property is missing assume that all the > > + * child containers inherit the domain from the parent > > + */ > > + if (!msi_domain) > > + > > + msi_domain = of_msi_get_domain(bus_dev, > > + bus_dev->of_node, > > + DOMAIN_BUS_FSL_MC_MSI); > > + } else { > > + msi_domain = iort_get_device_domain(dev, mc_dev->icid, > > + > DOMAIN_BUS_FSL_MC_MSI); > > } > > > > return msi_domain; > > diff --git a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c > > b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c > > index a5c8d577e424..b8b948fb6b2d 100644 > > --- a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c > > +++ b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c > > @@ -7,6 +7,8 @@ > > * > > */ > > > > +#include <linux/acpi.h> > > +#include <linux/acpi_iort.h> > > #include <linux/of_device.h> > > #include <linux/of_address.h> > > #include <linux/irq.h> > > @@ -30,7 +32,8 @@ static u32 fsl_mc_msi_domain_get_msi_id(struct > irq_domain *domain, > > u32 out_id; > > > > of_node = irq_domain_get_of_node(domain); > > - out_id = of_msi_map_id(&mc_dev->dev, of_node, mc_dev->icid); > > + out_id = of_node ? of_msi_map_id(&mc_dev->dev, of_node, mc_dev- > >icid) : > > + iort_msi_map_id(&mc_dev->dev, mc_dev->icid); > > > > return out_id; > > } > > @@ -79,7 +82,67 @@ static const struct of_device_id its_device_id[] = { > > {}, > > }; > > > > -static int __init its_fsl_mc_msi_init(void) > > +static int __init its_fsl_mc_msi_init_one(struct fwnode_handle *handle, > > + const char *name) > > +{ > > + struct irq_domain *parent; > > + struct irq_domain *mc_msi_domain; > > + > > + parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS); > > + if (!parent || !msi_get_domain_info(parent)) { > > + pr_err("%s: Unable to locate ITS domain\n", name); > > + return -ENXIO; > > + } > > + > > + mc_msi_domain = fsl_mc_msi_create_irq_domain(handle, > > + &its_fsl_mc_msi_domain_info, > > + parent); > > + if (!mc_msi_domain) > > + pr_err("ACPIF: unable to create fsl-mc domain\n"); > > + > > + pr_info("fsl-mc MSI: domain created\n"); > > + > > + return 0; > > +} > > + > > +static int __init > > +its_fsl_mc_msi_parse_madt(union acpi_subtable_headers *header, > > + const unsigned long end) > > +{ > > + struct acpi_madt_generic_translator *its_entry; > > + struct fwnode_handle *dom_handle; > > + const char *node_name; > > + int err = -ENXIO; > > + > > + its_entry = (struct acpi_madt_generic_translator *)header; > > + node_name = kasprintf(GFP_KERNEL, "ITS@0x%lx", > > + (long)its_entry->base_address); > > + > > + dom_handle = iort_find_domain_token(its_entry->translation_id); > > + if (!dom_handle) { > > + pr_err("%s: Unable to locate ITS domain handle\n", > node_name); > > + goto out; > > + } > > + > > + err = its_fsl_mc_msi_init_one(dom_handle, node_name); > > + if (!err) > > + pr_info("fsl-mc MSI: %s domain created\n", node_name); > > + > > +out: > > + kfree(node_name); > > + return err; > > +} > > + > > + > > +static int __init its_fsl_mc_acpi_msi_init(void) { > > + acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, > > + its_fsl_mc_msi_parse_madt, 0); > > + > > + return 0; > > +} > > + > > +static int __init its_fsl_mc_of_msi_init(void) > > { > > struct device_node *np; > > struct irq_domain *parent; > > @@ -113,4 +176,12 @@ static int __init its_fsl_mc_msi_init(void) > > return 0; > > } > > > > +static int __init its_fsl_mc_msi_init(void) { > > + its_fsl_mc_of_msi_init(); > > + its_fsl_mc_acpi_msi_init(); > > + > > + return 0; > > +} > > + > > early_initcall(its_fsl_mc_msi_init); > >
On Fri, May 22, 2020 at 05:32:02AM +0000, Makarand Pawagi wrote: [...] > > Subject: Re: [PATCH 12/12] bus: fsl-mc: Add ACPI support for fsl-mc > > > > Hi Lorenzo, > > > > On 5/21/2020 4:00 PM, Lorenzo Pieralisi wrote: > > > From: Diana Craciun <diana.craciun@oss.nxp.com> > > > > > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to > > > extract memory and other resources. > > > > > > Interrupt (GIC ITS) information is extracted from the MADT table by > > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > > > > > > IORT table is parsed to configure DMA. > > > > > > Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > > > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > > > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > > --- > > > > The author of this patch should be Makarand. I think I accidentaly broke it when > > we exchanged the patches. Very sorry about it. > > > > Will you be able to correct this or should I post another patch? I will update it. Lorenzo
This series is a v2 of a previous posting:
v1 -> v2
- Removed _rid() wrappers
- Fixed !CONFIG_ACPI compilation issue
- Converted of_pci_iommu_init() to use of_iommu_configure_dev_id()
v1: https://lore.kernel.org/linux-arm-kernel/20200521130008.8266-1-lorenzo.pieralisi@arm.com/
Original cover letter
---------------------
Firmware bindings provided in the ACPI IORT table[1] and device tree
bindings define rules to carry out input/output ID mappings - ie
retrieving an IOMMU/MSI controller input ID for a device with a given
ID.
At the moment these firmware bindings are used exclusively for PCI
devices and their requester ID to IOMMU/MSI id mapping but there is
nothing PCI specific in the ACPI and devicetree bindings that prevent
the firmware and kernel from using the firmware bindings to traslate
device IDs for any bus that requires its devices to carry out
input/output id translations.
The Freescale FSL bus is an example whereby the input/output ID
translation kernel code put in place for PCI can be reused for devices
attached to the bus that are not PCI devices.
This series updates the kernel code to make the MSI/IOMMU input/output
ID translation PCI agnostic and apply the resulting changes to the
device ID space provided by the Freescale FSL bus.
[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf
Cc: Rob Herring <robh+dt@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: "Joerg Roedel <joro@8bytes.org>
Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Diana Craciun (2):
of/irq: make of_msi_map_get_device_domain() bus agnostic
bus/fsl-mc: Refactor the MSI domain creation in the DPRC driver
Laurentiu Tudor (1):
dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus
Lorenzo Pieralisi (8):
ACPI/IORT: Make iort_match_node_callback walk the ACPI namespace for
NC
ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic
ACPI/IORT: Make iort_msi_map_rid() PCI agnostic
ACPI/IORT: Remove useless PCI bus walk
ACPI/IORT: Add an input ID to acpi_dma_configure()
of/iommu: Make of_map_rid() PCI agnostic
of/device: Add input id to of_dma_configure()
of/irq: Make of_msi_map_rid() PCI bus agnostic
Makarand Pawagi (1):
bus: fsl-mc: Add ACPI support for fsl-mc
.../devicetree/bindings/misc/fsl,qoriq-mc.txt | 50 +++++++-
drivers/acpi/arm64/iort.c | 108 ++++++++++++------
drivers/acpi/scan.c | 8 +-
drivers/bus/fsl-mc/dprc-driver.c | 31 ++---
drivers/bus/fsl-mc/fsl-mc-bus.c | 79 +++++++++----
drivers/bus/fsl-mc/fsl-mc-msi.c | 36 ++++--
drivers/bus/fsl-mc/fsl-mc-private.h | 6 +-
drivers/iommu/of_iommu.c | 81 +++++++------
drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 105 ++++++++++++++---
drivers/of/base.c | 42 +++----
drivers/of/device.c | 8 +-
drivers/of/irq.c | 34 +++---
drivers/pci/msi.c | 9 +-
include/acpi/acpi_bus.h | 9 +-
include/linux/acpi.h | 7 ++
include/linux/acpi_iort.h | 20 ++--
include/linux/of.h | 4 +-
include/linux/of_device.h | 16 ++-
include/linux/of_iommu.h | 6 +-
include/linux/of_irq.h | 13 ++-
20 files changed, 451 insertions(+), 221 deletions(-)
Hi Lorenzo, On 2020/6/19 16:20, Lorenzo Pieralisi wrote: > When the iort_match_node_callback is invoked for a named component > the match should be executed upon a device with an ACPI companion. > > For devices with no ACPI companion set-up the ACPI device tree must be > walked in order to find the first parent node with a companion set and > check the parent node against the named component entry to check whether > there is a match and therefore an IORT node describing the in/out ID > translation for the device has been found. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Hanjun Guo <guohanjun@huawei.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > --- > drivers/acpi/arm64/iort.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 28a6b387e80e..5eee81758184 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -264,15 +264,31 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node, > > if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) { > struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; > - struct acpi_device *adev = to_acpi_device_node(dev->fwnode); > + struct acpi_device *adev; > struct acpi_iort_named_component *ncomp; > + struct device *nc_dev = dev; > + > + /* > + * Walk the device tree to find a device with an > + * ACPI companion; there is no point in scanning > + * IORT for a device matching a named component if > + * the device does not have an ACPI companion to > + * start with. > + */ > + do { > + adev = ACPI_COMPANION(nc_dev); > + if (adev) > + break; > + > + nc_dev = nc_dev->parent; > + } while (nc_dev); I'm lost here, we need the ACPI_COMPANION (the same as to_acpi_device_node()) of the device, but why do we need to go up to find the parent node? For a platform device, if I use its parent's full path name for its named component entry, then it will match, but this will violate the IORT spec. Thanks Hanjun
On Mon, Jun 29, 2020 at 12:24:43PM +0800, Hanjun Guo wrote: > Hi Lorenzo, > > On 2020/6/19 16:20, Lorenzo Pieralisi wrote: > > When the iort_match_node_callback is invoked for a named component > > the match should be executed upon a device with an ACPI companion. > > > > For devices with no ACPI companion set-up the ACPI device tree must be > > walked in order to find the first parent node with a companion set and > > check the parent node against the named component entry to check whether > > there is a match and therefore an IORT node describing the in/out ID > > translation for the device has been found. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Hanjun Guo <guohanjun@huawei.com> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > --- > > drivers/acpi/arm64/iort.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index 28a6b387e80e..5eee81758184 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -264,15 +264,31 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node, > > if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) { > > struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; > > - struct acpi_device *adev = to_acpi_device_node(dev->fwnode); > > + struct acpi_device *adev; > > struct acpi_iort_named_component *ncomp; > > + struct device *nc_dev = dev; > > + > > + /* > > + * Walk the device tree to find a device with an > > + * ACPI companion; there is no point in scanning > > + * IORT for a device matching a named component if > > + * the device does not have an ACPI companion to > > + * start with. > > + */ > > + do { > > + adev = ACPI_COMPANION(nc_dev); > > + if (adev) > > + break; > > + > > + nc_dev = nc_dev->parent; > > + } while (nc_dev); > > I'm lost here, we need the ACPI_COMPANION (the same as > to_acpi_device_node()) of the device, but why do we need to go > up to find the parent node? For devices that aren't described in the DSDT - IORT translations are determined by their ACPI parent device. Do you see/Have you found any issue with this approach ? > For a platform device, if I use its parent's full path name for > its named component entry, then it will match, but this will violate > the IORT spec. Can you elaborate on this please I don't get the point you are making. Thanks, Lorenzo
On 2020/6/29 17:05, Lorenzo Pieralisi wrote: > On Mon, Jun 29, 2020 at 12:24:43PM +0800, Hanjun Guo wrote: >> Hi Lorenzo, >> >> On 2020/6/19 16:20, Lorenzo Pieralisi wrote: >>> When the iort_match_node_callback is invoked for a named component >>> the match should be executed upon a device with an ACPI companion. >>> >>> For devices with no ACPI companion set-up the ACPI device tree must be >>> walked in order to find the first parent node with a companion set and >>> check the parent node against the named component entry to check whether >>> there is a match and therefore an IORT node describing the in/out ID >>> translation for the device has been found. >>> >>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>> Cc: Will Deacon <will@kernel.org> >>> Cc: Hanjun Guo <guohanjun@huawei.com> >>> Cc: Sudeep Holla <sudeep.holla@arm.com> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Robin Murphy <robin.murphy@arm.com> >>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >>> --- >>> drivers/acpi/arm64/iort.c | 20 ++++++++++++++++++-- >>> 1 file changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >>> index 28a6b387e80e..5eee81758184 100644 >>> --- a/drivers/acpi/arm64/iort.c >>> +++ b/drivers/acpi/arm64/iort.c >>> @@ -264,15 +264,31 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node, >>> if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) { >>> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; >>> - struct acpi_device *adev = to_acpi_device_node(dev->fwnode); >>> + struct acpi_device *adev; >>> struct acpi_iort_named_component *ncomp; >>> + struct device *nc_dev = dev; >>> + >>> + /* >>> + * Walk the device tree to find a device with an >>> + * ACPI companion; there is no point in scanning >>> + * IORT for a device matching a named component if >>> + * the device does not have an ACPI companion to >>> + * start with. >>> + */ >>> + do { >>> + adev = ACPI_COMPANION(nc_dev); >>> + if (adev) >>> + break; >>> + >>> + nc_dev = nc_dev->parent; >>> + } while (nc_dev); >> >> I'm lost here, we need the ACPI_COMPANION (the same as >> to_acpi_device_node()) of the device, but why do we need to go >> up to find the parent node? > > For devices that aren't described in the DSDT - IORT translations > are determined by their ACPI parent device. Do you see/Have you > found any issue with this approach ? The spec says "Describes the IO relationships between devices represented in the ACPI namespace.", and in section 3.1.1.3 Named component node, it says: "Named component nodes are used to describe devices that are also included in the Differentiated System Description Table (DSDT). See [ACPI]." So from my understanding, the IORT spec for now, can only do ID translations for devices in the DSDT. > >> For a platform device, if I use its parent's full path name for >> its named component entry, then it will match, but this will violate >> the IORT spec. > > Can you elaborate on this please I don't get the point you > are making. For example, device A is not described in DSDT so can't represent as a NC node in IORT. Device B can be described in DSDT and it is the parent of device A, so device B can be represented in IORT with memory access properties and node flags with Substream width and Stall supported info. When we trying to translate device A's ID, we reuse all the memory access properties and node flags from its parent (device B), but will it the same? So the IORT spec don't support this, at least it's pretty vague I think. Thanks Hanjun
On Tue, Jun 30, 2020 at 11:06:41AM +0800, Hanjun Guo wrote: [...] > > For devices that aren't described in the DSDT - IORT translations > > are determined by their ACPI parent device. Do you see/Have you > > found any issue with this approach ? > > The spec says "Describes the IO relationships between devices > represented in the ACPI namespace.", and in section 3.1.1.3 Named > component node, it says: PCI devices aren't necessarily described in the ACPI namespace and we still use IORT to describe them - through the RC node. > "Named component nodes are used to describe devices that are also > included in the Differentiated System Description Table (DSDT). See > [ACPI]." > > So from my understanding, the IORT spec for now, can only do ID > translations for devices in the DSDT. I think you can read this multiple ways but this patch does not change this concept. What changes, is applying parent's node IORT mapping to child nodes with no associated DSDT nodes, it is the same thing we do with PCI and the _DMA method - we could update the wording in the specs if that clarifies but I don't think this deliberately disregards the specifications. > > > For a platform device, if I use its parent's full path name for > > > its named component entry, then it will match, but this will violate > > > the IORT spec. > > > > Can you elaborate on this please I don't get the point you > > are making. > > For example, device A is not described in DSDT so can't represent > as a NC node in IORT. Device B can be described in DSDT and it > is the parent of device A, so device B can be represented in IORT > with memory access properties and node flags with Substream width > and Stall supported info. > > When we trying to translate device A's ID, we reuse all the memory > access properties and node flags from its parent (device B), but > will it the same? I assume so why wouldn't it be ? Why would be describe them in a parent-child relationship if that's not how the system looks like in HW ? Do you have a specific example in mind that we should be aware of ? > So the IORT spec don't support this, at least it's pretty vague > I think. I think that's a matter of wording, it can be updated if it needs be, reach out if you see any issue with the current approach please. Thanks, Lorenzo
On 2020/6/30 18:24, Lorenzo Pieralisi wrote: > On Tue, Jun 30, 2020 at 11:06:41AM +0800, Hanjun Guo wrote: > > [...] > >>> For devices that aren't described in the DSDT - IORT translations >>> are determined by their ACPI parent device. Do you see/Have you >>> found any issue with this approach ? >> >> The spec says "Describes the IO relationships between devices >> represented in the ACPI namespace.", and in section 3.1.1.3 Named >> component node, it says: > > PCI devices aren't necessarily described in the ACPI namespace and we > still use IORT to describe them - through the RC node. > >> "Named component nodes are used to describe devices that are also >> included in the Differentiated System Description Table (DSDT). See >> [ACPI]." >> >> So from my understanding, the IORT spec for now, can only do ID >> translations for devices in the DSDT. > > I think you can read this multiple ways but this patch does not > change this concept. What changes, is applying parent's node IORT > mapping to child nodes with no associated DSDT nodes, it is the > same thing we do with PCI and the _DMA method - we could update > the wording in the specs if that clarifies but I don't think this > deliberately disregards the specifications. I agree, but it's better to update the wording of the spec. > >>>> For a platform device, if I use its parent's full path name for >>>> its named component entry, then it will match, but this will violate >>>> the IORT spec. >>> >>> Can you elaborate on this please I don't get the point you >>> are making. >> >> For example, device A is not described in DSDT so can't represent >> as a NC node in IORT. Device B can be described in DSDT and it >> is the parent of device A, so device B can be represented in IORT >> with memory access properties and node flags with Substream width >> and Stall supported info. >> >> When we trying to translate device A's ID, we reuse all the memory >> access properties and node flags from its parent (device B), but >> will it the same? > > I assume so why wouldn't it be ? Why would be describe them in > a parent-child relationship if that's not how the system looks like > in HW ? The point I'm making is that I'm not sure all the memory access and stall properties are the same for the parent and the device itself. > > Do you have a specific example in mind that we should be aware of ? > >> So the IORT spec don't support this, at least it's pretty vague >> I think. > > I think that's a matter of wording, it can be updated if it needs be, > reach out if you see any issue with the current approach please. If the all the properties for parent and device itself are the same, I have no strong opinion for this patch, but it's better to update the wording of the spec as well. Thanks Hanjun
On 2020-06-30 14:04, Hanjun Guo wrote: > On 2020/6/30 18:24, Lorenzo Pieralisi wrote: >> On Tue, Jun 30, 2020 at 11:06:41AM +0800, Hanjun Guo wrote: >> >> [...] >> >>>> For devices that aren't described in the DSDT - IORT translations >>>> are determined by their ACPI parent device. Do you see/Have you >>>> found any issue with this approach ? >>> >>> The spec says "Describes the IO relationships between devices >>> represented in the ACPI namespace.", and in section 3.1.1.3 Named >>> component node, it says: >> >> PCI devices aren't necessarily described in the ACPI namespace and we >> still use IORT to describe them - through the RC node. >> >>> "Named component nodes are used to describe devices that are also >>> included in the Differentiated System Description Table (DSDT). See >>> [ACPI]." >>> >>> So from my understanding, the IORT spec for now, can only do ID >>> translations for devices in the DSDT. >> >> I think you can read this multiple ways but this patch does not >> change this concept. What changes, is applying parent's node IORT >> mapping to child nodes with no associated DSDT nodes, it is the >> same thing we do with PCI and the _DMA method - we could update >> the wording in the specs if that clarifies but I don't think this >> deliberately disregards the specifications. > > I agree, but it's better to update the wording of the spec. > >> >>>>> For a platform device, if I use its parent's full path name for >>>>> its named component entry, then it will match, but this will violate >>>>> the IORT spec. >>>> >>>> Can you elaborate on this please I don't get the point you >>>> are making. >>> >>> For example, device A is not described in DSDT so can't represent >>> as a NC node in IORT. Device B can be described in DSDT and it >>> is the parent of device A, so device B can be represented in IORT >>> with memory access properties and node flags with Substream width >>> and Stall supported info. >>> >>> When we trying to translate device A's ID, we reuse all the memory >>> access properties and node flags from its parent (device B), but >>> will it the same? >> >> I assume so why wouldn't it be ? Why would be describe them in >> a parent-child relationship if that's not how the system looks like >> in HW ? > > The point I'm making is that I'm not sure all the memory access and > stall properties are the same for the parent and the device itself. Is that even a valid case though? The principal thing we want to accommodate here is when device B *is* the one accessing memory, either because it is a bridge with device A sat behind it, or because device A is actually just some logical function or subset of physical device B. If the topology is such that device A is a completely independent device with its own path to memory such that it could have different properties, I would expect that it *should* be described in DSDT, and I can't easily think of a good reason why it wouldn't be. I'm also struggling to imagine how it might even have an ID that had to be interpreted in the context of device B if it wasn't one of the cases above :/ I don't doubt that people could - or maybe even have - come up with crap DSDT bindings that don't represent the hardware sufficiently accurately, but I'm not sure that should be IORT's problem... Robin. >> Do you have a specific example in mind that we should be aware of ? >> >>> So the IORT spec don't support this, at least it's pretty vague >>> I think. >> >> I think that's a matter of wording, it can be updated if it needs be, >> reach out if you see any issue with the current approach please. > > If the all the properties for parent and device itself are the same, > I have no strong opinion for this patch, but it's better to update > the wording of the spec as well. > > Thanks > Hanjun >
On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: > From: Makarand Pawagi <makarand.pawagi@nxp.com> > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to > extract memory and other resources. > > Interrupt (GIC ITS) information is extracted from the MADT table > by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > > IORT table is parsed to configure DMA. > > Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > --- > drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- > drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 ++++++++++++++++----- > 3 files changed, 150 insertions(+), 52 deletions(-) > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c > index 824ff77bbe86..324d49d6df89 100644 > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c > @@ -18,6 +18,8 @@ > #include <linux/bitops.h> > #include <linux/msi.h> > #include <linux/dma-mapping.h> > +#include <linux/acpi.h> > +#include <linux/iommu.h> > > #include "fsl-mc-private.h" > > @@ -38,6 +40,7 @@ struct fsl_mc { > struct fsl_mc_device *root_mc_bus_dev; > u8 num_translation_ranges; > struct fsl_mc_addr_translation_range *translation_ranges; > + void *fsl_mc_regs; > }; > > /** > @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { > phys_addr_t start_phys_addr; > }; > > +#define FSL_MC_FAPR 0x28 > +#define MC_FAPR_PL BIT(18) > +#define MC_FAPR_BMT BIT(17) > + > /** > * fsl_mc_bus_match - device to driver matching callback > * @dev: the fsl-mc device to match against > @@ -124,7 +131,10 @@ static int fsl_mc_dma_configure(struct device *dev) > while (dev_is_fsl_mc(dma_dev)) > dma_dev = dma_dev->parent; > > - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > + if (dev_of_node(dma_dev)) > + return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > + > + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); > } > > static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, > @@ -865,8 +875,11 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > struct fsl_mc_io *mc_io = NULL; > int container_id; > phys_addr_t mc_portal_phys_addr; > - u32 mc_portal_size; > - struct resource res; > + u32 mc_portal_size, mc_stream_id; > + struct resource *plat_res; > + > + if (!iommu_present(&fsl_mc_bus_type)) > + return -EPROBE_DEFER; > > mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > if (!mc) > @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, mc); > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > + if (IS_ERR(mc->fsl_mc_regs)) > + return PTR_ERR(mc->fsl_mc_regs); > + > + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > + /* > + * HW ORs the PL and BMT bit, places the result in bit 15 of > + * the StreamID and ORs in the ICID. Calculate it accordingly. > + */ > + mc_stream_id = (mc_stream_id & 0xffff) | > + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > + 0x4000 : 0); > + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, > + &mc_stream_id); > + if (error) > + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > + error); > + } > + > /* > * Get physical address of MC portal for the root DPRC: > */ > - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > - if (error < 0) { > - dev_err(&pdev->dev, > - "of_address_to_resource() failed for %pOF\n", > - pdev->dev.of_node); > - return error; > - } > - > - mc_portal_phys_addr = res.start; > - mc_portal_size = resource_size(&res); > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mc_portal_phys_addr = plat_res->start; > + mc_portal_size = resource_size(plat_res); > error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > mc_portal_size, NULL, > FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io); > @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > mc_version.major, mc_version.minor, mc_version.revision); > > - error = get_mc_addr_translation_ranges(&pdev->dev, > - &mc->translation_ranges, > - &mc->num_translation_ranges); > - if (error < 0) > - goto error_cleanup_mc_io; > + if (dev_of_node(&pdev->dev)) { > + error = get_mc_addr_translation_ranges(&pdev->dev, > + &mc->translation_ranges, > + &mc->num_translation_ranges); > + if (error < 0) > + goto error_cleanup_mc_io; > + } > > error = dprc_get_container_id(mc_io, 0, &container_id); > if (error < 0) { > @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > goto error_cleanup_mc_io; > > mc->root_mc_bus_dev = mc_bus_dev; > + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; Makarand, this looks a bit weird. Is there really a reason for it? --- Best Regards, Laurentiu
Hi Robin, On 2020/7/2 0:12, Robin Murphy wrote: > On 2020-06-30 14:04, Hanjun Guo wrote: >> On 2020/6/30 18:24, Lorenzo Pieralisi wrote: >>> On Tue, Jun 30, 2020 at 11:06:41AM +0800, Hanjun Guo wrote: >>> >>> [...] >>> >>>>> For devices that aren't described in the DSDT - IORT translations >>>>> are determined by their ACPI parent device. Do you see/Have you >>>>> found any issue with this approach ? >>>> >>>> The spec says "Describes the IO relationships between devices >>>> represented in the ACPI namespace.", and in section 3.1.1.3 Named >>>> component node, it says: >>> >>> PCI devices aren't necessarily described in the ACPI namespace and we >>> still use IORT to describe them - through the RC node. >>> >>>> "Named component nodes are used to describe devices that are also >>>> included in the Differentiated System Description Table (DSDT). See >>>> [ACPI]." >>>> >>>> So from my understanding, the IORT spec for now, can only do ID >>>> translations for devices in the DSDT. >>> >>> I think you can read this multiple ways but this patch does not >>> change this concept. What changes, is applying parent's node IORT >>> mapping to child nodes with no associated DSDT nodes, it is the >>> same thing we do with PCI and the _DMA method - we could update >>> the wording in the specs if that clarifies but I don't think this >>> deliberately disregards the specifications. >> >> I agree, but it's better to update the wording of the spec. >> >>> >>>>>> For a platform device, if I use its parent's full path name for >>>>>> its named component entry, then it will match, but this will violate >>>>>> the IORT spec. >>>>> >>>>> Can you elaborate on this please I don't get the point you >>>>> are making. >>>> >>>> For example, device A is not described in DSDT so can't represent >>>> as a NC node in IORT. Device B can be described in DSDT and it >>>> is the parent of device A, so device B can be represented in IORT >>>> with memory access properties and node flags with Substream width >>>> and Stall supported info. >>>> >>>> When we trying to translate device A's ID, we reuse all the memory >>>> access properties and node flags from its parent (device B), but >>>> will it the same? >>> >>> I assume so why wouldn't it be ? Why would be describe them in >>> a parent-child relationship if that's not how the system looks like >>> in HW ? >> >> The point I'm making is that I'm not sure all the memory access and >> stall properties are the same for the parent and the device itself. > > Is that even a valid case though? The principal thing we want to > accommodate here is when device B *is* the one accessing memory, either > because it is a bridge with device A sat behind it, or because device A > is actually just some logical function or subset of physical device B. Thanks for the clarify, for CCA attributes, child device should be the same as its parent and that was written in the ACPI spec, so it's better to make it clear for other properties in the spec as well. > > If the topology is such that device A is a completely independent device > with its own path to memory such that it could have different > properties, I would expect that it *should* be described in DSDT, and I > can't easily think of a good reason why it wouldn't be. I'm also > struggling to imagine how it might even have an ID that had to be > interpreted in the context of device B if it wasn't one of the cases > above :/ > > I don't doubt that people could - or maybe even have - come up with crap > DSDT bindings that don't represent the hardware sufficiently accurately, > but I'm not sure that should be IORT's problem... As I said in previous email, I'm not against this patch, and seems have no regressions for platforms that using named component node such as D05/D06 (I will test it shortly to make sure), but it's better to update the wording of the spec (even after this patch set is merged). Thanks Hanjun
On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote: > > > On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: > > From: Makarand Pawagi <makarand.pawagi@nxp.com> > > > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to > > extract memory and other resources. > > > > Interrupt (GIC ITS) information is extracted from the MADT table > > by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > > > > IORT table is parsed to configure DMA. > > > > Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > --- > > drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- > > drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 ++++++++++++++++----- > > 3 files changed, 150 insertions(+), 52 deletions(-) > > > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c > > index 824ff77bbe86..324d49d6df89 100644 > > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c > > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c > > @@ -18,6 +18,8 @@ > > #include <linux/bitops.h> > > #include <linux/msi.h> > > #include <linux/dma-mapping.h> > > +#include <linux/acpi.h> > > +#include <linux/iommu.h> > > > > #include "fsl-mc-private.h" > > > > @@ -38,6 +40,7 @@ struct fsl_mc { > > struct fsl_mc_device *root_mc_bus_dev; > > u8 num_translation_ranges; > > struct fsl_mc_addr_translation_range *translation_ranges; > > + void *fsl_mc_regs; > > }; > > > > /** > > @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { > > phys_addr_t start_phys_addr; > > }; > > > > +#define FSL_MC_FAPR 0x28 > > +#define MC_FAPR_PL BIT(18) > > +#define MC_FAPR_BMT BIT(17) > > + > > /** > > * fsl_mc_bus_match - device to driver matching callback > > * @dev: the fsl-mc device to match against > > @@ -124,7 +131,10 @@ static int fsl_mc_dma_configure(struct device *dev) > > while (dev_is_fsl_mc(dma_dev)) > > dma_dev = dma_dev->parent; > > > > - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > > + if (dev_of_node(dma_dev)) > > + return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > > + > > + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); > > } > > > > static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, > > @@ -865,8 +875,11 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > > struct fsl_mc_io *mc_io = NULL; > > int container_id; > > phys_addr_t mc_portal_phys_addr; > > - u32 mc_portal_size; > > - struct resource res; > > + u32 mc_portal_size, mc_stream_id; > > + struct resource *plat_res; > > + > > + if (!iommu_present(&fsl_mc_bus_type)) > > + return -EPROBE_DEFER; > > > > mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > > if (!mc) > > @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, mc); > > > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > > + if (IS_ERR(mc->fsl_mc_regs)) > > + return PTR_ERR(mc->fsl_mc_regs); > > + > > + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > > + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > > + /* > > + * HW ORs the PL and BMT bit, places the result in bit 15 of > > + * the StreamID and ORs in the ICID. Calculate it accordingly. > > + */ > > + mc_stream_id = (mc_stream_id & 0xffff) | > > + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > > + 0x4000 : 0); > > + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, > > + &mc_stream_id); > > + if (error) > > + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > > + error); > > + } > > + > > /* > > * Get physical address of MC portal for the root DPRC: > > */ > > - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > > - if (error < 0) { > > - dev_err(&pdev->dev, > > - "of_address_to_resource() failed for %pOF\n", > > - pdev->dev.of_node); > > - return error; > > - } > > - > > - mc_portal_phys_addr = res.start; > > - mc_portal_size = resource_size(&res); > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + mc_portal_phys_addr = plat_res->start; > > + mc_portal_size = resource_size(plat_res); > > error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > > mc_portal_size, NULL, > > FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io); > > @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > > dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > > mc_version.major, mc_version.minor, mc_version.revision); > > > > - error = get_mc_addr_translation_ranges(&pdev->dev, > > - &mc->translation_ranges, > > - &mc->num_translation_ranges); > > - if (error < 0) > > - goto error_cleanup_mc_io; > > + if (dev_of_node(&pdev->dev)) { > > + error = get_mc_addr_translation_ranges(&pdev->dev, > > + &mc->translation_ranges, > > + &mc->num_translation_ranges); > > + if (error < 0) > > + goto error_cleanup_mc_io; > > + } > > > > error = dprc_get_container_id(mc_io, 0, &container_id); > > if (error < 0) { > > @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device *pdev) > > goto error_cleanup_mc_io; > > > > mc->root_mc_bus_dev = mc_bus_dev; > > + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > > Makarand, this looks a bit weird. Is there really a reason for it? Can you clarify please so that we can reach a conclusion on this matter ? Thanks, Lorenzo
On Thu, Jul 02, 2020 at 04:22:00PM +0800, Hanjun Guo wrote: > Hi Robin, > > On 2020/7/2 0:12, Robin Murphy wrote: > > On 2020-06-30 14:04, Hanjun Guo wrote: > > > On 2020/6/30 18:24, Lorenzo Pieralisi wrote: > > > > On Tue, Jun 30, 2020 at 11:06:41AM +0800, Hanjun Guo wrote: > > > > > > > > [...] > > > > > > > > > > For devices that aren't described in the DSDT - IORT translations > > > > > > are determined by their ACPI parent device. Do you see/Have you > > > > > > found any issue with this approach ? > > > > > > > > > > The spec says "Describes the IO relationships between devices > > > > > represented in the ACPI namespace.", and in section 3.1.1.3 Named > > > > > component node, it says: > > > > > > > > PCI devices aren't necessarily described in the ACPI namespace and we > > > > still use IORT to describe them - through the RC node. > > > > > > > > > "Named component nodes are used to describe devices that are also > > > > > included in the Differentiated System Description Table (DSDT). See > > > > > [ACPI]." > > > > > > > > > > So from my understanding, the IORT spec for now, can only do ID > > > > > translations for devices in the DSDT. > > > > > > > > I think you can read this multiple ways but this patch does not > > > > change this concept. What changes, is applying parent's node IORT > > > > mapping to child nodes with no associated DSDT nodes, it is the > > > > same thing we do with PCI and the _DMA method - we could update > > > > the wording in the specs if that clarifies but I don't think this > > > > deliberately disregards the specifications. > > > > > > I agree, but it's better to update the wording of the spec. > > > > > > > > > > > > > > For a platform device, if I use its parent's full path name for > > > > > > > its named component entry, then it will match, but this will violate > > > > > > > the IORT spec. > > > > > > > > > > > > Can you elaborate on this please I don't get the point you > > > > > > are making. > > > > > > > > > > For example, device A is not described in DSDT so can't represent > > > > > as a NC node in IORT. Device B can be described in DSDT and it > > > > > is the parent of device A, so device B can be represented in IORT > > > > > with memory access properties and node flags with Substream width > > > > > and Stall supported info. > > > > > > > > > > When we trying to translate device A's ID, we reuse all the memory > > > > > access properties and node flags from its parent (device B), but > > > > > will it the same? > > > > > > > > I assume so why wouldn't it be ? Why would be describe them in > > > > a parent-child relationship if that's not how the system looks like > > > > in HW ? > > > > > > The point I'm making is that I'm not sure all the memory access and > > > stall properties are the same for the parent and the device itself. > > > > Is that even a valid case though? The principal thing we want to > > accommodate here is when device B *is* the one accessing memory, either > > because it is a bridge with device A sat behind it, or because device A > > is actually just some logical function or subset of physical device B. > > Thanks for the clarify, for CCA attributes, child device should be the > same as its parent and that was written in the ACPI spec, so it's better > to make it clear for other properties in the spec as well. > > > > > If the topology is such that device A is a completely independent device > > with its own path to memory such that it could have different > > properties, I would expect that it *should* be described in DSDT, and I > > can't easily think of a good reason why it wouldn't be. I'm also > > struggling to imagine how it might even have an ID that had to be > > interpreted in the context of device B if it wasn't one of the cases > > above :/ > > > > I don't doubt that people could - or maybe even have - come up with crap > > DSDT bindings that don't represent the hardware sufficiently accurately, > > but I'm not sure that should be IORT's problem... > > As I said in previous email, I'm not against this patch, and seems > have no regressions for platforms that using named component node > such as D05/D06 (I will test it shortly to make sure), but it's better > to update the wording of the spec (even after this patch set is merged). Have you managed to test this series ? Thanks, Lorenzo
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Thursday, July 9, 2020 2:50 PM > To: Laurentiu Tudor <laurentiu.tudor@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; Makarand Pawagi > <makarand.pawagi@nxp.com>; Diana Madalina Craciun (OSS) > <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- > acpi@vger.kernel.org; devicetree@vger.kernel.org; linux-pci@vger.kernel.org; > Rob Herring <robh+dt@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; > Joerg Roedel <joro@8bytes.org>; Hanjun Guo <guohanjun@huawei.com>; Bjorn > Helgaas <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; > Robin Murphy <robin.murphy@arm.com>; Catalin Marinas > <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc Zyngier > <maz@kernel.org> > Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc > > Caution: EXT Email > > On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote: > > > > > > On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: > > > From: Makarand Pawagi <makarand.pawagi@nxp.com> > > > > > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table > > > to extract memory and other resources. > > > > > > Interrupt (GIC ITS) information is extracted from the MADT table by > > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > > > > > > IORT table is parsed to configure DMA. > > > > > > Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > > > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > > > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > > --- > > > drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- > > > drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- > > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 > > > ++++++++++++++++----- > > > 3 files changed, 150 insertions(+), 52 deletions(-) > > > > > > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c > > > b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89 > > > 100644 > > > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c > > > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c > > > @@ -18,6 +18,8 @@ > > > #include <linux/bitops.h> > > > #include <linux/msi.h> > > > #include <linux/dma-mapping.h> > > > +#include <linux/acpi.h> > > > +#include <linux/iommu.h> > > > > > > #include "fsl-mc-private.h" > > > > > > @@ -38,6 +40,7 @@ struct fsl_mc { > > > struct fsl_mc_device *root_mc_bus_dev; > > > u8 num_translation_ranges; > > > struct fsl_mc_addr_translation_range *translation_ranges; > > > + void *fsl_mc_regs; > > > }; > > > > > > /** > > > @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { > > > phys_addr_t start_phys_addr; > > > }; > > > > > > +#define FSL_MC_FAPR 0x28 > > > +#define MC_FAPR_PL BIT(18) > > > +#define MC_FAPR_BMT BIT(17) > > > + > > > /** > > > * fsl_mc_bus_match - device to driver matching callback > > > * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@ > > > static int fsl_mc_dma_configure(struct device *dev) > > > while (dev_is_fsl_mc(dma_dev)) > > > dma_dev = dma_dev->parent; > > > > > > - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > > > + if (dev_of_node(dma_dev)) > > > + return of_dma_configure_id(dev, dma_dev->of_node, 0, > > > + &input_id); > > > + > > > + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); > > > } > > > > > > static ssize_t modalias_show(struct device *dev, struct > > > device_attribute *attr, @@ -865,8 +875,11 @@ static int > fsl_mc_bus_probe(struct platform_device *pdev) > > > struct fsl_mc_io *mc_io = NULL; > > > int container_id; > > > phys_addr_t mc_portal_phys_addr; > > > - u32 mc_portal_size; > > > - struct resource res; > > > + u32 mc_portal_size, mc_stream_id; > > > + struct resource *plat_res; > > > + > > > + if (!iommu_present(&fsl_mc_bus_type)) > > > + return -EPROBE_DEFER; > > > > > > mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > > > if (!mc) > > > @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct > > > platform_device *pdev) > > > > > > platform_set_drvdata(pdev, mc); > > > > > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > > + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > > > + if (IS_ERR(mc->fsl_mc_regs)) > > > + return PTR_ERR(mc->fsl_mc_regs); > > > + > > > + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > > > + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > > > + /* > > > + * HW ORs the PL and BMT bit, places the result in bit 15 of > > > + * the StreamID and ORs in the ICID. Calculate it accordingly. > > > + */ > > > + mc_stream_id = (mc_stream_id & 0xffff) | > > > + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > > > + 0x4000 : 0); > > > + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, > > > + &mc_stream_id); > > > + if (error) > > > + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > > > + error); > > > + } > > > + > > > /* > > > * Get physical address of MC portal for the root DPRC: > > > */ > > > - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > > > - if (error < 0) { > > > - dev_err(&pdev->dev, > > > - "of_address_to_resource() failed for %pOF\n", > > > - pdev->dev.of_node); > > > - return error; > > > - } > > > - > > > - mc_portal_phys_addr = res.start; > > > - mc_portal_size = resource_size(&res); > > > + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + mc_portal_phys_addr = plat_res->start; > > > + mc_portal_size = resource_size(plat_res); > > > error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > > > mc_portal_size, NULL, > > > FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, > > > &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct > platform_device *pdev) > > > dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > > > mc_version.major, mc_version.minor, > > > mc_version.revision); > > > > > > - error = get_mc_addr_translation_ranges(&pdev->dev, > > > - &mc->translation_ranges, > > > - &mc->num_translation_ranges); > > > - if (error < 0) > > > - goto error_cleanup_mc_io; > > > + if (dev_of_node(&pdev->dev)) { > > > + error = get_mc_addr_translation_ranges(&pdev->dev, > > > + &mc->translation_ranges, > > > + &mc->num_translation_ranges); > > > + if (error < 0) > > > + goto error_cleanup_mc_io; > > > + } > > > > > > error = dprc_get_container_id(mc_io, 0, &container_id); > > > if (error < 0) { > > > @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device > *pdev) > > > goto error_cleanup_mc_io; > > > > > > mc->root_mc_bus_dev = mc_bus_dev; > > > + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > > > > Makarand, this looks a bit weird. Is there really a reason for it? > > Can you clarify please so that we can reach a conclusion on this matter ? > Laurentiu, can you clarify what exactly is the doubt here? Are you asking about fwnode assignment from pdev to mc_bus_dev? > Thanks, > Lorenzo
On Fri, Jun 19, 2020 at 09:20:06AM +0100, Lorenzo Pieralisi wrote: > Some HW devices are created as child devices of proprietary busses, > that have a bus specific policy defining how the child devices > wires representing the devices ID are translated into IOMMU and > IRQ controllers device IDs. > > Current IORT code provides translations for: > > - PCI devices, where the device ID is well identified at bus level > as the requester ID (RID) > - Platform devices that are endpoint devices where the device ID is > retrieved from the ACPI object IORT mappings (Named components single > mappings). A platform device is represented in IORT as a named > component node > > For devices that are child devices of proprietary busses the IORT > firmware represents the bus node as a named component node in IORT > and it is up to that named component node to define in/out bus > specific ID translations for the bus child devices that are > allocated and created in a bus specific manner. > > In order to make IORT ID translations available for proprietary > bus child devices, the current ACPI (and IORT) code must be > augmented to provide an additional ID parameter to acpi_dma_configure() > representing the child devices input ID. This ID is bus specific > and it is retrieved in bus specific code. > > By adding an ID parameter to acpi_dma_configure(), the IORT > code can map the child device ID to an IOMMU stream ID through > the IORT named component representing the bus in/out ID mappings. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Hanjun Guo <guohanjun@huawei.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > --- > drivers/acpi/arm64/iort.c | 59 +++++++++++++++++++++++++++++---------- > drivers/acpi/scan.c | 8 ++++-- > include/acpi/acpi_bus.h | 9 ++++-- > include/linux/acpi.h | 7 +++++ > include/linux/acpi_iort.h | 7 +++-- > 5 files changed, 67 insertions(+), 23 deletions(-) Hi Rafael, just to ask if the ACPI core changes in this patch are OK with you, thank you very much. Lorenzo > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 421c6976ab81..ec782e4a0fe4 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -978,19 +978,54 @@ static void iort_named_component_init(struct device *dev, > nc->node_flags); > } > > +static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node) > +{ > + struct acpi_iort_node *parent; > + int err = -ENODEV, i = 0; > + u32 streamid = 0; > + > + do { > + > + parent = iort_node_map_platform_id(node, &streamid, > + IORT_IOMMU_TYPE, > + i++); > + > + if (parent) > + err = iort_iommu_xlate(dev, parent, streamid); > + } while (parent && !err); > + > + return err; > +} > + > +static int iort_nc_iommu_map_id(struct device *dev, > + struct acpi_iort_node *node, > + const u32 *in_id) > +{ > + struct acpi_iort_node *parent; > + u32 streamid; > + > + parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE); > + if (parent) > + return iort_iommu_xlate(dev, parent, streamid); > + > + return -ENODEV; > +} > + > + > /** > - * iort_iommu_configure - Set-up IOMMU configuration for a device. > + * iort_iommu_configure_id - Set-up IOMMU configuration for a device. > * > * @dev: device to configure > + * @id_in: optional input id const value pointer > * > * Returns: iommu_ops pointer on configuration success > * NULL on configuration failure > */ > -const struct iommu_ops *iort_iommu_configure(struct device *dev) > +const struct iommu_ops *iort_iommu_configure_id(struct device *dev, > + const u32 *id_in) > { > - struct acpi_iort_node *node, *parent; > + struct acpi_iort_node *node; > const struct iommu_ops *ops; > - u32 streamid = 0; > int err = -ENODEV; > > /* > @@ -1019,21 +1054,13 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev) > if (fwspec && iort_pci_rc_supports_ats(node)) > fwspec->flags |= IOMMU_FWSPEC_PCI_RC_ATS; > } else { > - int i = 0; > - > node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT, > iort_match_node_callback, dev); > if (!node) > return NULL; > > - do { > - parent = iort_node_map_platform_id(node, &streamid, > - IORT_IOMMU_TYPE, > - i++); > - > - if (parent) > - err = iort_iommu_xlate(dev, parent, streamid); > - } while (parent && !err); > + err = id_in ? iort_nc_iommu_map_id(dev, node, id_in) : > + iort_nc_iommu_map(dev, node); > > if (!err) > iort_named_component_init(dev, node); > @@ -1058,6 +1085,7 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev) > > return ops; > } > + > #else > static inline const struct iommu_ops *iort_fwspec_iommu_ops(struct device *dev) > { return NULL; } > @@ -1066,7 +1094,8 @@ static inline int iort_add_device_replay(const struct iommu_ops *ops, > { return 0; } > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head) > { return 0; } > -const struct iommu_ops *iort_iommu_configure(struct device *dev) > +const struct iommu_ops *iort_iommu_configure_id(struct device *dev, > + const u32 *input_id) > { return NULL; } > #endif > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 8777faced51a..2142f1554761 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -1457,8 +1457,10 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, > * acpi_dma_configure - Set-up DMA configuration for the device. > * @dev: The pointer to the device > * @attr: device dma attributes > + * @input_id: input device id const value pointer > */ > -int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) > +int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr, > + const u32 *input_id) > { > const struct iommu_ops *iommu; > u64 dma_addr = 0, size = 0; > @@ -1470,7 +1472,7 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) > > iort_dma_setup(dev, &dma_addr, &size); > > - iommu = iort_iommu_configure(dev); > + iommu = iort_iommu_configure_id(dev, input_id); > if (PTR_ERR(iommu) == -EPROBE_DEFER) > return -EPROBE_DEFER; > > @@ -1479,7 +1481,7 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) > > return 0; > } > -EXPORT_SYMBOL_GPL(acpi_dma_configure); > +EXPORT_SYMBOL_GPL(acpi_dma_configure_id); > > static void acpi_init_coherency(struct acpi_device *adev) > { > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 5afb6ceb284f..a3abcc4b7d9f 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -588,8 +588,13 @@ bool acpi_dma_supported(struct acpi_device *adev); > enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev); > int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, > u64 *size); > -int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr); > - > +int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr, > + const u32 *input_id); > +static inline int acpi_dma_configure(struct device *dev, > + enum dev_dma_attr attr) > +{ > + return acpi_dma_configure_id(dev, attr, NULL); > +} > struct acpi_device *acpi_find_child_device(struct acpi_device *parent, > u64 address, bool check_children); > int acpi_is_root_bridge(acpi_handle); > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index d661cd0ee64d..6d2c47489d90 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -905,6 +905,13 @@ static inline int acpi_dma_configure(struct device *dev, > return 0; > } > > +static inline int acpi_dma_configure_id(struct device *dev, > + enum dev_dma_attr attr, > + const u32 *input_id) > +{ > + return 0; > +} > + > #define ACPI_PTR(_ptr) (NULL) > > static inline void acpi_device_set_enumerated(struct acpi_device *adev) > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h > index e51425e083da..20a32120bb88 100644 > --- a/include/linux/acpi_iort.h > +++ b/include/linux/acpi_iort.h > @@ -35,7 +35,8 @@ void acpi_configure_pmsi_domain(struct device *dev); > int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id); > /* IOMMU interface */ > void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *size); > -const struct iommu_ops *iort_iommu_configure(struct device *dev); > +const struct iommu_ops *iort_iommu_configure_id(struct device *dev, > + const u32 *id_in); > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head); > #else > static inline void acpi_iort_init(void) { } > @@ -48,8 +49,8 @@ static inline void acpi_configure_pmsi_domain(struct device *dev) { } > /* IOMMU interface */ > static inline void iort_dma_setup(struct device *dev, u64 *dma_addr, > u64 *size) { } > -static inline const struct iommu_ops *iort_iommu_configure( > - struct device *dev) > +static inline const struct iommu_ops *iort_iommu_configure_id( > + struct device *dev, const u32 *id_in) > { return NULL; } > static inline > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head) > -- > 2.26.1 >
On 7/9/2020 12:26 PM, Makarand Pawagi wrote: > > >> -----Original Message----- >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Sent: Thursday, July 9, 2020 2:50 PM >> To: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> Cc: linux-arm-kernel@lists.infradead.org; Makarand Pawagi >> <makarand.pawagi@nxp.com>; Diana Madalina Craciun (OSS) >> <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- >> acpi@vger.kernel.org; devicetree@vger.kernel.org; linux-pci@vger.kernel.org; >> Rob Herring <robh+dt@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; >> Joerg Roedel <joro@8bytes.org>; Hanjun Guo <guohanjun@huawei.com>; Bjorn >> Helgaas <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; >> Robin Murphy <robin.murphy@arm.com>; Catalin Marinas >> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc Zyngier >> <maz@kernel.org> >> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc >> >> Caution: EXT Email >> >> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote: >>> >>> >>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: >>>> From: Makarand Pawagi <makarand.pawagi@nxp.com> >>>> >>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table >>>> to extract memory and other resources. >>>> >>>> Interrupt (GIC ITS) information is extracted from the MADT table by >>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. >>>> >>>> IORT table is parsed to configure DMA. >>>> >>>> Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> >>>> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> >>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>>> --- >>>> drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- >>>> drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- >>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 >>>> ++++++++++++++++----- >>>> 3 files changed, 150 insertions(+), 52 deletions(-) >>>> >>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c >>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89 >>>> 100644 >>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c >>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c >>>> @@ -18,6 +18,8 @@ >>>> #include <linux/bitops.h> >>>> #include <linux/msi.h> >>>> #include <linux/dma-mapping.h> >>>> +#include <linux/acpi.h> >>>> +#include <linux/iommu.h> >>>> >>>> #include "fsl-mc-private.h" >>>> >>>> @@ -38,6 +40,7 @@ struct fsl_mc { >>>> struct fsl_mc_device *root_mc_bus_dev; >>>> u8 num_translation_ranges; >>>> struct fsl_mc_addr_translation_range *translation_ranges; >>>> + void *fsl_mc_regs; >>>> }; >>>> >>>> /** >>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { >>>> phys_addr_t start_phys_addr; >>>> }; >>>> >>>> +#define FSL_MC_FAPR 0x28 >>>> +#define MC_FAPR_PL BIT(18) >>>> +#define MC_FAPR_BMT BIT(17) >>>> + >>>> /** >>>> * fsl_mc_bus_match - device to driver matching callback >>>> * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@ >>>> static int fsl_mc_dma_configure(struct device *dev) >>>> while (dev_is_fsl_mc(dma_dev)) >>>> dma_dev = dma_dev->parent; >>>> >>>> - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); >>>> + if (dev_of_node(dma_dev)) >>>> + return of_dma_configure_id(dev, dma_dev->of_node, 0, >>>> + &input_id); >>>> + >>>> + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); >>>> } >>>> >>>> static ssize_t modalias_show(struct device *dev, struct >>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int >> fsl_mc_bus_probe(struct platform_device *pdev) >>>> struct fsl_mc_io *mc_io = NULL; >>>> int container_id; >>>> phys_addr_t mc_portal_phys_addr; >>>> - u32 mc_portal_size; >>>> - struct resource res; >>>> + u32 mc_portal_size, mc_stream_id; >>>> + struct resource *plat_res; >>>> + >>>> + if (!iommu_present(&fsl_mc_bus_type)) >>>> + return -EPROBE_DEFER; >>>> >>>> mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); >>>> if (!mc) >>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct >>>> platform_device *pdev) >>>> >>>> platform_set_drvdata(pdev, mc); >>>> >>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >>>> + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); >>>> + if (IS_ERR(mc->fsl_mc_regs)) >>>> + return PTR_ERR(mc->fsl_mc_regs); >>>> + >>>> + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { >>>> + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); >>>> + /* >>>> + * HW ORs the PL and BMT bit, places the result in bit 15 of >>>> + * the StreamID and ORs in the ICID. Calculate it accordingly. >>>> + */ >>>> + mc_stream_id = (mc_stream_id & 0xffff) | >>>> + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? >>>> + 0x4000 : 0); >>>> + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, >>>> + &mc_stream_id); >>>> + if (error) >>>> + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", >>>> + error); >>>> + } >>>> + >>>> /* >>>> * Get physical address of MC portal for the root DPRC: >>>> */ >>>> - error = of_address_to_resource(pdev->dev.of_node, 0, &res); >>>> - if (error < 0) { >>>> - dev_err(&pdev->dev, >>>> - "of_address_to_resource() failed for %pOF\n", >>>> - pdev->dev.of_node); >>>> - return error; >>>> - } >>>> - >>>> - mc_portal_phys_addr = res.start; >>>> - mc_portal_size = resource_size(&res); >>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + mc_portal_phys_addr = plat_res->start; >>>> + mc_portal_size = resource_size(plat_res); >>>> error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, >>>> mc_portal_size, NULL, >>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, >>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct >> platform_device *pdev) >>>> dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", >>>> mc_version.major, mc_version.minor, >>>> mc_version.revision); >>>> >>>> - error = get_mc_addr_translation_ranges(&pdev->dev, >>>> - &mc->translation_ranges, >>>> - &mc->num_translation_ranges); >>>> - if (error < 0) >>>> - goto error_cleanup_mc_io; >>>> + if (dev_of_node(&pdev->dev)) { >>>> + error = get_mc_addr_translation_ranges(&pdev->dev, >>>> + &mc->translation_ranges, >>>> + &mc->num_translation_ranges); >>>> + if (error < 0) >>>> + goto error_cleanup_mc_io; >>>> + } >>>> >>>> error = dprc_get_container_id(mc_io, 0, &container_id); >>>> if (error < 0) { >>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct platform_device >> *pdev) >>>> goto error_cleanup_mc_io; >>>> >>>> mc->root_mc_bus_dev = mc_bus_dev; >>>> + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; >>> >>> Makarand, this looks a bit weird. Is there really a reason for it? >> >> Can you clarify please so that we can reach a conclusion on this matter ? >> > Laurentiu, can you clarify what exactly is the doubt here? Are you asking about fwnode assignment from pdev to mc_bus_dev? > Yes. I remember that a while ago I tested without this fwnode assignment and didn't encounter any issues. Maybe we can just drop it? --- Best Regards, Laurentiu
> -----Original Message----- > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > Sent: Thursday, July 9, 2020 3:45 PM > To: Makarand Pawagi <makarand.pawagi@nxp.com>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> > Cc: linux-arm-kernel@lists.infradead.org; Diana Madalina Craciun (OSS) > <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- > acpi@vger.kernel.org; devicetree@vger.kernel.org; linux-pci@vger.kernel.org; > Rob Herring <robh+dt@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; > Joerg Roedel <joro@8bytes.org>; Hanjun Guo <guohanjun@huawei.com>; Bjorn > Helgaas <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; > Robin Murphy <robin.murphy@arm.com>; Catalin Marinas > <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc Zyngier > <maz@kernel.org> > Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc > > > > On 7/9/2020 12:26 PM, Makarand Pawagi wrote: > > > > > >> -----Original Message----- > >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Sent: Thursday, July 9, 2020 2:50 PM > >> To: Laurentiu Tudor <laurentiu.tudor@nxp.com> > >> Cc: linux-arm-kernel@lists.infradead.org; Makarand Pawagi > >> <makarand.pawagi@nxp.com>; Diana Madalina Craciun (OSS) > >> <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- > >> acpi@vger.kernel.org; devicetree@vger.kernel.org; > >> linux-pci@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Rafael > >> J. Wysocki <rjw@rjwysocki.net>; Joerg Roedel <joro@8bytes.org>; > >> Hanjun Guo <guohanjun@huawei.com>; Bjorn Helgaas > >> <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; Robin > >> Murphy <robin.murphy@arm.com>; Catalin Marinas > >> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc > >> Zyngier <maz@kernel.org> > >> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for > >> fsl-mc > >> > >> Caution: EXT Email > >> > >> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote: > >>> > >>> > >>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: > >>>> From: Makarand Pawagi <makarand.pawagi@nxp.com> > >>>> > >>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table > >>>> to extract memory and other resources. > >>>> > >>>> Interrupt (GIC ITS) information is extracted from the MADT table by > >>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > >>>> > >>>> IORT table is parsed to configure DMA. > >>>> > >>>> Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > >>>> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > >>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > >>>> --- > >>>> drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- > >>>> drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- > >>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 > >>>> ++++++++++++++++----- > >>>> 3 files changed, 150 insertions(+), 52 deletions(-) > >>>> > >>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c > >>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89 > >>>> 100644 > >>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c > >>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c > >>>> @@ -18,6 +18,8 @@ > >>>> #include <linux/bitops.h> > >>>> #include <linux/msi.h> > >>>> #include <linux/dma-mapping.h> > >>>> +#include <linux/acpi.h> > >>>> +#include <linux/iommu.h> > >>>> > >>>> #include "fsl-mc-private.h" > >>>> > >>>> @@ -38,6 +40,7 @@ struct fsl_mc { > >>>> struct fsl_mc_device *root_mc_bus_dev; > >>>> u8 num_translation_ranges; > >>>> struct fsl_mc_addr_translation_range *translation_ranges; > >>>> + void *fsl_mc_regs; > >>>> }; > >>>> > >>>> /** > >>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { > >>>> phys_addr_t start_phys_addr; > >>>> }; > >>>> > >>>> +#define FSL_MC_FAPR 0x28 > >>>> +#define MC_FAPR_PL BIT(18) > >>>> +#define MC_FAPR_BMT BIT(17) > >>>> + > >>>> /** > >>>> * fsl_mc_bus_match - device to driver matching callback > >>>> * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@ > >>>> static int fsl_mc_dma_configure(struct device *dev) > >>>> while (dev_is_fsl_mc(dma_dev)) > >>>> dma_dev = dma_dev->parent; > >>>> > >>>> - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > >>>> + if (dev_of_node(dma_dev)) > >>>> + return of_dma_configure_id(dev, dma_dev->of_node, 0, > >>>> + &input_id); > >>>> + > >>>> + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); > >>>> } > >>>> > >>>> static ssize_t modalias_show(struct device *dev, struct > >>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int > >> fsl_mc_bus_probe(struct platform_device *pdev) > >>>> struct fsl_mc_io *mc_io = NULL; > >>>> int container_id; > >>>> phys_addr_t mc_portal_phys_addr; > >>>> - u32 mc_portal_size; > >>>> - struct resource res; > >>>> + u32 mc_portal_size, mc_stream_id; > >>>> + struct resource *plat_res; > >>>> + > >>>> + if (!iommu_present(&fsl_mc_bus_type)) > >>>> + return -EPROBE_DEFER; > >>>> > >>>> mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > >>>> if (!mc) > >>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct > >>>> platform_device *pdev) > >>>> > >>>> platform_set_drvdata(pdev, mc); > >>>> > >>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > >>>> + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > >>>> + if (IS_ERR(mc->fsl_mc_regs)) > >>>> + return PTR_ERR(mc->fsl_mc_regs); > >>>> + > >>>> + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > >>>> + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > >>>> + /* > >>>> + * HW ORs the PL and BMT bit, places the result in bit 15 of > >>>> + * the StreamID and ORs in the ICID. Calculate it accordingly. > >>>> + */ > >>>> + mc_stream_id = (mc_stream_id & 0xffff) | > >>>> + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > >>>> + 0x4000 : 0); > >>>> + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, > >>>> + &mc_stream_id); > >>>> + if (error) > >>>> + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > >>>> + error); > >>>> + } > >>>> + > >>>> /* > >>>> * Get physical address of MC portal for the root DPRC: > >>>> */ > >>>> - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > >>>> - if (error < 0) { > >>>> - dev_err(&pdev->dev, > >>>> - "of_address_to_resource() failed for %pOF\n", > >>>> - pdev->dev.of_node); > >>>> - return error; > >>>> - } > >>>> - > >>>> - mc_portal_phys_addr = res.start; > >>>> - mc_portal_size = resource_size(&res); > >>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>> + mc_portal_phys_addr = plat_res->start; > >>>> + mc_portal_size = resource_size(plat_res); > >>>> error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > >>>> mc_portal_size, NULL, > >>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, > >>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct > >> platform_device *pdev) > >>>> dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > >>>> mc_version.major, mc_version.minor, > >>>> mc_version.revision); > >>>> > >>>> - error = get_mc_addr_translation_ranges(&pdev->dev, > >>>> - &mc->translation_ranges, > >>>> - &mc->num_translation_ranges); > >>>> - if (error < 0) > >>>> - goto error_cleanup_mc_io; > >>>> + if (dev_of_node(&pdev->dev)) { > >>>> + error = get_mc_addr_translation_ranges(&pdev->dev, > >>>> + &mc->translation_ranges, > >>>> + &mc->num_translation_ranges); > >>>> + if (error < 0) > >>>> + goto error_cleanup_mc_io; > >>>> + } > >>>> > >>>> error = dprc_get_container_id(mc_io, 0, &container_id); > >>>> if (error < 0) { > >>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct > >>>> platform_device > >> *pdev) > >>>> goto error_cleanup_mc_io; > >>>> > >>>> mc->root_mc_bus_dev = mc_bus_dev; > >>>> + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > >>> > >>> Makarand, this looks a bit weird. Is there really a reason for it? > >> > >> Can you clarify please so that we can reach a conclusion on this matter ? > >> > > Laurentiu, can you clarify what exactly is the doubt here? Are you asking about > fwnode assignment from pdev to mc_bus_dev? > > > > Yes. I remember that a while ago I tested without this fwnode assignment and > didn't encounter any issues. Maybe we can just drop it? Did you tested with PHY changes? Because this is needed for MAC driver, where it needs the mc bus node. > > --- > Best Regards, Laurentiu
On 7/9/2020 1:37 PM, Makarand Pawagi wrote: > > >> -----Original Message----- >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> Sent: Thursday, July 9, 2020 3:45 PM >> To: Makarand Pawagi <makarand.pawagi@nxp.com>; Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org; Diana Madalina Craciun (OSS) >> <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- >> acpi@vger.kernel.org; devicetree@vger.kernel.org; linux-pci@vger.kernel.org; >> Rob Herring <robh+dt@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; >> Joerg Roedel <joro@8bytes.org>; Hanjun Guo <guohanjun@huawei.com>; Bjorn >> Helgaas <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; >> Robin Murphy <robin.murphy@arm.com>; Catalin Marinas >> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc Zyngier >> <maz@kernel.org> >> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc >> >> >> >> On 7/9/2020 12:26 PM, Makarand Pawagi wrote: >>> >>> >>>> -----Original Message----- >>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>> Sent: Thursday, July 9, 2020 2:50 PM >>>> To: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>>> Cc: linux-arm-kernel@lists.infradead.org; Makarand Pawagi >>>> <makarand.pawagi@nxp.com>; Diana Madalina Craciun (OSS) >>>> <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- >>>> acpi@vger.kernel.org; devicetree@vger.kernel.org; >>>> linux-pci@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Rafael >>>> J. Wysocki <rjw@rjwysocki.net>; Joerg Roedel <joro@8bytes.org>; >>>> Hanjun Guo <guohanjun@huawei.com>; Bjorn Helgaas >>>> <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; Robin >>>> Murphy <robin.murphy@arm.com>; Catalin Marinas >>>> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc >>>> Zyngier <maz@kernel.org> >>>> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for >>>> fsl-mc >>>> >>>> Caution: EXT Email >>>> >>>> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote: >>>>> >>>>> >>>>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: >>>>>> From: Makarand Pawagi <makarand.pawagi@nxp.com> >>>>>> >>>>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table >>>>>> to extract memory and other resources. >>>>>> >>>>>> Interrupt (GIC ITS) information is extracted from the MADT table by >>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. >>>>>> >>>>>> IORT table is parsed to configure DMA. >>>>>> >>>>>> Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> >>>>>> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> >>>>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>>>>> --- >>>>>> drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- >>>>>> drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- >>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 >>>>>> ++++++++++++++++----- >>>>>> 3 files changed, 150 insertions(+), 52 deletions(-) >>>>>> >>>>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c >>>>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89 >>>>>> 100644 >>>>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c >>>>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c >>>>>> @@ -18,6 +18,8 @@ >>>>>> #include <linux/bitops.h> >>>>>> #include <linux/msi.h> >>>>>> #include <linux/dma-mapping.h> >>>>>> +#include <linux/acpi.h> >>>>>> +#include <linux/iommu.h> >>>>>> >>>>>> #include "fsl-mc-private.h" >>>>>> >>>>>> @@ -38,6 +40,7 @@ struct fsl_mc { >>>>>> struct fsl_mc_device *root_mc_bus_dev; >>>>>> u8 num_translation_ranges; >>>>>> struct fsl_mc_addr_translation_range *translation_ranges; >>>>>> + void *fsl_mc_regs; >>>>>> }; >>>>>> >>>>>> /** >>>>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { >>>>>> phys_addr_t start_phys_addr; >>>>>> }; >>>>>> >>>>>> +#define FSL_MC_FAPR 0x28 >>>>>> +#define MC_FAPR_PL BIT(18) >>>>>> +#define MC_FAPR_BMT BIT(17) >>>>>> + >>>>>> /** >>>>>> * fsl_mc_bus_match - device to driver matching callback >>>>>> * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@ >>>>>> static int fsl_mc_dma_configure(struct device *dev) >>>>>> while (dev_is_fsl_mc(dma_dev)) >>>>>> dma_dev = dma_dev->parent; >>>>>> >>>>>> - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); >>>>>> + if (dev_of_node(dma_dev)) >>>>>> + return of_dma_configure_id(dev, dma_dev->of_node, 0, >>>>>> + &input_id); >>>>>> + >>>>>> + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); >>>>>> } >>>>>> >>>>>> static ssize_t modalias_show(struct device *dev, struct >>>>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int >>>> fsl_mc_bus_probe(struct platform_device *pdev) >>>>>> struct fsl_mc_io *mc_io = NULL; >>>>>> int container_id; >>>>>> phys_addr_t mc_portal_phys_addr; >>>>>> - u32 mc_portal_size; >>>>>> - struct resource res; >>>>>> + u32 mc_portal_size, mc_stream_id; >>>>>> + struct resource *plat_res; >>>>>> + >>>>>> + if (!iommu_present(&fsl_mc_bus_type)) >>>>>> + return -EPROBE_DEFER; >>>>>> >>>>>> mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); >>>>>> if (!mc) >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct >>>>>> platform_device *pdev) >>>>>> >>>>>> platform_set_drvdata(pdev, mc); >>>>>> >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >>>>>> + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); >>>>>> + if (IS_ERR(mc->fsl_mc_regs)) >>>>>> + return PTR_ERR(mc->fsl_mc_regs); >>>>>> + >>>>>> + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { >>>>>> + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); >>>>>> + /* >>>>>> + * HW ORs the PL and BMT bit, places the result in bit 15 of >>>>>> + * the StreamID and ORs in the ICID. Calculate it accordingly. >>>>>> + */ >>>>>> + mc_stream_id = (mc_stream_id & 0xffff) | >>>>>> + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? >>>>>> + 0x4000 : 0); >>>>>> + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, >>>>>> + &mc_stream_id); >>>>>> + if (error) >>>>>> + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", >>>>>> + error); >>>>>> + } >>>>>> + >>>>>> /* >>>>>> * Get physical address of MC portal for the root DPRC: >>>>>> */ >>>>>> - error = of_address_to_resource(pdev->dev.of_node, 0, &res); >>>>>> - if (error < 0) { >>>>>> - dev_err(&pdev->dev, >>>>>> - "of_address_to_resource() failed for %pOF\n", >>>>>> - pdev->dev.of_node); >>>>>> - return error; >>>>>> - } >>>>>> - >>>>>> - mc_portal_phys_addr = res.start; >>>>>> - mc_portal_size = resource_size(&res); >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> + mc_portal_phys_addr = plat_res->start; >>>>>> + mc_portal_size = resource_size(plat_res); >>>>>> error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, >>>>>> mc_portal_size, NULL, >>>>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, >>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct >>>> platform_device *pdev) >>>>>> dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", >>>>>> mc_version.major, mc_version.minor, >>>>>> mc_version.revision); >>>>>> >>>>>> - error = get_mc_addr_translation_ranges(&pdev->dev, >>>>>> - &mc->translation_ranges, >>>>>> - &mc->num_translation_ranges); >>>>>> - if (error < 0) >>>>>> - goto error_cleanup_mc_io; >>>>>> + if (dev_of_node(&pdev->dev)) { >>>>>> + error = get_mc_addr_translation_ranges(&pdev->dev, >>>>>> + &mc->translation_ranges, >>>>>> + &mc->num_translation_ranges); >>>>>> + if (error < 0) >>>>>> + goto error_cleanup_mc_io; >>>>>> + } >>>>>> >>>>>> error = dprc_get_container_id(mc_io, 0, &container_id); >>>>>> if (error < 0) { >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct >>>>>> platform_device >>>> *pdev) >>>>>> goto error_cleanup_mc_io; >>>>>> >>>>>> mc->root_mc_bus_dev = mc_bus_dev; >>>>>> + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; >>>>> >>>>> Makarand, this looks a bit weird. Is there really a reason for it? >>>> >>>> Can you clarify please so that we can reach a conclusion on this matter ? >>>> >>> Laurentiu, can you clarify what exactly is the doubt here? Are you asking about >> fwnode assignment from pdev to mc_bus_dev? >>> >> >> Yes. I remember that a while ago I tested without this fwnode assignment and >> didn't encounter any issues. Maybe we can just drop it? > > Did you tested with PHY changes? Because this is needed for MAC driver, where it needs the mc bus node. > Good point, no i haven't. Then I'm ok with leaving it for the time being. --- Best Regards, Laurentiu
On 7/9/2020 1:37 PM, Makarand Pawagi wrote: > >> -----Original Message----- >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> Sent: Thursday, July 9, 2020 3:45 PM >> To: Makarand Pawagi <makarand.pawagi@nxp.com>; Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org; Diana Madalina Craciun (OSS) >> <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- >> acpi@vger.kernel.org; devicetree@vger.kernel.org; linux-pci@vger.kernel.org; >> Rob Herring <robh+dt@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; >> Joerg Roedel <joro@8bytes.org>; Hanjun Guo <guohanjun@huawei.com>; Bjorn >> Helgaas <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; >> Robin Murphy <robin.murphy@arm.com>; Catalin Marinas >> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc Zyngier >> <maz@kernel.org> >> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc >> >> >> >> On 7/9/2020 12:26 PM, Makarand Pawagi wrote: >>> >>>> -----Original Message----- >>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>> Sent: Thursday, July 9, 2020 2:50 PM >>>> To: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>>> Cc: linux-arm-kernel@lists.infradead.org; Makarand Pawagi >>>> <makarand.pawagi@nxp.com>; Diana Madalina Craciun (OSS) >>>> <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; linux- >>>> acpi@vger.kernel.org; devicetree@vger.kernel.org; >>>> linux-pci@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Rafael >>>> J. Wysocki <rjw@rjwysocki.net>; Joerg Roedel <joro@8bytes.org>; >>>> Hanjun Guo <guohanjun@huawei.com>; Bjorn Helgaas >>>> <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; Robin >>>> Murphy <robin.murphy@arm.com>; Catalin Marinas >>>> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc >>>> Zyngier <maz@kernel.org> >>>> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for >>>> fsl-mc >>>> >>>> Caution: EXT Email >>>> >>>> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote: >>>>> >>>>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: >>>>>> From: Makarand Pawagi <makarand.pawagi@nxp.com> >>>>>> >>>>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table >>>>>> to extract memory and other resources. >>>>>> >>>>>> Interrupt (GIC ITS) information is extracted from the MADT table by >>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. >>>>>> >>>>>> IORT table is parsed to configure DMA. >>>>>> >>>>>> Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> >>>>>> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> >>>>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>>>>> --- >>>>>> drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- >>>>>> drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- >>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 >>>>>> ++++++++++++++++----- >>>>>> 3 files changed, 150 insertions(+), 52 deletions(-) >>>>>> >>>>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c >>>>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index 824ff77bbe86..324d49d6df89 >>>>>> 100644 >>>>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c >>>>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c >>>>>> @@ -18,6 +18,8 @@ >>>>>> #include <linux/bitops.h> >>>>>> #include <linux/msi.h> >>>>>> #include <linux/dma-mapping.h> >>>>>> +#include <linux/acpi.h> >>>>>> +#include <linux/iommu.h> >>>>>> >>>>>> #include "fsl-mc-private.h" >>>>>> >>>>>> @@ -38,6 +40,7 @@ struct fsl_mc { >>>>>> struct fsl_mc_device *root_mc_bus_dev; >>>>>> u8 num_translation_ranges; >>>>>> struct fsl_mc_addr_translation_range *translation_ranges; >>>>>> + void *fsl_mc_regs; >>>>>> }; >>>>>> >>>>>> /** >>>>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { >>>>>> phys_addr_t start_phys_addr; >>>>>> }; >>>>>> >>>>>> +#define FSL_MC_FAPR 0x28 >>>>>> +#define MC_FAPR_PL BIT(18) >>>>>> +#define MC_FAPR_BMT BIT(17) >>>>>> + >>>>>> /** >>>>>> * fsl_mc_bus_match - device to driver matching callback >>>>>> * @dev: the fsl-mc device to match against @@ -124,7 +131,10 @@ >>>>>> static int fsl_mc_dma_configure(struct device *dev) >>>>>> while (dev_is_fsl_mc(dma_dev)) >>>>>> dma_dev = dma_dev->parent; >>>>>> >>>>>> - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); >>>>>> + if (dev_of_node(dma_dev)) >>>>>> + return of_dma_configure_id(dev, dma_dev->of_node, 0, >>>>>> + &input_id); >>>>>> + >>>>>> + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id); >>>>>> } >>>>>> >>>>>> static ssize_t modalias_show(struct device *dev, struct >>>>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int >>>> fsl_mc_bus_probe(struct platform_device *pdev) >>>>>> struct fsl_mc_io *mc_io = NULL; >>>>>> int container_id; >>>>>> phys_addr_t mc_portal_phys_addr; >>>>>> - u32 mc_portal_size; >>>>>> - struct resource res; >>>>>> + u32 mc_portal_size, mc_stream_id; >>>>>> + struct resource *plat_res; >>>>>> + >>>>>> + if (!iommu_present(&fsl_mc_bus_type)) >>>>>> + return -EPROBE_DEFER; >>>>>> >>>>>> mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); >>>>>> if (!mc) >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct >>>>>> platform_device *pdev) >>>>>> >>>>>> platform_set_drvdata(pdev, mc); >>>>>> >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >>>>>> + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); >>>>>> + if (IS_ERR(mc->fsl_mc_regs)) >>>>>> + return PTR_ERR(mc->fsl_mc_regs); >>>>>> + >>>>>> + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { >>>>>> + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); >>>>>> + /* >>>>>> + * HW ORs the PL and BMT bit, places the result in bit 15 of >>>>>> + * the StreamID and ORs in the ICID. Calculate it accordingly. >>>>>> + */ >>>>>> + mc_stream_id = (mc_stream_id & 0xffff) | >>>>>> + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? >>>>>> + 0x4000 : 0); >>>>>> + error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT, >>>>>> + &mc_stream_id); >>>>>> + if (error) >>>>>> + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", >>>>>> + error); >>>>>> + } >>>>>> + >>>>>> /* >>>>>> * Get physical address of MC portal for the root DPRC: >>>>>> */ >>>>>> - error = of_address_to_resource(pdev->dev.of_node, 0, &res); >>>>>> - if (error < 0) { >>>>>> - dev_err(&pdev->dev, >>>>>> - "of_address_to_resource() failed for %pOF\n", >>>>>> - pdev->dev.of_node); >>>>>> - return error; >>>>>> - } >>>>>> - >>>>>> - mc_portal_phys_addr = res.start; >>>>>> - mc_portal_size = resource_size(&res); >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> + mc_portal_phys_addr = plat_res->start; >>>>>> + mc_portal_size = resource_size(plat_res); >>>>>> error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, >>>>>> mc_portal_size, NULL, >>>>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, >>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct >>>> platform_device *pdev) >>>>>> dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", >>>>>> mc_version.major, mc_version.minor, >>>>>> mc_version.revision); >>>>>> >>>>>> - error = get_mc_addr_translation_ranges(&pdev->dev, >>>>>> - &mc->translation_ranges, >>>>>> - &mc->num_translation_ranges); >>>>>> - if (error < 0) >>>>>> - goto error_cleanup_mc_io; >>>>>> + if (dev_of_node(&pdev->dev)) { >>>>>> + error = get_mc_addr_translation_ranges(&pdev->dev, >>>>>> + &mc->translation_ranges, >>>>>> + &mc->num_translation_ranges); >>>>>> + if (error < 0) >>>>>> + goto error_cleanup_mc_io; >>>>>> + } >>>>>> >>>>>> error = dprc_get_container_id(mc_io, 0, &container_id); >>>>>> if (error < 0) { >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct >>>>>> platform_device >>>> *pdev) >>>>>> goto error_cleanup_mc_io; >>>>>> >>>>>> mc->root_mc_bus_dev = mc_bus_dev; >>>>>> + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; >>>>> Makarand, this looks a bit weird. Is there really a reason for it? >>>> Can you clarify please so that we can reach a conclusion on this matter ? >>>> >>> Laurentiu, can you clarify what exactly is the doubt here? Are you asking about >> fwnode assignment from pdev to mc_bus_dev? >> Yes. I remember that a while ago I tested without this fwnode assignment and >> didn't encounter any issues. Maybe we can just drop it? > Did you tested with PHY changes? Because this is needed for MAC driver, where it needs the mc bus node. Maybe it worth a comment or maybe have it in a different patch? Thanks, Diana > >> --- >> Best Regards, Laurentiu
> -----Original Message----- > From: Diana Madalina Craciun (OSS) <diana.craciun@oss.nxp.com> > Sent: Thursday, July 9, 2020 4:17 PM > To: Makarand Pawagi <makarand.pawagi@nxp.com>; Laurentiu Tudor > <laurentiu.tudor@nxp.com>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: linux-arm-kernel@lists.infradead.org; iommu@lists.linux-foundation.org; > linux-acpi@vger.kernel.org; devicetree@vger.kernel.org; linux- > pci@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Rafael J. Wysocki > <rjw@rjwysocki.net>; Joerg Roedel <joro@8bytes.org>; Hanjun Guo > <guohanjun@huawei.com>; Bjorn Helgaas <bhelgaas@google.com>; Sudeep > Holla <sudeep.holla@arm.com>; Robin Murphy <robin.murphy@arm.com>; > Catalin Marinas <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; > Marc Zyngier <maz@kernel.org> > Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc > > On 7/9/2020 1:37 PM, Makarand Pawagi wrote: > > > >> -----Original Message----- > >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > >> Sent: Thursday, July 9, 2020 3:45 PM > >> To: Makarand Pawagi <makarand.pawagi@nxp.com>; Lorenzo Pieralisi > >> <lorenzo.pieralisi@arm.com> > >> Cc: linux-arm-kernel@lists.infradead.org; Diana Madalina Craciun > >> (OSS) <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; > >> linux- acpi@vger.kernel.org; devicetree@vger.kernel.org; > >> linux-pci@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Rafael > >> J. Wysocki <rjw@rjwysocki.net>; Joerg Roedel <joro@8bytes.org>; > >> Hanjun Guo <guohanjun@huawei.com>; Bjorn Helgaas > >> <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; Robin > >> Murphy <robin.murphy@arm.com>; Catalin Marinas > >> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc > >> Zyngier <maz@kernel.org> > >> Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support > >> for fsl-mc > >> > >> > >> > >> On 7/9/2020 12:26 PM, Makarand Pawagi wrote: > >>> > >>>> -----Original Message----- > >>>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >>>> Sent: Thursday, July 9, 2020 2:50 PM > >>>> To: Laurentiu Tudor <laurentiu.tudor@nxp.com> > >>>> Cc: linux-arm-kernel@lists.infradead.org; Makarand Pawagi > >>>> <makarand.pawagi@nxp.com>; Diana Madalina Craciun (OSS) > >>>> <diana.craciun@oss.nxp.com>; iommu@lists.linux-foundation.org; > >>>> linux- acpi@vger.kernel.org; devicetree@vger.kernel.org; > >>>> linux-pci@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Rafael > >>>> J. Wysocki <rjw@rjwysocki.net>; Joerg Roedel <joro@8bytes.org>; > >>>> Hanjun Guo <guohanjun@huawei.com>; Bjorn Helgaas > >>>> <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; Robin > >>>> Murphy <robin.murphy@arm.com>; Catalin Marinas > >>>> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc > >>>> Zyngier <maz@kernel.org> > >>>> Subject: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support > >>>> for fsl-mc > >>>> > >>>> Caution: EXT Email > >>>> > >>>> On Wed, Jul 01, 2020 at 07:55:28PM +0300, Laurentiu Tudor wrote: > >>>>> > >>>>> On 6/19/2020 11:20 AM, Lorenzo Pieralisi wrote: > >>>>>> From: Makarand Pawagi <makarand.pawagi@nxp.com> > >>>>>> > >>>>>> Add ACPI support in the fsl-mc driver. Driver parses MC DSDT > >>>>>> table to extract memory and other resources. > >>>>>> > >>>>>> Interrupt (GIC ITS) information is extracted from the MADT table > >>>>>> by drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c. > >>>>>> > >>>>>> IORT table is parsed to configure DMA. > >>>>>> > >>>>>> Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com> > >>>>>> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > >>>>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > >>>>>> --- > >>>>>> drivers/bus/fsl-mc/fsl-mc-bus.c | 73 ++++++++++++---- > >>>>>> drivers/bus/fsl-mc/fsl-mc-msi.c | 37 +++++---- > >>>>>> drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 92 > >>>>>> ++++++++++++++++----- > >>>>>> 3 files changed, 150 insertions(+), 52 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c > >>>>>> b/drivers/bus/fsl-mc/fsl-mc-bus.c index > >>>>>> 824ff77bbe86..324d49d6df89 > >>>>>> 100644 > >>>>>> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c > >>>>>> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c > >>>>>> @@ -18,6 +18,8 @@ > >>>>>> #include <linux/bitops.h> > >>>>>> #include <linux/msi.h> > >>>>>> #include <linux/dma-mapping.h> > >>>>>> +#include <linux/acpi.h> > >>>>>> +#include <linux/iommu.h> > >>>>>> > >>>>>> #include "fsl-mc-private.h" > >>>>>> > >>>>>> @@ -38,6 +40,7 @@ struct fsl_mc { > >>>>>> struct fsl_mc_device *root_mc_bus_dev; > >>>>>> u8 num_translation_ranges; > >>>>>> struct fsl_mc_addr_translation_range *translation_ranges; > >>>>>> + void *fsl_mc_regs; > >>>>>> }; > >>>>>> > >>>>>> /** > >>>>>> @@ -56,6 +59,10 @@ struct fsl_mc_addr_translation_range { > >>>>>> phys_addr_t start_phys_addr; > >>>>>> }; > >>>>>> > >>>>>> +#define FSL_MC_FAPR 0x28 > >>>>>> +#define MC_FAPR_PL BIT(18) > >>>>>> +#define MC_FAPR_BMT BIT(17) > >>>>>> + > >>>>>> /** > >>>>>> * fsl_mc_bus_match - device to driver matching callback > >>>>>> * @dev: the fsl-mc device to match against @@ -124,7 +131,10 > >>>>>> @@ static int fsl_mc_dma_configure(struct device *dev) > >>>>>> while (dev_is_fsl_mc(dma_dev)) > >>>>>> dma_dev = dma_dev->parent; > >>>>>> > >>>>>> - return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id); > >>>>>> + if (dev_of_node(dma_dev)) > >>>>>> + return of_dma_configure_id(dev, dma_dev->of_node, 0, > >>>>>> + &input_id); > >>>>>> + > >>>>>> + return acpi_dma_configure_id(dev, DEV_DMA_COHERENT, > >>>>>> + &input_id); > >>>>>> } > >>>>>> > >>>>>> static ssize_t modalias_show(struct device *dev, struct > >>>>>> device_attribute *attr, @@ -865,8 +875,11 @@ static int > >>>> fsl_mc_bus_probe(struct platform_device *pdev) > >>>>>> struct fsl_mc_io *mc_io = NULL; > >>>>>> int container_id; > >>>>>> phys_addr_t mc_portal_phys_addr; > >>>>>> - u32 mc_portal_size; > >>>>>> - struct resource res; > >>>>>> + u32 mc_portal_size, mc_stream_id; > >>>>>> + struct resource *plat_res; > >>>>>> + > >>>>>> + if (!iommu_present(&fsl_mc_bus_type)) > >>>>>> + return -EPROBE_DEFER; > >>>>>> > >>>>>> mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > >>>>>> if (!mc) > >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct > >>>>>> platform_device *pdev) > >>>>>> > >>>>>> platform_set_drvdata(pdev, mc); > >>>>>> > >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > >>>>>> + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > >>>>>> + if (IS_ERR(mc->fsl_mc_regs)) > >>>>>> + return PTR_ERR(mc->fsl_mc_regs); > >>>>>> + > >>>>>> + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > >>>>>> + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > >>>>>> + /* > >>>>>> + * HW ORs the PL and BMT bit, places the result in bit 15 of > >>>>>> + * the StreamID and ORs in the ICID. Calculate it accordingly. > >>>>>> + */ > >>>>>> + mc_stream_id = (mc_stream_id & 0xffff) | > >>>>>> + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > >>>>>> + 0x4000 : 0); > >>>>>> + error = acpi_dma_configure_id(&pdev->dev, > DEV_DMA_COHERENT, > >>>>>> + &mc_stream_id); > >>>>>> + if (error) > >>>>>> + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > >>>>>> + error); > >>>>>> + } > >>>>>> + > >>>>>> /* > >>>>>> * Get physical address of MC portal for the root DPRC: > >>>>>> */ > >>>>>> - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > >>>>>> - if (error < 0) { > >>>>>> - dev_err(&pdev->dev, > >>>>>> - "of_address_to_resource() failed for %pOF\n", > >>>>>> - pdev->dev.of_node); > >>>>>> - return error; > >>>>>> - } > >>>>>> - > >>>>>> - mc_portal_phys_addr = res.start; > >>>>>> - mc_portal_size = resource_size(&res); > >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>>>> + mc_portal_phys_addr = plat_res->start; > >>>>>> + mc_portal_size = resource_size(plat_res); > >>>>>> error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > >>>>>> mc_portal_size, NULL, > >>>>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, > >>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct > >>>> platform_device *pdev) > >>>>>> dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > >>>>>> mc_version.major, mc_version.minor, > >>>>>> mc_version.revision); > >>>>>> > >>>>>> - error = get_mc_addr_translation_ranges(&pdev->dev, > >>>>>> - &mc->translation_ranges, > >>>>>> - &mc->num_translation_ranges); > >>>>>> - if (error < 0) > >>>>>> - goto error_cleanup_mc_io; > >>>>>> + if (dev_of_node(&pdev->dev)) { > >>>>>> + error = get_mc_addr_translation_ranges(&pdev->dev, > >>>>>> + &mc->translation_ranges, > >>>>>> + &mc->num_translation_ranges); > >>>>>> + if (error < 0) > >>>>>> + goto error_cleanup_mc_io; > >>>>>> + } > >>>>>> > >>>>>> error = dprc_get_container_id(mc_io, 0, &container_id); > >>>>>> if (error < 0) { > >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct > >>>>>> platform_device > >>>> *pdev) > >>>>>> goto error_cleanup_mc_io; > >>>>>> > >>>>>> mc->root_mc_bus_dev = mc_bus_dev; > >>>>>> + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > >>>>> Makarand, this looks a bit weird. Is there really a reason for it? > >>>> Can you clarify please so that we can reach a conclusion on this matter ? > >>>> > >>> Laurentiu, can you clarify what exactly is the doubt here? Are you > >>> asking about > >> fwnode assignment from pdev to mc_bus_dev? > >> Yes. I remember that a while ago I tested without this fwnode > >> assignment and didn't encounter any issues. Maybe we can just drop it? > > Did you tested with PHY changes? Because this is needed for MAC driver, > where it needs the mc bus node. > > Maybe it worth a comment or maybe have it in a different patch? > Since this change is needed for ACPI case and this is ACPI support case, I feel we should have this change in this patch only instead of separate patch. > Thanks, > Diana > > > > >> --- > >> Best Regards, Laurentiu
On 2020/7/9 17:21, Lorenzo Pieralisi wrote: > On Thu, Jul 02, 2020 at 04:22:00PM +0800, Hanjun Guo wrote: >> Hi Robin, >> >> On 2020/7/2 0:12, Robin Murphy wrote: >>> On 2020-06-30 14:04, Hanjun Guo wrote: >>>> On 2020/6/30 18:24, Lorenzo Pieralisi wrote: >>>>> On Tue, Jun 30, 2020 at 11:06:41AM +0800, Hanjun Guo wrote: >>>>> >>>>> [...] >>>>> >>>>>>> For devices that aren't described in the DSDT - IORT translations >>>>>>> are determined by their ACPI parent device. Do you see/Have you >>>>>>> found any issue with this approach ? >>>>>> >>>>>> The spec says "Describes the IO relationships between devices >>>>>> represented in the ACPI namespace.", and in section 3.1.1.3 Named >>>>>> component node, it says: >>>>> >>>>> PCI devices aren't necessarily described in the ACPI namespace and we >>>>> still use IORT to describe them - through the RC node. >>>>> >>>>>> "Named component nodes are used to describe devices that are also >>>>>> included in the Differentiated System Description Table (DSDT). See >>>>>> [ACPI]." >>>>>> >>>>>> So from my understanding, the IORT spec for now, can only do ID >>>>>> translations for devices in the DSDT. >>>>> >>>>> I think you can read this multiple ways but this patch does not >>>>> change this concept. What changes, is applying parent's node IORT >>>>> mapping to child nodes with no associated DSDT nodes, it is the >>>>> same thing we do with PCI and the _DMA method - we could update >>>>> the wording in the specs if that clarifies but I don't think this >>>>> deliberately disregards the specifications. >>>> >>>> I agree, but it's better to update the wording of the spec. >>>> >>>>> >>>>>>>> For a platform device, if I use its parent's full path name for >>>>>>>> its named component entry, then it will match, but this will violate >>>>>>>> the IORT spec. >>>>>>> >>>>>>> Can you elaborate on this please I don't get the point you >>>>>>> are making. >>>>>> >>>>>> For example, device A is not described in DSDT so can't represent >>>>>> as a NC node in IORT. Device B can be described in DSDT and it >>>>>> is the parent of device A, so device B can be represented in IORT >>>>>> with memory access properties and node flags with Substream width >>>>>> and Stall supported info. >>>>>> >>>>>> When we trying to translate device A's ID, we reuse all the memory >>>>>> access properties and node flags from its parent (device B), but >>>>>> will it the same? >>>>> >>>>> I assume so why wouldn't it be ? Why would be describe them in >>>>> a parent-child relationship if that's not how the system looks like >>>>> in HW ? >>>> >>>> The point I'm making is that I'm not sure all the memory access and >>>> stall properties are the same for the parent and the device itself. >>> >>> Is that even a valid case though? The principal thing we want to >>> accommodate here is when device B *is* the one accessing memory, either >>> because it is a bridge with device A sat behind it, or because device A >>> is actually just some logical function or subset of physical device B. >> >> Thanks for the clarify, for CCA attributes, child device should be the >> same as its parent and that was written in the ACPI spec, so it's better >> to make it clear for other properties in the spec as well. >> >>> >>> If the topology is such that device A is a completely independent device >>> with its own path to memory such that it could have different >>> properties, I would expect that it *should* be described in DSDT, and I >>> can't easily think of a good reason why it wouldn't be. I'm also >>> struggling to imagine how it might even have an ID that had to be >>> interpreted in the context of device B if it wasn't one of the cases >>> above :/ >>> >>> I don't doubt that people could - or maybe even have - come up with crap >>> DSDT bindings that don't represent the hardware sufficiently accurately, >>> but I'm not sure that should be IORT's problem... >> >> As I said in previous email, I'm not against this patch, and seems >> have no regressions for platforms that using named component node >> such as D05/D06 (I will test it shortly to make sure), but it's better >> to update the wording of the spec (even after this patch set is merged). > > Have you managed to test this series ? Sorry, I forgot to reply the email after the testing, adding this patch set on top of 5.8-rc3 and test it on D06, it worked well, and I printed every match device's name to confirm that there are no regressions. Thanks Hanjun
On Thu, Jul 09, 2020 at 10:35:14AM +0100, Lorenzo Pieralisi wrote: > On Fri, Jun 19, 2020 at 09:20:06AM +0100, Lorenzo Pieralisi wrote: > > Some HW devices are created as child devices of proprietary busses, > > that have a bus specific policy defining how the child devices > > wires representing the devices ID are translated into IOMMU and > > IRQ controllers device IDs. > > > > Current IORT code provides translations for: > > > > - PCI devices, where the device ID is well identified at bus level > > as the requester ID (RID) > > - Platform devices that are endpoint devices where the device ID is > > retrieved from the ACPI object IORT mappings (Named components single > > mappings). A platform device is represented in IORT as a named > > component node > > > > For devices that are child devices of proprietary busses the IORT > > firmware represents the bus node as a named component node in IORT > > and it is up to that named component node to define in/out bus > > specific ID translations for the bus child devices that are > > allocated and created in a bus specific manner. > > > > In order to make IORT ID translations available for proprietary > > bus child devices, the current ACPI (and IORT) code must be > > augmented to provide an additional ID parameter to acpi_dma_configure() > > representing the child devices input ID. This ID is bus specific > > and it is retrieved in bus specific code. > > > > By adding an ID parameter to acpi_dma_configure(), the IORT > > code can map the child device ID to an IOMMU stream ID through > > the IORT named component representing the bus in/out ID mappings. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Hanjun Guo <guohanjun@huawei.com> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > --- > > drivers/acpi/arm64/iort.c | 59 +++++++++++++++++++++++++++++---------- > > drivers/acpi/scan.c | 8 ++++-- > > include/acpi/acpi_bus.h | 9 ++++-- > > include/linux/acpi.h | 7 +++++ > > include/linux/acpi_iort.h | 7 +++-- > > 5 files changed, 67 insertions(+), 23 deletions(-) > > Hi Rafael, > > just to ask if the ACPI core changes in this patch are OK with you, > thank you very much. Hi Rafael, are you OK with ACPI core changes in this patch ? Please let me know, thanks. Lorenzo > Lorenzo > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index 421c6976ab81..ec782e4a0fe4 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -978,19 +978,54 @@ static void iort_named_component_init(struct device *dev, > > nc->node_flags); > > } > > > > +static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node) > > +{ > > + struct acpi_iort_node *parent; > > + int err = -ENODEV, i = 0; > > + u32 streamid = 0; > > + > > + do { > > + > > + parent = iort_node_map_platform_id(node, &streamid, > > + IORT_IOMMU_TYPE, > > + i++); > > + > > + if (parent) > > + err = iort_iommu_xlate(dev, parent, streamid); > > + } while (parent && !err); > > + > > + return err; > > +} > > + > > +static int iort_nc_iommu_map_id(struct device *dev, > > + struct acpi_iort_node *node, > > + const u32 *in_id) > > +{ > > + struct acpi_iort_node *parent; > > + u32 streamid; > > + > > + parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE); > > + if (parent) > > + return iort_iommu_xlate(dev, parent, streamid); > > + > > + return -ENODEV; > > +} > > + > > + > > /** > > - * iort_iommu_configure - Set-up IOMMU configuration for a device. > > + * iort_iommu_configure_id - Set-up IOMMU configuration for a device. > > * > > * @dev: device to configure > > + * @id_in: optional input id const value pointer > > * > > * Returns: iommu_ops pointer on configuration success > > * NULL on configuration failure > > */ > > -const struct iommu_ops *iort_iommu_configure(struct device *dev) > > +const struct iommu_ops *iort_iommu_configure_id(struct device *dev, > > + const u32 *id_in) > > { > > - struct acpi_iort_node *node, *parent; > > + struct acpi_iort_node *node; > > const struct iommu_ops *ops; > > - u32 streamid = 0; > > int err = -ENODEV; > > > > /* > > @@ -1019,21 +1054,13 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev) > > if (fwspec && iort_pci_rc_supports_ats(node)) > > fwspec->flags |= IOMMU_FWSPEC_PCI_RC_ATS; > > } else { > > - int i = 0; > > - > > node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT, > > iort_match_node_callback, dev); > > if (!node) > > return NULL; > > > > - do { > > - parent = iort_node_map_platform_id(node, &streamid, > > - IORT_IOMMU_TYPE, > > - i++); > > - > > - if (parent) > > - err = iort_iommu_xlate(dev, parent, streamid); > > - } while (parent && !err); > > + err = id_in ? iort_nc_iommu_map_id(dev, node, id_in) : > > + iort_nc_iommu_map(dev, node); > > > > if (!err) > > iort_named_component_init(dev, node); > > @@ -1058,6 +1085,7 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev) > > > > return ops; > > } > > + > > #else > > static inline const struct iommu_ops *iort_fwspec_iommu_ops(struct device *dev) > > { return NULL; } > > @@ -1066,7 +1094,8 @@ static inline int iort_add_device_replay(const struct iommu_ops *ops, > > { return 0; } > > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head) > > { return 0; } > > -const struct iommu_ops *iort_iommu_configure(struct device *dev) > > +const struct iommu_ops *iort_iommu_configure_id(struct device *dev, > > + const u32 *input_id) > > { return NULL; } > > #endif > > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > > index 8777faced51a..2142f1554761 100644 > > --- a/drivers/acpi/scan.c > > +++ b/drivers/acpi/scan.c > > @@ -1457,8 +1457,10 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, > > * acpi_dma_configure - Set-up DMA configuration for the device. > > * @dev: The pointer to the device > > * @attr: device dma attributes > > + * @input_id: input device id const value pointer > > */ > > -int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) > > +int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr, > > + const u32 *input_id) > > { > > const struct iommu_ops *iommu; > > u64 dma_addr = 0, size = 0; > > @@ -1470,7 +1472,7 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) > > > > iort_dma_setup(dev, &dma_addr, &size); > > > > - iommu = iort_iommu_configure(dev); > > + iommu = iort_iommu_configure_id(dev, input_id); > > if (PTR_ERR(iommu) == -EPROBE_DEFER) > > return -EPROBE_DEFER; > > > > @@ -1479,7 +1481,7 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr) > > > > return 0; > > } > > -EXPORT_SYMBOL_GPL(acpi_dma_configure); > > +EXPORT_SYMBOL_GPL(acpi_dma_configure_id); > > > > static void acpi_init_coherency(struct acpi_device *adev) > > { > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > index 5afb6ceb284f..a3abcc4b7d9f 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -588,8 +588,13 @@ bool acpi_dma_supported(struct acpi_device *adev); > > enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev); > > int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, > > u64 *size); > > -int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr); > > - > > +int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr, > > + const u32 *input_id); > > +static inline int acpi_dma_configure(struct device *dev, > > + enum dev_dma_attr attr) > > +{ > > + return acpi_dma_configure_id(dev, attr, NULL); > > +} > > struct acpi_device *acpi_find_child_device(struct acpi_device *parent, > > u64 address, bool check_children); > > int acpi_is_root_bridge(acpi_handle); > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index d661cd0ee64d..6d2c47489d90 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -905,6 +905,13 @@ static inline int acpi_dma_configure(struct device *dev, > > return 0; > > } > > > > +static inline int acpi_dma_configure_id(struct device *dev, > > + enum dev_dma_attr attr, > > + const u32 *input_id) > > +{ > > + return 0; > > +} > > + > > #define ACPI_PTR(_ptr) (NULL) > > > > static inline void acpi_device_set_enumerated(struct acpi_device *adev) > > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h > > index e51425e083da..20a32120bb88 100644 > > --- a/include/linux/acpi_iort.h > > +++ b/include/linux/acpi_iort.h > > @@ -35,7 +35,8 @@ void acpi_configure_pmsi_domain(struct device *dev); > > int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id); > > /* IOMMU interface */ > > void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *size); > > -const struct iommu_ops *iort_iommu_configure(struct device *dev); > > +const struct iommu_ops *iort_iommu_configure_id(struct device *dev, > > + const u32 *id_in); > > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head); > > #else > > static inline void acpi_iort_init(void) { } > > @@ -48,8 +49,8 @@ static inline void acpi_configure_pmsi_domain(struct device *dev) { } > > /* IOMMU interface */ > > static inline void iort_dma_setup(struct device *dev, u64 *dma_addr, > > u64 *size) { } > > -static inline const struct iommu_ops *iort_iommu_configure( > > - struct device *dev) > > +static inline const struct iommu_ops *iort_iommu_configure_id( > > + struct device *dev, const u32 *id_in) > > { return NULL; } > > static inline > > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head) > > -- > > 2.26.1 > >
On Fri, Jun 19, 2020 at 09:20:04AM +0100, Lorenzo Pieralisi wrote: > There is nothing PCI specific in iort_msi_map_rid(). > > Rename the function using a bus protocol agnostic name, > iort_msi_map_id(), and convert current callers to it. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Hanjun Guo <guohanjun@huawei.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > --- > drivers/acpi/arm64/iort.c | 12 ++++++------ > drivers/pci/msi.c | 2 +- Hi Bjorn, please let me know if you are OK with this change, thanks. Lorenzo > include/linux/acpi_iort.h | 6 +++--- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 902e2aaca946..53f9ef515089 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -568,22 +568,22 @@ static struct acpi_iort_node *iort_find_dev_node(struct device *dev) > } > > /** > - * iort_msi_map_rid() - Map a MSI requester ID for a device > + * iort_msi_map_id() - Map a MSI input ID for a device > * @dev: The device for which the mapping is to be done. > - * @req_id: The device requester ID. > + * @input_id: The device input ID. > * > - * Returns: mapped MSI RID on success, input requester ID otherwise > + * Returns: mapped MSI ID on success, input ID otherwise > */ > -u32 iort_msi_map_rid(struct device *dev, u32 req_id) > +u32 iort_msi_map_id(struct device *dev, u32 input_id) > { > struct acpi_iort_node *node; > u32 dev_id; > > node = iort_find_dev_node(dev); > if (!node) > - return req_id; > + return input_id; > > - iort_node_map_id(node, req_id, &dev_id, IORT_MSI_TYPE); > + iort_node_map_id(node, input_id, &dev_id, IORT_MSI_TYPE); > return dev_id; > } > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index 74a91f52ecc0..77f48b95e277 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -1536,7 +1536,7 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev) > > of_node = irq_domain_get_of_node(domain); > rid = of_node ? of_msi_map_rid(&pdev->dev, of_node, rid) : > - iort_msi_map_rid(&pdev->dev, rid); > + iort_msi_map_id(&pdev->dev, rid); > > return rid; > } > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h > index 08ec6bd2297f..e51425e083da 100644 > --- a/include/linux/acpi_iort.h > +++ b/include/linux/acpi_iort.h > @@ -28,7 +28,7 @@ void iort_deregister_domain_token(int trans_id); > struct fwnode_handle *iort_find_domain_token(int trans_id); > #ifdef CONFIG_ACPI_IORT > void acpi_iort_init(void); > -u32 iort_msi_map_rid(struct device *dev, u32 req_id); > +u32 iort_msi_map_id(struct device *dev, u32 id); > struct irq_domain *iort_get_device_domain(struct device *dev, u32 id, > enum irq_domain_bus_token bus_token); > void acpi_configure_pmsi_domain(struct device *dev); > @@ -39,8 +39,8 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev); > int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head); > #else > static inline void acpi_iort_init(void) { } > -static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id) > -{ return req_id; } > +static inline u32 iort_msi_map_id(struct device *dev, u32 id) > +{ return id; } > static inline struct irq_domain *iort_get_device_domain( > struct device *dev, u32 id, enum irq_domain_bus_token bus_token) > { return NULL; } > -- > 2.26.1 >
On Thu, Jul 09, 2020 at 10:52:52AM +0000, Makarand Pawagi wrote: [...] > > >>>> fsl_mc_bus_probe(struct platform_device *pdev) > > >>>>>> struct fsl_mc_io *mc_io = NULL; > > >>>>>> int container_id; > > >>>>>> phys_addr_t mc_portal_phys_addr; > > >>>>>> - u32 mc_portal_size; > > >>>>>> - struct resource res; > > >>>>>> + u32 mc_portal_size, mc_stream_id; > > >>>>>> + struct resource *plat_res; > > >>>>>> + > > >>>>>> + if (!iommu_present(&fsl_mc_bus_type)) > > >>>>>> + return -EPROBE_DEFER; > > >>>>>> > > >>>>>> mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > > >>>>>> if (!mc) > > >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct > > >>>>>> platform_device *pdev) > > >>>>>> > > >>>>>> platform_set_drvdata(pdev, mc); > > >>>>>> > > >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > >>>>>> + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, plat_res); > > >>>>>> + if (IS_ERR(mc->fsl_mc_regs)) > > >>>>>> + return PTR_ERR(mc->fsl_mc_regs); > > >>>>>> + > > >>>>>> + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > > >>>>>> + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > > >>>>>> + /* > > >>>>>> + * HW ORs the PL and BMT bit, places the result in bit 15 of > > >>>>>> + * the StreamID and ORs in the ICID. Calculate it accordingly. > > >>>>>> + */ > > >>>>>> + mc_stream_id = (mc_stream_id & 0xffff) | > > >>>>>> + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > > >>>>>> + 0x4000 : 0); > > >>>>>> + error = acpi_dma_configure_id(&pdev->dev, > > DEV_DMA_COHERENT, > > >>>>>> + &mc_stream_id); > > >>>>>> + if (error) > > >>>>>> + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > > >>>>>> + error); > > >>>>>> + } > > >>>>>> + > > >>>>>> /* > > >>>>>> * Get physical address of MC portal for the root DPRC: > > >>>>>> */ > > >>>>>> - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > > >>>>>> - if (error < 0) { > > >>>>>> - dev_err(&pdev->dev, > > >>>>>> - "of_address_to_resource() failed for %pOF\n", > > >>>>>> - pdev->dev.of_node); > > >>>>>> - return error; > > >>>>>> - } > > >>>>>> - > > >>>>>> - mc_portal_phys_addr = res.start; > > >>>>>> - mc_portal_size = resource_size(&res); > > >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > >>>>>> + mc_portal_phys_addr = plat_res->start; > > >>>>>> + mc_portal_size = resource_size(plat_res); > > >>>>>> error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > > >>>>>> mc_portal_size, NULL, > > >>>>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, > > >>>>>> &mc_io); @@ -903,11 +930,13 @@ static int fsl_mc_bus_probe(struct > > >>>> platform_device *pdev) > > >>>>>> dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > > >>>>>> mc_version.major, mc_version.minor, > > >>>>>> mc_version.revision); > > >>>>>> > > >>>>>> - error = get_mc_addr_translation_ranges(&pdev->dev, > > >>>>>> - &mc->translation_ranges, > > >>>>>> - &mc->num_translation_ranges); > > >>>>>> - if (error < 0) > > >>>>>> - goto error_cleanup_mc_io; > > >>>>>> + if (dev_of_node(&pdev->dev)) { > > >>>>>> + error = get_mc_addr_translation_ranges(&pdev->dev, > > >>>>>> + &mc->translation_ranges, > > >>>>>> + &mc->num_translation_ranges); > > >>>>>> + if (error < 0) > > >>>>>> + goto error_cleanup_mc_io; > > >>>>>> + } > > >>>>>> > > >>>>>> error = dprc_get_container_id(mc_io, 0, &container_id); > > >>>>>> if (error < 0) { > > >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct > > >>>>>> platform_device > > >>>> *pdev) > > >>>>>> goto error_cleanup_mc_io; > > >>>>>> > > >>>>>> mc->root_mc_bus_dev = mc_bus_dev; > > >>>>>> + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > > >>>>> Makarand, this looks a bit weird. Is there really a reason for it? > > >>>> Can you clarify please so that we can reach a conclusion on this matter ? > > >>>> > > >>> Laurentiu, can you clarify what exactly is the doubt here? Are you > > >>> asking about > > >> fwnode assignment from pdev to mc_bus_dev? > > >> Yes. I remember that a while ago I tested without this fwnode > > >> assignment and didn't encounter any issues. Maybe we can just drop it? > > > Did you tested with PHY changes? Because this is needed for MAC driver, > > where it needs the mc bus node. > > > > Maybe it worth a comment or maybe have it in a different patch? > > > Since this change is needed for ACPI case and this is ACPI support > case, I feel we should have this change in this patch only instead of > separate patch. Anyway - you need to seek feedback from Marc on whether patches 11 and 12 are OK from an irqchip perspective, it is possible we can take the rest of the series independently if everyone agrees but I don't necessarily see a reason for that. Long story short: you need Marc's ACK on [11-12], it is your code. Thanks, Lorenzo
On 2020-06-19 09:20, Lorenzo Pieralisi wrote: > From: Diana Craciun <diana.craciun@oss.nxp.com> > > The DPRC driver is not taking into account the msi-map property > and assumes that the icid is the same as the stream ID. Although > this assumption is correct, generalize the code to include a > translation between icid and streamID. > > Furthermore do not just copy the MSI domain from parent (for child > containers), but use the information provided by the msi-map property. > > If the msi-map property is missing from the device tree retain the old > behaviour for backward compatibility ie the child DPRC objects > inherit the MSI domain from the parent. > > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com> > --- > drivers/bus/fsl-mc/dprc-driver.c | 31 ++++++--------------- > drivers/bus/fsl-mc/fsl-mc-bus.c | 4 +-- > drivers/bus/fsl-mc/fsl-mc-msi.c | 31 +++++++++++++-------- > drivers/bus/fsl-mc/fsl-mc-private.h | 6 ++-- > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 15 +++++++++- > 5 files changed, 47 insertions(+), 40 deletions(-) For this patch and the following one: Acked-by: Marc Zyngier <maz@kernel.org> M.
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Wednesday, July 15, 2020 3:37 PM > To: Makarand Pawagi <makarand.pawagi@nxp.com> > Cc: Diana Madalina Craciun (OSS) <diana.craciun@oss.nxp.com>; Laurentiu > Tudor <laurentiu.tudor@nxp.com>; linux-arm-kernel@lists.infradead.org; > iommu@lists.linux-foundation.org; linux-acpi@vger.kernel.org; > devicetree@vger.kernel.org; linux-pci@vger.kernel.org; Rob Herring > <robh+dt@kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Joerg Roedel > <joro@8bytes.org>; Hanjun Guo <guohanjun@huawei.com>; Bjorn Helgaas > <bhelgaas@google.com>; Sudeep Holla <sudeep.holla@arm.com>; Robin > Murphy <robin.murphy@arm.com>; Catalin Marinas > <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Marc Zyngier > <maz@kernel.org> > Subject: Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc > > Caution: EXT Email > > On Thu, Jul 09, 2020 at 10:52:52AM +0000, Makarand Pawagi wrote: > > [...] > > > > >>>> fsl_mc_bus_probe(struct platform_device *pdev) > > > >>>>>> struct fsl_mc_io *mc_io = NULL; > > > >>>>>> int container_id; > > > >>>>>> phys_addr_t mc_portal_phys_addr; > > > >>>>>> - u32 mc_portal_size; > > > >>>>>> - struct resource res; > > > >>>>>> + u32 mc_portal_size, mc_stream_id; > > > >>>>>> + struct resource *plat_res; > > > >>>>>> + > > > >>>>>> + if (!iommu_present(&fsl_mc_bus_type)) > > > >>>>>> + return -EPROBE_DEFER; > > > >>>>>> > > > >>>>>> mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); > > > >>>>>> if (!mc) > > > >>>>>> @@ -874,19 +887,33 @@ static int fsl_mc_bus_probe(struct > > > >>>>>> platform_device *pdev) > > > >>>>>> > > > >>>>>> platform_set_drvdata(pdev, mc); > > > >>>>>> > > > >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > > >>>>>> + mc->fsl_mc_regs = devm_ioremap_resource(&pdev->dev, > plat_res); > > > >>>>>> + if (IS_ERR(mc->fsl_mc_regs)) > > > >>>>>> + return PTR_ERR(mc->fsl_mc_regs); > > > >>>>>> + > > > >>>>>> + if (IS_ENABLED(CONFIG_ACPI) && !dev_of_node(&pdev->dev)) { > > > >>>>>> + mc_stream_id = readl(mc->fsl_mc_regs + FSL_MC_FAPR); > > > >>>>>> + /* > > > >>>>>> + * HW ORs the PL and BMT bit, places the result in bit 15 of > > > >>>>>> + * the StreamID and ORs in the ICID. Calculate it accordingly. > > > >>>>>> + */ > > > >>>>>> + mc_stream_id = (mc_stream_id & 0xffff) | > > > >>>>>> + ((mc_stream_id & (MC_FAPR_PL | MC_FAPR_BMT)) ? > > > >>>>>> + 0x4000 : 0); > > > >>>>>> + error = acpi_dma_configure_id(&pdev->dev, > > > DEV_DMA_COHERENT, > > > >>>>>> + &mc_stream_id); > > > >>>>>> + if (error) > > > >>>>>> + dev_warn(&pdev->dev, "failed to configure dma: %d.\n", > > > >>>>>> + error); > > > >>>>>> + } > > > >>>>>> + > > > >>>>>> /* > > > >>>>>> * Get physical address of MC portal for the root DPRC: > > > >>>>>> */ > > > >>>>>> - error = of_address_to_resource(pdev->dev.of_node, 0, &res); > > > >>>>>> - if (error < 0) { > > > >>>>>> - dev_err(&pdev->dev, > > > >>>>>> - "of_address_to_resource() failed for %pOF\n", > > > >>>>>> - pdev->dev.of_node); > > > >>>>>> - return error; > > > >>>>>> - } > > > >>>>>> - > > > >>>>>> - mc_portal_phys_addr = res.start; > > > >>>>>> - mc_portal_size = resource_size(&res); > > > >>>>>> + plat_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > >>>>>> + mc_portal_phys_addr = plat_res->start; > > > >>>>>> + mc_portal_size = resource_size(plat_res); > > > >>>>>> error = fsl_create_mc_io(&pdev->dev, mc_portal_phys_addr, > > > >>>>>> mc_portal_size, NULL, > > > >>>>>> > > > >>>>>> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL, &mc_io); @@ -903,11 > +930,13 > > > >>>>>> @@ static int fsl_mc_bus_probe(struct > > > >>>> platform_device *pdev) > > > >>>>>> dev_info(&pdev->dev, "MC firmware version: %u.%u.%u\n", > > > >>>>>> mc_version.major, mc_version.minor, > > > >>>>>> mc_version.revision); > > > >>>>>> > > > >>>>>> - error = get_mc_addr_translation_ranges(&pdev->dev, > > > >>>>>> - &mc->translation_ranges, > > > >>>>>> - &mc->num_translation_ranges); > > > >>>>>> - if (error < 0) > > > >>>>>> - goto error_cleanup_mc_io; > > > >>>>>> + if (dev_of_node(&pdev->dev)) { > > > >>>>>> + error = get_mc_addr_translation_ranges(&pdev->dev, > > > >>>>>> + &mc->translation_ranges, > > > >>>>>> + &mc->num_translation_ranges); > > > >>>>>> + if (error < 0) > > > >>>>>> + goto error_cleanup_mc_io; > > > >>>>>> + } > > > >>>>>> > > > >>>>>> error = dprc_get_container_id(mc_io, 0, &container_id); > > > >>>>>> if (error < 0) { > > > >>>>>> @@ -934,6 +963,7 @@ static int fsl_mc_bus_probe(struct > > > >>>>>> platform_device > > > >>>> *pdev) > > > >>>>>> goto error_cleanup_mc_io; > > > >>>>>> > > > >>>>>> mc->root_mc_bus_dev = mc_bus_dev; > > > >>>>>> + mc_bus_dev->dev.fwnode = pdev->dev.fwnode; > > > >>>>> Makarand, this looks a bit weird. Is there really a reason for it? > > > >>>> Can you clarify please so that we can reach a conclusion on this matter ? > > > >>>> > > > >>> Laurentiu, can you clarify what exactly is the doubt here? Are > > > >>> you asking about > > > >> fwnode assignment from pdev to mc_bus_dev? > > > >> Yes. I remember that a while ago I tested without this fwnode > > > >> assignment and didn't encounter any issues. Maybe we can just drop it? > > > > Did you tested with PHY changes? Because this is needed for MAC > > > > driver, > > > where it needs the mc bus node. > > > > > > Maybe it worth a comment or maybe have it in a different patch? > > > > > Since this change is needed for ACPI case and this is ACPI support > > case, I feel we should have this change in this patch only instead of > > separate patch. > > Anyway - you need to seek feedback from Marc on whether patches > 11 and 12 are OK from an irqchip perspective, it is possible we can take the rest > of the series independently if everyone agrees but I don't necessarily see a > reason for that. > > Long story short: you need Marc's ACK on [11-12], it is your code. > Hi Marc, can you please review/ack this patch? > Thanks, > Lorenzo
On 2020-07-16 04:23, Makarand Pawagi wrote: >> -----Original Message----- >> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> [...] >> Anyway - you need to seek feedback from Marc on whether patches >> 11 and 12 are OK from an irqchip perspective, it is possible we can >> take the rest >> of the series independently if everyone agrees but I don't necessarily >> see a >> reason for that. >> >> Long story short: you need Marc's ACK on [11-12], it is your code. >> > Hi Marc, can you please review/ack this patch? https://lore.kernel.org/linux-acpi/bd07f44dad1d029e0d023202cbf5fc94@kernel.org/ M.
On Fri, Jun 19, 2020 at 09:20:01AM +0100, Lorenzo Pieralisi wrote: > This series is a v2 of a previous posting: > > v1 -> v2 > > - Removed _rid() wrappers > - Fixed !CONFIG_ACPI compilation issue > - Converted of_pci_iommu_init() to use of_iommu_configure_dev_id() > > v1: https://lore.kernel.org/linux-arm-kernel/20200521130008.8266-1-lorenzo.pieralisi@arm.com/ > > Original cover letter > --------------------- > > Firmware bindings provided in the ACPI IORT table[1] and device tree > bindings define rules to carry out input/output ID mappings - ie > retrieving an IOMMU/MSI controller input ID for a device with a given > ID. > > At the moment these firmware bindings are used exclusively for PCI > devices and their requester ID to IOMMU/MSI id mapping but there is > nothing PCI specific in the ACPI and devicetree bindings that prevent > the firmware and kernel from using the firmware bindings to traslate > device IDs for any bus that requires its devices to carry out > input/output id translations. > > The Freescale FSL bus is an example whereby the input/output ID > translation kernel code put in place for PCI can be reused for devices > attached to the bus that are not PCI devices. > > This series updates the kernel code to make the MSI/IOMMU input/output > ID translation PCI agnostic and apply the resulting changes to the > device ID space provided by the Freescale FSL bus. > > [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: "Joerg Roedel <joro@8bytes.org> > Cc: Hanjun Guo <guohanjun@huawei.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Marc Zyngier <maz@kernel.org> > > Diana Craciun (2): > of/irq: make of_msi_map_get_device_domain() bus agnostic > bus/fsl-mc: Refactor the MSI domain creation in the DPRC driver > > Laurentiu Tudor (1): > dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus > > Lorenzo Pieralisi (8): > ACPI/IORT: Make iort_match_node_callback walk the ACPI namespace for > NC > ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic > ACPI/IORT: Make iort_msi_map_rid() PCI agnostic > ACPI/IORT: Remove useless PCI bus walk > ACPI/IORT: Add an input ID to acpi_dma_configure() > of/iommu: Make of_map_rid() PCI agnostic > of/device: Add input id to of_dma_configure() > of/irq: Make of_msi_map_rid() PCI bus agnostic > > Makarand Pawagi (1): > bus: fsl-mc: Add ACPI support for fsl-mc > > .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 50 +++++++- > drivers/acpi/arm64/iort.c | 108 ++++++++++++------ > drivers/acpi/scan.c | 8 +- > drivers/bus/fsl-mc/dprc-driver.c | 31 ++--- > drivers/bus/fsl-mc/fsl-mc-bus.c | 79 +++++++++---- > drivers/bus/fsl-mc/fsl-mc-msi.c | 36 ++++-- > drivers/bus/fsl-mc/fsl-mc-private.h | 6 +- > drivers/iommu/of_iommu.c | 81 +++++++------ > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 105 ++++++++++++++--- > drivers/of/base.c | 42 +++---- > drivers/of/device.c | 8 +- > drivers/of/irq.c | 34 +++--- > drivers/pci/msi.c | 9 +- > include/acpi/acpi_bus.h | 9 +- > include/linux/acpi.h | 7 ++ > include/linux/acpi_iort.h | 20 ++-- > include/linux/of.h | 4 +- > include/linux/of_device.h | 16 ++- > include/linux/of_iommu.h | 6 +- > include/linux/of_irq.h | 13 ++- > 20 files changed, 451 insertions(+), 221 deletions(-) Hi guys, I think this series is ready for upstream (there are two ACKs missing from Rafael on patch (5) and Bjorn on patch (3) - I asked for them), it touches lots of subsystems so I am not really sure what's the best way to pull it, more so given that it is also late in the cycle (I do think it is best to merge it via a single tree, it does not make sense to split it up in my opinion). Please let me know. Thanks, Lorenzo
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Monday, July 20, 2020 10:25 PM > To: linux-arm-kernel@lists.infradead.org; Rob Herring <robh+dt@kernel.org>; > Rafael J. Wysocki <rjw@rjwysocki.net>; Bjorn Helgaas <bhelgaas@google.com>; > Catalin Marinas <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; > joro@8bytes.org > Cc: Hanjun Guo <guohanjun@huawei.com>; Sudeep Holla > <sudeep.holla@arm.com>; Robin Murphy <robin.murphy@arm.com>; Marc > Zyngier <maz@kernel.org>; iommu@lists.linux-foundation.org; linux- > acpi@vger.kernel.org; devicetree@vger.kernel.org; linux-pci@vger.kernel.org; > Makarand Pawagi <makarand.pawagi@nxp.com>; Diana Madalina Craciun (OSS) > <diana.craciun@oss.nxp.com>; Laurentiu Tudor <laurentiu.tudor@nxp.com> > Subject: [EXT] Re: [PATCH v2 00/12] ACPI/OF: Upgrade MSI/IOMMU ID mapping > APIs > > Caution: EXT Email > > On Fri, Jun 19, 2020 at 09:20:01AM +0100, Lorenzo Pieralisi wrote: > > This series is a v2 of a previous posting: > > > > v1 -> v2 > > > > - Removed _rid() wrappers > > - Fixed !CONFIG_ACPI compilation issue > > - Converted of_pci_iommu_init() to use of_iommu_configure_dev_id() > > > > v1: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .kernel.org%2Flinux-arm-kernel%2F20200521130008.8266-1-lorenzo.pierali > > > si%40arm.com%2F&data=02%7C01%7Cmakarand.pawagi%40nxp.com%7C > da7bade > > > c592846a1478808d82ccd9eb1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0% > 7C1%7 > > > C637308608924853281&sdata=ZoP3e2Q5Ijkz%2FtsGfgu9MYkmKXSHQExs5 > J64Sb > > h51YA%3D&reserved=0 > > > > Original cover letter > > --------------------- > > > > Firmware bindings provided in the ACPI IORT table[1] and device tree > > bindings define rules to carry out input/output ID mappings - ie > > retrieving an IOMMU/MSI controller input ID for a device with a given > > ID. > > > > At the moment these firmware bindings are used exclusively for PCI > > devices and their requester ID to IOMMU/MSI id mapping but there is > > nothing PCI specific in the ACPI and devicetree bindings that prevent > > the firmware and kernel from using the firmware bindings to traslate > > device IDs for any bus that requires its devices to carry out > > input/output id translations. > > > > The Freescale FSL bus is an example whereby the input/output ID > > translation kernel code put in place for PCI can be reused for devices > > attached to the bus that are not PCI devices. > > > > This series updates the kernel code to make the MSI/IOMMU input/output > > ID translation PCI agnostic and apply the resulting changes to the > > device ID space provided by the Freescale FSL bus. > > > > [1] > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfoc > > > enter.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.den0049d%2FDEN0049D_IO_ > Rema > > > pping_Table.pdf&data=02%7C01%7Cmakarand.pawagi%40nxp.com%7Cda > 7bade > > > c592846a1478808d82ccd9eb1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0% > 7C1%7 > > > C637308608924853281&sdata=RzpIo4AfAFsi2pdEuY%2FbnPAyase5cSmFIr5 > 2SX > > aOrTg%3D&reserved=0 > > > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > Cc: "Joerg Roedel <joro@8bytes.org> > > Cc: Hanjun Guo <guohanjun@huawei.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Marc Zyngier <maz@kernel.org> > > > > Diana Craciun (2): > > of/irq: make of_msi_map_get_device_domain() bus agnostic > > bus/fsl-mc: Refactor the MSI domain creation in the DPRC driver > > > > Laurentiu Tudor (1): > > dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc > > bus > > > > Lorenzo Pieralisi (8): > > ACPI/IORT: Make iort_match_node_callback walk the ACPI namespace for > > NC > > ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic > > ACPI/IORT: Make iort_msi_map_rid() PCI agnostic > > ACPI/IORT: Remove useless PCI bus walk > > ACPI/IORT: Add an input ID to acpi_dma_configure() > > of/iommu: Make of_map_rid() PCI agnostic > > of/device: Add input id to of_dma_configure() > > of/irq: Make of_msi_map_rid() PCI bus agnostic > > > > Makarand Pawagi (1): > > bus: fsl-mc: Add ACPI support for fsl-mc > > > > .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 50 +++++++- > > drivers/acpi/arm64/iort.c | 108 ++++++++++++------ > > drivers/acpi/scan.c | 8 +- > > drivers/bus/fsl-mc/dprc-driver.c | 31 ++--- > > drivers/bus/fsl-mc/fsl-mc-bus.c | 79 +++++++++---- > > drivers/bus/fsl-mc/fsl-mc-msi.c | 36 ++++-- > > drivers/bus/fsl-mc/fsl-mc-private.h | 6 +- > > drivers/iommu/of_iommu.c | 81 +++++++------ > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 105 ++++++++++++++--- > > drivers/of/base.c | 42 +++---- > > drivers/of/device.c | 8 +- > > drivers/of/irq.c | 34 +++--- > > drivers/pci/msi.c | 9 +- > > include/acpi/acpi_bus.h | 9 +- > > include/linux/acpi.h | 7 ++ > > include/linux/acpi_iort.h | 20 ++-- > > include/linux/of.h | 4 +- > > include/linux/of_device.h | 16 ++- > > include/linux/of_iommu.h | 6 +- > > include/linux/of_irq.h | 13 ++- > > 20 files changed, 451 insertions(+), 221 deletions(-) > > Hi guys, > > I think this series is ready for upstream (there are two ACKs missing from Rafael > on patch (5) and Bjorn on patch (3) - I asked for them), it touches lots of > subsystems so I am not really sure what's the best way to pull it, more so given > that it is also late in the cycle (I do think it is best to merge it via a single tree, it > does not make sense to split it up in my opinion). > > Please let me know. > Hi Lorenzo, I too find it suitable to merge it as a whole. Hi Rafael, Bjorn, Can you finalize your review for patch-5 and patch-3? > Thanks, > Lorenzo
On Wed, Jul 15, 2020 at 10:13:26AM +0100, Lorenzo Pieralisi wrote: > On Thu, Jul 09, 2020 at 10:35:14AM +0100, Lorenzo Pieralisi wrote: > > On Fri, Jun 19, 2020 at 09:20:06AM +0100, Lorenzo Pieralisi wrote: > > > Some HW devices are created as child devices of proprietary busses, > > > that have a bus specific policy defining how the child devices > > > wires representing the devices ID are translated into IOMMU and > > > IRQ controllers device IDs. > > > > > > Current IORT code provides translations for: > > > > > > - PCI devices, where the device ID is well identified at bus level > > > as the requester ID (RID) > > > - Platform devices that are endpoint devices where the device ID is > > > retrieved from the ACPI object IORT mappings (Named components single > > > mappings). A platform device is represented in IORT as a named > > > component node > > > > > > For devices that are child devices of proprietary busses the IORT > > > firmware represents the bus node as a named component node in IORT > > > and it is up to that named component node to define in/out bus > > > specific ID translations for the bus child devices that are > > > allocated and created in a bus specific manner. > > > > > > In order to make IORT ID translations available for proprietary > > > bus child devices, the current ACPI (and IORT) code must be > > > augmented to provide an additional ID parameter to acpi_dma_configure() > > > representing the child devices input ID. This ID is bus specific > > > and it is retrieved in bus specific code. > > > > > > By adding an ID parameter to acpi_dma_configure(), the IORT > > > code can map the child device ID to an IOMMU stream ID through > > > the IORT named component representing the bus in/out ID mappings. > > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > Cc: Will Deacon <will@kernel.org> > > > Cc: Hanjun Guo <guohanjun@huawei.com> > > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > Cc: Robin Murphy <robin.murphy@arm.com> > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > --- > > > drivers/acpi/arm64/iort.c | 59 +++++++++++++++++++++++++++++---------- > > > drivers/acpi/scan.c | 8 ++++-- > > > include/acpi/acpi_bus.h | 9 ++++-- > > > include/linux/acpi.h | 7 +++++ > > > include/linux/acpi_iort.h | 7 +++-- > > > 5 files changed, 67 insertions(+), 23 deletions(-) > > > > Hi Rafael, > > > > just to ask if the ACPI core changes in this patch are OK with you, > > thank you very much. > > Hi Rafael, > > are you OK with ACPI core changes in this patch ? > > Please let me know, thanks. Hi Rafael, gentle ping, I think we are missing v5.9, we would need your feedback on this please. Thanks, Lorenzo
On Tue, Jul 28, 2020 at 2:48 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Wed, Jul 15, 2020 at 10:13:26AM +0100, Lorenzo Pieralisi wrote: > > On Thu, Jul 09, 2020 at 10:35:14AM +0100, Lorenzo Pieralisi wrote: > > > On Fri, Jun 19, 2020 at 09:20:06AM +0100, Lorenzo Pieralisi wrote: > > > > Some HW devices are created as child devices of proprietary busses, > > > > that have a bus specific policy defining how the child devices > > > > wires representing the devices ID are translated into IOMMU and > > > > IRQ controllers device IDs. > > > > > > > > Current IORT code provides translations for: > > > > > > > > - PCI devices, where the device ID is well identified at bus level > > > > as the requester ID (RID) > > > > - Platform devices that are endpoint devices where the device ID is > > > > retrieved from the ACPI object IORT mappings (Named components single > > > > mappings). A platform device is represented in IORT as a named > > > > component node > > > > > > > > For devices that are child devices of proprietary busses the IORT > > > > firmware represents the bus node as a named component node in IORT > > > > and it is up to that named component node to define in/out bus > > > > specific ID translations for the bus child devices that are > > > > allocated and created in a bus specific manner. > > > > > > > > In order to make IORT ID translations available for proprietary > > > > bus child devices, the current ACPI (and IORT) code must be > > > > augmented to provide an additional ID parameter to acpi_dma_configure() > > > > representing the child devices input ID. This ID is bus specific > > > > and it is retrieved in bus specific code. > > > > > > > > By adding an ID parameter to acpi_dma_configure(), the IORT > > > > code can map the child device ID to an IOMMU stream ID through > > > > the IORT named component representing the bus in/out ID mappings. > > > > > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > Cc: Will Deacon <will@kernel.org> > > > > Cc: Hanjun Guo <guohanjun@huawei.com> > > > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > > Cc: Robin Murphy <robin.murphy@arm.com> > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > > > --- > > > > drivers/acpi/arm64/iort.c | 59 +++++++++++++++++++++++++++++---------- > > > > drivers/acpi/scan.c | 8 ++++-- > > > > include/acpi/acpi_bus.h | 9 ++++-- > > > > include/linux/acpi.h | 7 +++++ > > > > include/linux/acpi_iort.h | 7 +++-- > > > > 5 files changed, 67 insertions(+), 23 deletions(-) > > > > > > Hi Rafael, > > > > > > just to ask if the ACPI core changes in this patch are OK with you, > > > thank you very much. Sorry for the delay, I was offline last week. > > Hi Rafael, > > > > are you OK with ACPI core changes in this patch ? Yes, I am. Please feel free to route it through whatever tree you think would be appropriate. Thanks!
On Fri, 19 Jun 2020 09:20:01 +0100, Lorenzo Pieralisi wrote: > This series is a v2 of a previous posting: > > v1 -> v2 > > - Removed _rid() wrappers > - Fixed !CONFIG_ACPI compilation issue > - Converted of_pci_iommu_init() to use of_iommu_configure_dev_id() > > [...] Applied to arm64 (for-next/msi-iommu), thanks! [01/12] ACPI/IORT: Make iort_match_node_callback walk the ACPI namespace for NC https://git.kernel.org/arm64/c/07d2e59f27cd [02/12] ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic https://git.kernel.org/arm64/c/d1718a1b7a86 [03/12] ACPI/IORT: Make iort_msi_map_rid() PCI agnostic https://git.kernel.org/arm64/c/39c3cf566cea [04/12] ACPI/IORT: Remove useless PCI bus walk https://git.kernel.org/arm64/c/3a3d208beede [05/12] ACPI/IORT: Add an input ID to acpi_dma_configure() https://git.kernel.org/arm64/c/b8e069a2a8da [06/12] of/iommu: Make of_map_rid() PCI agnostic https://git.kernel.org/arm64/c/746a71d02b5d [07/12] of/device: Add input id to of_dma_configure() https://git.kernel.org/arm64/c/a081bd4af4ce [08/12] dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus https://git.kernel.org/arm64/c/5bda70c6162d [09/12] of/irq: make of_msi_map_get_device_domain() bus agnostic https://git.kernel.org/arm64/c/6f881aba0110 [10/12] of/irq: Make of_msi_map_rid() PCI bus agnostic https://git.kernel.org/arm64/c/2bcdd8f2c07f [11/12] bus/fsl-mc: Refactor the MSI domain creation in the DPRC driver https://git.kernel.org/arm64/c/998fb7badf03 [12/12] bus: fsl-mc: Add ACPI support for fsl-mc https://git.kernel.org/arm64/c/6305166c8771
On 2020/7/2 16:22, Hanjun Guo wrote: > > As I said in previous email, I'm not against this patch, and seems > have no regressions for platforms that using named component node > such as D05/D06 (I will test it shortly to make sure), but it's better > to update the wording of the spec (even after this patch set is merged). I can see that IORT revision E was updated to add IMP_DEF input IDs in ID mappings for Named Component nodes, thanks for that. Thanks Hanjun
Firmware bindings provided in the ACPI IORT table[1] and device tree bindings define rules to carry out input/output ID mappings - ie retrieving an IOMMU/MSI controller input ID for a device with a given ID. At the moment these firmware bindings are used exclusively for PCI devices and their requester ID to IOMMU/MSI id mapping but there is nothing PCI specific in the ACPI and devicetree bindings that prevent the firmware and kernel from using the firmware bindings to traslate device IDs for any bus that requires its devices to carry out input/output id translations. The Freescale FSL bus is an example whereby the input/output ID translation kernel code put in place for PCI can be reused for devices attached to the bus that are not PCI devices. This series updates the kernel code to make the MSI/IOMMU input/output ID translation PCI agnostic and apply the resulting changes to the device ID space provided by the Freescale FSL bus. [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0049d/DEN0049D_IO_Remapping_Table.pdf Cc: Rob Herring <robh+dt@kernel.org> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: "Joerg Roedel <joro@8bytes.org> Cc: Hanjun Guo <guohanjun@huawei.com> Cc: Sudeep Holla <sudeep.holla@arm.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Marc Zyngier <maz@kernel.org> Diana Craciun (3): of/irq: make of_msi_map_get_device_domain() bus agnostic bus/fsl-mc: Refactor the MSI domain creation in the DPRC driver bus: fsl-mc: Add ACPI support for fsl-mc Laurentiu Tudor (1): dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus Lorenzo Pieralisi (8): ACPI/IORT: Make iort_match_node_callback walk the ACPI namespace for NC ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic ACPI/IORT: Make iort_msi_map_rid() PCI agnostic ACPI/IORT: Remove useless PCI bus walk ACPI/IORT: Add an input ID to acpi_dma_configure() of/iommu: Make of_map_rid() PCI agnostic of/device: Add input id to of_dma_configure() of/irq: Make of_msi_map_rid() PCI bus agnostic .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 ++++- drivers/acpi/arm64/iort.c | 108 ++++++++++++------ drivers/acpi/scan.c | 8 +- drivers/bus/fsl-mc/dprc-driver.c | 31 ++--- drivers/bus/fsl-mc/fsl-mc-bus.c | 79 +++++++++---- drivers/bus/fsl-mc/fsl-mc-msi.c | 36 ++++-- drivers/bus/fsl-mc/fsl-mc-private.h | 6 +- drivers/iommu/of_iommu.c | 53 ++++++--- drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c | 88 +++++++++++++- drivers/of/base.c | 42 +++---- drivers/of/device.c | 8 +- drivers/of/irq.c | 34 +++--- drivers/pci/msi.c | 5 +- include/acpi/acpi_bus.h | 9 +- include/linux/acpi.h | 7 ++ include/linux/acpi_iort.h | 26 +++-- include/linux/of.h | 17 ++- include/linux/of_device.h | 16 ++- include/linux/of_iommu.h | 6 +- include/linux/of_irq.h | 19 ++- 20 files changed, 451 insertions(+), 177 deletions(-)