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).
Minidump kernel driver patches has been sent here
https://lore.kernel.org/lkml/[email protected]/
Changes in v3:
- 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/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
- 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/[email protected]/
- 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 | 88 ++++++++++++++++++++++++++++++----
drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++---
include/linux/firmware/qcom/qcom_scm.h | 2 +
4 files changed, 89 insertions(+), 27 deletions(-)
--
2.7.4
It was released by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.
Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.
Suggested-by: Srinivas Kandagatla <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/qcom_scm.c | 15 +++++++++++++++
include/linux/firmware/qcom/qcom_scm.h | 2 ++
2 files changed, 17 insertions(+)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 3e020d1..aca2556 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
}
EXPORT_SYMBOL(qcom_scm_set_remote_state);
+int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
+{
+ unsigned int old, new;
+ int ret;
+
+ ret = qcom_scm_io_readl(addr, &old);
+ if (ret)
+ return ret;
+
+ new = (old & ~mask) | val;
+
+ return qcom_scm_io_writel(addr, new);
+}
+EXPORT_SYMBOL(qcom_scm_io_update_field);
+
static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
{
struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 1e449a5..203a781 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
+ unsigned int val);
extern bool qcom_scm_restore_sec_cfg_available(void);
extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
--
2.7.4
Use qcom_scm_io_update_field() exported function introduced
in last commit.
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index daeb79a..3d3d520 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1016,6 +1016,8 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
unsigned long flags;
bool was_enabled;
u32 val;
+ u32 tmp_val;
+ u32 mask;
if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
set_bit(d->hwirq, pctrl->dual_edge_irqs);
@@ -1049,24 +1051,21 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
* With intr_target_use_scm interrupts are routed to
* application cpu using scm calls.
*/
+ mask = (7 << g->intr_target_bit);
+ tmp_val = g->intr_target_kpss_val << g->intr_target_bit;
if (pctrl->intr_target_use_scm) {
u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
int ret;
- qcom_scm_io_readl(addr, &val);
-
- val &= ~(7 << g->intr_target_bit);
- val |= g->intr_target_kpss_val << g->intr_target_bit;
-
- ret = qcom_scm_io_writel(addr, val);
+ ret = qcom_scm_io_update_field(addr, mask, tmp_val);
if (ret)
dev_err(pctrl->dev,
"Failed routing %lu interrupt to Apps proc",
d->hwirq);
} else {
val = msm_readl_intr_target(pctrl, g);
- val &= ~(7 << g->intr_target_bit);
- val |= g->intr_target_kpss_val << g->intr_target_bit;
+ val &= ~mask;
+ val |= tmp_val;
msm_writel_intr_target(val, pctrl, g);
}
--
2.7.4
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.
Signed-off-by: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/qcom_scm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index aca2556..eb491da 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,8 @@ module_param(download_mode, bool, 0);
#define SCM_HAS_IFACE_CLK BIT(1)
#define SCM_HAS_BUS_CLK BIT(2)
+#define QCOM_DOWNLOAD_MODE_MASK 0x30
+
struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -448,7 +450,8 @@ 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,
+ ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+ QCOM_DOWNLOAD_MODE_MASK,
enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
} else {
dev_err(__scm->dev,
--
2.7.4
Currently on Qualcomm SoC, download_mode is enabled if
CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected.
Refactor the code such that it supports multiple download
modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
instead, give interface to set the download mode from
module parameter.
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/Kconfig | 11 ---------
drivers/firmware/qcom_scm.c | 57 +++++++++++++++++++++++++++++++++++++++------
2 files changed, 50 insertions(+), 18 deletions(-)
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e304..ff7e9f3 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -215,17 +215,6 @@ config MTK_ADSP_IPC
config QCOM_SCM
tristate
-config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
- bool "Qualcomm download mode enabled by default"
- depends on QCOM_SCM
- help
- A device with "download mode" enabled will upon an unexpected
- warm-restart enter a special debug mode that allows the user to
- "download" memory content over USB for offline postmortem analysis.
- The feature can be enabled/disabled on the kernel command line.
-
- Say Y here to enable "download mode" by default.
-
config SYSFB
bool
select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index eb491da..69f864d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -20,17 +20,19 @@
#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)
#define SCM_HAS_BUS_CLK BIT(2)
#define QCOM_DOWNLOAD_MODE_MASK 0x30
+#define QCOM_DOWNLOAD_FULLDUMP 0x10
+#define QCOM_DOWNLOAD_NODUMP 0x0
struct qcom_scm {
struct device *dev;
@@ -439,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;
@@ -452,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_SCM_BOOT_SET_DLOAD_MODE : 0);
+ enable ? download_mode : 0);
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
@@ -1418,6 +1421,46 @@ 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;
+
+ 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)
+{
+ if (!strncmp(val, "full", strlen("full"))) {
+ 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)) {
+ pr_err("unknown download mode\n");
+ return -EINVAL;
+ }
+
+ if (__scm)
+ qcom_scm_set_download_mode(download_mode);
+
+ return 0;
+}
+
+static const struct kernel_param_ops download_mode_param_ops = {
+ .get = get_download_mode,
+ .set = set_download_mode,
+};
+
+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");
+
static int qcom_scm_probe(struct platform_device *pdev)
{
struct qcom_scm *scm;
@@ -1511,12 +1554,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
__get_convention();
/*
- * If requested enable "download mode", from this point on warmboot
+ * If "download mode" is requested, from this point on warmboot
* will cause the boot stages to enter download mode, unless
* disabled below by a clean shutdown/reboot.
*/
if (download_mode)
- qcom_scm_set_download_mode(true);
+ qcom_scm_set_download_mode(download_mode);
return 0;
}
@@ -1524,7 +1567,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
static void qcom_scm_shutdown(struct platform_device *pdev)
{
/* Clean shutdown, disable download mode to allow normal restart */
- qcom_scm_set_download_mode(false);
+ qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
}
static const struct of_device_id qcom_scm_dt_match[] = {
--
2.7.4
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 <[email protected]>
---
drivers/firmware/qcom_scm.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 69f864d..ba3eefc 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -32,6 +32,9 @@ static u32 download_mode;
#define QCOM_DOWNLOAD_MODE_MASK 0x30
#define QCOM_DOWNLOAD_FULLDUMP 0x10
+#define QCOM_DOWNLOAD_MINIDUMP 0x20
+#define QCOM_DOWNLOAD_BOTHDUMP (QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
+
#define QCOM_DOWNLOAD_NODUMP 0x0
struct qcom_scm {
@@ -1421,15 +1424,19 @@ 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;
+ int len = 0;
- if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
+ if (download_mode == QCOM_DOWNLOAD_FULLDUMP) {
len = sysfs_emit(buffer, "full\n");
- else if (download_mode == QCOM_DOWNLOAD_NODUMP)
+ } 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");
+ }
return len;
}
@@ -1438,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"))) {
+ download_mode = QCOM_DOWNLOAD_BOTHDUMP;
} else if (!strncmp(val, "off", strlen("off"))) {
download_mode = QCOM_DOWNLOAD_NODUMP;
} else if (kstrtouint(val, 0, &download_mode) ||
@@ -1459,7 +1470,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/17/2023 9:57 PM, Mukesh Ojha wrote:
> It was released by Srinivas K. that there is a need of
Typo: s/released/realized
-- Mukesh
> read-modify-write scm exported function so that it can
> be used by multiple clients.
>
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
>
> Suggested-by: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/firmware/qcom_scm.c | 15 +++++++++++++++
> include/linux/firmware/qcom/qcom_scm.h | 2 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 3e020d1..aca2556 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> }
> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>
> +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> + unsigned int old, new;
> + int ret;
> +
> + ret = qcom_scm_io_readl(addr, &old);
> + if (ret)
> + return ret;
> +
> + new = (old & ~mask) | val;
> +
> + return qcom_scm_io_writel(addr, new);
> +}
> +EXPORT_SYMBOL(qcom_scm_io_update_field);
> +
> static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> {
> struct qcom_scm_desc desc = {
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 1e449a5..203a781 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
>
> extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
> extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
> + unsigned int val);
>
> extern bool qcom_scm_restore_sec_cfg_available(void);
> extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <[email protected]> wrote:
> It was released by Srinivas K. that there is a need of
> read-modify-write scm exported function so that it can
> be used by multiple clients.
>
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
>
> Suggested-by: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
This is starting to reimplement regmap.
In this case regmap_update_bits().
What about just using regmap as accessor for these
registers instead?
Yours,
Linus Walleij
On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <[email protected]> wrote:
> Use qcom_scm_io_update_field() exported function introduced
> in last commit.
>
> Signed-off-by: Mukesh Ojha <[email protected]>
Fine by me, but I want you to first consider switching the
custom register accessors to regmap.
Yours,
Linus Walleij
On Fri, Mar 17, 2023 at 09:56:59PM +0100, Linus Walleij wrote:
> On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <[email protected]> wrote:
>
> > It was released by Srinivas K. that there is a need of
> > read-modify-write scm exported function so that it can
> > be used by multiple clients.
> >
> > Let's introduce qcom_scm_io_update_field() which masks
> > out the bits and write the passed value to that
> > bit-offset. Subsequent patch will use this function.
> >
> > Suggested-by: Srinivas Kandagatla <[email protected]>
> > Signed-off-by: Mukesh Ojha <[email protected]>
>
> This is starting to reimplement regmap.
> In this case regmap_update_bits().
>
> What about just using regmap as accessor for these
> registers instead?
>
I'm not sure it would be beneficial...
The regmap interface provides a standardized representation of a block
of registers, with the suitable accessors backing it. But in both cases
touched upon in this series, the addressed registers are part of regions
already handled by the kernel.
So it wouldn't be suitable to create a regmap-abstraction for "a block
of secure registers", at best that would give us two kinds of regmaps
abstracting the same register block.
Instead I believe we'd need to extend the struct regmap_config to
introduce a new table telling a new secure-or-unsecure-mmio-regmap which
accessor (secure or unsecure read/write) shoudl be used, and then have
e.g. pinctrl-msm register such regmap, passing the information about
which registers in its memory region is secure.
We'd still need qcom_scm_io_readl() and qcom_scm_io_writel() exported to
implement the new custom regmap implementation - and the struct
regmap_config needed in just pinctrl-msm alone would be larger than the
one function it replaces.
But please let me know if I'm missing something?
Regards,
Bjorn
On Fri, Mar 17, 2023 at 09:58:04PM +0100, Linus Walleij wrote:
> On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <[email protected]> wrote:
>
> > Use qcom_scm_io_update_field() exported function introduced
> > in last commit.
> >
> > Signed-off-by: Mukesh Ojha <[email protected]>
>
> Fine by me, but I want you to first consider switching the
> custom register accessors to regmap.
>
I took a quick look at it and there seem to be two ways that it can be
done.
We can retain the MSM_ACCESSOR() macros that generates the custom
register accessors, but plug in a regmap between these accessors and the
mmio operations. But this just adds a few extra hops inbetween the
driver and the volatile read/write, with a slight increase of memory,
without any obvious benefits.
The more alluring alternative is to replace the custom accessors with
reg_fields. This would allow us to replace some (perhaps many) of the
bit-manipulation with regmap_update_bits().
But at minimum we'd need one reg_field per register, per pin, so that's
5 reg_fields per pin which adds up to ~10-24kb extra space, depending on
platform.
Even more alluring would be to have reg_fields describing the actual
fields in the registers, which would allow us to better utilize the
regmap API directly. This would cost us 35-75kb of heap.
IMHO this is quite a significant effort, and given that the driver seems
to be doing its job I'd rather see such efforts being focused elsewhere.
Regards,
Bjorn
On Mon, Mar 20, 2023 at 4:19 AM Bjorn Andersson <[email protected]> wrote:
> > This is starting to reimplement regmap.
> > In this case regmap_update_bits().
> >
> > What about just using regmap as accessor for these
> > registers instead?
> >
>
> I'm not sure it would be beneficial...
Me neither, I don't know the details, I just notice the similarity in the
accessors.
> The regmap interface provides a standardized representation of a block
> of registers, with the suitable accessors backing it. But in both cases
> touched upon in this series, the addressed registers are part of regions
> already handled by the kernel.
>
> So it wouldn't be suitable to create a regmap-abstraction for "a block
> of secure registers", at best that would give us two kinds of regmaps
> abstracting the same register block.
From my viewpoint regmap does three things:
- Abstract one coherent region of registers under a shared lock, with
nifty accessors (such as mask-and-set with regmap_update_bits())
- Maps access patterns/permissions and permissible access range
- Optionally cache the contents
The way I would use it if these secure registers are in the same range as a
bunch of non-secure ones is for sharing a lock and the regmap accessors.
I wouldn't worry about access patterns and such. That usecase (block
access to certain registers or bits) is partly overdesign in some cases
IMO.
If regmap abstraction isn't helpful overall then we shouldn't do it.
Yours,
Linus Walleij
On Mon, Mar 20, 2023 at 5:07 AM Bjorn Andersson <[email protected]> wrote:
> > Fine by me, but I want you to first consider switching the
> > custom register accessors to regmap.
(...)
> IMHO this is quite a significant effort, and given that the driver seems
> to be doing its job I'd rather see such efforts being focused elsewhere.
I think you know it better than me, if regmap is just going to clutter
the view the don't do it.
Regmap does have the upside of looking the same on all platforms so it
would potentially give less maintenance burden.
Acked-by: Linus Walleij <[email protected]>
for these patches if you need to merge them elsewhere, I can also
queue them if you ACK them.
Yours,
Linus Walleij
On 17/03/2023 16:27, Mukesh Ojha wrote:
> It was released by Srinivas K. that there is a need of
> read-modify-write scm exported function so that it can
> be used by multiple clients.
>
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
>
> Suggested-by: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> ---
> drivers/firmware/qcom_scm.c | 15 +++++++++++++++
> include/linux/firmware/qcom/qcom_scm.h | 2 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 3e020d1..aca2556 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> }
> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>
> +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> + unsigned int old, new;
> + int ret;
> +
> + ret = qcom_scm_io_readl(addr, &old);
> + if (ret)
> + return ret;
> +
> + new = (old & ~mask) | val;
thanks for doing this,
With field semantics, val should be shifted within the function, so the
caller only sets value for field rather than passing a shifted value.
so this should be:
new = (old & ~mask) | (val << ffs(mask) - 1);
--srini
> +
> + return qcom_scm_io_writel(addr, new);
> +}
> +EXPORT_SYMBOL(qcom_scm_io_update_field);
> +
> static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> {
> struct qcom_scm_desc desc = {
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 1e449a5..203a781 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
>
> extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
> extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
> + unsigned int val);
>
> extern bool qcom_scm_restore_sec_cfg_available(void);
> extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);