[PATCH v3 0/5] Refactor to support multiple download mode

[PATCH v3 0/5] Refactor to support multiple download mode

[Mukesh Ojha]

Intention of this series to support multiple download mode and
only modify the required bits during setting tcsr register.

Other download modes are minidump, bothdump (full dump + minidump).

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

Changes in v3:
 - Removed [1] from the series and sent as a separate patch[2], although this series
   should be applied on top [2].
  [1] https://lore.kernel.org/lkml/1677664555-30191-2-git-send-email-quic_mojha@quicinc.com/
  [2] https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/
 - Introduce new exported symbol on suggestion from [srinivas.kandagatla]
 - Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c.
 - Addressed comment given by [dmitry.baryshkov]
 - Converted non-standard Originally-by to Signed-off-by.

Changes in v2: https://lore.kernel.org/lkml/1677664555-30191-1-git-send-email-quic_mojha@quicinc.com/
 - Addressed comment made by [bjorn]
 - Added download mask.
 - Passed download mode as parameter
 - Accept human accepatable download mode string.
 - enable = !!dload_mode
 - Shifted module param callback to somewhere down in
   the file so that it no longer need to know the
   prototype of qcom_scm_set_download_mode()
 - updated commit text.


Mukesh Ojha (5):
  firmware: qcom_scm: provide a read-modify-write function
  pinctrl: qcom: Use qcom_scm_io_update_field()
  firmware: scm: Modify only the download bits in TCSR register
  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            | 88 ++++++++++++++++++++++++++++++----
 drivers/pinctrl/qcom/pinctrl-msm.c     | 15 +++---
 include/linux/firmware/qcom/qcom_scm.h |  2 +
 4 files changed, 89 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH v3 1/5] firmware: qcom_scm: provide a read-modify-write function

[Mukesh Ojha]

It was released by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.

Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.

Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c            | 15 +++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 3e020d1..aca2556 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
 }
 EXPORT_SYMBOL(qcom_scm_set_remote_state);
 
+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;
+
+	return qcom_scm_io_writel(addr, new);
+}
+EXPORT_SYMBOL(qcom_scm_io_update_field);
+
 static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
 	struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 1e449a5..203a781 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
 
 extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
 extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
+				    unsigned int val);
 
 extern bool qcom_scm_restore_sec_cfg_available(void);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
-- 
2.7.4

Re: [PATCH v3 1/5] firmware: qcom_scm: provide a read-modify-write function

[Mukesh Ojha]

On 3/17/2023 9:57 PM, Mukesh Ojha wrote:
> It was released by Srinivas K. that there is a need of

Typo: s/released/realized

-- Mukesh

> read-modify-write scm exported function so that it can
> be used by multiple clients.
> 
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
> 
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/firmware/qcom_scm.c            | 15 +++++++++++++++
>   include/linux/firmware/qcom/qcom_scm.h |  2 ++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 3e020d1..aca2556 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>   }
>   EXPORT_SYMBOL(qcom_scm_set_remote_state);
>   
> +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;
> +
> +	return qcom_scm_io_writel(addr, new);
> +}
> +EXPORT_SYMBOL(qcom_scm_io_update_field);
> +
>   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>   {
>   	struct qcom_scm_desc desc = {
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 1e449a5..203a781 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
>   
>   extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>   extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
> +				    unsigned int val);
>   
>   extern bool qcom_scm_restore_sec_cfg_available(void);
>   extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);

Re: [PATCH v3 1/5] firmware: qcom_scm: provide a read-modify-write function

[Linus Walleij]

On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:

> It was released by Srinivas K. that there is a need of
> read-modify-write scm exported function so that it can
> be used by multiple clients.
>
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
>
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>

This is starting to reimplement regmap.
In this case regmap_update_bits().

What about just using regmap as accessor for these
registers instead?

Yours,
Linus Walleij

Re: [PATCH v3 1/5] firmware: qcom_scm: provide a read-modify-write function

[Bjorn Andersson]

On Fri, Mar 17, 2023 at 09:56:59PM +0100, Linus Walleij wrote:
> On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> 
> > It was released by Srinivas K. that there is a need of
> > read-modify-write scm exported function so that it can
> > be used by multiple clients.
> >
> > Let's introduce qcom_scm_io_update_field() which masks
> > out the bits and write the passed value to that
> > bit-offset. Subsequent patch will use this function.
> >
> > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> This is starting to reimplement regmap.
> In this case regmap_update_bits().
> 
> What about just using regmap as accessor for these
> registers instead?
> 

I'm not sure it would be beneficial...

The regmap interface provides a standardized representation of a block
of registers, with the suitable accessors backing it. But in both cases
touched upon in this series, the addressed registers are part of regions
already handled by the kernel.

So it wouldn't be suitable to create a regmap-abstraction for "a block
of secure registers", at best that would give us two kinds of regmaps
abstracting the same register block.


Instead I believe we'd need to extend the struct regmap_config to
introduce a new table telling a new secure-or-unsecure-mmio-regmap which
accessor (secure or unsecure read/write) shoudl be used, and then have
e.g. pinctrl-msm register such regmap, passing the information about
which registers in its memory region is secure.

We'd still need qcom_scm_io_readl() and qcom_scm_io_writel() exported to
implement the new custom regmap implementation - and the struct
regmap_config needed in just pinctrl-msm alone would be larger than the
one function it replaces.

But please let me know if I'm missing something?

Regards,
Bjorn

Re: [PATCH v3 1/5] firmware: qcom_scm: provide a read-modify-write function

[Linus Walleij]

On Mon, Mar 20, 2023 at 4:19 AM Bjorn Andersson <andersson@kernel.org> wrote:

> > This is starting to reimplement regmap.
> > In this case regmap_update_bits().
> >
> > What about just using regmap as accessor for these
> > registers instead?
> >
>
> I'm not sure it would be beneficial...

Me neither, I don't know the details, I just notice the similarity in the
accessors.

> The regmap interface provides a standardized representation of a block
> of registers, with the suitable accessors backing it. But in both cases
> touched upon in this series, the addressed registers are part of regions
> already handled by the kernel.
>
> So it wouldn't be suitable to create a regmap-abstraction for "a block
> of secure registers", at best that would give us two kinds of regmaps
> abstracting the same register block.

From my viewpoint regmap does three things:
- Abstract one coherent region of registers under a shared lock, with
  nifty accessors (such as mask-and-set with regmap_update_bits())
- Maps access patterns/permissions and permissible access range
- Optionally cache the contents

The way I would use it if these secure registers are in the same range as a
bunch of non-secure ones is for sharing a lock and the regmap accessors.

I wouldn't worry about access patterns and such. That usecase (block
access to certain registers or bits) is partly overdesign in some cases
IMO.

If regmap abstraction isn't helpful overall then we shouldn't do it.

Yours,
Linus Walleij

Re: [PATCH v3 1/5] firmware: qcom_scm: provide a read-modify-write function

[Srinivas Kandagatla]

On 17/03/2023 16:27, Mukesh Ojha wrote:
> It was released by Srinivas K. that there is a need of
> read-modify-write scm exported function so that it can
> be used by multiple clients.
> 
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
> 
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/firmware/qcom_scm.c            | 15 +++++++++++++++
>   include/linux/firmware/qcom/qcom_scm.h |  2 ++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 3e020d1..aca2556 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>   }
>   EXPORT_SYMBOL(qcom_scm_set_remote_state);
>   
> +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;

thanks for doing this,

With field semantics, val should be shifted within the function, so the 
caller only sets value for field rather than passing a shifted value.
so this should be:

new = (old & ~mask) | (val << ffs(mask) - 1);


--srini



> +
> +	return qcom_scm_io_writel(addr, new);
> +}
> +EXPORT_SYMBOL(qcom_scm_io_update_field);
> +
>   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>   {
>   	struct qcom_scm_desc desc = {
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 1e449a5..203a781 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
>   
>   extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>   extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
> +				    unsigned int val);
>   
>   extern bool qcom_scm_restore_sec_cfg_available(void);
>   extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);

[PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update_field()

[Mukesh Ojha]

Use qcom_scm_io_update_field() exported function introduced
in last commit.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index daeb79a..3d3d520 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1016,6 +1016,8 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	bool was_enabled;
 	u32 val;
+	u32 tmp_val;
+	u32 mask;
 
 	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
 		set_bit(d->hwirq, pctrl->dual_edge_irqs);
@@ -1049,24 +1051,21 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	 * With intr_target_use_scm interrupts are routed to
 	 * application cpu using scm calls.
 	 */
+	mask = (7 << g->intr_target_bit);
+	tmp_val = g->intr_target_kpss_val << g->intr_target_bit;
 	if (pctrl->intr_target_use_scm) {
 		u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
 		int ret;
 
-		qcom_scm_io_readl(addr, &val);
-
-		val &= ~(7 << g->intr_target_bit);
-		val |= g->intr_target_kpss_val << g->intr_target_bit;
-
-		ret = qcom_scm_io_writel(addr, val);
+		ret = qcom_scm_io_update_field(addr, mask, tmp_val);
 		if (ret)
 			dev_err(pctrl->dev,
 				"Failed routing %lu interrupt to Apps proc",
 				d->hwirq);
 	} else {
 		val = msm_readl_intr_target(pctrl, g);
-		val &= ~(7 << g->intr_target_bit);
-		val |= g->intr_target_kpss_val << g->intr_target_bit;
+		val &= ~mask;
+		val |= tmp_val;
 		msm_writel_intr_target(val, pctrl, g);
 	}
 
-- 
2.7.4

Re: [PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update_field()

[Linus Walleij]

On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:

> Use qcom_scm_io_update_field() exported function introduced
> in last commit.
>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>

Fine by me, but I want you to first consider switching the
custom register accessors to regmap.

Yours,
Linus Walleij

Re: [PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update_field()

[Bjorn Andersson]

On Fri, Mar 17, 2023 at 09:58:04PM +0100, Linus Walleij wrote:
> On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> 
> > Use qcom_scm_io_update_field() exported function introduced
> > in last commit.
> >
> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> Fine by me, but I want you to first consider switching the
> custom register accessors to regmap.
> 

I took a quick look at it and there seem to be two ways that it can be
done.

We can retain the MSM_ACCESSOR() macros that generates the custom
register accessors, but plug in a regmap between these accessors and the
mmio operations. But this just adds a few extra hops inbetween the
driver and the volatile read/write, with a slight increase of memory,
without any obvious benefits.


The more alluring alternative is to replace the custom accessors with
reg_fields. This would allow us to replace some (perhaps many) of the
bit-manipulation with regmap_update_bits().

But at minimum we'd need one reg_field per register, per pin, so that's
5 reg_fields per pin which adds up to ~10-24kb extra space, depending on
platform.

Even more alluring would be to have reg_fields describing the actual
fields in the registers, which would allow us to better utilize the
regmap API directly. This would cost us 35-75kb of heap.

IMHO this is quite a significant effort, and given that the driver seems
to be doing its job I'd rather see such efforts being focused elsewhere.

Regards,
Bjorn

Re: [PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update_field()

[Linus Walleij]

On Mon, Mar 20, 2023 at 5:07 AM Bjorn Andersson <andersson@kernel.org> wrote:

> > Fine by me, but I want you to first consider switching the
> > custom register accessors to regmap.
(...)
> IMHO this is quite a significant effort, and given that the driver seems
> to be doing its job I'd rather see such efforts being focused elsewhere.

I think you know it better than me, if regmap is just going to clutter
the view the don't do it.

Regmap does have the upside of looking the same on all platforms so it
would potentially give less maintenance burden.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
for these patches if you need to merge them elsewhere, I can also
queue them if you ACK them.

Yours,
Linus Walleij

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

[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.

Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index aca2556..eb491da 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,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;
@@ -448,7 +450,8 @@ 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,
+		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+				QCOM_DOWNLOAD_MODE_MASK,
 				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
 	} else {
 		dev_err(__scm->dev,
-- 
2.7.4

[PATCH v3 4/5] 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 CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
instead, give interface to set the download mode from
module parameter.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/Kconfig    | 11 ---------
 drivers/firmware/qcom_scm.c | 57 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 50 insertions(+), 18 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 eb491da..69f864d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -20,17 +20,19 @@
 #include <linux/clk.h>
 #include <linux/reset-controller.h>
 #include <linux/arm-smccc.h>
+#include <linux/kstrtox.h>
 
 #include "qcom_scm.h"
 
-static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
-module_param(download_mode, bool, 0);
+static u32 download_mode;
 
 #define SCM_HAS_CORE_CLK	BIT(0)
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
 
 #define QCOM_DOWNLOAD_MODE_MASK 0x30
+#define QCOM_DOWNLOAD_FULLDUMP	0x10
+#define QCOM_DOWNLOAD_NODUMP	0x0
 
 struct qcom_scm {
 	struct device *dev;
@@ -439,8 +441,9 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
 }
 
-static void qcom_scm_set_download_mode(bool enable)
+static void qcom_scm_set_download_mode(u32 download_mode)
 {
+	bool enable = !!download_mode;
 	bool avail;
 	int ret = 0;
 
@@ -452,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable)
 	} else if (__scm->dload_mode_addr) {
 		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
 				QCOM_DOWNLOAD_MODE_MASK,
-				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+				enable ? download_mode : 0);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
@@ -1418,6 +1421,46 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+
+static int get_download_mode(char *buffer, const struct kernel_param *kp)
+{
+	int len;
+
+	if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
+		len = sysfs_emit(buffer, "full\n");
+	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
+		len = sysfs_emit(buffer, "off\n");
+
+	return len;
+}
+
+static int set_download_mode(const char *val, const struct kernel_param *kp)
+{
+	if (!strncmp(val, "full", strlen("full"))) {
+		download_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (!strncmp(val, "off", strlen("off"))) {
+		download_mode = QCOM_DOWNLOAD_NODUMP;
+	} else if (kstrtouint(val, 0, &download_mode) ||
+		   !(download_mode == 0 || download_mode == 1)) {
+		pr_err("unknown download mode\n");
+		return -EINVAL;
+	}
+
+	if (__scm)
+		qcom_scm_set_download_mode(download_mode);
+
+	return 0;
+}
+
+static const struct kernel_param_ops download_mode_param_ops = {
+	.get = get_download_mode,
+	.set = set_download_mode,
+};
+
+module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
+MODULE_PARM_DESC(download_mode,
+		 "Download mode: off/full or 0/1 for existing users");
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
@@ -1511,12 +1554,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	__get_convention();
 
 	/*
-	 * If requested enable "download mode", from this point on warmboot
+	 * If "download mode" is requested, from this point on warmboot
 	 * will cause the boot stages to enter download mode, unless
 	 * disabled below by a clean shutdown/reboot.
 	 */
 	if (download_mode)
-		qcom_scm_set_download_mode(true);
+		qcom_scm_set_download_mode(download_mode);
 
 	return 0;
 }
@@ -1524,7 +1567,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 */
-	qcom_scm_set_download_mode(false);
+	qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
 }
 
 static const struct of_device_id qcom_scm_dt_match[] = {
-- 
2.7.4

[PATCH v3 5/5] 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 | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 69f864d..ba3eefc 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -32,6 +32,9 @@ static u32 download_mode;
 
 #define QCOM_DOWNLOAD_MODE_MASK 0x30
 #define QCOM_DOWNLOAD_FULLDUMP	0x10
+#define QCOM_DOWNLOAD_MINIDUMP  0x20
+#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
+
 #define QCOM_DOWNLOAD_NODUMP	0x0
 
 struct qcom_scm {
@@ -1421,15 +1424,19 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-
 static int get_download_mode(char *buffer, const struct kernel_param *kp)
 {
-	int len;
+	int len = 0;
 
-	if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
+	if (download_mode == QCOM_DOWNLOAD_FULLDUMP) {
 		len = sysfs_emit(buffer, "full\n");
-	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
+	} else if (download_mode == QCOM_DOWNLOAD_MINIDUMP) {
+		len = sysfs_emit(buffer, "mini\n");
+	} else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP) {
+		len = sysfs_emit(buffer, "both\n");
+	} else if (download_mode == QCOM_DOWNLOAD_NODUMP) {
 		len = sysfs_emit(buffer, "off\n");
+	}
 
 	return len;
 }
@@ -1438,6 +1445,10 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
 {
 	if (!strncmp(val, "full", strlen("full"))) {
 		download_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (!strncmp(val, "mini", strlen("mini"))) {
+		download_mode = QCOM_DOWNLOAD_MINIDUMP;
+	} else if (!strncmp(val, "both", strlen("both"))) {
+		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
 	} else if (!strncmp(val, "off", strlen("off"))) {
 		download_mode = QCOM_DOWNLOAD_NODUMP;
 	} else if (kstrtouint(val, 0, &download_mode) ||
@@ -1459,7 +1470,7 @@ static const struct kernel_param_ops download_mode_param_ops = {
 
 module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
 MODULE_PARM_DESC(download_mode,
-		 "Download mode: off/full or 0/1 for existing users");
+		 "download mode: off/full/mini/both(full+mini) or 0/1 for existing users");
 
 static int qcom_scm_probe(struct platform_device *pdev)
 {
-- 
2.7.4