Re: [PATCH v9 1/5] venus: firmware: add routine to reset ARM9

[Vikash Garodia]

Hi Stanimir,

On 2018-10-01 20:00, Stanimir Varbanov wrote:
> Hi,
>
> On 09/20/2018 06:31 AM, Alexandre Courbot wrote:
> > On Thu, Sep 20, 2018 at 2:55 AM Vikash Garodia 
> > <vgarodia@xxxxxxxxxxxxxx> wrote:
> > > On 2018-09-19 20:30, Stanimir Varbanov wrote:
> > > > Hi Alex,
> > > >
> > > > On 09/19/2018 10:32 AM, Alexandre Courbot wrote:
> > > > > On Wed, Sep 19, 2018 at 8:43 AM Vikash Garodia
> > > > > <vgarodia@xxxxxxxxxxxxxx> wrote:
> > > > > > Add routine to reset the ARM9 and brings it out of reset. Also
> > > > > > abstract the Venus CPU state handling with a new function. This
> > > > > > is in preparation to add PIL functionality in venus driver.
> > > > > >
> > > > > > Signed-off-by: Vikash Garodia <vgarodia@xxxxxxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/media/platform/qcom/venus/core.h         |  2 ++
> > > > > >  drivers/media/platform/qcom/venus/firmware.c     | 33
> > > > > > ++++++++++++++++++++++++
> > > > > >  drivers/media/platform/qcom/venus/firmware.h     | 11 ++++++++
> > > > > >  drivers/media/platform/qcom/venus/hfi_venus.c    | 13 +++-------
> > > > > >  drivers/media/platform/qcom/venus/hfi_venus_io.h |  7 +++++
> > > > > >  5 files changed, 57 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/platform/qcom/venus/core.h
> > > > > > b/drivers/media/platform/qcom/venus/core.h
> > > > > > index 2f02365..dfd5c10 100644
> > > > > > --- a/drivers/media/platform/qcom/venus/core.h
> > > > > > +++ b/drivers/media/platform/qcom/venus/core.h
> > > > > > @@ -98,6 +98,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
> > > > > > + * @no_tz:     a flag that suggests presence of trustzone
> > > > >
> > > > > Looks like it suggests the absence of trustzone?
> > > > >
> > > > > Actually I would rename it as use_tz and set it if TrustZone is 
> > > > > used.
> > > > > This would avoid double-negative statements like what we see below.
> > > >
> > > > I find this suggestion reasonable.
> > >
> > > Initially i planned to keep it as a positive flag. The reason behind
> > > keeping it
> > > as no_tz was to keep the default value of this flag to 0 indicating 
> > > tz
> > > is present
> > > by default.
> > > I can switch it to use_tz though and set it in firmware_init after 
> > > the
> > > presence of
> > > firmware node is checked.
> >
> > Making sure the flag is explicitly initialized instead of relying on
> > default initialization is another good reason to have that change
> > IMHO. :)
>
> Vikash, care to send a new version, or will fix that with follow up 
> patches?

I will provide a new series with the suggested change.

Thanks,
Vikash