| Message ID | 1449490142-27502-4-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 7, 2015 at 6:08 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > This patch allows device drivers to use more than one reserved memory > region assigned to given device. When NULL name is passed to > of_reserved_mem_device_init(), the default (first) region is used. Every property that's an array does not need a name property. Just use indexes please. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/of/of_reserved_mem.c | 101 +++++++++++++++++++++++++++++++--------- > include/linux/of_reserved_mem.h | 6 ++- > 2 files changed, 84 insertions(+), 23 deletions(-) > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c > index 1a3556a9e9ea..0a0b23b73004 100644 > --- a/drivers/of/of_reserved_mem.c > +++ b/drivers/of/of_reserved_mem.c > @@ -21,6 +21,7 @@ > #include <linux/sizes.h> > #include <linux/of_reserved_mem.h> > #include <linux/sort.h> > +#include <linux/slab.h> > > #define MAX_RESERVED_REGIONS 16 > static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; > @@ -287,31 +288,84 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node) > return NULL; > } > > +static struct reserved_mem *__node_to_rmem(struct device_node *node, > + const char *name) > +{ > + struct reserved_mem *rmem; > + struct device_node *target; > + int idx = 0; > + > + if (!node) > + return NULL; > + > + if (name) { > + idx = of_property_match_string(node, > + "memory-region-names", name); > + if (idx < 0) > + return NULL; > + } > + > + target = of_parse_phandle(node, "memory-region", idx); > + if (!target) > + return NULL; > + rmem = __find_rmem(target); > + of_node_put(target); > + > + return rmem; > +} > + > +struct rmem_assigned_device { > + struct device *dev; > + struct reserved_mem *rmem; > + struct list_head list; > +}; > + > +static LIST_HEAD(of_rmem_assigned_device_list); > +static DEFINE_MUTEX(of_rmem_assigned_device_mutex); Not that this is a fast or contended path, but I think a spinlock would be more appropriate here. > + > /** > * of_reserved_mem_device_init() - assign reserved memory region to given device > + * @dev: Pointer to the device to configure > + * @np: Pointer to the device_node with 'reserved-memory' property > + * @name: Optional name of the selected region (can be NULL) > + * > + * This function assigns respective DMA-mapping operations based on reserved > + * memory regionspecified by 'memory-region' property in @np node, named @name > + * to the @dev device. When NULL name is provided, the default (first) memory > + * region is used. When driver needs to use more than one reserved memory > + * region, it should allocate child devices and initialize regions by name for > + * each of child device. > * > - * This function assign memory region pointed by "memory-region" device tree > - * property to the given device. > + * Returns error code or zero on success. > */ > -int of_reserved_mem_device_init(struct device *dev) > +int of_reserved_mem_device_init(struct device *dev, struct device_node *np, > + const char *name) > { > + struct rmem_assigned_device *rd; > struct reserved_mem *rmem; > - struct device_node *np; > int ret; > > - np = of_parse_phandle(dev->of_node, "memory-region", 0); > - if (!np) > - return -ENODEV; > - > - rmem = __find_rmem(np); > - of_node_put(np); > - > + rmem = __node_to_rmem(np, name); > if (!rmem || !rmem->ops || !rmem->ops->device_init) > return -EINVAL; > > + rd = kmalloc(sizeof(struct rmem_assigned_device), GFP_KERNEL); > + if (!rd) > + return -ENOMEM; > + > ret = rmem->ops->device_init(rmem, dev); > - if (ret == 0) > + if (ret == 0) { > + rd->dev = dev; > + rd->rmem = rmem; > + > + mutex_lock(&of_rmem_assigned_device_mutex); > + list_add(&rd->list, &of_rmem_assigned_device_list); > + mutex_unlock(&of_rmem_assigned_device_mutex); > + > dev_info(dev, "assigned reserved memory node %s\n", rmem->name); > + } else { > + kfree(rd); > + } > > return ret; > } > @@ -319,21 +373,26 @@ EXPORT_SYMBOL_GPL(of_reserved_mem_device_init); > > /** > * of_reserved_mem_device_release() - release reserved memory device structures > + * @dev: Pointer to the device to deconfigure > * > * This function releases structures allocated for memory region handling for > * the given device. > */ > void of_reserved_mem_device_release(struct device *dev) > { > - struct reserved_mem *rmem; > - struct device_node *np; > - > - np = of_parse_phandle(dev->of_node, "memory-region", 0); > - if (!np) > - return; > - > - rmem = __find_rmem(np); > - of_node_put(np); > + struct rmem_assigned_device *rd; > + struct reserved_mem *rmem = NULL; > + > + mutex_lock(&of_rmem_assigned_device_mutex); > + list_for_each_entry(rd, &of_rmem_assigned_device_list, list) { > + if (rd->dev == dev) { > + rmem = rd->rmem; > + list_del(&rd->list); > + kfree(rd); > + break; Is this function supposed to be called multiple times to release each region. That's not very obvious and which region it removes undefined for the call as it is just the first entry it finds. Either both functions should init/release all regions or a single specified region. I suppose there could be reasons not to init all regions, but would expect that is the exception. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 2015-12-08 15:58, Rob Herring wrote: > On Mon, Dec 7, 2015 at 6:08 AM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> This patch allows device drivers to use more than one reserved memory >> region assigned to given device. When NULL name is passed to >> of_reserved_mem_device_init(), the default (first) region is used. > Every property that's an array does not need a name property. Just use > indexes please. Okay, I will update the patch and add support for indices in the main implementation as well as a wrapper, which accepts "name" parameter. > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/of/of_reserved_mem.c | 101 +++++++++++++++++++++++++++++++--------- >> include/linux/of_reserved_mem.h | 6 ++- >> 2 files changed, 84 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c >> index 1a3556a9e9ea..0a0b23b73004 100644 >> --- a/drivers/of/of_reserved_mem.c >> +++ b/drivers/of/of_reserved_mem.c >> @@ -21,6 +21,7 @@ >> #include <linux/sizes.h> >> #include <linux/of_reserved_mem.h> >> #include <linux/sort.h> >> +#include <linux/slab.h> >> >> #define MAX_RESERVED_REGIONS 16 >> static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; >> @@ -287,31 +288,84 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node) >> return NULL; >> } >> >> +static struct reserved_mem *__node_to_rmem(struct device_node *node, >> + const char *name) >> +{ >> + struct reserved_mem *rmem; >> + struct device_node *target; >> + int idx = 0; >> + >> + if (!node) >> + return NULL; >> + >> + if (name) { >> + idx = of_property_match_string(node, >> + "memory-region-names", name); >> + if (idx < 0) >> + return NULL; >> + } >> + >> + target = of_parse_phandle(node, "memory-region", idx); >> + if (!target) >> + return NULL; >> + rmem = __find_rmem(target); >> + of_node_put(target); >> + >> + return rmem; >> +} >> + >> +struct rmem_assigned_device { >> + struct device *dev; >> + struct reserved_mem *rmem; >> + struct list_head list; >> +}; >> + >> +static LIST_HEAD(of_rmem_assigned_device_list); >> +static DEFINE_MUTEX(of_rmem_assigned_device_mutex); > Not that this is a fast or contended path, but I think a spinlock > would be more appropriate here. This is not meant to be called really often and for all kinds on initialization lists and structures I saw that mutexes are used instead of spinlocks. There is no intention to let this function to be called from atomic context. > >> + >> /** >> * of_reserved_mem_device_init() - assign reserved memory region to given device >> + * @dev: Pointer to the device to configure >> + * @np: Pointer to the device_node with 'reserved-memory' property >> + * @name: Optional name of the selected region (can be NULL) >> + * >> + * This function assigns respective DMA-mapping operations based on reserved >> + * memory regionspecified by 'memory-region' property in @np node, named @name >> + * to the @dev device. When NULL name is provided, the default (first) memory >> + * region is used. When driver needs to use more than one reserved memory >> + * region, it should allocate child devices and initialize regions by name for >> + * each of child device. >> * >> - * This function assign memory region pointed by "memory-region" device tree >> - * property to the given device. >> + * Returns error code or zero on success. >> */ >> -int of_reserved_mem_device_init(struct device *dev) >> +int of_reserved_mem_device_init(struct device *dev, struct device_node *np, >> + const char *name) >> { >> + struct rmem_assigned_device *rd; >> struct reserved_mem *rmem; >> - struct device_node *np; >> int ret; >> >> - np = of_parse_phandle(dev->of_node, "memory-region", 0); >> - if (!np) >> - return -ENODEV; >> - >> - rmem = __find_rmem(np); >> - of_node_put(np); >> - >> + rmem = __node_to_rmem(np, name); >> if (!rmem || !rmem->ops || !rmem->ops->device_init) >> return -EINVAL; >> >> + rd = kmalloc(sizeof(struct rmem_assigned_device), GFP_KERNEL); >> + if (!rd) >> + return -ENOMEM; >> + >> ret = rmem->ops->device_init(rmem, dev); >> - if (ret == 0) >> + if (ret == 0) { >> + rd->dev = dev; >> + rd->rmem = rmem; >> + >> + mutex_lock(&of_rmem_assigned_device_mutex); >> + list_add(&rd->list, &of_rmem_assigned_device_list); >> + mutex_unlock(&of_rmem_assigned_device_mutex); >> + >> dev_info(dev, "assigned reserved memory node %s\n", rmem->name); >> + } else { >> + kfree(rd); >> + } >> >> return ret; >> } >> @@ -319,21 +373,26 @@ EXPORT_SYMBOL_GPL(of_reserved_mem_device_init); >> >> /** >> * of_reserved_mem_device_release() - release reserved memory device structures >> + * @dev: Pointer to the device to deconfigure >> * >> * This function releases structures allocated for memory region handling for >> * the given device. >> */ >> void of_reserved_mem_device_release(struct device *dev) >> { >> - struct reserved_mem *rmem; >> - struct device_node *np; >> - >> - np = of_parse_phandle(dev->of_node, "memory-region", 0); >> - if (!np) >> - return; >> - >> - rmem = __find_rmem(np); >> - of_node_put(np); >> + struct rmem_assigned_device *rd; >> + struct reserved_mem *rmem = NULL; >> + >> + mutex_lock(&of_rmem_assigned_device_mutex); >> + list_for_each_entry(rd, &of_rmem_assigned_device_list, list) { >> + if (rd->dev == dev) { >> + rmem = rd->rmem; >> + list_del(&rd->list); >> + kfree(rd); >> + break; > Is this function supposed to be called multiple times to release each > region. That's not very obvious and which region it removes undefined > for the call as it is just the first entry it finds. > > Either both functions should init/release all regions or a single > specified region. I suppose there could be reasons not to init all > regions, but would expect that is the exception. Current implementation allows only to initialize (assign) one region for given struct device. I can add a check if given device has been already initialized if needed. To let device driver to use more regions, additional child devices need to be allocated and then for each separate call of of_reserved_mem_device_init() is needed. This is a limitation of current dma-mapping subsystem, for which the only 'key' for selecting dma/memory address space is a struct device pointer. Maybe later dma-mapping will be extended to support named/intexed dma address spaces, but for now this is the only solution. Best regards
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 1a3556a9e9ea..0a0b23b73004 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -21,6 +21,7 @@ #include <linux/sizes.h> #include <linux/of_reserved_mem.h> #include <linux/sort.h> +#include <linux/slab.h> #define MAX_RESERVED_REGIONS 16 static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; @@ -287,31 +288,84 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node) return NULL; } +static struct reserved_mem *__node_to_rmem(struct device_node *node, + const char *name) +{ + struct reserved_mem *rmem; + struct device_node *target; + int idx = 0; + + if (!node) + return NULL; + + if (name) { + idx = of_property_match_string(node, + "memory-region-names", name); + if (idx < 0) + return NULL; + } + + target = of_parse_phandle(node, "memory-region", idx); + if (!target) + return NULL; + rmem = __find_rmem(target); + of_node_put(target); + + return rmem; +} + +struct rmem_assigned_device { + struct device *dev; + struct reserved_mem *rmem; + struct list_head list; +}; + +static LIST_HEAD(of_rmem_assigned_device_list); +static DEFINE_MUTEX(of_rmem_assigned_device_mutex); + /** * of_reserved_mem_device_init() - assign reserved memory region to given device + * @dev: Pointer to the device to configure + * @np: Pointer to the device_node with 'reserved-memory' property + * @name: Optional name of the selected region (can be NULL) + * + * This function assigns respective DMA-mapping operations based on reserved + * memory regionspecified by 'memory-region' property in @np node, named @name + * to the @dev device. When NULL name is provided, the default (first) memory + * region is used. When driver needs to use more than one reserved memory + * region, it should allocate child devices and initialize regions by name for + * each of child device. * - * This function assign memory region pointed by "memory-region" device tree - * property to the given device. + * Returns error code or zero on success. */ -int of_reserved_mem_device_init(struct device *dev) +int of_reserved_mem_device_init(struct device *dev, struct device_node *np, + const char *name) { + struct rmem_assigned_device *rd; struct reserved_mem *rmem; - struct device_node *np; int ret; - np = of_parse_phandle(dev->of_node, "memory-region", 0); - if (!np) - return -ENODEV; - - rmem = __find_rmem(np); - of_node_put(np); - + rmem = __node_to_rmem(np, name); if (!rmem || !rmem->ops || !rmem->ops->device_init) return -EINVAL; + rd = kmalloc(sizeof(struct rmem_assigned_device), GFP_KERNEL); + if (!rd) + return -ENOMEM; + ret = rmem->ops->device_init(rmem, dev); - if (ret == 0) + if (ret == 0) { + rd->dev = dev; + rd->rmem = rmem; + + mutex_lock(&of_rmem_assigned_device_mutex); + list_add(&rd->list, &of_rmem_assigned_device_list); + mutex_unlock(&of_rmem_assigned_device_mutex); + dev_info(dev, "assigned reserved memory node %s\n", rmem->name); + } else { + kfree(rd); + } return ret; } @@ -319,21 +373,26 @@ EXPORT_SYMBOL_GPL(of_reserved_mem_device_init); /** * of_reserved_mem_device_release() - release reserved memory device structures + * @dev: Pointer to the device to deconfigure * * This function releases structures allocated for memory region handling for * the given device. */ void of_reserved_mem_device_release(struct device *dev) { - struct reserved_mem *rmem; - struct device_node *np; - - np = of_parse_phandle(dev->of_node, "memory-region", 0); - if (!np) - return; - - rmem = __find_rmem(np); - of_node_put(np); + struct rmem_assigned_device *rd; + struct reserved_mem *rmem = NULL; + + mutex_lock(&of_rmem_assigned_device_mutex); + list_for_each_entry(rd, &of_rmem_assigned_device_list, list) { + if (rd->dev == dev) { + rmem = rd->rmem; + list_del(&rd->list); + kfree(rd); + break; + } + } + mutex_unlock(&of_rmem_assigned_device_mutex); if (!rmem || !rmem->ops || !rmem->ops->device_release) return; diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h index ad2f67054372..05dd38490657 100644 --- a/include/linux/of_reserved_mem.h +++ b/include/linux/of_reserved_mem.h @@ -28,14 +28,16 @@ typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem); _OF_DECLARE(reservedmem, name, compat, init, reservedmem_of_init_fn) #ifdef CONFIG_OF_RESERVED_MEM -int of_reserved_mem_device_init(struct device *dev); +int of_reserved_mem_device_init(struct device *dev, struct device_node *np, + const char *name); void of_reserved_mem_device_release(struct device *dev); void fdt_init_reserved_mem(void); void fdt_reserved_mem_save_node(unsigned long node, const char *uname, phys_addr_t base, phys_addr_t size); #else -static inline int of_reserved_mem_device_init(struct device *dev) +static inline int of_reserved_mem_device_init(struct device *dev, + struct device_node *np, const char *name) { return -ENOSYS; }
This patch allows device drivers to use more than one reserved memory region assigned to given device. When NULL name is passed to of_reserved_mem_device_init(), the default (first) region is used. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/of/of_reserved_mem.c | 101 +++++++++++++++++++++++++++++++--------- include/linux/of_reserved_mem.h | 6 ++- 2 files changed, 84 insertions(+), 23 deletions(-)