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

[Stephen Boyd <sboyd@codeaurora.org>]

On 09/28, Rob Clark wrote:
> Add interfaces needed for configuring OCMEM.
> 
> Signed-off-by: Rob Clark <robdclark@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
> 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.

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

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project