From: Mukesh Ojha <quic_mojha@quicinc.com>
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 <quic_poovendh@quicinc.com>
Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
---
Changes in V5:
- Added the Signed-off-by tag for user Poovendhan
- Dropped the macro QCOM_DOWNLOAD_MODE_SHIFT in the favor of
PREP_FIELD
drivers/firmware/qcom_scm.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 104d86e49b97..3830dcf14326 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,10 @@ 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_MASK BIT(4)
+
struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -440,6 +444,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 +453,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;
+ ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+ QCOM_DOWNLOAD_MODE_MASK,
+ FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
--
2.34.1
On 7/20/2023 12:04 AM, Kathiravan T wrote:
> From: Mukesh Ojha <quic_mojha@quicinc.com>
>
> 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 <quic_poovendh@quicinc.com>
> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
> ---
> Changes in V5:
> - Added the Signed-off-by tag for user Poovendhan
> - Dropped the macro QCOM_DOWNLOAD_MODE_SHIFT in the favor of
> PREP_FIELD
>
> drivers/firmware/qcom_scm.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 104d86e49b97..3830dcf14326 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -30,6 +30,10 @@ 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_MASK BIT(4)
> +
Can you update __qcom_scm_set_dload_mode to use the FIELD_PREP bits as
well? Ideally, you should be able to have no duplicate logic in
__qcom_scm_set_dload_mode and in qcom_scm_set_download_mode. Before your
patch, it was duplicated and we probably should've had it de-duplicated.
With this patch, the logic and constants used have diverged when they
don't need to.
> struct qcom_scm {
> struct device *dev;
> struct clk *core_clk;
> @@ -440,6 +444,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 +453,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;
> + ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> + QCOM_DOWNLOAD_MODE_MASK,
> + FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download mode\n");
On 7/25/2023 12:35 AM, Elliot Berman wrote:
>
>
> On 7/20/2023 12:04 AM, Kathiravan T wrote:
>> From: Mukesh Ojha <quic_mojha@quicinc.com>
>>
>> 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 <quic_poovendh@quicinc.com>
>> Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
>> ---
>> Changes in V5:
>> - Added the Signed-off-by tag for user Poovendhan
>> - Dropped the macro QCOM_DOWNLOAD_MODE_SHIFT in the favor of
>> PREP_FIELD
>>
>> drivers/firmware/qcom_scm.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 104d86e49b97..3830dcf14326 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -30,6 +30,10 @@ 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_MASK BIT(4)
>> +
>
> Can you update __qcom_scm_set_dload_mode to use the FIELD_PREP bits as
> well? Ideally, you should be able to have no duplicate logic in
> __qcom_scm_set_dload_mode and in qcom_scm_set_download_mode. Before
> your patch, it was duplicated and we probably should've had it
> de-duplicated. With this patch, the logic and constants used have
> diverged when they don't need to.
Sure, will check this.
>
>> struct qcom_scm {
>> struct device *dev;
>> struct clk *core_clk;
>> @@ -440,6 +444,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 +453,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;
>> + ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
>> + QCOM_DOWNLOAD_MODE_MASK,
>> + FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
>> } else {
>> dev_err(__scm->dev,
>> "No available mechanism for setting download mode\n");
Hi Kathiravan,
kernel test robot noticed the following build errors:
[auto build test ERROR on linusw-pinctrl/devel]
[also build test ERROR on linusw-pinctrl/for-next linus/master v6.5-rc2 next-20230721]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kathiravan-T/firmware-qcom_scm-provide-a-read-modify-write-function/20230720-150657
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link: https://lore.kernel.org/r/20230720070408.1093698-4-quic_kathirav%40quicinc.com
patch subject: [PATCH V5 3/3] firmware: scm: Modify only the download bits in TCSR register
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230722/202307220736.M9yPz2Cs-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230722/202307220736.M9yPz2Cs-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307220736.M9yPz2Cs-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/firmware/qcom_scm.c: In function 'qcom_scm_set_download_mode':
>> drivers/firmware/qcom_scm.c:459:48: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
459 | FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
| ^~~~~~~~~~
cc1: some warnings being treated as errors
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for SM_GCC_8350
Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 || COMPILE_TEST [=n])
Selected by [m]:
- SM_VIDEOCC_8350 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
WARNING: unmet direct dependencies detected for SM_GCC_8450
Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 || COMPILE_TEST [=n])
Selected by [m]:
- SM_GPUCC_8450 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
- SM_VIDEOCC_8450 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
WARNING: unmet direct dependencies detected for SM_GCC_8550
Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 || COMPILE_TEST [=n])
Selected by [m]:
- SM_GPUCC_8550 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
- SM_VIDEOCC_8550 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
vim +/FIELD_PREP +459 drivers/firmware/qcom_scm.c
443
444 static void qcom_scm_set_download_mode(bool enable)
445 {
446 bool avail;
447 int val;
448 int ret = 0;
449
450 avail = __qcom_scm_is_call_available(__scm->dev,
451 QCOM_SCM_SVC_BOOT,
452 QCOM_SCM_BOOT_SET_DLOAD_MODE);
453 if (avail) {
454 ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
455 } else if (__scm->dload_mode_addr) {
456 val = enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP;
457 ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
458 QCOM_DOWNLOAD_MODE_MASK,
> 459 FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
460 } else {
461 dev_err(__scm->dev,
462 "No available mechanism for setting download mode\n");
463 }
464
465 if (ret)
466 dev_err(__scm->dev, "failed to set download mode: %d\n", ret);
467 }
468
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2026 Red Hat, Inc.