[PATCH 0/3] Qualcomm SMEM V12 Support

[PATCH 0/3] Qualcomm SMEM V12 Support

[Chris Lew]

SMEM V12 was devised to make better use of the global SMEM region. The
global heap region is formatted to be similar to a private partition.
This allows the maximum number of smem items to increase. The maximum
item number is written by the bootloader in a region after the table 
of contents. The number of hosts are increased for later chipsets.

This patchset depends on patch: Qualcomm SMEM cached item support

Chris Lew (3):
  soc: qcom: smem: Support global partition
  soc: qcom: smem: Support dynamic item limit
  soc: qcom: smem: Increase the number of hosts

 drivers/soc/qcom/smem.c | 177 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 145 insertions(+), 32 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] soc: qcom: smem: Support global partition

[Chris Lew]

SMEM V12 creates a global partition to allocate global
smem items from instead of a global heap. The global
partition has the same structure as a private partition.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---
 drivers/soc/qcom/smem.c | 134 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 105 insertions(+), 29 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c28275be0038..fed2934d6bda 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -65,11 +65,20 @@
 /*
  * Item 3 of the global heap contains an array of versions for the various
  * software components in the SoC. We verify that the boot loader version is
- * what the expected version (SMEM_EXPECTED_VERSION) as a sanity check.
+ * a valid version as a sanity check.
  */
 #define SMEM_ITEM_VERSION	3
 #define  SMEM_MASTER_SBL_VERSION_INDEX	7
-#define  SMEM_EXPECTED_VERSION		11
+#define  SMEM_GLOBAL_HEAP_VERSION	11
+
+/*
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
+ */
+#define SMEM_GLOBAL_PART_VERSION	12
+#define SMEM_GLOBAL_HOST	0xfffe
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -231,6 +240,8 @@ struct smem_region {
  * struct qcom_smem - device data for the smem device
  * @dev:	device pointer
  * @hwlock:	reference to a hwspinlock
+ * @global_partition:	pointer to global partition when in use
+ * @global_cacheline:	cacheline size for global partition
  * @partitions:	list of pointers to partitions affecting the current
  *		processor/host
  * @cacheline:	list of cacheline sizes for each host
@@ -242,6 +253,8 @@ struct qcom_smem {
 
 	struct hwspinlock *hwlock;
 
+	struct smem_partition_header *global_partition;
+	size_t global_cacheline;
 	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
 	size_t cacheline[SMEM_HOST_COUNT];
 
@@ -318,16 +331,14 @@ static void *cached_entry_to_item(struct smem_private_entry *e)
 #define HWSPINLOCK_TIMEOUT	1000
 
 static int qcom_smem_alloc_private(struct qcom_smem *smem,
-				   unsigned host,
+				   struct smem_partition_header *phdr,
 				   unsigned item,
 				   size_t size)
 {
-	struct smem_partition_header *phdr;
 	struct smem_private_entry *hdr, *end;
 	size_t alloc_size;
 	void *cached;
 
-	phdr = smem->partitions[host];
 	hdr = phdr_to_first_uncached_entry(phdr);
 	end = phdr_to_last_uncached_entry(phdr);
 	cached = phdr_to_last_cached_entry(phdr);
@@ -335,8 +346,8 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
 	while (hdr < end) {
 		if (hdr->canary != SMEM_PRIVATE_CANARY) {
 			dev_err(smem->dev,
-				"Found invalid canary in host %d partition\n",
-				host);
+				"Found invalid canary in hosts %d:%d partition\n",
+				phdr->host0, phdr->host1);
 			return -EINVAL;
 		}
 
@@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 {
 	unsigned long flags;
 	int ret;
+	struct smem_partition_header *phdr;
 
 	if (!__smem)
 		return -EPROBE_DEFER;
@@ -435,10 +447,15 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 	if (ret)
 		return ret;
 
-	if (host < SMEM_HOST_COUNT && __smem->partitions[host])
-		ret = qcom_smem_alloc_private(__smem, host, item, size);
-	else
+	if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
+		phdr = __smem->partitions[host];
+		ret = qcom_smem_alloc_private(__smem, phdr, item, size);
+	} else if (__smem->global_partition) {
+		phdr = __smem->global_partition;
+		ret = qcom_smem_alloc_private(__smem, phdr, item, size);
+	} else {
 		ret = qcom_smem_alloc_global(__smem, item, size);
+	}
 
 	hwspin_unlock_irqrestore(__smem->hwlock, &flags);
 
@@ -480,16 +497,12 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
 }
 
 static void *qcom_smem_get_private(struct qcom_smem *smem,
-				   unsigned host,
+				   struct smem_partition_header *phdr,
+				   size_t cacheline,
 				   unsigned item,
 				   size_t *size)
 {
-	struct smem_partition_header *phdr;
 	struct smem_private_entry *e, *end;
-	size_t cacheline;
-
-	phdr = smem->partitions[host];
-	cacheline = smem->cacheline[host];
 
 	e = phdr_to_first_uncached_entry(phdr);
 	end = phdr_to_last_uncached_entry(phdr);
@@ -532,7 +545,8 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
 	return ERR_PTR(-ENOENT);
 
 invalid_canary:
-	dev_err(smem->dev, "Found invalid canary in host %d partition\n", host);
+	dev_err(smem->dev, "Found invalid canary in hosts %d:%d partition\n",
+			phdr->host0, phdr->host1);
 
 	return ERR_PTR(-EINVAL);
 }
@@ -551,6 +565,8 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 	unsigned long flags;
 	int ret;
 	void *ptr = ERR_PTR(-EPROBE_DEFER);
+	struct smem_partition_header *phdr;
+	size_t cacheln;
 
 	if (!__smem)
 		return ptr;
@@ -561,10 +577,17 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 	if (ret)
 		return ERR_PTR(ret);
 
-	if (host < SMEM_HOST_COUNT && __smem->partitions[host])
-		ptr = qcom_smem_get_private(__smem, host, item, size);
-	else
+	if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
+		phdr = __smem->partitions[host];
+		cacheln = __smem->cacheline[host];
+		ptr = qcom_smem_get_private(__smem, phdr, cacheln, item, size);
+	} else if (__smem->global_partition) {
+		phdr = __smem->global_partition;
+		cacheln = __smem->global_cacheline;
+		ptr = qcom_smem_get_private(__smem, phdr, cacheln, item, size);
+	} else {
 		ptr = qcom_smem_get_global(__smem, item, size);
+	}
 
 	hwspin_unlock_irqrestore(__smem->hwlock, &flags);
 
@@ -593,6 +616,10 @@ int qcom_smem_get_free_space(unsigned host)
 		phdr = __smem->partitions[host];
 		ret = le32_to_cpu(phdr->offset_free_cached) -
 		      le32_to_cpu(phdr->offset_free_uncached);
+	} else if (__smem->global_partition) {
+		phdr = __smem->global_partition;
+		ret = le32_to_cpu(phdr->offset_free_cached) -
+		      le32_to_cpu(phdr->offset_free_uncached);
 	} else {
 		header = __smem->regions[0].virt_base;
 		ret = le32_to_cpu(header->available);
@@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
 
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
+	struct smem_header *header;
 	__le32 *versions;
-	size_t size;
 
-	versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
-	if (IS_ERR(versions)) {
-		dev_err(smem->dev, "Unable to read the version item\n");
-		return -ENOENT;
+	header = smem->regions[0].virt_base;
+	versions = header->version;
+
+	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+}
+
+static int qcom_smem_set_global_partition(struct qcom_smem *smem,
+				struct smem_ptable_entry *entry)
+{
+	struct smem_partition_header *header;
+	u32  host0, host1;
+
+	if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
+		dev_err(smem->dev, "Invalid entry for gloabl partition\n");
+		return -EINVAL;
 	}
 
-	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
-		dev_err(smem->dev, "Version item is too small\n");
+	if (smem->global_partition) {
+		dev_err(smem->dev, "Already found the global partition\n");
 		return -EINVAL;
 	}
+	header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
+	host0 = le16_to_cpu(header->host0);
+	host1 = le16_to_cpu(header->host1);
 
-	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+	if (memcmp(header->magic, SMEM_PART_MAGIC,
+		    sizeof(header->magic))) {
+		dev_err(smem->dev, "Gloal partition has invalid magic\n");
+		return -EINVAL;
+	}
+
+	if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
+		dev_err(smem->dev, "Global partition hosts are invalid\n");
+		return -EINVAL;
+	}
+
+	if (header->size != entry->size) {
+		dev_err(smem->dev, "Global partition has invalid size\n");
+		return -EINVAL;
+	}
+
+	if (le32_to_cpu(header->offset_free_uncached) >
+			le32_to_cpu(header->size)) {
+		dev_err(smem->dev,
+			"Global partition has invalid free pointer\n");
+		return -EINVAL;
+	}
+
+	smem->global_partition = header;
+	smem->global_cacheline = le32_to_cpu(entry->cacheline);
+
+	return 0;
 }
 
 static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
@@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
 		host0 = le16_to_cpu(entry->host0);
 		host1 = le16_to_cpu(entry->host1);
 
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
+			if (qcom_smem_set_global_partition(smem, entry))
+				return -EINVAL;
+			continue;
+		}
+
 		if (host0 != local_host && host1 != local_host)
 			continue;
 
@@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	}
 
 	version = qcom_smem_get_sbl_version(smem);
-	if (version >> 16 != SMEM_EXPECTED_VERSION) {
+	switch (version >> 16) {
+	case SMEM_GLOBAL_PART_VERSION:
+	case SMEM_GLOBAL_HEAP_VERSION:
+	default:
 		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
 		return -EINVAL;
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] soc: qcom: smem: Support dynamic item limit

[Chris Lew]

In V12 SMEM, SBL writes SMEM parameter information
after the TOC. Use the SBL provided item count
as the max item number.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---
 drivers/soc/qcom/smem.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index fed2934d6bda..a51f4ba42173 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -225,6 +225,24 @@ struct smem_private_entry {
 #define SMEM_PRIVATE_CANARY	0xa5a5
 
 /**
+ * struct smem_info - smem region info located after the table of contents
+ * @magic:	magic number, must be SMEM_INFO_MAGIC
+ * @size:	size of the smem region
+ * @base_addr:	base address of the smem region
+ * @reserved: for now reserved entry
+ * @num_items: highest accepted item number
+ */
+struct smem_info {
+	u8 magic[4];
+	__le32 size;
+	__le32 base_addr;
+	__le32 reserved;
+	__le32 num_items;
+};
+
+static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
+
+/**
  * struct smem_region - representation of a chunk of memory used for smem
  * @aux_base:	identifier of aux_mem base
  * @virt_base:	virtual base address of memory with this aux_mem identifier
@@ -245,6 +263,7 @@ struct smem_region {
  * @partitions:	list of pointers to partitions affecting the current
  *		processor/host
  * @cacheline:	list of cacheline sizes for each host
+ * @item_count: max accepted item number
  * @num_regions: number of @regions
  * @regions:	list of the memory regions defining the shared memory
  */
@@ -257,6 +276,7 @@ struct qcom_smem {
 	size_t global_cacheline;
 	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
 	size_t cacheline[SMEM_HOST_COUNT];
+	u32 item_count;
 
 	unsigned num_regions;
 	struct smem_region regions[0];
@@ -388,7 +408,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
 	struct smem_header *header;
 	struct smem_global_entry *entry;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))
 		return -EINVAL;
 
 	header = smem->regions[0].virt_base;
@@ -473,7 +493,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
 	u32 aux_base;
 	unsigned i;
 
-	if (WARN_ON(item >= SMEM_ITEM_COUNT))
+	if (WARN_ON(item >= smem->item_count))
 		return ERR_PTR(-EINVAL);
 
 	header = smem->regions[0].virt_base;
@@ -640,6 +660,19 @@ static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
 
+static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
+{
+	struct smem_ptable *ptable;
+	struct smem_info *info;
+
+	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
+	info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+	if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+		return SMEM_ITEM_COUNT;
+
+	return le32_to_cpu(info->num_items);
+}
+
 static int qcom_smem_set_global_partition(struct qcom_smem *smem,
 				struct smem_ptable_entry *entry)
 {
@@ -857,7 +890,11 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	version = qcom_smem_get_sbl_version(smem);
 	switch (version >> 16) {
 	case SMEM_GLOBAL_PART_VERSION:
+		smem->item_count = qcom_smem_get_dynamic_item(smem);
+		break;
 	case SMEM_GLOBAL_HEAP_VERSION:
+		smem->item_count = SMEM_ITEM_COUNT;
+		break;
 	default:
 		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
 		return -EINVAL;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] soc: qcom: smem: Increase the number of hosts

[Chris Lew]

Increase the maximum number of hosts in a system to 10.

Signed-off-by: Chris Lew <clew@codeaurora.org>
---
 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index a51f4ba42173..4385f3b4bca9 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -93,7 +93,7 @@
 #define SMEM_HOST_APPS		0
 
 /* Max number of processors/hosts in a system */
-#define SMEM_HOST_COUNT		9
+#define SMEM_HOST_COUNT		10
 
 /**
   * struct smem_proc_comm - proc_comm communication struct (legacy)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

[Arun Kumar Neelakantam]

On 8/18/2017 6:45 AM, Chris Lew wrote:
> @@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
>   	}
>   
>   	version = qcom_smem_get_sbl_version(smem);
> -	if (version >> 16 != SMEM_EXPECTED_VERSION) {
> +	switch (version >> 16) {
> +	case SMEM_GLOBAL_PART_VERSION:
> +	case SMEM_GLOBAL_HEAP_VERSION:
break statement is needed for supported versions
> +	default:
>   		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
>   		return -EINVAL;
>   	}

Re: [PATCH 2/3] soc: qcom: smem: Support dynamic item limit

[Arun Kumar Neelakantam]

On 8/18/2017 6:45 AM, Chris Lew wrote:
> In V12 SMEM, SBL writes SMEM parameter information
> after the TOC. Use the SBL provided item count
> as the max item number.
>
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---
>   drivers/soc/qcom/smem.c | 41 +++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index fed2934d6bda..a51f4ba42173 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -225,6 +225,24 @@ struct smem_private_entry {
>   #define SMEM_PRIVATE_CANARY	0xa5a5
>   
>   /**
> + * struct smem_info - smem region info located after the table of contents
> + * @magic:	magic number, must be SMEM_INFO_MAGIC
> + * @size:	size of the smem region
> + * @base_addr:	base address of the smem region
> + * @reserved: for now reserved entry
> + * @num_items: highest accepted item number
> + */
> +struct smem_info {
> +	u8 magic[4];
> +	__le32 size;
> +	__le32 base_addr;
> +	__le32 reserved;
> +	__le32 num_items;
The num_items is first 16bits of reserved field, we dont have 32bit 
reserved and 32 bit for num_items
> +};
> +
> +static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
> +
> +/**
>    * struct smem_region - representation of a chunk of memory used for smem
>    * @aux_base:	identifier of aux_mem base
>    * @virt_base:	virtual base address of memory with this aux_mem identifier
> @@ -245,6 +263,7 @@ struct smem_region {
>    * @partitions:	list of pointers to partitions affecting the current
>    *		processor/host
>    * @cacheline:	list of cacheline sizes for each host
> + * @item_count: max accepted item number
>    * @num_regions: number of @regions
>    * @regions:	list of the memory regions defining the shared memory
>    */
> @@ -257,6 +276,7 @@ struct qcom_smem {
>   	size_t global_cacheline;
>   	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
>   	size_t cacheline[SMEM_HOST_COUNT];
> +	u32 item_count;
>   
>   	unsigned num_regions;
>   	struct smem_region regions[0];
> @@ -388,7 +408,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
>   	struct smem_header *header;
>   	struct smem_global_entry *entry;
>   
> -	if (WARN_ON(item >= SMEM_ITEM_COUNT))
> +	if (WARN_ON(item >= smem->item_count))
can you please add this item_count check from partition alloc/get also 
if avoid any unknown clients usage of smem
>   		return -EINVAL;
>   
>   	header = smem->regions[0].virt_base;
> @@ -473,7 +493,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
>   	u32 aux_base;
>   	unsigned i;
>   
> -	if (WARN_ON(item >= SMEM_ITEM_COUNT))
> +	if (WARN_ON(item >= smem->item_count))
>   		return ERR_PTR(-EINVAL);
>   
>   	header = smem->regions[0].virt_base;
> @@ -640,6 +660,19 @@ static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
>   	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
>   }
>   
> +static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
> +{
> +	struct smem_ptable *ptable;
> +	struct smem_info *info;
> +
> +	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
> +	info = (struct smem_info *)&ptable->entry[ptable->num_entries];
> +	if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
> +		return SMEM_ITEM_COUNT;
> +
> +	return le32_to_cpu(info->num_items);
> +}
> +
>   static int qcom_smem_set_global_partition(struct qcom_smem *smem,
>   				struct smem_ptable_entry *entry)
>   {
> @@ -857,7 +890,11 @@ static int qcom_smem_probe(struct platform_device *pdev)
>   	version = qcom_smem_get_sbl_version(smem);
>   	switch (version >> 16) {
>   	case SMEM_GLOBAL_PART_VERSION:
> +		smem->item_count = qcom_smem_get_dynamic_item(smem);
> +		break;
>   	case SMEM_GLOBAL_HEAP_VERSION:
> +		smem->item_count = SMEM_ITEM_COUNT;
> +		break;
>   	default:
>   		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
>   		return -EINVAL;

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

[Bjorn Andersson]

On Thu 17 Aug 18:15 PDT 2017, Chris Lew wrote:

> SMEM V12 creates a global partition to allocate global
> smem items from instead of a global heap. The global
> partition has the same structure as a private partition.
> 

Welcome to LKML!

This looks quite reasonable, just some minor comments below.

> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---
>  drivers/soc/qcom/smem.c | 134 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 105 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index c28275be0038..fed2934d6bda 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -65,11 +65,20 @@
>  /*
>   * Item 3 of the global heap contains an array of versions for the various
>   * software components in the SoC. We verify that the boot loader version is
> - * what the expected version (SMEM_EXPECTED_VERSION) as a sanity check.
> + * a valid version as a sanity check.
>   */
>  #define SMEM_ITEM_VERSION	3

SMEM_ITEM_VERSION is now unused, which means that the comment should be
updated with respect to item 3 and the versions array of smem_header.

>  #define  SMEM_MASTER_SBL_VERSION_INDEX	7
> -#define  SMEM_EXPECTED_VERSION		11
> +#define  SMEM_GLOBAL_HEAP_VERSION	11
> +
> +/*
> + * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
> + * for the global heap. A new global partition is created from the global heap
> + * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
> + * set by the bootloader.
> + */

Please incorporate this paragraph in the larger comment at the top of
the file, so we keep that up to date.

[..]
> @@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
>  {
>  	unsigned long flags;
>  	int ret;
> +	struct smem_partition_header *phdr;

Sorry for my OCD, but please maintain my reverse XMAS tree (put this
declaration first, as it's the longest).

>  
>  	if (!__smem)
>  		return -EPROBE_DEFER;
[..]
> @@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
>  
>  static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
>  {
> +	struct smem_header *header;
>  	__le32 *versions;
> -	size_t size;
>  
> -	versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
> -	if (IS_ERR(versions)) {
> -		dev_err(smem->dev, "Unable to read the version item\n");
> -		return -ENOENT;
> +	header = smem->regions[0].virt_base;
> +	versions = header->version;
> +
> +	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
> +}

I would prefer if you split the change of this function into its own
patch, just to clarify that it's unrelated to the new version support.

> +
> +static int qcom_smem_set_global_partition(struct qcom_smem *smem,
> +				struct smem_ptable_entry *entry)
> +{
> +	struct smem_partition_header *header;
> +	u32  host0, host1;
> +
> +	if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
> +		dev_err(smem->dev, "Invalid entry for gloabl partition\n");
> +		return -EINVAL;
>  	}
>  
> -	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
> -		dev_err(smem->dev, "Version item is too small\n");
> +	if (smem->global_partition) {
> +		dev_err(smem->dev, "Already found the global partition\n");
>  		return -EINVAL;
>  	}
> +	header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
> +	host0 = le16_to_cpu(header->host0);
> +	host1 = le16_to_cpu(header->host1);
>  
> -	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
> +	if (memcmp(header->magic, SMEM_PART_MAGIC,
> +		    sizeof(header->magic))) {
> +		dev_err(smem->dev, "Gloal partition has invalid magic\n");
> +		return -EINVAL;
> +	}
> +
> +	if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
> +		dev_err(smem->dev, "Global partition hosts are invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	if (header->size != entry->size) {

This happens to work, because they are both in the same endian. But
please sprinkle some le32_to_cpu() here as well.

> +		dev_err(smem->dev, "Global partition has invalid size\n");
> +		return -EINVAL;
> +	}
> +
> +	if (le32_to_cpu(header->offset_free_uncached) >
> +			le32_to_cpu(header->size)) {

Consider using local variables in favor of wrapping lines.

> +		dev_err(smem->dev,
> +			"Global partition has invalid free pointer\n");
> +		return -EINVAL;
> +	}
> +
> +	smem->global_partition = header;
> +	smem->global_cacheline = le32_to_cpu(entry->cacheline);
> +
> +	return 0;
>  }
>  
>  static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
> @@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
>  		host0 = le16_to_cpu(entry->host0);
>  		host1 = le16_to_cpu(entry->host1);
>  
> +		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
> +			if (qcom_smem_set_global_partition(smem, entry))
> +				return -EINVAL;
> +			continue;
> +		}
> +

As you're not able to leverage any of the checks from this loop I think
it's cleaner to duplicate the traversal of the partition table in both
functions and call the "search for global partition" directly from
probe - if the version indicates there should be one.

>  		if (host0 != local_host && host1 != local_host)
>  			continue;
>  
> @@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  	}
>  
>  	version = qcom_smem_get_sbl_version(smem);
> -	if (version >> 16 != SMEM_EXPECTED_VERSION) {
> +	switch (version >> 16) {
> +	case SMEM_GLOBAL_PART_VERSION:
> +	case SMEM_GLOBAL_HEAP_VERSION:

As Arun pointed out, there should be a "break" here.

> +	default:
>  		dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
>  		return -EINVAL;
>  	}

Regards,
Bjorn

Re: [PATCH 2/3] soc: qcom: smem: Support dynamic item limit

[Bjorn Andersson]

On Thu 17 Aug 18:15 PDT 2017, Chris Lew wrote:

> In V12 SMEM, SBL writes SMEM parameter information
> after the TOC. Use the SBL provided item count
> as the max item number.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> ---
>  drivers/soc/qcom/smem.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index fed2934d6bda..a51f4ba42173 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -225,6 +225,24 @@ struct smem_private_entry {
>  #define SMEM_PRIVATE_CANARY	0xa5a5
>  
>  /**
> + * struct smem_info - smem region info located after the table of contents
> + * @magic:	magic number, must be SMEM_INFO_MAGIC
> + * @size:	size of the smem region
> + * @base_addr:	base address of the smem region
> + * @reserved: for now reserved entry
> + * @num_items: highest accepted item number

Try to make the indentation of the descriptions a little bit more
consistent.

> + */
> +struct smem_info {
> +	u8 magic[4];
> +	__le32 size;
> +	__le32 base_addr;

May I ask why you keep the size and address of SMEM here? Didn't we just
use that information to find this item?

> +	__le32 reserved;
> +	__le32 num_items;
> +};
> +
> +static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
> +
> +/**
>   * struct smem_region - representation of a chunk of memory used for smem
>   * @aux_base:	identifier of aux_mem base
>   * @virt_base:	virtual base address of memory with this aux_mem identifier
> @@ -245,6 +263,7 @@ struct smem_region {
>   * @partitions:	list of pointers to partitions affecting the current
>   *		processor/host
>   * @cacheline:	list of cacheline sizes for each host
> + * @item_count: max accepted item number
>   * @num_regions: number of @regions
>   * @regions:	list of the memory regions defining the shared memory
>   */
> @@ -257,6 +276,7 @@ struct qcom_smem {
>  	size_t global_cacheline;
>  	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
>  	size_t cacheline[SMEM_HOST_COUNT];
> +	u32 item_count;
>  
>  	unsigned num_regions;
>  	struct smem_region regions[0];
> @@ -388,7 +408,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
>  	struct smem_header *header;
>  	struct smem_global_entry *entry;
>  
> -	if (WARN_ON(item >= SMEM_ITEM_COUNT))
> +	if (WARN_ON(item >= smem->item_count))
>  		return -EINVAL;
>  
>  	header = smem->regions[0].virt_base;
> @@ -473,7 +493,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
>  	u32 aux_base;
>  	unsigned i;
>  
> -	if (WARN_ON(item >= SMEM_ITEM_COUNT))
> +	if (WARN_ON(item >= smem->item_count))
>  		return ERR_PTR(-EINVAL);
>  
>  	header = smem->regions[0].virt_base;
> @@ -640,6 +660,19 @@ static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
>  	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
>  }
>  
> +static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
> +{
> +	struct smem_ptable *ptable;
> +	struct smem_info *info;
> +
> +	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;

This should be expanded to check the magic and version of the ptable,
but per my suggestion of duplicating this logic in patch 1 this should
likely just be broken out into a "get pointer to valid ptable"-function.

> +	info = (struct smem_info *)&ptable->entry[ptable->num_entries];
> +	if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
> +		return SMEM_ITEM_COUNT;
> +
> +	return le32_to_cpu(info->num_items);
> +}

Other than that this looks good.

Regards,
Bjorn

Re: [PATCH 3/3] soc: qcom: smem: Increase the number of hosts

[Bjorn Andersson]

On Thu 17 Aug 18:15 PDT 2017, Chris Lew wrote:

> Increase the maximum number of hosts in a system to 10.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/soc/qcom/smem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index a51f4ba42173..4385f3b4bca9 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -93,7 +93,7 @@
>  #define SMEM_HOST_APPS		0
>  
>  /* Max number of processors/hosts in a system */
> -#define SMEM_HOST_COUNT		9
> +#define SMEM_HOST_COUNT		10
>  
>  /**
>    * struct smem_proc_comm - proc_comm communication struct (legacy)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

[Chris Lew]

Hi Bjorn,

Thanks for the review.

On 8/21/2017 10:17 AM, Bjorn Andersson wrote:
>>   #define  SMEM_MASTER_SBL_VERSION_INDEX	7
>> -#define  SMEM_EXPECTED_VERSION		11
>> +#define  SMEM_GLOBAL_HEAP_VERSION	11
>> +
>> +/*
>> + * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
>> + * for the global heap. A new global partition is created from the global heap
>> + * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
>> + * set by the bootloader.
>> + */
> 
> Please incorporate this paragraph in the larger comment at the top of
> the file, so we keep that up to date.
> 
> [..]


Will do.

>> @@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
>>   {
>>   	unsigned long flags;
>>   	int ret;
>> +	struct smem_partition_header *phdr;
> 
> Sorry for my OCD, but please maintain my reverse XMAS tree (put this
> declaration first, as it's the longest).
> 

Ok, will change.

>>   
>>   	if (!__smem)
>>   		return -EPROBE_DEFER;
> [..]
>> @@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
>>   
>>   static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
>>   {
>> +	struct smem_header *header;
>>   	__le32 *versions;
>> -	size_t size;
>>   
>> -	versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
>> -	if (IS_ERR(versions)) {
>> -		dev_err(smem->dev, "Unable to read the version item\n");
>> -		return -ENOENT;
>> +	header = smem->regions[0].virt_base;
>> +	versions = header->version;
>> +
>> +	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
>> +}
> 
> I would prefer if you split the change of this function into its own
> patch, just to clarify that it's unrelated to the new version support.
>


Ok, will separate the change and adjust the descriptions accordingly.

>> +
>> +static int qcom_smem_set_global_partition(struct qcom_smem *smem,
>> +				struct smem_ptable_entry *entry)
>> +{
>> +	struct smem_partition_header *header;
>> +	u32  host0, host1;
>> +
>> +	if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
>> +		dev_err(smem->dev, "Invalid entry for gloabl partition\n");
>> +		return -EINVAL;
>>   	}
>>   
>> -	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
>> -		dev_err(smem->dev, "Version item is too small\n");
>> +	if (smem->global_partition) {
>> +		dev_err(smem->dev, "Already found the global partition\n");
>>   		return -EINVAL;
>>   	}
>> +	header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
>> +	host0 = le16_to_cpu(header->host0);
>> +	host1 = le16_to_cpu(header->host1);
>>   
>> -	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
>> +	if (memcmp(header->magic, SMEM_PART_MAGIC,
>> +		    sizeof(header->magic))) {
>> +		dev_err(smem->dev, "Gloal partition has invalid magic\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
>> +		dev_err(smem->dev, "Global partition hosts are invalid\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (header->size != entry->size) {
> 
> This happens to work, because they are both in the same endian. But
> please sprinkle some le32_to_cpu() here as well.
> 


These checks mimic the sanity checks being done in enumerate_partitions. 
Should I create a patch to increase le32_to_cpu usage in 
qcom_smem_enumerate_partitions?

>> +		dev_err(smem->dev, "Global partition has invalid size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (le32_to_cpu(header->offset_free_uncached) >
>> +			le32_to_cpu(header->size)) {
> 
> Consider using local variables in favor of wrapping lines.
>


Ok, will do.

>> +		dev_err(smem->dev,
>> +			"Global partition has invalid free pointer\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	smem->global_partition = header;
>> +	smem->global_cacheline = le32_to_cpu(entry->cacheline);
>> +
>> +	return 0;
>>   }
>>   
>>   static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
>> @@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
>>   		host0 = le16_to_cpu(entry->host0);
>>   		host1 = le16_to_cpu(entry->host1);
>>   
>> +		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
>> +			if (qcom_smem_set_global_partition(smem, entry))
>> +				return -EINVAL;
>> +			continue;
>> +		}
>> +
> 
> As you're not able to leverage any of the checks from this loop I think
> it's cleaner to duplicate the traversal of the partition table in both
> functions and call the "search for global partition" directly from
> probe - if the version indicates there should be one.
> 


Ok, will set the global partition in the version case statement and 
error out of the probe if finding the global partition fails since it is 
not optional in the new version.

>>   		if (host0 != local_host && host1 != local_host)
>>   			continue;
>>   
>> @@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
>>   	}
>>   
>>   	version = qcom_smem_get_sbl_version(smem);
>> -	if (version >> 16 != SMEM_EXPECTED_VERSION) {
>> +	switch (version >> 16) {
>> +	case SMEM_GLOBAL_PART_VERSION:
>> +	case SMEM_GLOBAL_HEAP_VERSION:
> 
> As Arun pointed out, there should be a "break" here.
> 


Will change.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

[Bjorn Andersson]

On Tue 22 Aug 17:28 PDT 2017, Chris Lew wrote:
> On 8/21/2017 10:17 AM, Bjorn Andersson wrote:
[..]
> > > +static int qcom_smem_set_global_partition(struct qcom_smem *smem,
> > > +				struct smem_ptable_entry *entry)
> > > +{
[..]
> > > +	if (header->size != entry->size) {
> > 
> > This happens to work, because they are both in the same endian. But
> > please sprinkle some le32_to_cpu() here as well.
> > 
> 
> 
> These checks mimic the sanity checks being done in enumerate_partitions.
> Should I create a patch to increase le32_to_cpu usage in
> qcom_smem_enumerate_partitions?
> 

Oops, yeah please do, just for completeness sake.

[..]
> > > @@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,
> > >   		host0 = le16_to_cpu(entry->host0);
> > >   		host1 = le16_to_cpu(entry->host1);
> > > +		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {
> > > +			if (qcom_smem_set_global_partition(smem, entry))
> > > +				return -EINVAL;
> > > +			continue;
> > > +		}
> > > +
> > 
> > As you're not able to leverage any of the checks from this loop I think
> > it's cleaner to duplicate the traversal of the partition table in both
> > functions and call the "search for global partition" directly from
> > probe - if the version indicates there should be one.
> > 
> 
> 
> Ok, will set the global partition in the version case statement and error
> out of the probe if finding the global partition fails since it is not
> optional in the new version.
> 

Sounds good.

Regards,
Bjorn