2023-06-28 12:48:25

by Mukesh Ojha

Subject: [PATCH v4 19/21] firmware: scm: Modify only the download bits in TCSR register

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.

Co-developed-by: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/qcom_scm.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 104d86e49b97..a9ff77d16c42 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,11 @@ module_param(download_mode, bool, 0);
#define SCM_HAS_IFACE_CLK BIT(1)
#define SCM_HAS_BUS_CLK BIT(2)

+#define QCOM_DOWNLOAD_FULLDUMP 0x1
+#define QCOM_DOWNLOAD_NODUMP 0x0
+#define QCOM_DOWNLOAD_MODE_SHIFT 4
+#define QCOM_DOWNLOAD_MODE_MASK 0x30
+
struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -440,6 +445,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
static void qcom_scm_set_download_mode(bool enable)
{
bool avail;
+ int val;
int ret = 0;

avail = __qcom_scm_is_call_available(__scm->dev,
@@ -448,8 +454,10 @@ 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);
+ val = (enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP);
+ val <<= QCOM_DOWNLOAD_MODE_SHIFT;
+ ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+ QCOM_DOWNLOAD_MODE_MASK, val);
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
--
2.7.4

2023-06-28 15:49:21

by Konrad Dybcio

Subject: Re: [PATCH v4 19/21] firmware: scm: Modify only the download bits in TCSR register

On 28.06.2023 14:34, 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.
>
> Co-developed-by: Poovendhan Selvaraj <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/firmware/qcom_scm.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 104d86e49b97..a9ff77d16c42 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -30,6 +30,11 @@ module_param(download_mode, bool, 0);
> #define SCM_HAS_IFACE_CLK BIT(1)
> #define SCM_HAS_BUS_CLK BIT(2)
>
> +#define QCOM_DOWNLOAD_FULLDUMP 0x1
> +#define QCOM_DOWNLOAD_NODUMP 0x0
> +#define QCOM_DOWNLOAD_MODE_SHIFT 4
> +#define QCOM_DOWNLOAD_MODE_MASK 0x30
GENMASK and then FIELD_PREP below?

> +
> struct qcom_scm {
> struct device *dev;
> struct clk *core_clk;
> @@ -440,6 +445,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> static void qcom_scm_set_download_mode(bool enable)
> {
> bool avail;
> + int val;
What's wrong with initializing it in the same line as ret?

> int ret = 0;
>
> avail = __qcom_scm_is_call_available(__scm->dev,
> @@ -448,8 +454,10 @@ 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);
> + val = (enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP);
unnecessary braces

Konrad
> + val <<= QCOM_DOWNLOAD_MODE_SHIFT;
> + ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> + QCOM_DOWNLOAD_MODE_MASK, val);
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download mode\n");

2023-06-30 15:16:16

by Mukesh Ojha

Subject: Re: [PATCH v4 19/21] firmware: scm: Modify only the download bits in TCSR register

On 6/28/2023 8:50 PM, Konrad Dybcio wrote:
> On 28.06.2023 14:34, 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.
>>
>> Co-developed-by: Poovendhan Selvaraj <[email protected]>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> ---
>> drivers/firmware/qcom_scm.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 104d86e49b97..a9ff77d16c42 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -30,6 +30,11 @@ module_param(download_mode, bool, 0);
>> #define SCM_HAS_IFACE_CLK BIT(1)
>> #define SCM_HAS_BUS_CLK BIT(2)
>>
>> +#define QCOM_DOWNLOAD_FULLDUMP 0x1
>> +#define QCOM_DOWNLOAD_NODUMP 0x0
>> +#define QCOM_DOWNLOAD_MODE_SHIFT 4
>> +#define QCOM_DOWNLOAD_MODE_MASK 0x30
> GENMASK and then FIELD_PREP below?

Just checked about FIELD_PREP, it works, thanks.

FIELD_PREP(GENMASK(5, 4), QCOM_DOWNLOAD_FULLDUMP)
FIELD_PREP(GENMASK(5, 4), QCOM_DOWNLOAD_NODUMP)

-Mukesh

>
>> +
>> struct qcom_scm {
>> struct device *dev;
>> struct clk *core_clk;
>> @@ -440,6 +445,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>> static void qcom_scm_set_download_mode(bool enable)
>> {
>> bool avail;
>> + int val;
> What's wrong with initializing it in the same line as ret?
>
>> int ret = 0;
>>
>> avail = __qcom_scm_is_call_available(__scm->dev,
>> @@ -448,8 +454,10 @@ 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);
>> + val = (enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP);
> unnecessary braces
>
> Konrad
>> + val <<= QCOM_DOWNLOAD_MODE_SHIFT;
>> + ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
>> + QCOM_DOWNLOAD_MODE_MASK, val);
>> } else {
>> dev_err(__scm->dev,
>> "No available mechanism for setting download mode\n");