| Message ID | 1679935281-18445-1-git-send-email-quic_mojha@quicinc.com |
|---|---|
| Headers | show |
| Series | Refactor to support multiple download mode | expand |
On Mon, Mar 27, 2023 at 10:11:21PM +0530, Mukesh Ojha wrote: > Currently, scm driver only supports full dump when download > mode is selected. Add support to enable minidump as well both > dump(full dump + minidump). > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > drivers/firmware/qcom_scm.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 0c94429..19315d0 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -32,6 +32,8 @@ static u32 download_mode; > > #define QCOM_DOWNLOAD_MODE_MASK 0x30 > #define QCOM_DOWNLOAD_FULLDUMP 0x1 > +#define QCOM_DOWNLOAD_MINIDUMP 0x2 > +#define QCOM_DOWNLOAD_BOTHDUMP (QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP) > #define QCOM_DOWNLOAD_NODUMP 0x0 > > struct qcom_scm { > @@ -1421,13 +1423,16 @@ 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_MINIDUMP) > + len = sysfs_emit(buffer, "mini\n"); > + else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP) > + len = sysfs_emit(buffer, "both\n"); > else if (download_mode == QCOM_DOWNLOAD_NODUMP) > len = sysfs_emit(buffer, "off\n"); > > @@ -1440,6 +1445,10 @@ static int set_download_mode(const char *val, const struct kernel_param *kp) > > if (!strncmp(val, "full", strlen("full"))) { > download_mode = QCOM_DOWNLOAD_FULLDUMP; > + } else if (!strncmp(val, "mini", strlen("mini"))) { > + download_mode = QCOM_DOWNLOAD_MINIDUMP; > + } else if (!strncmp(val, "both", strlen("both"))) { "both" isn't very future proof... How about allowing mini,full? You don't need to do string tokenizing etc, just strcmp mini,full (and full,mini if you want to be fancy)... Regards, Bjorn > + download_mode = QCOM_DOWNLOAD_BOTHDUMP; > } else if (!strncmp(val, "off", strlen("off"))) { > download_mode = QCOM_DOWNLOAD_NODUMP; > } else if (kstrtouint(val, 0, &download_mode) || > @@ -1462,7 +1471,7 @@ static const struct kernel_param_ops download_mode_param_ops = { > > module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644); > MODULE_PARM_DESC(download_mode, > - "Download mode: off/full or 0/1 for existing users"); > + "download mode: off/full/mini/both(full+mini) or 0/1 for existing users"); > > static int qcom_scm_probe(struct platform_device *pdev) > { > -- > 2.7.4 >
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. -- 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
On 3/27/2023 11:57 PM, Bjorn Andersson wrote: > On Mon, Mar 27, 2023 at 10:11:21PM +0530, Mukesh Ojha wrote: >> Currently, scm driver only supports full dump when download >> mode is selected. Add support to enable minidump as well both >> dump(full dump + minidump). >> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> --- >> drivers/firmware/qcom_scm.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 0c94429..19315d0 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -32,6 +32,8 @@ static u32 download_mode; >> >> #define QCOM_DOWNLOAD_MODE_MASK 0x30 >> #define QCOM_DOWNLOAD_FULLDUMP 0x1 >> +#define QCOM_DOWNLOAD_MINIDUMP 0x2 >> +#define QCOM_DOWNLOAD_BOTHDUMP (QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP) >> #define QCOM_DOWNLOAD_NODUMP 0x0 >> >> struct qcom_scm { >> @@ -1421,13 +1423,16 @@ 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_MINIDUMP) >> + len = sysfs_emit(buffer, "mini\n"); >> + else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP) >> + len = sysfs_emit(buffer, "both\n"); >> else if (download_mode == QCOM_DOWNLOAD_NODUMP) >> len = sysfs_emit(buffer, "off\n"); >> >> @@ -1440,6 +1445,10 @@ static int set_download_mode(const char *val, const struct kernel_param *kp) >> >> if (!strncmp(val, "full", strlen("full"))) { >> download_mode = QCOM_DOWNLOAD_FULLDUMP; >> + } else if (!strncmp(val, "mini", strlen("mini"))) { >> + download_mode = QCOM_DOWNLOAD_MINIDUMP; >> + } else if (!strncmp(val, "both", strlen("both"))) { > > "both" isn't very future proof... > > How about allowing mini,full? You don't need to do string tokenizing > etc, just strcmp mini,full (and full,mini if you want to be fancy)... > Thanks for the suggestion, this looks good. I have applied the changes. -- Mukesh > Regards, > Bjorn > >> + download_mode = QCOM_DOWNLOAD_BOTHDUMP; >> } else if (!strncmp(val, "off", strlen("off"))) { >> download_mode = QCOM_DOWNLOAD_NODUMP; >> } else if (kstrtouint(val, 0, &download_mode) || >> @@ -1462,7 +1471,7 @@ static const struct kernel_param_ops download_mode_param_ops = { >> >> module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644); >> MODULE_PARM_DESC(download_mode, >> - "Download mode: off/full or 0/1 for existing users"); >> + "download mode: off/full/mini/both(full+mini) or 0/1 for existing users"); >> >> static int qcom_scm_probe(struct platform_device *pdev) >> { >> -- >> 2.7.4 >>
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
On 3/29/2023 3:44 AM, Dmitry Baryshkov wrote: > 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(). You are awesome !! Thanks. Have applied the change. -- Mukesh > >> >> -- 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 >
Intention of this series to support multiple download mode and only modify the required bits during setting tcsr register. Other download modes are minidump, bothdump (full dump + minidump). Latest minidump kernel driver patches has been sent here https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/ Also, this series should be applied on https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/ Changes in v4: - val should be shifted within the function [srinivas.kandagatla] i.e new = (old & ~mask) | (val << ffs(mask) - 1); - Added Acked-by [linus.walleij] on pinctrl change. Changes in v3 : https://lore.kernel.org/lkml/1679070482-8391-1-git-send-email-quic_mojha@quicinc.com/ - Removed [1] from the series and sent as a separate patch[2], although this series should be applied on top [2]. [1] https://lore.kernel.org/lkml/1677664555-30191-2-git-send-email-quic_mojha@quicinc.com/ [2] https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/ - Introduce new exported symbol on suggestion from [srinivas.kandagatla] - Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c. - Addressed comment given by [dmitry.baryshkov] - Converted non-standard Originally-by to Signed-off-by. Changes in v2: https://lore.kernel.org/lkml/1677664555-30191-1-git-send-email-quic_mojha@quicinc.com/ - Addressed comment made by [bjorn] - Added download mask. - Passed download mode as parameter - Accept human accepatable download mode string. - enable = !!dload_mode - Shifted module param callback to somewhere down in the file so that it no longer need to know the prototype of qcom_scm_set_download_mode() - updated commit text. Mukesh Ojha (5): firmware: qcom_scm: provide a read-modify-write function pinctrl: qcom: Use qcom_scm_io_update_field() firmware: scm: Modify only the download bits in TCSR register firmware: qcom_scm: Refactor code to support multiple download mode firmware: qcom_scm: Add multiple download mode support drivers/firmware/Kconfig | 11 ----- drivers/firmware/qcom_scm.c | 89 +++++++++++++++++++++++++++++++--- drivers/pinctrl/qcom/pinctrl-msm.c | 11 ++--- include/linux/firmware/qcom/qcom_scm.h | 2 + 4 files changed, 87 insertions(+), 26 deletions(-)