diff mbox

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

Message ID 20160909142343.13314-8-lorenzo.pieralisi@arm.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Lorenzo Pieralisi Sept. 9, 2016, 2:23 p.m. UTC
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 <lorenzo.pieralisi@arm.com>
Cc: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Tomasz Nowicki <tn@semihalf.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 drivers/acpi/arm64/iort.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)

Comments

Nate Watterson Sept. 13, 2016, 7:46 a.m. UTC | #1
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 <lorenzo.pieralisi@arm.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Tomasz Nowicki <tn@semihalf.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  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 <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/pci.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
> 
>  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, &node, 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(&pdev->dev, attr);
> +
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		goto dma_deconfigure;
> +
> +	return 0;
> +
> +dma_deconfigure:
> +	acpi_dma_deconfigure(&pdev->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 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.

> +}
> +
>  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();
>  }
Hanjun Guo Sept. 13, 2016, 8:15 a.m. UTC | #2
On 2016/9/13 15:46, nwatters@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 <lorenzo.pieralisi@arm.com>
>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>> Cc: Tomasz Nowicki <tn@semihalf.com>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> ---
>>  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 <linux/kernel.h>
>>  #include <linux/list.h>
>>  #include <linux/pci.h>
>> +#include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>
>>  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, &node, 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(&pdev->dev, attr);
>> +
>> +    ret = platform_device_add(pdev);
>> +    if (ret)
>> +        goto dma_deconfigure;
>> +
>> +    return 0;
>> +
>> +dma_deconfigure:
>> +    acpi_dma_deconfigure(&pdev->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 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.

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Sept. 13, 2016, 8:24 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo Sept. 13, 2016, 8:48 a.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robin Murphy Sept. 13, 2016, 3:25 p.m. UTC | #5
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 <lorenzo.pieralisi@arm.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Tomasz Nowicki <tn@semihalf.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  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 <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/pci.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
>  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, &node, 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(&pdev->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(&pdev->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();
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Sept. 13, 2016, 4:29 p.m. UTC | #6
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, &node, 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(&pdev->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(&pdev->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();
> >  }
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/pci.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 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, &node, 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(&pdev->dev, attr);
+
+	ret = platform_device_add(pdev);
+	if (ret)
+		goto dma_deconfigure;
+
+	return 0;
+
+dma_deconfigure:
+	acpi_dma_deconfigure(&pdev->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();
 }