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

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
>

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
>>

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

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

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

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

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

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

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