| Message ID | 20230720070408.1093698-4-quic_kathirav@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce the read-modify-write API to collect | expand |
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
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");
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");