lkml.org 
[lkml]   [2018]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 3/4] venus: firmware: add no TZ boot and shutdown routine
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.

\
 
 \ /
  Last update: 2018-08-27 14:50    [W:0.057 / U:1.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site