Re: [PATCH v2 4/5] media: venus: add no TZ boot and shutdown routine

[Tomasz Figa]

Hi Vikash,

On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@xxxxxxxxxxxxxx> wrote:
[snip]
> +int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,
> +                                                       size_t mem_size)
> +{
> +       struct iommu_domain *iommu;
> +       struct device *dev;
> +       int ret;
> +
> +       if (!core->fw.dev)
> +               return -EPROBE_DEFER;

Is it really possible that the device appears after the probe is retried?

> +
> +       dev = core->fw.dev;
> +
> +       iommu = iommu_domain_alloc(&platform_bus_type);
> +       if (!iommu) {
> +               dev_err(dev, "Failed to allocate iommu domain\n");
> +               return -ENOMEM;
> +       }
> +
> +       ret = iommu_attach_device(iommu, dev);
> +       if (ret) {
> +               dev_err(dev, "could not attach device\n");
> +               goto err_attach;
> +       }
> +
> +       ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
> +                       IOMMU_READ|IOMMU_WRITE|IOMMU_PRIV);
> +       if (ret) {
> +               dev_err(dev, "could not map video firmware region\n");
> +               goto err_map;
> +       }

I'm not very familiar with translation capabilities of ARM SMMU, so
that might be an elementary question, but could you explain how this
works? Will this make the firmware device (transfers with firmware
PASID) use different page directory from the main device (all the
other PASIDs; used with DMA mapping API)?

+Will and Robin, just in case.

> +       core->fw.iommu_domain = iommu;
> +       venus_reset_hw(core);
> +
> +       return 0;
> +
> +err_map:
> +       iommu_detach_device(iommu, dev);
> +err_attach:
> +       iommu_domain_free(iommu);
> +       return ret;
> +}
[snip]
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 0916826..67fdd89 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -14,10 +14,15 @@
>  #ifndef __VENUS_FIRMWARE_H__
>  #define __VENUS_FIRMWARE_H__
>
> +#define VENUS_PAS_ID                   9

Shouldn't this normally be given in DT?

Best regards,
Tomasz