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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 28, 2015 at 4:51 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 09/28, Rob Clark wrote:
>> Add interfaces needed for configuring OCMEM.
>>
>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
>> ---
>>  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
>> index e9c306b..656d8fe 100644
>> --- a/drivers/firmware/qcom_scm-32.c
>> +++ b/drivers/firmware/qcom_scm-32.c
>> @@ -500,6 +500,63 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>>               req, req_cnt * sizeof(*req), resp, sizeof(*resp));
>>  }
>>
>> +int __qcom_scm_ocmem_secure_cfg(unsigned sec_id)
>> +{
>> +     int ret, scm_ret = 0;
>> +     struct msm_scm_sec_cfg {
>> +             unsigned int id;
>> +             unsigned int spare;
>
>
> __le32 for both
>
>> +     } cfg;
>> +
>> +     cfg.id = sec_id;
>> +
>> +
>
> nitpick: drop double space
>
>> +     ret = qcom_scm_call(QCOM_SCM_OCMEM_SECURE_SVC, QCOM_SCM_OCMEM_SECURE_CFG,
>> +                     &cfg, sizeof(cfg), &scm_ret, sizeof(scm_ret));
>> +
>> +     if (ret || scm_ret) {
>> +             pr_err("ocmem: Failed to enable secure programming\n");
>
> Maybe the caller should print something if they care instead of
> burying it down here.
>
>> +             return ret ? ret : -EINVAL;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +int __qcom_scm_ocmem_lock(uint32_t id, uint32_t offset, uint32_t size,
>> +             uint32_t mode)
>
> Please use u32 here instead of uint32_t. uint32_t is not for
> kernel code.
>
>> +{
>> +     struct ocmem_tz_lock {
>> +             u32 id;
>> +             u32 offset;
>> +             u32 size;
>> +             u32 mode;
>
> All __le32
>
>> +     } request;
>> +
>> +     request.id = id;
>> +     request.offset = offset;
>> +     request.size = size;
>> +     request.mode = mode;
>
> And then do the cpu_to_le32() stuff here.
>
>> +
>> +     return qcom_scm_call(QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_LOCK_CMD,
>> +                     &request, sizeof(request), NULL, 0);
>> +}
>> +
>> +int __qcom_scm_ocmem_unlock(uint32_t id, uint32_t offset, uint32_t size)
>
> u32
>
>> +{
>> +     struct ocmem_tz_unlock {
>> +             u32 id;
>> +             u32 offset;
>> +             u32 size;
>
> __le32
>
>> +     } request;
>> +
>> +     request.id = id;
>> +     request.offset = offset;
>> +     request.size = size;
>> +
>> +     return qcom_scm_call(QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_UNLOCK_CMD,
>> +                     &request, sizeof(request), NULL, 0);
>> +}
>> +
>>  bool __qcom_scm_pas_supported(u32 peripheral)
>>  {
>>       __le32 out;
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 118df0a..59b1007 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -154,6 +154,112 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>>  EXPORT_SYMBOL(qcom_scm_hdcp_req);
>>
>>  /**
>> + * qcom_scm_ocmem_secure_available() - Check if secure environment supports
>> + * OCMEM.
>> + *
>> + * Return true if OCMEM secure interface is supported, false if not.
>> + */
>> +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()..


>> +
>> +     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);
>> +
>> +/**
>> + * qcom_scm_ocmem_secure_cfg() - call OCMEM secure cfg interface
>> + */
>> +int qcom_scm_ocmem_secure_cfg(unsigned sec_id)
>> +{
>> +     int ret = qcom_scm_clk_enable();
>> +
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = __qcom_scm_ocmem_secure_cfg(sec_id);
>> +     qcom_scm_clk_disable();
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(qcom_scm_ocmem_secure_cfg);
>> +
>> +/**
>> + * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available
>> + */
>> +bool qcom_scm_ocmem_lock_available(void)
>> +{
>> +     int ret = qcom_scm_clk_enable();
>
> No need for clocks?
>
>> +
>> +     if (ret)
>> +             goto clk_err;
>> +
>> +     ret = __qcom_scm_is_call_available(QCOM_SCM_OCMEM_SVC,
>> +                     QCOM_SCM_OCMEM_LOCK_CMD);
>> +
>> +     qcom_scm_clk_disable();
>> +
>> +clk_err:
>> +     return (ret > 0) ? true : false;
>> +}
>> +EXPORT_SYMBOL(qcom_scm_ocmem_lock_available);
>> +
>> +/**
>> + * qcom_scm_ocmem_lock() - call OCMEM lock interface to assign an OCMEM
>> + * region to the specified initiator
>> + *
>> + * @id:     tz initiator id
>> + * @offset: OCMEM offset
>> + * @size:   OCMEM size
>> + * @mode:   access mode (WIDE/NARROW)
>> + */
>> +int qcom_scm_ocmem_lock(uint32_t id, uint32_t offset, uint32_t size,
>> +             uint32_t mode)
>> +{
>> +     int ret = qcom_scm_clk_enable();
>> +
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = __qcom_scm_ocmem_lock(id, offset, size, mode);
>> +     qcom_scm_clk_disable();
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(qcom_scm_ocmem_lock);
>> +
>> +/**
>> + * qcom_scm_ocmem_unlock() - call OCMEM unlock interface to release an OCMEM
>> + * region from the specified initiator
>> + *
>> + * @id:     tz initiator id
>> + * @offset: OCMEM offset
>> + * @size:   OCMEM size
>> + */
>> +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..

>> 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..

BR,
-R

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux