From: Tingwei Zhang tingwei@codeaurora.org
On some Qualcomm Technologies Inc. SoCs like SC7180, there exists a hardware errata where the APSS (Application Processor SubSystem)/CPU watchdog counter is stopped when ETM register TRCPDCR.PU=1. Since the ETMs share the same power domain as that of respective CPU cores, they are powered on when the CPU core is powered on. So we can disable powering up of the trace unit after checking for this errata via new property called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org --- .../devicetree/bindings/arm/coresight.txt | 6 ++++ drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++------- 2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt index 846f6daae71b..d2030128fe46 100644 --- a/Documentation/devicetree/bindings/arm/coresight.txt +++ b/Documentation/devicetree/bindings/arm/coresight.txt @@ -108,6 +108,12 @@ its hardware characteristcs. * arm,cp14: must be present if the system accesses ETM/PTM management registers via co-processor 14.
+ * qcom,tupwr-disable: boolean. Indicates that trace unit power up can + be disabled on Qualcomm Technologies Inc. systems where ETMs are in + the same power domain as their CPU cores. This property is required + to identify such systems with hardware errata where the CPU watchdog + counter is stopped when TRCPDCR.PU=1. + * Optional property for TMC:
* arm,buffer-size: size of contiguous buffer space for TMC ETR diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index fb0f5f4f3a91..6886b44f6947 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -104,6 +104,11 @@ struct etm4_enable_arg { int rc; };
+static inline bool etm4_can_disable_tupwr(struct device *dev) +{ + return fwnode_property_present(dev_fwnode(dev), "qcom,tupwr-disable"); +} + static int etm4_enable_hw(struct etmv4_drvdata *drvdata) { int i, rc; @@ -196,12 +201,14 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) writel_relaxed(config->vmid_mask0, drvdata->base + TRCVMIDCCTLR0); writel_relaxed(config->vmid_mask1, drvdata->base + TRCVMIDCCTLR1);
- /* - * Request to keep the trace unit powered and also - * emulation of powerdown - */ - writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU, - drvdata->base + TRCPDCR); + if (!etm4_can_disable_tupwr(etm_dev)) { + /* + * Request to keep the trace unit powered and also + * emulation of powerdown + */ + writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU, + drvdata->base + TRCPDCR); + }
/* Enable the trace unit */ writel_relaxed(1, drvdata->base + TRCPRGCTLR); @@ -476,10 +483,12 @@ static void etm4_disable_hw(void *info)
CS_UNLOCK(drvdata->base);
- /* power can be removed from the trace unit now */ - control = readl_relaxed(drvdata->base + TRCPDCR); - control &= ~TRCPDCR_PU; - writel_relaxed(control, drvdata->base + TRCPDCR); + if (!etm4_can_disable_tupwr(etm_dev)) { + /* power can be removed from the trace unit now */ + control = readl_relaxed(drvdata->base + TRCPDCR); + control &= ~TRCPDCR_PU; + writel_relaxed(control, drvdata->base + TRCPDCR); + }
control = readl_relaxed(drvdata->base + TRCPRGCTLR);
Good morning Sai,
On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
From: Tingwei Zhang tingwei@codeaurora.org
On some Qualcomm Technologies Inc. SoCs like SC7180, there exists a hardware errata where the APSS (Application Processor SubSystem)/CPU watchdog counter is stopped when ETM register TRCPDCR.PU=1.
Fun stuff...
Since the ETMs share the same power domain as that of respective CPU cores, they are powered on when the CPU core is powered on. So we can disable powering up of the trace unit after checking for this errata via new property called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org
Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
.../devicetree/bindings/arm/coresight.txt | 6 ++++ drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
Please split in two patches.
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt index 846f6daae71b..d2030128fe46 100644 --- a/Documentation/devicetree/bindings/arm/coresight.txt +++ b/Documentation/devicetree/bindings/arm/coresight.txt @@ -108,6 +108,12 @@ its hardware characteristcs.
- arm,cp14: must be present if the system accesses ETM/PTM management registers via co-processor 14.
- qcom,tupwr-disable: boolean. Indicates that trace unit power up can
be disabled on Qualcomm Technologies Inc. systems where ETMs are inthe same power domain as their CPU cores. This property is requiredto identify such systems with hardware errata where the CPU watchdogcounter is stopped when TRCPDCR.PU=1.
I think something like "qcom,skip-power-up" would be clearer.
Also, a better choice of words is that TRCPDCR.PU does not have to be set on Qualcomm...
- Optional property for TMC:
- arm,buffer-size: size of contiguous buffer space for TMC ETR
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index fb0f5f4f3a91..6886b44f6947 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -104,6 +104,11 @@ struct etm4_enable_arg { int rc; }; +static inline bool etm4_can_disable_tupwr(struct device *dev) +{
- return fwnode_property_present(dev_fwnode(dev), "qcom,tupwr-disable");
+}
Please call fwnode_property_present() at initialisation time to set a new drvdata::skip_power_up variable. From there just switch on that in etm4_enable/disable_hw().
Thanks, Mathieu
static int etm4_enable_hw(struct etmv4_drvdata *drvdata) { int i, rc; @@ -196,12 +201,14 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) writel_relaxed(config->vmid_mask0, drvdata->base + TRCVMIDCCTLR0); writel_relaxed(config->vmid_mask1, drvdata->base + TRCVMIDCCTLR1);
- /*
* Request to keep the trace unit powered and also* emulation of powerdown*/- writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,
drvdata->base + TRCPDCR);
- if (!etm4_can_disable_tupwr(etm_dev)) {
/** Request to keep the trace unit powered and also* emulation of powerdown*/writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,drvdata->base + TRCPDCR);- }
/* Enable the trace unit */ writel_relaxed(1, drvdata->base + TRCPRGCTLR); @@ -476,10 +483,12 @@ static void etm4_disable_hw(void *info) CS_UNLOCK(drvdata->base);
- /* power can be removed from the trace unit now */
- control = readl_relaxed(drvdata->base + TRCPDCR);
- control &= ~TRCPDCR_PU;
- writel_relaxed(control, drvdata->base + TRCPDCR);
- if (!etm4_can_disable_tupwr(etm_dev)) {
/* power can be removed from the trace unit now */control = readl_relaxed(drvdata->base + TRCPDCR);control &= ~TRCPDCR_PU;writel_relaxed(control, drvdata->base + TRCPDCR);- }
control = readl_relaxed(drvdata->base + TRCPRGCTLR); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Mathieu,
On 2020-05-14 23:30, Mathieu Poirier wrote:
Good morning Sai,
On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
From: Tingwei Zhang tingwei@codeaurora.org
On some Qualcomm Technologies Inc. SoCs like SC7180, there exists a hardware errata where the APSS (Application Processor SubSystem)/CPU watchdog counter is stopped when ETM register TRCPDCR.PU=1.
Fun stuff...
Yes :)
Since the ETMs share the same power domain as that of respective CPU cores, they are powered on when the CPU core is powered on. So we can disable powering up of the trace unit after checking for this errata via new property called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org
Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
Tingwei is the author, so if I understand correctly, his signed-off-by should appear first, am I wrong?
.../devicetree/bindings/arm/coresight.txt | 6 ++++ drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
Please split in two patches.
Sure, I will split the dt-binding into separate patch, checkpatch did warn.
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt index 846f6daae71b..d2030128fe46 100644 --- a/Documentation/devicetree/bindings/arm/coresight.txt +++ b/Documentation/devicetree/bindings/arm/coresight.txt @@ -108,6 +108,12 @@ its hardware characteristcs.
- arm,cp14: must be present if the system accesses ETM/PTM
management registers via co-processor 14.
- qcom,tupwr-disable: boolean. Indicates that trace unit power up
can
be disabled on Qualcomm Technologies Inc. systems where ETMs arein
the same power domain as their CPU cores. This property isrequired
to identify such systems with hardware errata where the CPUwatchdog
counter is stopped when TRCPDCR.PU=1.I think something like "qcom,skip-power-up" would be clearer.
Also, a better choice of words is that TRCPDCR.PU does not have to be set on Qualcomm...
Yes "qcom,skip-power-up" is a lot better, thanks. Also will use something as you suggested for description.
Optional property for TMC:
- arm,buffer-size: size of contiguous buffer space for TMC ETR
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index fb0f5f4f3a91..6886b44f6947 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -104,6 +104,11 @@ struct etm4_enable_arg { int rc; };
+static inline bool etm4_can_disable_tupwr(struct device *dev) +{
- return fwnode_property_present(dev_fwnode(dev),
"qcom,tupwr-disable"); +}
Please call fwnode_property_present() at initialisation time to set a new drvdata::skip_power_up variable. From there just switch on that in etm4_enable/disable_hw().
Will do, thanks.
Thanks, Sai
On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org wrote:
Hi Mathieu,
On 2020-05-14 23:30, Mathieu Poirier wrote:
Good morning Sai,
On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
From: Tingwei Zhang tingwei@codeaurora.org
On some Qualcomm Technologies Inc. SoCs like SC7180, there exists a hardware errata where the APSS (Application Processor SubSystem)/CPU watchdog counter is stopped when ETM register TRCPDCR.PU=1.
Fun stuff...
Yes :)
Since the ETMs share the same power domain as that of respective CPU cores, they are powered on when the CPU core is powered on. So we can disable powering up of the trace unit after checking for this errata via new property called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org
Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
Tingwei is the author, so if I understand correctly, his signed-off-by should appear first, am I wrong?
It's a gray area and depends on who's code is more prevalent in the patch. If Tingwei wrote the most of the code then his name is in the "from:" section, yours as co-developer and he signs off on it (as I suggested). If you did most of the work then it is the opposite. Adding a Co-developed and a signed-off with the same name doesn't make sense.
.../devicetree/bindings/arm/coresight.txt | 6 ++++ drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
Please split in two patches.
Sure, I will split the dt-binding into separate patch, checkpatch did warn.
And you still sent me the patch... I usually run checkpatch before all the submissions I review and flatly ignore patches that return errors. You got lucky...
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt index 846f6daae71b..d2030128fe46 100644 --- a/Documentation/devicetree/bindings/arm/coresight.txt +++ b/Documentation/devicetree/bindings/arm/coresight.txt @@ -108,6 +108,12 @@ its hardware characteristcs. * arm,cp14: must be present if the system accesses ETM/PTM management registers via co-processor 14.
- qcom,tupwr-disable: boolean. Indicates that trace unit power up
can
be disabled on Qualcomm Technologies Inc. systems where ETMs arein
the same power domain as their CPU cores. This property isrequired
to identify such systems with hardware errata where the CPUwatchdog
counter is stopped when TRCPDCR.PU=1.I think something like "qcom,skip-power-up" would be clearer.
Also, a better choice of words is that TRCPDCR.PU does not have to be set on Qualcomm...
Yes "qcom,skip-power-up" is a lot better, thanks. Also will use something as you suggested for description.
Optional property for TMC:
- arm,buffer-size: size of contiguous buffer space for TMC ETR
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index fb0f5f4f3a91..6886b44f6947 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -104,6 +104,11 @@ struct etm4_enable_arg { int rc; };
+static inline bool etm4_can_disable_tupwr(struct device *dev) +{
- return fwnode_property_present(dev_fwnode(dev),
"qcom,tupwr-disable"); +}
Please call fwnode_property_present() at initialisation time to set a new drvdata::skip_power_up variable. From there just switch on that in etm4_enable/disable_hw().
Will do, thanks.
Thanks, Sai
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Mathieu,
On 2020-05-15 20:22, Mathieu Poirier wrote:
On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org wrote:
Hi Mathieu,
On 2020-05-14 23:30, Mathieu Poirier wrote:
Good morning Sai,
On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
From: Tingwei Zhang tingwei@codeaurora.org
On some Qualcomm Technologies Inc. SoCs like SC7180, there exists a hardware errata where the APSS (Application Processor SubSystem)/CPU watchdog counter is stopped when ETM register TRCPDCR.PU=1.
Fun stuff...
Yes :)
Since the ETMs share the same power domain as that of respective CPU cores, they are powered on when the CPU core is powered on. So we can disable powering up of the trace unit after checking for this errata via new property called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org
Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
Tingwei is the author, so if I understand correctly, his signed-off-by should appear first, am I wrong?
It's a gray area and depends on who's code is more prevalent in the patch. If Tingwei wrote the most of the code then his name is in the "from:" section, yours as co-developer and he signs off on it (as I suggested). If you did most of the work then it is the opposite. Adding a Co-developed and a signed-off with the same name doesn't make sense.
I did check the documentation for submitting patches: Documentation/process/submitting-patches.rst. And it clearly states that "Co-developed-by must be followed by Signed-off by the co-author and the last Signed-off-by: must always be that of the developer submitting the patch".
Quoting below from the doc:
Co-developed-by: <snip> ...Since Co-developed-by: denotes authorship, every Co-developed-by: must be immediately followed by a Signed-off-by: of the associated co-author. Standard sign-off procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the chronological history of the patch insofar as possible, regardless of whether the author is attributed via From: or Co-developed-by:. Notably, the last Signed-off-by: must always be that of the developer submitting the patch.
.../devicetree/bindings/arm/coresight.txt | 6 ++++ drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
Please split in two patches.
Sure, I will split the dt-binding into separate patch, checkpatch did warn.
And you still sent me the patch... I usually run checkpatch before all the submissions I review and flatly ignore patches that return errors. You got lucky...
I did not mean to ignore it or else I wouldn't have run checkpatch itself. I checked other cases like "arm,scatter-gather" where the binding and the driver change was in a single patch, hence I thought it's not a very strict rule.
Thanks, Sai
On Fri, May 15, 2020 at 08:37:13PM +0530, Sai Prakash Ranjan wrote:
Hi Mathieu,
On 2020-05-15 20:22, Mathieu Poirier wrote:
On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org wrote:
Hi Mathieu,
On 2020-05-14 23:30, Mathieu Poirier wrote:
Good morning Sai,
On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
From: Tingwei Zhang tingwei@codeaurora.org
On some Qualcomm Technologies Inc. SoCs like SC7180, there exists a hardware errata where the APSS (Application Processor SubSystem)/CPU watchdog counter is stopped when ETM register TRCPDCR.PU=1.
Fun stuff...
Yes :)
Since the ETMs share the same power domain as that of respective CPU cores, they are powered on when the CPU core is powered on. So we can disable powering up of the trace unit after checking for this errata via new property called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org
Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
Tingwei is the author, so if I understand correctly, his signed-off-by should appear first, am I wrong?
It's a gray area and depends on who's code is more prevalent in the patch. If Tingwei wrote the most of the code then his name is in the "from:" section, yours as co-developer and he signs off on it (as I suggested). If you did most of the work then it is the opposite. Adding a Co-developed and a signed-off with the same name doesn't make sense.
I did check the documentation for submitting patches: Documentation/process/submitting-patches.rst. And it clearly states that "Co-developed-by must be followed by Signed-off by the co-author and the last Signed-off-by: must always be that of the developer submitting the patch".
Quoting below from the doc:
Co-developed-by: <snip> ...Since Co-developed-by: denotes authorship, every Co-developed-by: must be immediately followed by a Signed-off-by: of the associated co-author. Standard sign-off procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the chronological history of the patch insofar as possible, regardless of whether the author is attributed via From: or Co-developed-by:. Notably, the last Signed-off-by: must always be that of the developer submitting the patch.
Ah yes, glad to see that got clarified. You can ignore my recommendation on that snippet.
.../devicetree/bindings/arm/coresight.txt | 6 ++++ drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
Please split in two patches.
Sure, I will split the dt-binding into separate patch, checkpatch did warn.
And you still sent me the patch... I usually run checkpatch before all the submissions I review and flatly ignore patches that return errors. You got lucky...
I did not mean to ignore it or else I wouldn't have run checkpatch itself. I checked other cases like "arm,scatter-gather" where the binding and the driver change was in a single patch, hence I thought it's not a very strict rule.
The patch has another warning for a line over 80 characters, that should have been fixed before sending. Putting DT changes in a separate patch is always better for the DT people. They review tons of patches and making their life easier is always a good thing.
Regards, Mathieu
Thanks, Sai -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Mathieu,
On 2020-05-15 21:21, Mathieu Poirier wrote:
On Fri, May 15, 2020 at 08:37:13PM +0530, Sai Prakash Ranjan wrote:
Hi Mathieu,
On 2020-05-15 20:22, Mathieu Poirier wrote:
On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org wrote:
Hi Mathieu,
On 2020-05-14 23:30, Mathieu Poirier wrote:
Good morning Sai,
On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
From: Tingwei Zhang tingwei@codeaurora.org
On some Qualcomm Technologies Inc. SoCs like SC7180, there exists a hardware errata where the APSS (Application Processor SubSystem)/CPU watchdog counter is stopped when ETM register TRCPDCR.PU=1.
Fun stuff...
Yes :)
Since the ETMs share the same power domain as that of respective CPU cores, they are powered on when the CPU core is powered on. So we can disable powering up of the trace unit after checking for this errata via new property called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org
Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
Tingwei is the author, so if I understand correctly, his signed-off-by should appear first, am I wrong?
It's a gray area and depends on who's code is more prevalent in the patch. If Tingwei wrote the most of the code then his name is in the "from:" section, yours as co-developer and he signs off on it (as I suggested). If you did most of the work then it is the opposite. Adding a Co-developed and a signed-off with the same name doesn't make sense.
I did check the documentation for submitting patches: Documentation/process/submitting-patches.rst. And it clearly states that "Co-developed-by must be followed by Signed-off by the co-author and the last Signed-off-by: must always be that of the developer submitting the patch".
Quoting below from the doc:
Co-developed-by: <snip> ...Since Co-developed-by: denotes authorship, every Co-developed-by: must be immediately followed by a Signed-off-by: of the associated co-author. Standard sign-off procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the chronological history of the patch insofar as possible, regardless of whether the author is attributed via From: or Co-developed-by:. Notably, the last Signed-off-by: must always be that of the developer submitting the patch.
Ah yes, glad to see that got clarified. You can ignore my recommendation on that snippet.
.../devicetree/bindings/arm/coresight.txt | 6 ++++ drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
Please split in two patches.
Sure, I will split the dt-binding into separate patch, checkpatch did warn.
And you still sent me the patch... I usually run checkpatch before all the submissions I review and flatly ignore patches that return errors. You got lucky...
I did not mean to ignore it or else I wouldn't have run checkpatch itself. I checked other cases like "arm,scatter-gather" where the binding and the driver change was in a single patch, hence I thought it's not a very strict rule.
The patch has another warning for a line over 80 characters, that should have been fixed before sending. Putting DT changes in a separate patch is always better for the DT people. They review tons of patches and making their life easier is always a good thing.
Ok, I will fix this and resend. I did not want to change it in case if it affects readability since most maintainers prefer to ignore this 80 characters warning if it affects readability. I will keep this in mind for future patches as well.
Thanks, Sai
On 2020-05-15 21:28, Sai Prakash Ranjan wrote:
Hi Mathieu,
On 2020-05-15 21:21, Mathieu Poirier wrote:
On Fri, May 15, 2020 at 08:37:13PM +0530, Sai Prakash Ranjan wrote:
Hi Mathieu,
On 2020-05-15 20:22, Mathieu Poirier wrote:
On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org wrote:
Hi Mathieu,
On 2020-05-14 23:30, Mathieu Poirier wrote:
Good morning Sai,
On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote: > From: Tingwei Zhang tingwei@codeaurora.org > > On some Qualcomm Technologies Inc. SoCs like SC7180, there > exists a hardware errata where the APSS (Application Processor > SubSystem)/CPU watchdog counter is stopped when ETM register > TRCPDCR.PU=1.
Fun stuff...
Yes :)
> Since the ETMs share the same power domain as > that of respective CPU cores, they are powered on when the > CPU core is powered on. So we can disable powering up of the > trace unit after checking for this errata via new property > called "qcom,tupwr-disable". > > Signed-off-by: Tingwei Zhang tingwei@codeaurora.org > Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org > Signed-off-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org
Co-developed-by: Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org
Tingwei is the author, so if I understand correctly, his signed-off-by should appear first, am I wrong?
It's a gray area and depends on who's code is more prevalent in the patch. If Tingwei wrote the most of the code then his name is in the "from:" section, yours as co-developer and he signs off on it (as I suggested). If you did most of the work then it is the opposite. Adding a Co-developed and a signed-off with the same name doesn't make sense.
I did check the documentation for submitting patches: Documentation/process/submitting-patches.rst. And it clearly states that "Co-developed-by must be followed by Signed-off by the co-author and the last Signed-off-by: must always be that of the developer submitting the patch".
Quoting below from the doc:
Co-developed-by: <snip> ...Since Co-developed-by: denotes authorship, every Co-developed-by: must be immediately followed by a Signed-off-by: of the associated co-author. Standard sign-off procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the chronological history of the patch insofar as possible, regardless of whether the author is attributed via From: or Co-developed-by:. Notably, the last Signed-off-by: must always be that of the developer submitting the patch.
Ah yes, glad to see that got clarified. You can ignore my recommendation on that snippet.
> --- > .../devicetree/bindings/arm/coresight.txt | 6 ++++ > drivers/hwtracing/coresight/coresight-etm4x.c | 29 > ++++++++++++-------
Please split in two patches.
Sure, I will split the dt-binding into separate patch, checkpatch did warn.
And you still sent me the patch... I usually run checkpatch before all the submissions I review and flatly ignore patches that return errors. You got lucky...
I did not mean to ignore it or else I wouldn't have run checkpatch itself. I checked other cases like "arm,scatter-gather" where the binding and the driver change was in a single patch, hence I thought it's not a very strict rule.
The patch has another warning for a line over 80 characters, that should have been fixed before sending. Putting DT changes in a separate patch is always better for the DT people. They review tons of patches and making their life easier is always a good thing.
Ok, I will fix this and resend. I did not want to change it in case if it affects readability since most maintainers prefer to ignore this 80 characters warning if it affects readability. I will keep this in mind for future patches as well.
Now fixed all checkpatch warnings and addressed other review comments. Posted v3 - https://lore.kernel.org/patchwork/cover/1242572/
Thanks, Sai
-
Mathieu Poirier -
Sai Prakash Ranjan