Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Tue, 2016-09-13 at 07:23 +, Y.B. Lu wrote:
> > 


> > 
> > -Original Message-
> > From: linux-mmc-ow...@vger.kernel.org [mailto:linux-mmc-
> > ow...@vger.kernel.org] On Behalf Of Scott Wood
> > Sent: Tuesday, September 13, 2016 7:25 AM
> > To: Y.B. Lu; linux-...@vger.kernel.org; ulf.hans...@linaro.org; Arnd
> > Bergmann
> > Cc: linuxppc-...@lists.ozlabs.org; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-ker...@vger.kernel.org; linux-
> > c...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-
> > foundation.org; net...@vger.kernel.org; Mark Rutland; Rob Herring;
> > Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> > Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> > Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> > 
> > BTW, aren't ls2080a and ls2085a the same die?  And is there no non-E
> > version of LS2080A/LS2040A?
> [Lu Yangbo-B47093] I checked all the svr values in chip errata doc "Revision
> level to part marking cross-reference" table.
> I found ls2080a and ls2085a were in two separate doc. And I didn’t find non-
> E version of LS2080A/LS2040A in chip errata doc.
> Do you know is there any other doc we can confirm this?

No.  Traditionally we've always had E and non-E versions of each chip, but I
have no knowledge of whether that has changed (I do note that the way that E-
status is indicated in SVR has changed).

But please label LS2080A and LS2085A as the same die (or provide strong
evidence that they are not).

> 
> > 
> > 
> > > 
> > > > > 
> > > > > + do {
> > > > > + if (!matches->soc_id)
> > > > > + return NULL;
> > > > > + if (glob_match(svr_match, matches->soc_id))
> > > > > + break;
> > > > > + } while (matches++);
> > > > Are you expecting "matches++" to ever evaluate as false?
> > > [Lu Yangbo-B47093] Yes, this is used to match the soc we use in
> > > qoriq_soc array until getting true.
> > > We need to get the name and die information defined in array.
> > I'm not asking whether the glob_match will ever return true.  I'm saying
> > that "matches++" will never become NULL.
> [Lu Yangbo-B47093] The matches++ will never become NULL while it will return
> NULL after matching for all the members in array.

"matches++" will never "return NULL".  It's just an incrementing address.  It
won't be null until you wrap around the address space, and even if the other
loop terminators never kicked in you'd crash long before that happens.

Please rewrite the loop as something like:

while (matches->soc_id) {
if (glob_match(...))
return matches;

matches++;
}

return NULL;


> > > > > + /* Register soc device */
> > > > > + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > > > + if (!soc_dev_attr) {
> > > > > + ret = -ENOMEM;
> > > > > + goto out_unmap;
> > > > > + }
> > > > Couldn't this be statically allocated?
> > > [Lu Yangbo-B47093] Do you mean we define this struct statically ?
> > > 
> > > static struct soc_device_attribute soc_dev_attr;
> > Yes.
> > 
> [Lu Yangbo-B47093] It's ok to define it statically. Is there any need to do
> that?

It's simpler.

-Scott

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 09/14] drivers: iommu: arm-smmu-v3: add IORT configuration

[Robin Murphy]

On 09/09/16 15:23, Lorenzo Pieralisi wrote:
> In ACPI bases systems, in order to be able to create platform
> devices and initialize them for ARM SMMU v3 components, the IORT
> kernel implementation requires a set of static functions to be
> used by the IORT kernel layer to configure platform devices for
> ARM SMMU v3 components.
> 
> Add static configuration functions to the IORT kernel layer for
> the ARM SMMU v3 components, so that the ARM SMMU v3 driver can
> initialize its respective platform device by relying on the IORT
> kernel infrastructure and by adding a corresponding ACPI device
> early probe section entry.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Will Deacon 
> Cc: Robin Murphy 
> Cc: Joerg Roedel 
> ---
>  drivers/acpi/arm64/iort.c   | 103 
> +++-
>  drivers/iommu/arm-smmu-v3.c |  95 +++-
>  2 files changed, 195 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index e0a9b16..a2ad102 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -425,6 +425,95 @@ struct irq_domain *iort_get_device_domain(struct device 
> *dev, u32 req_id)
>   return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
>  }
>  
> +static void __init acpi_iort_register_irq(int hwirq, const char *name,
> +   int trigger,
> +   struct resource *res)
> +{
> + int irq = acpi_register_gsi(NULL, hwirq, trigger,
> + ACPI_ACTIVE_HIGH);
> +
> + if (irq < 0) {

irq <= 0 ?

> + pr_err("could not register gsi hwirq %d name [%s]\n", hwirq,
> +   name);
> + return;
> + }
> +
> + res->start = irq;
> + res->end = irq;
> + res->flags = IORESOURCE_IRQ;
> + res->name = name;
> +}
> +
> +static int __init arm_smmu_v3_count_resources(struct acpi_iort_node *node)
> +{
> + struct acpi_iort_smmu_v3 *smmu;
> + /* Always present mem resource */
> + int num_res = 1;
> +
> + /* Retrieve SMMUv3 specific data */
> + smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> +
> + if (smmu->event_gsiv)
> + num_res++;
> +
> + if (smmu->pri_gsiv)
> + num_res++;
> +
> + if (smmu->gerr_gsiv)
> + num_res++;
> +
> + if (smmu->sync_gsiv)
> + num_res++;
> +
> + return num_res;
> +}
> +
> +static void __init arm_smmu_v3_init_resources(struct resource *res,
> +   struct acpi_iort_node *node)
> +{
> + struct acpi_iort_smmu_v3 *smmu;
> + int num_res = 0;
> +
> + /* Retrieve SMMUv3 specific data */
> + smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> +
> + res[num_res].start = smmu->base_address;
> + res[num_res].end = smmu->base_address + SZ_128K - 1;
> + res[num_res].flags = IORESOURCE_MEM;
> +
> + num_res++;
> +
> + if (smmu->event_gsiv)
> + acpi_iort_register_irq(smmu->event_gsiv, "eventq",
> +ACPI_EDGE_SENSITIVE,
> +[num_res++]);
> +
> + if (smmu->pri_gsiv)
> + acpi_iort_register_irq(smmu->pri_gsiv, "priq",
> +ACPI_EDGE_SENSITIVE,
> +[num_res++]);
> +
> + if (smmu->gerr_gsiv)
> + acpi_iort_register_irq(smmu->gerr_gsiv, "gerror",
> +ACPI_EDGE_SENSITIVE,
> +[num_res++]);
> +
> + if (smmu->sync_gsiv)
> + acpi_iort_register_irq(smmu->sync_gsiv, "cmdq-sync",
> +ACPI_EDGE_SENSITIVE,
> +[num_res++]);
> +}
> +
> +static bool __init arm_smmu_v3_is_coherent(struct acpi_iort_node *node)
> +{
> + struct acpi_iort_smmu_v3 *smmu;
> +
> + /* Retrieve SMMUv3 specific data */
> + smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> +
> + return smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
> +}
> +
>  struct iort_iommu_config {
>   const char *name;
>   int (*iommu_init)(struct acpi_iort_node *node);
> @@ -434,10 +523,22 @@ struct iort_iommu_config {
>struct acpi_iort_node *node);
>  };
>  
> +static const struct iort_iommu_config iort_arm_smmu_v3_cfg __initconst = {
> + .name = "arm-smmu-v3",
> + .iommu_is_coherent = arm_smmu_v3_is_coherent,
> + .iommu_count_resources = arm_smmu_v3_count_resources,
> + .iommu_init_resources = arm_smmu_v3_init_resources
> +};
> +
>  static __init
>  const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node 
> *node)
>  {
> - return NULL;
> + switch (node->type) {
> + 

[PATCH v2] iommu/io-pgtable-arm: Check for v7s-incapable systems

[Robin Murphy]

On machines with no 32-bit addressable RAM whatsoever, we shouldn't
even touch the v7s format as it's never going to work.

Fixes: e5fc9753b1a8 ("iommu/io-pgtable: Add ARMv7 short descriptor support")
Reported-by: Eric Auger 
Tested-by: Eric Auger 
Signed-off-by: Robin Murphy 
---

Apparently, in my haste, I overlooked that not everything that can do
COMPILE_TEST also has a PHYS_OFFSET.

 drivers/iommu/io-pgtable-arm-v7s.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c 
b/drivers/iommu/io-pgtable-arm-v7s.c
index def8ca1c982d..f50e51c1a9c8 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -633,6 +633,10 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct 
io_pgtable_cfg *cfg,
 {
struct arm_v7s_io_pgtable *data;
 
+#ifdef PHYS_OFFSET
+   if (upper_32_bits(PHYS_OFFSET))
+   return NULL;
+#endif
if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
return NULL;
 
-- 
2.8.1.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 07/14] drivers: acpi: iort: add support for ARM SMMU platform devices creation

[Lorenzo Pieralisi]

On Tue, Sep 13, 2016 at 04:25:55PM +0100, Robin Murphy wrote:

[...]

> > +/**
> > + * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
> > + * @fwnode: IORT node associated fwnode handle
> > + * @node: Pointer to SMMU ACPI IORT node
> > + *
> > + * Returns: 0 on success, <0 failure
> > + */
> > +static int __init iort_add_smmu_platform_device(struct fwnode_handle 
> > *fwnode,
> > +   struct acpi_iort_node *node)
> > +{
> > +   struct platform_device *pdev;
> > +   struct resource *r;
> > +   enum dev_dma_attr attr;
> > +   int ret, count;
> > +   const struct iort_iommu_config *ops = iort_get_iommu_cfg(node);
> > +
> > +   if (!ops)
> > +   return -ENODEV;
> > +
> > +   pdev = platform_device_alloc(ops->name, PLATFORM_DEVID_AUTO);
> > +   if (!pdev)
> > +   return PTR_ERR(pdev);
> > +
> > +   count = ops->iommu_count_resources(node);
> > +
> > +   r = kcalloc(count, sizeof(*r), GFP_KERNEL);
> > +   if (!r) {
> > +   ret = -ENOMEM;
> > +   goto dev_put;
> > +   }
> > +
> > +   ops->iommu_init_resources(r, node);
> > +
> > +   ret = platform_device_add_resources(pdev, r, count);
> > +   /*
> > +* Resources are duplicated in platform_device_add_resources,
> > +* free their allocated memory
> > +*/
> > +   kfree(r);
> > +
> > +   if (ret)
> > +   goto dev_put;
> > +
> > +   /*
> > +* Add a copy of IORT node pointer to platform_data to
> > +* be used to retrieve IORT data information.
> > +*/
> > +   ret = platform_device_add_data(pdev, , sizeof(node));
> > +   if (ret)
> > +   goto dev_put;
> > +
> > +   pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> > +   if (!pdev->dev.dma_mask) {
> > +   ret = -ENOMEM;
> > +   goto dev_put;
> > +   }
> 
> Since this is exclusively for creating SMMUs, and we know they should
> never have weird shenanigans going on requiring different masks, I'd be
> inclined to just point dev.dma_mask at dev.coherent_dma_mask and be done
> with it.

That sounds reasonable yes, I will do.

> > +
> > +   pdev->dev.fwnode = fwnode;
> > +
> > +   /*
> > +* Set default dma mask value for the table walker,
> > +* to be overridden on probing with correct value.
> > +*/
> > +   *pdev->dev.dma_mask = DMA_BIT_MASK(32);
> > +   pdev->dev.coherent_dma_mask = *pdev->dev.dma_mask;
> > +
> > +   attr = ops->iommu_is_coherent(node) ?
> > +DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
> > +
> > +   /* Configure DMA for the page table walker */
> > +   acpi_dma_configure(>dev, attr);
> 
> Oh look, some more code which would be simpler if acpi_dma_configure()
> set the default mask itself ;)

Eheh I have no objections, apart from the effect that change can
have on non-ARM probing paths, it could also help remove some code
from ACPI core while at it.

Thanks !
Lorenzo

> 
> Robin.
> 
> > +
> > +   ret = platform_device_add(pdev);
> > +   if (ret)
> > +   goto dma_deconfigure;
> > +
> > +   return 0;
> > +
> > +dma_deconfigure:
> > +   acpi_dma_deconfigure(>dev);
> > +   kfree(pdev->dev.dma_mask);
> > +
> > +dev_put:
> > +   platform_device_put(pdev);
> > +
> > +   return ret;
> > +}
> > +
> > +static acpi_status __init iort_match_iommu_callback(struct acpi_iort_node 
> > *node,
> > +   void *context)
> > +{
> > +   int ret;
> > +   struct fwnode_handle *fwnode;
> > +
> > +   fwnode = iort_get_fwnode(node);
> > +
> > +   if (!fwnode)
> > +   return AE_NOT_FOUND;
> > +
> > +   ret = iort_add_smmu_platform_device(fwnode, node);
> > +   if (ret) {
> > +   pr_err("Error in platform device creation\n");
> > +   return AE_ERROR;
> > +   }
> > +
> > +   return AE_OK;
> > +}
> > +
> > +static void __init iort_smmu_init(void)
> > +{
> > +   iort_scan_node(ACPI_IORT_NODE_SMMU, iort_match_iommu_callback, NULL);
> > +   iort_scan_node(ACPI_IORT_NODE_SMMU_V3, iort_match_iommu_callback, NULL);
> > +}
> > +
> >  void __init acpi_iort_init(void)
> >  {
> > acpi_status status;
> > @@ -436,4 +566,5 @@ void __init acpi_iort_init(void)
> > }
> >  
> > acpi_probe_device_table(iort);
> > +   iort_smmu_init();
> >  }
> > 
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 06/14] drivers: acpi: implement acpi_dma_configure

[Lorenzo Pieralisi]

On Tue, Sep 13, 2016 at 03:41:06PM +0100, Robin Murphy wrote:
> On 09/09/16 15:23, Lorenzo Pieralisi wrote:
> > On DT based systems, the of_dma_configure() API implements DMA
> > configuration for a given device. On ACPI systems an API equivalent to
> > of_dma_configure() is missing which implies that it is currently not
> > possible to set-up DMA operations for devices through the ACPI generic
> > kernel layer.
> > 
> > This patch fills the gap by introducing acpi_dma_configure/deconfigure()
> > calls that for now are just wrappers around arch_setup_dma_ops() and
> > arch_teardown_dma_ops() and also updates ACPI and PCI core code to use
> > the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions.
> > 
> > The DMA range size passed to arch_setup_dma_ops() is sized according
> > to the device coherent_dma_mask (starting at address 0x0), mirroring the
> > DT probing path behaviour when a dma-ranges property is not provided
> > for the device being probed; this changes the current arch_setup_dma_ops()
> > call parameters in the ACPI probing case, but since arch_setup_dma_ops()
> > is a NOP on all architectures but ARM/ARM64 this patch does not change
> > the current kernel behaviour on them.
> > 
> > Signed-off-by: Lorenzo Pieralisi 
> > Acked-by: Bjorn Helgaas  [pci]
> > Cc: Bjorn Helgaas 
> > Cc: Robin Murphy 
> > Cc: Tomasz Nowicki 
> > Cc: Joerg Roedel 
> > Cc: "Rafael J. Wysocki" 
> > ---
> >  drivers/acpi/glue.c |  4 ++--
> >  drivers/acpi/scan.c | 24 
> >  drivers/pci/probe.c |  3 +--
> >  include/acpi/acpi_bus.h |  2 ++
> >  include/linux/acpi.h|  5 +
> >  5 files changed, 34 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> > index 5ea5dc2..f8d6564 100644
> > --- a/drivers/acpi/glue.c
> > +++ b/drivers/acpi/glue.c
> > @@ -227,8 +227,7 @@ int acpi_bind_one(struct device *dev, struct 
> > acpi_device *acpi_dev)
> >  
> > attr = acpi_get_dma_attr(acpi_dev);
> > if (attr != DEV_DMA_NOT_SUPPORTED)
> > -   arch_setup_dma_ops(dev, 0, 0, NULL,
> > -  attr == DEV_DMA_COHERENT);
> > +   acpi_dma_configure(dev, attr);

Here's the call for non-PCI devices ;-)

> > acpi_physnode_link_name(physical_node_name, node_id);
> > retval = sysfs_create_link(_dev->dev.kobj, >kobj,
> > @@ -251,6 +250,7 @@ int acpi_bind_one(struct device *dev, struct 
> > acpi_device *acpi_dev)
> > return 0;
> >  
> >   err:
> > +   acpi_dma_deconfigure(dev);
> > ACPI_COMPANION_SET(dev, NULL);
> > put_device(dev);
> > put_device(_dev->dev);
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index e878fc7..9614232 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -1370,6 +1370,30 @@ enum dev_dma_attr acpi_get_dma_attr(struct 
> > acpi_device *adev)
> > return DEV_DMA_NON_COHERENT;
> >  }
> >  
> > +/**
> > + * acpi_dma_configure - Set-up DMA configuration for the device.
> > + * @dev: The pointer to the device
> > + * @attr: device dma attributes
> > + */
> > +void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
> > +{
> > +   /*
> > +* Assume dma valid range starts at 0 and covers the whole
> > +* coherent_dma_mask.
> > +*/
> > +   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, NULL,
> > +  attr == DEV_DMA_COHERENT);
> 
> This looks a bit hairy - if we're setting up the DMA configuration at
> this point can we really always rely on the device already having a
> valid DMA mask? I think it would make sense to at least check, and apply
> the standard default 32-bit mask  if not.

That's a good point, but we would be changing the x86/IA64 probing path
if we do what you suggest; so I think you are right and it should be a
safe change to make but I still need to make sure we do not break
anything in the process.

> > +}
> > +
> > +/**
> > + * acpi_dma_deconfigure - Tear-down DMA configuration for the device.
> > + * @dev: The pointer to the device
> > + */
> > +void acpi_dma_deconfigure(struct device *dev)
> > +{
> > +   arch_teardown_dma_ops(dev);
> > +}
> 
> As touched upon in the dwc-usb3 thread, is it worth exporting these for
> the benefit of modular bus/glue code, to match of_dma_configure()?

I think yes, I will do.

> >  static void acpi_init_coherency(struct acpi_device *adev)
> >  {
> > unsigned long long cca = 0;
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 93f280d..e96d482 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1734,8 +1734,7 @@ static void pci_dma_configure(struct pci_dev *dev)
> > if (attr == DEV_DMA_NOT_SUPPORTED)
> > dev_warn(>dev, "DMA not supported.\n");
> > else
> > -   

Re: [PATCH v5 07/14] drivers: acpi: iort: add support for ARM SMMU platform devices creation

[Robin Murphy]

On 09/09/16 15:23, Lorenzo Pieralisi wrote:
> In ARM ACPI systems, IOMMU components are specified through static
> IORT table entries. In order to create platform devices for the
> corresponding ARM SMMU components, IORT kernel code should be made
> able to parse IORT table entries and create platform devices
> dynamically.
> 
> This patch adds the generic IORT infrastructure required to create
> platform devices for ARM SMMUs.
> 
> ARM SMMU versions have different resources requirement therefore this
> patch also introduces an IORT specific structure (ie iort_iommu_config)
> that contains hooks (to be defined when the corresponding ARM SMMU
> driver support is added to the kernel) to be used to define the
> platform devices names, init the IOMMUs, count their resources and
> finally initialize them.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Hanjun Guo 
> Cc: Tomasz Nowicki 
> Cc: "Rafael J. Wysocki" 
> ---
>  drivers/acpi/arm64/iort.c | 131 
> ++
>  1 file changed, 131 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index b89b3d3..e0a9b16 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  struct iort_its_msi_chip {
> @@ -424,6 +425,135 @@ struct irq_domain *iort_get_device_domain(struct device 
> *dev, u32 req_id)
>   return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
>  }
>  
> +struct iort_iommu_config {
> + const char *name;
> + int (*iommu_init)(struct acpi_iort_node *node);
> + bool (*iommu_is_coherent)(struct acpi_iort_node *node);
> + int (*iommu_count_resources)(struct acpi_iort_node *node);
> + void (*iommu_init_resources)(struct resource *res,
> +  struct acpi_iort_node *node);
> +};
> +
> +static __init
> +const struct iort_iommu_config *iort_get_iommu_cfg(struct acpi_iort_node 
> *node)
> +{
> + return NULL;
> +}
> +
> +/**
> + * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
> + * @fwnode: IORT node associated fwnode handle
> + * @node: Pointer to SMMU ACPI IORT node
> + *
> + * Returns: 0 on success, <0 failure
> + */
> +static int __init iort_add_smmu_platform_device(struct fwnode_handle *fwnode,
> + struct acpi_iort_node *node)
> +{
> + struct platform_device *pdev;
> + struct resource *r;
> + enum dev_dma_attr attr;
> + int ret, count;
> + const struct iort_iommu_config *ops = iort_get_iommu_cfg(node);
> +
> + if (!ops)
> + return -ENODEV;
> +
> + pdev = platform_device_alloc(ops->name, PLATFORM_DEVID_AUTO);
> + if (!pdev)
> + return PTR_ERR(pdev);
> +
> + count = ops->iommu_count_resources(node);
> +
> + r = kcalloc(count, sizeof(*r), GFP_KERNEL);
> + if (!r) {
> + ret = -ENOMEM;
> + goto dev_put;
> + }
> +
> + ops->iommu_init_resources(r, node);
> +
> + ret = platform_device_add_resources(pdev, r, count);
> + /*
> +  * Resources are duplicated in platform_device_add_resources,
> +  * free their allocated memory
> +  */
> + kfree(r);
> +
> + if (ret)
> + goto dev_put;
> +
> + /*
> +  * Add a copy of IORT node pointer to platform_data to
> +  * be used to retrieve IORT data information.
> +  */
> + ret = platform_device_add_data(pdev, , sizeof(node));
> + if (ret)
> + goto dev_put;
> +
> + pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> + if (!pdev->dev.dma_mask) {
> + ret = -ENOMEM;
> + goto dev_put;
> + }

Since this is exclusively for creating SMMUs, and we know they should
never have weird shenanigans going on requiring different masks, I'd be
inclined to just point dev.dma_mask at dev.coherent_dma_mask and be done
with it.

> +
> + pdev->dev.fwnode = fwnode;
> +
> + /*
> +  * Set default dma mask value for the table walker,
> +  * to be overridden on probing with correct value.
> +  */
> + *pdev->dev.dma_mask = DMA_BIT_MASK(32);
> + pdev->dev.coherent_dma_mask = *pdev->dev.dma_mask;
> +
> + attr = ops->iommu_is_coherent(node) ?
> +  DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
> +
> + /* Configure DMA for the page table walker */
> + acpi_dma_configure(>dev, attr);

Oh look, some more code which would be simpler if acpi_dma_configure()
set the default mask itself ;)

Robin.

> +
> + ret = platform_device_add(pdev);
> + if (ret)
> + goto dma_deconfigure;
> +
> + return 0;
> +
> +dma_deconfigure:
> + acpi_dma_deconfigure(>dev);
> + kfree(pdev->dev.dma_mask);
> +
> +dev_put:
> + platform_device_put(pdev);
> +
> + return ret;
> +}
> +

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[kbuild test robot]

Hi Marek,

[auto build test ERROR on iommu/next]
[also build test ERROR on v4.8-rc6 next-20160913]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Marek-Szyprowski/Exynos-IOMMU-proper-runtime-PM-support-use-device-dependencies/20160913-205434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/iommu/exynos-iommu.c: In function 'exynos_iommu_of_xlate':
>> drivers/iommu/exynos-iommu.c:1232:2: error: implicit declaration of function 
>> 'device_link_add' [-Werror=implicit-function-declaration]
 device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
 ^
>> drivers/iommu/exynos-iommu.c:1232:37: error: 'DEVICE_LINK_AVAILABLE' 
>> undeclared (first use in this function)
 device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
^
   drivers/iommu/exynos-iommu.c:1232:37: note: each undeclared identifier is 
reported only once for each function it appears in
>> drivers/iommu/exynos-iommu.c:1233:4: error: 'DEVICE_LINK_PERSISTENT' 
>> undeclared (first use in this function)
   DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
   ^
>> drivers/iommu/exynos-iommu.c:1233:29: error: 'DEVICE_LINK_PM_RUNTIME' 
>> undeclared (first use in this function)
   DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
^
   cc1: some warnings being treated as errors

vim +/device_link_add +1232 drivers/iommu/exynos-iommu.c

  1226  
  1227  /*
  1228   * SYSMMU will be runtime enabled via device link (dependency) 
to its
  1229   * master device, so there are no direct calls to 
pm_runtime_get/put
  1230   * in this driver.
  1231   */
> 1232  device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
> 1233  DEVICE_LINK_PERSISTENT | 
> DEVICE_LINK_PM_RUNTIME);
  1234  
  1235  return 0;
  1236  }

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 06/14] drivers: acpi: implement acpi_dma_configure

[Robin Murphy]

On 09/09/16 15:23, Lorenzo Pieralisi wrote:
> On DT based systems, the of_dma_configure() API implements DMA
> configuration for a given device. On ACPI systems an API equivalent to
> of_dma_configure() is missing which implies that it is currently not
> possible to set-up DMA operations for devices through the ACPI generic
> kernel layer.
> 
> This patch fills the gap by introducing acpi_dma_configure/deconfigure()
> calls that for now are just wrappers around arch_setup_dma_ops() and
> arch_teardown_dma_ops() and also updates ACPI and PCI core code to use
> the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions.
> 
> The DMA range size passed to arch_setup_dma_ops() is sized according
> to the device coherent_dma_mask (starting at address 0x0), mirroring the
> DT probing path behaviour when a dma-ranges property is not provided
> for the device being probed; this changes the current arch_setup_dma_ops()
> call parameters in the ACPI probing case, but since arch_setup_dma_ops()
> is a NOP on all architectures but ARM/ARM64 this patch does not change
> the current kernel behaviour on them.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Acked-by: Bjorn Helgaas  [pci]
> Cc: Bjorn Helgaas 
> Cc: Robin Murphy 
> Cc: Tomasz Nowicki 
> Cc: Joerg Roedel 
> Cc: "Rafael J. Wysocki" 
> ---
>  drivers/acpi/glue.c |  4 ++--
>  drivers/acpi/scan.c | 24 
>  drivers/pci/probe.c |  3 +--
>  include/acpi/acpi_bus.h |  2 ++
>  include/linux/acpi.h|  5 +
>  5 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 5ea5dc2..f8d6564 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -227,8 +227,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
>  
>   attr = acpi_get_dma_attr(acpi_dev);
>   if (attr != DEV_DMA_NOT_SUPPORTED)
> - arch_setup_dma_ops(dev, 0, 0, NULL,
> -attr == DEV_DMA_COHERENT);
> + acpi_dma_configure(dev, attr);
>  
>   acpi_physnode_link_name(physical_node_name, node_id);
>   retval = sysfs_create_link(_dev->dev.kobj, >kobj,
> @@ -251,6 +250,7 @@ int acpi_bind_one(struct device *dev, struct acpi_device 
> *acpi_dev)
>   return 0;
>  
>   err:
> + acpi_dma_deconfigure(dev);
>   ACPI_COMPANION_SET(dev, NULL);
>   put_device(dev);
>   put_device(_dev->dev);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index e878fc7..9614232 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1370,6 +1370,30 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device 
> *adev)
>   return DEV_DMA_NON_COHERENT;
>  }
>  
> +/**
> + * acpi_dma_configure - Set-up DMA configuration for the device.
> + * @dev: The pointer to the device
> + * @attr: device dma attributes
> + */
> +void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
> +{
> + /*
> +  * Assume dma valid range starts at 0 and covers the whole
> +  * coherent_dma_mask.
> +  */
> + arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, NULL,
> +attr == DEV_DMA_COHERENT);

This looks a bit hairy - if we're setting up the DMA configuration at
this point can we really always rely on the device already having a
valid DMA mask? I think it would make sense to at least check, and apply
the standard default 32-bit mask  if not.

> +}
> +
> +/**
> + * acpi_dma_deconfigure - Tear-down DMA configuration for the device.
> + * @dev: The pointer to the device
> + */
> +void acpi_dma_deconfigure(struct device *dev)
> +{
> + arch_teardown_dma_ops(dev);
> +}

As touched upon in the dwc-usb3 thread, is it worth exporting these for
the benefit of modular bus/glue code, to match of_dma_configure()?

> +
>  static void acpi_init_coherency(struct acpi_device *adev)
>  {
>   unsigned long long cca = 0;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 93f280d..e96d482 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1734,8 +1734,7 @@ static void pci_dma_configure(struct pci_dev *dev)
>   if (attr == DEV_DMA_NOT_SUPPORTED)
>   dev_warn(>dev, "DMA not supported.\n");
>   else
> - arch_setup_dma_ops(>dev, 0, 0, NULL,
> -attr == DEV_DMA_COHERENT);
> + acpi_dma_configure(>dev, attr);
>   }

What about non-PCI stuff? I see there's at least an
acpi_create_platform_device() which I'd also kind of expect to see as a
caller (which indeed might also tie in with the aforementioned default
mask initialisation).

Robin.

>  
>   pci_put_host_bridge_device(bridge);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index c1a524d..4242c31 

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Ulf Hansson]

On 13 September 2016 at 14:49, Marek Szyprowski
 wrote:
> This patch uses recently introduced device links to track the runtime pm
> state of the master's device. This way each SYSMMU controller is runtime
> activated when its master's device is active and can save/restore its state
> instead of being enabled all the time. This way SYSMMU controllers no
> longer prevents respective power domains to be turned off when master's
> device is not used.

Apologize for not reviewing earlier and if you find my
questions/suggestions being silly. You may ignore them, if you don't
think they deserves a proper answer. :-)

I am not so familiar with the IOMMU subsystem, but I am wondering
whether the issue you are solving, is similar to what can be observed
for DMA and serial drivers. And of course also for other IOMMU
drivers.

In general the DMA/serial drivers requires to use the
pm_runtime_irq_safe() option, to be able to easily deploy runtime PM
support (of course there are some other workarounds as well).

As we know, using the pm_runtime_irq_safe() option comes with some
limitations, such as the runtime PM callbacks is not allowed to sleep.
For a PM domain (genpd) that is attached to the device, this also
means it must not be powered off.

To solve this problem, I was thinking we could convert to use the
asynchronous pm_runtime_get() API, when trying to runtime resume the
device from atomic contexts.

Of course when it turns out that the device isn't yet runtime resumed
immediately after calling pm_runtime_get(), the request needs to be
put on a request queue to be managed shortly after instead. Doing it
like this, would remove the need to use the pm_runtime_irq_safe()
option.

I realize that such change needs to be implemented in common code for
IOMMU drivers, if at all possible.

Anyway, I hope you at least get the idea and I just wanted to mention
that I have been exploring this option for DMA and serial drivers.

Kind regards
Uffe

>
> Signed-off-by: Marek Szyprowski 
> ---
>  drivers/iommu/exynos-iommu.c | 225 
> ++-
>  1 file changed, 94 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index b0fa4d432e71..34717a0b1902 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -206,6 +206,7 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] 
> = {
>  struct exynos_iommu_owner {
> struct list_head controllers;   /* list of sysmmu_drvdata.owner_node 
> */
> struct iommu_domain *domain;/* domain this device is attached */
> +   struct mutex rpm_lock;  /* for runtime pm of all sysmmus */
>  };
>
>  /*
> @@ -237,8 +238,8 @@ struct sysmmu_drvdata {
> struct clk *aclk;   /* SYSMMU's aclk clock */
> struct clk *pclk;   /* SYSMMU's pclk clock */
> struct clk *clk_master; /* master's device clock */
> -   int activations;/* number of calls to sysmmu_enable */
> spinlock_t lock;/* lock for modyfying state */
> +   int active; /* current status */
> struct exynos_iommu_domain *domain; /* domain we belong to */
> struct list_head domain_node;   /* node for domain clients list */
> struct list_head owner_node;/* node for owner controllers list */
> @@ -251,25 +252,6 @@ static struct exynos_iommu_domain 
> *to_exynos_domain(struct iommu_domain *dom)
> return container_of(dom, struct exynos_iommu_domain, domain);
>  }
>
> -static bool set_sysmmu_active(struct sysmmu_drvdata *data)
> -{
> -   /* return true if the System MMU was not active previously
> -  and it needs to be initialized */
> -   return ++data->activations == 1;
> -}
> -
> -static bool set_sysmmu_inactive(struct sysmmu_drvdata *data)
> -{
> -   /* return true if the System MMU is needed to be disabled */
> -   BUG_ON(data->activations < 1);
> -   return --data->activations == 0;
> -}
> -
> -static bool is_sysmmu_active(struct sysmmu_drvdata *data)
> -{
> -   return data->activations > 0;
> -}
> -
>  static void sysmmu_unblock(struct sysmmu_drvdata *data)
>  {
> writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
> @@ -389,7 +371,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void 
> *dev_id)
> unsigned short reg_status, reg_clear;
> int ret = -ENOSYS;
>
> -   WARN_ON(!is_sysmmu_active(data));
> +   WARN_ON(!data->active);
>
> if (MMU_MAJ_VER(data->version) < 5) {
> reg_status = REG_INT_STATUS;
> @@ -435,40 +417,19 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void 
> *dev_id)
> return IRQ_HANDLED;
>  }
>
> -static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
> +static void __sysmmu_disable(struct sysmmu_drvdata *data)
>  {
> +   unsigned long flags;
> +
> 

Re: [PATCH] iommu/io-pgtable-arm: Check for v7s-incapable systems

[Auger Eric]

Hi,
On 13/09/2016 15:26, Robin Murphy wrote:
> On machines with no 32-bit addressable RAM whatsoever, we shouldn't
> even touch the v7s format as it's never going to work.
> 
> Fixes: e5fc9753b1a8 ("iommu/io-pgtable: Add ARMv7 short descriptor support")
> Reported-by: Eric Auger 
> Signed-off-by: Robin Murphy 
> ---
>  drivers/iommu/io-pgtable-arm-v7s.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c 
> b/drivers/iommu/io-pgtable-arm-v7s.c
> index def8ca1c982d..b7759a48f4ed 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -633,6 +633,9 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct 
> io_pgtable_cfg *cfg,
>  {
>   struct arm_v7s_io_pgtable *data;
>  
> + if (upper_32_bits(PHYS_OFFSET))
> + return NULL;
> +
>   if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
>   return NULL;
>  
> 
Fixes the oops on AMD Overdrive
(CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST=y and no DMA_API)

Tested-by: Eric Auger 

Thanks

Eric

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic

[Lorenzo Pieralisi]

On Tue, Sep 13, 2016 at 02:38:35PM +0100, Robin Murphy wrote:
> >  static int arm_smmu_match_node(struct device *dev, void *data)
> >  {
> > -   return dev->of_node == data;
> > +   struct fwnode_handle *fwnode;
> > +
> > +   fwnode = dev->of_node ? >of_node->fwnode : dev->fwnode;
> > +
> > +   return fwnode == data;
> >  }
> 
> Maybe we should hoist the dev_fwnode() helper from property.c up to
> property.h so we can just have "return dev_fwnode(dev) == data;" here?

Yes, that's one way of doing it. The other would be initializing
dev->fwnode to >of_node->fwnode in the DT probe path but first
I need to understand why that is not done in the first place.

[...]

> > +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> > +{
> > +   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > +   size_t size;
> > +
> > +   if (!fwspec)
> > +   return -EINVAL;
> > +
> > +   size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 1]);
> > +   fwspec = krealloc(fwspec, size, GFP_KERNEL);
> > +   if (!fwspec)
> > +   return -ENOMEM;
> > +
> > +   while (num_ids--)
> > +   fwspec->ids[fwspec->num_ids++] = *ids++;
> 
> You've still got the +1 bug and incomprehensible loop from the old code
> here, rather than the fixed version being removed below. Although now
> that I've taken the plunge and done it properly in core code from the
> outset, that should hopefully become moot.

Gah sorry, rebase mistake. I will wait for the dust to settle before
churning out a new series, it is hard to respin without a stable
base (hopefully your series will make this patch useless).

Thanks !
Lorenzo

> Robin.
> 
> > +
> > +   arch_set_iommu_fwspec(dev, fwspec);
> > +   return 0;
> > +}
> > +
> > +inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> > +{
> > +   return arch_get_iommu_fwspec(dev);
> > +}
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 38669b8..ab3c069 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char 
> > *prefix, int index,
> >  }
> >  EXPORT_SYMBOL_GPL(of_get_dma_window);
> >  
> > -struct of_iommu_node {
> > -   struct list_head list;
> > -   struct device_node *np;
> > -   const struct iommu_ops *ops;
> > -};
> > -static LIST_HEAD(of_iommu_list);
> > -static DEFINE_SPINLOCK(of_iommu_lock);
> > -
> > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
> > -{
> > -   struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> > -
> > -   if (WARN_ON(!iommu))
> > -   return;
> > -
> > -   of_node_get(np);
> > -   INIT_LIST_HEAD(>list);
> > -   iommu->np = np;
> > -   iommu->ops = ops;
> > -   spin_lock(_iommu_lock);
> > -   list_add_tail(>list, _iommu_list);
> > -   spin_unlock(_iommu_lock);
> > -}
> > -
> > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> > -{
> > -   struct of_iommu_node *node;
> > -   const struct iommu_ops *ops = NULL;
> > -
> > -   spin_lock(_iommu_lock);
> > -   list_for_each_entry(node, _iommu_list, list)
> > -   if (node->np == np) {
> > -   ops = node->ops;
> > -   break;
> > -   }
> > -   spin_unlock(_iommu_lock);
> > -   return ops;
> > -}
> > -
> >  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> >  {
> > struct of_phandle_args *iommu_spec = data;
> > @@ -226,57 +187,3 @@ static int __init of_iommu_init(void)
> > return 0;
> >  }
> >  postcore_initcall_sync(of_iommu_init);
> > -
> > -int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)
> > -{
> > -   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -
> > -   if (fwspec)
> > -   return 0;
> > -
> > -   fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> > -   if (!fwspec)
> > -   return -ENOMEM;
> > -
> > -   fwspec->iommu_np = of_node_get(iommu_np);
> > -   fwspec->iommu_ops = of_iommu_get_ops(iommu_np);
> > -   arch_set_iommu_fwspec(dev, fwspec);
> > -   return 0;
> > -}
> > -
> > -void iommu_fwspec_free(struct device *dev)
> > -{
> > -   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -
> > -   if (fwspec) {
> > -   of_node_put(fwspec->iommu_np);
> > -   kfree(fwspec);
> > -   }
> > -}
> > -
> > -int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> > -{
> > -   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -   size_t size;
> > -   int i;
> > -
> > -   if (!fwspec)
> > -   return -EINVAL;
> > -
> > -   size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> > -   fwspec = krealloc(fwspec, size, GFP_KERNEL);
> > -   if (!fwspec)
> > -   return -ENOMEM;
> > -
> > -   for (i = 0; i < num_ids; i++)
> > -   fwspec->ids[fwspec->num_ids + i] = ids[i];
> > -
> > -   fwspec->num_ids += num_ids;
> > -   arch_set_iommu_fwspec(dev, fwspec);
> > -   return 0;
> > -}
> > 

Re: [PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic

[Robin Murphy]

Hi Lorenzo,

On 09/09/16 15:23, Lorenzo Pieralisi wrote:
> The iommu_fwspec structure, used to hold per device iommu configuration
> data is not OF specific and therefore can be moved to a generic
> and OF independent compilation unit.
> 
> In particular, the iommu_fwspec handling hinges on the device_node
> pointer to identify the IOMMU device associated with the iommu_fwspec
> structure, that is easily converted to a more generic fwnode_handle
> pointer that can cater for OF and non-OF (ie ACPI) systems.
> 
> Create the files and related Kconfig entry to decouple iommu_fwspec
> structure from the OF iommu kernel layer.
> 
> Given that the current iommu_fwspec implementation relies on
> the arch specific struct device.archdata.iommu field in its
> implementation, by making the code standalone and independent
> of the OF layer this patch makes sure that the iommu_fwspec
> kernel code can be selected only on arches implementing the
> struct device.archdata.iommu field by adding an explicit
> arch dependency in its config entry.
> 
> Current drivers using the iommu_fwspec for streamid translation
> are converted to the new iommu_fwspec API by simply converting
> the device_node to its fwnode_handle pointer.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Will Deacon 
> Cc: Hanjun Guo 
> Cc: Robin Murphy 
> Cc: Joerg Roedel 
> ---
>  drivers/iommu/Kconfig|   4 ++
>  drivers/iommu/Makefile   |   1 +
>  drivers/iommu/arm-smmu-v3.c  |  16 --
>  drivers/iommu/arm-smmu.c |  17 +++---
>  drivers/iommu/iommu-fwspec.c | 126 
> +++
>  drivers/iommu/of_iommu.c |  93 
>  include/linux/iommu-fwspec.h |  70 
>  include/linux/of_iommu.h |  38 -
>  8 files changed, 234 insertions(+), 131 deletions(-)
>  create mode 100644 drivers/iommu/iommu-fwspec.c
>  create mode 100644 include/linux/iommu-fwspec.h
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 101cb17..873bd41 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -70,6 +70,10 @@ config OF_IOMMU
>  config HAVE_IOMMU_FWSPEC
>   bool
>  
> +config IOMMU_FWSPEC
> +   def_bool y
> +   depends on IOMMU_API
> +
>  # IOMMU-agnostic DMA-mapping layer
>  config IOMMU_DMA
>   bool
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 195f7b9..bbbc6d6 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
>  obj-$(CONFIG_IOMMU_IOVA) += iova.o
> +obj-$(CONFIG_IOMMU_FWSPEC) += iommu-fwspec.o
>  obj-$(CONFIG_OF_IOMMU)   += of_iommu.o
>  obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
>  obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index be293b5..a7e9de9 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1720,13 +1721,18 @@ static struct platform_driver arm_smmu_driver;
>  
>  static int arm_smmu_match_node(struct device *dev, void *data)
>  {
> - return dev->of_node == data;
> + struct fwnode_handle *fwnode;
> +
> + fwnode = dev->of_node ? >of_node->fwnode : dev->fwnode;
> +
> + return fwnode == data;
>  }

Maybe we should hoist the dev_fwnode() helper from property.c up to
property.h so we can just have "return dev_fwnode(dev) == data;" here?

>  
> -static struct arm_smmu_device *arm_smmu_get_by_node(struct device_node *np)
> +static struct arm_smmu_device *
> +arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
>  {
>   struct device *dev = driver_find_device(_smmu_driver.driver, NULL,
> - np, arm_smmu_match_node);
> + fwnode, arm_smmu_match_node);
>   put_device(dev);
>   return dev ? dev_get_drvdata(dev) : NULL;
>  }
> @@ -1762,7 +1768,7 @@ static int arm_smmu_add_device(struct device *dev)
>   master = fwspec->iommu_priv;
>   smmu = master->smmu;
>   } else {
> - smmu = arm_smmu_get_by_node(fwspec->iommu_np);
> + smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
>   if (!smmu)
>   return -ENODEV;
>   master = kzalloc(sizeof(*master), GFP_KERNEL);
> @@ -1874,7 +1880,7 @@ out_unlock:
>  
>  static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args 
> *args)
>  {
> - int ret = iommu_fwspec_init(dev, args->np);
> + int ret = iommu_fwspec_init(dev, >np->fwnode);
>  
>   if (!ret)
>   ret = 

[PATCH] iommu/io-pgtable-arm: Check for v7s-incapable systems

[Robin Murphy]

On machines with no 32-bit addressable RAM whatsoever, we shouldn't
even touch the v7s format as it's never going to work.

Fixes: e5fc9753b1a8 ("iommu/io-pgtable: Add ARMv7 short descriptor support")
Reported-by: Eric Auger 
Signed-off-by: Robin Murphy 
---
 drivers/iommu/io-pgtable-arm-v7s.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c 
b/drivers/iommu/io-pgtable-arm-v7s.c
index def8ca1c982d..b7759a48f4ed 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -633,6 +633,9 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct 
io_pgtable_cfg *cfg,
 {
struct arm_v7s_io_pgtable *data;
 
+   if (upper_32_bits(PHYS_OFFSET))
+   return NULL;
+
if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
return NULL;
 
-- 
2.8.1.dirty

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU

[Auger Eric]

Hi Robin,

On 13/09/2016 14:40, Robin Murphy wrote:
> Hi Eric,
> 
> On 13/09/16 13:14, Auger Eric wrote:
>> Hi Robin
>>
>> On 12/09/2016 18:13, Robin Murphy wrote:
>>> Hi all,
>>>
>>> To any more confusing fixups and crazily numbered extra patches, here's
>>> a quick v7 with everything rebased into the right order. The significant
>>> change this time is to implement iommu_fwspec properly from the start,
>>> which ends up being far simpler and more robust than faffing about
>>> introducing it somewhere 'less intrusive' to move toward core code later.
>>>
>>> New branch in the logical place:
>>>
>>> git://linux-arm.org/linux-rm iommu/generic-v7
>> I just tested your branch on AMD overdrive *without* updating the device
>> tree description according to the new syntax and I get a kernel oops.
>> See logs attached. Continuing my investigations ...
> 
> Looking at that backtrace, it seems the offending commit is actually in
> Will's devel branch _underneath_ this series; what's blowing up there is
> the short-descriptor io-pgtable selftests, which you should be able to
> reproduce on anything back to 4.6-rc1 with
> CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST=y.
I confirm that when disabling the option, I don't get the oops anymore.

Thanks!

Eric
> 
> The short-descriptor code is never going to work on Seattle due to the
> lack of 32-bit addressable memory - in normal use it would fail
> gracefully because it couldn't allocate anything, but since the
> selftests bypass the DMA API and corresponding checks, you end up with
> nastiness happening via truncated addresses. A while back I did start
> looking into generalising the selftests to remove all the "if
> (!selftest_running)" special-casing; might be time to pick that up again.
> 
> Robin.
> 
>>
>> Best Regards
>>
>> Eric
>>>
>>> Robin.
>>>
>>> Mark Rutland (1):
>>>   Docs: dt: add PCI IOMMU map bindings
>>>
>>> Robin Murphy (21):
>>>   of/irq: Break out msi-map lookup (again)
>>>   iommu/of: Handle iommu-map property for PCI
>>>   iommu: Introduce iommu_fwspec
>>>   Docs: dt: document ARM SMMUv3 generic binding usage
>>>   iommu/arm-smmu: Fall back to global bypass
>>>   iommu/arm-smmu: Implement of_xlate() for SMMUv3
>>>   iommu/arm-smmu: Support non-PCI devices with SMMUv3
>>>   iommu/arm-smmu: Set PRIVCFG in stage 1 STEs
>>>   iommu/arm-smmu: Handle stream IDs more dynamically
>>>   iommu/arm-smmu: Consolidate stream map entry state
>>>   iommu/arm-smmu: Keep track of S2CR state
>>>   iommu/arm-smmu: Refactor mmu-masters handling
>>>   iommu/arm-smmu: Streamline SMMU data lookups
>>>   iommu/arm-smmu: Add a stream map entry iterator
>>>   iommu/arm-smmu: Intelligent SMR allocation
>>>   iommu/arm-smmu: Convert to iommu_fwspec
>>>   Docs: dt: document ARM SMMU generic binding usage
>>>   iommu/arm-smmu: Wire up generic configuration support
>>>   iommu/arm-smmu: Set domain geometry
>>>   iommu/dma: Add support for mapping MSIs
>>>   iommu/dma: Avoid PCI host bridge windows
>>>
>>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt  |   8 +-
>>>  .../devicetree/bindings/iommu/arm,smmu.txt |  63 +-
>>>  .../devicetree/bindings/pci/pci-iommu.txt  | 171 
>>>  arch/arm64/mm/dma-mapping.c|   2 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_iommu.h  |   2 +-
>>>  drivers/iommu/Kconfig  |   2 +-
>>>  drivers/iommu/arm-smmu-v3.c| 386 +
>>>  drivers/iommu/arm-smmu.c   | 962 
>>> ++---
>>>  drivers/iommu/dma-iommu.c  | 161 +++-
>>>  drivers/iommu/iommu.c  |  56 ++
>>>  drivers/iommu/of_iommu.c   |  52 +-
>>>  drivers/irqchip/irq-gic-v2m.c  |   3 +
>>>  drivers/irqchip/irq-gic-v3-its.c   |   3 +
>>>  drivers/of/irq.c   |  78 +-
>>>  drivers/of/of_pci.c| 102 +++
>>>  include/linux/device.h |   3 +
>>>  include/linux/dma-iommu.h  |  12 +-
>>>  include/linux/iommu.h  |  38 +
>>>  include/linux/of_pci.h |  10 +
>>>  19 files changed, 1323 insertions(+), 791 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt
>>>
>>
>>
>>
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Marek Szyprowski]

This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active and can save/restore its state
instead of being enabled all the time. This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.

Signed-off-by: Marek Szyprowski 
---
 drivers/iommu/exynos-iommu.c | 225 ++-
 1 file changed, 94 insertions(+), 131 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b0fa4d432e71..34717a0b1902 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -206,6 +206,7 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = {
 struct exynos_iommu_owner {
struct list_head controllers;   /* list of sysmmu_drvdata.owner_node */
struct iommu_domain *domain;/* domain this device is attached */
+   struct mutex rpm_lock;  /* for runtime pm of all sysmmus */
 };
 
 /*
@@ -237,8 +238,8 @@ struct sysmmu_drvdata {
struct clk *aclk;   /* SYSMMU's aclk clock */
struct clk *pclk;   /* SYSMMU's pclk clock */
struct clk *clk_master; /* master's device clock */
-   int activations;/* number of calls to sysmmu_enable */
spinlock_t lock;/* lock for modyfying state */
+   int active; /* current status */
struct exynos_iommu_domain *domain; /* domain we belong to */
struct list_head domain_node;   /* node for domain clients list */
struct list_head owner_node;/* node for owner controllers list */
@@ -251,25 +252,6 @@ static struct exynos_iommu_domain *to_exynos_domain(struct 
iommu_domain *dom)
return container_of(dom, struct exynos_iommu_domain, domain);
 }
 
-static bool set_sysmmu_active(struct sysmmu_drvdata *data)
-{
-   /* return true if the System MMU was not active previously
-  and it needs to be initialized */
-   return ++data->activations == 1;
-}
-
-static bool set_sysmmu_inactive(struct sysmmu_drvdata *data)
-{
-   /* return true if the System MMU is needed to be disabled */
-   BUG_ON(data->activations < 1);
-   return --data->activations == 0;
-}
-
-static bool is_sysmmu_active(struct sysmmu_drvdata *data)
-{
-   return data->activations > 0;
-}
-
 static void sysmmu_unblock(struct sysmmu_drvdata *data)
 {
writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
@@ -389,7 +371,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
unsigned short reg_status, reg_clear;
int ret = -ENOSYS;
 
-   WARN_ON(!is_sysmmu_active(data));
+   WARN_ON(!data->active);
 
if (MMU_MAJ_VER(data->version) < 5) {
reg_status = REG_INT_STATUS;
@@ -435,40 +417,19 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void 
*dev_id)
return IRQ_HANDLED;
 }
 
-static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_disable(struct sysmmu_drvdata *data)
 {
+   unsigned long flags;
+
clk_enable(data->clk_master);
 
+   spin_lock_irqsave(>lock, flags);
writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
writel(0, data->sfrbase + REG_MMU_CFG);
-
-   __sysmmu_disable_clocks(data);
-}
-
-static bool __sysmmu_disable(struct sysmmu_drvdata *data)
-{
-   bool disabled;
-   unsigned long flags;
-
-   spin_lock_irqsave(>lock, flags);
-
-   disabled = set_sysmmu_inactive(data);
-
-   if (disabled) {
-   data->pgtable = 0;
-   data->domain = NULL;
-
-   __sysmmu_disable_nocount(data);
-
-   dev_dbg(data->sysmmu, "Disabled\n");
-   } else  {
-   dev_dbg(data->sysmmu, "%d times left to disable\n",
-   data->activations);
-   }
-
+   data->active = false;
spin_unlock_irqrestore(>lock, flags);
 
-   return disabled;
+   __sysmmu_disable_clocks(data);
 }
 
 static void __sysmmu_init_config(struct sysmmu_drvdata *data)
@@ -485,17 +446,19 @@ static void __sysmmu_init_config(struct sysmmu_drvdata 
*data)
writel(cfg, data->sfrbase + REG_MMU_CFG);
 }
 
-static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_enable(struct sysmmu_drvdata *data)
 {
+   unsigned long flags;
+
__sysmmu_enable_clocks(data);
 
+   spin_lock_irqsave(>lock, flags);
writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
-
__sysmmu_init_config(data);
-
__sysmmu_set_ptbase(data, data->pgtable);
-
writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
+   data->active = true;
+   spin_unlock_irqrestore(>lock, flags);
 
/*
 * SYSMMU driver keeps master's clock enabled only for the short
@@ 

[PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Marek Szyprowski]

Hello,

This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using recently introduce device
links, which lets SYSMMU controller's runtime PM to follow master's device
runtime PM state (the device which actually performs DMA transaction).
The main idea behind this solution is an observation that any DMA activity
from master device can be done only when master device is active, thus when
master device is suspended SYSMMU controller device can also be suspended.

This patchset solves the situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).

Patch requires second version of Rafeal's "Functional dependencies
between devices" patchset, which is available here:
https://lkml.org/lkml/2016/9/8/798

If one wants to test this patchset, I've provided a branch with all needed
patches (some fixes for Exynos4 FIMC-IS driver are needed):
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.8-iommu-pm-v3

Patches are based on vanilla v4.8-rc6 kernel with Rafael's patches applied.

Best regards
Marek Szyprowski
Samsung R Institute Poland


Changelog:
v3:
- rebased on top of latest device dependencies/links patchset
- added proper locking between runtime pm, iommu_attach/detach and sysmmu
  enable/disable(added per iommu owner device's rpm lock)

v2:
- replaced PM notifiers with generic device dependencies/links developped
  by Rafael J. Wysocki

v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version


Patch summary:

Marek Szyprowski (2):
  iommu/exynos: Remove excessive, useless debug
  iommu/exynos: Add proper runtime pm support

 drivers/iommu/exynos-iommu.c | 228 ++-
 1 file changed, 94 insertions(+), 134 deletions(-)

-- 
1.9.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v3 1/2] iommu/exynos: Remove excessive, useless debug

[Marek Szyprowski]

Remove excessive, useless debug about skipping TLB invalidation, which
is a normal situation when more aggressive power management is enabled.

Signed-off-by: Marek Szyprowski 
---
 drivers/iommu/exynos-iommu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 75fbe5d30cb3..b0fa4d432e71 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -579,9 +579,6 @@ static void sysmmu_tlb_invalidate_entry(struct 
sysmmu_drvdata *data,
sysmmu_unblock(data);
}
clk_disable(data->clk_master);
-   } else {
-   dev_dbg(data->master,
-   "disabled. Skipping TLB invalidation @ %#x\n", iova);
}
spin_unlock_irqrestore(>lock, flags);
 }
-- 
1.9.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU

[Robin Murphy]

Hi Eric,

On 13/09/16 13:14, Auger Eric wrote:
> Hi Robin
> 
> On 12/09/2016 18:13, Robin Murphy wrote:
>> Hi all,
>>
>> To any more confusing fixups and crazily numbered extra patches, here's
>> a quick v7 with everything rebased into the right order. The significant
>> change this time is to implement iommu_fwspec properly from the start,
>> which ends up being far simpler and more robust than faffing about
>> introducing it somewhere 'less intrusive' to move toward core code later.
>>
>> New branch in the logical place:
>>
>> git://linux-arm.org/linux-rm iommu/generic-v7
> I just tested your branch on AMD overdrive *without* updating the device
> tree description according to the new syntax and I get a kernel oops.
> See logs attached. Continuing my investigations ...

Looking at that backtrace, it seems the offending commit is actually in
Will's devel branch _underneath_ this series; what's blowing up there is
the short-descriptor io-pgtable selftests, which you should be able to
reproduce on anything back to 4.6-rc1 with
CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST=y.

The short-descriptor code is never going to work on Seattle due to the
lack of 32-bit addressable memory - in normal use it would fail
gracefully because it couldn't allocate anything, but since the
selftests bypass the DMA API and corresponding checks, you end up with
nastiness happening via truncated addresses. A while back I did start
looking into generalising the selftests to remove all the "if
(!selftest_running)" special-casing; might be time to pick that up again.

Robin.

> 
> Best Regards
> 
> Eric
>>
>> Robin.
>>
>> Mark Rutland (1):
>>   Docs: dt: add PCI IOMMU map bindings
>>
>> Robin Murphy (21):
>>   of/irq: Break out msi-map lookup (again)
>>   iommu/of: Handle iommu-map property for PCI
>>   iommu: Introduce iommu_fwspec
>>   Docs: dt: document ARM SMMUv3 generic binding usage
>>   iommu/arm-smmu: Fall back to global bypass
>>   iommu/arm-smmu: Implement of_xlate() for SMMUv3
>>   iommu/arm-smmu: Support non-PCI devices with SMMUv3
>>   iommu/arm-smmu: Set PRIVCFG in stage 1 STEs
>>   iommu/arm-smmu: Handle stream IDs more dynamically
>>   iommu/arm-smmu: Consolidate stream map entry state
>>   iommu/arm-smmu: Keep track of S2CR state
>>   iommu/arm-smmu: Refactor mmu-masters handling
>>   iommu/arm-smmu: Streamline SMMU data lookups
>>   iommu/arm-smmu: Add a stream map entry iterator
>>   iommu/arm-smmu: Intelligent SMR allocation
>>   iommu/arm-smmu: Convert to iommu_fwspec
>>   Docs: dt: document ARM SMMU generic binding usage
>>   iommu/arm-smmu: Wire up generic configuration support
>>   iommu/arm-smmu: Set domain geometry
>>   iommu/dma: Add support for mapping MSIs
>>   iommu/dma: Avoid PCI host bridge windows
>>
>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt  |   8 +-
>>  .../devicetree/bindings/iommu/arm,smmu.txt |  63 +-
>>  .../devicetree/bindings/pci/pci-iommu.txt  | 171 
>>  arch/arm64/mm/dma-mapping.c|   2 +-
>>  drivers/gpu/drm/exynos/exynos_drm_iommu.h  |   2 +-
>>  drivers/iommu/Kconfig  |   2 +-
>>  drivers/iommu/arm-smmu-v3.c| 386 +
>>  drivers/iommu/arm-smmu.c   | 962 
>> ++---
>>  drivers/iommu/dma-iommu.c  | 161 +++-
>>  drivers/iommu/iommu.c  |  56 ++
>>  drivers/iommu/of_iommu.c   |  52 +-
>>  drivers/irqchip/irq-gic-v2m.c  |   3 +
>>  drivers/irqchip/irq-gic-v3-its.c   |   3 +
>>  drivers/of/irq.c   |  78 +-
>>  drivers/of/of_pci.c| 102 +++
>>  include/linux/device.h |   3 +
>>  include/linux/dma-iommu.h  |  12 +-
>>  include/linux/iommu.h  |  38 +
>>  include/linux/of_pci.h |  10 +
>>  19 files changed, 1323 insertions(+), 791 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt
>>
> 
> 
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 00/22] Generic DT bindings for PCI IOMMUs and ARM SMMU

[Auger Eric]

Hi Robin

On 12/09/2016 18:13, Robin Murphy wrote:
> Hi all,
> 
> To any more confusing fixups and crazily numbered extra patches, here's
> a quick v7 with everything rebased into the right order. The significant
> change this time is to implement iommu_fwspec properly from the start,
> which ends up being far simpler and more robust than faffing about
> introducing it somewhere 'less intrusive' to move toward core code later.
> 
> New branch in the logical place:
> 
> git://linux-arm.org/linux-rm iommu/generic-v7
I just tested your branch on AMD overdrive *without* updating the device
tree description according to the new syntax and I get a kernel oops.
See logs attached. Continuing my investigations ...

Best Regards

Eric
> 
> Robin.
> 
> Mark Rutland (1):
>   Docs: dt: add PCI IOMMU map bindings
> 
> Robin Murphy (21):
>   of/irq: Break out msi-map lookup (again)
>   iommu/of: Handle iommu-map property for PCI
>   iommu: Introduce iommu_fwspec
>   Docs: dt: document ARM SMMUv3 generic binding usage
>   iommu/arm-smmu: Fall back to global bypass
>   iommu/arm-smmu: Implement of_xlate() for SMMUv3
>   iommu/arm-smmu: Support non-PCI devices with SMMUv3
>   iommu/arm-smmu: Set PRIVCFG in stage 1 STEs
>   iommu/arm-smmu: Handle stream IDs more dynamically
>   iommu/arm-smmu: Consolidate stream map entry state
>   iommu/arm-smmu: Keep track of S2CR state
>   iommu/arm-smmu: Refactor mmu-masters handling
>   iommu/arm-smmu: Streamline SMMU data lookups
>   iommu/arm-smmu: Add a stream map entry iterator
>   iommu/arm-smmu: Intelligent SMR allocation
>   iommu/arm-smmu: Convert to iommu_fwspec
>   Docs: dt: document ARM SMMU generic binding usage
>   iommu/arm-smmu: Wire up generic configuration support
>   iommu/arm-smmu: Set domain geometry
>   iommu/dma: Add support for mapping MSIs
>   iommu/dma: Avoid PCI host bridge windows
> 
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt  |   8 +-
>  .../devicetree/bindings/iommu/arm,smmu.txt |  63 +-
>  .../devicetree/bindings/pci/pci-iommu.txt  | 171 
>  arch/arm64/mm/dma-mapping.c|   2 +-
>  drivers/gpu/drm/exynos/exynos_drm_iommu.h  |   2 +-
>  drivers/iommu/Kconfig  |   2 +-
>  drivers/iommu/arm-smmu-v3.c| 386 +
>  drivers/iommu/arm-smmu.c   | 962 
> ++---
>  drivers/iommu/dma-iommu.c  | 161 +++-
>  drivers/iommu/iommu.c  |  56 ++
>  drivers/iommu/of_iommu.c   |  52 +-
>  drivers/irqchip/irq-gic-v2m.c  |   3 +
>  drivers/irqchip/irq-gic-v3-its.c   |   3 +
>  drivers/of/irq.c   |  78 +-
>  drivers/of/of_pci.c| 102 +++
>  include/linux/device.h |   3 +
>  include/linux/dma-iommu.h  |  12 +-
>  include/linux/iommu.h  |  38 +
>  include/linux/of_pci.h |  10 +
>  19 files changed, 1323 insertions(+), 791 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt
> 



[3.185447] CPU features: detected feature: 32-bit EL0 Support
[3.185455] CPU: All CPU(s) started at EL2
[3.185475] alternatives: patching kernel code
[3.188549] devtmpfs: initialized
[3.189484] SMBIOS 3.0.0 present.
[3.189570] clocksource: jiffies: mask: 0x max_cycles: 0x, 
max_idle_ns: 764504178510 ns
[3.189750] pinctrl core: initialized pinctrl subsystem
[3.190113] NET: Registered protocol family 16
[3.205134] cpuidle: using governor menu
[3.205292] arm-smmu: deprecated "mmu-masters" DT property in use; DMA API 
support unavailable
[3.205319] arm-smmu e060.smmu: probing hardware configuration...
[3.205326] arm-smmu e060.smmu: SMMUv1 with:
[3.205334] arm-smmu e060.smmu:  stage 2 translation
[3.205341] arm-smmu e060.smmu:  non-coherent table walk
[3.205348] arm-smmu e060.smmu:  (IDR0.CTTW overridden by dma-coherent 
property)
[3.205358] arm-smmu e060.smmu:  stream matching with 32 register 
groups, mask 0x7fff
[3.205369] arm-smmu e060.smmu:  8 context banks (8 stage-2 only)
[3.205377] arm-smmu e060.smmu:  Supported page sizes: 0x60211000
[3.205384] arm-smmu e060.smmu:  Stage-2: 40-bit IPA -> 40-bit PA
[3.205489] arm-smmu: deprecated "mmu-masters" DT property in use; DMA API 
support unavailable
[3.205512] arm-smmu e080.smmu: probing hardware configuration...
[3.205519] arm-smmu e080.smmu: SMMUv1 with:
[3.205525] arm-smmu e080.smmu:  stage 2 translation
[3.205533] arm-smmu e080.smmu:  non-coherent table walk
[3.205539] arm-smmu e080.smmu:  (IDR0.CTTW overridden by dma-coherent 
property)
[3.205550] arm-smmu e080.smmu:  stream matching with 32 register 
groups, 

[PATCH v7.1 04/22] iommu: Introduce iommu_fwspec

[Robin Murphy]

Introduce a common structure to hold the per-device firmware data that
most IOMMU drivers need to keep track of. This enables us to configure
much of that data from common firmware code, and consolidate a lot of
the equivalent implementations, device look-up tables, etc. which are
currently strewn across IOMMU drivers.

This will also be enable us to address the outstanding "multiple IOMMUs
on the platform bus" problem by tweaking IOMMU API calls to prefer
dev->fwspec->ops before falling back to dev->bus->iommu_ops, and thus
gracefully handle those troublesome systems which we currently cannot.

As the first user, hook up the OF IOMMU configuration mechanism. The
driver-defined nature of DT cells means that we still need the drivers
to translate and add the IDs themselves, but future users such as the
much less free-form ACPI IORT will be much simpler and self-contained.

CC: Greg Kroah-Hartman 
Suggested-by: Will Deacon 
Signed-off-by: Robin Murphy 

---

- Drop the 'gradual introduction' fiddling and go straight to struct
  device and common code, as it prevents all the silly build issues
  and ultimately makes life simpler for everyone
- This time without missing "static inline"s on the stubs...

---
 drivers/iommu/iommu.c| 58 
 drivers/iommu/of_iommu.c |  8 +--
 include/linux/device.h   |  3 +++
 include/linux/iommu.h| 39 
 4 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b06d93594436..9a2f1960873b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 static struct kset *iommu_group_kset;
@@ -1613,3 +1614,60 @@ out:
 
return ret;
 }
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+ const struct iommu_ops *ops)
+{
+   struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+   if (fwspec)
+   return ops == fwspec->ops ? 0 : -EINVAL;
+
+   fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
+   if (!fwspec)
+   return -ENOMEM;
+
+   of_node_get(to_of_node(iommu_fwnode));
+   fwspec->iommu_fwnode = iommu_fwnode;
+   fwspec->ops = ops;
+   dev->iommu_fwspec = fwspec;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_fwspec_init);
+
+void iommu_fwspec_free(struct device *dev)
+{
+   struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+   if (fwspec) {
+   fwnode_handle_put(fwspec->iommu_fwnode);
+   kfree(fwspec);
+   dev->iommu_fwspec = NULL;
+   }
+}
+EXPORT_SYMBOL_GPL(iommu_fwspec_free);
+
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
+{
+   struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+   size_t size;
+   int i;
+
+   if (!fwspec)
+   return -EINVAL;
+
+   size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
+   if (size > sizeof(*fwspec)) {
+   fwspec = krealloc(dev->iommu_fwspec, size, GFP_KERNEL);
+   if (!fwspec)
+   return -ENOMEM;
+   }
+
+   for (i = 0; i < num_ids; i++)
+   fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+   fwspec->num_ids += num_ids;
+   dev->iommu_fwspec = fwspec;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 19e1e8f2f871..5b82862f571f 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -167,7 +167,9 @@ static const struct iommu_ops
return NULL;
 
ops = of_iommu_get_ops(iommu_spec.np);
-   if (!ops || !ops->of_xlate || ops->of_xlate(>dev, _spec))
+   if (!ops || !ops->of_xlate ||
+   iommu_fwspec_init(>dev, _spec.np->fwnode, ops) ||
+   ops->of_xlate(>dev, _spec))
ops = NULL;
 
of_node_put(iommu_spec.np);
@@ -196,7 +198,9 @@ const struct iommu_ops *of_iommu_configure(struct device 
*dev,
np = iommu_spec.np;
ops = of_iommu_get_ops(np);
 
-   if (!ops || !ops->of_xlate || ops->of_xlate(dev, _spec))
+   if (!ops || !ops->of_xlate ||
+   iommu_fwspec_init(dev, >fwnode, ops) ||
+   ops->of_xlate(dev, _spec))
goto err_put_node;
 
of_node_put(np);
diff --git a/include/linux/device.h b/include/linux/device.h
index 38f02814d53a..bc41e87a969b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -41,6 +41,7 @@ struct device_node;
 struct fwnode_handle;
 struct iommu_ops;
 struct iommu_group;
+struct iommu_fwspec;
 
 struct bus_attribute {
struct attributeattr;
@@ -765,6 +766,7 @@ struct device_dma_parameters {
  * gone away. This should 

Re: [PATCH v5 07/14] drivers: acpi: iort: add support for ARM SMMU platform devices creation

[Hanjun Guo]

On 2016/9/13 16:24, Lorenzo Pieralisi wrote:

On Tue, Sep 13, 2016 at 04:15:31PM +0800, Hanjun Guo wrote:

[...]


+static acpi_status __init iort_match_iommu_callback(struct
acpi_iort_node *node,
+void *context)
+{
+int ret;
+struct fwnode_handle *fwnode;
+
+fwnode = iort_get_fwnode(node);
+
+if (!fwnode)
+return AE_NOT_FOUND;
+
+ret = iort_add_smmu_platform_device(fwnode, node);
+if (ret) {
+pr_err("Error in platform device creation\n");
+return AE_ERROR;
+}
+
+return AE_OK;
+}
+
+static void __init iort_smmu_init(void)
+{
+iort_scan_node(ACPI_IORT_NODE_SMMU, iort_match_iommu_callback,
NULL);
+iort_scan_node(ACPI_IORT_NODE_SMMU_V3, iort_match_iommu_callback,
NULL);


Since iort_scan_node() returns after the first successful match it finds,
only the first SMMU_V3 in my IORT is being enumerated. I think you need
to go back to the "iterator" like approach you had been using or make
iort_match_iommu_callback() always return a non-AE_OK value so the scan
continues and has a chance to visit all of the SMMU_V3 nodes.


Please use the updated version of IORT patch (aka Tomasz's v11)
then things will work fine.


Nate is right, I was too keen on using iort_scan_node(), it does
not really work here (unless as he said I return a value !AE_OK in
the callback, which is horrible), I reverted back to the iterator
approach and I can push out a fixed up branch if useful before next
posting.


Ah, sorry, I just noticed "the first SMMU_V3" which is pretty similar
with the second problem which is noticed by Nate...

Thanks
Hanjun

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 12/14] drivers: acpi: iort: replace rid map type with type mask

[Hanjun Guo]

Hi Lorenzo,

On 2016/9/9 22:23, Lorenzo Pieralisi wrote:

IORT tables provide data that allow the kernel to carry out
device ID mappings between endpoints and system components
(eg interrupt controllers, IOMMUs). When the mapping for a
given device ID is carried out, the translation mechanism
is done on a per-subsystem basis rather than a component
subtype (ie the IOMMU kernel layer will look for mappings
from a device to all IORT node types corresponding to IOMMU
components), therefore the corresponding mapping API should
work on a range (ie mask) of IORT node types corresponding
to a common set of components (eg IOMMUs) rather than a
specific node type.

Upgrade the IORT iort_node_map_rid() API to work with a
type mask instead of a single node type so that it can
be used for mappings that span multiple components types
(ie IOMMUs).

Signed-off-by: Lorenzo Pieralisi 
Cc: Hanjun Guo 
Cc: Tomasz Nowicki 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/arm64/iort.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index a12dda9..36ea93e 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -25,6 +25,9 @@
 #include 
 #include 

+#define IORT_TYPE_MASK(type)   (1 << (type))
+#define IORT_MSI_TYPE  (1 << ACPI_IORT_NODE_ITS_GROUP)
+
 struct iort_its_msi_chip {
struct list_headlist;
struct fwnode_handle*fw_node;
@@ -283,7 +286,7 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 
type, u32 rid_in,

 static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
u32 rid_in, u32 *rid_out,
-   u8 type)
+   u8 type_mask)
 {
u32 rid = rid_in;

@@ -292,7 +295,7 @@ static struct acpi_iort_node *iort_node_map_rid(struct 
acpi_iort_node *node,
struct acpi_iort_id_mapping *map;
int i;

-   if (node->type == type) {
+   if (IORT_TYPE_MASK(node->type) & type_mask) {
if (rid_out)
*rid_out = rid;
return node;
@@ -365,7 +368,7 @@ u32 iort_msi_map_rid(struct device *dev, u32 req_id)
if (!node)
return req_id;

-   iort_node_map_rid(node, req_id, _id, ACPI_IORT_NODE_ITS_GROUP);
+   iort_node_map_rid(node, req_id, _id, IORT_MSI_TYPE);
return dev_id;
 }


I think you forgot to update another function which is ok in your v4
patch set:

@@ -411,7 +414,7 @@ iort_dev_find_its_id(struct device *dev, u32 req_id, 
unsigned int idx,

return -ENXIO;
}

-   node = iort_node_map_rid(node, req_id, NULL, ACPI_IORT_NODE_ITS_GROUP);
+   node = iort_node_map_rid(node, req_id, NULL, IORT_MSI_TYPE);
if (!node) {
dev_err(dev, "can't find related ITS node\n");
return -ENXIO;

Others are look good to me.

Thanks
Hanjun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 07/14] drivers: acpi: iort: add support for ARM SMMU platform devices creation

[Lorenzo Pieralisi]

On Tue, Sep 13, 2016 at 04:15:31PM +0800, Hanjun Guo wrote:

[...]

> >>+static acpi_status __init iort_match_iommu_callback(struct
> >>acpi_iort_node *node,
> >>+void *context)
> >>+{
> >>+int ret;
> >>+struct fwnode_handle *fwnode;
> >>+
> >>+fwnode = iort_get_fwnode(node);
> >>+
> >>+if (!fwnode)
> >>+return AE_NOT_FOUND;
> >>+
> >>+ret = iort_add_smmu_platform_device(fwnode, node);
> >>+if (ret) {
> >>+pr_err("Error in platform device creation\n");
> >>+return AE_ERROR;
> >>+}
> >>+
> >>+return AE_OK;
> >>+}
> >>+
> >>+static void __init iort_smmu_init(void)
> >>+{
> >>+iort_scan_node(ACPI_IORT_NODE_SMMU, iort_match_iommu_callback,
> >>NULL);
> >>+iort_scan_node(ACPI_IORT_NODE_SMMU_V3, iort_match_iommu_callback,
> >>NULL);
> >
> >Since iort_scan_node() returns after the first successful match it finds,
> >only the first SMMU_V3 in my IORT is being enumerated. I think you need
> >to go back to the "iterator" like approach you had been using or make
> >iort_match_iommu_callback() always return a non-AE_OK value so the scan
> >continues and has a chance to visit all of the SMMU_V3 nodes.
> 
> Please use the updated version of IORT patch (aka Tomasz's v11)
> then things will work fine.

Nate is right, I was too keen on using iort_scan_node(), it does
not really work here (unless as he said I return a value !AE_OK in
the callback, which is horrible), I reverted back to the iterator
approach and I can push out a fixed up branch if useful before next
posting.

Thanks,
Lorenzo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 14/14] drivers: acpi: iort: introduce iort_iommu_configure

[Hanjun Guo]

Hi Nate,

On 2016/9/13 16:14, Nate Watterson wrote:

On 2016-09-09 10:23, Lorenzo Pieralisi wrote:

DT based systems have a generic kernel API to configure IOMMUs
for devices (ie of_iommu_configure()).

On ARM based ACPI systems, the of_iommu_configure() equivalent can
be implemented atop ACPI IORT kernel API, with the corresponding
functions to map device identifiers to IOMMUs and retrieve the
corresponding IOMMU operations necessary for DMA operations set-up.

By relying on the iommu_fwspec generic kernel infrastructure,
implement the IORT based IOMMU configuration for ARM ACPI systems
and hook it up in the ACPI kernel layer that implements DMA
configuration for a device.

Signed-off-by: Lorenzo Pieralisi 
Cc: Hanjun Guo 
Cc: Tomasz Nowicki 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/arm64/iort.c | 96
+++
 drivers/acpi/scan.c   |  7 +++-
 include/linux/acpi_iort.h |  6 +++
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 7c68eb4..55a4ae9 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -19,6 +19,7 @@
 #define pr_fmt(fmt)"ACPI: IORT: " fmt

 #include 
+#include 
 #include 
 #include 
 #include 
@@ -27,6 +28,8 @@

 #define IORT_TYPE_MASK(type)(1 << (type))
 #define IORT_MSI_TYPE(1 << ACPI_IORT_NODE_ITS_GROUP)
+#define IORT_IOMMU_TYPE((1 << ACPI_IORT_NODE_SMMU) |\
+(1 << ACPI_IORT_NODE_SMMU_V3))

 struct iort_its_msi_chip {
 struct list_headlist;
@@ -467,6 +470,99 @@ struct irq_domain *iort_get_device_domain(struct
device *dev, u32 req_id)
 return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
 }

+static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
+{
+u32 *rid = data;
+
+*rid = alias;
+return 0;
+}
+
+static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
+   struct fwnode_handle *fwnode)
+{
+int ret = iommu_fwspec_init(dev, fwnode);
+
+if (!ret)
+ret = iommu_fwspec_add_ids(dev, , 1);
+
+return ret;
+}
+
+static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
+struct acpi_iort_node *node,
+u32 streamid)
+{
+struct fwnode_handle *iort_fwnode = NULL;
+int ret;
+
+if (node) {
+iort_fwnode = iort_get_fwnode(node);
+if (!iort_fwnode)
+return NULL;
+
+ret = arm_smmu_iort_xlate(dev, streamid,
+  iort_fwnode);
+if (!ret)
+return fwspec_iommu_get_ops(iort_fwnode);
+}
+
+return NULL;
+}
+
+/**
+ * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ *
+ * @dev: device to configure
+ *
+ * Returns: iommu_ops pointer on configuration success
+ *  NULL on configuration failure
+ */
+const struct iommu_ops *iort_iommu_configure(struct device *dev)
+{
+struct acpi_iort_node *node, *parent;
+const struct iommu_ops *ops = NULL;
+u32 streamid = 0;
+
+if (dev_is_pci(dev)) {
+struct pci_bus *bus = to_pci_dev(dev)->bus;
+u32 rid;
+
+pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
+   );
+
+node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+  iort_match_node_callback, >dev);
+if (!node)
+return NULL;
+
+parent = iort_node_map_rid(node, rid, ,
+   IORT_IOMMU_TYPE);
+
+ops = iort_iommu_xlate(dev, parent, streamid);
+
+} else {
+int i = 0;
+
+node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+  iort_match_node_callback, dev);
+if (!node)
+return NULL;
+


Nothing wrong with your code here, but wanted to warn you that there
appears to be a bug in iort_match_node_callback() for NAMED_COMPONENTS.

iort_match_node_callback() {
acpi_status status = AE_NOT_FOUND;
...
case ACPI_IORT_NODE_NAMED_COMPONENT: {
...
status = acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, );
if (ACPI_FAILURE(status)) {
dev_warn(dev, "Can't get device full path name\n");
break;
}

ncomp = (struct acpi_iort_named_component *)node->node_data;
if (!strcmp(ncomp->device_name, buf.pointer))
status = AE_OK;

acpi_os_free(buf.pointer);
break;
}
...
return status;
}

Notice how if strcmp() fails, status remains set to the status of the call
to acpi_get_name() which must have been OK since we would have broken out
of the switch statement otherwise. This is causing all manner of platform
devices not even described in the IORT to get hooked up using the IDs of
the first properly iommu-attached NAMED_COMPONENT device found in the IORT.


As I said in previous email, please use the new version of 

Re: [PATCH v5 07/14] drivers: acpi: iort: add support for ARM SMMU platform devices creation

[Hanjun Guo]

On 2016/9/13 15:46, nwatt...@codeaurora.org wrote:

On 2016-09-09 10:23, Lorenzo Pieralisi wrote:

In ARM ACPI systems, IOMMU components are specified through static
IORT table entries. In order to create platform devices for the
corresponding ARM SMMU components, IORT kernel code should be made
able to parse IORT table entries and create platform devices
dynamically.

This patch adds the generic IORT infrastructure required to create
platform devices for ARM SMMUs.

ARM SMMU versions have different resources requirement therefore this
patch also introduces an IORT specific structure (ie iort_iommu_config)
that contains hooks (to be defined when the corresponding ARM SMMU
driver support is added to the kernel) to be used to define the
platform devices names, init the IOMMUs, count their resources and
finally initialize them.

Signed-off-by: Lorenzo Pieralisi 
Cc: Hanjun Guo 
Cc: Tomasz Nowicki 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/arm64/iort.c | 131
++
 1 file changed, 131 insertions(+)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index b89b3d3..e0a9b16 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 struct iort_its_msi_chip {
@@ -424,6 +425,135 @@ struct irq_domain *iort_get_device_domain(struct
device *dev, u32 req_id)
 return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
 }

+struct iort_iommu_config {
+const char *name;
+int (*iommu_init)(struct acpi_iort_node *node);
+bool (*iommu_is_coherent)(struct acpi_iort_node *node);
+int (*iommu_count_resources)(struct acpi_iort_node *node);
+void (*iommu_init_resources)(struct resource *res,
+ struct acpi_iort_node *node);
+};
+
+static __init
+const struct iort_iommu_config *iort_get_iommu_cfg(struct
acpi_iort_node *node)
+{
+return NULL;
+}
+
+/**
+ * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
+ * @fwnode: IORT node associated fwnode handle
+ * @node: Pointer to SMMU ACPI IORT node
+ *
+ * Returns: 0 on success, <0 failure
+ */
+static int __init iort_add_smmu_platform_device(struct fwnode_handle
*fwnode,
+struct acpi_iort_node *node)
+{
+struct platform_device *pdev;
+struct resource *r;
+enum dev_dma_attr attr;
+int ret, count;
+const struct iort_iommu_config *ops = iort_get_iommu_cfg(node);
+
+if (!ops)
+return -ENODEV;
+
+pdev = platform_device_alloc(ops->name, PLATFORM_DEVID_AUTO);
+if (!pdev)
+return PTR_ERR(pdev);
+
+count = ops->iommu_count_resources(node);
+
+r = kcalloc(count, sizeof(*r), GFP_KERNEL);
+if (!r) {
+ret = -ENOMEM;
+goto dev_put;
+}
+
+ops->iommu_init_resources(r, node);
+
+ret = platform_device_add_resources(pdev, r, count);
+/*
+ * Resources are duplicated in platform_device_add_resources,
+ * free their allocated memory
+ */
+kfree(r);
+
+if (ret)
+goto dev_put;
+
+/*
+ * Add a copy of IORT node pointer to platform_data to
+ * be used to retrieve IORT data information.
+ */
+ret = platform_device_add_data(pdev, , sizeof(node));
+if (ret)
+goto dev_put;
+
+pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask),
GFP_KERNEL);
+if (!pdev->dev.dma_mask) {
+ret = -ENOMEM;
+goto dev_put;
+}
+
+pdev->dev.fwnode = fwnode;
+
+/*
+ * Set default dma mask value for the table walker,
+ * to be overridden on probing with correct value.
+ */
+*pdev->dev.dma_mask = DMA_BIT_MASK(32);
+pdev->dev.coherent_dma_mask = *pdev->dev.dma_mask;
+
+attr = ops->iommu_is_coherent(node) ?
+ DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
+
+/* Configure DMA for the page table walker */
+acpi_dma_configure(>dev, attr);
+
+ret = platform_device_add(pdev);
+if (ret)
+goto dma_deconfigure;
+
+return 0;
+
+dma_deconfigure:
+acpi_dma_deconfigure(>dev);
+kfree(pdev->dev.dma_mask);
+
+dev_put:
+platform_device_put(pdev);
+
+return ret;
+}
+
+static acpi_status __init iort_match_iommu_callback(struct
acpi_iort_node *node,
+void *context)
+{
+int ret;
+struct fwnode_handle *fwnode;
+
+fwnode = iort_get_fwnode(node);
+
+if (!fwnode)
+return AE_NOT_FOUND;
+
+ret = iort_add_smmu_platform_device(fwnode, node);
+if (ret) {
+pr_err("Error in platform device creation\n");
+return AE_ERROR;
+}
+
+return AE_OK;
+}
+
+static void __init iort_smmu_init(void)
+{
+iort_scan_node(ACPI_IORT_NODE_SMMU, iort_match_iommu_callback,
NULL);
+iort_scan_node(ACPI_IORT_NODE_SMMU_V3, iort_match_iommu_callback,
NULL);


Since iort_scan_node() returns after the first successful match 

Re: [PATCH v5 14/14] drivers: acpi: iort: introduce iort_iommu_configure

[Nate Watterson]

On 2016-09-09 10:23, Lorenzo Pieralisi wrote:

DT based systems have a generic kernel API to configure IOMMUs
for devices (ie of_iommu_configure()).

On ARM based ACPI systems, the of_iommu_configure() equivalent can
be implemented atop ACPI IORT kernel API, with the corresponding
functions to map device identifiers to IOMMUs and retrieve the
corresponding IOMMU operations necessary for DMA operations set-up.

By relying on the iommu_fwspec generic kernel infrastructure,
implement the IORT based IOMMU configuration for ARM ACPI systems
and hook it up in the ACPI kernel layer that implements DMA
configuration for a device.

Signed-off-by: Lorenzo Pieralisi 
Cc: Hanjun Guo 
Cc: Tomasz Nowicki 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/arm64/iort.c | 96 
+++

 drivers/acpi/scan.c   |  7 +++-
 include/linux/acpi_iort.h |  6 +++
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 7c68eb4..55a4ae9 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -19,6 +19,7 @@
 #define pr_fmt(fmt)"ACPI: IORT: " fmt

 #include 
+#include 
 #include 
 #include 
 #include 
@@ -27,6 +28,8 @@

 #define IORT_TYPE_MASK(type)   (1 << (type))
 #define IORT_MSI_TYPE  (1 << ACPI_IORT_NODE_ITS_GROUP)
+#define IORT_IOMMU_TYPE((1 << ACPI_IORT_NODE_SMMU) | \
+   (1 << ACPI_IORT_NODE_SMMU_V3))

 struct iort_its_msi_chip {
struct list_headlist;
@@ -467,6 +470,99 @@ struct irq_domain *iort_get_device_domain(struct
device *dev, u32 req_id)
return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
 }

+static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
+{
+   u32 *rid = data;
+
+   *rid = alias;
+   return 0;
+}
+
+static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
+  struct fwnode_handle *fwnode)
+{
+   int ret = iommu_fwspec_init(dev, fwnode);
+
+   if (!ret)
+   ret = iommu_fwspec_add_ids(dev, , 1);
+
+   return ret;
+}
+
+static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
+   struct acpi_iort_node *node,
+   u32 streamid)
+{
+   struct fwnode_handle *iort_fwnode = NULL;
+   int ret;
+
+   if (node) {
+   iort_fwnode = iort_get_fwnode(node);
+   if (!iort_fwnode)
+   return NULL;
+
+   ret = arm_smmu_iort_xlate(dev, streamid,
+ iort_fwnode);
+   if (!ret)
+   return fwspec_iommu_get_ops(iort_fwnode);
+   }
+
+   return NULL;
+}
+
+/**
+ * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ *
+ * @dev: device to configure
+ *
+ * Returns: iommu_ops pointer on configuration success
+ *  NULL on configuration failure
+ */
+const struct iommu_ops *iort_iommu_configure(struct device *dev)
+{
+   struct acpi_iort_node *node, *parent;
+   const struct iommu_ops *ops = NULL;
+   u32 streamid = 0;
+
+   if (dev_is_pci(dev)) {
+   struct pci_bus *bus = to_pci_dev(dev)->bus;
+   u32 rid;
+
+   pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
+  );
+
+   node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+ iort_match_node_callback, >dev);
+   if (!node)
+   return NULL;
+
+   parent = iort_node_map_rid(node, rid, ,
+  IORT_IOMMU_TYPE);
+
+   ops = iort_iommu_xlate(dev, parent, streamid);
+
+   } else {
+   int i = 0;
+
+   node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+ iort_match_node_callback, dev);
+   if (!node)
+   return NULL;
+


Nothing wrong with your code here, but wanted to warn you that there
appears to be a bug in iort_match_node_callback() for NAMED_COMPONENTS.

iort_match_node_callback() {
acpi_status status = AE_NOT_FOUND;
...
case ACPI_IORT_NODE_NAMED_COMPONENT: {
...
status = acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, );
if (ACPI_FAILURE(status)) {
dev_warn(dev, "Can't get device full path name\n");
break;
}

ncomp = (struct acpi_iort_named_component *)node->node_data;
if (!strcmp(ncomp->device_name, buf.pointer))
status = AE_OK;

acpi_os_free(buf.pointer);
break;
}
...
return status;

Re: [PATCH v5 07/14] drivers: acpi: iort: add support for ARM SMMU platform devices creation

[nwatters]

On 2016-09-09 10:23, Lorenzo Pieralisi wrote:

In ARM ACPI systems, IOMMU components are specified through static
IORT table entries. In order to create platform devices for the
corresponding ARM SMMU components, IORT kernel code should be made
able to parse IORT table entries and create platform devices
dynamically.

This patch adds the generic IORT infrastructure required to create
platform devices for ARM SMMUs.

ARM SMMU versions have different resources requirement therefore this
patch also introduces an IORT specific structure (ie iort_iommu_config)
that contains hooks (to be defined when the corresponding ARM SMMU
driver support is added to the kernel) to be used to define the
platform devices names, init the IOMMUs, count their resources and
finally initialize them.

Signed-off-by: Lorenzo Pieralisi 
Cc: Hanjun Guo 
Cc: Tomasz Nowicki 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/arm64/iort.c | 131 
++

 1 file changed, 131 insertions(+)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index b89b3d3..e0a9b16 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 struct iort_its_msi_chip {
@@ -424,6 +425,135 @@ struct irq_domain *iort_get_device_domain(struct
device *dev, u32 req_id)
return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
 }

+struct iort_iommu_config {
+   const char *name;
+   int (*iommu_init)(struct acpi_iort_node *node);
+   bool (*iommu_is_coherent)(struct acpi_iort_node *node);
+   int (*iommu_count_resources)(struct acpi_iort_node *node);
+   void (*iommu_init_resources)(struct resource *res,
+struct acpi_iort_node *node);
+};
+
+static __init
+const struct iort_iommu_config *iort_get_iommu_cfg(struct 
acpi_iort_node *node)

+{
+   return NULL;
+}
+
+/**
+ * iort_add_smmu_platform_device() - Allocate a platform device for 
SMMU

+ * @fwnode: IORT node associated fwnode handle
+ * @node: Pointer to SMMU ACPI IORT node
+ *
+ * Returns: 0 on success, <0 failure
+ */
+static int __init iort_add_smmu_platform_device(struct fwnode_handle 
*fwnode,

+   struct acpi_iort_node *node)
+{
+   struct platform_device *pdev;
+   struct resource *r;
+   enum dev_dma_attr attr;
+   int ret, count;
+   const struct iort_iommu_config *ops = iort_get_iommu_cfg(node);
+
+   if (!ops)
+   return -ENODEV;
+
+   pdev = platform_device_alloc(ops->name, PLATFORM_DEVID_AUTO);
+   if (!pdev)
+   return PTR_ERR(pdev);
+
+   count = ops->iommu_count_resources(node);
+
+   r = kcalloc(count, sizeof(*r), GFP_KERNEL);
+   if (!r) {
+   ret = -ENOMEM;
+   goto dev_put;
+   }
+
+   ops->iommu_init_resources(r, node);
+
+   ret = platform_device_add_resources(pdev, r, count);
+   /*
+* Resources are duplicated in platform_device_add_resources,
+* free their allocated memory
+*/
+   kfree(r);
+
+   if (ret)
+   goto dev_put;
+
+   /*
+* Add a copy of IORT node pointer to platform_data to
+* be used to retrieve IORT data information.
+*/
+   ret = platform_device_add_data(pdev, , sizeof(node));
+   if (ret)
+   goto dev_put;
+
+	pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), 
GFP_KERNEL);

+   if (!pdev->dev.dma_mask) {
+   ret = -ENOMEM;
+   goto dev_put;
+   }
+
+   pdev->dev.fwnode = fwnode;
+
+   /*
+* Set default dma mask value for the table walker,
+* to be overridden on probing with correct value.
+*/
+   *pdev->dev.dma_mask = DMA_BIT_MASK(32);
+   pdev->dev.coherent_dma_mask = *pdev->dev.dma_mask;
+
+   attr = ops->iommu_is_coherent(node) ?
+DEV_DMA_COHERENT : DEV_DMA_NON_COHERENT;
+
+   /* Configure DMA for the page table walker */
+   acpi_dma_configure(>dev, attr);
+
+   ret = platform_device_add(pdev);
+   if (ret)
+   goto dma_deconfigure;
+
+   return 0;
+
+dma_deconfigure:
+   acpi_dma_deconfigure(>dev);
+   kfree(pdev->dev.dma_mask);
+
+dev_put:
+   platform_device_put(pdev);
+
+   return ret;
+}
+
+static acpi_status __init iort_match_iommu_callback(struct
acpi_iort_node *node,
+   void *context)
+{
+   int ret;
+   struct fwnode_handle *fwnode;
+
+   fwnode = iort_get_fwnode(node);
+
+   if (!fwnode)
+   return AE_NOT_FOUND;
+
+   ret = iort_add_smmu_platform_device(fwnode, node);
+   if (ret) {
+   pr_err("Error in platform device creation\n");
+   return AE_ERROR;
+   }
+