Crashdump collection is enabled based on the DLOAD bit in the TCSR register.
This bit is set during bootup and clearing during shutdown. During crash,
dload bit is not cleared, due to which uboot starts crashdump collection.
Enable the support for download mode to collect the crashdumps if
system crashes, to debug crashes extensively.
During the bootup, bootloaders initialize the SMEM. However the bootup
after crash, SMEM will not be initialized again. If the memory for the
SMEM is not reserved, linux consumes that region, which leads to the
loss of SMEM data. So, during the next bootup after crash, bootloaders
will hang due to invalid data present in the SMEM region. Due to this,
added the SMEM support along with crashdump collection series.
This patch series adds the support for crashdump collection.
DTS patch depends on the IPQ9574 baseport series
https://lore.kernel.org/linux-arm-kernel/[email protected]/
V5:
- change logs are added to the respective patches.
V4 can be found at
https://lore.kernel.org/linux-arm-kernel/[email protected]/
V3 can be found at
https://lore.kernel.org/linux-arm-msm/[email protected]/
Changes in V2:
- rebased on linux-next/master
- dropped co-developed by tag wherever applicable
- V1 can be found at
https://lore.kernel.org/linux-arm-kernel/[email protected]/
Poovendhan Selvaraj (5):
dt-bindings: scm: Add compatible for IPQ9574
dt-bindings: mfd: Add the tcsr compatible for IPQ9574
arm64: dts: qcom: ipq9574: Enable the download mode support
arm64: dts: qcom: ipq9574: Add SMEM support
firmware: scm: Modify only the DLOAD bit in TCSR register for download
mode
.../bindings/firmware/qcom,scm.yaml | 1 +
.../devicetree/bindings/mfd/qcom,tcsr.yaml | 1 +
arch/arm64/boot/dts/qcom/ipq9574.dtsi | 25 +++++++++++++++++++
drivers/firmware/qcom_scm.c | 21 ++++++++++++----
4 files changed, 43 insertions(+), 5 deletions(-)
base-commit: 509583475828c4fd86897113f78315c1431edcc3
--
2.17.1
Add the scm compatible string for IPQ9574 SoC
Acked-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Poovendhan Selvaraj <[email protected]>
---
Changes in V5:
- Added the Acked by tag
Changes in V4:
- No changes
Changes in V3:
- No changes
Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index a66e99812b1f..868fc7e555a6 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -27,6 +27,7 @@ properties:
- qcom,scm-ipq6018
- qcom,scm-ipq806x
- qcom,scm-ipq8074
+ - qcom,scm-ipq9574
- qcom,scm-mdm9607
- qcom,scm-msm8226
- qcom,scm-msm8660
--
2.17.1
Add the tcsr compatible string for IPQ9574 SoC
Acked-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Poovendhan Selvaraj <[email protected]>
---
Changes in V5:
- No changes
Changes in V4:
- No changes
Changes in V3:
- Updated the subject
- Added the Acked by tag
Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml b/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
index 2eeebe920e6e..203fc890c9a3 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
@@ -31,6 +31,7 @@ properties:
- qcom,tcsr-ipq5332
- qcom,tcsr-ipq6018
- qcom,tcsr-ipq8064
+ - qcom,tcsr-ipq9574
- qcom,tcsr-mdm9615
- qcom,tcsr-msm8226
- qcom,tcsr-msm8660
--
2.17.1
Enable the support for download mode to collect the crashdumps if
system crashes, to debug crashes extensively.
Signed-off-by: Poovendhan Selvaraj <[email protected]>
---
Changes in V5:
- No changes
Changes in V4:
- Dropped smem related nodes from this patch
- Mapped the entire TCSR_REGS region and updated the offset in
qcom,dload-mode property
- updated the commit message
Changes in V3:
- No changes
arch/arm64/boot/dts/qcom/ipq9574.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 3bb7435f5e7f..099948f36efc 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -81,6 +81,13 @@
reg = <0x0 0x40000000 0x0 0x0>;
};
+ firmware {
+ scm {
+ compatible = "qcom,scm-ipq9574", "qcom,scm";
+ qcom,dload-mode = <&tcsr 0x6100>;
+ };
+ };
+
pmu {
compatible = "arm,cortex-a73-pmu";
interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
@@ -142,6 +149,11 @@
#power-domain-cells = <1>;
};
+ tcsr: syscon@1937000 {
+ compatible = "qcom,tcsr-ipq9574", "syscon";
+ reg = <0x01937000 0x21000>;
+ };
+
sdhc_1: mmc@7804000 {
compatible = "qcom,ipq9574-sdhci", "qcom,sdhci-msm-v5";
reg = <0x07804000 0x1000>, <0x07805000 0x1000>;
--
2.17.1
Add the required nodes to support SMEM
Signed-off-by: Poovendhan Selvaraj <[email protected]>
---
Changes in V5:
- Dropped unrelated changes
Changes in V4:
- Added required nodes for smem support
arch/arm64/boot/dts/qcom/ipq9574.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 099948f36efc..14a3396b0381 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -107,6 +107,13 @@
reg = <0x0 0x4a600000 0x0 0x400000>;
no-map;
};
+
+ smem@4aa00000 {
+ compatible = "qcom,smem";
+ reg = <0x0 0x4aa00000 0x0 0x00100000>;
+ hwlocks = <&tcsr_mutex 0>;
+ no-map;
+ };
};
soc: soc@0 {
@@ -149,6 +156,12 @@
#power-domain-cells = <1>;
};
+ tcsr_mutex: hwlock@1905000 {
+ compatible = "qcom,tcsr-mutex";
+ reg = <0x01905000 0x20000>;
+ #hwlock-cells = <1>;
+ };
+
tcsr: syscon@1937000 {
compatible = "qcom,tcsr-ipq9574", "syscon";
reg = <0x01937000 0x21000>;
--
2.17.1
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: Anusha Rao <[email protected]>
Signed-off-by: Anusha Rao <[email protected]>
Co-developed-by: Kathiravan Thirumoorthy <[email protected]>
Signed-off-by: Kathiravan Thirumoorthy <[email protected]>
Signed-off-by: Poovendhan Selvaraj <[email protected]>
---
Changes in V5:
- checking the return value in qcom_scm_set_download_mode function as
suggested by Srinivas Kandagatla
Changes in V4:
- retain the orginal value of tcsr register when download mode
is not set
drivers/firmware/qcom_scm.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 468d4d5ab550..d88c5f14bd54 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -407,7 +407,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
}
EXPORT_SYMBOL(qcom_scm_set_remote_state);
-static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
+static int __qcom_scm_set_dload_mode(struct device *dev, u32 val, bool enable)
{
struct qcom_scm_desc desc = {
.svc = QCOM_SCM_SVC_BOOT,
@@ -417,7 +417,8 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
.owner = ARM_SMCCC_OWNER_SIP,
};
- desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
+ desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
+ val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
}
@@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool enable)
{
bool avail;
int ret = 0;
+ u32 dload_addr_val;
avail = __qcom_scm_is_call_available(__scm->dev,
QCOM_SCM_SVC_BOOT,
QCOM_SCM_BOOT_SET_DLOAD_MODE);
+ ret = qcom_scm_io_readl(__scm->dload_mode_addr, &dload_addr_val);
+
+ if (ret) {
+ dev_err(__scm->dev,
+ "failed to read dload mode address value: %d\n", ret);
+ return;
+ }
+
if (avail) {
- ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
+ ret = __qcom_scm_set_dload_mode(__scm->dev, dload_addr_val, 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_writel(__scm->dload_mode_addr, enable ?
+ dload_addr_val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
+ dload_addr_val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
--
2.17.1
On 2/16/2023 5:30 PM, Poovendhan Selvaraj wrote:
> 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: Anusha Rao <[email protected]>
> Signed-off-by: Anusha Rao <[email protected]>
> Co-developed-by: Kathiravan Thirumoorthy <[email protected]>
> Signed-off-by: Kathiravan Thirumoorthy <[email protected]>
> Signed-off-by: Poovendhan Selvaraj <[email protected]>
> ---
> Changes in V5:
> - checking the return value in qcom_scm_set_download_mode function as
> suggested by Srinivas Kandagatla
>
> Changes in V4:
> - retain the orginal value of tcsr register when download mode
> is not set
>
> drivers/firmware/qcom_scm.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 468d4d5ab550..d88c5f14bd54 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -407,7 +407,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> }
> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>
> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> +static int __qcom_scm_set_dload_mode(struct device *dev, u32 val, bool enable)
> {
> struct qcom_scm_desc desc = {
> .svc = QCOM_SCM_SVC_BOOT,
> @@ -417,7 +417,8 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> .owner = ARM_SMCCC_OWNER_SIP,
> };
>
> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> + desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
> + val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
>
> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> }
> @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool enable)
> {
> bool avail;
> int ret = 0;
> + u32 dload_addr_val;
>
> avail = __qcom_scm_is_call_available(__scm->dev,
> QCOM_SCM_SVC_BOOT,
> QCOM_SCM_BOOT_SET_DLOAD_MODE);
> + ret = qcom_scm_io_readl(__scm->dload_mode_addr, &dload_addr_val);
> +
> + if (ret) {
> + dev_err(__scm->dev,
> + "failed to read dload mode address value: %d\n", ret);
> + return;
> + }
> +
> if (avail) {
> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> + ret = __qcom_scm_set_dload_mode(__scm->dev, dload_addr_val, enable);
Did you test this on a target where it comes under this if statement?
does it really need to know dload_mode_addr for this target ?
-Mukesh
> } 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_writel(__scm->dload_mode_addr, enable ?
> + dload_addr_val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
> + dload_addr_val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download mode\n");
On 2/16/2023 7:30 PM, Mukesh Ojha wrote:
>
>
> On 2/16/2023 5:30 PM, Poovendhan Selvaraj wrote:
>> 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: Anusha Rao <[email protected]>
>> Signed-off-by: Anusha Rao <[email protected]>
>> Co-developed-by: Kathiravan Thirumoorthy <[email protected]>
>> Signed-off-by: Kathiravan Thirumoorthy <[email protected]>
>> Signed-off-by: Poovendhan Selvaraj <[email protected]>
>> ---
>> Changes in V5:
>> - checking the return value in qcom_scm_set_download_mode function as
>> suggested by Srinivas Kandagatla
>>
>> Changes in V4:
>> - retain the orginal value of tcsr register when download mode
>> is not set
>>
>> drivers/firmware/qcom_scm.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 468d4d5ab550..d88c5f14bd54 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -407,7 +407,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>> }
>> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>> +static int __qcom_scm_set_dload_mode(struct device *dev, u32 val,
>> bool enable)
>> {
>> struct qcom_scm_desc desc = {
>> .svc = QCOM_SCM_SVC_BOOT,
>> @@ -417,7 +417,8 @@ static int __qcom_scm_set_dload_mode(struct device
>> *dev, bool enable)
>> .owner = ARM_SMCCC_OWNER_SIP,
>> };
>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>> + desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
>> + val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>> }
>> @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool enable)
>> {
>> bool avail;
>> int ret = 0;
>> + u32 dload_addr_val;
>> avail = __qcom_scm_is_call_available(__scm->dev,
>> QCOM_SCM_SVC_BOOT,
>> QCOM_SCM_BOOT_SET_DLOAD_MODE);
>> + ret = qcom_scm_io_readl(__scm->dload_mode_addr, &dload_addr_val);
>> +
>> + if (ret) {
>> + dev_err(__scm->dev,
>> + "failed to read dload mode address value: %d\n", ret);
>> + return;
>> + }
>> +
>> if (avail) {
>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>> + ret = __qcom_scm_set_dload_mode(__scm->dev, dload_addr_val,
>> enable);
>
> Did you test this on a target where it comes under this if statement?
> does it really need to know dload_mode_addr for this target ?
Can we do something like this? I would let other review as well.
--------------------------------------->0-------------------------------------------
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54..26b7eda 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -419,6 +419,7 @@ static void qcom_scm_set_download_mode(bool enable)
{
bool avail;
int ret = 0;
+ u32 dload_addr_val;
avail = __qcom_scm_is_call_available(__scm->dev,
QCOM_SCM_SVC_BOOT,
@@ -426,8 +427,16 @@ 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_readl(__scm->dload_mode_addr,
&dload_addr_val);
+ if (ret) {
+ dev_err(__scm->dev,
+ "failed to read dload mode address
value: %d\n", ret);
+ return;
+ }
+
+ ret = qcom_scm_io_writel(__scm->dload_mode_addr, enable ?
+ dload_addr_val |
QCOM_SCM_BOOT_SET_DLOAD_MODE :
+ dload_addr_val &
~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download
mode\n");
-Mukesh
>
> -Mukesh
>> } 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_writel(__scm->dload_mode_addr, enable ?
>> + dload_addr_val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
>> + dload_addr_val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
>> } else {
>> dev_err(__scm->dev,
>> "No available mechanism for setting download mode\n");
On 2/18/2023 1:19 AM, Mukesh Ojha wrote:
>
>
> On 2/16/2023 7:30 PM, Mukesh Ojha wrote:
>>
>>
>> On 2/16/2023 5:30 PM, Poovendhan Selvaraj wrote:
>>> 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: Anusha Rao <[email protected]>
>>> Signed-off-by: Anusha Rao <[email protected]>
>>> Co-developed-by: Kathiravan Thirumoorthy <[email protected]>
>>> Signed-off-by: Kathiravan Thirumoorthy <[email protected]>
>>> Signed-off-by: Poovendhan Selvaraj <[email protected]>
>>> ---
>>> Changes in V5:
>>> - checking the return value in qcom_scm_set_download_mode
>>> function as
>>> suggested by Srinivas Kandagatla
>>>
>>> Changes in V4:
>>> - retain the orginal value of tcsr register when download mode
>>> is not set
>>>
>>> drivers/firmware/qcom_scm.c | 21 ++++++++++++++++-----
>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>> index 468d4d5ab550..d88c5f14bd54 100644
>>> --- a/drivers/firmware/qcom_scm.c
>>> +++ b/drivers/firmware/qcom_scm.c
>>> @@ -407,7 +407,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>>> }
>>> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>>> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>> +static int __qcom_scm_set_dload_mode(struct device *dev, u32 val,
>>> bool enable)
>>> {
>>> struct qcom_scm_desc desc = {
>>> .svc = QCOM_SCM_SVC_BOOT,
>>> @@ -417,7 +417,8 @@ static int __qcom_scm_set_dload_mode(struct
>>> device *dev, bool enable)
>>> .owner = ARM_SMCCC_OWNER_SIP,
>>> };
>>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>>> + desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
>>> + val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>>> }
>>> @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool
>>> enable)
>>> {
>>> bool avail;
>>> int ret = 0;
>>> + u32 dload_addr_val;
>>> avail = __qcom_scm_is_call_available(__scm->dev,
>>> QCOM_SCM_SVC_BOOT,
>>> QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>> + ret = qcom_scm_io_readl(__scm->dload_mode_addr, &dload_addr_val);
>>> +
>>> + if (ret) {
>>> + dev_err(__scm->dev,
>>> + "failed to read dload mode address value: %d\n", ret);
>>> + return;
>>> + }
>>> +
>>> if (avail) {
>>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>>> + ret = __qcom_scm_set_dload_mode(__scm->dev, dload_addr_val,
>>> enable);
>>
>> Did you test this on a target where it comes under this if statement?
>> does it really need to know dload_mode_addr for this target ?
>
>
> Can we do something like this? I would let other review as well.
>
> --------------------------------------->0-------------------------------------------
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index cdbfe54..26b7eda 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -419,6 +419,7 @@ static void qcom_scm_set_download_mode(bool enable)
> {
> bool avail;
> int ret = 0;
> + u32 dload_addr_val;
>
> avail = __qcom_scm_is_call_available(__scm->dev,
> QCOM_SCM_SVC_BOOT,
> @@ -426,8 +427,16 @@ 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_readl(__scm->dload_mode_addr,
> &dload_addr_val);
> + if (ret) {
> + dev_err(__scm->dev,
> + "failed to read dload mode address
> value: %d\n", ret);
> + return;
> + }
> +
> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, enable ?
> + dload_addr_val |
> QCOM_SCM_BOOT_SET_DLOAD_MODE :
> + dload_addr_val &
> ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
> } else {
> dev_err(__scm->dev,
> "No available mechanism for setting download
> mode\n");
>
> -Mukesh
Okay sure..Agreed, will address this in the next patch.
>>
>> -Mukesh
>>> } 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_writel(__scm->dload_mode_addr, enable ?
>>> + dload_addr_val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
>>> + dload_addr_val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
>>> } else {
>>> dev_err(__scm->dev,
>>> "No available mechanism for setting download mode\n");
Best Regards,
Poovendhan S
Hi,
On 2/20/2023 4:00 PM, POOVENDHAN SELVARAJ wrote:
>
> On 2/18/2023 1:19 AM, Mukesh Ojha wrote:
>>
>>
>> On 2/16/2023 7:30 PM, Mukesh Ojha wrote:
>>>
>>>
>>> On 2/16/2023 5:30 PM, Poovendhan Selvaraj wrote:
>>>> 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: Anusha Rao <[email protected]>
>>>> Signed-off-by: Anusha Rao <[email protected]>
>>>> Co-developed-by: Kathiravan Thirumoorthy <[email protected]>
>>>> Signed-off-by: Kathiravan Thirumoorthy <[email protected]>
>>>> Signed-off-by: Poovendhan Selvaraj <[email protected]>
>>>> ---
>>>> Changes in V5:
>>>> - checking the return value in qcom_scm_set_download_mode
>>>> function as
>>>> suggested by Srinivas Kandagatla
>>>>
>>>> Changes in V4:
>>>> - retain the orginal value of tcsr register when download mode
>>>> is not set
>>>>
>>>> drivers/firmware/qcom_scm.c | 21 ++++++++++++++++-----
>>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>>> index 468d4d5ab550..d88c5f14bd54 100644
>>>> --- a/drivers/firmware/qcom_scm.c
>>>> +++ b/drivers/firmware/qcom_scm.c
>>>> @@ -407,7 +407,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>>>> }
>>>> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>>>> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>>> +static int __qcom_scm_set_dload_mode(struct device *dev, u32 val,
>>>> bool enable)
>>>> {
>>>> struct qcom_scm_desc desc = {
>>>> .svc = QCOM_SCM_SVC_BOOT,
>>>> @@ -417,7 +417,8 @@ static int __qcom_scm_set_dload_mode(struct
>>>> device *dev, bool enable)
>>>> .owner = ARM_SMCCC_OWNER_SIP,
>>>> };
>>>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>>>> + desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
>>>> + val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>>>> }
>>>> @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool
>>>> enable)
>>>> {
>>>> bool avail;
>>>> int ret = 0;
>>>> + u32 dload_addr_val;
>>>> avail = __qcom_scm_is_call_available(__scm->dev,
>>>> QCOM_SCM_SVC_BOOT,
>>>> QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>>> + ret = qcom_scm_io_readl(__scm->dload_mode_addr, &dload_addr_val);
>>>> +
>>>> + if (ret) {
>>>> + dev_err(__scm->dev,
>>>> + "failed to read dload mode address value: %d\n", ret);
>>>> + return;
>>>> + }
>>>> +
>>>> if (avail) {
>>>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>>>> + ret = __qcom_scm_set_dload_mode(__scm->dev, dload_addr_val,
>>>> enable);
>>>
>>> Did you test this on a target where it comes under this if statement?
>>> does it really need to know dload_mode_addr for this target ?
>>
>>
>> Can we do something like this? I would let other review as well.
>>
>> --------------------------------------->0-------------------------------------------
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index cdbfe54..26b7eda 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -419,6 +419,7 @@ static void qcom_scm_set_download_mode(bool enable)
>> {
>> bool avail;
>> int ret = 0;
>> + u32 dload_addr_val;
>>
>> avail = __qcom_scm_is_call_available(__scm->dev,
>> QCOM_SCM_SVC_BOOT,
>> @@ -426,8 +427,16 @@ 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_readl(__scm->dload_mode_addr,
>> &dload_addr_val);
>> + if (ret) {
>> + dev_err(__scm->dev,
>> + "failed to read dload mode address
>> value: %d\n", ret);
>> + return;
>> + }
>> +
>> + ret = qcom_scm_io_writel(__scm->dload_mode_addr, enable ?
>> + dload_addr_val |
>> QCOM_SCM_BOOT_SET_DLOAD_MODE :
>> + dload_addr_val &
>> ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
>> } else {
>> dev_err(__scm->dev,
>> "No available mechanism for setting download
>> mode\n");
>>
>> -Mukesh
>
> Okay sure..Agreed, will address this in the next patch.
Also, not sure, if its better to keep the old behavior working for
targets that does not support 'READ' of this address. If one such
thing exists, that will be broken now. In such a case, we should
ignore if scm_io_readl fails, still write and dload_addr_val should
be '0' initialised.
Regards,
Sricharan
On 2/22/2023 12:22 PM, Sricharan Ramabadhran wrote:
> Hi,
>
> On 2/20/2023 4:00 PM, POOVENDHAN SELVARAJ wrote:
>>
>> On 2/18/2023 1:19 AM, Mukesh Ojha wrote:
>>>
>>>
>>> On 2/16/2023 7:30 PM, Mukesh Ojha wrote:
>>>>
>>>>
>>>> On 2/16/2023 5:30 PM, Poovendhan Selvaraj wrote:
>>>>> 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: Anusha Rao <[email protected]>
>>>>> Signed-off-by: Anusha Rao <[email protected]>
>>>>> Co-developed-by: Kathiravan Thirumoorthy <[email protected]>
>>>>> Signed-off-by: Kathiravan Thirumoorthy <[email protected]>
>>>>> Signed-off-by: Poovendhan Selvaraj <[email protected]>
>>>>> ---
>>>>> Changes in V5:
>>>>> - checking the return value in qcom_scm_set_download_mode
>>>>> function as
>>>>> suggested by Srinivas Kandagatla
>>>>>
>>>>> Changes in V4:
>>>>> - retain the orginal value of tcsr register when download mode
>>>>> is not set
>>>>>
>>>>> drivers/firmware/qcom_scm.c | 21 ++++++++++++++++-----
>>>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>>>> index 468d4d5ab550..d88c5f14bd54 100644
>>>>> --- a/drivers/firmware/qcom_scm.c
>>>>> +++ b/drivers/firmware/qcom_scm.c
>>>>> @@ -407,7 +407,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
>>>>> }
>>>>> EXPORT_SYMBOL(qcom_scm_set_remote_state);
>>>>> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
>>>>> +static int __qcom_scm_set_dload_mode(struct device *dev, u32 val,
>>>>> bool enable)
>>>>> {
>>>>> struct qcom_scm_desc desc = {
>>>>> .svc = QCOM_SCM_SVC_BOOT,
>>>>> @@ -417,7 +417,8 @@ static int __qcom_scm_set_dload_mode(struct
>>>>> device *dev, bool enable)
>>>>> .owner = ARM_SMCCC_OWNER_SIP,
>>>>> };
>>>>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>>>>> + desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
>>>>> + val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>>>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>>>>> }
>>>>> @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool
>>>>> enable)
>>>>> {
>>>>> bool avail;
>>>>> int ret = 0;
>>>>> + u32 dload_addr_val;
>>>>> avail = __qcom_scm_is_call_available(__scm->dev,
>>>>> QCOM_SCM_SVC_BOOT,
>>>>> QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>>>> + ret = qcom_scm_io_readl(__scm->dload_mode_addr, &dload_addr_val);
>>>>> +
>>>>> + if (ret) {
>>>>> + dev_err(__scm->dev,
>>>>> + "failed to read dload mode address value: %d\n", ret);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> if (avail) {
>>>>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>>>>> + ret = __qcom_scm_set_dload_mode(__scm->dev,
>>>>> dload_addr_val, enable);
>>>>
>>>> Did you test this on a target where it comes under this if
>>>> statement? does it really need to know dload_mode_addr for this
>>>> target ?
>>>
>>>
>>> Can we do something like this? I would let other review as well.
>>>
>>> --------------------------------------->0-------------------------------------------
>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>> index cdbfe54..26b7eda 100644
>>> --- a/drivers/firmware/qcom_scm.c
>>> +++ b/drivers/firmware/qcom_scm.c
>>> @@ -419,6 +419,7 @@ static void qcom_scm_set_download_mode(bool enable)
>>> {
>>> bool avail;
>>> int ret = 0;
>>> + u32 dload_addr_val;
>>>
>>> avail = __qcom_scm_is_call_available(__scm->dev,
>>> QCOM_SCM_SVC_BOOT,
>>> @@ -426,8 +427,16 @@ 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_readl(__scm->dload_mode_addr,
>>> &dload_addr_val);
>>> + if (ret) {
>>> + dev_err(__scm->dev,
>>> + "failed to read dload mode address
>>> value: %d\n", ret);
>>> + return;
>>> + }
>>> +
>>> + ret = qcom_scm_io_writel(__scm->dload_mode_addr,
>>> enable ?
>>> + dload_addr_val |
>>> QCOM_SCM_BOOT_SET_DLOAD_MODE :
>>> + dload_addr_val &
>>> ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
>>> } else {
>>> dev_err(__scm->dev,
>>> "No available mechanism for setting download
>>> mode\n");
>>>
>>> -Mukesh
>>
>> Okay sure..Agreed, will address this in the next patch.
>
> Also, not sure, if its better to keep the old behavior working for
> targets that does not support 'READ' of this address. If one such
> thing exists, that will be broken now. In such a case, we should
> ignore if scm_io_readl fails, still write and dload_addr_val should
> be '0' initialised.
Why would a secure read of this register would fail, if one is allowed
to do secure write ?
Honestly, i was not understanding the purpose of this bitwise handling
of this patch, i thought it is trying to fix existing issue for
some target.
For some of the upstream target(e.g sm8450, i verified it myself), it is
not an issue.
arch/arm64/boot/dts/qcom/msm8916.dtsi: qcom,dload-mode
= <&tcsr 0x6100>;
arch/arm64/boot/dts/qcom/msm8976.dtsi: qcom,dload-mode
= <&tcsr 0x6100>;
arch/arm64/boot/dts/qcom/msm8996.dtsi: qcom,dload-mode
= <&tcsr_2 0x13000>;
arch/arm64/boot/dts/qcom/sm8450.dtsi: qcom,dload-mode
= <&tcsr 0x13000>;
However, it looks valid to handle only the effective bits. I have worked
on top of this patch and tested it and posted here.
https://lore.kernel.org/lkml/[email protected]/
Do you have any example of any upstream target where this would fail ?
-Mukesh
>
>
> Regards,
> Sricharan
>
<..>
>>>>>> };
>>>>>> - desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
>>>>>> + desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
>>>>>> + val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>>>>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>>>>>> }
>>>>>> @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool
>>>>>> enable)
>>>>>> {
>>>>>> bool avail;
>>>>>> int ret = 0;
>>>>>> + u32 dload_addr_val;
>>>>>> avail = __qcom_scm_is_call_available(__scm->dev,
>>>>>> QCOM_SCM_SVC_BOOT,
>>>>>> QCOM_SCM_BOOT_SET_DLOAD_MODE);
>>>>>> + ret = qcom_scm_io_readl(__scm->dload_mode_addr,
>>>>>> &dload_addr_val);
>>>>>> +
>>>>>> + if (ret) {
>>>>>> + dev_err(__scm->dev,
>>>>>> + "failed to read dload mode address value: %d\n", ret);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> if (avail) {
>>>>>> - ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>>>>>> + ret = __qcom_scm_set_dload_mode(__scm->dev,
>>>>>> dload_addr_val, enable);
>>>>>
>>>>> Did you test this on a target where it comes under this if
>>>>> statement? does it really need to know dload_mode_addr for this
>>>>> target ?
>>>>
>>>>
>>>> Can we do something like this? I would let other review as well.
>>>>
>>>> --------------------------------------->0-------------------------------------------
>>>>
>>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>>>> index cdbfe54..26b7eda 100644
>>>> --- a/drivers/firmware/qcom_scm.c
>>>> +++ b/drivers/firmware/qcom_scm.c
>>>> @@ -419,6 +419,7 @@ static void qcom_scm_set_download_mode(bool enable)
>>>> {
>>>> bool avail;
>>>> int ret = 0;
>>>> + u32 dload_addr_val;
>>>>
>>>> avail = __qcom_scm_is_call_available(__scm->dev,
>>>> QCOM_SCM_SVC_BOOT,
>>>> @@ -426,8 +427,16 @@ 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_readl(__scm->dload_mode_addr,
>>>> &dload_addr_val);
>>>> + if (ret) {
>>>> + dev_err(__scm->dev,
>>>> + "failed to read dload mode address
>>>> value: %d\n", ret);
>>>> + return;
>>>> + }
>>>> +
>>>> + ret = qcom_scm_io_writel(__scm->dload_mode_addr,
>>>> enable ?
>>>> + dload_addr_val |
>>>> QCOM_SCM_BOOT_SET_DLOAD_MODE :
>>>> + dload_addr_val &
>>>> ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
>>>> } else {
>>>> dev_err(__scm->dev,
>>>> "No available mechanism for setting download
>>>> mode\n");
>>>>
>>>> -Mukesh
>>>
>>> Okay sure..Agreed, will address this in the next patch.
>>
>> Also, not sure, if its better to keep the old behavior working for
>> targets that does not support 'READ' of this address. If one such
>> thing exists, that will be broken now. In such a case, we should
>> ignore if scm_io_readl fails, still write and dload_addr_val should
>> be '0' initialised.
>
> Why would a secure read of this register would fail, if one is allowed
> to do secure write ?
>
> Honestly, i was not understanding the purpose of this bitwise handling
> of this patch, i thought it is trying to fix existing issue for
> some target.
>
> For some of the upstream target(e.g sm8450, i verified it myself), it is
> not an issue.
>
> arch/arm64/boot/dts/qcom/msm8916.dtsi: qcom,dload-mode
> = <&tcsr 0x6100>;
> arch/arm64/boot/dts/qcom/msm8976.dtsi: qcom,dload-mode
> = <&tcsr 0x6100>;
> arch/arm64/boot/dts/qcom/msm8996.dtsi: qcom,dload-mode
> = <&tcsr_2 0x13000>;
> arch/arm64/boot/dts/qcom/sm8450.dtsi: qcom,dload-mode
> = <&tcsr 0x13000>;
>
>
> However, it looks valid to handle only the effective bits. I have worked
> on top of this patch and tested it and posted here.
>
> https://lore.kernel.org/lkml/[email protected]/
>
>
> Do you have any example of any upstream target where this would fail ?
Actually not sure. I was saying just based on the fact that,
previously it was 'unconditional write' on all targets, now its a
'conditional write'. If 'read' would never fail from secure,
then your patch looks fine.
Regards,
Sricharan
y
On Thu, 16 Feb 2023 17:30:07 +0530, Poovendhan Selvaraj wrote:
> Crashdump collection is enabled based on the DLOAD bit in the TCSR register.
> This bit is set during bootup and clearing during shutdown. During crash,
> dload bit is not cleared, due to which uboot starts crashdump collection.
>
> Enable the support for download mode to collect the crashdumps if
> system crashes, to debug crashes extensively.
>
> [...]
Applied, thanks!
[1/5] dt-bindings: scm: Add compatible for IPQ9574
commit: 81ac39144bf65c5f8ee00e61308fe1f3399b347c
Best regards,
--
Bjorn Andersson <[email protected]>