Messages in this thread | | | Date | Wed, 29 Mar 2023 01:14:22 +0300 | Subject | Re: [PATCH v4 4/5] firmware: qcom_scm: Refactor code to support multiple download mode | From | Dmitry Baryshkov <> |
| |
On 28/03/2023 11:18, Mukesh Ojha wrote: > > > On 3/27/2023 11:53 PM, Bjorn Andersson wrote: >> On Mon, Mar 27, 2023 at 10:11:20PM +0530, Mukesh Ojha wrote: >> [..] >>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>> index 3c6c5e7..0c94429 100644 >>> --- a/drivers/firmware/qcom_scm.c >>> +++ b/drivers/firmware/qcom_scm.c >>> @@ -20,11 +20,11 @@ >>> #include <linux/clk.h> >>> #include <linux/reset-controller.h> >>> #include <linux/arm-smccc.h> >>> +#include <linux/kstrtox.h> >>> #include "qcom_scm.h" >>> -static bool download_mode = >>> IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT); >>> -module_param(download_mode, bool, 0); >>> +static u32 download_mode; >>> #define SCM_HAS_CORE_CLK BIT(0) >>> #define SCM_HAS_IFACE_CLK BIT(1) >>> @@ -32,6 +32,7 @@ module_param(download_mode, bool, 0); >>> #define QCOM_DOWNLOAD_MODE_MASK 0x30 >>> #define QCOM_DOWNLOAD_FULLDUMP 0x1 >>> +#define QCOM_DOWNLOAD_NODUMP 0x0 >>> struct qcom_scm { >>> struct device *dev; >>> @@ -440,8 +441,9 @@ static int __qcom_scm_set_dload_mode(struct >>> device *dev, bool enable) >>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL); >>> } >>> -static void qcom_scm_set_download_mode(bool enable) >>> +static void qcom_scm_set_download_mode(u32 download_mode) >>> { >>> + bool enable = !!download_mode; >>> bool avail; >>> int ret = 0; >>> @@ -453,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable) >>> } else if (__scm->dload_mode_addr) { >>> ret = qcom_scm_io_update_field(__scm->dload_mode_addr, >>> QCOM_DOWNLOAD_MODE_MASK, >>> - enable ? QCOM_DOWNLOAD_FULLDUMP : 0); >>> + enable ? download_mode : 0); >> >> Afaict, with QCOM_DOWNLOAD_NODUMP as 0, this says: >> >> when download_mode is non-zero, write that value, otherwise write 0 >> >> That should be the same as "write download_mode", so you should be able >> to drop the enable part. >> >>> } else { >>> dev_err(__scm->dev, >>> "No available mechanism for setting download mode\n"); >>> @@ -1419,6 +1421,49 @@ static irqreturn_t qcom_scm_irq_handler(int >>> irq, void *data) >>> return IRQ_HANDLED; >>> } >>> + >>> +static int get_download_mode(char *buffer, const struct kernel_param >>> *kp) >>> +{ >>> + int len = 0; >>> + >>> + if (download_mode == QCOM_DOWNLOAD_FULLDUMP) >>> + len = sysfs_emit(buffer, "full\n"); >>> + else if (download_mode == QCOM_DOWNLOAD_NODUMP) >>> + len = sysfs_emit(buffer, "off\n"); >>> + >>> + return len; >>> +} >>> + >>> +static int set_download_mode(const char *val, const struct >>> kernel_param *kp) >>> +{ >>> + u32 old = download_mode; >>> + >>> + if (!strncmp(val, "full", strlen("full"))) { >> >> strcmp loops over the two string until they differ and/or both are >> '\0'. >> >> As such, the only thing you achieve by using strncmp(.., T, strlen(T)) >> is that the code has to iterate over T twice - and you make the code >> harder to read. > > > If we use strcmp, i need to use "full\n" which we would not want to do. > I think, we need to take this hit.
There is a special helper for the sysfs files. See sysfs_streq().
> > -- Mukesh >> >>> + download_mode = QCOM_DOWNLOAD_FULLDUMP; >>> + } else if (!strncmp(val, "off", strlen("off"))) { >>> + download_mode = QCOM_DOWNLOAD_NODUMP; >>> + } else if (kstrtouint(val, 0, &download_mode) || >>> + !(download_mode == 0 || download_mode == 1)) { >>> + download_mode = old; >>> + pr_err("unknown download mode\n"); >> >> This will result in a lone "unknown download mode" line somewhere in the >> kernel log, without association to any driver or any indication what the >> unknown value was. >> >> pr_err("qcom_scm: unknown download mode: %s\n", val); >> >> Would give both context and let the reader know right there what value >> the code wasn't able to match. >> >> Regards, >> Bjorn
-- With best wishes Dmitry
| |