| Message ID | 1676990381-18184-3-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refactor to support multiple download mode | expand |
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@quicinc.com> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > 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; Minor nit if you happen to need to send out a v2: put in reverse Christmas tree order. > > 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)); Should the dload_mode_addr variable be renamed to something else? I'm not sure what else is in that register. Brian
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@quicinc.com> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > 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 >
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 : + dload_addr_val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE)); } else { dev_err(__scm->dev, "No available mechanism for setting download mode\n");
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> --- drivers/firmware/qcom_scm.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)