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/[email protected]/
[2]
https://lore.kernel.org/linux-arm-msm/[email protected]/
[3]
https://lore.kernel.org/linux-arm-msm/[email protected]/
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
From: Mukesh Ojha <[email protected]>
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 <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
Signed-off-by: Kathiravan T <[email protected]>
---
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
From: Mukesh Ojha <[email protected]>
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 <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
Signed-off-by: Kathiravan T <[email protected]>
---
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
From: Mukesh Ojha <[email protected]>
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 <[email protected]>
Signed-off-by: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
Signed-off-by: Kathiravan T <[email protected]>
---
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
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/[email protected]/
>
> [2]
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> [3]
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
>
> 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(-)
>
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/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230722/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
On 7/20/2023 12:04 AM, Kathiravan T wrote:
> From: Mukesh Ojha <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> Signed-off-by: Kathiravan T <[email protected]>
> ---
> 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);
On Thu, Jul 20, 2023 at 12:34:07PM +0530, Kathiravan T wrote:
> From: Mukesh Ojha <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> Signed-off-by: Kathiravan T <[email protected]>
> ---
> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> Signed-off-by: Kathiravan T <[email protected]>
>> ---
>> 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/[email protected]/
> Regards,
> Bjorn
On 7/22/2023 6:47 AM, Trilok Soni wrote:
> On 7/20/2023 12:04 AM, Kathiravan T wrote:
>> From: Mukesh Ojha <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> Signed-off-by: Kathiravan T <[email protected]>
>> ---
>> 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);
On 7/20/2023 12:04 AM, Kathiravan T wrote:
> From: Mukesh Ojha <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Poovendhan Selvaraj <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> Signed-off-by: Kathiravan T <[email protected]>
> ---
> 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");
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/[email protected]/
>
> [2]
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> [3]
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
>
> 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(-)
>
On 7/20/2023 12:04 AM, Kathiravan T wrote:
> From: Mukesh Ojha <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mukesh Ojha <[email protected]>
> Signed-off-by: Kathiravan T <[email protected]>
After updating EXPORT_SYMBOL -> EXPORT_SYMBOL_GPL:
Reviewed-by: Elliot Berman <[email protected]>
> ---
> 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);
On 7/25/2023 12:35 AM, Elliot Berman wrote:
>
>
> On 7/20/2023 12:04 AM, Kathiravan T wrote:
>> From: Mukesh Ojha <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Poovendhan Selvaraj <[email protected]>
>> Signed-off-by: Mukesh Ojha <[email protected]>
>> Signed-off-by: Kathiravan T <[email protected]>
>> ---
>> 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");
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/[email protected]/
>
>> 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/[email protected]/
>>
>>
>> [2]
>> https://lore.kernel.org/linux-arm-msm/[email protected]/
>>
>>
>> [3]
>> https://lore.kernel.org/linux-arm-msm/[email protected]/
>>
>>
>>
>> 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(-)
>>
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 <[email protected]>
> >>
> >>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 <[email protected]>
> >>Signed-off-by: Mukesh Ojha <[email protected]>
> >>Signed-off-by: Kathiravan T <[email protected]>
> >>---
> >>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/