Messages in this thread | | | Date | Thu, 16 Mar 2023 20:34:25 +0530 | Subject | Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode | From | Mukesh Ojha <> |
| |
On 3/8/2023 8:36 PM, Srinivas Kandagatla wrote: > > > On 01/03/2023 09:55, Mukesh Ojha 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. >> >> Originally-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> --- >> Changes in v2: >> - Addressed comment made by Bjorn. >> - Added download mask from patch 3 to this. >> >> drivers/firmware/qcom_scm.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 51eb853..c9f1fad 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -27,6 +27,8 @@ module_param(download_mode, bool, 0); >> #define SCM_HAS_IFACE_CLK BIT(1) >> #define SCM_HAS_BUS_CLK BIT(2) >> +#define QCOM_DOWNLOAD_MODE_MASK 0x30 >> + >> struct qcom_scm { >> struct device *dev; >> struct clk *core_clk; >> @@ -419,6 +421,7 @@ static void qcom_scm_set_download_mode(bool enable) >> { >> bool avail; >> int ret = 0; >> + u32 val; >> avail = __qcom_scm_is_call_available(__scm->dev, >> QCOM_SCM_SVC_BOOT, >> @@ -426,8 +429,18 @@ 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, &val); >> + if (ret) { >> + dev_err(__scm->dev, >> + "failed to read dload mode address value: %d\n", ret); >> + return; >> + } >> + >> + val &= ~QCOM_DOWNLOAD_MODE_MASK; >> + if (enable) >> + val |= QCOM_SCM_BOOT_SET_DLOAD_MODE; >> + >> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, val); > > > This is the second instance of such pattern of usage one is already in > ./drivers/pinctrl/qcom/pinctrl-msm.c > > I think it makes sense to move setting fields in register to a dedicated > function like this: > > int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, > unsigned int val) > { > unsigned int old, new; > int ret; > > ret = qcom_scm_io_readl(addr, &old); > if (ret) > return ret; > > new = (old & ~mask) | (val << ffs(mask) - 1); > > return qcom_scm_io_writel(addr, new); > } > EXPORT_SYMBOL(qcom_scm_io_update_field); > > > then we could use it like this: > ret = qcom_scm_io_update_field(__scm->dload_mode_addr, > QCOM_DOWNLOAD_MODE_MASK, dl_mode)
Thanks for the review,
will do it in v2, will let /drivers/pinctrl/qcom/pinctrl-msm.c uses this.
-Mukesh > > > --srini >> } else { >> dev_err(__scm->dev, >> "No available mechanism for setting download mode\n");
| |