| Message ID | 1573593774-12539-11-git-send-email-eberman@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restructure, improve target support for qcom_scm driver | expand |
Quoting Elliot Berman (2019-11-12 13:22:46) > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c > index 977654bb..b82b450 100644 > --- a/drivers/firmware/qcom_scm-64.c > +++ b/drivers/firmware/qcom_scm-64.c > @@ -302,21 +302,20 @@ int __qcom_scm_hdcp_req(struct device *dev, struct qcom_scm_hdcp_req *req, > > void __qcom_scm_init(void) > { > - u64 cmd; > - struct arm_smccc_res res; > - u32 function = SMCCC_FUNCNUM(QCOM_SCM_SVC_INFO, QCOM_SCM_INFO_IS_CALL_AVAIL); > - > - /* First try a SMC64 call */ > - cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64, > - ARM_SMCCC_OWNER_SIP, function); > - > - arm_smccc_smc(cmd, QCOM_SCM_ARGS(1), cmd & (~BIT(ARM_SMCCC_TYPE_SHIFT)), > - 0, 0, 0, 0, 0, &res); > - > - if (!res.a0 && res.a1) > - qcom_smccc_convention = ARM_SMCCC_SMC_64; > - else > - qcom_smccc_convention = ARM_SMCCC_SMC_32; > + qcom_smccc_convention = ARM_SMCCC_SMC_64; > + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, > + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) Is this asking if the "is call available function" is available by using the is call available function? That is recursive. Isn't that why we make a manually open coded SMC call to see if it works? If this isn't going to work we may want to just have a property in DT that tells us what to do. > + goto out; > + > + qcom_smccc_convention = ARM_SMCCC_SMC_32; > + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, > + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) > + goto out; > + > + qcom_smccc_convention = -1; > + BUG(); This BUG() is new and not mentioned in the commit text. Why can't we just start failing all scm calls if we can't detect the calling convention? > +out: > + pr_debug("QCOM SCM SMC Convention: %llu\n", qcom_smccc_convention); Maybe pr_info() is more appropriate. PSCI currently prints out the version info so maybe printing something like "QCOM SCM SMC_64 calling convention" will be useful for early debugging.
On 2019-11-15 16:21, Stephen Boyd wrote: > Quoting Elliot Berman (2019-11-12 13:22:46) >> diff --git a/drivers/firmware/qcom_scm-64.c >> b/drivers/firmware/qcom_scm-64.c >> index 977654bb..b82b450 100644 >> --- a/drivers/firmware/qcom_scm-64.c >> +++ b/drivers/firmware/qcom_scm-64.c >> @@ -302,21 +302,20 @@ int __qcom_scm_hdcp_req(struct device *dev, >> struct qcom_scm_hdcp_req *req, >> >> void __qcom_scm_init(void) >> { >> - u64 cmd; >> - struct arm_smccc_res res; >> - u32 function = SMCCC_FUNCNUM(QCOM_SCM_SVC_INFO, >> QCOM_SCM_INFO_IS_CALL_AVAIL); >> - >> - /* First try a SMC64 call */ >> - cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, >> ARM_SMCCC_SMC_64, >> - ARM_SMCCC_OWNER_SIP, function); >> - >> - arm_smccc_smc(cmd, QCOM_SCM_ARGS(1), cmd & >> (~BIT(ARM_SMCCC_TYPE_SHIFT)), >> - 0, 0, 0, 0, 0, &res); >> - >> - if (!res.a0 && res.a1) >> - qcom_smccc_convention = ARM_SMCCC_SMC_64; >> - else >> - qcom_smccc_convention = ARM_SMCCC_SMC_32; >> + qcom_smccc_convention = ARM_SMCCC_SMC_64; >> + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, >> + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) > > Is this asking if the "is call available function" is available by > using > the is call available function? That is recursive. Isn't that why we > make a manually open coded SMC call to see if it works? If this isn't > going to work we may want to just have a property in DT that tells us > what to do. Yes. The reason the open coded SMC call was made was because a fast call works better here. __qcom_scm_is_call_available uses standard call, and I'll address this in v3. >> + BUG(); > > This BUG() is new and not mentioned in the commit text. Why can't we > just start failing all scm calls if we can't detect the calling > convention? Bjorn has requested that the BUG was introduced in v1: https://lore.kernel.org/patchwork/patch/1148619/#1350062 >> +out: >> + pr_debug("QCOM SCM SMC Convention: %llu\n", >> qcom_smccc_convention); > > Maybe pr_info() is more appropriate. PSCI currently prints out the > version info so maybe printing something like "QCOM SCM SMC_64 calling > convention" will be useful for early debugging. Sure, will do. Thanks, Elliot
Quoting eberman@codeaurora.org (2019-11-15 17:29:03) > On 2019-11-15 16:21, Stephen Boyd wrote: > > Quoting Elliot Berman (2019-11-12 13:22:46) > >> diff --git a/drivers/firmware/qcom_scm-64.c > >> b/drivers/firmware/qcom_scm-64.c > >> index 977654bb..b82b450 100644 > >> --- a/drivers/firmware/qcom_scm-64.c > >> +++ b/drivers/firmware/qcom_scm-64.c > >> @@ -302,21 +302,20 @@ int __qcom_scm_hdcp_req(struct device *dev, > >> struct qcom_scm_hdcp_req *req, > >> > >> void __qcom_scm_init(void) > >> { > >> - u64 cmd; > >> - struct arm_smccc_res res; > >> - u32 function = SMCCC_FUNCNUM(QCOM_SCM_SVC_INFO, > >> QCOM_SCM_INFO_IS_CALL_AVAIL); > >> - > >> - /* First try a SMC64 call */ > >> - cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, > >> ARM_SMCCC_SMC_64, > >> - ARM_SMCCC_OWNER_SIP, function); > >> - > >> - arm_smccc_smc(cmd, QCOM_SCM_ARGS(1), cmd & > >> (~BIT(ARM_SMCCC_TYPE_SHIFT)), > >> - 0, 0, 0, 0, 0, &res); > >> - > >> - if (!res.a0 && res.a1) > >> - qcom_smccc_convention = ARM_SMCCC_SMC_64; > >> - else > >> - qcom_smccc_convention = ARM_SMCCC_SMC_32; > >> + qcom_smccc_convention = ARM_SMCCC_SMC_64; > >> + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, > >> + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) > > > > Is this asking if the "is call available function" is available by > > using > > the is call available function? That is recursive. Isn't that why we > > make a manually open coded SMC call to see if it works? If this isn't > > going to work we may want to just have a property in DT that tells us > > what to do. > > Yes. The reason the open coded SMC call was made was because a fast call > works better here. __qcom_scm_is_call_available uses standard call, and > I'll address this in v3. So there will be a patch before this that makes __qcom_scm_is_call_available use SMCCC? I still don't get how it won't be recursive but I'll have to wait until v3 I guess. > > >> + BUG(); > > > > This BUG() is new and not mentioned in the commit text. Why can't we > > just start failing all scm calls if we can't detect the calling > > convention? > > Bjorn has requested that the BUG was introduced in v1: > https://lore.kernel.org/patchwork/patch/1148619/#1350062 > Ok. I'd prefer a WARN_ON() instead but it's not really up to me. At least mention this in the commit text.
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c index 977654bb..b82b450 100644 --- a/drivers/firmware/qcom_scm-64.c +++ b/drivers/firmware/qcom_scm-64.c @@ -302,21 +302,20 @@ int __qcom_scm_hdcp_req(struct device *dev, struct qcom_scm_hdcp_req *req, void __qcom_scm_init(void) { - u64 cmd; - struct arm_smccc_res res; - u32 function = SMCCC_FUNCNUM(QCOM_SCM_SVC_INFO, QCOM_SCM_INFO_IS_CALL_AVAIL); - - /* First try a SMC64 call */ - cmd = ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64, - ARM_SMCCC_OWNER_SIP, function); - - arm_smccc_smc(cmd, QCOM_SCM_ARGS(1), cmd & (~BIT(ARM_SMCCC_TYPE_SHIFT)), - 0, 0, 0, 0, 0, &res); - - if (!res.a0 && res.a1) - qcom_smccc_convention = ARM_SMCCC_SMC_64; - else - qcom_smccc_convention = ARM_SMCCC_SMC_32; + qcom_smccc_convention = ARM_SMCCC_SMC_64; + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) + goto out; + + qcom_smccc_convention = ARM_SMCCC_SMC_32; + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) + goto out; + + qcom_smccc_convention = -1; + BUG(); +out: + pr_debug("QCOM SCM SMC Convention: %llu\n", qcom_smccc_convention); } bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral)
Improve the calling convention detection to use __qcom_scm_is_call_available() and not blindly assume 32-bit mode if the checks fails. Signed-off-by: Elliot Berman <eberman@codeaurora.org> --- drivers/firmware/qcom_scm-64.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)