[PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Stephan Gerhold]

Reserved memory can be either looked up using the generic function
of_address_to_resource() or using the special of_reserved_mem_lookup().
The latter has the advantage that it ensures that the referenced memory
region was really reserved and is not e.g. status = "disabled".

of_reserved_mem also supports allocating reserved memory dynamically at
boot time. This works only when using of_reserved_mem_lookup() since
there won't be a fixed address in the device tree.

Switch the code to use of_reserved_mem_lookup(). There is no functional
difference for static reserved memory allocations.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
See e.g. [1] for an example of dynamically allocated reserved memory.
(This patch does *not* depend on [1] and is useful without as well...)

[1]: https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
---
 drivers/media/platform/qcom/venus/firmware.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index cfb11c551167..2e7ffdaff7b2 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -10,6 +10,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/of_device.h>
 #include <linux/firmware/qcom/qcom_scm.h>
@@ -82,9 +83,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 			 phys_addr_t *mem_phys, size_t *mem_size)
 {
 	const struct firmware *mdt;
+	struct reserved_mem *rmem;
 	struct device_node *node;
 	struct device *dev;
-	struct resource r;
 	ssize_t fw_size;
 	void *mem_va;
 	int ret;
@@ -99,13 +100,16 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 		return -EINVAL;
 	}
 
-	ret = of_address_to_resource(node, 0, &r);
-	if (ret)
-		goto err_put_node;
+	rmem = of_reserved_mem_lookup(node);
+	of_node_put(node);
+	if (!rmem) {
+		dev_err(dev, "failed to lookup reserved memory-region\n");
+		return -EINVAL;
+	}
 
 	ret = request_firmware(&mdt, fwname, dev);
 	if (ret < 0)
-		goto err_put_node;
+		return ret;
 
 	fw_size = qcom_mdt_get_size(mdt);
 	if (fw_size < 0) {
@@ -113,17 +117,17 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 		goto err_release_fw;
 	}
 
-	*mem_phys = r.start;
-	*mem_size = resource_size(&r);
+	*mem_phys = rmem->base;
+	*mem_size = rmem->size;
 
 	if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {
 		ret = -EINVAL;
 		goto err_release_fw;
 	}
 
-	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
+	mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
 	if (!mem_va) {
-		dev_err(dev, "unable to map memory region: %pR\n", &r);
+		dev_err(dev, "unable to map memory region %pa size %#zx\n", mem_phys, *mem_size);
 		ret = -ENOMEM;
 		goto err_release_fw;
 	}
@@ -138,8 +142,6 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
 	memunmap(mem_va);
 err_release_fw:
 	release_firmware(mdt);
-err_put_node:
-	of_node_put(node);
 	return ret;
 }
 

---
base-commit: 9f9f8ca6f012d25428f8605cb36369a449db8508
change-id: 20230529-venus-of-rmem-f649885114fd

Best regards,
-- 
Stephan Gerhold <stephan@gerhold.net>

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Konrad Dybcio]

On 29.05.2023 20:16, Stephan Gerhold wrote:
> Reserved memory can be either looked up using the generic function
> of_address_to_resource() or using the special of_reserved_mem_lookup().
> The latter has the advantage that it ensures that the referenced memory
> region was really reserved and is not e.g. status = "disabled".
> 
> of_reserved_mem also supports allocating reserved memory dynamically at
> boot time. This works only when using of_reserved_mem_lookup() since
> there won't be a fixed address in the device tree.
> 
> Switch the code to use of_reserved_mem_lookup(). There is no functional
> difference for static reserved memory allocations.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> See e.g. [1] for an example of dynamically allocated reserved memory.
> (This patch does *not* depend on [1] and is useful without as well...)
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

>  drivers/media/platform/qcom/venus/firmware.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
[...]

> -	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
> +	mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
>  	if (!mem_va) {
> -		dev_err(dev, "unable to map memory region: %pR\n", &r);
> +		dev_err(dev, "unable to map memory region %pa size %#zx\n", mem_phys, *mem_size);
Nit : I'm not sure which is more useful, but many mapping functions
seem to prefer printing

start, start+size-1

instead of 

start, size

on failure.

Konrad
>  		ret = -ENOMEM;
>  		goto err_release_fw;
>  	}
> @@ -138,8 +142,6 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  	memunmap(mem_va);
>  err_release_fw:
>  	release_firmware(mdt);
> -err_put_node:
> -	of_node_put(node);
>  	return ret;
>  }
>  
> 
> ---
> base-commit: 9f9f8ca6f012d25428f8605cb36369a449db8508
> change-id: 20230529-venus-of-rmem-f649885114fd
> 
> Best regards,

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Stephan Gerhold]

On Mon, May 29, 2023 at 08:36:27PM +0200, Konrad Dybcio wrote:
> On 29.05.2023 20:16, Stephan Gerhold wrote:
> > Reserved memory can be either looked up using the generic function
> > of_address_to_resource() or using the special of_reserved_mem_lookup().
> > The latter has the advantage that it ensures that the referenced memory
> > region was really reserved and is not e.g. status = "disabled".
> > 
> > of_reserved_mem also supports allocating reserved memory dynamically at
> > boot time. This works only when using of_reserved_mem_lookup() since
> > there won't be a fixed address in the device tree.
> > 
> > Switch the code to use of_reserved_mem_lookup(). There is no functional
> > difference for static reserved memory allocations.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > See e.g. [1] for an example of dynamically allocated reserved memory.
> > (This patch does *not* depend on [1] and is useful without as well...)
> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
> > ---
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> >  drivers/media/platform/qcom/venus/firmware.c | 24 +++++++++++++-----------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> [...]
> 
> > -	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
> > +	mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
> >  	if (!mem_va) {
> > -		dev_err(dev, "unable to map memory region: %pR\n", &r);
> > +		dev_err(dev, "unable to map memory region %pa size %#zx\n", mem_phys, *mem_size);
> Nit : I'm not sure which is more useful, but many mapping functions
> seem to prefer printing
> 
> start, start+size-1
> 
> instead of 
> 
> start, size
> 
> on failure.
> 

Shrug, both is fine for me. I just used this because the qcom remoteproc
usages of of_address_to_resource() that I also patched already seemed to
log it similar to this.

If someone wants me to resend this with start, start+size-1 as suggested
by Konrad (or something else) then just let me know. :)

Thanks,
Stephan

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Vikash Garodia]

Hi Stephan,

On 5/29/2023 11:46 PM, Stephan Gerhold wrote:
> Reserved memory can be either looked up using the generic function
> of_address_to_resource() or using the special of_reserved_mem_lookup().
> The latter has the advantage that it ensures that the referenced memory
> region was really reserved and is not e.g. status = "disabled".
> 
> of_reserved_mem also supports allocating reserved memory dynamically at
> boot time. This works only when using of_reserved_mem_lookup() since
> there won't be a fixed address in the device tree.
IIUC, this would avoid precomputing the hard range for different firmware
regions and also make it more flexible to adjust the sizes, if anyone wants a
bigger size later.
Incase a specific firmware needs a dedicate start address, do we have an option
to specify the same ?

Thanks,
Vikash
> Switch the code to use of_reserved_mem_lookup(). There is no functional
> difference for static reserved memory allocations.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> See e.g. [1] for an example of dynamically allocated reserved memory.
> (This patch does *not* depend on [1] and is useful without as well...)
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
> ---
>  drivers/media/platform/qcom/venus/firmware.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index cfb11c551167..2e7ffdaff7b2 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -10,6 +10,7 @@
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
>  #include <linux/firmware/qcom/qcom_scm.h>
> @@ -82,9 +83,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  			 phys_addr_t *mem_phys, size_t *mem_size)
>  {
>  	const struct firmware *mdt;
> +	struct reserved_mem *rmem;
>  	struct device_node *node;
>  	struct device *dev;
> -	struct resource r;
>  	ssize_t fw_size;
>  	void *mem_va;
>  	int ret;
> @@ -99,13 +100,16 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		return -EINVAL;
>  	}
>  
> -	ret = of_address_to_resource(node, 0, &r);
> -	if (ret)
> -		goto err_put_node;
> +	rmem = of_reserved_mem_lookup(node);
> +	of_node_put(node);
> +	if (!rmem) {
> +		dev_err(dev, "failed to lookup reserved memory-region\n");
> +		return -EINVAL;
> +	}
>  
>  	ret = request_firmware(&mdt, fwname, dev);
>  	if (ret < 0)
> -		goto err_put_node;
> +		return ret;
>  
>  	fw_size = qcom_mdt_get_size(mdt);
>  	if (fw_size < 0) {
> @@ -113,17 +117,17 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		goto err_release_fw;
>  	}
>  
> -	*mem_phys = r.start;
> -	*mem_size = resource_size(&r);
> +	*mem_phys = rmem->base;
> +	*mem_size = rmem->size;
>  
>  	if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {
>  		ret = -EINVAL;
>  		goto err_release_fw;
>  	}
>  
> -	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
> +	mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
>  	if (!mem_va) {
> -		dev_err(dev, "unable to map memory region: %pR\n", &r);
> +		dev_err(dev, "unable to map memory region %pa size %#zx\n", mem_phys, *mem_size);
>  		ret = -ENOMEM;
>  		goto err_release_fw;
>  	}
> @@ -138,8 +142,6 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  	memunmap(mem_va);
>  err_release_fw:
>  	release_firmware(mdt);
> -err_put_node:
> -	of_node_put(node);
>  	return ret;
>  }
>  
> 
> ---
> base-commit: 9f9f8ca6f012d25428f8605cb36369a449db8508
> change-id: 20230529-venus-of-rmem-f649885114fd
> 
> Best regards,

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Stephan Gerhold]

On Wed, May 31, 2023 at 11:36:52AM +0530, Vikash Garodia wrote:
> On 5/29/2023 11:46 PM, Stephan Gerhold wrote:
> > Reserved memory can be either looked up using the generic function
> > of_address_to_resource() or using the special of_reserved_mem_lookup().
> > The latter has the advantage that it ensures that the referenced memory
> > region was really reserved and is not e.g. status = "disabled".
> > 
> > of_reserved_mem also supports allocating reserved memory dynamically at
> > boot time. This works only when using of_reserved_mem_lookup() since
> > there won't be a fixed address in the device tree.
> IIUC, this would avoid precomputing the hard range for different firmware
> regions and also make it more flexible to adjust the sizes, if anyone wants a
> bigger size later.
> Incase a specific firmware needs a dedicate start address, do we have an option
> to specify the same ?
> 

If you want a specific start address (or in other words: a fixed base
address) then you should continue using static reservation for that
component. You can mix static and dynamic reservations. The static ones
(with fixed addresses) will be reserved first, then the dynamic ones
will be allocated from the free space.

I have this example for one device in my proposal at [1]:

	/* Firmware must be loaded at address 0x8b600000 */
	wcnss_mem: wcnss@8b600000 {
		reg = <0x8b600000 0x600000>;
		no-map;
	};
	/* Firmware can be loaded anywhere with 1 MiB alignment */
	venus_mem: venus {
		size = <0x500000>;
		alignment = <0x100000>;
		no-map;
	};

The wcnss_mem will be always at 0x8b600000, but the venus_mem can be
loaded somewhere around that. If only certain regions need a fixed
address this still provides the flexibility to change sizes more easily.

Does that answer your question? I wasn't sure what exactly you mean with
a "dedicated start address". :)

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Vikash Garodia]

On 5/31/2023 12:26 PM, Stephan Gerhold wrote:
> On Wed, May 31, 2023 at 11:36:52AM +0530, Vikash Garodia wrote:
>> On 5/29/2023 11:46 PM, Stephan Gerhold wrote:
>>> Reserved memory can be either looked up using the generic function
>>> of_address_to_resource() or using the special of_reserved_mem_lookup().
>>> The latter has the advantage that it ensures that the referenced memory
>>> region was really reserved and is not e.g. status = "disabled".
>>>
>>> of_reserved_mem also supports allocating reserved memory dynamically at
>>> boot time. This works only when using of_reserved_mem_lookup() since
>>> there won't be a fixed address in the device tree.
>> IIUC, this would avoid precomputing the hard range for different firmware
>> regions and also make it more flexible to adjust the sizes, if anyone wants a
>> bigger size later.
>> Incase a specific firmware needs a dedicate start address, do we have an option
>> to specify the same ?
>>
> 
> If you want a specific start address (or in other words: a fixed base
> address) then you should continue using static reservation for that
> component. You can mix static and dynamic reservations. The static ones
> (with fixed addresses) will be reserved first, then the dynamic ones
> will be allocated from the free space.
> 
> I have this example for one device in my proposal at [1]:
> 
> 	/* Firmware must be loaded at address 0x8b600000 */
> 	wcnss_mem: wcnss@8b600000 {
> 		reg = <0x8b600000 0x600000>;
> 		no-map;
> 	};
> 	/* Firmware can be loaded anywhere with 1 MiB alignment */
> 	venus_mem: venus {
> 		size = <0x500000>;
> 		alignment = <0x100000>;
> 		no-map;
> 	};
> 
> The wcnss_mem will be always at 0x8b600000, but the venus_mem can be
> loaded somewhere around that. If only certain regions need a fixed
> address this still provides the flexibility to change sizes more easily.
> 
> Does that answer your question? I wasn't sure what exactly you mean with
> a "dedicated start address". :)
Yes, it clarified the need if any subsystem wants a specific start address.

One more thing, lets say, we keep it dynamic allocation and at the same time we
need to pass the start address to TZ call in [1]. How do we get that allocated
address so as to pass in [1] ?

Thanks,
Vikash

[1]:
https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/soc/qcom/mdt_loader.c#L249

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Mukesh Ojha]

On 5/31/2023 1:04 PM, Vikash Garodia wrote:
> 
> On 5/31/2023 12:26 PM, Stephan Gerhold wrote:
>> On Wed, May 31, 2023 at 11:36:52AM +0530, Vikash Garodia wrote:
>>> On 5/29/2023 11:46 PM, Stephan Gerhold wrote:
>>>> Reserved memory can be either looked up using the generic function
>>>> of_address_to_resource() or using the special of_reserved_mem_lookup().
>>>> The latter has the advantage that it ensures that the referenced memory
>>>> region was really reserved and is not e.g. status = "disabled".
>>>>
>>>> of_reserved_mem also supports allocating reserved memory dynamically at
>>>> boot time. This works only when using of_reserved_mem_lookup() since
>>>> there won't be a fixed address in the device tree.
>>> IIUC, this would avoid precomputing the hard range for different firmware
>>> regions and also make it more flexible to adjust the sizes, if anyone wants a
>>> bigger size later.
>>> Incase a specific firmware needs a dedicate start address, do we have an option
>>> to specify the same ?
>>>
>>
>> If you want a specific start address (or in other words: a fixed base
>> address) then you should continue using static reservation for that
>> component. You can mix static and dynamic reservations. The static ones
>> (with fixed addresses) will be reserved first, then the dynamic ones
>> will be allocated from the free space.
>>
>> I have this example for one device in my proposal at [1]:
>>
>> 	/* Firmware must be loaded at address 0x8b600000 */
>> 	wcnss_mem: wcnss@8b600000 {
>> 		reg = <0x8b600000 0x600000>;
>> 		no-map;
>> 	};
>> 	/* Firmware can be loaded anywhere with 1 MiB alignment */
>> 	venus_mem: venus {
>> 		size = <0x500000>;
>> 		alignment = <0x100000>;
>> 		no-map;
>> 	};
>>
>> The wcnss_mem will be always at 0x8b600000, but the venus_mem can be
>> loaded somewhere around that. If only certain regions need a fixed
>> address this still provides the flexibility to change sizes more easily.
>>
>> Does that answer your question? I wasn't sure what exactly you mean with
>> a "dedicated start address". :)
> Yes, it clarified the need if any subsystem wants a specific start address.
> 
> One more thing, lets say, we keep it dynamic allocation and at the same time we
> need to pass the start address to TZ call in [1]. How do we get that allocated
> address so as to pass in [1] ?

+	*mem_phys = rmem->base;

It will provide the start, is not it ?

-- Mukesh

> 
> Thanks,
> Vikash
> 
> [1]:
> https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/soc/qcom/mdt_loader.c#L249

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Stephan Gerhold]

On Wed, May 31, 2023 at 01:15:26PM +0530, Mukesh Ojha wrote:
> 
> 
> On 5/31/2023 1:04 PM, Vikash Garodia wrote:
> > 
> > On 5/31/2023 12:26 PM, Stephan Gerhold wrote:
> > > On Wed, May 31, 2023 at 11:36:52AM +0530, Vikash Garodia wrote:
> > > > On 5/29/2023 11:46 PM, Stephan Gerhold wrote:
> > > > > Reserved memory can be either looked up using the generic function
> > > > > of_address_to_resource() or using the special of_reserved_mem_lookup().
> > > > > The latter has the advantage that it ensures that the referenced memory
> > > > > region was really reserved and is not e.g. status = "disabled".
> > > > > 
> > > > > of_reserved_mem also supports allocating reserved memory dynamically at
> > > > > boot time. This works only when using of_reserved_mem_lookup() since
> > > > > there won't be a fixed address in the device tree.
> > > > IIUC, this would avoid precomputing the hard range for different firmware
> > > > regions and also make it more flexible to adjust the sizes, if anyone wants a
> > > > bigger size later.
> > > > Incase a specific firmware needs a dedicate start address, do we have an option
> > > > to specify the same ?
> > > > 
> > > 
> > > If you want a specific start address (or in other words: a fixed base
> > > address) then you should continue using static reservation for that
> > > component. You can mix static and dynamic reservations. The static ones
> > > (with fixed addresses) will be reserved first, then the dynamic ones
> > > will be allocated from the free space.
> > > 
> > > I have this example for one device in my proposal at [1]:
> > > 
> > > 	/* Firmware must be loaded at address 0x8b600000 */
> > > 	wcnss_mem: wcnss@8b600000 {
> > > 		reg = <0x8b600000 0x600000>;
> > > 		no-map;
> > > 	};
> > > 	/* Firmware can be loaded anywhere with 1 MiB alignment */
> > > 	venus_mem: venus {
> > > 		size = <0x500000>;
> > > 		alignment = <0x100000>;
> > > 		no-map;
> > > 	};
> > > 
> > > The wcnss_mem will be always at 0x8b600000, but the venus_mem can be
> > > loaded somewhere around that. If only certain regions need a fixed
> > > address this still provides the flexibility to change sizes more easily.
> > > 
> > > Does that answer your question? I wasn't sure what exactly you mean with
> > > a "dedicated start address". :)
> > Yes, it clarified the need if any subsystem wants a specific start address.
> > 
> > One more thing, lets say, we keep it dynamic allocation and at the same time we
> > need to pass the start address to TZ call in [1]. How do we get that allocated
> > address so as to pass in [1] ?
> 
> +	*mem_phys = rmem->base;
> 
> It will provide the start, is not it ?
> 

Yes, when using of_reserved_mem_lookup() the allocated address is
available in rmem->base. If you have this patch applied then it should
be given to the TZ call as expected.

Thanks,
Stephan

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Vikash Garodia]

On 5/29/2023 11:46 PM, Stephan Gerhold wrote:
> Reserved memory can be either looked up using the generic function
> of_address_to_resource() or using the special of_reserved_mem_lookup().
> The latter has the advantage that it ensures that the referenced memory
> region was really reserved and is not e.g. status = "disabled".
> 
> of_reserved_mem also supports allocating reserved memory dynamically at
> boot time. This works only when using of_reserved_mem_lookup() since
> there won't be a fixed address in the device tree.
> 
> Switch the code to use of_reserved_mem_lookup(). There is no functional
> difference for static reserved memory allocations.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>

> ---
> See e.g. [1] for an example of dynamically allocated reserved memory.
> (This patch does *not* depend on [1] and is useful without as well...)
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
> ---
>  drivers/media/platform/qcom/venus/firmware.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index cfb11c551167..2e7ffdaff7b2 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -10,6 +10,7 @@
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
>  #include <linux/firmware/qcom/qcom_scm.h>
> @@ -82,9 +83,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  			 phys_addr_t *mem_phys, size_t *mem_size)
>  {
>  	const struct firmware *mdt;
> +	struct reserved_mem *rmem;
>  	struct device_node *node;
>  	struct device *dev;
> -	struct resource r;
>  	ssize_t fw_size;
>  	void *mem_va;
>  	int ret;
> @@ -99,13 +100,16 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		return -EINVAL;
>  	}
>  
> -	ret = of_address_to_resource(node, 0, &r);
> -	if (ret)
> -		goto err_put_node;
> +	rmem = of_reserved_mem_lookup(node);
> +	of_node_put(node);
> +	if (!rmem) {
> +		dev_err(dev, "failed to lookup reserved memory-region\n");
> +		return -EINVAL;
> +	}
>  
>  	ret = request_firmware(&mdt, fwname, dev);
>  	if (ret < 0)
> -		goto err_put_node;
> +		return ret;
>  
>  	fw_size = qcom_mdt_get_size(mdt);
>  	if (fw_size < 0) {
> @@ -113,17 +117,17 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		goto err_release_fw;
>  	}
>  
> -	*mem_phys = r.start;
> -	*mem_size = resource_size(&r);
> +	*mem_phys = rmem->base;
> +	*mem_size = rmem->size;
>  
>  	if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {
>  		ret = -EINVAL;
>  		goto err_release_fw;
>  	}
>  
> -	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
> +	mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
>  	if (!mem_va) {
> -		dev_err(dev, "unable to map memory region: %pR\n", &r);
> +		dev_err(dev, "unable to map memory region %pa size %#zx\n", mem_phys, *mem_size);
>  		ret = -ENOMEM;
>  		goto err_release_fw;
>  	}
> @@ -138,8 +142,6 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  	memunmap(mem_va);
>  err_release_fw:
>  	release_firmware(mdt);
> -err_put_node:
> -	of_node_put(node);
>  	return ret;
>  }
>  
> 
> ---
> base-commit: 9f9f8ca6f012d25428f8605cb36369a449db8508
> change-id: 20230529-venus-of-rmem-f649885114fd
> 
> Best regards,

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Stephan Gerhold]

Hi Stanimir,

I see that you already tagged the Venus updates for 6.6, but could you
try to still apply this patch as well for 6.6? It's a requirement for
some DT cleanup I'm working on and ideally needs to go in a kernel
release earlier to avoid bisect problems.

AFAICT it's been on the list for more than two months now with two
Reviewed-by, so should be fine to just apply it. :)

Thanks!
Stephan

On Mon, May 29, 2023 at 08:16:14PM +0200, Stephan Gerhold wrote:
> Reserved memory can be either looked up using the generic function
> of_address_to_resource() or using the special of_reserved_mem_lookup().
> The latter has the advantage that it ensures that the referenced memory
> region was really reserved and is not e.g. status = "disabled".
> 
> of_reserved_mem also supports allocating reserved memory dynamically at
> boot time. This works only when using of_reserved_mem_lookup() since
> there won't be a fixed address in the device tree.
> 
> Switch the code to use of_reserved_mem_lookup(). There is no functional
> difference for static reserved memory allocations.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> See e.g. [1] for an example of dynamically allocated reserved memory.
> (This patch does *not* depend on [1] and is useful without as well...)
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20230510-dt-resv-bottom-up-v1-5-3bf68873dbed@gerhold.net/
> ---
>  drivers/media/platform/qcom/venus/firmware.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index cfb11c551167..2e7ffdaff7b2 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -10,6 +10,7 @@
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
>  #include <linux/firmware/qcom/qcom_scm.h>
> @@ -82,9 +83,9 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  			 phys_addr_t *mem_phys, size_t *mem_size)
>  {
>  	const struct firmware *mdt;
> +	struct reserved_mem *rmem;
>  	struct device_node *node;
>  	struct device *dev;
> -	struct resource r;
>  	ssize_t fw_size;
>  	void *mem_va;
>  	int ret;
> @@ -99,13 +100,16 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		return -EINVAL;
>  	}
>  
> -	ret = of_address_to_resource(node, 0, &r);
> -	if (ret)
> -		goto err_put_node;
> +	rmem = of_reserved_mem_lookup(node);
> +	of_node_put(node);
> +	if (!rmem) {
> +		dev_err(dev, "failed to lookup reserved memory-region\n");
> +		return -EINVAL;
> +	}
>  
>  	ret = request_firmware(&mdt, fwname, dev);
>  	if (ret < 0)
> -		goto err_put_node;
> +		return ret;
>  
>  	fw_size = qcom_mdt_get_size(mdt);
>  	if (fw_size < 0) {
> @@ -113,17 +117,17 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		goto err_release_fw;
>  	}
>  
> -	*mem_phys = r.start;
> -	*mem_size = resource_size(&r);
> +	*mem_phys = rmem->base;
> +	*mem_size = rmem->size;
>  
>  	if (*mem_size < fw_size || fw_size > VENUS_FW_MEM_SIZE) {
>  		ret = -EINVAL;
>  		goto err_release_fw;
>  	}
>  
> -	mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
> +	mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
>  	if (!mem_va) {
> -		dev_err(dev, "unable to map memory region: %pR\n", &r);
> +		dev_err(dev, "unable to map memory region %pa size %#zx\n", mem_phys, *mem_size);
>  		ret = -ENOMEM;
>  		goto err_release_fw;
>  	}
> @@ -138,8 +142,6 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  	memunmap(mem_va);
>  err_release_fw:
>  	release_firmware(mdt);
> -err_put_node:
> -	of_node_put(node);
>  	return ret;
>  }
>  
> 
> ---
> base-commit: 9f9f8ca6f012d25428f8605cb36369a449db8508
> change-id: 20230529-venus-of-rmem-f649885114fd
> 
> Best regards,
> -- 
> Stephan Gerhold <stephan@gerhold.net>
> 

Re: [PATCH] media: venus: firmware: Use of_reserved_mem_lookup()

[Stanimir Varbanov]

Hi Stephan,

On 2.08.23 г. 11:57 ч., Stephan Gerhold wrote:
> Hi Stanimir,
> 
> I see that you already tagged the Venus updates for 6.6, but could you
> try to still apply this patch as well for 6.6? It's a requirement for
> some DT cleanup I'm working on and ideally needs to go in a kernel
> release earlier to avoid bisect problems.
> 
> AFAICT it's been on the list for more than two months now with two
> Reviewed-by, so should be fine to just apply it. :)
> 

Done, thanks for the reminder.

-- 
regards,
Stan