Re: [PATCH V4 5/5] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

On 2/15/2023 4:25 PM, POOVENDHAN SELVARAJ wrote:

On 2/14/2023 6:27 PM, Srinivas Kandagatla wrote:

quoted

On 14/02/2023 05:14, Poovendhan Selvaraj wrote:

quoted

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 <redacted>
Signed-off-by: Anusha Rao <redacted>
Co-developed-by: Kathiravan Thirumoorthy <redacted>
Signed-off-by: Kathiravan Thirumoorthy <redacted>
Signed-off-by: Poovendhan Selvaraj <redacted>
---
  Changes in V4:
    - retain the orginal value of tcsr register when download mode
      is not set

  drivers/firmware/qcom_scm.c | 15 ++++++++++-----
  1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 468d4d5ab550..8a34b386ac3a 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);

why not read the value here before setting the DLOAD Mode instead of 
doing it in qcom_scm_set_download_mode()?
that would make the code simple and readable.

dload_addr_val is used in both if and else if cases in 
qcom_scm_set_download_mode(),
so we read in qcom_scm_set_download_mode() function and pass to 
__qcom_scm_set_dload_mode().

quoted

quoted

        return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
  }

@@ -426,15 +427,19 @@ 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);
+

not checking ret value here before proceeding?

Okay, sure..will address in next patch series.

quoted

quoted

      if (avail) {
-        ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
+        ret = __qcom_scm_set_dload_mode(__scm->dev, dload_addr_val, 
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_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");

Please excuse the format in Previous mail response.
Had some issues in email client and now it's fixed.

Regards,
Poovendhan S




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel