[PATCH 0/4] Refactor to support multiple download mode

[PATCH 0/4] Refactor to support multiple download mode

[Mukesh Ojha]

While 1/4 and 2/4 are the fixes and 3/4 and 4/4 are trying to
support multiple download mode like full/mini/both dump.

4/4 will help to enable minidump mode for Qualcomm SoC.

Minidump kernel driver patches has been sent here
https://lore.kernel.org/lkml/1676978713-7394-1-git-send-email-quic_mojha@quicinc.com/

Mukesh Ojha (4):
  firmware: qcom_scm: Clear download bit during reboot
  firmware: scm: Modify only the DLOAD bit in TCSR register for download
    mode
  firmware: qcom_scm: Refactor code to support multiple download mode
  firmware: qcom_scm: Add multiple download mode support

 drivers/firmware/Kconfig    | 11 ---------
 drivers/firmware/qcom_scm.c | 60 +++++++++++++++++++++++++++++++++++++++------
 include/linux/qcom_scm.h    |  6 +++++
 3 files changed, 58 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 1/4] firmware: qcom_scm: Clear download bit during reboot

[Mukesh Ojha]

During normal restart of a system download bit should
be cleared irrespective of whether download mode is
set or not.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54..51eb853 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1418,8 +1418,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 static void qcom_scm_shutdown(struct platform_device *pdev)
 {
 	/* Clean shutdown, disable download mode to allow normal restart */
-	if (download_mode)
-		qcom_scm_set_download_mode(false);
+	qcom_scm_set_download_mode(false);
 }
 
 static const struct of_device_id qcom_scm_dt_match[] = {
-- 
2.7.4

[PATCH 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

[Mukesh Ojha]

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 :
+				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

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

[Brian Masney]

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

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

[Bjorn Andersson]

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
> 

[PATCH 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[Mukesh Ojha]

Currently on Qualcomm SoC, download_mode is enabled if
CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected. Refactor
the code such that it supports multiple download modes and
drop the config.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/Kconfig    | 11 -----------
 drivers/firmware/qcom_scm.c | 45 +++++++++++++++++++++++++++++++++++++++------
 include/linux/qcom_scm.h    |  4 ++++
 3 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e304..ff7e9f3 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -215,17 +215,6 @@ config MTK_ADSP_IPC
 config QCOM_SCM
 	tristate
 
-config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
-	bool "Qualcomm download mode enabled by default"
-	depends on QCOM_SCM
-	help
-	  A device with "download mode" enabled will upon an unexpected
-	  warm-restart enter a special debug mode that allows the user to
-	  "download" memory content over USB for offline postmortem analysis.
-	  The feature can be enabled/disabled on the kernel command line.
-
-	  Say Y here to enable "download mode" by default.
-
 config SYSFB
 	bool
 	select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index c376ba8..4975d3c 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -20,8 +20,43 @@
 
 #include "qcom_scm.h"
 
-static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
-module_param(download_mode, bool, 0);
+static unsigned int download_mode;
+static struct qcom_scm *__scm;
+static void qcom_scm_set_download_mode(bool enable);
+static int set_dload_mode(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+	int old_mode = download_mode;
+
+	ret = param_set_int(val, kp);
+	if (ret)
+		return ret;
+
+	switch (download_mode) {
+	case QCOM_DOWNLOAD_FULLDUMP:
+		if (__scm)
+			qcom_scm_set_download_mode(true);
+		break;
+	case QCOM_DOWNLOAD_NODUMP:
+		if (__scm)
+			qcom_scm_set_download_mode(false);
+		break;
+	default:
+		pr_err("unknown download mode\n");
+		download_mode = old_mode;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct kernel_param_ops dload_mode_param_ops = {
+	.set = set_dload_mode,
+	.get = param_get_hexint,
+};
+module_param_cb(download_mode, &dload_mode_param_ops, &download_mode, 0644);
+MODULE_PARM_DESC(download_mode,
+		 "Download mode: 0x0=no dump mode (default), 0x10=full dump mode");
 
 #define SCM_HAS_CORE_CLK	BIT(0)
 #define SCM_HAS_IFACE_CLK	BIT(1)
@@ -70,8 +105,6 @@ static const char * const qcom_scm_convention_names[] = {
 	[SMC_CONVENTION_LEGACY] = "smc legacy",
 };
 
-static struct qcom_scm *__scm;
-
 static int qcom_scm_clk_enable(void)
 {
 	int ret;
@@ -435,8 +468,8 @@ static void qcom_scm_set_download_mode(bool enable)
 		}
 
 		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));
+				((dload_addr_val & ~QCOM_DOWNLOAD_MODE_MASK) | download_mode) :
+				dload_addr_val & ~QCOM_DOWNLOAD_MODE_MASK);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index f833564..dd6aced 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -14,6 +14,10 @@
 #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
 #define QCOM_SCM_HDCP_MAX_REQ_CNT	5
 
+#define QCOM_DOWNLOAD_MODE_MASK 0x30
+#define QCOM_DOWNLOAD_NODUMP	0x0
+#define QCOM_DOWNLOAD_FULLDUMP	0x10
+
 struct qcom_scm_hdcp_req {
 	u32 addr;
 	u32 val;
-- 
2.7.4

Re: [PATCH 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[Brian Masney]

On Tue, Feb 21, 2023 at 08:09:40PM +0530, Mukesh Ojha wrote:
> Currently on Qualcomm SoC, download_mode is enabled if
> CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected. Refactor
> the code such that it supports multiple download modes and
> drop the config.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/Kconfig    | 11 -----------
>  drivers/firmware/qcom_scm.c | 45 +++++++++++++++++++++++++++++++++++++++------
>  include/linux/qcom_scm.h    |  4 ++++
>  3 files changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b59e304..ff7e9f3 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -215,17 +215,6 @@ config MTK_ADSP_IPC
>  config QCOM_SCM
>  	tristate
>  
> -config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> -	bool "Qualcomm download mode enabled by default"
> -	depends on QCOM_SCM
> -	help
> -	  A device with "download mode" enabled will upon an unexpected
> -	  warm-restart enter a special debug mode that allows the user to
> -	  "download" memory content over USB for offline postmortem analysis.
> -	  The feature can be enabled/disabled on the kernel command line.
> -
> -	  Say Y here to enable "download mode" by default.
> -
>  config SYSFB
>  	bool
>  	select BOOT_VESA_SUPPORT
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index c376ba8..4975d3c 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -20,8 +20,43 @@
>  
>  #include "qcom_scm.h"
>  
> -static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
> -module_param(download_mode, bool, 0);
> +static unsigned int download_mode;
> +static struct qcom_scm *__scm;

Add newline. Also, moving __scm wasn't called out in the commit
description.

> +static void qcom_scm_set_download_mode(bool enable);
> +static int set_dload_mode(const char *val, const struct kernel_param *kp)

Can set_dload_mode() be placed after qcom_scm_set_download_mode() so
that you don't need to declare it before hand.

> +{
> +	int ret;
> +	int old_mode = download_mode;
> +
> +	ret = param_set_int(val, kp);
> +	if (ret)
> +		return ret;
> +
> +	switch (download_mode) {
> +	case QCOM_DOWNLOAD_FULLDUMP:
> +		if (__scm)
> +			qcom_scm_set_download_mode(true);
> +		break;
> +	case QCOM_DOWNLOAD_NODUMP:
> +		if (__scm)
> +			qcom_scm_set_download_mode(false);
> +		break;
> +	default:
> +		pr_err("unknown download mode\n");
> +		download_mode = old_mode;
> +		return -EINVAL;

param_set_int() has been called already and has the invalid value. I
think the param_set_int() should be called after the download mode has
been set successfully.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct kernel_param_ops dload_mode_param_ops = {
> +	.set = set_dload_mode,
> +	.get = param_get_hexint,
> +};
> +module_param_cb(download_mode, &dload_mode_param_ops, &download_mode, 0644);
> +MODULE_PARM_DESC(download_mode,
> +		 "Download mode: 0x0=no dump mode (default), 0x10=full dump mode");
>  
>  #define SCM_HAS_CORE_CLK	BIT(0)
>  #define SCM_HAS_IFACE_CLK	BIT(1)
> @@ -70,8 +105,6 @@ static const char * const qcom_scm_convention_names[] = {
>  	[SMC_CONVENTION_LEGACY] = "smc legacy",
>  };
>  
> -static struct qcom_scm *__scm;
> -
>  static int qcom_scm_clk_enable(void)
>  {
>  	int ret;
> @@ -435,8 +468,8 @@ static void qcom_scm_set_download_mode(bool enable)
>  		}
>  
>  		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));
> +				((dload_addr_val & ~QCOM_DOWNLOAD_MODE_MASK) | download_mode) :
> +				dload_addr_val & ~QCOM_DOWNLOAD_MODE_MASK);

These two lines were introduced in an earlier patch in this series. Why
not just introduce the QCOM_DOWNLOAD_MODE_MASK there?

Brian

Re: [PATCH 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[Bjorn Andersson]

On Tue, Feb 21, 2023 at 08:09:40PM +0530, Mukesh Ojha wrote:
> Currently on Qualcomm SoC, download_mode is enabled if
> CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected. Refactor
> the code such that it supports multiple download modes and
> drop the config.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/Kconfig    | 11 -----------
>  drivers/firmware/qcom_scm.c | 45 +++++++++++++++++++++++++++++++++++++++------
>  include/linux/qcom_scm.h    |  4 ++++
>  3 files changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b59e304..ff7e9f3 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -215,17 +215,6 @@ config MTK_ADSP_IPC
>  config QCOM_SCM
>  	tristate
>  
> -config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> -	bool "Qualcomm download mode enabled by default"
> -	depends on QCOM_SCM
> -	help
> -	  A device with "download mode" enabled will upon an unexpected
> -	  warm-restart enter a special debug mode that allows the user to
> -	  "download" memory content over USB for offline postmortem analysis.
> -	  The feature can be enabled/disabled on the kernel command line.
> -
> -	  Say Y here to enable "download mode" by default.
> -
>  config SYSFB
>  	bool
>  	select BOOT_VESA_SUPPORT
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index c376ba8..4975d3c 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -20,8 +20,43 @@
>  
>  #include "qcom_scm.h"
>  
> -static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
> -module_param(download_mode, bool, 0);
> +static unsigned int download_mode;
> +static struct qcom_scm *__scm;
> +static void qcom_scm_set_download_mode(bool enable);
> +static int set_dload_mode(const char *val, const struct kernel_param *kp)
> +{
> +	int ret;
> +	int old_mode = download_mode;
> +
> +	ret = param_set_int(val, kp);
> +	if (ret)
> +		return ret;
> +
> +	switch (download_mode) {
> +	case QCOM_DOWNLOAD_FULLDUMP:
> +		if (__scm)

If you, as I ask below, pass download_mode to
qcom_scm_set_download_mode(), you could replace this switch statement
with just:

if (__scm)
	qcom_scm_set_download_mode(download_mode);

> +			qcom_scm_set_download_mode(true);
> +		break;
> +	case QCOM_DOWNLOAD_NODUMP:
> +		if (__scm)
> +			qcom_scm_set_download_mode(false);
> +		break;
> +	default:

Please sanity check val before param_set_int() above instead.

> +		pr_err("unknown download mode\n");
> +		download_mode = old_mode;
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct kernel_param_ops dload_mode_param_ops = {
> +	.set = set_dload_mode,
> +	.get = param_get_hexint,
> +};
> +module_param_cb(download_mode, &dload_mode_param_ops, &download_mode, 0644);
> +MODULE_PARM_DESC(download_mode,
> +		 "Download mode: 0x0=no dump mode (default), 0x10=full dump mode");

The module parameters are an interface for humans, so rather than
forcing the operator to play with magical numbers; how about accepting
the values "off", "full" and "minidump"? (And accepting 0 and 1 for any
existing users).

>  
>  #define SCM_HAS_CORE_CLK	BIT(0)
>  #define SCM_HAS_IFACE_CLK	BIT(1)
> @@ -70,8 +105,6 @@ static const char * const qcom_scm_convention_names[] = {
>  	[SMC_CONVENTION_LEGACY] = "smc legacy",
>  };
>  
> -static struct qcom_scm *__scm;
> -
>  static int qcom_scm_clk_enable(void)
>  {
>  	int ret;
> @@ -435,8 +468,8 @@ static void qcom_scm_set_download_mode(bool enable)
>  		}
>  
>  		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));
> +				((dload_addr_val & ~QCOM_DOWNLOAD_MODE_MASK) | download_mode) :

You're passing 2 pieces of information from set_dload_mode(), one by
argument and one through a global variable - and currently they carry
the same "information", in the next patch enable will be !!download_mode.

Please pass download_mode (u32 mode) as an argument to the function -
even if it's accessible in the global scope.

> +				dload_addr_val & ~QCOM_DOWNLOAD_MODE_MASK);
>  	} else {
>  		dev_err(__scm->dev,
>  			"No available mechanism for setting download mode\n");
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index f833564..dd6aced 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -14,6 +14,10 @@
>  #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
>  #define QCOM_SCM_HDCP_MAX_REQ_CNT	5
>  
> +#define QCOM_DOWNLOAD_MODE_MASK 0x30
> +#define QCOM_DOWNLOAD_NODUMP	0x0
> +#define QCOM_DOWNLOAD_FULLDUMP	0x10

I don't see who the intended user of these constants are, please move
them into qcom_scm.c.

Thanks,
Bjorn

> +
>  struct qcom_scm_hdcp_req {
>  	u32 addr;
>  	u32 val;
> -- 
> 2.7.4
> 

[PATCH 4/4] firmware: qcom_scm: Add multiple download mode support

[Mukesh Ojha]

Currently, scm driver only supports full dump when download
mode is selected. Add support to enable minidump as well both
dump(full dump + minidump).

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 5 ++++-
 include/linux/qcom_scm.h    | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 4975d3c..b7a2715 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -34,6 +34,8 @@ static int set_dload_mode(const char *val, const struct kernel_param *kp)
 
 	switch(download_mode) {
 	case QCOM_DOWNLOAD_FULLDUMP:
+	case QCOM_DOWNLOAD_MINIDUMP:
+	case QCOM_DOWNLOAD_BOTHDUMP:
 		if (__scm)
 			qcom_scm_set_download_mode(true);
 		break;
@@ -56,7 +58,8 @@ static const struct kernel_param_ops dload_mode_param_ops = {
 };
 module_param_cb(download_mode, &dload_mode_param_ops, &download_mode, 0644);
 MODULE_PARM_DESC(download_mode,
-		 "Download mode: 0x0=no dump mode (default), 0x10=full dump mode");
+		 "Download mode: 0x0=no dump mode (default), 0x10=full dump mode, "
+		 "0x20=mini dump mode 0x30=both(full + mini)dump mode");
 
 #define SCM_HAS_CORE_CLK	BIT(0)
 #define SCM_HAS_IFACE_CLK	BIT(1)
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index dd6aced..8304c73c 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -17,6 +17,8 @@
 #define QCOM_DOWNLOAD_MODE_MASK 0x30
 #define QCOM_DOWNLOAD_NODUMP	0x0
 #define QCOM_DOWNLOAD_FULLDUMP	0x10
+#define QCOM_DOWNLOAD_MINIDUMP	0x20
+#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
 
 struct qcom_scm_hdcp_req {
 	u32 addr;
-- 
2.7.4