| Message ID | 20230529-venus-of-rmem-v1-1-dfcdc5047ffb@gerhold.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: venus: firmware: Use of_reserved_mem_lookup() | expand |
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,
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
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,
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/
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
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
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,
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
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> >
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.
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; }
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(-) --- base-commit: 9f9f8ca6f012d25428f8605cb36369a449db8508 change-id: 20230529-venus-of-rmem-f649885114fd Best regards,