diff mbox series

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

Message ID 1677664555-30191-3-git-send-email-quic_mojha@quicinc.com
State New
Headers show
Series Refactor to support multiple download mode | expand

Commit Message

Mukesh Ojha March 1, 2023, 9:55 a.m. UTC
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>
---
Changes in v2:
 - Addressed comment made by Bjorn.
 - Added download mask from patch 3 to this.

 drivers/firmware/qcom_scm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Srinivas Kandagatla March 8, 2023, 3:06 p.m. UTC | #1
On 01/03/2023 09:55, 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>
> ---
> Changes in v2:
>   - Addressed comment made by Bjorn.
>   - Added download mask from patch 3 to this.
> 
>   drivers/firmware/qcom_scm.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 51eb853..c9f1fad 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -27,6 +27,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;
> @@ -419,6 +421,7 @@ static void qcom_scm_set_download_mode(bool enable)
>   {
>   	bool avail;
>   	int ret = 0;
> +	u32 val;
>   
>   	avail = __qcom_scm_is_call_available(__scm->dev,
>   					     QCOM_SCM_SVC_BOOT,
> @@ -426,8 +429,18 @@ 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, &val);
> +		if (ret) {
> +			dev_err(__scm->dev,
> +				"failed to read dload mode address value: %d\n", ret);
> +			return;
> +		}
> +
> +		val &= ~QCOM_DOWNLOAD_MODE_MASK;
> +		if (enable)
> +			val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
> +
> +		ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);


This is the second instance of such pattern of usage one is already in 
./drivers/pinctrl/qcom/pinctrl-msm.c

I think it makes sense to move setting fields in register to a dedicated 
function like this:

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);


then we could use it like this:
ret = qcom_scm_io_update_field(__scm->dload_mode_addr, 
QCOM_DOWNLOAD_MODE_MASK, dl_mode)


--srini
>   	} else {
>   		dev_err(__scm->dev,
>   			"No available mechanism for setting download mode\n");
diff mbox series

Patch

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 51eb853..c9f1fad 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -27,6 +27,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;
@@ -419,6 +421,7 @@  static void qcom_scm_set_download_mode(bool enable)
 {
 	bool avail;
 	int ret = 0;
+	u32 val;
 
 	avail = __qcom_scm_is_call_available(__scm->dev,
 					     QCOM_SCM_SVC_BOOT,
@@ -426,8 +429,18 @@  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, &val);
+		if (ret) {
+			dev_err(__scm->dev,
+				"failed to read dload mode address value: %d\n", ret);
+			return;
+		}
+
+		val &= ~QCOM_DOWNLOAD_MODE_MASK;
+		if (enable)
+			val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
+
+		ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");