On 19/07/17 13:51, Stanimir Varbanov wrote: > In venus_boot(), we pass a pointer to a phys_addr_t > into dmam_alloc_coherent, which the compiler warns about: > > platform/qcom/venus/firmware.c: In function 'venus_boot': > platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of > 'dmam_alloc_coherent' from incompatible pointer type > [-Werror=incompatible-pointer-types] > > To avoid the error refactor venus_boot function by discard > dma_alloc_coherent invocation because we don't want to map the > memory for the device. Something more, the usage of > DMA mapping API is actually wrong and the current > implementation relies on several bugs in DMA mapping code. > When these bugs are fixed that will break firmware loading, > so fix this now to avoid future troubles. > > The meaning of venus_boot is to copy the content of the > firmware buffer into reserved (and memblock removed) > block of memory and pass that physical address to the > trusted zone for authentication and mapping through iommu > form the secure world. After iommu mapping is done the iova > is passed as ane entry point to the remote processor. > > After this change memory-region property is parsed manually > and the physical address is memremap to CPU, call mdt_load to > load firmware segments into proper places and unmap > reserved memory. > > Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper > functions") > Signed-off-by: Stanimir Varbanov <stanimir.varba...@linaro.org>
Arnd, is this OK for you? Regards, Hans > --- > this is v2 of the patch 2/4 form this [1] patchset. The only > change is rewording in the patch description. > > [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg115717.html > > drivers/media/platform/qcom/venus/core.c | 10 ++-- > drivers/media/platform/qcom/venus/core.h | 1 - > drivers/media/platform/qcom/venus/firmware.c | 74 > ++++++++++++---------------- > drivers/media/platform/qcom/venus/firmware.h | 5 +- > 4 files changed, 39 insertions(+), 51 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c > b/drivers/media/platform/qcom/venus/core.c > index 694f57a78288..a70368cb713f 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct > *work) > hfi_core_deinit(core, true); > hfi_destroy(core); > mutex_lock(&core->lock); > - venus_shutdown(&core->dev_fw); > + venus_shutdown(core->dev); > > pm_runtime_put_sync(core->dev); > > @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct > *work) > > pm_runtime_get_sync(core->dev); > > - ret |= venus_boot(core->dev, &core->dev_fw, core->res->fwname); > + ret |= venus_boot(core->dev, core->res->fwname); > > ret |= hfi_core_resume(core, true); > > @@ -207,7 +207,7 @@ static int venus_probe(struct platform_device *pdev) > if (ret < 0) > goto err_runtime_disable; > > - ret = venus_boot(dev, &core->dev_fw, core->res->fwname); > + ret = venus_boot(dev, core->res->fwname); > if (ret) > goto err_runtime_disable; > > @@ -238,7 +238,7 @@ static int venus_probe(struct platform_device *pdev) > err_core_deinit: > hfi_core_deinit(core, false); > err_venus_shutdown: > - venus_shutdown(&core->dev_fw); > + venus_shutdown(dev); > err_runtime_disable: > pm_runtime_set_suspended(dev); > pm_runtime_disable(dev); > @@ -259,7 +259,7 @@ static int venus_remove(struct platform_device *pdev) > WARN_ON(ret); > > hfi_destroy(core); > - venus_shutdown(&core->dev_fw); > + venus_shutdown(dev); > of_platform_depopulate(dev); > > pm_runtime_put_sync(dev); > diff --git a/drivers/media/platform/qcom/venus/core.h > b/drivers/media/platform/qcom/venus/core.h > index e542700eee32..cba092bcb76d 100644 > --- a/drivers/media/platform/qcom/venus/core.h > +++ b/drivers/media/platform/qcom/venus/core.h > @@ -101,7 +101,6 @@ struct venus_core { > struct device *dev; > struct device *dev_dec; > struct device *dev_enc; > - struct device dev_fw; > struct mutex lock; > struct list_head instances; > atomic_t insts_count; > diff --git a/drivers/media/platform/qcom/venus/firmware.c > b/drivers/media/platform/qcom/venus/firmware.c > index 1b1a4f355918..d6d9560c1c19 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -12,29 +12,27 @@ > * > */ > > -#include <linux/dma-mapping.h> > +#include <linux/device.h> > #include <linux/firmware.h> > #include <linux/kernel.h> > +#include <linux/io.h> > #include <linux/of.h> > -#include <linux/of_reserved_mem.h> > -#include <linux/slab.h> > +#include <linux/of_address.h> > #include <linux/qcom_scm.h> > +#include <linux/sizes.h> > #include <linux/soc/qcom/mdt_loader.h> > > #include "firmware.h" > > #define VENUS_PAS_ID 9 > -#define VENUS_FW_MEM_SIZE SZ_8M > +#define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > -static void device_release_dummy(struct device *dev) > -{ > - of_reserved_mem_device_release(dev); > -} > - > -int venus_boot(struct device *parent, struct device *fw_dev, const char > *fwname) > +int venus_boot(struct device *dev, const char *fwname) > { > const struct firmware *mdt; > + struct device_node *node; > phys_addr_t mem_phys; > + struct resource r; > ssize_t fw_size; > size_t mem_size; > void *mem_va; > @@ -43,66 +41,58 @@ int venus_boot(struct device *parent, struct device > *fw_dev, const char *fwname) > if (!qcom_scm_is_available()) > return -EPROBE_DEFER; > > - fw_dev->parent = parent; > - fw_dev->release = device_release_dummy; > + node = of_parse_phandle(dev->of_node, "memory-region", 0); > + if (!node) { > + dev_err(dev, "no memory-region specified\n"); > + return -EINVAL; > + } > > - ret = dev_set_name(fw_dev, "%s:%s", dev_name(parent), "firmware"); > + ret = of_address_to_resource(node, 0, &r); > if (ret) > return ret; > > - ret = device_register(fw_dev); > - if (ret < 0) > - return ret; > + mem_phys = r.start; > + mem_size = resource_size(&r); > > - ret = of_reserved_mem_device_init_by_idx(fw_dev, parent->of_node, 0); > - if (ret) > - goto err_unreg_device; > + if (mem_size < VENUS_FW_MEM_SIZE) > + return -EINVAL; > > - mem_size = VENUS_FW_MEM_SIZE; > - > - mem_va = dmam_alloc_coherent(fw_dev, mem_size, &mem_phys, GFP_KERNEL); > + mem_va = memremap(r.start, mem_size, MEMREMAP_WC); > if (!mem_va) { > - ret = -ENOMEM; > - goto err_unreg_device; > + dev_err(dev, "unable to map memory region: %pa+%zx\n", > + &r.start, mem_size); > + return -ENOMEM; > } > > - ret = request_firmware(&mdt, fwname, fw_dev); > + ret = request_firmware(&mdt, fwname, dev); > if (ret < 0) > - goto err_unreg_device; > + goto err_unmap; > > fw_size = qcom_mdt_get_size(mdt); > if (fw_size < 0) { > ret = fw_size; > release_firmware(mdt); > - goto err_unreg_device; > + goto err_unmap; > } > > - ret = qcom_mdt_load(fw_dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys, > + ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys, > mem_size); > > release_firmware(mdt); > > if (ret) > - goto err_unreg_device; > + goto err_unmap; > > ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID); > if (ret) > - goto err_unreg_device; > - > - return 0; > + goto err_unmap; > > -err_unreg_device: > - device_unregister(fw_dev); > +err_unmap: > + memunmap(mem_va); > return ret; > } > > -int venus_shutdown(struct device *fw_dev) > +int venus_shutdown(struct device *dev) > { > - int ret; > - > - ret = qcom_scm_pas_shutdown(VENUS_PAS_ID); > - device_unregister(fw_dev); > - memset(fw_dev, 0, sizeof(*fw_dev)); > - > - return ret; > + return qcom_scm_pas_shutdown(VENUS_PAS_ID); > } > diff --git a/drivers/media/platform/qcom/venus/firmware.h > b/drivers/media/platform/qcom/venus/firmware.h > index f81a98979798..428efb56d339 100644 > --- a/drivers/media/platform/qcom/venus/firmware.h > +++ b/drivers/media/platform/qcom/venus/firmware.h > @@ -16,8 +16,7 @@ > > struct device; > > -int venus_boot(struct device *parent, struct device *fw_dev, > - const char *fwname); > -int venus_shutdown(struct device *fw_dev); > +int venus_boot(struct device *dev, const char *fwname); > +int venus_shutdown(struct device *dev); > > #endif >