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
