Re: [PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine

[Vikash Garodia <vgarodia@codeaurora.org>]

On 2018-08-27 08:36, Alexandre Courbot wrote:
> On Fri, Aug 24, 2018 at 9:26 PM Vikash Garodia 
> <vgarodia@codeaurora.org> wrote:
>> 
>> Hi Alex,
>> 
>> On 2018-08-24 13:09, Alexandre Courbot wrote:
>> > On Thu, Aug 23, 2018 at 11:29 PM Vikash Garodia
>> > <vgarodia@codeaurora.org> wrote:
>> 
>> [snip]
>> 
>> >> +struct video_firmware {
>> >> +       struct device *dev;
>> >> +       struct iommu_domain *iommu_domain;
>> >> +};
>> >> +
>> >>  /**
>> >>   * struct venus_core - holds core parameters valid for all instances
>> >>   *
>> >> @@ -98,6 +103,7 @@ struct venus_caps {
>> >>   * @dev:               convenience struct device pointer
>> >>   * @dev_dec:   convenience struct device pointer for decoder device
>> >>   * @dev_enc:   convenience struct device pointer for encoder device
>> >> + * @fw:                a struct for venus firmware info
>> >>   * @no_tz:     a flag that suggests presence of trustzone
>> >>   * @lock:      a lock for this strucure
>> >>   * @instances: a list_head of all instances
>> >> @@ -130,6 +136,7 @@ struct venus_core {
>> >>         struct device *dev;
>> >>         struct device *dev_dec;
>> >>         struct device *dev_enc;
>> >> +       struct video_firmware fw;
>> >
>> > Since struct video_firmware is only used here I think you can declare
>> > it inline, i.e.
>> >
>> >     struct {
>> >         struct device *dev;
>> >         struct iommu_domain *iommu_domain;
>> >     } fw;
>> >
>> > This structure is actually a good candidate to hold the firmware
>> > memory area start address and size.
>> 
>> I can make it inline.
>> Memory area and size are common parameters populated
>> locally while loading the firmware with or without tz. Firmware struct
>> has
>> info more specific to firmware device.
>> 
>> [snip]
>> 
>> >
>> >> +{
>> >> +       struct iommu_domain *iommu_dom;
>> >> +       struct device *dev;
>> >> +       int ret;
>> >> +
>> >> +       dev = core->fw.dev;
>> >> +       if (!dev)
>> >> +               return -EPROBE_DEFER;
>> >> +
>> >> +       iommu_dom = iommu_domain_alloc(&platform_bus_type);
>> >> +       if (!iommu_dom) {
>> >> +               dev_err(dev, "Failed to allocate iommu domain\n");
>> >> +               return -ENOMEM;
>> >> +       }
>> >> +
>> >> +       ret = iommu_attach_device(iommu_dom, dev);
>> >> +       if (ret) {
>> >> +               dev_err(dev, "could not attach device\n");
>> >> +               goto err_attach;
>> >> +       }
>> >
>> > I think like the above belongs more in venus_firmware_init()
>> > (introduced in patch 4/4) than here. There is no reason to
>> > detach/reattach the iommu if we stop the firmware.
>> 
>> Consider the case when we want to reload the firmware during error
>> recovery.
>> Boot and shutdown will be needed in such case without the need to
>> populate
>> the firmware device again.
> 
> Is there a need to reattach the iommu domain in case of an error?

re-attach is not needed. We can have alloc/attach in init and 
detach/free in deinit.
map/reset and unmap/reset can continue to remain in boot and shutdown 
calls. Let me
know if this is good, i can repatch the series.