[PATCH 1/4] qcom-scm: add ocmem support

[Bjorn Andersson]

On Mon 28 Sep 14:08 PDT 2015, Rob Clark wrote:

> On Mon, Sep 28, 2015 at 4:51 PM, Stephen Boyd <sboyd at codeaurora.org> wrote:
> > On 09/28, Rob Clark wrote:
> >> Add interfaces needed for configuring OCMEM.
> >>
> >> Signed-off-by: Rob Clark <robdclark at gmail.com>
> >> ---
> >>  drivers/firmware/qcom_scm-32.c |  57 ++++++++++++++++++++++
> >>  drivers/firmware/qcom_scm-64.c |  16 +++++++
> >>  drivers/firmware/qcom_scm.c    | 106 +++++++++++++++++++++++++++++++++++++++++
> >>  drivers/firmware/qcom_scm.h    |  13 +++++
> >>  include/linux/qcom_scm.h       |  10 ++++
> >>  5 files changed, 202 insertions(+)
> >>
> >> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
[..]
> >> +bool qcom_scm_ocmem_secure_available(void)
> >> +{
> >> +     int ret = qcom_scm_clk_enable();
> >
> > I doubt we need to enable clocks to figure out if a call is
> > available. Please drop clk stuff here.
> 
> hmm, hdcp did, but pas didn't..  otoh it looks like the call to
> __qcom_scm_pas_supported() *should* be wrapped in clk enable/disable..
> 
> And __qcom_scm_is_call_available() does call qcom_scm_call().  Which
> is, I assume, what needs the clk's..  so not entirely sure if *all*
> the clk enable/disable stuff should be stripped out, or if missing clk
> stuff should be added in qcom_scm_pas_supported()..
> 

The scm clocks here are the crypto engine clocks, they are not needed to
check if TZ implements PAS for a given processor or not.

But it could be argued that this is simply an assumption I make of the
black box we're calling into...

> 
> >> +
> >> +     if (ret)
> >> +             goto clk_err;
> >> +
> >> +     ret = __qcom_scm_is_call_available(QCOM_SCM_OCMEM_SECURE_SVC,
> >> +                     QCOM_SCM_OCMEM_SECURE_CFG);
> >> +
> >> +     qcom_scm_clk_disable();
> >> +
> >> +clk_err:
> >> +     return (ret > 0) ? true : false;
> >> +}
> >> +EXPORT_SYMBOL(qcom_scm_ocmem_secure_available);
> >> +
[..]
> >> +int qcom_scm_ocmem_unlock(uint32_t id, uint32_t offset, uint32_t size)
> >> +{
> >> +     int ret = qcom_scm_clk_enable();
> >> +
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     ret = __qcom_scm_ocmem_unlock(id, offset, size);
> >> +     qcom_scm_clk_disable();
> >> +
> >> +     return ret;
> >> +}
> >> +EXPORT_SYMBOL(qcom_scm_ocmem_unlock);
> >
> > I don't think we need any clocks for lock/unlock/cfg either. The
> > scm clocks are some crypto clocks that the secure side isn't able
> > to enable and we don't have a device in DT for them. In the ocmem
> > case, we should rely on the ocmem device to get the clocks and
> > turn them on before calling any scm APIs that may require those
> > clocks.
> 
> Hmm, if that is true then we should probably drop the clks for hdcp
> fxns too, and maybe add a comment somewhere since it isn't really
> clear what the clks are for (and when it is unclear, folks will just
> cargo-cult what the existing fxns are doing).  As-is it is hard to
> tell what is required and what is luck..
> 

I would expect hdcp to use the crypto engines in some way and we don't
want people to feel that they should add the random clocks here, so
commenting them is probably the way to go.

> >> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> >> index 46d41e4..a934457 100644
> >> --- a/include/linux/qcom_scm.h
> >> +++ b/include/linux/qcom_scm.h
> >> @@ -23,10 +23,20 @@ struct qcom_scm_hdcp_req {
> >>       u32 val;
> >>  };
> >>
> >> +extern bool qcom_scm_is_available(void);
> >
> > Is this used? Looks like noise.
> 
> perhaps should be split out into a separate patch..  but I am using
> this, and it seems like a good idea to avoid null ptr deref's of
> __scm.  Probably some of the scm callers should call this first..
> either that or we should make other scm entry points behave better if
> __scm is null..
> 

This is part of Andy's platformication, didn't he export it properly?
I use it as well from the remoteproc.

Regards,
Bjorn