| Message ID | 1572917256-24205-10-git-send-email-eberman@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restructure, improve target support for qcom_scm driver | expand |
On Mon 04 Nov 17:27 PST 2019, Elliot Berman wrote: > - Use enum to describe SMC convention. > - Improve SMC convention detection to use __qcom_scm_is_call_available > instead of circumventing qcom_scm_call_smccc. > - Improve SMC convention detection to check that SMCCC-32 works, instead > of just assuming it does of SMCCC-64 does not. I was about to tell you that your list represent individual changes, but I think you should rewrite the commit message instead. Something like: """ 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 | 42 ++++++++++++++++++++++++------------------ > 1 file changed, 24 insertions(+), 18 deletions(-) > > diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c > index f79b0dc..2579246 100644 > --- a/drivers/firmware/qcom_scm-64.c > +++ b/drivers/firmware/qcom_scm-64.c > @@ -58,7 +58,13 @@ struct arm_smccc_args { > unsigned long a[8]; > }; > > -static u64 qcom_smccc_convention = -1; > +enum qcom_smc_convention { > + SMC_CONVENTION_UNKNOWN, > + SMC_CONVENTION_ARM_32, > + SMC_CONVENTION_ARM_64, > +}; > + > +static enum qcom_smc_convention qcom_smc_convention = SMC_CONVENTION_UNKNOWN; > static DEFINE_MUTEX(qcom_scm_lock); > > #define QCOM_SCM_EBUSY_WAIT_MS 30 > @@ -103,7 +109,9 @@ static int ___qcom_scm_call_smccc(struct device *dev, > > smc.a[0] = ARM_SMCCC_CALL_VAL( > atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL, > - qcom_smccc_convention, Use a local variable instead of using a ternary operator in the middle of the arguments. > + (qcom_smc_convention == SMC_CONVENTION_ARM_64) ? > + ARM_SMCCC_SMC_64 : > + ARM_SMCCC_SMC_32, Here SMC_CONVENTION_UNKNOWN would mean ARM_SMCCC_SMC_32... > desc->owner, > SMCCC_FUNCNUM(desc->svc, desc->cmd)); > smc.a[1] = desc->arginfo; > @@ -117,7 +125,7 @@ static int ___qcom_scm_call_smccc(struct device *dev, > if (!args_virt) > return -ENOMEM; > > - if (qcom_smccc_convention == ARM_SMCCC_SMC_32) { > + if (qcom_smc_convention == SMC_CONVENTION_ARM_32) { ...but here it would mean ARM_SMCCC_SMC_64. > __le32 *args = args_virt; > > for (i = 0; i < SMCCC_N_EXT_ARGS; i++) > @@ -583,19 +591,17 @@ int __qcom_scm_qsmmu500_wait_safe_toggle(struct device *dev, bool en) > > 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_smc_convention = SMC_CONVENTION_ARM_64; > + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, > + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) > + goto out; > + > + qcom_smc_convention = SMC_CONVENTION_ARM_32; > + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, > + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) > + goto out; > + > + qcom_smc_convention = SMC_CONVENTION_UNKNOWN; If above two tests can be considered reliable I would suggest that you fail hard here instead. And if so I think you should postpone the introduction of the enum until you actually need it to represent the legacy mode. Regards, Bjorn > +out: > + pr_debug("QCOM SCM SMC Convention: %d\n", qcom_smc_convention); > } > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 2019-11-07 11:18, Bjorn Andersson wrote: >> + (qcom_smc_convention == SMC_CONVENTION_ARM_64) ? >> + ARM_SMCCC_SMC_64 : >> + ARM_SMCCC_SMC_32, > > Here SMC_CONVENTION_UNKNOWN would mean ARM_SMCCC_SMC_32... Idea is that __qcom_scm_call_smccc would only be called if qcom_smc_convention is SMC_CONVENTION_ARM_64 or _32. It should not be possible to get into __qcom_scm_call_smccc with the current convention being SMC_CONVENTION_UNKNOWN. > >> desc->owner, >> SMCCC_FUNCNUM(desc->svc, desc->cmd)); >> smc.a[1] = desc->arginfo; >> @@ -117,7 +125,7 @@ static int ___qcom_scm_call_smccc(struct device >> *dev, >> if (!args_virt) >> return -ENOMEM; >> >> - if (qcom_smccc_convention == ARM_SMCCC_SMC_32) { >> + if (qcom_smc_convention == SMC_CONVENTION_ARM_32) { > > ...but here it would mean ARM_SMCCC_SMC_64. I will clean up to be consistent what the "else" case is. >> @@ -583,19 +591,17 @@ int __qcom_scm_qsmmu500_wait_safe_toggle(struct >> device *dev, bool en) >> >> 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_smc_convention = SMC_CONVENTION_ARM_64; >> + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, >> + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) >> + goto out; >> + >> + qcom_smc_convention = SMC_CONVENTION_ARM_32; >> + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, >> + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) >> + goto out; >> + >> + qcom_smc_convention = SMC_CONVENTION_UNKNOWN; > > If above two tests can be considered reliable I would suggest that you > fail hard here instead. Is the suggestion here to BUG out? Thanks, Elliot -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On Thu 07 Nov 12:20 PST 2019, eberman@codeaurora.org wrote: > On 2019-11-07 11:18, Bjorn Andersson wrote: [..] > > > @@ -583,19 +591,17 @@ int > > > __qcom_scm_qsmmu500_wait_safe_toggle(struct device *dev, bool en) > > > > > > 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_smc_convention = SMC_CONVENTION_ARM_64; > > > + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, > > > + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) > > > + goto out; > > > + > > > + qcom_smc_convention = SMC_CONVENTION_ARM_32; > > > + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, > > > + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) > > > + goto out; > > > + > > > + qcom_smc_convention = SMC_CONVENTION_UNKNOWN; > > > > If above two tests can be considered reliable I would suggest that you > > fail hard here instead. > > Is the suggestion here to BUG out? > We generally do not want that, but leaving it "unknown" feels like the next scm call will have similar outcome to calling BUG() here, but be harder to debug... So I would be willing to accept a BUG() here. Regards, Bjorn
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c index f79b0dc..2579246 100644 --- a/drivers/firmware/qcom_scm-64.c +++ b/drivers/firmware/qcom_scm-64.c @@ -58,7 +58,13 @@ struct arm_smccc_args { unsigned long a[8]; }; -static u64 qcom_smccc_convention = -1; +enum qcom_smc_convention { + SMC_CONVENTION_UNKNOWN, + SMC_CONVENTION_ARM_32, + SMC_CONVENTION_ARM_64, +}; + +static enum qcom_smc_convention qcom_smc_convention = SMC_CONVENTION_UNKNOWN; static DEFINE_MUTEX(qcom_scm_lock); #define QCOM_SCM_EBUSY_WAIT_MS 30 @@ -103,7 +109,9 @@ static int ___qcom_scm_call_smccc(struct device *dev, smc.a[0] = ARM_SMCCC_CALL_VAL( atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL, - qcom_smccc_convention, + (qcom_smc_convention == SMC_CONVENTION_ARM_64) ? + ARM_SMCCC_SMC_64 : + ARM_SMCCC_SMC_32, desc->owner, SMCCC_FUNCNUM(desc->svc, desc->cmd)); smc.a[1] = desc->arginfo; @@ -117,7 +125,7 @@ static int ___qcom_scm_call_smccc(struct device *dev, if (!args_virt) return -ENOMEM; - if (qcom_smccc_convention == ARM_SMCCC_SMC_32) { + if (qcom_smc_convention == SMC_CONVENTION_ARM_32) { __le32 *args = args_virt; for (i = 0; i < SMCCC_N_EXT_ARGS; i++) @@ -583,19 +591,17 @@ int __qcom_scm_qsmmu500_wait_safe_toggle(struct device *dev, bool en) 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_smc_convention = SMC_CONVENTION_ARM_64; + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) + goto out; + + qcom_smc_convention = SMC_CONVENTION_ARM_32; + if (__qcom_scm_is_call_available(NULL, QCOM_SCM_SVC_INFO, + QCOM_SCM_INFO_IS_CALL_AVAIL) == 1) + goto out; + + qcom_smc_convention = SMC_CONVENTION_UNKNOWN; +out: + pr_debug("QCOM SCM SMC Convention: %d\n", qcom_smc_convention); }
- Use enum to describe SMC convention. - Improve SMC convention detection to use __qcom_scm_is_call_available instead of circumventing qcom_scm_call_smccc. - Improve SMC convention detection to check that SMCCC-32 works, instead of just assuming it does of SMCCC-64 does not. Signed-off-by: Elliot Berman <eberman@codeaurora.org> --- drivers/firmware/qcom_scm-64.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-)