[v5,4/4] add test/example driver for ux500 platform

From: Benjamin Gaignard <benjamin.gaignard@linaro.org>

DO NOT MERGE
ux500-ion driver is provided as example.
Define 2 CMA heaps, one on a specific CMA region reserved at boot time
the other will use the default CMA region.
Since we can have multiple instances of this driver while only one ion device
can be instanciated, we need to take care of ion_heap structure reallocation.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |   77 +++++++++++++++++++
 drivers/gpu/ion/Kconfig            |    5 ++
 drivers/gpu/ion/Makefile           |    1 +
 drivers/gpu/ion/ux500/Makefile     |    1 +
 drivers/gpu/ion/ux500/ux500_ion.c  |  142 ++++++++++++++++++++++++++++++++++++
 5 files changed, 226 insertions(+)
 create mode 100644 drivers/gpu/ion/ux500/Makefile
 create mode 100644 drivers/gpu/ion/ux500/ux500_ion.c

Hi,

Can we modify the struct ion_platform_heap to add a priv field which
can be used for passing the
device?
1. The ion_heap_create can be retained as such instead of having a
ion_heap_create_full newly
defined.
2. We can have multiple cma heaps within the same ion device. If a
single cma heap id is used per
ion device, board file can initialize the ion_platform_heap priv
member to ion platform_device dev
as is being done in the current set of patches. Else, we could declare
dummy devices in board file
and pass them as priv member of ion_platform heap.
Usecase: Define multiple cma heaps for different SDRAM banks and
userspace could use same ion
device to try allocating from a specific heap for avoid RW switching
overhead or any of the cma heaps.

 - Nishanth Peethambaran


On Tue, Jun 12, 2012 at 8:19 AM,  <benjamin.gaignard@stericsson.com> wrote:
> From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>
> DO NOT MERGE
> ux500-ion driver is provided as example.
> Define 2 CMA heaps, one on a specific CMA region reserved at boot time
> the other will use the default CMA region.
> Since we can have multiple instances of this driver while only one ion device
> can be instanciated, we need to take care of ion_heap structure reallocation.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  arch/arm/mach-ux500/board-mop500.c |   77 +++++++++++++++++++
>  drivers/gpu/ion/Kconfig            |    5 ++
>  drivers/gpu/ion/Makefile           |    1 +
>  drivers/gpu/ion/ux500/Makefile     |    1 +
>  drivers/gpu/ion/ux500/ux500_ion.c  |  142 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 226 insertions(+)
>  create mode 100644 drivers/gpu/ion/ux500/Makefile
>  create mode 100644 drivers/gpu/ion/ux500/ux500_ion.c
>
> diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
> index 77d03c1..e4e75ac 100644
> --- a/arch/arm/mach-ux500/board-mop500.c
> +++ b/arch/arm/mach-ux500/board-mop500.c
> @@ -30,6 +30,11 @@
>  #include <linux/gpio_keys.h>
>  #include <linux/delay.h>
>
> +#ifdef CONFIG_ION
> +#include <linux/ion.h>
> +#include <linux/dma-contiguous.h>
> +#endif
> +
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>
> @@ -54,6 +59,62 @@
>  #include "board-mop500.h"
>  #include "board-mop500-regulators.h"
>
> +#ifdef CONFIG_ION
> +static u64 snowball_dmamask = DMA_BIT_MASK(32);
> +
> +static struct ion_platform_heap snowball_ion_heap1[] = {
> +       [0] = {
> +               .type = ION_HEAP_TYPE_DMA,
> +               .id = 1,
> +               .name = "ion-cma-heap-1",
> +               .base = 0,
> +               .size = (16 * SZ_1M),
> +       },
> +};
> +
> +static struct ion_platform_data snowball_ion_data1 = {
> +       .heaps = snowball_ion_heap1,
> +       .nr = ARRAY_SIZE(snowball_ion_heap1),
> +};
> +
> +static struct platform_device snowball_ion_device1 = {
> +       .name = "ion-ux500",
> +       .id = 1,
> +       .dev = {
> +               .dma_mask = &snowball_dmamask,
> +               .coherent_dma_mask = DMA_BIT_MASK(32),
> +               .platform_data = &snowball_ion_data1,
> +       },
> +       .num_resources = 0,
> +};
> +
> +static struct ion_platform_heap snowball_ion_heap2[] = {
> +       [0] = {
> +               .type = ION_HEAP_TYPE_DMA,
> +               .id = 2,
> +               .name = "ion-cma-heap-2",
> +               .base = 0,
> +               .size = (16 * SZ_1M),
> +       },
> +};
> +
> +static struct ion_platform_data snowball_ion_data2 = {
> +       .heaps = snowball_ion_heap2,
> +       .nr = ARRAY_SIZE(snowball_ion_heap2),
> +};
> +
> +static struct platform_device snowball_ion_device2 = {
> +       .name = "ion-ux500",
> +       .id = 2,
> +       .dev = {
> +               .dma_mask = &snowball_dmamask,
> +               .coherent_dma_mask = DMA_BIT_MASK(32),
> +               .platform_data = &snowball_ion_data2,
> +       },
> +       .num_resources = 0,
> +};
> +#endif
> +
>  static struct gpio_led snowball_led_array[] = {
>        {
>                .name = "user_led",
> @@ -607,8 +668,21 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
>        &snowball_key_dev,
>        &snowball_sbnet_dev,
>        &ab8500_device,
> +#ifdef CONFIG_ION
> +       &snowball_ion_device1,
> +       &snowball_ion_device2,
> +#endif
>  };
>
> +#ifdef CONFIG_ION
> +static void __init mop500_reserve(void)
> +{
> +       dma_declare_contiguous(&snowball_ion_device1.dev,
> +                       snowball_ion_heap1[0].size,
> +                       snowball_ion_heap1[0].base, 0);
> +}
> +#endif
> +
>  static void __init mop500_init_machine(void)
>  {
>        struct device *parent = NULL;
> @@ -741,6 +815,9 @@ MACHINE_START(SNOWBALL, "Calao Systems Snowball platform")
>        .timer          = &ux500_timer,
>        .handle_irq     = gic_handle_irq,
>        .init_machine   = snowball_init_machine,
> +#ifdef CONFIG_ION
> +       .reserve        = mop500_reserve,
> +#endif
>  MACHINE_END
>
>  #ifdef CONFIG_MACH_UX500_DT
> diff --git a/drivers/gpu/ion/Kconfig b/drivers/gpu/ion/Kconfig
> index b5bfdb4..bfe572d 100644
> --- a/drivers/gpu/ion/Kconfig
> +++ b/drivers/gpu/ion/Kconfig
> @@ -11,3 +11,8 @@ config ION_TEGRA
>        help
>          Choose this option if you wish to use ion on an nVidia Tegra.
>
> +config ION_UX500
> +       tristate "Ion for ux500"
> +       depends on ARCH_U8500 && ION
> +       help
> +         Choose this option if you wish to use ion on ux500 platforms.
> diff --git a/drivers/gpu/ion/Makefile b/drivers/gpu/ion/Makefile
> index 32d3385..a7ea570 100644
> --- a/drivers/gpu/ion/Makefile
> +++ b/drivers/gpu/ion/Makefile
> @@ -2,3 +2,4 @@ ion-driver-objs += ion.o ion_heap.o ion_system_heap.o ion_carveout_heap.o
>  obj-$(CONFIG_ION) += ion-driver.o
>  obj-$(CONFIG_CMA) += ion_cma_heap.o
>  obj-$(CONFIG_ION_TEGRA) += tegra/
> +obj-$(CONFIG_ION_UX500) += ux500/
> diff --git a/drivers/gpu/ion/ux500/Makefile b/drivers/gpu/ion/ux500/Makefile
> new file mode 100644
> index 0000000..691c600
> --- /dev/null
> +++ b/drivers/gpu/ion/ux500/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ION_UX500) += ux500_ion.o
> diff --git a/drivers/gpu/ion/ux500/ux500_ion.c b/drivers/gpu/ion/ux500/ux500_ion.c
> new file mode 100644
> index 0000000..c7560c7
> --- /dev/null
> +++ b/drivers/gpu/ion/ux500/ux500_ion.c
> @@ -0,0 +1,142 @@
> +/*
> + * drivers/gpu/ion/ux500/ux500_ion.c
> + *
> + * Copyright (C) Linaro 2012
> + * Author: <benjamin.gaignard@linaro.org> for ST-Ericsson.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/ion.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include "../ion_priv.h"
> +
> +struct ion_device *ux500_ion_device;
> +int num_heaps;
> +struct ion_heap **ux500_ion_heaps;
> +
> +int ux500_ion_probe(struct platform_device *pdev)
> +{
> +       struct ion_platform_data *pdata = pdev->dev.platform_data;
> +       int err;
> +       int i, previous_heaps_count = 0;
> +
> +       /* test if it is the first time we try to create ions heaps */
> +       if (num_heaps == 0) {
> +               num_heaps = pdata->nr;
> +
> +               ux500_ion_heaps =
> +                   kzalloc(sizeof(struct ion_heap *) * pdata->nr, GFP_KERNEL);
> +               memset(ux500_ion_heaps, 0,
> +                      sizeof(struct ion_heap *) * pdata->nr);
> +
> +               dev_dbg(&pdev->dev, "create ion device\n");
> +               ux500_ion_device = ion_device_create(NULL);
> +               if (IS_ERR_OR_NULL(ux500_ion_device)) {
> +                       kfree(ux500_ion_heaps);
> +                       num_heaps = 0;
> +                       dev_err(&pdev->dev, "failed to create ion device\n");
> +                       return PTR_ERR(ux500_ion_device);
> +               }
> +       } else {
> +               struct ion_heap **new_ux500_ion_heaps;
> +
> +               previous_heaps_count = num_heaps;
> +               num_heaps += pdata->nr;
> +
> +               /* allocate a bigger array of ion_heap */
> +               new_ux500_ion_heaps =
> +                   kzalloc(sizeof(struct ion_heap *) * num_heaps, GFP_KERNEL);
> +               memset(new_ux500_ion_heaps, 0,
> +                      sizeof(struct ion_heap *) * num_heaps);
> +
> +               dev_dbg(&pdev->dev, "realloc ion heap\n");
> +               /* copy old heap array info into the new one */
> +               for (i = 0; i < previous_heaps_count; i++)
> +                       new_ux500_ion_heaps[i] = ux500_ion_heaps[i];
> +
> +               /* free old heap array and swap it with the new one */
> +               kfree(ux500_ion_heaps);
> +               ux500_ion_heaps = new_ux500_ion_heaps;
> +       }
> +
> +       /* create the heaps as specified in the board file */
> +       for (i = previous_heaps_count; i < num_heaps; i++) {
> +               struct ion_platform_heap *heap_data =
> +                   &pdata->heaps[i - previous_heaps_count];
> +
> +               ux500_ion_heaps[i] =
> +                       ion_heap_create_full(heap_data, &pdev->dev);
> +
> +               if (IS_ERR_OR_NULL(ux500_ion_heaps[i])) {
> +                       err = PTR_ERR(ux500_ion_heaps[i]);
> +                       ux500_ion_heaps[i] = NULL;
> +                       dev_err(&pdev->dev,
> +                               "failed to create heap type %d id %d\n",
> +                               heap_data->type, heap_data->id);
> +                       goto err;
> +               }
> +               ion_device_add_heap(ux500_ion_device, ux500_ion_heaps[i]);
> +       }
> +
> +       platform_set_drvdata(pdev, ux500_ion_device);
> +
> +       return 0;
> +err:
> +       for (i = 0; i < num_heaps; i++) {
> +               if (ux500_ion_heaps[i])
> +                       ion_heap_destroy(ux500_ion_heaps[i]);
> +       }
> +       kfree(ux500_ion_heaps);
> +       return err;
> +}
> +
> +int ux500_ion_remove(struct platform_device *pdev)
> +{
> +       struct ion_device *idev = platform_get_drvdata(pdev);
> +       int i;
> +
> +       ion_device_destroy(idev);
> +       for (i = 0; i < num_heaps; i++)
> +               ion_heap_destroy(ux500_ion_heaps[i]);
> +       kfree(ux500_ion_heaps);
> +       return 0;
> +}
> +
> +static struct platform_driver ux500_ion_driver = {
> +       .probe = ux500_ion_probe,
> +       .remove = ux500_ion_remove,
> +       .driver = {
> +                  .name = "ion-ux500",
> +       }
> +};
> +
> +static int __init ux500_ion_init(void)
> +{
> +       ux500_ion_device = NULL;
> +       num_heaps = 0;
> +       ux500_ion_heaps = NULL;
> +
> +       return platform_driver_register(&ux500_ion_driver);
> +}
> +
> +static void __exit ux500_ion_exit(void)
> +{
> +       if (ux500_ion_device)
> +               ion_device_destroy(ux500_ion_device);
> +
> +       platform_driver_unregister(&ux500_ion_driver);
> +}
> +
> +module_init(ux500_ion_init);
> +module_exit(ux500_ion_exit);
> --
> 1.7.10
>
>
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

Hello,

The addition of a private field in ion_platform_heap has been discussed in
the previous versions of the patches:
http://lists.linaro.org/pipermail/linaro-mm-sig/2012-March/001431.html

Each CMA area need to be linked with one device, so we need to have
multiple ion-driver device beacuse there is only one ION device.
If we don't link CMA to one device driver, all calls to dma_alloc_coherent
will go to the default CMA area.

I'm not sure about your use case, but if you mean that you want to define
multiple heaps and let the ION allocate the memory in one of them, it is
already what ION does in ion_alloc function by checking heap_mask and flags.

Benjamin

Hi,

I missed out that you are stil creating only one ION miscdevice though
multiple ion-driver devices are created.

Isn't it better to add a priv field in the platform heap than
modifying the heap creation API? The priv field can be then
used for extended purposes for custom heaps (board file and heap
implementation needs to know what is being
passed in the priv field).

 - Nishanth Peethambaran



On Wed, Jun 13, 2012 at 9:05 AM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> Hello,
>
> The addition of a private field in ion_platform_heap has been discussed in
> the previous versions of the patches:
> http://lists.linaro.org/pipermail/linaro-mm-sig/2012-March/001431.html
>
> Each CMA area need to be linked with one device, so we need to have multiple
> ion-driver device beacuse there is only one ION device.
> If we don't link CMA to one device driver, all calls to dma_alloc_coherent
> will go to the default CMA area.
>
> I'm not sure about your use case, but if you mean that you want to define
> multiple heaps and let the ION allocate the memory in one of them, it is
> already what ION does in ion_alloc function by checking heap_mask and flags.
>
> Benjamin

Hi,

I agree with Nishanth here. Internally, we've added a private field to 
the platform_heap_data to pass generic board specific data to heaps. 
Similar to carveout heap:


static struct ion_platform_heap snowball_ion_heap1[] = {
	[0] = {
		.type = ION_HEAP_TYPE_DMA,
		.id = 1,
		.name = "ion-cma-heap-1",
		.base = 0,
		.size = (16 * SZ_1M),
		.priv = &snowball_ion_device1.dev
	},
};

----

struct ion_cma_heap {
	struct ion_heap heap;
	struct device *dev;
}

struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
{
         struct ion_cma_heap *heap;

	heap = kzalloc(sizeof(*heap), GFP_KERNEL);
	if (!heap)
		return ERR_PTR(-ENOMEM);

	
	heap->dev = data->priv;
	heap->heap.ops = &ion_cma_ops;
	heap->heap.type = ION_HEAP_TYPE_DMA;

	return &heap->heap;
}

And all other accesses become container_of to get the heap. This makes 
it easier to pass in other data that may not be directly linked to the 
platform device.

Laura

On 6/13/2012 7:31 PM, Nishanth Peethambaran wrote:
> Hi,
>
> I missed out that you are stil creating only one ION miscdevice though
> multiple ion-driver devices are created.
>
> Isn't it better to add a priv field in the platform heap than
> modifying the heap creation API? The priv field can be then
> used for extended purposes for custom heaps (board file and heap
> implementation needs to know what is being
> passed in the priv field).
>
>   - Nishanth Peethambaran
>
>
>
> On Wed, Jun 13, 2012 at 9:05 AM, Benjamin Gaignard
> <benjamin.gaignard@linaro.org>  wrote:
>> Hello,
>>
>> The addition of a private field in ion_platform_heap has been discussed in
>> the previous versions of the patches:
>> http://lists.linaro.org/pipermail/linaro-mm-sig/2012-March/001431.html
>>
>> Each CMA area need to be linked with one device, so we need to have multiple
>> ion-driver device beacuse there is only one ION device.
>> If we don't link CMA to one device driver, all calls to dma_alloc_coherent
>> will go to the default CMA area.
>>
>> I'm not sure about your use case, but if you mean that you want to define
>> multiple heaps and let the ION allocate the memory in one of them, it is
>> already what ION does in ion_alloc function by checking heap_mask and flags.
>>
>> Benjamin
>
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

Hi,

I have try to minimize the change in ion_heap_platform structure, that why
I haven't add a private field.

If we put the device in a private field we have to declare it before the
heap but in sametime we need put the heap in device platform_data.
I don't have spend lot of time on it but for me it looks like a infinite
loop... maybe I'm wrong.

Rebecca, do you have some guidelines/recommandations about this problem ?

Benjamin

Hi,

The priv field could be initialized before board initialization gets
called also.

- Nishanth Peethambaran
  +91-9448074166



On Fri, Jun 15, 2012 at 1:42 AM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> Hi,
>
> I have try to minimize the change in ion_heap_platform structure, that why I
> haven't add a private field.
>
> If we put the device in a private field we have to declare it before the
> heap but in sametime we need put the heap in device platform_data.
> I don't have spend lot of time on it but for me it looks like a infinite
> loop... maybe I'm wrong.
>
> Rebecca, do you have some guidelines/recommandations about this problem ?
>
> Benjamin
>
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
>

Hi,

The priv field could be initialized before board initialization gets
called also.

 - Nishanth Peethambaran

On Fri, Jun 15, 2012 at 1:42 AM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> Hi,
>
> I have try to minimize the change in ion_heap_platform structure, that why I
> haven't add a private field.
>
> If we put the device in a private field we have to declare it before the
> heap but in sametime we need put the heap in device platform_data.
> I don't have spend lot of time on it but for me it looks like a infinite
> loop... maybe I'm wrong.
>
> Rebecca, do you have some guidelines/recommandations about this problem ?
>
> Benjamin