Messages in this thread | | | Date | Wed, 22 Feb 2023 12:52:56 +0530 | Subject | Re: [PATCH V5 5/5] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode | From | Mukesh Ojha <> |
| |
On 2/22/2023 12:22 PM, Sricharan Ramabadhran wrote: > Hi, > > On 2/20/2023 4:00 PM, POOVENDHAN SELVARAJ wrote: >> >> On 2/18/2023 1:19 AM, Mukesh Ojha wrote: >>> >>> >>> On 2/16/2023 7:30 PM, Mukesh Ojha wrote: >>>> >>>> >>>> On 2/16/2023 5:30 PM, Poovendhan Selvaraj wrote: >>>>> CrashDump collection is based on the DLOAD bit of TCSR register. >>>>> To retain other bits, we read the register and modify only the >>>>> DLOAD bit as >>>>> the other bits have their own significance. >>>>> >>>>> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com> >>>>> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com> >>>>> Co-developed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> >>>>> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> >>>>> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com> >>>>> --- >>>>> Changes in V5: >>>>> - checking the return value in qcom_scm_set_download_mode >>>>> function as >>>>> suggested by Srinivas Kandagatla >>>>> >>>>> Changes in V4: >>>>> - retain the orginal value of tcsr register when download mode >>>>> is not set >>>>> >>>>> drivers/firmware/qcom_scm.c | 21 ++++++++++++++++----- >>>>> 1 file changed, 16 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>>>> index 468d4d5ab550..d88c5f14bd54 100644 >>>>> --- a/drivers/firmware/qcom_scm.c >>>>> +++ b/drivers/firmware/qcom_scm.c >>>>> @@ -407,7 +407,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id) >>>>> } >>>>> EXPORT_SYMBOL(qcom_scm_set_remote_state); >>>>> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) >>>>> +static int __qcom_scm_set_dload_mode(struct device *dev, u32 val, >>>>> bool enable) >>>>> { >>>>> struct qcom_scm_desc desc = { >>>>> .svc = QCOM_SCM_SVC_BOOT, >>>>> @@ -417,7 +417,8 @@ static int __qcom_scm_set_dload_mode(struct >>>>> device *dev, bool enable) >>>>> .owner = ARM_SMCCC_OWNER_SIP, >>>>> }; >>>>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0; >>>>> + desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE : >>>>> + val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE); >>>>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL); >>>>> } >>>>> @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool >>>>> enable) >>>>> { >>>>> bool avail; >>>>> int ret = 0; >>>>> + u32 dload_addr_val; >>>>> avail = __qcom_scm_is_call_available(__scm->dev, >>>>> QCOM_SCM_SVC_BOOT, >>>>> QCOM_SCM_BOOT_SET_DLOAD_MODE); >>>>> + ret = qcom_scm_io_readl(__scm->dload_mode_addr, &dload_addr_val); >>>>> + >>>>> + if (ret) { >>>>> + dev_err(__scm->dev, >>>>> + "failed to read dload mode address value: %d\n", ret); >>>>> + return; >>>>> + } >>>>> + >>>>> if (avail) { >>>>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable); >>>>> + ret = __qcom_scm_set_dload_mode(__scm->dev, >>>>> dload_addr_val, enable); >>>> >>>> Did you test this on a target where it comes under this if >>>> statement? does it really need to know dload_mode_addr for this >>>> target ? >>> >>> >>> Can we do something like this? I would let other review as well. >>> >>> --------------------------------------->0------------------------------------------- >>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>> index cdbfe54..26b7eda 100644 >>> --- a/drivers/firmware/qcom_scm.c >>> +++ b/drivers/firmware/qcom_scm.c >>> @@ -419,6 +419,7 @@ static void qcom_scm_set_download_mode(bool enable) >>> { >>> bool avail; >>> int ret = 0; >>> + u32 dload_addr_val; >>> >>> avail = __qcom_scm_is_call_available(__scm->dev, >>> QCOM_SCM_SVC_BOOT, >>> @@ -426,8 +427,16 @@ static void qcom_scm_set_download_mode(bool enable) >>> if (avail) { >>> ret = __qcom_scm_set_dload_mode(__scm->dev, enable); >>> } else if (__scm->dload_mode_addr) { >>> - ret = qcom_scm_io_writel(__scm->dload_mode_addr, >>> - enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE >>> : 0); >>> + ret = qcom_scm_io_readl(__scm->dload_mode_addr, >>> &dload_addr_val); >>> + if (ret) { >>> + dev_err(__scm->dev, >>> + "failed to read dload mode address >>> value: %d\n", ret); >>> + return; >>> + } >>> + >>> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, >>> enable ? >>> + dload_addr_val | >>> QCOM_SCM_BOOT_SET_DLOAD_MODE : >>> + dload_addr_val & >>> ~(QCOM_SCM_BOOT_SET_DLOAD_MODE)); >>> } else { >>> dev_err(__scm->dev, >>> "No available mechanism for setting download >>> mode\n"); >>> >>> -Mukesh >> >> Okay sure..Agreed, will address this in the next patch. > > Also, not sure, if its better to keep the old behavior working for > targets that does not support 'READ' of this address. If one such > thing exists, that will be broken now. In such a case, we should > ignore if scm_io_readl fails, still write and dload_addr_val should > be '0' initialised.
Why would a secure read of this register would fail, if one is allowed to do secure write ?
Honestly, i was not understanding the purpose of this bitwise handling of this patch, i thought it is trying to fix existing issue for some target.
For some of the upstream target(e.g sm8450, i verified it myself), it is not an issue.
arch/arm64/boot/dts/qcom/msm8916.dtsi: qcom,dload-mode = <&tcsr 0x6100>; arch/arm64/boot/dts/qcom/msm8976.dtsi: qcom,dload-mode = <&tcsr 0x6100>; arch/arm64/boot/dts/qcom/msm8996.dtsi: qcom,dload-mode = <&tcsr_2 0x13000>; arch/arm64/boot/dts/qcom/sm8450.dtsi: qcom,dload-mode = <&tcsr 0x13000>;
However, it looks valid to handle only the effective bits. I have worked on top of this patch and tested it and posted here.
https://lore.kernel.org/lkml/1676990381-18184-1-git-send-email-quic_mojha@quicinc.com/
Do you have any example of any upstream target where this would fail ?
-Mukesh > > > Regards, > Sricharan >
| |