On Tue, Feb 21, 2023 at 08:09:39PM +0530, 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@xxxxxxxxxxx>
> Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> ---
> drivers/firmware/qcom_scm.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 51eb853..c376ba8 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 :
I prefer to avoid using the ternary operator inside writel, in
particular since you in the next patch mask out the DLOAD_MASK here as
well...
Please make this:
readl();
dload_addr_val &= ~MASK;
if (enable)
dload_addr_val |= DLOAD_MODE;
writel();
Also, this is the only "value" we're working on in this function, so
"u32 val" should be sufficient.
Thanks,
Bjorn
> + dload_addr_val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download mode\n");
> --
> 2.7.4
>
