Some Qualcomm ETM implementations require skipping powering up
the trace unit, as the ETMs are in the same power domain as
their CPU cores.
Via commit 5214b563588e ("coresight: etm4x: Add support for
sysreg only devices"), the setting of 'skip_power_up' flag was
moved after the 'etm4_init_arch_data' function is called, whereas
the flag value is itself used inside the function. This causes
a crash when ETM mode 'Low-power state behavior override' is set
on some Qualcomm parts.
Fix the same.
Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
Cc: Mike Leach <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhupesh Sharma <redacted>
---
- v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
- Addressed the review comments from Suzuki.
- Rebased on linux-next.
drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
@@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)return-EINVAL;/* TRCPDCR is not accessible with system instructions. */-if(!desc.access.io_mem||-fwnode_property_present(dev_fwnode(dev),"qcom,skip-power-up"))+if(!desc.access.io_mem)drvdata->skip_power_up=true;major=ETM_ARCH_MAJOR_VERSION(drvdata->arch);
--
2.35.3
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Some Qualcomm ETM implementations require skipping powering up
the trace unit, as the ETMs are in the same power domain as
their CPU cores.
Via commit 5214b563588e ("coresight: etm4x: Add support for
sysreg only devices"), the setting of 'skip_power_up' flag was
moved after the 'etm4_init_arch_data' function is called, whereas
the flag value is itself used inside the function. This causes
a crash when ETM mode 'Low-power state behavior override' is set
on some Qualcomm parts.
Fix the same.
Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
Cc: Mike Leach <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhupesh Sharma <redacted>
---
- v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
- Addressed the review comments from Suzuki.
- Rebased on linux-next.
drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
@@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)return-EINVAL;/* TRCPDCR is not accessible with system instructions. */-if(!desc.access.io_mem||-fwnode_property_present(dev_fwnode(dev),"qcom,skip-power-up"))+if(!desc.access.io_mem)drvdata->skip_power_up=true;major=ETM_ARCH_MAJOR_VERSION(drvdata->arch);
Hi Bhupesh,
On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
quoted hunk
Some Qualcomm ETM implementations require skipping powering up
the trace unit, as the ETMs are in the same power domain as
their CPU cores.
Via commit 5214b563588e ("coresight: etm4x: Add support for
sysreg only devices"), the setting of 'skip_power_up' flag was
moved after the 'etm4_init_arch_data' function is called, whereas
the flag value is itself used inside the function. This causes
a crash when ETM mode 'Low-power state behavior override' is set
on some Qualcomm parts.
Fix the same.
Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
Cc: Mike Leach <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhupesh Sharma <redacted>
---
- v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
- Addressed the review comments from Suzuki.
- Rebased on linux-next.
drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
I personally think this sentence should be placed in the function
etm4_probe(), you need to move it just before smp call
etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
to be respected.
quoted hunk
/*
* ETMs implementing sysreg access must implement TRCDEVARCH.
*/
@@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) return -EINVAL; /* TRCPDCR is not accessible with system instructions. */- if (!desc.access.io_mem ||- fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))+ if (!desc.access.io_mem) drvdata->skip_power_up = true;
I prefer to move the condition checking for "desc.access.io_mem" to
etm4_init_sysreg_access(), this can make sure the flag skip_power_up
is set correctly based on property of system register access.
A side topic, in the mainline kernel I found the value
"desc.access.io_mem" is always zero (see the initialized value in
etm4_probe() and etm4_init_sysreg_access()). Should we initialize
desc.access.io_mem to true in etm4_probe()?
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2022-09-14 14:44:07
On 13/09/2022 11:52, Leo Yan wrote:
Hi Bhupesh,
On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
quoted
Some Qualcomm ETM implementations require skipping powering up
the trace unit, as the ETMs are in the same power domain as
their CPU cores.
Via commit 5214b563588e ("coresight: etm4x: Add support for
sysreg only devices"), the setting of 'skip_power_up' flag was
moved after the 'etm4_init_arch_data' function is called, whereas
the flag value is itself used inside the function. This causes
a crash when ETM mode 'Low-power state behavior override' is set
on some Qualcomm parts.
Fix the same.
Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only devices")
Cc: Mike Leach <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhupesh Sharma <redacted>
---
- v1 can be seen here: https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
- Addressed the review comments from Suzuki.
- Rebased on linux-next.
drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
I personally think this sentence should be placed in the function
etm4_probe(), you need to move it just before smp call
etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
to be respected.
Or we could move this to init_iomem_access() and explicitly set the flag
to true in init_sysreg_access().
quoted
/*
* ETMs implementing sysreg access must implement TRCDEVARCH.
*/
@@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) return -EINVAL; /* TRCPDCR is not accessible with system instructions. */- if (!desc.access.io_mem ||- fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))+ if (!desc.access.io_mem) drvdata->skip_power_up = true;
I prefer to move the condition checking for "desc.access.io_mem" to
etm4_init_sysreg_access(), this can make sure the flag skip_power_up
is set correctly based on property of system register access.
+1
A side topic, in the mainline kernel I found the value
"desc.access.io_mem" is always zero (see the initialized value in
etm4_probe() and etm4_init_sysreg_access()). Should we initialize
desc.access.io_mem to true in etm4_probe()?
Thats not true. It is initialised at :
etm4_init_iomem_access():
*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
Where
#define CSDEV_ACCESS_IOMEM(_addr) \
((struct csdev_access) { \
.io_mem = true, \
.base = (_addr), \
})
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Sep 14, 2022 at 03:43:53PM +0100, Suzuki Kuruppassery Poulose wrote:
[...]
quoted
quoted
+ /*
+ * Some Qualcomm implementations require skipping powering up the trace unit,
+ * as the ETMs are in the same power domain as their CPU cores.
+ *
+ * Since the 'skip_power_up' flag is used inside 'etm4_init_arch_data' function,
+ * initialize it before the function is called.
+ */
+ if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
+ drvdata->skip_power_up = true;
+
I personally think this sentence should be placed in the function
etm4_probe(), you need to move it just before smp call
etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
to be respected.
Or we could move this to init_iomem_access() and explicitly set the flag
to true in init_sysreg_access().
Agreed, this is more clear.
[...]
quoted
A side topic, in the mainline kernel I found the value
"desc.access.io_mem" is always zero (see the initialized value in
etm4_probe() and etm4_init_sysreg_access()). Should we initialize
desc.access.io_mem to true in etm4_probe()?
Thats not true. It is initialised at :
etm4_init_iomem_access():
*csa = CSDEV_ACCESS_IOMEM(drvdata->base);
Where
#define CSDEV_ACCESS_IOMEM(_addr) \
((struct csdev_access) { \
.io_mem = true, \
.base = (_addr), \
})
From: Suzuki K Poulose <suzuki.poulose@arm.com> Date: 2022-11-07 16:44:22
Hi Bupesh,
\
On 14/09/2022 15:43, Suzuki K Poulose wrote:
On 13/09/2022 11:52, Leo Yan wrote:
quoted
Hi Bhupesh,
On Thu, Aug 25, 2022 at 10:52:32AM +0530, Bhupesh Sharma wrote:
quoted
Some Qualcomm ETM implementations require skipping powering up
the trace unit, as the ETMs are in the same power domain as
their CPU cores.
Via commit 5214b563588e ("coresight: etm4x: Add support for
sysreg only devices"), the setting of 'skip_power_up' flag was
moved after the 'etm4_init_arch_data' function is called, whereas
the flag value is itself used inside the function. This causes
a crash when ETM mode 'Low-power state behavior override' is set
on some Qualcomm parts.
Fix the same.
Fixes: 5214b563588e ("coresight: etm4x: Add support for sysreg only
devices")
Cc: Mike Leach <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Bhupesh Sharma <redacted>
---
- v1 can be seen here:
https://lore.kernel.org/lkml/20220803191236.3037591-1-bhupesh.sharma@linaro.org/
- Addressed the review comments from Suzuki.
- Rebased on linux-next.
drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
etmv4_drvdata *drvdata,
if (!cpu_supports_sysreg_trace())
return false;
+ /*
+ * Some Qualcomm implementations require skipping powering up
the trace unit,
+ * as the ETMs are in the same power domain as their CPU cores.
+ *
+ * Since the 'skip_power_up' flag is used inside
'etm4_init_arch_data' function,
+ * initialize it before the function is called.
+ */
+ if (fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
+ drvdata->skip_power_up = true;
+
I personally think this sentence should be placed in the function
etm4_probe(), you need to move it just before smp call
etm4_init_arch_data(), this can allow DT property "qcom,skip-power-up"
to be respected.
Or we could move this to init_iomem_access() and explicitly set the flag
to true in init_sysreg_access().
quoted
quoted
/*
* ETMs implementing sysreg access must implement TRCDEVARCH.
*/
@@ -1951,8 +1961,7 @@ static int etm4_probe(struct device *dev, void
__iomem *base, u32 etm_pid)
return -EINVAL;
/* TRCPDCR is not accessible with system instructions. */
- if (!desc.access.io_mem ||
- fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
+ if (!desc.access.io_mem)
drvdata->skip_power_up = true;
I prefer to move the condition checking for "desc.access.io_mem" to
etm4_init_sysreg_access(), this can make sure the flag skip_power_up
is set correctly based on property of system register access.
+1
Do you plan to send an updated patch for this one ?
Kind regards
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel