Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753300AbbFMMdX (ORCPT ); Sat, 13 Jun 2015 08:33:23 -0400 Received: from forward4l.mail.yandex.net ([84.201.143.137]:52435 "EHLO forward4l.mail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751958AbbFMMdQ (ORCPT ); Sat, 13 Jun 2015 08:33:16 -0400 To: Greg Kroah-Hartman Subject: Re: [PATCH v4 1/2] staging: ion: Add generic ion-physmem driver X-PHP-Originating-Script: 501:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Sat, 13 Jun 2015 15:33:09 +0300 From: Andrew Cc: Sudip Mukherjee , pebolle@tiscali.nl, =?UTF-8?Q?Arve_Hj=EF=BF=BDnnev=EF=BF=BDg?= , Riley Andrews , Chen Gang , Fabian Frederick , Android Kernel Team , linux-kernel@vger.kernel.org In-Reply-To: <20150613001648.GA5234@kroah.com> References: <1433861905-9847-1-git-send-email-andrew@ncrmnt.org> <1433861905-9847-2-git-send-email-andrew@ncrmnt.org> <20150613001648.GA5234@kroah.com> Message-ID: <0dd898d126214953e368b99cfc121777@mail.ncrmnt.org> User-Agent: Roundcube Webmail/1.0.2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10879 Lines: 356 Greg Kroah-Hartman писал 13.06.2015 03:16: > On Tue, Jun 09, 2015 at 05:58:24PM +0300, Andrew Andrianov wrote: >> This patch adds a generic ion driver that allows >> ion heaps to be added via devicetree. It provides >> a simple and generic way to feed physical memory regions >> to ion without writing a custom driver, e.g. >> >> ion_sram: ion@0x00100000 { >> compatible = "ion,physmem"; >> reg = <0x00100000 0x40000>; >> reg-names = "memory"; >> ion-heap-id = <1>; >> ion-heap-type = ; >> ion-heap-align = <0x10>; >> ion-heap-name = "SRAM"; >> }; >> >> Signed-off-by: Andrew Andrianov >> --- >> drivers/staging/android/ion/Kconfig | 7 + >> drivers/staging/android/ion/Makefile | 5 +- >> drivers/staging/android/ion/ion_physmem.c | 229 >> ++++++++++++++++++++++++++++++ >> include/dt-bindings/ion,physmem.h | 17 +++ >> 4 files changed, 256 insertions(+), 2 deletions(-) >> create mode 100644 drivers/staging/android/ion/ion_physmem.c >> create mode 100644 include/dt-bindings/ion,physmem.h >> >> diff --git a/drivers/staging/android/ion/Kconfig >> b/drivers/staging/android/ion/Kconfig >> index 3452346..c558cf8 100644 >> --- a/drivers/staging/android/ion/Kconfig >> +++ b/drivers/staging/android/ion/Kconfig >> @@ -33,3 +33,10 @@ config ION_TEGRA >> help >> Choose this option if you wish to use ion on an nVidia Tegra. >> >> +config ION_PHYSMEM >> + bool "Generic PhysMem ION driver" >> + depends on ION >> + help >> + Provides a generic ION driver that registers the >> + /dev/ion device with heaps from devicetree entries. >> + This heaps are made of chunks of physical memory > > That last sentance doesn't make sense to me :( Neither it does to me, will fix ASAP. Must some old relic that went unnoticed. > > And have you tested this driver build on a non-DT build (like x86-64)? > Nothing beyond just building it on x86_64_defconfig + CONFIG_ANDROID + CONFIG_ION* and a few tests on private older branches for ARM with no devicetree (ARM Realview EB). >> diff --git a/drivers/staging/android/ion/Makefile >> b/drivers/staging/android/ion/Makefile >> index b56fd2b..ac9856d 100644 >> --- a/drivers/staging/android/ion/Makefile >> +++ b/drivers/staging/android/ion/Makefile >> @@ -5,6 +5,7 @@ ifdef CONFIG_COMPAT >> obj-$(CONFIG_ION) += compat_ion.o >> endif >> >> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o >> -obj-$(CONFIG_ION_TEGRA) += tegra/ >> +obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o >> +obj-$(CONFIG_ION_PHYSMEM) += ion_physmem.o >> +obj-$(CONFIG_ION_TEGRA) += tegra/ >> >> diff --git a/drivers/staging/android/ion/ion_physmem.c >> b/drivers/staging/android/ion/ion_physmem.c >> new file mode 100644 >> index 0000000..ba5135f >> --- /dev/null >> +++ b/drivers/staging/android/ion/ion_physmem.c >> @@ -0,0 +1,229 @@ >> +/* >> + * Copyright (C) 2015 RC Module >> + * Andrew Andrianov >> + * >> + * This program is free software; you can redistribute it and/or >> modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * Generic devicetree physmem driver for ION Memory Manager >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include "ion.h" >> +#include "ion_priv.h" >> + >> +#define DRVNAME "ion-physmem" >> + >> +static struct ion_device *idev; >> +static uint32_t claimed_heap_ids; >> + >> +static const struct of_device_id of_match_table[] = { >> + { .compatible = "ion,physmem", }, >> + { /* end of list */ } >> +}; >> + >> +struct physmem_ion_dev { >> + struct ion_platform_heap data; >> + struct ion_heap *heap; >> + int need_free_coherent; >> + void *freepage_ptr; >> + struct clk *clk; >> + uint32_t heap_id; >> +}; >> + >> +static int ion_physmem_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + u32 ion_heap_id, ion_heap_align, ion_heap_type; >> + ion_phys_addr_t addr; >> + size_t size = 0; >> + const char *ion_heap_name = NULL; >> + struct resource *res; >> + struct physmem_ion_dev *ipdev; >> + >> + /* >> + Looks like we can only have one ION device in our system. >> + Therefore we call ion_device_create on first probe and only >> + add heaps to it on subsequent probe calls. >> + FixMe: >> + 1. Do we need to hold a spinlock here? >> + 2. Can several probes race here on SMP? >> + */ >> + >> + if (!idev) { >> + idev = ion_device_create(NULL); >> + dev_info(&pdev->dev, "ION PhysMem Driver. (c) RC Module 2015\n"); >> + if (!idev) >> + return -ENOMEM; >> + } >> + >> + ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL); >> + if (!ipdev) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, ipdev); >> + >> + /* Read out name first for the sake of sane error-reporting */ >> + ret = of_property_read_string(pdev->dev.of_node, "ion-heap-name", >> + &ion_heap_name); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-id", >> + &ion_heap_id); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + /* Check id to be sane first */ >> + if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) { >> + dev_err(&pdev->dev, "bad heap id specified: %d\n", >> + ion_heap_id); >> + goto errfreeipdev; >> + } >> + >> + if ((1 << ion_heap_id) & claimed_heap_ids) { >> + dev_err(&pdev->dev, "heap id %d is already claimed\n", >> + ion_heap_id); >> + goto errfreeipdev; >> + } >> + >> + ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-type", >> + &ion_heap_type); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-align", >> + &ion_heap_align); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory"); >> + /* Not always needed, throw no error if missing */ >> + if (res) { >> + /* Fill in some defaults */ >> + addr = res->start; >> + size = resource_size(res); >> + } >> + >> + switch (ion_heap_type) { >> + case ION_HEAP_TYPE_DMA: >> + if (res) { >> + ret = dma_declare_coherent_memory(&pdev->dev, >> + res->start, >> + res->start, >> + resource_size(res), >> + DMA_MEMORY_MAP | >> + DMA_MEMORY_EXCLUSIVE); >> + if (ret == 0) { >> + ret = -ENODEV; >> + goto errfreeipdev; >> + } >> + } >> + /* >> + * If no memory region declared in dt we assume that >> + * the user is okay with plain dma_alloc_coherent. >> + */ >> + break; >> + case ION_HEAP_TYPE_CARVEOUT: >> + case ION_HEAP_TYPE_CHUNK: >> + if (size == 0) { >> + ret = -EIO; >> + goto errfreeipdev; >> + } >> + ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL); >> + if (ipdev->freepage_ptr) { >> + addr = virt_to_phys(ipdev->freepage_ptr); >> + } else { >> + ret = -ENOMEM; >> + goto errfreeipdev; >> + } >> + break; >> + } >> + >> + ipdev->data.id = ion_heap_id; >> + ipdev->data.type = ion_heap_type; >> + ipdev->data.name = ion_heap_name; >> + ipdev->data.align = ion_heap_align; >> + ipdev->data.base = addr; >> + ipdev->data.size = size; >> + >> + /* This one make dma_declare_coherent_memory actually work */ >> + ipdev->data.priv = &pdev->dev; >> + >> + ipdev->heap = ion_heap_create(&ipdev->data); >> + if (!ipdev->heap) >> + goto errfreeipdev; >> + >> + /* If it's needed - take care enable clocks */ >> + ipdev->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(ipdev->clk)) >> + ipdev->clk = NULL; >> + else >> + clk_prepare_enable(ipdev->clk); >> + >> + ion_device_add_heap(idev, ipdev->heap); >> + claimed_heap_ids |= (1 << ion_heap_id); >> + ipdev->heap_id = ion_heap_id; >> + >> + dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx >> len %lu KiB\n", >> + ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align, >> + (unsigned long int)addr, ((unsigned long int)(size / 1024))); >> + return 0; >> + >> +errfreeipdev: >> + kfree(ipdev); >> + dev_err(&pdev->dev, "Failed to register heap: %s\n", >> + ion_heap_name); >> + return -ENOMEM; >> +} >> + >> +static int ion_physmem_remove(struct platform_device *pdev) >> +{ >> + struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev); >> + >> + ion_heap_destroy(ipdev->heap); >> + claimed_heap_ids &= ~(1 << ipdev->heap_id); >> + if (ipdev->need_free_coherent) >> + dma_release_declared_memory(&pdev->dev); >> + if (ipdev->freepage_ptr) >> + free_pages_exact(ipdev->freepage_ptr, ipdev->data.size); >> + kfree(ipdev->heap); >> + if (ipdev->clk) >> + clk_disable_unprepare(ipdev->clk); >> + kfree(ipdev); >> + >> + /* We only remove heaps and disable clocks here. >> + * There's no point in nuking the device itself, since: >> + * a). ION driver modules can't be unloaded (yet?) >> + * b). ion_device_destroy() looks like a stub and doesn't >> + * take care to free heaps/clients. >> + * c). I can't think of a scenario where it will be required >> + */ >> + >> + return 0; >> +} >> + >> +static struct platform_driver ion_physmem_driver = { >> + .probe = ion_physmem_probe, >> + .remove = ion_physmem_remove, >> + .driver = { >> + .name = "ion-physmem", >> + .of_match_table = of_match_ptr(of_match_table), >> + }, >> +}; >> + >> +static int __init ion_physmem_init(void) >> +{ >> + return platform_driver_register(&ion_physmem_driver); >> +} >> +device_initcall(ion_physmem_init); >> diff --git a/include/dt-bindings/ion,physmem.h >> b/include/dt-bindings/ion,physmem.h >> new file mode 100644 >> index 0000000..d26b742 >> --- /dev/null >> +++ b/include/dt-bindings/ion,physmem.h >> @@ -0,0 +1,16 @@ >> +/* >> + * This header provides constants for ION physmem driver. >> + * >> + */ >> + >> +#ifndef _DT_BINDINGS_ION_PHYSMEM_H >> +#define _DT_BINDINGS_ION_PHYSMEM_H >> + >> +#define ION_HEAP_TYPE_SYSTEM 0 >> +#define ION_HEAP_TYPE_SYSTEM_CONTIG 1 >> +#define ION_HEAP_TYPE_CARVEOUT 2 >> +#define ION_HEAP_TYPE_CHUNK 3 >> +#define ION_HEAP_TYPE_DMA 4 >> +#define ION_HEAP_TYPE_CUSTOM 5 > > Odd indentation choice, pick one and stick with it, don't mix in the > same list. Got it, will fix and resend as soon as I get near the box with the actual code. > > thanks, > > greg k-h -- Regards, Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/