Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755355AbbFMAQu (ORCPT ); Fri, 12 Jun 2015 20:16:50 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:36931 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752097AbbFMAQs (ORCPT ); Fri, 12 Jun 2015 20:16:48 -0400 Date: Fri, 12 Jun 2015 17:16:48 -0700 From: Greg Kroah-Hartman To: Andrew Andrianov Cc: Sudip Mukherjee , pebolle@tiscali.nl, Arve =?utf-8?B?SGrvv71ubmV277+9Zw==?= , Riley Andrews , Chen Gang , Fabian Frederick , Android Kernel Team , linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 1/2] staging: ion: Add generic ion-physmem driver Message-ID: <20150613001648.GA5234@kroah.com> References: <1433861905-9847-1-git-send-email-andrew@ncrmnt.org> <1433861905-9847-2-git-send-email-andrew@ncrmnt.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1433861905-9847-2-git-send-email-andrew@ncrmnt.org> User-Agent: Mutt/1.5.23+89 (0255b37be491) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10107 Lines: 327 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 :( And have you tested this driver build on a non-DT build (like x86-64)? > 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. thanks, greg k-h -- 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/