2023-10-04 17:26:37

by Mukesh Ojha

Subject: [PATCH v7 3/3] firmware: scm: Modify only the download bits in TCSR register

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: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
Tested-by: Kathiravan Thirumoorthy <[email protected]> # IPQ9574 and IPQ5332
---
drivers/firmware/qcom_scm.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 084e4782b88d..da3d028f6451 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -4,6 +4,8 @@
*/

#include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
#include <linux/clk.h>
#include <linux/completion.h>
#include <linux/cpumask.h>
@@ -27,6 +29,10 @@
static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
module_param(download_mode, bool, 0);

+#define QCOM_DLOAD_MASK GENMASK(5, 4)
+#define QCOM_DLOAD_FULLDUMP 0x1
+#define QCOM_DLOAD_NODUMP 0x0
+
struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -518,6 +524,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)

static void qcom_scm_set_download_mode(bool enable)
{
+ u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
bool avail;
int ret = 0;

@@ -527,8 +534,9 @@ 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_update_field(__scm->dload_mode_addr,
+ QCOM_DLOAD_MASK,
+ FIELD_PREP(QCOM_DLOAD_MASK, val));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
--
2.7.4

2023-10-24 19:54:19

by Dmitry Baryshkov

Subject: Re: [PATCH v7 3/3] firmware: scm: Modify only the download bits in TCSR register

On Wed, 4 Oct 2023 at 20:28, Mukesh Ojha <[email protected]> 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.

Nit: please take a look at
`Documentation/process/submitting-patches.rst`: `Describe your changes
in imperative mood'.

We do not read registers. Driver does. Nevertheless, this is a minor
issue, which shouldn't prevent this patch from being applied.

>
> Co-developed-by: Poovendhan Selvaraj <[email protected]>
> Signed-off-by: Poovendhan Selvaraj <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> Tested-by: Kathiravan Thirumoorthy <[email protected]> # IPQ9574 and IPQ5332

Reviewed-by: Dmitry Baryshkov <[email protected]>

> ---
> drivers/firmware/qcom_scm.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 084e4782b88d..da3d028f6451 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -4,6 +4,8 @@
> */
>
> #include <linux/arm-smccc.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> #include <linux/clk.h>
> #include <linux/completion.h>
> #include <linux/cpumask.h>
> @@ -27,6 +29,10 @@
> static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
> module_param(download_mode, bool, 0);
>
> +#define QCOM_DLOAD_MASK GENMASK(5, 4)
> +#define QCOM_DLOAD_FULLDUMP 0x1
> +#define QCOM_DLOAD_NODUMP 0x0

Nit: it might be better to move these defines after all struct definitions.

> +
> struct qcom_scm {
> struct device *dev;
> struct clk *core_clk;
> @@ -518,6 +524,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>
> static void qcom_scm_set_download_mode(bool enable)
> {
> + u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
> bool avail;
> int ret = 0;
>
> @@ -527,8 +534,9 @@ 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_update_field(__scm->dload_mode_addr,
> + QCOM_DLOAD_MASK,
> + FIELD_PREP(QCOM_DLOAD_MASK, val));
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download mode\n");
> --
> 2.7.4
>


--
With best wishes
Dmitry