| Message ID | 20200514105915.27516-1-saiprakash.ranjan@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm4x: Add support to disable trace unit power up | expand |
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 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. > + 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 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. >> + > > 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 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. > >> + > > > > 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
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);