diff mbox series

[v4,2/3] remoteproc: qcom: pas: make region assign more generic

Message ID 20231208-topic-sm8650-upstream-remoteproc-v4-2-a96c3e5f0913@linaro.org (mailing list archive)
State Superseded
Headers show
Series remoteproc: qcom: Introduce DSP support for SM8650 | expand

Commit Message

Neil Armstrong Dec. 8, 2023, 3:04 p.m. UTC 
The current memory region assign only supports a single
memory region.

But new platforms introduces more regions to make the
memory requirements more flexible for various use cases.
Those new platforms also shares the memory region between the
DSP and HLOS.

To handle this, make the region assign more generic in order
to support more than a single memory region and also permit
setting the regions permissions as shared.

Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 100 ++++++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 34 deletions(-)

Comments

Konrad Dybcio Dec. 9, 2023, 6:06 p.m. UTC | #1 
On 8.12.2023 16:04, Neil Armstrong wrote:
> The current memory region assign only supports a single
> memory region.
> 
> But new platforms introduces more regions to make the
> memory requirements more flexible for various use cases.
> Those new platforms also shares the memory region between the
> DSP and HLOS.
> 
> To handle this, make the region assign more generic in order
> to support more than a single memory region and also permit
> setting the regions permissions as shared.
> 
> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
[...]

> +	for (offset = 0; offset < adsp->region_assign_count; ++offset) {
> +		struct reserved_mem *rmem = NULL;
> +
> +		node = of_parse_phandle(adsp->dev->of_node, "memory-region",
> +					adsp->region_assign_idx + offset);
> +		if (node)
> +			rmem = of_reserved_mem_lookup(node);
> +		of_node_put(node);
Shouldn't this only be called when parse_phandle succeeds? (separate
patch with a fix + cc stable if so?)

> +		if (!rmem) {
> +			dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n",
> +				offset);
> +			return -EINVAL;
> +		}
>  
> -	perm.vmid = QCOM_SCM_VMID_MSS_MSA;
> -	perm.perm = QCOM_SCM_PERM_RW;
> +		if (adsp->region_assign_shared)  {
> +			perm[0].vmid = QCOM_SCM_VMID_HLOS;
> +			perm[0].perm = QCOM_SCM_PERM_RW;
> +			perm[1].vmid = adsp->region_assign_vmid;
> +			perm[1].perm = QCOM_SCM_PERM_RW;
> +			perm_size = 2;
> +		} else {
> +			perm[0].vmid = adsp->region_assign_vmid;
> +			perm[0].perm = QCOM_SCM_PERM_RW;
> +			perm_size = 1;
> +		}
>  
> -	adsp->region_assign_phys = rmem->base;
> -	adsp->region_assign_size = rmem->size;
> -	adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
> +		adsp->region_assign_phys[offset] = rmem->base;
> +		adsp->region_assign_size[offset] = rmem->size;
> +		adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);
>  
> -	ret = qcom_scm_assign_mem(adsp->region_assign_phys,
> -				  adsp->region_assign_size,
> -				  &adsp->region_assign_perms,
I think this should be renamed to region_assign_owner(s)

> -				  &perm, 1);
> -	if (ret < 0) {
> -		dev_err(adsp->dev, "assign memory failed\n");
> -		return ret;
> +		ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
> +					  adsp->region_assign_size[offset],
> +					  &adsp->region_assign_perms[offset],
> +					  perm, perm_size);
> +		if (ret < 0) {
> +			dev_err(adsp->dev, "assign memory %d failed\n", offset);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -629,20 +653,23 @@ static int adsp_assign_memory_region(struct qcom_adsp *adsp)
>  static void adsp_unassign_memory_region(struct qcom_adsp *adsp)
>  {
>  	struct qcom_scm_vmperm perm;
> +	int offset;
>  	int ret;
>  
> -	if (!adsp->region_assign_idx)
> +	if (!adsp->region_assign_idx || adsp->region_assign_shared)
So when it's *shared*, we don't want to un-assign it?

Konrad
Neil Armstrong Dec. 11, 2023, 9:37 a.m. UTC | #2 
On 09/12/2023 19:06, Konrad Dybcio wrote:
> On 8.12.2023 16:04, Neil Armstrong wrote:
>> The current memory region assign only supports a single
>> memory region.
>>
>> But new platforms introduces more regions to make the
>> memory requirements more flexible for various use cases.
>> Those new platforms also shares the memory region between the
>> DSP and HLOS.
>>
>> To handle this, make the region assign more generic in order
>> to support more than a single memory region and also permit
>> setting the regions permissions as shared.
>>
>> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
> [...]
> 
>> +	for (offset = 0; offset < adsp->region_assign_count; ++offset) {
>> +		struct reserved_mem *rmem = NULL;
>> +
>> +		node = of_parse_phandle(adsp->dev->of_node, "memory-region",
>> +					adsp->region_assign_idx + offset);
>> +		if (node)
>> +			rmem = of_reserved_mem_lookup(node);
>> +		of_node_put(node);
> Shouldn't this only be called when parse_phandle succeeds? (separate
> patch with a fix + cc stable if so?)

It's not a bug, it was added like that because of_node_put() already
checks for a NULL pointer:
https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/of/dynamic.c#L45

> 
>> +		if (!rmem) {
>> +			dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n",
>> +				offset);
>> +			return -EINVAL;
>> +		}
>>   
>> -	perm.vmid = QCOM_SCM_VMID_MSS_MSA;
>> -	perm.perm = QCOM_SCM_PERM_RW;
>> +		if (adsp->region_assign_shared)  {
>> +			perm[0].vmid = QCOM_SCM_VMID_HLOS;
>> +			perm[0].perm = QCOM_SCM_PERM_RW;
>> +			perm[1].vmid = adsp->region_assign_vmid;
>> +			perm[1].perm = QCOM_SCM_PERM_RW;
>> +			perm_size = 2;
>> +		} else {
>> +			perm[0].vmid = adsp->region_assign_vmid;
>> +			perm[0].perm = QCOM_SCM_PERM_RW;
>> +			perm_size = 1;
>> +		}
>>   
>> -	adsp->region_assign_phys = rmem->base;
>> -	adsp->region_assign_size = rmem->size;
>> -	adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
>> +		adsp->region_assign_phys[offset] = rmem->base;
>> +		adsp->region_assign_size[offset] = rmem->size;
>> +		adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);
>>   
>> -	ret = qcom_scm_assign_mem(adsp->region_assign_phys,
>> -				  adsp->region_assign_size,
>> -				  &adsp->region_assign_perms,
> I think this should be renamed to region_assign_owner(s)

Why ? this bitfield is names "perms" everywhere qcom_scm_assign_mem is used

> 
>> -				  &perm, 1);
>> -	if (ret < 0) {
>> -		dev_err(adsp->dev, "assign memory failed\n");
>> -		return ret;
>> +		ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
>> +					  adsp->region_assign_size[offset],
>> +					  &adsp->region_assign_perms[offset],
>> +					  perm, perm_size);
>> +		if (ret < 0) {
>> +			dev_err(adsp->dev, "assign memory %d failed\n", offset);
>> +			return ret;
>> +		}
>>   	}
>>   
>>   	return 0;
>> @@ -629,20 +653,23 @@ static int adsp_assign_memory_region(struct qcom_adsp *adsp)
>>   static void adsp_unassign_memory_region(struct qcom_adsp *adsp)
>>   {
>>   	struct qcom_scm_vmperm perm;
>> +	int offset;
>>   	int ret;
>>   
>> -	if (!adsp->region_assign_idx)
>> +	if (!adsp->region_assign_idx || adsp->region_assign_shared)
> So when it's *shared*, we don't want to un-assign it?

Exact, when shared the region stays shared, as downstream does, un-assigning will fail
in this case.

> 
> Konrad

Thanks,
Neil
Konrad Dybcio Dec. 11, 2023, 9:54 a.m. UTC | #3 
On 11.12.2023 10:37, Neil Armstrong wrote:
> On 09/12/2023 19:06, Konrad Dybcio wrote:
>> On 8.12.2023 16:04, Neil Armstrong wrote:
>>> The current memory region assign only supports a single
>>> memory region.
>>>
>>> But new platforms introduces more regions to make the
>>> memory requirements more flexible for various use cases.
>>> Those new platforms also shares the memory region between the
>>> DSP and HLOS.
>>>
>>> To handle this, make the region assign more generic in order
>>> to support more than a single memory region and also permit
>>> setting the regions permissions as shared.
>>>
>>> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>> [...]
>>
>>> +    for (offset = 0; offset < adsp->region_assign_count; ++offset) {
>>> +        struct reserved_mem *rmem = NULL;
>>> +
>>> +        node = of_parse_phandle(adsp->dev->of_node, "memory-region",
>>> +                    adsp->region_assign_idx + offset);
>>> +        if (node)
>>> +            rmem = of_reserved_mem_lookup(node);
>>> +        of_node_put(node);
>> Shouldn't this only be called when parse_phandle succeeds? (separate
>> patch with a fix + cc stable if so?)
> 
> It's not a bug, it was added like that because of_node_put() already
> checks for a NULL pointer:
> https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/of/dynamic.c#L45
Ack

> 
>>
>>> +        if (!rmem) {
>>> +            dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n",
>>> +                offset);
>>> +            return -EINVAL;
>>> +        }
>>>   -    perm.vmid = QCOM_SCM_VMID_MSS_MSA;
>>> -    perm.perm = QCOM_SCM_PERM_RW;
>>> +        if (adsp->region_assign_shared)  {
>>> +            perm[0].vmid = QCOM_SCM_VMID_HLOS;
>>> +            perm[0].perm = QCOM_SCM_PERM_RW;
>>> +            perm[1].vmid = adsp->region_assign_vmid;
>>> +            perm[1].perm = QCOM_SCM_PERM_RW;
>>> +            perm_size = 2;
>>> +        } else {
>>> +            perm[0].vmid = adsp->region_assign_vmid;
>>> +            perm[0].perm = QCOM_SCM_PERM_RW;
>>> +            perm_size = 1;
>>> +        }
>>>   -    adsp->region_assign_phys = rmem->base;
>>> -    adsp->region_assign_size = rmem->size;
>>> -    adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
>>> +        adsp->region_assign_phys[offset] = rmem->base;
>>> +        adsp->region_assign_size[offset] = rmem->size;
>>> +        adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);
>>>   -    ret = qcom_scm_assign_mem(adsp->region_assign_phys,
>>> -                  adsp->region_assign_size,
>>> -                  &adsp->region_assign_perms,
>> I think this should be renamed to region_assign_owner(s)
> 
> Why ? this bitfield is names "perms" everywhere qcom_scm_assign_mem is used
And IMO that's not correct - there's the qcom_scm_vmperm.perm field which
is oneOf r/w/x/rw/rwx and this one is filled with ORed BIT()-ed elements
allowed in qcom_scm_vmperm.vmid (QCOM_SCM_VMID_...)

Konrad
Neil Armstrong Dec. 11, 2023, 11:19 a.m. UTC | #4 
On 11/12/2023 10:54, Konrad Dybcio wrote:
> On 11.12.2023 10:37, Neil Armstrong wrote:
>> On 09/12/2023 19:06, Konrad Dybcio wrote:
>>> On 8.12.2023 16:04, Neil Armstrong wrote:
>>>> The current memory region assign only supports a single
>>>> memory region.
>>>>
>>>> But new platforms introduces more regions to make the
>>>> memory requirements more flexible for various use cases.
>>>> Those new platforms also shares the memory region between the
>>>> DSP and HLOS.
>>>>
>>>> To handle this, make the region assign more generic in order
>>>> to support more than a single memory region and also permit
>>>> setting the regions permissions as shared.
>>>>
>>>> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>> [...]
>>>
>>>> +    for (offset = 0; offset < adsp->region_assign_count; ++offset) {
>>>> +        struct reserved_mem *rmem = NULL;
>>>> +
>>>> +        node = of_parse_phandle(adsp->dev->of_node, "memory-region",
>>>> +                    adsp->region_assign_idx + offset);
>>>> +        if (node)
>>>> +            rmem = of_reserved_mem_lookup(node);
>>>> +        of_node_put(node);
>>> Shouldn't this only be called when parse_phandle succeeds? (separate
>>> patch with a fix + cc stable if so?)
>>
>> It's not a bug, it was added like that because of_node_put() already
>> checks for a NULL pointer:
>> https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/of/dynamic.c#L45
> Ack
> 
>>
>>>
>>>> +        if (!rmem) {
>>>> +            dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n",
>>>> +                offset);
>>>> +            return -EINVAL;
>>>> +        }
>>>>    -    perm.vmid = QCOM_SCM_VMID_MSS_MSA;
>>>> -    perm.perm = QCOM_SCM_PERM_RW;
>>>> +        if (adsp->region_assign_shared)  {
>>>> +            perm[0].vmid = QCOM_SCM_VMID_HLOS;
>>>> +            perm[0].perm = QCOM_SCM_PERM_RW;
>>>> +            perm[1].vmid = adsp->region_assign_vmid;
>>>> +            perm[1].perm = QCOM_SCM_PERM_RW;
>>>> +            perm_size = 2;
>>>> +        } else {
>>>> +            perm[0].vmid = adsp->region_assign_vmid;
>>>> +            perm[0].perm = QCOM_SCM_PERM_RW;
>>>> +            perm_size = 1;
>>>> +        }
>>>>    -    adsp->region_assign_phys = rmem->base;
>>>> -    adsp->region_assign_size = rmem->size;
>>>> -    adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
>>>> +        adsp->region_assign_phys[offset] = rmem->base;
>>>> +        adsp->region_assign_size[offset] = rmem->size;
>>>> +        adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);
>>>>    -    ret = qcom_scm_assign_mem(adsp->region_assign_phys,
>>>> -                  adsp->region_assign_size,
>>>> -                  &adsp->region_assign_perms,
>>> I think this should be renamed to region_assign_owner(s)
>>
>> Why ? this bitfield is names "perms" everywhere qcom_scm_assign_mem is used
> And IMO that's not correct - there's the qcom_scm_vmperm.perm field which
> is oneOf r/w/x/rw/rwx and this one is filled with ORed BIT()-ed elements
> allowed in qcom_scm_vmperm.vmid (QCOM_SCM_VMID_...)

Ok right I just use the same namings as in rmtfs_mem, fastrpc & ath10k/qmi,
but indeed the qcom_scm_assign_mem() 3rd param name is srcvm but doc says "vmid for current set of owners",
so yeah it could be named owners.

I'll send a v5 with the rename.

Neil


> 
> Konrad
diff mbox series

Patch

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 913a5d2068e8..46371f1ad32d 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -33,6 +33,8 @@ 
 
 #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS	100
 
+#define MAX_ASSIGN_COUNT 2
+
 struct adsp_data {
 	int crash_reason_smem;
 	const char *firmware_name;
@@ -51,6 +53,9 @@  struct adsp_data {
 	int ssctl_id;
 
 	int region_assign_idx;
+	int region_assign_count;
+	bool region_assign_shared;
+	int region_assign_vmid;
 };
 
 struct qcom_adsp {
@@ -87,15 +92,18 @@  struct qcom_adsp {
 	phys_addr_t dtb_mem_phys;
 	phys_addr_t mem_reloc;
 	phys_addr_t dtb_mem_reloc;
-	phys_addr_t region_assign_phys;
+	phys_addr_t region_assign_phys[MAX_ASSIGN_COUNT];
 	void *mem_region;
 	void *dtb_mem_region;
 	size_t mem_size;
 	size_t dtb_mem_size;
-	size_t region_assign_size;
+	size_t region_assign_size[MAX_ASSIGN_COUNT];
 
 	int region_assign_idx;
-	u64 region_assign_perms;
+	int region_assign_count;
+	bool region_assign_shared;
+	int region_assign_vmid;
+	u64 region_assign_perms[MAX_ASSIGN_COUNT];
 
 	struct qcom_rproc_glink glink_subdev;
 	struct qcom_rproc_subdev smd_subdev;
@@ -590,37 +598,53 @@  static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
 
 static int adsp_assign_memory_region(struct qcom_adsp *adsp)
 {
-	struct reserved_mem *rmem = NULL;
-	struct qcom_scm_vmperm perm;
+	struct qcom_scm_vmperm perm[MAX_ASSIGN_COUNT];
 	struct device_node *node;
+	unsigned int perm_size;
+	int offset;
 	int ret;
 
 	if (!adsp->region_assign_idx)
 		return 0;
 
-	node = of_parse_phandle(adsp->dev->of_node, "memory-region", adsp->region_assign_idx);
-	if (node)
-		rmem = of_reserved_mem_lookup(node);
-	of_node_put(node);
-	if (!rmem) {
-		dev_err(adsp->dev, "unable to resolve shareable memory-region\n");
-		return -EINVAL;
-	}
+	for (offset = 0; offset < adsp->region_assign_count; ++offset) {
+		struct reserved_mem *rmem = NULL;
+
+		node = of_parse_phandle(adsp->dev->of_node, "memory-region",
+					adsp->region_assign_idx + offset);
+		if (node)
+			rmem = of_reserved_mem_lookup(node);
+		of_node_put(node);
+		if (!rmem) {
+			dev_err(adsp->dev, "unable to resolve shareable memory-region index %d\n",
+				offset);
+			return -EINVAL;
+		}
 
-	perm.vmid = QCOM_SCM_VMID_MSS_MSA;
-	perm.perm = QCOM_SCM_PERM_RW;
+		if (adsp->region_assign_shared)  {
+			perm[0].vmid = QCOM_SCM_VMID_HLOS;
+			perm[0].perm = QCOM_SCM_PERM_RW;
+			perm[1].vmid = adsp->region_assign_vmid;
+			perm[1].perm = QCOM_SCM_PERM_RW;
+			perm_size = 2;
+		} else {
+			perm[0].vmid = adsp->region_assign_vmid;
+			perm[0].perm = QCOM_SCM_PERM_RW;
+			perm_size = 1;
+		}
 
-	adsp->region_assign_phys = rmem->base;
-	adsp->region_assign_size = rmem->size;
-	adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
+		adsp->region_assign_phys[offset] = rmem->base;
+		adsp->region_assign_size[offset] = rmem->size;
+		adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);
 
-	ret = qcom_scm_assign_mem(adsp->region_assign_phys,
-				  adsp->region_assign_size,
-				  &adsp->region_assign_perms,
-				  &perm, 1);
-	if (ret < 0) {
-		dev_err(adsp->dev, "assign memory failed\n");
-		return ret;
+		ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
+					  adsp->region_assign_size[offset],
+					  &adsp->region_assign_perms[offset],
+					  perm, perm_size);
+		if (ret < 0) {
+			dev_err(adsp->dev, "assign memory %d failed\n", offset);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -629,20 +653,23 @@  static int adsp_assign_memory_region(struct qcom_adsp *adsp)
 static void adsp_unassign_memory_region(struct qcom_adsp *adsp)
 {
 	struct qcom_scm_vmperm perm;
+	int offset;
 	int ret;
 
-	if (!adsp->region_assign_idx)
+	if (!adsp->region_assign_idx || adsp->region_assign_shared)
 		return;
 
-	perm.vmid = QCOM_SCM_VMID_HLOS;
-	perm.perm = QCOM_SCM_PERM_RW;
+	for (offset = 0; offset < adsp->region_assign_count; ++offset) {
+		perm.vmid = QCOM_SCM_VMID_HLOS;
+		perm.perm = QCOM_SCM_PERM_RW;
 
-	ret = qcom_scm_assign_mem(adsp->region_assign_phys,
-				  adsp->region_assign_size,
-				  &adsp->region_assign_perms,
-				  &perm, 1);
-	if (ret < 0)
-		dev_err(adsp->dev, "unassign memory failed\n");
+		ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
+					  adsp->region_assign_size[offset],
+					  &adsp->region_assign_perms[offset],
+					  &perm, 1);
+		if (ret < 0)
+			dev_err(adsp->dev, "unassign memory %d failed\n", offset);
+	}
 }
 
 static int adsp_probe(struct platform_device *pdev)
@@ -696,6 +723,9 @@  static int adsp_probe(struct platform_device *pdev)
 	adsp->info_name = desc->sysmon_name;
 	adsp->decrypt_shutdown = desc->decrypt_shutdown;
 	adsp->region_assign_idx = desc->region_assign_idx;
+	adsp->region_assign_count = min_t(int, MAX_ASSIGN_COUNT, desc->region_assign_count);
+	adsp->region_assign_vmid = desc->region_assign_vmid;
+	adsp->region_assign_shared = desc->region_assign_shared;
 	if (dtb_fw_name) {
 		adsp->dtb_firmware_name = dtb_fw_name;
 		adsp->dtb_pas_id = desc->dtb_pas_id;
@@ -1163,6 +1193,8 @@  static const struct adsp_data sm8550_mpss_resource = {
 	.sysmon_name = "modem",
 	.ssctl_id = 0x12,
 	.region_assign_idx = 2,
+	.region_assign_count = 1,
+	.region_assign_vmid = QCOM_SCM_VMID_MSS_MSA,
 };
 
 static const struct of_device_id adsp_of_match[] = {