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

[PATCH v6 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, full dump, both fulldump + minidump, nodump.

Latest minidump kernel driver patches has been sent here
https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/

Also, this series should be applied on
https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/

Changes in v6:
  - Applied suggested API change(at v4) by [dmitry.baryshkov] 

Changes in v5: https://lore.kernel.org/lkml/1680017869-22421-1-git-send-email-quic_mojha@quicinc.com/
  - Tried to fix the issue reported by kernel test robot
    https://lore.kernel.org/lkml/202303280535.acb66sQT-lkp@intel.com/

  - Applied some of the improvement suggested by [Bjorn.andersson]
 
    . Dropped 'both' instead support full,mini or mini,full for setting download
    mode to collect both minidump and full dump.
    
    . logging improvement.
    

Changes in v4: https://lore.kernel.org/lkml/1679935281-18445-1-git-send-email-quic_mojha@quicinc.com/
  - val should be shifted within the function [srinivas.kandagatla]
    i.e new = (old & ~mask) | (val << ffs(mask) - 1);
  - Added Acked-by [linus.walleij] on pinctrl change.

Changes in v3 : https://lore.kernel.org/lkml/1679070482-8391-1-git-send-email-quic_mojha@quicinc.com/
 - 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     | 11 ++---
 include/linux/firmware/qcom/qcom_scm.h |  2 +
 4 files changed, 86 insertions(+), 26 deletions(-)

-- 
2.7.4

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

[Mukesh Ojha]

It was realized 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 5f281cb..cb0bc32 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 << (ffs(mask) - 1);
+
+	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 v6 1/5] firmware: qcom_scm: provide a read-modify-write function

[andy.shevchenko]

Wed, Mar 29, 2023 at 01:16:48PM +0530, Mukesh Ojha kirjoitti:
> It was realized 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.

...

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

It's a bit non-standard to see left shift here instead of masking.

	new = (old & ~mask) | (val & mask);

is usual pattern.

Note as well that your code is prone to subtle mistakes when overflow may
easily override bits outside the mask.

> +	return qcom_scm_io_writel(addr, new);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Mukesh Ojha]

Use qcom_scm_io_update_field() exported function introduced
in last commit.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index daeb79a..6e1a5e4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1016,6 +1016,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	bool was_enabled;
 	u32 val;
+	u32 mask;
 
 	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
 		set_bit(d->hwirq, pctrl->dual_edge_irqs);
@@ -1049,23 +1050,19 @@ 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);
 	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, g->intr_target_kpss_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 &= ~mask;
 		val |= g->intr_target_kpss_val << g->intr_target_bit;
 		msm_writel_intr_target(val, pctrl, g);
 	}
-- 
2.7.4

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

[andy.shevchenko]

Wed, Mar 29, 2023 at 01:16:49PM +0530, Mukesh Ojha kirjoitti:
> Use qcom_scm_io_update_field() exported function introduced
> in last commit.

...

>  	u32 val;
> +	u32 mask;

Swapping their order may help with readability.

...

> +	mask = (7 << g->intr_target_bit);

Why not GENMASK(2, 0) at the same time?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v6 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cb0bc32..8e39b97 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,9 @@ 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
+#define QCOM_DOWNLOAD_FULLDUMP	0x1
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -448,8 +451,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_DOWNLOAD_MODE_MASK,
+				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.7.4

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

[Bjorn Andersson]

On Wed, Mar 29, 2023 at 01:16:50PM +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.
> 
> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>

With Poovendhan being the first one to sign off the patch, was he the
author? Or should this be Co-developed-by: Poovendhan ?

Regards,
Bjorn

> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/qcom_scm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cb0bc32..8e39b97 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -30,6 +30,9 @@ 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
> +#define QCOM_DOWNLOAD_FULLDUMP	0x1
> +
>  struct qcom_scm {
>  	struct device *dev;
>  	struct clk *core_clk;
> @@ -448,8 +451,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_DOWNLOAD_MODE_MASK,
> +				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
>  	} else {
>  		dev_err(__scm->dev,
>  			"No available mechanism for setting download mode\n");
> -- 
> 2.7.4
> 

[PATCH v6 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 | 60 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 19 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 8e39b97..fa6502a 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -20,11 +20,11 @@
 #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)
@@ -32,6 +32,7 @@ module_param(download_mode, bool, 0);
 
 #define QCOM_DOWNLOAD_MODE_MASK 0x30
 #define QCOM_DOWNLOAD_FULLDUMP	0x1
+#define QCOM_DOWNLOAD_NODUMP	0x0
 
 struct qcom_scm {
 	struct device *dev;
@@ -440,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,8 +454,7 @@ static void qcom_scm_set_download_mode(bool enable)
 		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
 	} else if (__scm->dload_mode_addr) {
 		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
-				QCOM_DOWNLOAD_MODE_MASK,
-				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
+				QCOM_DOWNLOAD_MODE_MASK, download_mode);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
@@ -1419,6 +1420,49 @@ 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 = 0;
+
+	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)
+{
+	u32 old = download_mode;
+
+	if (sysfs_streq(val, "full")) {
+		download_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (sysfs_streq(val, "off")) {
+		download_mode = QCOM_DOWNLOAD_NODUMP;
+	} else if (kstrtouint(val, 0, &download_mode) ||
+		   !(download_mode == 0 || download_mode == 1)) {
+		download_mode = old;
+		pr_err("qcom_scm: unknown download mode: %s\n", val);
+		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;
@@ -1512,12 +1556,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;
 }
@@ -1525,7 +1569,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

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

[andy.shevchenko]

Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:
> 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.

...

>  #include <linux/clk.h>
>  #include <linux/reset-controller.h>
>  #include <linux/arm-smccc.h>

> +#include <linux/kstrtox.h>

Can this be located after clk.h which makes (some) order in this block?

...

>  #define QCOM_DOWNLOAD_MODE_MASK 0x30
>  #define QCOM_DOWNLOAD_FULLDUMP	0x1
> +#define QCOM_DOWNLOAD_NODUMP	0x0

Okay, so you start backward ordering.
But see comments to the next patch.

...

>  		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> -				QCOM_DOWNLOAD_MODE_MASK,
> -				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
> +				QCOM_DOWNLOAD_MODE_MASK, download_mode);

Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
so you won't change lines that were introduced just before.

...

>  }
>  
> +

Stray change.

> +static int get_download_mode(char *buffer, const struct kernel_param *kp)
> +{
> +	int len = 0;
> +
> +	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;

You can return directly.

Also, what about download_mode that doesn't fit to the above two?

> +}

...

> +static int set_download_mode(const char *val, const struct kernel_param *kp)
> +{
> +	u32 old = download_mode;
> +
> +	if (sysfs_streq(val, "full")) {
> +		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> +	} else if (sysfs_streq(val, "off")) {
> +		download_mode = QCOM_DOWNLOAD_NODUMP;

NIH sysfs_match_string().

> +	} else if (kstrtouint(val, 0, &download_mode) ||
> +		   !(download_mode == 0 || download_mode == 1)) {
> +		download_mode = old;
> +		pr_err("qcom_scm: unknown download mode: %s\n", val);

> +		return -EINVAL;

Do not shadow the error code from kstrtouint() it can be different to this one.

> +	}
> +
> +	if (__scm)
> +		qcom_scm_set_download_mode(download_mode);
> +
> +	return 0;
> +}

...

Have you updated corresponding documentation about this parameter?
Or there is none?

-- 
With Best Regards,
Andy Shevchenko

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

[Mukesh Ojha]

On 5/27/2023 3:38 AM, andy.shevchenko@gmail.com wrote:
> Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:
>> 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.
> 
> ...
> 
>>   #include <linux/clk.h>
>>   #include <linux/reset-controller.h>
>>   #include <linux/arm-smccc.h>
> 
>> +#include <linux/kstrtox.h>
> 
> Can this be located after clk.h which makes (some) order in this block?

Sure.

> 
> ...
> 
>>   #define QCOM_DOWNLOAD_MODE_MASK 0x30
>>   #define QCOM_DOWNLOAD_FULLDUMP	0x1
>> +#define QCOM_DOWNLOAD_NODUMP	0x0
> 
> Okay, so you start backward ordering.
> But see comments to the next patch.

Will fix this by doing it in ascending order..


> 
> ...
> 
>>   		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
>> -				QCOM_DOWNLOAD_MODE_MASK,
>> -				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
>> +				QCOM_DOWNLOAD_MODE_MASK, download_mode);
> 
> Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
> so you won't change lines that were introduced just before.

If you notice, I have just converted download mode data type from bool
to int in this patch and hence the changing the line here. Last patch 
was about just using the exported API, so i hope you would be fine here.

> 
> ...
> 
>>   }
>>   
>> +
> 
> Stray change.
> 
>> +static int get_download_mode(char *buffer, const struct kernel_param *kp)
>> +{
>> +	int len = 0;
>> +
>> +	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;
> 
> You can return directly.

Ok.

>  > Also, what about download_mode that doesn't fit to the above two?

return sysfs_emit(buffer, "unknown\n"); ?

> 
>> +}
> 
> ...
> 
>> +static int set_download_mode(const char *val, const struct kernel_param *kp)
>> +{
>> +	u32 old = download_mode;
>> +
>> +	if (sysfs_streq(val, "full")) {
>> +		download_mode = QCOM_DOWNLOAD_FULLDUMP;
>> +	} else if (sysfs_streq(val, "off")) {
>> +		download_mode = QCOM_DOWNLOAD_NODUMP;
> 
> NIH sysfs_match_string().

NIH ?

My apology, if i did not get this..
Do you want me to use sysfs_match_string()
and how would that help compare to what is present now ?

> 
>> +	} else if (kstrtouint(val, 0, &download_mode) ||
>> +		   !(download_mode == 0 || download_mode == 1)) {
>> +		download_mode = old;
>> +		pr_err("qcom_scm: unknown download mode: %s\n", val);
> 
>> +		return -EINVAL;
> 
> Do not shadow the error code from kstrtouint() it can be different to this one.

Will fix this.

> 
>> +	}
>> +
>> +	if (__scm)
>> +		qcom_scm_set_download_mode(download_mode);
>> +
>> +	return 0;
>> +}
> 
> ...
> 
> Have you updated corresponding documentation about this parameter?
> Or there is none?

There is none as of yet outside this file; should that be good what i 
have added in 5/5..

> 

-Mukesh

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

[Andy Shevchenko]

On Mon, Jun 26, 2023 at 7:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> On 5/27/2023 3:38 AM, andy.shevchenko@gmail.com wrote:
> > Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:

...

> >>              ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> >> -                            QCOM_DOWNLOAD_MODE_MASK,
> >> -                            enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
> >> +                            QCOM_DOWNLOAD_MODE_MASK, download_mode);
> >
> > Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
> > so you won't change lines that were introduced just before.
>
> If you notice, I have just converted download mode data type from bool
> to int in this patch and hence the changing the line here. Last patch
> was about just using the exported API, so i hope you would be fine here.

Thank you for elaboration. I'm fine with that.

...

> >  > Also, what about download_mode that doesn't fit to the above two?
>
> return sysfs_emit(buffer, "unknown\n"); ?

For example, yes.

...

> >> +static int set_download_mode(const char *val, const struct kernel_param *kp)
> >> +{
> >> +    u32 old = download_mode;
> >> +
> >> +    if (sysfs_streq(val, "full")) {
> >> +            download_mode = QCOM_DOWNLOAD_FULLDUMP;
> >> +    } else if (sysfs_streq(val, "off")) {
> >> +            download_mode = QCOM_DOWNLOAD_NODUMP;
> >
> > NIH sysfs_match_string().
>
> NIH ?

Not Invented Here

> My apology, if i did not get this..
> Do you want me to use sysfs_match_string()

Yes.

> and how would that help compare to what is present now ?

This will make your ABi gathered in one place (all strings and all
values) and less code will be used esp. if it's going to be expanded
in the future (isn't it in the next patches?).


> >> +    }
> >> +
> >> +    if (__scm)
> >> +            qcom_scm_set_download_mode(download_mode);
> >> +
> >> +    return 0;
> >> +}

...

> > Have you updated corresponding documentation about this parameter?
> > Or there is none?
>
> There is none as of yet outside this file; should that be good what i
> have added in 5/5..

It was a long time ago when I reviewed this. Just a note that all ABI
must be documented, debugfs is highly recommended to be documented.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v6 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 as
enable it along with fulldump.

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

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fa6502a..780e50a 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -32,6 +32,8 @@ static u32 download_mode;
 
 #define QCOM_DOWNLOAD_MODE_MASK 0x30
 #define QCOM_DOWNLOAD_FULLDUMP	0x1
+#define QCOM_DOWNLOAD_MINIDUMP  0x2
+#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
 #define QCOM_DOWNLOAD_NODUMP	0x0
 
 struct qcom_scm {
@@ -1420,13 +1422,16 @@ 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 = 0;
 
 	if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
 		len = sysfs_emit(buffer, "full\n");
+	else if (download_mode == QCOM_DOWNLOAD_MINIDUMP)
+		len = sysfs_emit(buffer, "mini\n");
+	else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)
+		len = sysfs_emit(buffer, "full,mini\n");
 	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
 		len = sysfs_emit(buffer, "off\n");
 
@@ -1437,8 +1442,12 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
 {
 	u32 old = download_mode;
 
-	if (sysfs_streq(val, "full")) {
+	if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
+		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
+	} else if (sysfs_streq(val, "full")) {
 		download_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (sysfs_streq(val, "mini")) {
+		download_mode = QCOM_DOWNLOAD_MINIDUMP;
 	} else if (sysfs_streq(val, "off")) {
 		download_mode = QCOM_DOWNLOAD_NODUMP;
 	} else if (kstrtouint(val, 0, &download_mode) ||
@@ -1461,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/full,mini or mini,full and 0/1 for existing users");
 
 static int qcom_scm_probe(struct platform_device *pdev)
 {
-- 
2.7.4

Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode support

[andy.shevchenko]

Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti:
> Currently, scm driver only supports full dump when download
> mode is selected. Add support to enable minidump as well as
> enable it along with fulldump.

...

>  #define QCOM_DOWNLOAD_MODE_MASK 0x30
>  #define QCOM_DOWNLOAD_FULLDUMP	0x1
> +#define QCOM_DOWNLOAD_MINIDUMP  0x2
> +#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)

Now order is broken.

>  #define QCOM_DOWNLOAD_NODUMP	0x0

...

> @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -

Stray change and ping-pong style at the same time.

...

>  	if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
>  		len = sysfs_emit(buffer, "full\n");
> +	else if (download_mode == QCOM_DOWNLOAD_MINIDUMP)
> +		len = sysfs_emit(buffer, "mini\n");
> +	else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)

> +		len = sysfs_emit(buffer, "full,mini\n");

Why not "both" ?

>  	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
>  		len = sysfs_emit(buffer, "off\n");


With an array (for streq_match_string() call suggested earlier) this become as
simple as

	if (mode >= ARRAY_SIZE(...))
		return sysfs_emit("Oh heh!\n");

	return sysfs_emit("%s\n", array[mode]);

...

> -	if (sysfs_streq(val, "full")) {

Why changing this line?

> +	if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
> +		download_mode = QCOM_DOWNLOAD_BOTHDUMP;

It's way too hard, esp. taking into account that once user enters wrong order,
user can't simply validate this by reading value back.

Use "both" and that's it.

> +	} else if (sysfs_streq(val, "full")) {
>  		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> +	} else if (sysfs_streq(val, "mini")) {
> +		download_mode = QCOM_DOWNLOAD_MINIDUMP;

...

>  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/full,mini or mini,full and 0/1 for existing users");

You really must be consistent with at least a couple of things:
1) capitalization;
2) indentation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode support

[Bjorn Andersson]

On Sat, May 27, 2023 at 01:14:50AM +0300, andy.shevchenko@gmail.com wrote:
> Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti:
> > Currently, scm driver only supports full dump when download
> > mode is selected. Add support to enable minidump as well as
> > enable it along with fulldump.
> 
> ...
> 
> >  #define QCOM_DOWNLOAD_MODE_MASK 0x30
> >  #define QCOM_DOWNLOAD_FULLDUMP	0x1
> > +#define QCOM_DOWNLOAD_MINIDUMP  0x2
> > +#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
> 
> Now order is broken.
> 
> >  #define QCOM_DOWNLOAD_NODUMP	0x0
> 
> ...
> 
> > @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -
> 
> Stray change and ping-pong style at the same time.
> 
> ...
> 
> >  	if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
> >  		len = sysfs_emit(buffer, "full\n");
> > +	else if (download_mode == QCOM_DOWNLOAD_MINIDUMP)
> > +		len = sysfs_emit(buffer, "mini\n");
> > +	else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)
> 
> > +		len = sysfs_emit(buffer, "full,mini\n");
> 
> Why not "both" ?
> 

"both" isn't very future proof (and I think we've had additional
variations in the past already), so I asked for this form.

Many thanks for your thorough review, Andy!

Regards,
Bjorn

> >  	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
> >  		len = sysfs_emit(buffer, "off\n");
> 
> 
> With an array (for streq_match_string() call suggested earlier) this become as
> simple as
> 
> 	if (mode >= ARRAY_SIZE(...))
> 		return sysfs_emit("Oh heh!\n");
> 
> 	return sysfs_emit("%s\n", array[mode]);
> 
> ...
> 
> > -	if (sysfs_streq(val, "full")) {
> 
> Why changing this line?
> 
> > +	if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
> > +		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
> 
> It's way too hard, esp. taking into account that once user enters wrong order,
> user can't simply validate this by reading value back.
> 
> Use "both" and that's it.
> 
> > +	} else if (sysfs_streq(val, "full")) {
> >  		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> > +	} else if (sysfs_streq(val, "mini")) {
> > +		download_mode = QCOM_DOWNLOAD_MINIDUMP;
> 
> ...
> 
> >  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/full,mini or mini,full and 0/1 for existing users");
> 
> You really must be consistent with at least a couple of things:
> 1) capitalization;
> 2) indentation.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode support

[Andy Shevchenko]

On Sat, May 27, 2023 at 2:32 AM Bjorn Andersson <andersson@kernel.org> wrote:
> On Sat, May 27, 2023 at 01:14:50AM +0300, andy.shevchenko@gmail.com wrote:
> > Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti:

...

> > >     if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
> > >             len = sysfs_emit(buffer, "full\n");
> > > +   else if (download_mode == QCOM_DOWNLOAD_MINIDUMP)
> > > +           len = sysfs_emit(buffer, "mini\n");
> > > +   else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)
> >
> > > +           len = sysfs_emit(buffer, "full,mini\n");
> >
> > Why not "both" ?

> "both" isn't very future proof (and I think we've had additional
> variations in the past already), so I asked for this form.

Okay, so this should be the bit flags and we should parse a list of
the values. In that case I may agree with the approach.

> >       if (mode >= ARRAY_SIZE(...))
> >               return sysfs_emit("Oh heh!\n");
> >
> >       return sysfs_emit("%s\n", array[mode]);

...

> > > +   if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
> > > +           download_mode = QCOM_DOWNLOAD_BOTHDUMP;
> >
> > It's way too hard, esp. taking into account that once user enters wrong order,
> > user can't simply validate this by reading value back.
> >
> > Use "both" and that's it.
> >
> > > +   } else if (sysfs_streq(val, "full")) {
> > >             download_mode = QCOM_DOWNLOAD_FULLDUMP;
> > > +   } else if (sysfs_streq(val, "mini")) {
> > > +           download_mode = QCOM_DOWNLOAD_MINIDUMP;

As per above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/5] Refactor to support multiple download mode

[Mukesh Ojha]

Bjorn and others,

Any further comments on this series ?

-- Mukesh

On 3/29/2023 1:16 PM, Mukesh Ojha wrote:
> Intention of this series to support multiple download mode and
> only modify the required bits during setting tcsr register.
> 
> Other download modes are minidump, full dump, both fulldump + minidump, nodump.
> 
> Latest minidump kernel driver patches has been sent here
> https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/
> 
> Also, this series should be applied on
> https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/
> 
> Changes in v6:
>    - Applied suggested API change(at v4) by [dmitry.baryshkov]
> 
> Changes in v5: https://lore.kernel.org/lkml/1680017869-22421-1-git-send-email-quic_mojha@quicinc.com/
>    - Tried to fix the issue reported by kernel test robot
>      https://lore.kernel.org/lkml/202303280535.acb66sQT-lkp@intel.com/
> 
>    - Applied some of the improvement suggested by [Bjorn.andersson]
>   
>      . Dropped 'both' instead support full,mini or mini,full for setting download
>      mode to collect both minidump and full dump.
>      
>      . logging improvement.
>      
> 
> Changes in v4: https://lore.kernel.org/lkml/1679935281-18445-1-git-send-email-quic_mojha@quicinc.com/
>    - val should be shifted within the function [srinivas.kandagatla]
>      i.e new = (old & ~mask) | (val << ffs(mask) - 1);
>    - Added Acked-by [linus.walleij] on pinctrl change.
> 
> Changes in v3 : https://lore.kernel.org/lkml/1679070482-8391-1-git-send-email-quic_mojha@quicinc.com/
>   - 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     | 11 ++---
>   include/linux/firmware/qcom/qcom_scm.h |  2 +
>   4 files changed, 86 insertions(+), 26 deletions(-)
> 

Re: [PATCH v6 0/5] Refactor to support multiple download mode

[andy.shevchenko]

Mon, Apr 10, 2023 at 08:04:44PM +0530, Mukesh Ojha kirjoitti:
> Bjorn and others,
> 
> Any further comments on this series ?

> -- Mukesh
I have comments.

> On 3/29/2023 1:16 PM, Mukesh Ojha wrote:

-- 
With Best Regards,
Andy Shevchenko