[PATCH v11 0/4] Misc SCM driver changes

[PATCH v11 0/4] Misc SCM driver changes

[Mukesh Ojha]

First two changes changes are needed to enable download mode on
chipset like IPQ9574 and IPQ5332 SoCs as for these targets others
bits in download mode registers are used for different purpose
and earlier kernel code was mistakenly overwrite the other bits.

First three changes(1/4-3/4) are SCM driver specific while 4/4 from
pinctrl try to use the introduced API(1/3).

Changes from v10:
 - Rebased on linux-next tag 20240108

Changes from v9: https://lore.kernel.org/lkml/1698648967-974-1-git-send-email-quic_mojha@quicinc.com/
 - Added 3/4 new patch.
 - commit subject modification.

Change from v8: https://lore.kernel.org/lkml/1698235506-16993-1-git-send-email-quic_mojha@quicinc.com/
 - Introduce enum for dload mode constants as per suggestion from [Elliot].
 - Rebased on linux-next.

Changes from v7: https://lore.kernel.org/lkml/1696440338-12561-1-git-send-email-quic_mojha@quicinc.com/
 - Rebased it on next-20231025.
 - Added reviewed-by tag and take care of comment made about
   commit text should be in imperative mode.
 - Modified the name of the API to qcom_scm_io_rmw() as per suggestion
   made by [Dmitry]
 - Moved spinlock inside qcom_scm structure.
 - Corrected the patch order as per subsystem SCM first then pinctrl.

Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mojha@quicinc.com/ - Removed mistakenly added macros.
   https://lore.kernel.org/lkml/9da888dc-401a-4cbb-b616-b4654fa79e35@quicinc.com/
 - Added Acked-by tag from Linus.w to 2/3.
Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mojha@quicinc.com/
 - Removed mistakenly added macros.
   https://lore.kernel.org/lkml/9da888dc-401a-4cbb-b616-b4654fa79e35@quicinc.com/
 - Added Acked-by tag from Linus.w to 2/3.

Changes in v6: https://lore.kernel.org/lkml/1680076012-10785-1-git-send-email-quic_mojha@quicinc.com/
 - Rebased it on latest tag available on linux-next
 - Added missed Poovendhan sign-off on 15/17 and tested-by tag from
   Kathiravan. Thanks to him for testing and reminding me of missing sign-off.
 - Addressed comments made on dload mode patch v6 version

Changes in v5: https://lore.kernel.org/lkml/1680017869-22421-1-git-send-email-quic_mojha@quicinc.com/
  - Tried to fix the issue reported by kernel test robot
    https://lore.kernel.org/lkml/202303280535.acb66sQT-lkp@intel.com/

  - Applied some of the improvement suggested by [Bjorn.andersson]

    . Dropped 'both' instead support full,mini or mini,full for setting download
    mode to collect both minidump and full dump.

    . logging improvement.

Changes in v4: https://lore.kernel.org/lkml/1679935281-18445-1-git-send-email-quic_mojha@quicinc.com/
  - 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 (4):
  firmware: qcom: scm: provide a read-modify-write function
  firmware: qcom: scm: Modify only the download bits in TCSR register
  firmware: qcom: scm: Rework dload mode availability check
  pinctrl: qcom: Use qcom_scm_io_rmw() function

 drivers/firmware/qcom/qcom_scm.c       | 50 ++++++++++++++++++++++++++++------
 drivers/pinctrl/qcom/pinctrl-msm.c     | 10 +++----
 include/linux/firmware/qcom/qcom_scm.h |  1 +
 3 files changed, 47 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

It was realized 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_rmw() which masks out the bits
and write the passed value to that bit-offset.

Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
---
 drivers/firmware/qcom/qcom_scm.c       | 26 ++++++++++++++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 520de9b5633a..25549178a30f 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -19,6 +19,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 #include <linux/reset-controller.h>
 #include <linux/types.h>
 
@@ -41,6 +42,8 @@ struct qcom_scm {
 	int scm_vote_count;
 
 	u64 dload_mode_addr;
+	/* Atomic context only */
+	spinlock_t lock;
 };
 
 struct qcom_scm_current_perm_info {
@@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void)
 	return ret ? : res.result[0];
 }
 
+int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
+{
+	unsigned int old, new;
+	int ret;
+
+	if (!__scm)
+		return -EINVAL;
+
+	spin_lock(&__scm->lock);
+	ret = qcom_scm_io_readl(addr, &old);
+	if (ret)
+		goto unlock;
+
+	new = (old & ~mask) | (val & mask);
+
+	ret = qcom_scm_io_writel(addr, new);
+unlock:
+	spin_unlock(&__scm->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
+
 static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
 	struct qcom_scm_desc desc = {
@@ -1824,6 +1849,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 		return ret;
 
 	mutex_init(&scm->scm_bw_lock);
+	spin_lock_init(&scm->lock);
 
 	scm->path = devm_of_icc_get(&pdev->dev, NULL);
 	if (IS_ERR(scm->path))
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index ccaf28846054..3a8bb2e603b3 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral);
 
 int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
 int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val);
 
 bool qcom_scm_restore_sec_cfg_available(void);
 int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
-- 
2.7.4

[PATCH v11 2/4] firmware: qcom: scm: Modify only the download bits in TCSR register

[Mukesh Ojha]

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>
---
 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 25549178a30f..4421f219fe9a 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>
@@ -117,6 +119,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",
@@ -523,6 +531,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;
 
@@ -532,8 +541,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.7.4

[PATCH v11 3/4] firmware: qcom: scm: Rework dload mode availability check

[Mukesh Ojha]

QCOM_SCM_BOOT_SET_DLOAD_MODE was only valid for very older
target and firmware and for recent targets there is dload
mode tcsr registers is available to set the download mode.

So, it is better to keep it as fallback check instead of
checking its availability and failing it always.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 4421f219fe9a..87bcd5c02f2b 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -532,18 +532,16 @@ 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;
 
-	avail = __qcom_scm_is_call_available(__scm->dev,
-					     QCOM_SCM_SVC_BOOT,
-					     QCOM_SCM_BOOT_SET_DLOAD_MODE);
-	if (avail) {
-		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
-	} else if (__scm->dload_mode_addr) {
+	if (__scm->dload_mode_addr) {
 		ret = qcom_scm_io_rmw(__scm->dload_mode_addr,
 				      QCOM_DLOAD_MASK,
 				      FIELD_PREP(QCOM_DLOAD_MASK, val));
+	} else if (__qcom_scm_is_call_available(__scm->dev,
+						QCOM_SCM_SVC_BOOT,
+						QCOM_SCM_BOOT_SET_DLOAD_MODE)) {
+		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.7.4

[PATCH v11 4/4] pinctrl: qcom: Use qcom_scm_io_rmw() function

[Mukesh Ojha]

Use qcom_scm_io_rmw() exported function in pinctrl-msm
driver.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index aeaf0d1958f5..1bd5c8c43fcd 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1082,22 +1082,20 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	if (g->intr_target_width)
 		intr_target_mask = GENMASK(g->intr_target_width - 1, 0);
 
+	intr_target_mask <<= 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 &= ~(intr_target_mask << g->intr_target_bit);
-		val |= g->intr_target_kpss_val << g->intr_target_bit;
-
-		ret = qcom_scm_io_writel(addr, val);
+		val = g->intr_target_kpss_val << g->intr_target_bit;
+		ret = qcom_scm_io_rmw(addr, intr_target_mask, 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 &= ~(intr_target_mask << g->intr_target_bit);
+		val &= ~intr_target_mask;
 		val |= g->intr_target_kpss_val << g->intr_target_bit;
 		msm_writel_intr_target(val, pctrl, g);
 	}
-- 
2.7.4

Re: [PATCH v11 2/4] firmware: qcom: scm: Modify only the download bits in TCSR register

[Elliot Berman]

On 1/8/2024 7:27 AM, 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 25549178a30f..4421f219fe9a 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>
> @@ -117,6 +119,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",
> @@ -523,6 +531,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;
>  
> @@ -532,8 +541,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");

Re: [PATCH v11 3/4] firmware: qcom: scm: Rework dload mode availability check

[Elliot Berman]

On 1/8/2024 7:27 AM, Mukesh Ojha wrote:
> QCOM_SCM_BOOT_SET_DLOAD_MODE was only valid for very older
> target and firmware and for recent targets there is dload
> mode tcsr registers is available to set the download mode.
> 
> So, it is better to keep it as fallback check instead of
> checking its availability and failing it always.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>

Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>

> ---
>  drivers/firmware/qcom/qcom_scm.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 4421f219fe9a..87bcd5c02f2b 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -532,18 +532,16 @@ 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;
>  
> -	avail = __qcom_scm_is_call_available(__scm->dev,
> -					     QCOM_SCM_SVC_BOOT,
> -					     QCOM_SCM_BOOT_SET_DLOAD_MODE);
> -	if (avail) {
> -		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> -	} else if (__scm->dload_mode_addr) {
> +	if (__scm->dload_mode_addr) {
>  		ret = qcom_scm_io_rmw(__scm->dload_mode_addr,
>  				      QCOM_DLOAD_MASK,
>  				      FIELD_PREP(QCOM_DLOAD_MASK, val));
> +	} else if (__qcom_scm_is_call_available(__scm->dev,
> +						QCOM_SCM_SVC_BOOT,
> +						QCOM_SCM_BOOT_SET_DLOAD_MODE)) {
> +		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>  	} else {
>  		dev_err(__scm->dev,
>  			"No available mechanism for setting download mode\n");

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Pavan Kondeti]

On Mon, Jan 08, 2024 at 08:57:31PM +0530, Mukesh Ojha wrote:
> It was realized 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_rmw() which masks out the bits
> and write the passed value to that bit-offset.
> 
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
> ---
>  drivers/firmware/qcom/qcom_scm.c       | 26 ++++++++++++++++++++++++++
>  include/linux/firmware/qcom/qcom_scm.h |  1 +
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..25549178a30f 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -19,6 +19,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>  #include <linux/reset-controller.h>
>  #include <linux/types.h>
>  
> @@ -41,6 +42,8 @@ struct qcom_scm {
>  	int scm_vote_count;
>  
>  	u64 dload_mode_addr;
> +	/* Atomic context only */
> +	spinlock_t lock;

IMHO, this comment can be confusing later. one might think that
qcom_scm_call_atomic() needs to be called with this lock, but that does
not seems to be the intention here.

>  };
>  
>  struct qcom_scm_current_perm_info {
> @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void)
>  	return ret ? : res.result[0];
>  }
>  
> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> +	unsigned int old, new;
> +	int ret;
> +
> +	if (!__scm)
> +		return -EINVAL;
> +
> +	spin_lock(&__scm->lock);

So, this function can't be called from hardirq context. If that ever
happens, with this new spinlock (without disabling interrupts), can
result in deadlock.

> +	ret = qcom_scm_io_readl(addr, &old);
> +	if (ret)
> +		goto unlock;
> +
> +	new = (old & ~mask) | (val & mask);
> +
> +	ret = qcom_scm_io_writel(addr, new);
> +unlock:
> +	spin_unlock(&__scm->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);

Thanks,
Pavan

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

On 1/9/2024 10:34 AM, Pavan Kondeti wrote:
> On Mon, Jan 08, 2024 at 08:57:31PM +0530, Mukesh Ojha wrote:
>> It was realized 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_rmw() which masks out the bits
>> and write the passed value to that bit-offset.
>>
>> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
>> ---
>>   drivers/firmware/qcom/qcom_scm.c       | 26 ++++++++++++++++++++++++++
>>   include/linux/firmware/qcom/qcom_scm.h |  1 +
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 520de9b5633a..25549178a30f 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/of_irq.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>>   #include <linux/reset-controller.h>
>>   #include <linux/types.h>
>>   
>> @@ -41,6 +42,8 @@ struct qcom_scm {
>>   	int scm_vote_count;
>>   
>>   	u64 dload_mode_addr;
>> +	/* Atomic context only */
>> +	spinlock_t lock;
> 
> IMHO, this comment can be confusing later. one might think that
> qcom_scm_call_atomic() needs to be called with this lock, but that does
> not seems to be the intention here.
> 
>>   };
>>   
>>   struct qcom_scm_current_perm_info {
>> @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void)
>>   	return ret ? : res.result[0];
>>   }
>>   
>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
>> +{
>> +	unsigned int old, new;
>> +	int ret;
>> +
>> +	if (!__scm)
>> +		return -EINVAL;
>> +
>> +	spin_lock(&__scm->lock);
> 
> So, this function can't be called from hardirq context. If that ever
> happens, with this new spinlock (without disabling interrupts), can
> result in deadlock.

Ok, let's make it fully atomic with spin_lock_irqsave();

-Mukesh
> 
>> +	ret = qcom_scm_io_readl(addr, &old);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	new = (old & ~mask) | (val & mask);
>> +
>> +	ret = qcom_scm_io_writel(addr, new);
>> +unlock:
>> +	spin_unlock(&__scm->lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
> 
> Thanks,
> Pavan

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Linus Walleij]

On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:

> It was realized 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_rmw() which masks out the bits
> and write the passed value to that bit-offset.
(...)
> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> +       unsigned int old, new;
> +       int ret;
> +
> +       if (!__scm)
> +               return -EINVAL;
> +
> +       spin_lock(&__scm->lock);
> +       ret = qcom_scm_io_readl(addr, &old);
> +       if (ret)
> +               goto unlock;
> +
> +       new = (old & ~mask) | (val & mask);
> +
> +       ret = qcom_scm_io_writel(addr, new);
> +unlock:
> +       spin_unlock(&__scm->lock);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);

This looks a lot like you are starting to re-invent regmaps
regmap_update_bits().

If you are starting to realize you need more and more of
regmap, why not use regmap and its functions?

Yours,
Linus Walleij

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

On 1/9/2024 6:44 PM, Linus Walleij wrote:
> On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> 
>> It was realized 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_rmw() which masks out the bits
>> and write the passed value to that bit-offset.
> (...)
>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
>> +{
>> +       unsigned int old, new;
>> +       int ret;
>> +
>> +       if (!__scm)
>> +               return -EINVAL;
>> +
>> +       spin_lock(&__scm->lock);
>> +       ret = qcom_scm_io_readl(addr, &old);
>> +       if (ret)
>> +               goto unlock;
>> +
>> +       new = (old & ~mask) | (val & mask);
>> +
>> +       ret = qcom_scm_io_writel(addr, new);
>> +unlock:
>> +       spin_unlock(&__scm->lock);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
> 
> This looks a lot like you are starting to re-invent regmaps
> regmap_update_bits().
> 
> If you are starting to realize you need more and more of
> regmap, why not use regmap and its functions?

I think, this discussion has happened already ..

https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/

-Mukesh

> 
> Yours,
> Linus Walleij

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Linus Walleij]

On Tue, Jan 9, 2024 at 2:24 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> On 1/9/2024 6:44 PM, Linus Walleij wrote:
> > On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> >
> >> It was realized 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_rmw() which masks out the bits
> >> and write the passed value to that bit-offset.
> > (...)
> >> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
> >> +{
> >> +       unsigned int old, new;
> >> +       int ret;
> >> +
> >> +       if (!__scm)
> >> +               return -EINVAL;
> >> +
> >> +       spin_lock(&__scm->lock);
> >> +       ret = qcom_scm_io_readl(addr, &old);
> >> +       if (ret)
> >> +               goto unlock;
> >> +
> >> +       new = (old & ~mask) | (val & mask);
> >> +
> >> +       ret = qcom_scm_io_writel(addr, new);
> >> +unlock:
> >> +       spin_unlock(&__scm->lock);
> >> +       return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
> >
> > This looks a lot like you are starting to re-invent regmaps
> > regmap_update_bits().
> >
> > If you are starting to realize you need more and more of
> > regmap, why not use regmap and its functions?
>
> I think, this discussion has happened already ..
>
> https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/

That discussion ended with:

[Bjorn]
> 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.

When you add more and more accessors the premise starts to
change, and it becomes more and more of a reimplementation.

It may be time to actually fix this.

Yours,
Linus Walleij

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Bjorn Andersson]

On Tue, Jan 09, 2024 at 02:34:10PM +0100, Linus Walleij wrote:
> On Tue, Jan 9, 2024 at 2:24 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> > On 1/9/2024 6:44 PM, Linus Walleij wrote:
> > > On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> > >
> > >> It was realized 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_rmw() which masks out the bits
> > >> and write the passed value to that bit-offset.
> > > (...)
> > >> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
> > >> +{
> > >> +       unsigned int old, new;
> > >> +       int ret;
> > >> +
> > >> +       if (!__scm)
> > >> +               return -EINVAL;
> > >> +
> > >> +       spin_lock(&__scm->lock);
> > >> +       ret = qcom_scm_io_readl(addr, &old);
> > >> +       if (ret)
> > >> +               goto unlock;
> > >> +
> > >> +       new = (old & ~mask) | (val & mask);
> > >> +
> > >> +       ret = qcom_scm_io_writel(addr, new);
> > >> +unlock:
> > >> +       spin_unlock(&__scm->lock);
> > >> +       return ret;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
> > >
> > > This looks a lot like you are starting to re-invent regmaps
> > > regmap_update_bits().
> > >
> > > If you are starting to realize you need more and more of
> > > regmap, why not use regmap and its functions?
> >
> > I think, this discussion has happened already ..
> >
> > https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/
> 
> That discussion ended with:
> 
> [Bjorn]
> > 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.
> 
> When you add more and more accessors the premise starts to
> change, and it becomes more and more of a reimplementation.
> 
> It may be time to actually fix this.
> 

Thought I had replied to this already, did we discuss this previously as
well?

My concern with expressing this as a regmap is that from the provider's
point of view, the regmap would span the entire 32-bit address space.
I'm guessing that there's something on the other side limiting what
subregions are actually accessible for each platform/firmware
configuration, but I'm not convinced that regmap a good abstraction...

Regards,
Bjorn

> Yours,
> Linus Walleij

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Bjorn Andersson]

On Mon, Jan 08, 2024 at 08:57:31PM +0530, Mukesh Ojha wrote:
> It was realized by Srinivas K. that there is a need of

"need" is a strong word for this functionality, unless there's some use
case that I'm missing.

> read-modify-write scm exported function so that it can
> be used by multiple clients.
> 
> Let's introduce qcom_scm_io_rmw() which masks out the bits
> and write the passed value to that bit-offset.
> 
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
> ---
>  drivers/firmware/qcom/qcom_scm.c       | 26 ++++++++++++++++++++++++++
>  include/linux/firmware/qcom/qcom_scm.h |  1 +
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..25549178a30f 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -19,6 +19,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>  #include <linux/reset-controller.h>
>  #include <linux/types.h>
>  
> @@ -41,6 +42,8 @@ struct qcom_scm {
>  	int scm_vote_count;
>  
>  	u64 dload_mode_addr;
> +	/* Atomic context only */
> +	spinlock_t lock;
>  };
>  
>  struct qcom_scm_current_perm_info {
> @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void)
>  	return ret ? : res.result[0];
>  }
>  
> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
> +{
> +	unsigned int old, new;
> +	int ret;
> +
> +	if (!__scm)
> +		return -EINVAL;
> +
> +	spin_lock(&__scm->lock);

Please express that this lock is just for create mutual exclusion
between rmw operations, nothing else.

Also please make a statement why this is desirable and/or needed.

Regards,
Bjorn

> +	ret = qcom_scm_io_readl(addr, &old);
> +	if (ret)
> +		goto unlock;
> +
> +	new = (old & ~mask) | (val & mask);
> +
> +	ret = qcom_scm_io_writel(addr, new);
> +unlock:
> +	spin_unlock(&__scm->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
> +
>  static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>  {
>  	struct qcom_scm_desc desc = {
> @@ -1824,6 +1849,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	mutex_init(&scm->scm_bw_lock);
> +	spin_lock_init(&scm->lock);
>  
>  	scm->path = devm_of_icc_get(&pdev->dev, NULL);
>  	if (IS_ERR(scm->path))
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index ccaf28846054..3a8bb2e603b3 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral);
>  
>  int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>  int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val);
>  
>  bool qcom_scm_restore_sec_cfg_available(void);
>  int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
> -- 
> 2.7.4
> 

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

On 2/17/2024 12:09 AM, Bjorn Andersson wrote:
> On Mon, Jan 08, 2024 at 08:57:31PM +0530, Mukesh Ojha wrote:
>> It was realized by Srinivas K. that there is a need of
> 
> "need" is a strong word for this functionality, unless there's some use
> case that I'm missing.

I would rather say as below,

""
It is possible that different bits of a secure register is used
for different purpose and currently, there is no such available
function from SCM driver to do that; one similar usage was pointed
by Srinivas K. inside pinctrl-msm where interrupt configuration
register lying in secure region and written via read-modify-write operation.

Export qcom_scm_io_rmw() to do read-modify-write operation on secure 
register and reuse it wherever applicable.


""
> 
>> read-modify-write scm exported function so that it can
>> be used by multiple clients.
>>
>> Let's introduce qcom_scm_io_rmw() which masks out the bits
>> and write the passed value to that bit-offset.
>>
>> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332
>> ---
>>   drivers/firmware/qcom/qcom_scm.c       | 26 ++++++++++++++++++++++++++
>>   include/linux/firmware/qcom/qcom_scm.h |  1 +
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 520de9b5633a..25549178a30f 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/of_irq.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>>   #include <linux/reset-controller.h>
>>   #include <linux/types.h>
>>   
>> @@ -41,6 +42,8 @@ struct qcom_scm {
>>   	int scm_vote_count;
>>   
>>   	u64 dload_mode_addr;
>> +	/* Atomic context only */
>> +	spinlock_t lock;
>>   };
>>   
>>   struct qcom_scm_current_perm_info {
>> @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void)
>>   	return ret ? : res.result[0];
>>   }
>>   
>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
>> +{
>> +	unsigned int old, new;
>> +	int ret;
>> +
>> +	if (!__scm)
>> +		return -EINVAL;
>> +
>> +	spin_lock(&__scm->lock);
> 
> Please express that this lock is just for create mutual exclusion
> between rmw operations, nothing else.
> 
> Also please make a statement why this is desirable and/or needed.

Sure.

However, i was thinking of reusing existing scm_query_lock with rename
which is used only during boot up in __get_convention() .

-Mukesh
> 
> Regards,
> Bjorn
> 
>> +	ret = qcom_scm_io_readl(addr, &old);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	new = (old & ~mask) | (val & mask);
>> +
>> +	ret = qcom_scm_io_writel(addr, new);
>> +unlock:
>> +	spin_unlock(&__scm->lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
>> +
>>   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>   {
>>   	struct qcom_scm_desc desc = {
>> @@ -1824,6 +1849,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>   		return ret;
>>   
>>   	mutex_init(&scm->scm_bw_lock);
>> +	spin_lock_init(&scm->lock);
>>   
>>   	scm->path = devm_of_icc_get(&pdev->dev, NULL);
>>   	if (IS_ERR(scm->path))
>> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
>> index ccaf28846054..3a8bb2e603b3 100644
>> --- a/include/linux/firmware/qcom/qcom_scm.h
>> +++ b/include/linux/firmware/qcom/qcom_scm.h
>> @@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral);
>>   
>>   int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
>>   int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val);
>>   
>>   bool qcom_scm_restore_sec_cfg_available(void);
>>   int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
>> -- 
>> 2.7.4
>>

Re: [PATCH v11 1/4] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

On 2/17/2024 12:01 AM, Bjorn Andersson wrote:
> On Tue, Jan 09, 2024 at 02:34:10PM +0100, Linus Walleij wrote:
>> On Tue, Jan 9, 2024 at 2:24 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>>> On 1/9/2024 6:44 PM, Linus Walleij wrote:
>>>> On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>>>>
>>>>> It was realized 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_rmw() which masks out the bits
>>>>> and write the passed value to that bit-offset.
>>>> (...)
>>>>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
>>>>> +{
>>>>> +       unsigned int old, new;
>>>>> +       int ret;
>>>>> +
>>>>> +       if (!__scm)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       spin_lock(&__scm->lock);
>>>>> +       ret = qcom_scm_io_readl(addr, &old);
>>>>> +       if (ret)
>>>>> +               goto unlock;
>>>>> +
>>>>> +       new = (old & ~mask) | (val & mask);
>>>>> +
>>>>> +       ret = qcom_scm_io_writel(addr, new);
>>>>> +unlock:
>>>>> +       spin_unlock(&__scm->lock);
>>>>> +       return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
>>>>
>>>> This looks a lot like you are starting to re-invent regmaps
>>>> regmap_update_bits().
>>>>
>>>> If you are starting to realize you need more and more of
>>>> regmap, why not use regmap and its functions?
>>>
>>> I think, this discussion has happened already ..
>>>
>>> https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/
>>
>> That discussion ended with:
>>
>> [Bjorn]
>>> 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.
>>
>> When you add more and more accessors the premise starts to
>> change, and it becomes more and more of a reimplementation.
>>
>> It may be time to actually fix this.
>>
> 
> Thought I had replied to this already, did we discuss this previously as
> well?
> 
> My concern with expressing this as a regmap is that from the provider's
> point of view, the regmap would span the entire 32-bit address space.
> I'm guessing that there's something on the other side limiting what
> subregions are actually accessible for each platform/firmware
> configuration, but I'm not convinced that regmap a good abstraction...

To add more to it, in current series, we are just accessing single 
register for read/write and using regmap for this looks overkill to
me.

-Mukesh
> 
> Regards,
> Bjorn
> 
>> Yours,
>> Linus Walleij