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@xxxxxxxxxxx>
Signed-off-by: Anusha Rao <quic_anusha@xxxxxxxxxxx>
Co-developed-by: Kathiravan Thirumoorthy <quic_kathirav@xxxxxxxxxxx>
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@xxxxxxxxxxx>
Signed-off-by: Poovendhan Selvaraj <quic_poovendh@xxxxxxxxxxx>
---
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@xxxxxxxxxxx/
Do you have any example of any upstream target where this would fail ?
-Mukesh
Regards,
Sricharan
