1. Patchew
  2. linux
  3. [PATCH v2 0/4] Refactor to support multiple download mode
  4. View patch

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

Mukesh Ojha posted 4 patches 3 years, 1 month ago
There is a newer version of this series
[PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode
Posted by Mukesh Ojha 3 years, 1 month ago 
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);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.7.4
Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode
Posted by Srinivas Kandagatla 3 years, 1 month ago 

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");
Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode
Posted by Mukesh Ojha 3 years ago 

On 3/8/2023 8:36 PM, Srinivas Kandagatla wrote:
> 
> 
> 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)

Thanks for the review,

will do it in v2, will let /drivers/pinctrl/qcom/pinctrl-msm.c uses this.

-Mukesh
> 
> 
> --srini
>>       } else {
>>           dev_err(__scm->dev,
>>               "No available mechanism for setting download mode\n");
Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode
Posted by Dmitry Baryshkov 3 years, 1 month ago 
On 01/03/2023 11: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);

Any locking for this RMW?

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

-- 
With best wishes
Dmitry
Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode
Posted by Mukesh Ojha 3 years, 1 month ago 

On 3/1/2023 4:09 PM, Dmitry Baryshkov wrote:
> On 01/03/2023 11: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);
> 
> Any locking for this RMW?

While you ask this, i thought about who all are the user of this 
function. Only, multiple calls to module param callback where
this race could be possible.

I am doubtful, if introducing global mutex lock will be allowed to 
handle this. Any comments.


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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.