| Message ID | 1443466314-1810-2-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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.
On Mon, Sep 28, 2015 at 4:51 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > 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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 28 Sep 14:08 PDT 2015, Rob Clark wrote: > On Mon, Sep 28, 2015 at 4:51 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > 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 [..] > >> +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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/28, Bjorn Andersson wrote: > On Mon 28 Sep 14:08 PDT 2015, Rob Clark wrote: > > > On Mon, Sep 28, 2015 at 4:51 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > On 09/28, Rob Clark wrote: > > >> +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... Let's not make assumptions. They're not needed to check if it has support for something. > > > > > >> + > > >> + 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. Yes HDCP uses crypto for something so those clock calls should stay. If the clocks were used by a HDCP device then it would be like the ocmem case, but it isn't. > > > >> 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. Do we probe defer ocmem if scm isn't ready? Maybe we should name it qcom_scm_is_probed() and have it return -EPROBE_DEFER if it isn't probed and 0 if it is probed. Then drivers just call that function and return the error if there is one. I'd rather not litter scm_*() APIs with checks to see if the driver has probed yet. Just let those crash the system. Of course, this probably doesn't matter because we don't need to do any clock stuff here anyway.
On Mon, Sep 28, 2015 at 6:35 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/28, Bjorn Andersson wrote: >> On Mon 28 Sep 14:08 PDT 2015, Rob Clark wrote: >> >> > On Mon, Sep 28, 2015 at 4:51 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> > > On 09/28, Rob Clark wrote: >> > >> +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... > > Let's not make assumptions. They're not needed to check if it has > support for something. > >> >> > >> > >> + >> > >> + 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. > > Yes HDCP uses crypto for something so those clock calls should > stay. If the clocks were used by a HDCP device then it would be > like the ocmem case, but it isn't. > >> >> > >> 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. > > Do we probe defer ocmem if scm isn't ready? Maybe we should name > it qcom_scm_is_probed() and have it return -EPROBE_DEFER if it > isn't probed and 0 if it is probed. Then drivers just call that > function and return the error if there is one. I'd rather not > litter scm_*() APIs with checks to see if the driver has probed > yet. Just let those crash the system. Of course, this probably > doesn't matter because we don't need to do any clock stuff here > anyway. currently, we just skip the scm calls.. which is maybe not the right thing. With the various kconfig options in downstream driver, I can't say I'm really sure if there are any cases where we would skip scm completely and do everything from kernel.. That said, I'm not a huge fan of 'crash the system' approach in general.. it's kinda nice if the system can at least boot far enough to tell what went wrong, especially on devices without serial ports (or jtag, etc). If I can assume there should always be scm, then an 'if (!qcom_scm_is_available()) return -EPROBE_DEFER;' somewhere early on in the probe seems like the right thing. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + } cfg; + + cfg.id = sec_id; + + + 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"); + return ret ? ret : -EINVAL; + } + + return 0; +} + +int __qcom_scm_ocmem_lock(uint32_t id, uint32_t offset, uint32_t size, + uint32_t mode) +{ + struct ocmem_tz_lock { + u32 id; + u32 offset; + u32 size; + u32 mode; + } request; + + request.id = id; + request.offset = offset; + request.size = size; + request.mode = mode; + + 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) +{ + struct ocmem_tz_unlock { + u32 id; + u32 offset; + u32 size; + } 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-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..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(); + + 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(); + + 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); + +/** * qcom_scm_pas_supported() - Check if the peripheral authentication service is * available for the given peripherial * @peripheral: peripheral id diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h index 220d19c..e01656f3 100644 --- a/drivers/firmware/qcom_scm.h +++ b/drivers/firmware/qcom_scm.h @@ -36,6 +36,19 @@ extern int __qcom_scm_is_call_available(u32 svc_id, u32 cmd_id); extern int __qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp); +#define QCOM_SCM_OCMEM_SECURE_SVC 0xc +#define QCOM_SCM_OCMEM_SECURE_CFG 0x2 + +extern int __qcom_scm_ocmem_secure_cfg(unsigned sec_id); + +#define QCOM_SCM_OCMEM_SVC 0xf +#define QCOM_SCM_OCMEM_LOCK_CMD 0x1 +#define QCOM_SCM_OCMEM_UNLOCK_CMD 0x2 + +extern int __qcom_scm_ocmem_lock(uint32_t id, uint32_t offset, uint32_t size, + uint32_t mode); +extern int __qcom_scm_ocmem_unlock(uint32_t id, uint32_t offset, uint32_t size); + #define QCOM_SCM_SVC_PIL 0x2 #define QCOM_SCM_PAS_INIT_IMAGE_CMD 0x1 #define QCOM_SCM_PAS_MEM_SETUP_CMD 0x2 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); + extern bool qcom_scm_hdcp_available(void); extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp); +extern bool qcom_scm_ocmem_secure_available(void); +extern int qcom_scm_ocmem_secure_cfg(unsigned sec_id); + +extern bool qcom_scm_ocmem_lock_available(void); +extern int qcom_scm_ocmem_lock(uint32_t id, uint32_t offset, uint32_t size, + uint32_t mode); +extern int qcom_scm_ocmem_unlock(uint32_t id, uint32_t offset, uint32_t size); + extern bool qcom_scm_pas_supported(u32 peripheral); extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size); extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
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(+)