[PATCH V5 0/3] Introduce the read-modify-write API to collect

[PATCH V5 0/3] Introduce the read-modify-write API to collect

[Kathiravan T]

On IPQ platforms, to collect the crashdump, we need to just modify the
DLOAD bit in the TCSR register. Current infrastructure, overwrites the
entire regiter value when enabling the crashdump feature, which leads to
crashdump not gets collected. This series introduce the
qcom_scm_io_update_field API to achieve the same.

Intially this approach is posted by Poovendhan[1], later Mukesh
integrated this patch in his minidump support series[2]. Based on the
current feedback on the minidump series, seems it will take sometime to
get into a good shape, in the meantime these patches doesn't have any
dependency with the minidump series. As discussed with the Mukesh[3],
posting these 3 patches to enable the crashdump on IPQ chipsets.

Since the current version of minidump series is V4, I'm posting this as
a V5. Please let me know if this should be V1.

[1]
https://lore.kernel.org/linux-arm-msm/20230113160012.14893-4-quic_poovendh@quicinc.com/

[2]
https://lore.kernel.org/linux-arm-msm/1676990381-18184-3-git-send-email-quic_mojha@quicinc.com/

[3]
https://lore.kernel.org/linux-arm-msm/d77f5601-2b08-a7c7-1400-7ab68b8add3a@quicinc.com/


Mukesh Ojha (3):
  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

 drivers/firmware/qcom_scm.c            | 26 ++++++++++++++++++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.c     | 12 +++++-------
 include/linux/firmware/qcom/qcom_scm.h |  2 ++
 3 files changed, 31 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH V5 1/3] firmware: qcom_scm: provide a read-modify-write function

[Kathiravan T]

From: Mukesh Ojha <quic_mojha@quicinc.com>

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_update_field() 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>
Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
---
Changes in V5:
	- No changes

 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 fde33acd46b7..104d86e49b97 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 & mask);
+
+	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 250ea4efb7cb..ca41e4eb33ad 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.34.1

Re: [PATCH V5 1/3] firmware: qcom_scm: provide a read-modify-write function

[Trilok Soni]

On 7/20/2023 12:04 AM, Kathiravan T wrote:
> From: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> 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_update_field() 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>
> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
> ---
> Changes in V5:
> 	- No changes
> 
>   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 fde33acd46b7..104d86e49b97 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 & mask);
> +
> +	return qcom_scm_io_writel(addr, new);
> +}
> +EXPORT_SYMBOL(qcom_scm_io_update_field);

EXPORT_SYMBO_GPL please.

> +
>   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 250ea4efb7cb..ca41e4eb33ad 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);

Re: [PATCH V5 1/3] firmware: qcom_scm: provide a read-modify-write function

[Kathiravan T]

On 7/22/2023 6:47 AM, Trilok Soni wrote:
> On 7/20/2023 12:04 AM, Kathiravan T wrote:
>> From: Mukesh Ojha <quic_mojha@quicinc.com>
>>
>> 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_update_field() 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>
>> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
>> ---
>> Changes in V5:
>>     - No changes
>>
>>   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 fde33acd46b7..104d86e49b97 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 & mask);
>> +
>> +    return qcom_scm_io_writel(addr, new);
>> +}
>> +EXPORT_SYMBOL(qcom_scm_io_update_field);
>
> EXPORT_SYMBO_GPL please.


Sure, is it okay if I send the patch to convert the existing 
EXPORT_SYMBOL to EXPORT_SYMBOL_GPL as well?


>
>> +
>>   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 250ea4efb7cb..ca41e4eb33ad 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);

Re: [PATCH V5 1/3] firmware: qcom_scm: provide a read-modify-write function

[Guru Das Srinagesh]

On Jul 23 2023 19:25, Kathiravan T wrote:
> 
> On 7/22/2023 6:47 AM, Trilok Soni wrote:
> >On 7/20/2023 12:04 AM, Kathiravan T wrote:
> >>From: Mukesh Ojha <quic_mojha@quicinc.com>
> >>
> >>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_update_field() 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>
> >>Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
> >>---
> >>Changes in V5:
> >>    - No changes
> >>
> >>  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 fde33acd46b7..104d86e49b97 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 & mask);
> >>+
> >>+    return qcom_scm_io_writel(addr, new);
> >>+}
> >>+EXPORT_SYMBOL(qcom_scm_io_update_field);
> >
> >EXPORT_SYMBO_GPL please.
> 
> 
> Sure, is it okay if I send the patch to convert the existing EXPORT_SYMBOL
> to EXPORT_SYMBOL_GPL as well?

This is done already [1].

[1] https://lore.kernel.org/lkml/19d9ac0bf79f957574ef9b3b73246ea0113cc0fd.1690503893.git.quic_gurus@quicinc.com/

Re: [PATCH V5 1/3] firmware: qcom_scm: provide a read-modify-write function

[Elliot Berman]

On 7/20/2023 12:04 AM, Kathiravan T wrote:
> From: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> 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_update_field() 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>
> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>


After updating EXPORT_SYMBOL -> EXPORT_SYMBOL_GPL:

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

> ---
> Changes in V5:
> 	- No changes
> 
>   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 fde33acd46b7..104d86e49b97 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 & mask);
> +
> +	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 250ea4efb7cb..ca41e4eb33ad 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);

[PATCH V5 2/3] pinctrl: qcom: Use qcom_scm_io_update_field()

[Kathiravan T]

From: Mukesh Ojha <quic_mojha@quicinc.com>

Use qcom_scm_io_update_field() function introduced in the commit
1f899e6997bb ("firmware: qcom_scm: provide a read-modify-write function").

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
---
Changes in V5:
	- Dropped the ununecessary paranthesis
	- Updated the commit message to indicate the commit ID in which
	  qcom_scm_io_update_field is introduced instead of simply
	  mentioning the "last commit"

 drivers/pinctrl/qcom/pinctrl-msm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 2585ef2b2793..5ecde5bea38b 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1040,6 +1040,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	const struct msm_pingroup *g;
 	unsigned long flags;
 	bool was_enabled;
+	u32 mask;
 	u32 val;
 
 	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
@@ -1074,23 +1075,20 @@ 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 = GENMASK(2, 0) << 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);
+		val = g->intr_target_kpss_val << g->intr_target_bit;
+		ret = qcom_scm_io_update_field(addr, 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 &= ~(7 << g->intr_target_bit);
+		val &= ~mask;
 		val |= g->intr_target_kpss_val << g->intr_target_bit;
 		msm_writel_intr_target(val, pctrl, g);
 	}
-- 
2.34.1

Re: [PATCH V5 2/3] pinctrl: qcom: Use qcom_scm_io_update_field()

[Bjorn Andersson]

On Thu, Jul 20, 2023 at 12:34:07PM +0530, Kathiravan T wrote:
> From: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> Use qcom_scm_io_update_field() function introduced in the commit
> 1f899e6997bb ("firmware: qcom_scm: provide a read-modify-write function").
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
> ---
> Changes in V5:
> 	- Dropped the ununecessary paranthesis
> 	- Updated the commit message to indicate the commit ID in which
> 	  qcom_scm_io_update_field is introduced instead of simply
> 	  mentioning the "last commit"
> 
>  drivers/pinctrl/qcom/pinctrl-msm.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 2585ef2b2793..5ecde5bea38b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -1040,6 +1040,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  	const struct msm_pingroup *g;
>  	unsigned long flags;
>  	bool was_enabled;
> +	u32 mask;
>  	u32 val;
>  
>  	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> @@ -1074,23 +1075,20 @@ 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 = GENMASK(2, 0) << 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);
> +		val = g->intr_target_kpss_val << g->intr_target_bit;
> +		ret = qcom_scm_io_update_field(addr, mask, val);

Be aware when you resubmit that this code has changed. So please base
your changes on linux-next.

Regards,
Bjorn

Re: [PATCH V5 2/3] pinctrl: qcom: Use qcom_scm_io_update_field()

[Kathiravan T]

On 7/22/2023 8:39 AM, Bjorn Andersson wrote:
> On Thu, Jul 20, 2023 at 12:34:07PM +0530, Kathiravan T wrote:
>> From: Mukesh Ojha <quic_mojha@quicinc.com>
>>
>> Use qcom_scm_io_update_field() function introduced in the commit
>> 1f899e6997bb ("firmware: qcom_scm: provide a read-modify-write function").
>>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
>> ---
>> Changes in V5:
>> 	- Dropped the ununecessary paranthesis
>> 	- Updated the commit message to indicate the commit ID in which
>> 	  qcom_scm_io_update_field is introduced instead of simply
>> 	  mentioning the "last commit"
>>
>>   drivers/pinctrl/qcom/pinctrl-msm.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 2585ef2b2793..5ecde5bea38b 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -1040,6 +1040,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>   	const struct msm_pingroup *g;
>>   	unsigned long flags;
>>   	bool was_enabled;
>> +	u32 mask;
>>   	u32 val;
>>   
>>   	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
>> @@ -1074,23 +1075,20 @@ 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 = GENMASK(2, 0) << 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);
>> +		val = g->intr_target_kpss_val << g->intr_target_bit;
>> +		ret = qcom_scm_io_update_field(addr, mask, val);
> Be aware when you resubmit that this code has changed. So please base
> your changes on linux-next.


I applied and tested this change on top of linux-next before sending it. 
I hope you are referring to the Ninad's patch[1] which is not available 
on linux-next yet. I shall wait for couple of days before sending the 
another version or let me resend based on Ninad's patch. Please let me 
know if you are referring something else.

[1] 
https://lore.kernel.org/linux-arm-msm/20230718064246.12429-1-quic_ninanaik@quicinc.com/


> Regards,
> Bjorn

[PATCH V5 3/3] firmware: scm: Modify only the download bits in TCSR register

[Kathiravan T]

From: Mukesh Ojha <quic_mojha@quicinc.com>

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.

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>
Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
---
Changes in V5:
	- Added the Signed-off-by tag for user Poovendhan
	- Dropped the macro QCOM_DOWNLOAD_MODE_SHIFT in the favor of
	  PREP_FIELD

 drivers/firmware/qcom_scm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 104d86e49b97..3830dcf14326 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,10 @@ module_param(download_mode, bool, 0);
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
 
+#define QCOM_DOWNLOAD_FULLDUMP		0x1
+#define QCOM_DOWNLOAD_NODUMP		0x0
+#define QCOM_DOWNLOAD_MODE_MASK		BIT(4)
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -440,6 +444,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 static void qcom_scm_set_download_mode(bool enable)
 {
 	bool avail;
+	int val;
 	int ret = 0;
 
 	avail = __qcom_scm_is_call_available(__scm->dev,
@@ -448,8 +453,10 @@ 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);
+		val = enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP;
+		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+					       QCOM_DOWNLOAD_MODE_MASK,
+					       FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.34.1

Re: [PATCH V5 3/3] firmware: scm: Modify only the download bits in TCSR register

[kernel test robot]

Hi Kathiravan,

kernel test robot noticed the following build errors:

[auto build test ERROR on linusw-pinctrl/devel]
[also build test ERROR on linusw-pinctrl/for-next linus/master v6.5-rc2 next-20230721]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kathiravan-T/firmware-qcom_scm-provide-a-read-modify-write-function/20230720-150657
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/20230720070408.1093698-4-quic_kathirav%40quicinc.com
patch subject: [PATCH V5 3/3] firmware: scm: Modify only the download bits in TCSR register
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230722/202307220736.M9yPz2Cs-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230722/202307220736.M9yPz2Cs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307220736.M9yPz2Cs-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/qcom_scm.c: In function 'qcom_scm_set_download_mode':
>> drivers/firmware/qcom_scm.c:459:48: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
     459 |                                                FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
         |                                                ^~~~~~~~~~
   cc1: some warnings being treated as errors

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SM_GCC_8350
   Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 || COMPILE_TEST [=n])
   Selected by [m]:
   - SM_VIDEOCC_8350 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
   WARNING: unmet direct dependencies detected for SM_GCC_8450
   Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 || COMPILE_TEST [=n])
   Selected by [m]:
   - SM_GPUCC_8450 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
   - SM_VIDEOCC_8450 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
   WARNING: unmet direct dependencies detected for SM_GCC_8550
   Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 || COMPILE_TEST [=n])
   Selected by [m]:
   - SM_GPUCC_8550 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
   - SM_VIDEOCC_8550 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]


vim +/FIELD_PREP +459 drivers/firmware/qcom_scm.c

   443	
   444	static void qcom_scm_set_download_mode(bool enable)
   445	{
   446		bool avail;
   447		int val;
   448		int ret = 0;
   449	
   450		avail = __qcom_scm_is_call_available(__scm->dev,
   451						     QCOM_SCM_SVC_BOOT,
   452						     QCOM_SCM_BOOT_SET_DLOAD_MODE);
   453		if (avail) {
   454			ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
   455		} else if (__scm->dload_mode_addr) {
   456			val = enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP;
   457			ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
   458						       QCOM_DOWNLOAD_MODE_MASK,
 > 459						       FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
   460		} else {
   461			dev_err(__scm->dev,
   462				"No available mechanism for setting download mode\n");
   463		}
   464	
   465		if (ret)
   466			dev_err(__scm->dev, "failed to set download mode: %d\n", ret);
   467	}
   468	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V5 3/3] firmware: scm: Modify only the download bits in TCSR register

[Elliot Berman]

On 7/20/2023 12:04 AM, Kathiravan T wrote:
> From: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> 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.
> 
> 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>
> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
> ---
> Changes in V5:
> 	- Added the Signed-off-by tag for user Poovendhan
> 	- Dropped the macro QCOM_DOWNLOAD_MODE_SHIFT in the favor of
> 	  PREP_FIELD
> 
>   drivers/firmware/qcom_scm.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 104d86e49b97..3830dcf14326 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -30,6 +30,10 @@ module_param(download_mode, bool, 0);
>   #define SCM_HAS_IFACE_CLK	BIT(1)
>   #define SCM_HAS_BUS_CLK		BIT(2)
>   
> +#define QCOM_DOWNLOAD_FULLDUMP		0x1
> +#define QCOM_DOWNLOAD_NODUMP		0x0
> +#define QCOM_DOWNLOAD_MODE_MASK		BIT(4)
> +

Can you update __qcom_scm_set_dload_mode to use the FIELD_PREP bits as 
well? Ideally, you should be able to have no duplicate logic in 
__qcom_scm_set_dload_mode and in qcom_scm_set_download_mode. Before your 
patch, it was duplicated and we probably should've had it de-duplicated. 
With this patch, the logic and constants used have diverged when they 
don't need to.

>   struct qcom_scm {
>   	struct device *dev;
>   	struct clk *core_clk;
> @@ -440,6 +444,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>   static void qcom_scm_set_download_mode(bool enable)
>   {
>   	bool avail;
> +	int val;
>   	int ret = 0;
>   
>   	avail = __qcom_scm_is_call_available(__scm->dev,
> @@ -448,8 +453,10 @@ 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);
> +		val = enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP;
> +		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> +					       QCOM_DOWNLOAD_MODE_MASK,
> +					       FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
>   	} else {
>   		dev_err(__scm->dev,
>   			"No available mechanism for setting download mode\n");

Re: [PATCH V5 3/3] firmware: scm: Modify only the download bits in TCSR register

[Kathiravan T]

On 7/25/2023 12:35 AM, Elliot Berman wrote:
>
>
> On 7/20/2023 12:04 AM, Kathiravan T wrote:
>> From: Mukesh Ojha <quic_mojha@quicinc.com>
>>
>> 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.
>>
>> 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>
>> Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
>> ---
>> Changes in V5:
>>     - Added the Signed-off-by tag for user Poovendhan
>>     - Dropped the macro QCOM_DOWNLOAD_MODE_SHIFT in the favor of
>>       PREP_FIELD
>>
>>   drivers/firmware/qcom_scm.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 104d86e49b97..3830dcf14326 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -30,6 +30,10 @@ module_param(download_mode, bool, 0);
>>   #define SCM_HAS_IFACE_CLK    BIT(1)
>>   #define SCM_HAS_BUS_CLK        BIT(2)
>>   +#define QCOM_DOWNLOAD_FULLDUMP        0x1
>> +#define QCOM_DOWNLOAD_NODUMP        0x0
>> +#define QCOM_DOWNLOAD_MODE_MASK        BIT(4)
>> +
>
> Can you update __qcom_scm_set_dload_mode to use the FIELD_PREP bits as 
> well? Ideally, you should be able to have no duplicate logic in 
> __qcom_scm_set_dload_mode and in qcom_scm_set_download_mode. Before 
> your patch, it was duplicated and we probably should've had it 
> de-duplicated. With this patch, the logic and constants used have 
> diverged when they don't need to.


Sure, will check this.


>
>>   struct qcom_scm {
>>       struct device *dev;
>>       struct clk *core_clk;
>> @@ -440,6 +444,7 @@ static int __qcom_scm_set_dload_mode(struct 
>> device *dev, bool enable)
>>   static void qcom_scm_set_download_mode(bool enable)
>>   {
>>       bool avail;
>> +    int val;
>>       int ret = 0;
>>         avail = __qcom_scm_is_call_available(__scm->dev,
>> @@ -448,8 +453,10 @@ 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);
>> +        val = enable ? QCOM_DOWNLOAD_FULLDUMP : QCOM_DOWNLOAD_NODUMP;
>> +        ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
>> +                           QCOM_DOWNLOAD_MODE_MASK,
>> +                           FIELD_PREP(QCOM_DOWNLOAD_MODE_MASK, val));
>>       } else {
>>           dev_err(__scm->dev,
>>               "No available mechanism for setting download mode\n");

Re: [PATCH V5 0/3] Introduce the read-modify-write API to collect

[Kathiravan T]

On 7/20/2023 12:34 PM, Kathiravan T wrote:


$subject is messed up, it should be "Introduce the read-modify-write API 
to collect the crashdump on IPQ chipsets". Will correct it in next spin 
or let me know if I need to respin.


> On IPQ platforms, to collect the crashdump, we need to just modify the
> DLOAD bit in the TCSR register. Current infrastructure, overwrites the
> entire regiter value when enabling the crashdump feature, which leads to
> crashdump not gets collected. This series introduce the
> qcom_scm_io_update_field API to achieve the same.
>
> Intially this approach is posted by Poovendhan[1], later Mukesh
> integrated this patch in his minidump support series[2]. Based on the
> current feedback on the minidump series, seems it will take sometime to
> get into a good shape, in the meantime these patches doesn't have any
> dependency with the minidump series. As discussed with the Mukesh[3],
> posting these 3 patches to enable the crashdump on IPQ chipsets.
>
> Since the current version of minidump series is V4, I'm posting this as
> a V5. Please let me know if this should be V1.
>
> [1]
> https://lore.kernel.org/linux-arm-msm/20230113160012.14893-4-quic_poovendh@quicinc.com/
>
> [2]
> https://lore.kernel.org/linux-arm-msm/1676990381-18184-3-git-send-email-quic_mojha@quicinc.com/
>
> [3]
> https://lore.kernel.org/linux-arm-msm/d77f5601-2b08-a7c7-1400-7ab68b8add3a@quicinc.com/
>
>
> Mukesh Ojha (3):
>    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
>
>   drivers/firmware/qcom_scm.c            | 26 ++++++++++++++++++++++++--
>   drivers/pinctrl/qcom/pinctrl-msm.c     | 12 +++++-------
>   include/linux/firmware/qcom/qcom_scm.h |  2 ++
>   3 files changed, 31 insertions(+), 9 deletions(-)
>

Re: [PATCH V5 0/3] Introduce the read-modify-write API to collect

[Elliot Berman]

On 7/20/2023 12:04 AM, Kathiravan T wrote:
> On IPQ platforms, to collect the crashdump, we need to just modify the
> DLOAD bit in the TCSR register. Current infrastructure, overwrites the
> entire regiter value when enabling the crashdump feature, which leads to
> crashdump not gets collected. This series introduce the
> qcom_scm_io_update_field API to achieve the same.
> 

I don't think you describe patch 2 in the subject line or cover letter. 
As best I can tell, Patches 2 and 3 are independent. They're similar 
only in that they both depend on patch 1.

> Intially this approach is posted by Poovendhan[1], later Mukesh
> integrated this patch in his minidump support series[2]. Based on the
> current feedback on the minidump series, seems it will take sometime to
> get into a good shape, in the meantime these patches doesn't have any
> dependency with the minidump series. As discussed with the Mukesh[3],
> posting these 3 patches to enable the crashdump on IPQ chipsets.
> 
> Since the current version of minidump series is V4, I'm posting this as
> a V5. Please let me know if this should be V1.
> 
> [1]
> https://lore.kernel.org/linux-arm-msm/20230113160012.14893-4-quic_poovendh@quicinc.com/
> 
> [2]
> https://lore.kernel.org/linux-arm-msm/1676990381-18184-3-git-send-email-quic_mojha@quicinc.com/
> 
> [3]
> https://lore.kernel.org/linux-arm-msm/d77f5601-2b08-a7c7-1400-7ab68b8add3a@quicinc.com/
> 
> 
> Mukesh Ojha (3):
>    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
> 
>   drivers/firmware/qcom_scm.c            | 26 ++++++++++++++++++++++++--
>   drivers/pinctrl/qcom/pinctrl-msm.c     | 12 +++++-------
>   include/linux/firmware/qcom/qcom_scm.h |  2 ++
>   3 files changed, 31 insertions(+), 9 deletions(-)
> 

Re: [PATCH V5 0/3] Introduce the read-modify-write API to collect

[Kathiravan T]

On 7/25/2023 12:35 AM, Elliot Berman wrote:
>
>
> On 7/20/2023 12:04 AM, Kathiravan T wrote:
>> On IPQ platforms, to collect the crashdump, we need to just modify the
>> DLOAD bit in the TCSR register. Current infrastructure, overwrites the
>> entire regiter value when enabling the crashdump feature, which leads to
>> crashdump not gets collected. This series introduce the
>> qcom_scm_io_update_field API to achieve the same.
>>
>
> I don't think you describe patch 2 in the subject line or cover 
> letter. As best I can tell, Patches 2 and 3 are independent. They're 
> similar only in that they both depend on patch 1.


Yeah. I missed that part. I'm thinking of dropping the 2nd patch and 
send only the patch 1 and patch 3 in the next spin. Once the patch 1 and 
the another pinctrl patch which Bjorn's is referring [1] (Hopefully, If 
I am not wrong) is landed in linux-next, I can send out the patch 2 
separately. Do let me know if this okay.

[1] 
https://lore.kernel.org/linux-arm-msm/2d790f7e-b373-f0ee-d978-fb78bc4f1ed1@quicinc.com/


>
>> Intially this approach is posted by Poovendhan[1], later Mukesh
>> integrated this patch in his minidump support series[2]. Based on the
>> current feedback on the minidump series, seems it will take sometime to
>> get into a good shape, in the meantime these patches doesn't have any
>> dependency with the minidump series. As discussed with the Mukesh[3],
>> posting these 3 patches to enable the crashdump on IPQ chipsets.
>>
>> Since the current version of minidump series is V4, I'm posting this as
>> a V5. Please let me know if this should be V1.
>>
>> [1]
>> https://lore.kernel.org/linux-arm-msm/20230113160012.14893-4-quic_poovendh@quicinc.com/ 
>>
>>
>> [2]
>> https://lore.kernel.org/linux-arm-msm/1676990381-18184-3-git-send-email-quic_mojha@quicinc.com/ 
>>
>>
>> [3]
>> https://lore.kernel.org/linux-arm-msm/d77f5601-2b08-a7c7-1400-7ab68b8add3a@quicinc.com/ 
>>
>>
>>
>> Mukesh Ojha (3):
>>    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
>>
>>   drivers/firmware/qcom_scm.c            | 26 ++++++++++++++++++++++++--
>>   drivers/pinctrl/qcom/pinctrl-msm.c     | 12 +++++-------
>>   include/linux/firmware/qcom/qcom_scm.h |  2 ++
>>   3 files changed, 31 insertions(+), 9 deletions(-)
>>