First two changes changes are needed to enable download mode on
chipset like IPQ9574 and IPQ5332 SoCs as for these targets others
bits in download mode registers are used for different purpose
and earlier kernel code was mistakenly overwrite the other bits.
First three changes(1/4-3/4) are SCM driver specific while 4/4 from
pinctrl try to use the introduced API(1/3).
Changes from v9: https://lore.kernel.org/lkml/[email protected]/
- Added 3/4 new patch.
- commit subject modification.
Change from v8: https://lore.kernel.org/lkml/[email protected]/
- Introduce enum for dload mode constants as per suggestion from [Elliot].
- Rebased on linux-next.
Changes from v7: https://lore.kernel.org/lkml/[email protected]/
- Rebased it on next-20231025.
- Added reviewed-by tag and take care of comment made about
commit text should be in imperative mode.
- Modified the name of the API to qcom_scm_io_rmw() as per suggestion
made by [Dmitry]
- Moved spinlock inside qcom_scm structure.
- Corrected the patch order as per subsystem SCM first then pinctrl.
Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/[email protected]/ - Removed mistakenly added macros.
https://lore.kernel.org/lkml/[email protected]/
- Added Acked-by tag from Linus.w to 2/3.
Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/[email protected]/
- Removed mistakenly added macros.
https://lore.kernel.org/lkml/[email protected]/
- Added Acked-by tag from Linus.w to 2/3.
Changes in v6: https://lore.kernel.org/lkml/[email protected]/
- Rebased it on latest tag available on linux-next
- Added missed Poovendhan sign-off on 15/17 and tested-by tag from
Kathiravan. Thanks to him for testing and reminding me of missing sign-off.
- Addressed comments made on dload mode patch v6 version
Changes in v5: https://lore.kernel.org/lkml/[email protected]/
- Tried to fix the issue reported by kernel test robot
https://lore.kernel.org/lkml/[email protected]/
- Applied some of the improvement suggested by [Bjorn.andersson]
. Dropped 'both' instead support full,mini or mini,full for setting download
mode to collect both minidump and full dump.
. logging improvement.
Changes in v4: https://lore.kernel.org/lkml/[email protected]/
- val should be shifted within the function [srinivas.kandagatla]
i.e new = (old & ~mask) | (val << ffs(mask) - 1);
- Added Acked-by [linus.walleij] on pinctrl change.
Changes in v3 : https://lore.kernel.org/lkml/[email protected]/
- Removed [1] from the series and sent as a separate patch[2], although this series
should be applied on top [2].
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
- Introduce new exported symbol on suggestion from [srinivas.kandagatla]
- Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c.
- Addressed comment given by [dmitry.baryshkov]
- Converted non-standard Originally-by to Signed-off-by.
Changes in v2: https://lore.kernel.org/lkml/[email protected]/
- Addressed comment made by [bjorn]
- Added download mask.
- Passed download mode as parameter
- Accept human accepatable download mode string.
- enable = !!dload_mode
- Shifted module param callback to somewhere down in
the file so that it no longer need to know the
prototype of qcom_scm_set_download_mode()
- updated commit text.
Mukesh Ojha (4):
firmware: qcom: scm: provide a read-modify-write function
firmware: qcom: scm: Modify only the download bits in TCSR register
firmware: qcom: scm: Rework dload mode availability check
pinctrl: qcom: Use qcom_scm_io_rmw() function
drivers/firmware/qcom/qcom_scm.c | 50 ++++++++++++++++++++++++++++------
drivers/pinctrl/qcom/pinctrl-msm.c | 10 +++----
include/linux/firmware/qcom/qcom_scm.h | 1 +
3 files changed, 47 insertions(+), 14 deletions(-)
--
2.7.4
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 <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
Tested-by: Kathiravan Thirumoorthy <[email protected]> # 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
Crashdump collection is done based on DLOAD bits of TCSR register.
To retain other bits, scm driver need to read the register and
modify only the DLOAD bits, as other bits in TCSR may have their
own significance.
Co-developed-by: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Poovendhan Selvaraj <[email protected]>
Signed-off-by: Mukesh Ojha <[email protected]>
Tested-by: Kathiravan Thirumoorthy <[email protected]> # IPQ9574 and IPQ5332
Reviewed-by: Dmitry Baryshkov <[email protected]>
---
drivers/firmware/qcom/qcom_scm.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 25549178a30f..4421f219fe9a 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -4,6 +4,8 @@
*/
#include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
#include <linux/clk.h>
#include <linux/completion.h>
#include <linux/cpumask.h>
@@ -117,6 +119,12 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE BIT(0)
#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL BIT(1)
+#define QCOM_DLOAD_MASK GENMASK(5, 4)
+enum qcom_dload_mode {
+ QCOM_DLOAD_NODUMP = 0,
+ QCOM_DLOAD_FULLDUMP = 1,
+};
+
static const char * const qcom_scm_convention_names[] = {
[SMC_CONVENTION_UNKNOWN] = "unknown",
[SMC_CONVENTION_ARM_32] = "smc arm 32",
@@ -523,6 +531,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
static void qcom_scm_set_download_mode(bool enable)
{
+ u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
bool avail;
int ret = 0;
@@ -532,8 +541,9 @@ static void qcom_scm_set_download_mode(bool enable)
if (avail) {
ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else if (__scm->dload_mode_addr) {
- ret = qcom_scm_io_writel(__scm->dload_mode_addr,
- enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+ ret = qcom_scm_io_rmw(__scm->dload_mode_addr,
+ QCOM_DLOAD_MASK,
+ FIELD_PREP(QCOM_DLOAD_MASK, val));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
--
2.7.4
QCOM_SCM_BOOT_SET_DLOAD_MODE was only valid for very older
target and firmware and for recent targets there is dload
mode tcsr registers is available to set the download mode.
So, it is better to keep it as fallback check instead of
checking its availability and failing it always.
Signed-off-by: Mukesh Ojha <[email protected]>
---
drivers/firmware/qcom/qcom_scm.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 4421f219fe9a..87bcd5c02f2b 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -532,18 +532,16 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
static void qcom_scm_set_download_mode(bool enable)
{
u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
- bool avail;
int ret = 0;
- avail = __qcom_scm_is_call_available(__scm->dev,
- QCOM_SCM_SVC_BOOT,
- QCOM_SCM_BOOT_SET_DLOAD_MODE);
- if (avail) {
- ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
- } else if (__scm->dload_mode_addr) {
+ if (__scm->dload_mode_addr) {
ret = qcom_scm_io_rmw(__scm->dload_mode_addr,
QCOM_DLOAD_MASK,
FIELD_PREP(QCOM_DLOAD_MASK, val));
+ } else if (__qcom_scm_is_call_available(__scm->dev,
+ QCOM_SCM_SVC_BOOT,
+ QCOM_SCM_BOOT_SET_DLOAD_MODE)) {
+ ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
--
2.7.4
Use qcom_scm_io_rmw() exported function in pinctrl-msm
driver.
Signed-off-by: Mukesh Ojha <[email protected]>
Acked-by: Linus Walleij <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 395040346d0f..9323b916cd7c 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1078,22 +1078,20 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
if (g->intr_target_width)
intr_target_mask = GENMASK(g->intr_target_width - 1, 0);
+ intr_target_mask <<= g->intr_target_bit;
if (pctrl->intr_target_use_scm) {
u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
int ret;
- qcom_scm_io_readl(addr, &val);
- val &= ~(intr_target_mask << g->intr_target_bit);
- val |= g->intr_target_kpss_val << g->intr_target_bit;
-
- ret = qcom_scm_io_writel(addr, val);
+ val = g->intr_target_kpss_val << g->intr_target_bit;
+ ret = qcom_scm_io_rmw(addr, intr_target_mask, val);
if (ret)
dev_err(pctrl->dev,
"Failed routing %lu interrupt to Apps proc",
d->hwirq);
} else {
val = msm_readl_intr_target(pctrl, g);
- val &= ~(intr_target_mask << g->intr_target_bit);
+ val &= ~intr_target_mask;
val |= g->intr_target_kpss_val << g->intr_target_bit;
msm_writel_intr_target(val, pctrl, g);
}
--
2.7.4
Bjorn,
Any comments on these patches?
-Mukesh
On 11/2/2023 10:44 PM, Mukesh Ojha wrote:
> First two changes changes are needed to enable download mode on
> chipset like IPQ9574 and IPQ5332 SoCs as for these targets others
> bits in download mode registers are used for different purpose
> and earlier kernel code was mistakenly overwrite the other bits.
>
> First three changes(1/4-3/4) are SCM driver specific while 4/4 from
> pinctrl try to use the introduced API(1/3).
>
> Changes from v9: https://lore.kernel.org/lkml/[email protected]/
> - Added 3/4 new patch.
> - commit subject modification.
>
> Change from v8: https://lore.kernel.org/lkml/[email protected]/
> - Introduce enum for dload mode constants as per suggestion from [Elliot].
> - Rebased on linux-next.
>
> Changes from v7: https://lore.kernel.org/lkml/[email protected]/
> - Rebased it on next-20231025.
> - Added reviewed-by tag and take care of comment made about
> commit text should be in imperative mode.
> - Modified the name of the API to qcom_scm_io_rmw() as per suggestion
> made by [Dmitry]
> - Moved spinlock inside qcom_scm structure.
> - Corrected the patch order as per subsystem SCM first then pinctrl.
>
> Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/[email protected]/ - Removed mistakenly added macros.
> https://lore.kernel.org/lkml/[email protected]/
> - Added Acked-by tag from Linus.w to 2/3.
> Change from minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/[email protected]/
> - Removed mistakenly added macros.
> https://lore.kernel.org/lkml/[email protected]/
> - Added Acked-by tag from Linus.w to 2/3.
>
> Changes in v6: https://lore.kernel.org/lkml/[email protected]/
> - Rebased it on latest tag available on linux-next
> - Added missed Poovendhan sign-off on 15/17 and tested-by tag from
> Kathiravan. Thanks to him for testing and reminding me of missing sign-off.
> - Addressed comments made on dload mode patch v6 version
>
> Changes in v5: https://lore.kernel.org/lkml/[email protected]/
> - Tried to fix the issue reported by kernel test robot
> https://lore.kernel.org/lkml/[email protected]/
>
> - Applied some of the improvement suggested by [Bjorn.andersson]
>
> . Dropped 'both' instead support full,mini or mini,full for setting download
> mode to collect both minidump and full dump.
>
> . logging improvement.
>
> Changes in v4: https://lore.kernel.org/lkml/[email protected]/
> - val should be shifted within the function [srinivas.kandagatla]
> i.e new = (old & ~mask) | (val << ffs(mask) - 1);
> - Added Acked-by [linus.walleij] on pinctrl change.
>
> Changes in v3 : https://lore.kernel.org/lkml/[email protected]/
> - Removed [1] from the series and sent as a separate patch[2], although this series
> should be applied on top [2].
> [1] https://lore.kernel.org/lkml/[email protected]/
> [2] https://lore.kernel.org/lkml/[email protected]/
> - Introduce new exported symbol on suggestion from [srinivas.kandagatla]
> - Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c.
> - Addressed comment given by [dmitry.baryshkov]
> - Converted non-standard Originally-by to Signed-off-by.
>
> Changes in v2: https://lore.kernel.org/lkml/[email protected]/
> - Addressed comment made by [bjorn]
> - Added download mask.
> - Passed download mode as parameter
> - Accept human accepatable download mode string.
> - enable = !!dload_mode
> - Shifted module param callback to somewhere down in
> the file so that it no longer need to know the
> prototype of qcom_scm_set_download_mode()
> - updated commit text.
>
>
> Mukesh Ojha (4):
> firmware: qcom: scm: provide a read-modify-write function
> firmware: qcom: scm: Modify only the download bits in TCSR register
> firmware: qcom: scm: Rework dload mode availability check
> pinctrl: qcom: Use qcom_scm_io_rmw() function
>
> drivers/firmware/qcom/qcom_scm.c | 50 ++++++++++++++++++++++++++++------
> drivers/pinctrl/qcom/pinctrl-msm.c | 10 +++----
> include/linux/firmware/qcom/qcom_scm.h | 1 +
> 3 files changed, 47 insertions(+), 14 deletions(-)
>
On 11/28/2023 1:20 AM, Mukesh Ojha wrote:
> Bjorn,
>
> Any comments on these patches?
Just a reminder., in case it got missed.
-Mukesh
>
> -Mukesh
>
> On 11/2/2023 10:44 PM, Mukesh Ojha wrote:
>> First two changes changes are needed to enable download mode on
>> chipset like IPQ9574 and IPQ5332 SoCs as for these targets others
>> bits in download mode registers are used for different purpose
>> and earlier kernel code was mistakenly overwrite the other bits.
>>
>> First three changes(1/4-3/4) are SCM driver specific while 4/4 from
>> pinctrl try to use the introduced API(1/3).
>>
>> Changes from v9:
>> https://lore.kernel.org/lkml/[email protected]/
>> - Added 3/4 new patch.
>> - commit subject modification.
>>
>> Change from v8:
>> https://lore.kernel.org/lkml/[email protected]/
>> - Introduce enum for dload mode constants as per suggestion from
>> [Elliot].
>> - Rebased on linux-next.
>>
>> Changes from v7:
>> https://lore.kernel.org/lkml/[email protected]/
>> - Rebased it on next-20231025.
>> - Added reviewed-by tag and take care of comment made about
>> commit text should be in imperative mode.
>> - Modified the name of the API to qcom_scm_io_rmw() as per suggestion
>> made by [Dmitry]
>> - Moved spinlock inside qcom_scm structure.
>> - Corrected the patch order as per subsystem SCM first then pinctrl.
>>
>> Change from
>> minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/[email protected]/ - Removed mistakenly added macros.
>>
>> https://lore.kernel.org/lkml/[email protected]/
>> - Added Acked-by tag from Linus.w to 2/3.
>> Change from
>> minidump-v5(13/17-15/17):https://lore.kernel.org/lkml/[email protected]/
>> - Removed mistakenly added macros.
>>
>> https://lore.kernel.org/lkml/[email protected]/
>> - Added Acked-by tag from Linus.w to 2/3.
>>
>> Changes in v6:
>> https://lore.kernel.org/lkml/[email protected]/
>> - Rebased it on latest tag available on linux-next
>> - Added missed Poovendhan sign-off on 15/17 and tested-by tag from
>> Kathiravan. Thanks to him for testing and reminding me of missing
>> sign-off.
>> - Addressed comments made on dload mode patch v6 version
>>
>> Changes in v5:
>> https://lore.kernel.org/lkml/[email protected]/
>> - Tried to fix the issue reported by kernel test robot
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> - Applied some of the improvement suggested by [Bjorn.andersson]
>>
>> . Dropped 'both' instead support full,mini or mini,full for
>> setting download
>> mode to collect both minidump and full dump.
>>
>> . logging improvement.
>>
>> Changes in v4:
>> https://lore.kernel.org/lkml/[email protected]/
>> - val should be shifted within the function [srinivas.kandagatla]
>> i.e new = (old & ~mask) | (val << ffs(mask) - 1);
>> - Added Acked-by [linus.walleij] on pinctrl change.
>>
>> Changes in v3 :
>> https://lore.kernel.org/lkml/[email protected]/
>> - Removed [1] from the series and sent as a separate patch[2],
>> although this series
>> should be applied on top [2].
>> [1]
>> https://lore.kernel.org/lkml/[email protected]/
>> [2]
>> https://lore.kernel.org/lkml/[email protected]/
>> - Introduce new exported symbol on suggestion from
>> [srinivas.kandagatla]
>> - Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c.
>> - Addressed comment given by [dmitry.baryshkov]
>> - Converted non-standard Originally-by to Signed-off-by.
>>
>> Changes in v2:
>> https://lore.kernel.org/lkml/[email protected]/
>> - Addressed comment made by [bjorn]
>> - Added download mask.
>> - Passed download mode as parameter
>> - Accept human accepatable download mode string.
>> - enable = !!dload_mode
>> - Shifted module param callback to somewhere down in
>> the file so that it no longer need to know the
>> prototype of qcom_scm_set_download_mode()
>> - updated commit text.
>>
>>
>> Mukesh Ojha (4):
>> firmware: qcom: scm: provide a read-modify-write function
>> firmware: qcom: scm: Modify only the download bits in TCSR register
>> firmware: qcom: scm: Rework dload mode availability check
>> pinctrl: qcom: Use qcom_scm_io_rmw() function
>>
>> drivers/firmware/qcom/qcom_scm.c | 50
>> ++++++++++++++++++++++++++++------
>> drivers/pinctrl/qcom/pinctrl-msm.c | 10 +++----
>> include/linux/firmware/qcom/qcom_scm.h | 1 +
>> 3 files changed, 47 insertions(+), 14 deletions(-)
>>