| Message ID | 1412037291-16880-5-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bjorn, On Tuesday 30 September 2014 06:04 AM, Bjorn Andersson wrote: > The Shared Memory Manager driver implements an interface for allocating > and accessing items in the memory area shared among all of the > processors in a Qualcomm platform. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > --- <snip> > + > +static int qcom_smem_probe(struct platform_device *pdev) > +{ > + struct qcom_smem *smem; > + struct resource *res; > + size_t array_size; > + int num_regions = 0; > + int i; > + > + for (i = 0; i < pdev->num_resources; i++) { > + res = &pdev->resource[i]; > + > + if (resource_type(res) == IORESOURCE_MEM) > + num_regions++; > + } > + > + if (num_regions == 0) { > + dev_err(&pdev->dev, "no smem regions specified\n"); > + return -EINVAL; > + } > + > + array_size = num_regions * sizeof(struct smem_region); > + smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL); > + if (!smem) > + return -ENOMEM; > + > + smem->dev = &pdev->dev; > + smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL); Compilation breaks while I try to compile with this patch. Do I missing anything? Below is the error I am getting, drivers/soc/qcom/qcom_smem.c: In function ‘qcom_smem_probe’: drivers/soc/qcom/qcom_smem.c:274:2: error: implicit declaration of function ‘of_hwspin_lock_request’ [-Werror=implicit-function-declaration] smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL) Thanks, --Kiran
Hi, On Tuesday 30 September 2014 11:47 AM, Kiran Padwal wrote: > Hi Bjorn, > > On Tuesday 30 September 2014 06:04 AM, Bjorn Andersson wrote: >> The Shared Memory Manager driver implements an interface for allocating >> and accessing items in the memory area shared among all of the >> processors in a Qualcomm platform. >> >> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> >> --- > > <snip> > >> + >> +static int qcom_smem_probe(struct platform_device *pdev) >> +{ >> + struct qcom_smem *smem; >> + struct resource *res; >> + size_t array_size; >> + int num_regions = 0; >> + int i; >> + >> + for (i = 0; i < pdev->num_resources; i++) { >> + res = &pdev->resource[i]; >> + >> + if (resource_type(res) == IORESOURCE_MEM) >> + num_regions++; >> + } >> + >> + if (num_regions == 0) { >> + dev_err(&pdev->dev, "no smem regions specified\n"); >> + return -EINVAL; >> + } >> + >> + array_size = num_regions * sizeof(struct smem_region); >> + smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL); >> + if (!smem) >> + return -ENOMEM; >> + >> + smem->dev = &pdev->dev; >> + smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL); > > Compilation breaks while I try to compile with this patch. > Do I missing anything? Below is the error I am getting, > > drivers/soc/qcom/qcom_smem.c: In function ‘qcom_smem_probe’: > drivers/soc/qcom/qcom_smem.c:274:2: error: implicit declaration of function ‘of_hwspin_lock_request’ [-Werror=implicit-function-declaration] > smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL) I am sorry I missed the note in commit message of this patch. I did look for any such note in cover letter and also googled around for any change to find this function. Could not find and hence posted comment. Please ignore. > > Thanks, > --Kiran > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon 29 Sep 23:28 PDT 2014, Kiran Padwal wrote: > > Hi, > > On Tuesday 30 September 2014 11:47 AM, Kiran Padwal wrote: > > Hi Bjorn, > > > > On Tuesday 30 September 2014 06:04 AM, Bjorn Andersson wrote: [..] > >> + smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL); > > > > Compilation breaks while I try to compile with this patch. > > Do I missing anything? Below is the error I am getting, > > > > drivers/soc/qcom/qcom_smem.c: In function ‘qcom_smem_probe’: > > drivers/soc/qcom/qcom_smem.c:274:2: error: implicit declaration of function ‘of_hwspin_lock_request’ [-Werror=implicit-function-declaration] > > smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL) > > I am sorry I missed the note in commit message of this patch. I did look for any such note in cover letter > and also googled around for any change to find this function. > Could not find and hence posted comment. Please ignore. > I respun this ontop of Suman Anna's latest hwspinlock patches [1] and [2]. The above code should now be: hwlock_id = of_hwspin_lock_get_id(pdev->dev.of_node, 0); smem->hwlock = hwspin_lock_request_specific(hwlock_id); Will include this in the next spin. [1] https://patchwork.kernel.org/patch/4898051/ [2] https://patchwork.kernel.org/patch/4898071/ Regards, Bjorn
Here in my initial detailed pass. I still have some "issues" that I want to clarify on my end, but I think I have plenty of comments to start with. On 9/29/2014 6:34 PM, Bjorn Andersson wrote: > The Shared Memory Manager driver implements an interface for allocating > and accessing items in the memory area shared among all of the > processors in a Qualcomm platform. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > --- > > In later platforms this is extended to support "secure smem", I do > unfortunately not have any devices that I could test this with so I have only > implemented the "insecure" version for now. > > I was thinking about implementing an of_xlate as this would make sense for many > things. It is however not feasible for SMD, that would require us to list 257 > items. > > It would make sense to enhance this with a method of keeping track of currently > consumed items, both to take care of "mutual exclusion" between consumers as > well as knowing when it's safe to remove the device and/or unload the driver. By "mutual exclusion" I assume you mean that two different entities are using the same smem id. Such a usecase is outside the purview of this driver, and is essentially unsupported. Any such usecases would need to be handled by the client. In short, smem items support one direct client. I'm not sure it makes sense to unload the driver. I understand this is a qcom specific driver, but it is critical to our devices. Any situation I can think of in which this driver would be unloaded, would ultimately result in a reboot of the device. > > I did consider exposing the items as regmaps, but for many of the items the > regmap context is consuming far more space then the actual data. And we would > end up creating 3-400 regmap contexts in a normal system. > > > Also note that the hwspinlock framework does not yet support devicetree based > retrieval of spinlock handles, so that part needs to be changed. > > drivers/soc/qcom/Kconfig | 8 + > drivers/soc/qcom/Makefile | 1 + > drivers/soc/qcom/qcom_smem.c | 328 ++++++++++++++++++++++++++++++++++++ > include/linux/soc/qcom/qcom_smem.h | 14 ++ > 4 files changed, 351 insertions(+) > create mode 100644 drivers/soc/qcom/qcom_smem.c > create mode 100644 include/linux/soc/qcom/qcom_smem.h > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index 7bd2c94..9e6bc56 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -9,3 +9,11 @@ config QCOM_GSBI > functions for connecting the underlying serial UART, SPI, and I2C > devices to the output pins. > > +config QCOM_SMEM > + tristate "Qualcomm Shared Memory Interface" > + depends on ARCH_QCOM > + help > + Say y here to enable support for the Qualcomm Shared Memory Manager. > + The driver provides an interface to items in a heap shared among all > + processors in a Qualcomm platform and is used for exchanging > + information as well as a fifo based communication mechanism (SMD). I don't think the reference to SMD is appropriate here. SMD is one of many clients that make use of SMEM. > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index 4389012..b1f7b18 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o > +obj-$(CONFIG_QCOM_SMEM) += qcom_smem.o > diff --git a/drivers/soc/qcom/qcom_smem.c b/drivers/soc/qcom/qcom_smem.c > new file mode 100644 > index 0000000..9766c86 > --- /dev/null > +++ b/drivers/soc/qcom/qcom_smem.c I see there appears to be a very small ammount of precidence (one other file), but it seems pointlessly redundant to prefix "qcom_" to the file when it exists in a "qcom" directory. What is the other point of view that I am missing? > @@ -0,0 +1,328 @@ > +/* > + * Copyright (c) 2014, Sony Mobile Communications AB. > + * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * 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/module.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/hwspinlock.h> > +#include <linux/soc/qcom/qcom_smem.h> Alphabetized please. > + > +#define AUX_BASE_MASK 0xfffffffc > +#define HWSPINLOCK_TIMEOUT 1000 > +#define SMEM_MAX_ITEMS 512 > + > +/** > + * struct smem_proc_comm - proc_comm communication struct (legacy) > + * @command: current command to be executed > + * @status: status of the currently requested command > + * @params: parameters to the command > + */ > +struct smem_proc_comm { > + u32 command; > + u32 status; > + u32 params[2]; > +}; > + > +/** > + * struct smem_entry - entry to reference smem items on the heap > + * @allocated: boolean to indicate if this entry is used > + * @offset: offset to the allocated space > + * @size: size of the allocated space, 8 byte aligned > + * @aux_base: base address for the memory region used by this unit, or 0 for > + * the default region. bits 0,1 are reserved > + */ > +struct smem_entry { > + u32 allocated; > + u32 offset; > + u32 size; > + u32 aux_base; /* bits 1:0 reserved */ Inline comment appears redundant since its addressed in the comment block above > +}; > + > +/** > + * struct smem_header - header found in beginning of primary smem region > + * @proc_comm: proc_comm communication interface (legacy) > + * @version: array of versions for the various subsystems > + * @smem_initialized: boolean to indicate that smem is initialized > + * @free_offset: index of the first unallocated byte in smem > + * @available: number of bytes available for allocation > + * @unused: reserved field > + * toc: array of references to smem entries "@toc"? > + */ > +struct smem_header { > + struct smem_proc_comm proc_comm[4]; > + u32 version[32]; > + u32 smem_initialized; > + u32 free_offset; > + u32 available; > + u32 unused; I see that you inlined the smem_heap_info struct. That is slightly problematic since we have some uses of that structure, and without it, accessing id 1 becomes complicated. I would prefer you reintroduce it. > + struct smem_entry toc[SMEM_MAX_ITEMS]; > +}; > + > +/** > + * struct smem_area - memory region used for smem heap smem_region? > + * @aux_base: physical base address of the region, used for entry lookup > + * @virt_base: virtual address of the mapping > + */ > +struct smem_region { > + u32 aux_base; > + void __iomem *virt_base; > +}; > + > +/** > + * struct qcom_smem - control struct for the smem driver > + * @dev: device pointer > + * @hwlock: hwspinlock to be held during heap operations > + * @num_regions: number of entires in the regions array > + * @regions: array of memory regions, region 0 contains smem_header > + */ > +struct qcom_smem { > + struct device *dev; > + > + struct hwspinlock *hwlock; > + > + unsigned num_regions; > + struct smem_region regions[0]; > +}; > + > +/* Pointer to the one and only smem handle */ > +static struct qcom_smem *smem_handle; > + > +/** > + * of_get_qcom_smem - retrieve a smem handle from a phandle "of_get_qcom_smem()"? > + * @client_node: of_node of the client > + * > + * Resolve the phandle in the property "qcom,smem" of @client_node and use this > + * to retrieve an smem handle. > + */ > +struct qcom_smem *of_get_qcom_smem(struct device_node *client_node) > +{ > + struct device_node *node; > + > + node = of_parse_phandle(client_node, "qcom,smem", 0); > + if (!node) > + return ERR_PTR(-EINVAL); > + > + of_node_put(node); > + > + if (!smem_handle) > + return ERR_PTR(-EPROBE_DEFER); > + else if (smem_handle->dev->of_node != node) > + return ERR_PTR(-EINVAL); > + > + return smem_handle; > +} > +EXPORT_SYMBOL(of_get_qcom_smem); Why? I understand what this does in a technical sense, but I don't understand the reasoning behind it. This forces every entity that is a client of smem to have some kind of existance in DT, when for a lot of them, they won't have a need or justification to be in DT other than to have a phandle to smem. Smem is a special memory allocator. I don't recall seeing a DT hook for kmalloc/dma_alloc_coherent/alloc_skb/etc so I'm not seeing why we have one here. All this does is get the clients the qcom_smem handle so that they can pass it back to smem for alloc and get, but there doesn't seem to be a need for it. The handle is a singleton which this driver already has a reference to, so giving it out to the clients and having them hand it back seems Rube Goldberg-esq. It also gives the clients the ability to screw with the handle, which I would prefer they never even touch since they have no business doing. You don't even sanity check the handle when it gets passed in. What happens when a client screws up and passes in NULL or a random pointer? The only positive thing about this that I see is it centralizes EPROBE_DEFER, but only for well behaved clients. It doesn't appear that it would be too difficult to have that functionality in the simplified API you've presented. > + > +/** > + * qcom_smem_alloc - allocate space for a smem item "qcom_smem_alloc()"? > + * @smem: smem handle > + * @smem_id: item to be allocated > + * @size: number of bytes to be allocated > + * > + * Allocate space for a given smem item of size @size, given that the item is > + * not yet allocated. Return? > + */ > +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size) > +{ > + struct smem_header *header = smem->regions[0].virt_base; > + struct smem_entry *entry; > + unsigned long flags; > + int ret; > + > + size = ALIGN(size, 8); > + > + if (WARN_ON(smem_id >= SMEM_MAX_ITEMS)) > + return -EINVAL; > + > + ret = hwspin_lock_timeout_irqsave(smem->hwlock, > + HWSPINLOCK_TIMEOUT, > + &flags); > + if (ret) > + return ret; > + > + entry = &header->toc[smem_id]; > + if (entry->allocated) { > + ret = -EEXIST; > + goto out; > + } > + > + if (WARN_ON(size > header->available)) { > + ret = -ENOMEM; > + goto out; > + } > + > + entry->offset = header->free_offset; > + entry->size = size; wmb() is needed here. We need to ensure the remote side sees the offset and size before seeing allocated because they may be doing a lookup without allocation without the lock. > + entry->allocated = 1; > + > + header->free_offset += size; > + header->available -= size; > + > + /* Commit the changes before we release the spin lock */ > + wmb(); > +out: > + hwspin_unlock_irqrestore(smem->hwlock, &flags); > + return ret; > +} > +EXPORT_SYMBOL(qcom_smem_alloc); > + qcom_smem_get_free_space() kerneldoc? > +unsigned qcom_smem_get_free_space(struct qcom_smem *smem) > +{ > + struct smem_header *header = smem->regions[0].virt_base; > + > + return header->available; > +} > +EXPORT_SYMBOL(qcom_smem_get_free_space); > + > +/** > + * qcom_smem_get - resolve ptr of size of a smem item "qcom_smem_get()"? > + * @smem: smem handle > + * @smem_id: item id to be resolved > + * @ptr: pointer to be filled out with address of the item > + * @size: pointer to be filled out with size of the item > + * > + * Looks up pointer and size of a smem item. Return? > + */ > +int qcom_smem_get(struct qcom_smem *smem, > + unsigned smem_id, > + void **ptr, > + size_t *size) > +{ > + struct smem_header *header = smem->regions[0].virt_base; > + struct smem_region *area; > + struct smem_entry *entry; > + unsigned long flags; > + u32 aux_base; > + unsigned i; > + int ret; > + > + if (WARN_ON(smem_id >= SMEM_MAX_ITEMS)) > + return -EINVAL; > + > + ret = hwspin_lock_timeout_irqsave(smem->hwlock, > + HWSPINLOCK_TIMEOUT, > + &flags); > + if (ret) > + return ret; > + > + entry = &header->toc[smem_id]; > + if (!entry->allocated) { > + ret = -ENXIO; > + goto out; > + } > + > + if (ptr != NULL) { > + aux_base = entry->aux_base & AUX_BASE_MASK; > + > + for (i = 0; i < smem->num_regions; i++) { > + area = &smem->regions[i]; > + > + if (area->aux_base == aux_base || !aux_base) { > + *ptr = area->virt_base + entry->offset; > + break; > + } > + } > + } > + if (size != NULL) > + *size = entry->size; > + > +out: > + hwspin_unlock_irqrestore(smem->hwlock, &flags); > + return ret; > +} > +EXPORT_SYMBOL(qcom_smem_get); > + > +static int qcom_smem_probe(struct platform_device *pdev) > +{ > + struct qcom_smem *smem; > + struct resource *res; > + size_t array_size; > + int num_regions = 0; > + int i; > + > + for (i = 0; i < pdev->num_resources; i++) { > + res = &pdev->resource[i]; > + > + if (resource_type(res) == IORESOURCE_MEM) > + num_regions++; > + } > + > + if (num_regions == 0) { > + dev_err(&pdev->dev, "no smem regions specified\n"); > + return -EINVAL; > + } > + > + array_size = num_regions * sizeof(struct smem_region); > + smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL); > + if (!smem) > + return -ENOMEM; > + > + smem->dev = &pdev->dev; > + smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL); > + if (IS_ERR(smem->hwlock)) > + return PTR_ERR(smem->hwlock); > + > + smem->num_regions = num_regions; > + > + for (i = 0; i < num_regions; i++) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, i); > + > + smem->regions[i].aux_base = (u32)res->start; > + smem->regions[i].virt_base = devm_ioremap(&pdev->dev, > + res->start, > + resource_size(res)); > + if (!smem->regions[i].virt_base) > + return -ENOMEM; > + } > + > + dev_set_drvdata(&pdev->dev, smem); > + smem_handle = smem; > + > + dev_info(&pdev->dev, "Qualcomm Shared Memory Interface probed\n"); > + > + return 0; > +} > + > +static const struct of_device_id qcom_smem_of_match[] = { > + { .compatible = "qcom,smem" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, qcom_smem_of_match); > + > +static struct platform_driver qcom_smem_driver = { > + .probe = qcom_smem_probe, > + .driver = { > + .name = "qcom_smem", > + .owner = THIS_MODULE, > + .of_match_table = qcom_smem_of_match, > + }, > +}; > + > +static int __init qcom_smem_init(void) > +{ > + return platform_driver_register(&qcom_smem_driver); > +} > +arch_initcall(qcom_smem_init); > + > +static void __exit qcom_smem_exit(void) > +{ > + platform_driver_unregister(&qcom_smem_driver); > +} > +module_exit(qcom_smem_exit) > + > +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>"); > +MODULE_DESCRIPTION("Qualcomm Shared Memory Interface"); > +MODULE_LICENSE("GPLv2"); > diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h include/soc/ already exists, why add include/linux/soc? > new file mode 100644 > index 0000000..7c0d3fd > --- /dev/null > +++ b/include/linux/soc/qcom/qcom_smem.h > @@ -0,0 +1,14 @@ > +#ifndef __QCOM_SMEM_H__ > +#define __QCOM_SMEM_H__ > + Why are the item definitions missing? > +struct device_node; > +struct qcom_smem; > + > +struct qcom_smem *of_get_qcom_smem(struct device_node *node); > + > +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size); > +int qcom_smem_get(struct qcom_smem *smem, unsigned item, void **ptr, size_t *size); > + > +unsigned qcom_smem_get_free_space(struct qcom_smem *smem); The above functions should be stubbed out when !CONFIG_QCOM_SMEM, no? > + > +#endif >
On Wed 08 Oct 14:33 PDT 2014, Jeffrey Hugo wrote: > Here in my initial detailed pass. I still have some "issues" that I > want to clarify on my end, but I think I have plenty of comments to > start with. > Thanks Jeff! > On 9/29/2014 6:34 PM, Bjorn Andersson wrote: > > The Shared Memory Manager driver implements an interface for allocating > > and accessing items in the memory area shared among all of the > > processors in a Qualcomm platform. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > > --- > > > > In later platforms this is extended to support "secure smem", I do > > unfortunately not have any devices that I could test this with so I have only > > implemented the "insecure" version for now. > > > > I was thinking about implementing an of_xlate as this would make sense for many > > things. It is however not feasible for SMD, that would require us to list 257 > > items. > > > > It would make sense to enhance this with a method of keeping track of currently > > consumed items, both to take care of "mutual exclusion" between consumers as > > well as knowing when it's safe to remove the device and/or unload the driver. > > By "mutual exclusion" I assume you mean that two different entities are > using the same smem id. Such a usecase is outside the purview of this > driver, and is essentially unsupported. Any such usecases would need to > be handled by the client. In short, smem items support one direct client. > Good, I figured that that was the case. So either we ignore the problem (just hand out pointers to drivers that want them) or we enforce unique ownership. In the latter case we would have to have a model for "releasing" items, e.g. when a consumer is removed. I can't think of any practical case when this would actually matter. > I'm not sure it makes sense to unload the driver. I understand this is > a qcom specific driver, but it is critical to our devices. Any > situation I can think of in which this driver would be unloaded, would > ultimately result in a reboot of the device. > In the case when you compile all parts of this series as modules you could unload them, but I agree that it makes little sense. But either I need to handle it or prevent it I guess. [..] > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > index 7bd2c94..9e6bc56 100644 > > --- a/drivers/soc/qcom/Kconfig > > +++ b/drivers/soc/qcom/Kconfig > > @@ -9,3 +9,11 @@ config QCOM_GSBI > > functions for connecting the underlying serial UART, SPI, and I2C > > devices to the output pins. > > > > +config QCOM_SMEM > > + tristate "Qualcomm Shared Memory Interface" > > + depends on ARCH_QCOM > > + help > > + Say y here to enable support for the Qualcomm Shared Memory Manager. > > + The driver provides an interface to items in a heap shared among all > > + processors in a Qualcomm platform and is used for exchanging > > + information as well as a fifo based communication mechanism (SMD). > > I don't think the reference to SMD is appropriate here. SMD is one of > many clients that make use of SMEM. > Ok > > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > > index 4389012..b1f7b18 100644 > > --- a/drivers/soc/qcom/Makefile > > +++ b/drivers/soc/qcom/Makefile > > @@ -1 +1,2 @@ > > obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o > > +obj-$(CONFIG_QCOM_SMEM) += qcom_smem.o > > diff --git a/drivers/soc/qcom/qcom_smem.c b/drivers/soc/qcom/qcom_smem.c > > new file mode 100644 > > index 0000000..9766c86 > > --- /dev/null > > +++ b/drivers/soc/qcom/qcom_smem.c > > I see there appears to be a very small ammount of precidence (one other > file), but it seems pointlessly redundant to prefix "qcom_" to the file > when it exists in a "qcom" directory. What is the other point of view > that I am missing? > I don't know where in the tree this driver should be, but your point is very much valid. > > @@ -0,0 +1,328 @@ > > +/* > > + * Copyright (c) 2014, Sony Mobile Communications AB. > > + * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * 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/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/io.h> > > +#include <linux/slab.h> > > +#include <linux/of.h> > > +#include <linux/hwspinlock.h> > > +#include <linux/soc/qcom/qcom_smem.h> > > Alphabetized please. > Ok [..] > > + > > +/** > > + * struct smem_entry - entry to reference smem items on the heap > > + * @allocated: boolean to indicate if this entry is used > > + * @offset: offset to the allocated space > > + * @size: size of the allocated space, 8 byte aligned > > + * @aux_base: base address for the memory region used by this unit, or 0 for > > + * the default region. bits 0,1 are reserved > > + */ > > +struct smem_entry { > > + u32 allocated; > > + u32 offset; > > + u32 size; > > + u32 aux_base; /* bits 1:0 reserved */ > > Inline comment appears redundant since its addressed in the comment > block above > Oops, thanks > > +}; > > + > > +/** > > + * struct smem_header - header found in beginning of primary smem region > > + * @proc_comm: proc_comm communication interface (legacy) > > + * @version: array of versions for the various subsystems > > + * @smem_initialized: boolean to indicate that smem is initialized > > + * @free_offset: index of the first unallocated byte in smem > > + * @available: number of bytes available for allocation > > + * @unused: reserved field > > + * toc: array of references to smem entries > > "@toc"? > Indeed, thanks > > + */ > > +struct smem_header { > > + struct smem_proc_comm proc_comm[4]; > > + u32 version[32]; > > + u32 smem_initialized; > > + u32 free_offset; > > + u32 available; > > + u32 unused; > > I see that you inlined the smem_heap_info struct. That is slightly > problematic since we have some uses of that structure, and without it, > accessing id 1 becomes complicated. I would prefer you reintroduce it. > I need to look into the code to better understand what you're referring to, but I can reinstate it. > > + struct smem_entry toc[SMEM_MAX_ITEMS]; > > +}; > > + > > +/** > > + * struct smem_area - memory region used for smem heap > > smem_region? > Of course > > + * @aux_base: physical base address of the region, used for entry lookup > > + * @virt_base: virtual address of the mapping > > + */ > > +struct smem_region { > > + u32 aux_base; > > + void __iomem *virt_base; > > +}; > > + > > +/** > > + * struct qcom_smem - control struct for the smem driver > > + * @dev: device pointer > > + * @hwlock: hwspinlock to be held during heap operations > > + * @num_regions: number of entires in the regions array > > + * @regions: array of memory regions, region 0 contains smem_header > > + */ > > +struct qcom_smem { > > + struct device *dev; > > + > > + struct hwspinlock *hwlock; > > + > > + unsigned num_regions; > > + struct smem_region regions[0]; > > +}; > > + > > +/* Pointer to the one and only smem handle */ > > +static struct qcom_smem *smem_handle; > > + > > +/** > > + * of_get_qcom_smem - retrieve a smem handle from a phandle > > "of_get_qcom_smem()"? > Ok > > + * @client_node: of_node of the client > > + * > > + * Resolve the phandle in the property "qcom,smem" of @client_node and use this > > + * to retrieve an smem handle. > > + */ > > +struct qcom_smem *of_get_qcom_smem(struct device_node *client_node) > > +{ > > + struct device_node *node; > > + > > + node = of_parse_phandle(client_node, "qcom,smem", 0); > > + if (!node) > > + return ERR_PTR(-EINVAL); > > + > > + of_node_put(node); > > + > > + if (!smem_handle) > > + return ERR_PTR(-EPROBE_DEFER); > > + else if (smem_handle->dev->of_node != node) > > + return ERR_PTR(-EINVAL); > > + > > + return smem_handle; > > +} > > +EXPORT_SYMBOL(of_get_qcom_smem); > > Why? I understand what this does in a technical sense, but I don't > understand the reasoning behind it. This forces every entity that is a > client of smem to have some kind of existance in DT, when for a lot of > them, they won't have a need or justification to be in DT other than to > have a phandle to smem. > By modelling this as a device, with a "proper" life cycle we get around many of the quirks related to clients accessing the api before this driver is initialized. > Smem is a special memory allocator. I don't recall seeing a DT hook for > kmalloc/dma_alloc_coherent/alloc_skb/etc so I'm not seeing why we have > one here. > The problem with smem is that it has dependencies to another device (hwspinlock), so we have to have some sort of life cycle management (e.g. probe deferral). > All this does is get the clients the qcom_smem handle so that they can > pass it back to smem for alloc and get, but there doesn't seem to be a > need for it. The handle is a singleton which this driver already has a > reference to, so giving it out to the clients and having them hand it > back seems Rube Goldberg-esq. It also gives the clients the ability to > screw with the handle, which I would prefer they never even touch since > they have no business doing. You don't even sanity check the handle > when it gets passed in. What happens when a client screws up and passes > in NULL or a random pointer? > I guess there will never be more than one smem instance, so you're right in that it feels a little bit strange to have to explicitly get a handle to smem. This api is defined to only take sane pointers, this is the kernel. If you pass a random pointer I guess you get random behavior. > The only positive thing about this that I see is it centralizes > EPROBE_DEFER, but only for well behaved clients. It doesn't appear that > it would be too difficult to have that functionality in the simplified > API you've presented. > I don't see much reason to support anything but "well behaved clients". Moving the probe defer to get/alloc would in my eyes force clients to implement strange logic to do late deferal of operations. > > + > > +/** > > + * qcom_smem_alloc - allocate space for a smem item > > "qcom_smem_alloc()"? > Ok > > + * @smem: smem handle > > + * @smem_id: item to be allocated > > + * @size: number of bytes to be allocated > > + * > > + * Allocate space for a given smem item of size @size, given that the item is > > + * not yet allocated. > > Return? > That sounds reasonable to document here :) > > + */ > > +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size) > > +{ > > + struct smem_header *header = smem->regions[0].virt_base; > > + struct smem_entry *entry; > > + unsigned long flags; > > + int ret; > > + > > + size = ALIGN(size, 8); > > + > > + if (WARN_ON(smem_id >= SMEM_MAX_ITEMS)) > > + return -EINVAL; > > + > > + ret = hwspin_lock_timeout_irqsave(smem->hwlock, > > + HWSPINLOCK_TIMEOUT, > > + &flags); > > + if (ret) > > + return ret; > > + > > + entry = &header->toc[smem_id]; > > + if (entry->allocated) { > > + ret = -EEXIST; > > + goto out; > > + } > > + > > + if (WARN_ON(size > header->available)) { > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > + entry->offset = header->free_offset; > > + entry->size = size; > > wmb() is needed here. We need to ensure the remote side sees the offset > and size before seeing allocated because they may be doing a lookup > without allocation without the lock. > Ahh of course, thanks! But if that's specified in the "protocol", then does that mean that we don't need to lock at all in get() as the order will make sure that an item is not allocated before it's structure is fully filled in? > > + entry->allocated = 1; > > + > > + header->free_offset += size; > > + header->available -= size; > > + > > + /* Commit the changes before we release the spin lock */ > > + wmb(); > > +out: > > + hwspin_unlock_irqrestore(smem->hwlock, &flags); > > + return ret; > > +} > > +EXPORT_SYMBOL(qcom_smem_alloc); > > + > > qcom_smem_get_free_space() kerneldoc? > Ok > > +unsigned qcom_smem_get_free_space(struct qcom_smem *smem) > > +{ > > + struct smem_header *header = smem->regions[0].virt_base; > > + > > + return header->available; > > +} > > +EXPORT_SYMBOL(qcom_smem_get_free_space); > > + > > +/** > > + * qcom_smem_get - resolve ptr of size of a smem item > > "qcom_smem_get()"? > Ok > > + * @smem: smem handle > > + * @smem_id: item id to be resolved > > + * @ptr: pointer to be filled out with address of the item > > + * @size: pointer to be filled out with size of the item > > + * > > + * Looks up pointer and size of a smem item. > > Return? > Ok > > + */ > > +int qcom_smem_get(struct qcom_smem *smem, > > + unsigned smem_id, > > + void **ptr, > > + size_t *size) > > +{ [..] > > diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h > > include/soc/ already exists, why add include/linux/soc? > I have to admit I don't really understand why the tegra stuff is in include/soc/, will have to do some more investigation. > > new file mode 100644 > > index 0000000..7c0d3fd > > --- /dev/null > > +++ b/include/linux/soc/qcom/qcom_smem.h > > @@ -0,0 +1,14 @@ > > +#ifndef __QCOM_SMEM_H__ > > +#define __QCOM_SMEM_H__ > > + > > Why are the item definitions missing? > Sorry, I don't understand what you're looking for. > > +struct device_node; > > +struct qcom_smem; > > + > > +struct qcom_smem *of_get_qcom_smem(struct device_node *node); > > + > > +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size); > > +int qcom_smem_get(struct qcom_smem *smem, unsigned item, void **ptr, size_t *size); > > + > > +unsigned qcom_smem_get_free_space(struct qcom_smem *smem); > > The above functions should be stubbed out when !CONFIG_QCOM_SMEM, no? > Either that or we just have the clients depend on CONFIG_QCOM_SMEM. > > + > > +#endif > > > Regards, Bjorn
Hi, Am 17.10.2014 um 16:51 schrieb Bjorn Andersson: > On Wed 08 Oct 14:33 PDT 2014, Jeffrey Hugo wrote: >> On 9/29/2014 6:34 PM, Bjorn Andersson wrote: >>> diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h >> >> include/soc/ already exists, why add include/linux/soc? >> > > I have to admit I don't really understand why the tegra stuff is in > include/soc/, will have to do some more investigation. AFAIR soc/ was mach-* stuff that is shared between arm and arm64. Cheers, Andreas
On Wed 08 Oct 14:33 PDT 2014, Jeffrey Hugo wrote: [..] > > + */ > > +struct smem_header { > > + struct smem_proc_comm proc_comm[4]; > > + u32 version[32]; > > + u32 smem_initialized; > > + u32 free_offset; > > + u32 available; > > + u32 unused; > > I see that you inlined the smem_heap_info struct. That is slightly > problematic since we have some uses of that structure, and without it, > accessing id 1 becomes complicated. I would prefer you reintroduce it. > Could you help me better understand what you mean here? I've scanned through msm-3.4 and msm-3.10 and read your comment numerous times, but I can't find any uses that needs it nor figure out why it would be difficult to access item 1. > > + struct smem_entry toc[SMEM_MAX_ITEMS]; > > +}; Thanks, Bjorn
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 7bd2c94..9e6bc56 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -9,3 +9,11 @@ config QCOM_GSBI functions for connecting the underlying serial UART, SPI, and I2C devices to the output pins. +config QCOM_SMEM + tristate "Qualcomm Shared Memory Interface" + depends on ARCH_QCOM + help + Say y here to enable support for the Qualcomm Shared Memory Manager. + The driver provides an interface to items in a heap shared among all + processors in a Qualcomm platform and is used for exchanging + information as well as a fifo based communication mechanism (SMD). diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index 4389012..b1f7b18 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o +obj-$(CONFIG_QCOM_SMEM) += qcom_smem.o diff --git a/drivers/soc/qcom/qcom_smem.c b/drivers/soc/qcom/qcom_smem.c new file mode 100644 index 0000000..9766c86 --- /dev/null +++ b/drivers/soc/qcom/qcom_smem.c @@ -0,0 +1,328 @@ +/* + * Copyright (c) 2014, Sony Mobile Communications AB. + * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * 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/module.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/hwspinlock.h> +#include <linux/soc/qcom/qcom_smem.h> + +#define AUX_BASE_MASK 0xfffffffc +#define HWSPINLOCK_TIMEOUT 1000 +#define SMEM_MAX_ITEMS 512 + +/** + * struct smem_proc_comm - proc_comm communication struct (legacy) + * @command: current command to be executed + * @status: status of the currently requested command + * @params: parameters to the command + */ +struct smem_proc_comm { + u32 command; + u32 status; + u32 params[2]; +}; + +/** + * struct smem_entry - entry to reference smem items on the heap + * @allocated: boolean to indicate if this entry is used + * @offset: offset to the allocated space + * @size: size of the allocated space, 8 byte aligned + * @aux_base: base address for the memory region used by this unit, or 0 for + * the default region. bits 0,1 are reserved + */ +struct smem_entry { + u32 allocated; + u32 offset; + u32 size; + u32 aux_base; /* bits 1:0 reserved */ +}; + +/** + * struct smem_header - header found in beginning of primary smem region + * @proc_comm: proc_comm communication interface (legacy) + * @version: array of versions for the various subsystems + * @smem_initialized: boolean to indicate that smem is initialized + * @free_offset: index of the first unallocated byte in smem + * @available: number of bytes available for allocation + * @unused: reserved field + * toc: array of references to smem entries + */ +struct smem_header { + struct smem_proc_comm proc_comm[4]; + u32 version[32]; + u32 smem_initialized; + u32 free_offset; + u32 available; + u32 unused; + struct smem_entry toc[SMEM_MAX_ITEMS]; +}; + +/** + * struct smem_area - memory region used for smem heap + * @aux_base: physical base address of the region, used for entry lookup + * @virt_base: virtual address of the mapping + */ +struct smem_region { + u32 aux_base; + void __iomem *virt_base; +}; + +/** + * struct qcom_smem - control struct for the smem driver + * @dev: device pointer + * @hwlock: hwspinlock to be held during heap operations + * @num_regions: number of entires in the regions array + * @regions: array of memory regions, region 0 contains smem_header + */ +struct qcom_smem { + struct device *dev; + + struct hwspinlock *hwlock; + + unsigned num_regions; + struct smem_region regions[0]; +}; + +/* Pointer to the one and only smem handle */ +static struct qcom_smem *smem_handle; + +/** + * of_get_qcom_smem - retrieve a smem handle from a phandle + * @client_node: of_node of the client + * + * Resolve the phandle in the property "qcom,smem" of @client_node and use this + * to retrieve an smem handle. + */ +struct qcom_smem *of_get_qcom_smem(struct device_node *client_node) +{ + struct device_node *node; + + node = of_parse_phandle(client_node, "qcom,smem", 0); + if (!node) + return ERR_PTR(-EINVAL); + + of_node_put(node); + + if (!smem_handle) + return ERR_PTR(-EPROBE_DEFER); + else if (smem_handle->dev->of_node != node) + return ERR_PTR(-EINVAL); + + return smem_handle; +} +EXPORT_SYMBOL(of_get_qcom_smem); + +/** + * qcom_smem_alloc - allocate space for a smem item + * @smem: smem handle + * @smem_id: item to be allocated + * @size: number of bytes to be allocated + * + * Allocate space for a given smem item of size @size, given that the item is + * not yet allocated. + */ +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size) +{ + struct smem_header *header = smem->regions[0].virt_base; + struct smem_entry *entry; + unsigned long flags; + int ret; + + size = ALIGN(size, 8); + + if (WARN_ON(smem_id >= SMEM_MAX_ITEMS)) + return -EINVAL; + + ret = hwspin_lock_timeout_irqsave(smem->hwlock, + HWSPINLOCK_TIMEOUT, + &flags); + if (ret) + return ret; + + entry = &header->toc[smem_id]; + if (entry->allocated) { + ret = -EEXIST; + goto out; + } + + if (WARN_ON(size > header->available)) { + ret = -ENOMEM; + goto out; + } + + entry->offset = header->free_offset; + entry->size = size; + entry->allocated = 1; + + header->free_offset += size; + header->available -= size; + + /* Commit the changes before we release the spin lock */ + wmb(); +out: + hwspin_unlock_irqrestore(smem->hwlock, &flags); + return ret; +} +EXPORT_SYMBOL(qcom_smem_alloc); + +unsigned qcom_smem_get_free_space(struct qcom_smem *smem) +{ + struct smem_header *header = smem->regions[0].virt_base; + + return header->available; +} +EXPORT_SYMBOL(qcom_smem_get_free_space); + +/** + * qcom_smem_get - resolve ptr of size of a smem item + * @smem: smem handle + * @smem_id: item id to be resolved + * @ptr: pointer to be filled out with address of the item + * @size: pointer to be filled out with size of the item + * + * Looks up pointer and size of a smem item. + */ +int qcom_smem_get(struct qcom_smem *smem, + unsigned smem_id, + void **ptr, + size_t *size) +{ + struct smem_header *header = smem->regions[0].virt_base; + struct smem_region *area; + struct smem_entry *entry; + unsigned long flags; + u32 aux_base; + unsigned i; + int ret; + + if (WARN_ON(smem_id >= SMEM_MAX_ITEMS)) + return -EINVAL; + + ret = hwspin_lock_timeout_irqsave(smem->hwlock, + HWSPINLOCK_TIMEOUT, + &flags); + if (ret) + return ret; + + entry = &header->toc[smem_id]; + if (!entry->allocated) { + ret = -ENXIO; + goto out; + } + + if (ptr != NULL) { + aux_base = entry->aux_base & AUX_BASE_MASK; + + for (i = 0; i < smem->num_regions; i++) { + area = &smem->regions[i]; + + if (area->aux_base == aux_base || !aux_base) { + *ptr = area->virt_base + entry->offset; + break; + } + } + } + if (size != NULL) + *size = entry->size; + +out: + hwspin_unlock_irqrestore(smem->hwlock, &flags); + return ret; +} +EXPORT_SYMBOL(qcom_smem_get); + +static int qcom_smem_probe(struct platform_device *pdev) +{ + struct qcom_smem *smem; + struct resource *res; + size_t array_size; + int num_regions = 0; + int i; + + for (i = 0; i < pdev->num_resources; i++) { + res = &pdev->resource[i]; + + if (resource_type(res) == IORESOURCE_MEM) + num_regions++; + } + + if (num_regions == 0) { + dev_err(&pdev->dev, "no smem regions specified\n"); + return -EINVAL; + } + + array_size = num_regions * sizeof(struct smem_region); + smem = devm_kzalloc(&pdev->dev, sizeof(*smem) + array_size, GFP_KERNEL); + if (!smem) + return -ENOMEM; + + smem->dev = &pdev->dev; + smem->hwlock = of_hwspin_lock_request(pdev->dev.of_node, NULL); + if (IS_ERR(smem->hwlock)) + return PTR_ERR(smem->hwlock); + + smem->num_regions = num_regions; + + for (i = 0; i < num_regions; i++) { + res = platform_get_resource(pdev, IORESOURCE_MEM, i); + + smem->regions[i].aux_base = (u32)res->start; + smem->regions[i].virt_base = devm_ioremap(&pdev->dev, + res->start, + resource_size(res)); + if (!smem->regions[i].virt_base) + return -ENOMEM; + } + + dev_set_drvdata(&pdev->dev, smem); + smem_handle = smem; + + dev_info(&pdev->dev, "Qualcomm Shared Memory Interface probed\n"); + + return 0; +} + +static const struct of_device_id qcom_smem_of_match[] = { + { .compatible = "qcom,smem" }, + {} +}; +MODULE_DEVICE_TABLE(of, qcom_smem_of_match); + +static struct platform_driver qcom_smem_driver = { + .probe = qcom_smem_probe, + .driver = { + .name = "qcom_smem", + .owner = THIS_MODULE, + .of_match_table = qcom_smem_of_match, + }, +}; + +static int __init qcom_smem_init(void) +{ + return platform_driver_register(&qcom_smem_driver); +} +arch_initcall(qcom_smem_init); + +static void __exit qcom_smem_exit(void) +{ + platform_driver_unregister(&qcom_smem_driver); +} +module_exit(qcom_smem_exit) + +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>"); +MODULE_DESCRIPTION("Qualcomm Shared Memory Interface"); +MODULE_LICENSE("GPLv2"); diff --git a/include/linux/soc/qcom/qcom_smem.h b/include/linux/soc/qcom/qcom_smem.h new file mode 100644 index 0000000..7c0d3fd --- /dev/null +++ b/include/linux/soc/qcom/qcom_smem.h @@ -0,0 +1,14 @@ +#ifndef __QCOM_SMEM_H__ +#define __QCOM_SMEM_H__ + +struct device_node; +struct qcom_smem; + +struct qcom_smem *of_get_qcom_smem(struct device_node *node); + +int qcom_smem_alloc(struct qcom_smem *smem, unsigned smem_id, size_t size); +int qcom_smem_get(struct qcom_smem *smem, unsigned item, void **ptr, size_t *size); + +unsigned qcom_smem_get_free_space(struct qcom_smem *smem); + +#endif
The Shared Memory Manager driver implements an interface for allocating and accessing items in the memory area shared among all of the processors in a Qualcomm platform. Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- In later platforms this is extended to support "secure smem", I do unfortunately not have any devices that I could test this with so I have only implemented the "insecure" version for now. I was thinking about implementing an of_xlate as this would make sense for many things. It is however not feasible for SMD, that would require us to list 257 items. It would make sense to enhance this with a method of keeping track of currently consumed items, both to take care of "mutual exclusion" between consumers as well as knowing when it's safe to remove the device and/or unload the driver. I did consider exposing the items as regmaps, but for many of the items the regmap context is consuming far more space then the actual data. And we would end up creating 3-400 regmap contexts in a normal system. Also note that the hwspinlock framework does not yet support devicetree based retrieval of spinlock handles, so that part needs to be changed. drivers/soc/qcom/Kconfig | 8 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/qcom_smem.c | 328 ++++++++++++++++++++++++++++++++++++ include/linux/soc/qcom/qcom_smem.h | 14 ++ 4 files changed, 351 insertions(+) create mode 100644 drivers/soc/qcom/qcom_smem.c create mode 100644 include/linux/soc/qcom/qcom_smem.h