| Message ID | 20240227155308.18395-4-quic_mojha@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc SCM driver changes | expand |
On Tue, Feb 27, 2024 at 09:23:02PM +0530, Mukesh Ojha wrote: > Crashdump collection is done based on DLOAD bits of TCSR register. > To retain other bits, scm driver need to read the register and > modify only the DLOAD bits, as other bits in TCSR may 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> > Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332 > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Reviewed-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/firmware/qcom/qcom_scm.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 8f766fce5f7c..bd6bfdf2d828 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -4,6 +4,8 @@ > */ > > #include <linux/arm-smccc.h> > +#include <linux/bitfield.h> > +#include <linux/bits.h> > #include <linux/clk.h> > #include <linux/completion.h> > #include <linux/cpumask.h> > @@ -114,6 +116,12 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { > #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) > #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) > > +#define QCOM_DLOAD_MASK GENMASK(5, 4) > +enum qcom_dload_mode { > + QCOM_DLOAD_NODUMP = 0, > + QCOM_DLOAD_FULLDUMP = 1, These values are not enumerations, they represent fixed/defined values in the interface. As such it's appropriate to use #define. Regards, Bjorn > +}; > + > static const char * const qcom_scm_convention_names[] = { > [SMC_CONVENTION_UNKNOWN] = "unknown", > [SMC_CONVENTION_ARM_32] = "smc arm 32", > @@ -531,6 +539,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) > > static void qcom_scm_set_download_mode(bool enable) > { > + u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP; > bool avail; > int ret = 0; > > @@ -540,8 +549,9 @@ 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); > + ret = qcom_scm_io_rmw(__scm->dload_mode_addr, > + QCOM_DLOAD_MASK, > + FIELD_PREP(QCOM_DLOAD_MASK, val)); > } else { > dev_err(__scm->dev, > "No available mechanism for setting download mode\n"); > -- > 2.43.0.254.ga26002b62827 >
On 3/3/2024 12:43 AM, Bjorn Andersson wrote: > On Tue, Feb 27, 2024 at 09:23:02PM +0530, Mukesh Ojha wrote: >> Crashdump collection is done based on DLOAD bits of TCSR register. >> To retain other bits, scm driver need to read the register and >> modify only the DLOAD bits, as other bits in TCSR may 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> >> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332 >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Reviewed-by: Elliot Berman <quic_eberman@quicinc.com> >> --- >> drivers/firmware/qcom/qcom_scm.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c >> index 8f766fce5f7c..bd6bfdf2d828 100644 >> --- a/drivers/firmware/qcom/qcom_scm.c >> +++ b/drivers/firmware/qcom/qcom_scm.c >> @@ -4,6 +4,8 @@ >> */ >> >> #include <linux/arm-smccc.h> >> +#include <linux/bitfield.h> >> +#include <linux/bits.h> >> #include <linux/clk.h> >> #include <linux/completion.h> >> #include <linux/cpumask.h> >> @@ -114,6 +116,12 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { >> #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) >> #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) >> >> +#define QCOM_DLOAD_MASK GENMASK(5, 4) >> +enum qcom_dload_mode { >> + QCOM_DLOAD_NODUMP = 0, >> + QCOM_DLOAD_FULLDUMP = 1, > > These values are not enumerations, they represent fixed/defined values > in the interface. As such it's appropriate to use #define. > Thanks for giving reasoning on why it should be #define and not enum. -Mukesh
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 8f766fce5f7c..bd6bfdf2d828 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -4,6 +4,8 @@ */ #include <linux/arm-smccc.h> +#include <linux/bitfield.h> +#include <linux/bits.h> #include <linux/clk.h> #include <linux/completion.h> #include <linux/cpumask.h> @@ -114,6 +116,12 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = { #define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0) #define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1) +#define QCOM_DLOAD_MASK GENMASK(5, 4) +enum qcom_dload_mode { + QCOM_DLOAD_NODUMP = 0, + QCOM_DLOAD_FULLDUMP = 1, +}; + static const char * const qcom_scm_convention_names[] = { [SMC_CONVENTION_UNKNOWN] = "unknown", [SMC_CONVENTION_ARM_32] = "smc arm 32", @@ -531,6 +539,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) static void qcom_scm_set_download_mode(bool enable) { + u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP; bool avail; int ret = 0; @@ -540,8 +549,9 @@ 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); + ret = qcom_scm_io_rmw(__scm->dload_mode_addr, + QCOM_DLOAD_MASK, + FIELD_PREP(QCOM_DLOAD_MASK, val)); } else { dev_err(__scm->dev, "No available mechanism for setting download mode\n");