[PATCH 4/6] qcom-scm: add ocmem support
On Tue, Sep 29, 2015 at 6:33 PM, Stephen Boyd wrote:
> On 09/29, Rob Clark wrote:
>> On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd
>> wrote:
>> > On 09/29, Rob Clark wrote:
>> >> diff --git a/drivers/firmware/qcom_scm-32.c
>> >> b/drivers/firmware/qcom_scm-32.c
>> >> index c1e4325..e1ac97f 100644
>> >> --- a/drivers/firmware/qcom_scm-32.c
>> >> +++ b/drivers/firmware/qcom_scm-32.c
>> >> @@ -500,6 +500,59 @@ 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 {
>> >
>> > We've left these as anonymous structs for things like
>> > qcom_scm_set_boot_addr(), maybe we should do the same here.
>> >
>> >> + __le32 id;
>> >> + __le32 spare;
>> >
>> > Also, the iommu driver would use this API and it uses this
>> > "spare" element, so perhaps this whole function should be renamed
>> > to be more generic and take two values. Downstream the function
>> > is called scm_restore_sec_cfg, so maybe something similar. And
>> > the service id is MP for "memory protection", so
>> > QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION?
>>
>> heh,
>>
>> #define SCM_SVC_MP0xC
>> #define IOMMU_SECURE_CFG2
>>
>> vs.
>>
>> #define OCMEM_SECURE_SVC_ID 12
>> #define OCMEM_SECURE_CFG_ID 0x2
>>
>> that wasn't obscure at all!
>
> :)
>
>>
>> Maybe then there is a better name than spare? Looks like downstream
>> iommu calls it cb_num?
>
> Yeah I think that's the only use to indicate which context bank
> it is. Maybe we can have a single id configure API and a special
> iommu context bank API that both funnel into the same private two
> number API. Otherwise we have a bunch of callers passing 0 for
> the second argument because they don't care.
so fwiw, I went thru all the downstream scm_call() callers.. there
are a lot of callers to SCM_SVC_MP service (through a couple different
#defines), but most of them are different cmd-id's. The ones using
SECURE_CFG (0x2) are:
* dwc3_msm_restore_sec_config()
* ocmem_restore_sec_program()
* msm_iommu_sec_program_iommu()
so we have two points passing in zero for ctx-bank, one that does not.
I don't think it is worth having two API's to save hard-coding zero in
two places ;-)
BR,
-R
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[PATCH 4/6] qcom-scm: add ocmem support
On 10/01, Rob Clark wrote: > On Tue, Sep 29, 2015 at 6:33 PM, Stephen Boyd wrote: > > On 09/29, Rob Clark wrote: > >> On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd > >> wrote: > > > > Yeah I think that's the only use to indicate which context bank > > it is. Maybe we can have a single id configure API and a special > > iommu context bank API that both funnel into the same private two > > number API. Otherwise we have a bunch of callers passing 0 for > > the second argument because they don't care. > > so fwiw, I went thru all the downstream scm_call() callers.. there > are a lot of callers to SCM_SVC_MP service (through a couple different > #defines), but most of them are different cmd-id's. The ones using > SECURE_CFG (0x2) are: > > * dwc3_msm_restore_sec_config() > * ocmem_restore_sec_program() > * msm_iommu_sec_program_iommu() > > so we have two points passing in zero for ctx-bank, one that does not. > I don't think it is worth having two API's to save hard-coding zero in > two places ;-) > What sources are you looking at? It seems like whatever you have is over a year old. About a year ago, we consolidated all calls to this specific SCM call into a single API called scm_restore_sec_cfg (see commit 9933a272db9a5612bcc2ee0ef9149f70c8166eb3 "qcom: scm: Provide an API that restores security configuration" on msm-3.10). Looking at our latest msm-3.10 branch I see drivers/crypto/msm/ice.c:ret = scm_restore_sec_cfg(cbuf.device_id, cbuf.spare, _ret); cbuf.spare is 0 here. drivers/iommu/msm_iommu_sec.c: ret = scm_restore_sec_cfg(drvdata->sec_id, ctx_drvdata->num, _ret); This is the only real user of spare drivers/pci/host/pci-msm.c: ret = scm_restore_sec_cfg(dev->scm_dev_id, 0, _ret); drivers/scsi/ufs/ufs-qcom.c: ret = scm_restore_sec_cfg(cbuf.device_id, cbuf.spare, _ret); cbuf.spare is 0 here. drivers/soc/qcom/ocmem_core.c: rc = scm_restore_sec_cfg(sec_id, 0, _ret); drivers/video/msm/mdss/mdss_mdp.c: ret = scm_restore_sec_cfg(SEC_DEVICE_MDSS, 0, _ret); So that's 6 callers and 1 uses the second argument. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH 4/6] qcom-scm: add ocmem support
On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd wrote:
> On 09/29, Rob Clark wrote:
>> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
>> index c1e4325..e1ac97f 100644
>> --- a/drivers/firmware/qcom_scm-32.c
>> +++ b/drivers/firmware/qcom_scm-32.c
>> @@ -500,6 +500,59 @@ 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 {
>
> We've left these as anonymous structs for things like
> qcom_scm_set_boot_addr(), maybe we should do the same here.
>
>> + __le32 id;
>> + __le32 spare;
>
> Also, the iommu driver would use this API and it uses this
> "spare" element, so perhaps this whole function should be renamed
> to be more generic and take two values. Downstream the function
> is called scm_restore_sec_cfg, so maybe something similar. And
> the service id is MP for "memory protection", so
> QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION?
heh,
#define SCM_SVC_MP0xC
#define IOMMU_SECURE_CFG2
vs.
#define OCMEM_SECURE_SVC_ID 12
#define OCMEM_SECURE_CFG_ID 0x2
that wasn't obscure at all!
Maybe then there is a better name than spare? Looks like downstream
iommu calls it cb_num?
BR,
-R
> Otherwise this patch looks good.
>
>> + } cfg;
>> +
>> + cfg.id = cpu_to_le32(sec_id);
>> +
>> + ret = qcom_scm_call(QCOM_SCM_OCMEM_SECURE_SVC,
>> QCOM_SCM_OCMEM_SECURE_CFG,
>> + , sizeof(cfg), _ret, sizeof(scm_ret));
>> +
>> + if (ret || scm_ret)
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[PATCH 4/6] qcom-scm: add ocmem support
Add interfaces needed for configuring OCMEM.
Signed-off-by: Rob Clark
---
drivers/firmware/qcom_scm-32.c | 53
drivers/firmware/qcom_scm-64.c | 16 +++
drivers/firmware/qcom_scm.c| 61 ++
drivers/firmware/qcom_scm.h| 12 +
include/linux/qcom_scm.h | 7 +
5 files changed, 149 insertions(+)
diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index c1e4325..e1ac97f 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -500,6 +500,59 @@ 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 {
+ __le32 id;
+ __le32 spare;
+ } cfg;
+
+ cfg.id = cpu_to_le32(sec_id);
+
+ ret = qcom_scm_call(QCOM_SCM_OCMEM_SECURE_SVC,
QCOM_SCM_OCMEM_SECURE_CFG,
+ , sizeof(cfg), _ret, sizeof(scm_ret));
+
+ if (ret || scm_ret)
+ return ret ? ret : -EINVAL;
+
+ return 0;
+}
+
+int __qcom_scm_ocmem_lock(u32 id, u32 offset, u32 size, u32 mode)
+{
+ struct ocmem_tz_lock {
+ __le32 id;
+ __le32 offset;
+ __le32 size;
+ __le32 mode;
+ } request;
+
+ request.id = cpu_to_le32(id);
+ request.offset = cpu_to_le32(offset);
+ request.size = cpu_to_le32(size);
+ request.mode = cpu_to_le32(mode);
+
+ return qcom_scm_call(QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_LOCK_CMD,
+ , sizeof(request), NULL, 0);
+}
+
+int __qcom_scm_ocmem_unlock(u32 id, u32 offset, u32 size)
+{
+ struct ocmem_tz_unlock {
+ __le32 id;
+ __le32 offset;
+ __le32 size;
+ } request;
+
+ request.id = cpu_to_le32(id);
+ request.offset = cpu_to_le32(offset);
+ request.size = cpu_to_le32(size);
+
+ return qcom_scm_call(QCOM_SCM_OCMEM_SVC, QCOM_SCM_OCMEM_UNLOCK_CMD,
+ , sizeof(request), NULL, 0);
+}
+
bool __qcom_scm_pas_supported(u32 peripheral)
{
__le32 out;
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index e64fd92..ef5c59e 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -62,6 +62,22 @@ int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32
req_cnt, u32 *resp)
return -ENOTSUPP;
}
+int __qcom_scm_ocmem_secure_cfg(unsigned sec_id)
+{
+ return -ENOTSUPP;
+}
+
+int __qcom_scm_ocmem_lock(uint32_t id, uint32_t offset, uint32_t size,
+ uint32_t mode)
+{
+ return -ENOTSUPP;
+}
+
+int __qcom_scm_ocmem_unlock(uint32_t id, uint32_t offset, uint32_t size)
+{
+ return -ENOTSUPP;
+}
+
bool __qcom_scm_pas_supported(u32 peripheral)
{
return false;
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 118df0a..83a6b77 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -154,6 +154,67 @@ 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)
+{
+ return __qcom_scm_is_call_available(QCOM_SCM_OCMEM_SECURE_SVC,
+ QCOM_SCM_OCMEM_SECURE_CFG);
+}
+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)
+{
+ return __qcom_scm_ocmem_secure_cfg(sec_id);
+}
+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)
+{
+ return __qcom_scm_is_call_available(QCOM_SCM_OCMEM_SVC,
+ QCOM_SCM_OCMEM_LOCK_CMD);
+}
+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(u32 id, u32 offset, u32 size, u32 mode)
+{
+ return __qcom_scm_ocmem_lock(id, offset, size, mode);
+}
+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(u32 id, u32 offset, u32 size)
+{
+ return __qcom_scm_ocmem_unlock(id, offset,
[PATCH 4/6] qcom-scm: add ocmem support
On 09/29, Rob Clark wrote:
> On Tue, Sep 29, 2015 at 5:38 PM, Stephen Boyd wrote:
> > On 09/29, Rob Clark wrote:
> >> diff --git a/drivers/firmware/qcom_scm-32.c
> >> b/drivers/firmware/qcom_scm-32.c
> >> index c1e4325..e1ac97f 100644
> >> --- a/drivers/firmware/qcom_scm-32.c
> >> +++ b/drivers/firmware/qcom_scm-32.c
> >> @@ -500,6 +500,59 @@ 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 {
> >
> > We've left these as anonymous structs for things like
> > qcom_scm_set_boot_addr(), maybe we should do the same here.
> >
> >> + __le32 id;
> >> + __le32 spare;
> >
> > Also, the iommu driver would use this API and it uses this
> > "spare" element, so perhaps this whole function should be renamed
> > to be more generic and take two values. Downstream the function
> > is called scm_restore_sec_cfg, so maybe something similar. And
> > the service id is MP for "memory protection", so
> > QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION?
>
> heh,
>
> #define SCM_SVC_MP0xC
> #define IOMMU_SECURE_CFG2
>
> vs.
>
> #define OCMEM_SECURE_SVC_ID 12
> #define OCMEM_SECURE_CFG_ID 0x2
>
> that wasn't obscure at all!
:)
>
> Maybe then there is a better name than spare? Looks like downstream
> iommu calls it cb_num?
Yeah I think that's the only use to indicate which context bank
it is. Maybe we can have a single id configure API and a special
iommu context bank API that both funnel into the same private two
number API. Otherwise we have a bunch of callers passing 0 for
the second argument because they don't care.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 4/6] qcom-scm: add ocmem support
On 09/29, Rob Clark wrote:
> diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> index c1e4325..e1ac97f 100644
> --- a/drivers/firmware/qcom_scm-32.c
> +++ b/drivers/firmware/qcom_scm-32.c
> @@ -500,6 +500,59 @@ 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 {
We've left these as anonymous structs for things like
qcom_scm_set_boot_addr(), maybe we should do the same here.
> + __le32 id;
> + __le32 spare;
Also, the iommu driver would use this API and it uses this
"spare" element, so perhaps this whole function should be renamed
to be more generic and take two values. Downstream the function
is called scm_restore_sec_cfg, so maybe something similar. And
the service id is MP for "memory protection", so
QCOM_SCM_OCMEM_SECURE_SVC could be QCOM_SCM_MEMORY_PROTECTION?
Otherwise this patch looks good.
> + } cfg;
> +
> + cfg.id = cpu_to_le32(sec_id);
> +
> + ret = qcom_scm_call(QCOM_SCM_OCMEM_SECURE_SVC,
> QCOM_SCM_OCMEM_SECURE_CFG,
> + , sizeof(cfg), _ret, sizeof(scm_ret));
> +
> + if (ret || scm_ret)
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
