diff mbox series

[v8,2/3] firmware: scm: Modify only the download bits in TCSR register

Message ID 1698235506-16993-3-git-send-email-quic_mojha@quicinc.com
State Superseded
Headers show
Series Misc SCM changes | expand

Commit Message

Mukesh Ojha Oct. 25, 2023, 12:05 p.m. UTC 
Crashdump collection is done based on DLOAD bits of TCSR register.
To retain other bits, scm driver need to read the register and
modify only the DLOAD bits, as other bits in TCSR may have their
own significance.

Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/firmware/qcom/qcom_scm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Elliot Berman Oct. 25, 2023, 11:53 p.m. UTC | #1 
On 10/25/2023 5:05 AM, Mukesh Ojha wrote:
> Crashdump collection is done based on DLOAD bits of TCSR register.
> To retain other bits, scm driver need to read the register and
> modify only the DLOAD bits, as other bits in TCSR may have their
> own significance.
> 
> Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 25549178a30f..f1c4a9f9a53f 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -4,6 +4,8 @@
>   */
>  
>  #include <linux/arm-smccc.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/cpumask.h>
> @@ -117,6 +119,10 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>  #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
>  #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
>  
> +#define QCOM_DLOAD_MASK		GENMASK(5, 4)
> +#define QCOM_DLOAD_FULLDUMP	0x1
> +#define QCOM_DLOAD_NODUMP	0x0
> +


Enum would be better here for related constants.

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index f1c4a9f9a53f..95f73a8c51d7 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -122,4 +122,6 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 #define QCOM_DLOAD_MASK                GENMASK(5, 4)
-#define QCOM_DLOAD_FULLDUMP    0x1
-#define QCOM_DLOAD_NODUMP      0x0
+enum qcom_dload_mode {
+       QCOM_DLOAD_NODUMP       = 0,
+       QCOM_DLOAD_FULLDUMP     = 1,
+};
 


>  static const char * const qcom_scm_convention_names[] = {
>  	[SMC_CONVENTION_UNKNOWN] = "unknown",
>  	[SMC_CONVENTION_ARM_32] = "smc arm 32",
> @@ -523,6 +529,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>  
>  static void qcom_scm_set_download_mode(bool enable)
>  {
> +	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
>  	bool avail;
>  	int ret = 0;
>  
> @@ -532,8 +539,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_rmw(__scm->dload_mode_addr,
> +					       QCOM_DLOAD_MASK,
> +					       FIELD_PREP(QCOM_DLOAD_MASK, val));
>  	} else {
>  		dev_err(__scm->dev,
>  			"No available mechanism for setting download mode\n");

- Elliot
Mukesh Ojha Oct. 27, 2023, 9:03 a.m. UTC | #2 
On 10/26/2023 5:23 AM, Elliot Berman wrote:
> 
> 
> On 10/25/2023 5:05 AM, Mukesh Ojha wrote:
>> Crashdump collection is done based on DLOAD bits of TCSR register.
>> To retain other bits, scm driver need to read the register and
>> modify only the DLOAD bits, as other bits in TCSR may have their
>> own significance.
>>
>> Co-developed-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
>> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/firmware/qcom/qcom_scm.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 25549178a30f..f1c4a9f9a53f 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -4,6 +4,8 @@
>>    */
>>   
>>   #include <linux/arm-smccc.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>>   #include <linux/clk.h>
>>   #include <linux/completion.h>
>>   #include <linux/cpumask.h>
>> @@ -117,6 +119,10 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>>   #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
>>   #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
>>   
>> +#define QCOM_DLOAD_MASK		GENMASK(5, 4)
>> +#define QCOM_DLOAD_FULLDUMP	0x1
>> +#define QCOM_DLOAD_NODUMP	0x0
>> +
> 
> 
> Enum would be better here for related constants.
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index f1c4a9f9a53f..95f73a8c51d7 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -122,4 +122,6 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
>   #define QCOM_DLOAD_MASK                GENMASK(5, 4)
> -#define QCOM_DLOAD_FULLDUMP    0x1
> -#define QCOM_DLOAD_NODUMP      0x0
> +enum qcom_dload_mode {
> +       QCOM_DLOAD_NODUMP       = 0,
> +       QCOM_DLOAD_FULLDUMP     = 1,
> +};

Would it be fine, if i do it during when i add some more modes with
minidump ?

Please ack, otherwise, will send another version.

-Mukesh

>   
> 
> 
>>   static const char * const qcom_scm_convention_names[] = {
>>   	[SMC_CONVENTION_UNKNOWN] = "unknown",
>>   	[SMC_CONVENTION_ARM_32] = "smc arm 32",
>> @@ -523,6 +529,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>   
>>   static void qcom_scm_set_download_mode(bool enable)
>>   {
>> +	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
>>   	bool avail;
>>   	int ret = 0;
>>   
>> @@ -532,8 +539,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_rmw(__scm->dload_mode_addr,
>> +					       QCOM_DLOAD_MASK,
>> +					       FIELD_PREP(QCOM_DLOAD_MASK, val));
>>   	} else {
>>   		dev_err(__scm->dev,
>>   			"No available mechanism for setting download mode\n");
> 
> - Elliot
diff mbox series

Patch

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 25549178a30f..f1c4a9f9a53f 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -4,6 +4,8 @@ 
  */
 
 #include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/cpumask.h>
@@ -117,6 +119,10 @@  static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
 #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
 
+#define QCOM_DLOAD_MASK		GENMASK(5, 4)
+#define QCOM_DLOAD_FULLDUMP	0x1
+#define QCOM_DLOAD_NODUMP	0x0
+
 static const char * const qcom_scm_convention_names[] = {
 	[SMC_CONVENTION_UNKNOWN] = "unknown",
 	[SMC_CONVENTION_ARM_32] = "smc arm 32",
@@ -523,6 +529,7 @@  static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 
 static void qcom_scm_set_download_mode(bool enable)
 {
+	u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
 	bool avail;
 	int ret = 0;
 
@@ -532,8 +539,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_rmw(__scm->dload_mode_addr,
+					       QCOM_DLOAD_MASK,
+					       FIELD_PREP(QCOM_DLOAD_MASK, val));
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");