| Message ID | 20180518012024.22645-1-kim.phillips@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 17, 2018 at 08:20:19PM -0500, Kim Phillips wrote: > A coresight topology doesn't need to include links, i.e., a source can > be directly connected to a sink. As such, selecting and/or depending on > LINKS_AND_SINKS is no longer needed. I'm good with this patch but now the help text for CORESIGHT_LINKS_AND_SINKS no longer match what the config does. I see two ways to fix this: 1) Rework the help text. 2) Rework CORESIGHT_LINKS_AND_SINKS to be CORESIGHT_FUNNEL and move coresight-replicator.o under CORESIGHT_DYNAMIC_REPLICATOR in the Makefile. I really liked your idea of making the replicator driver intelligent enough to deal with both DT and platform declaration, which merges two driver into one. I'm obviously favouring the second option but recognise it doesn't have to be part of this patchet. So for this set please rework the help text for CORESIGHT_LINKS_AND_SINKS. Once we've dealt with this topic we can refactor the replicator driver. Thanks, Mathieu > > Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Signed-off-by: Kim Phillips <kim.phillips@arm.com> > --- > drivers/hwtracing/coresight/Kconfig | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > index ef9cb3c164e1..83fb78651ef9 100644 > --- a/drivers/hwtracing/coresight/Kconfig > +++ b/drivers/hwtracing/coresight/Kconfig > @@ -23,7 +23,6 @@ config CORESIGHT_LINKS_AND_SINKS > > config CORESIGHT_LINK_AND_SINK_TMC > bool "Coresight generic TMC driver" > - depends on CORESIGHT_LINKS_AND_SINKS > help > This enables support for the Trace Memory Controller driver. > Depending on its configuration the device can act as a link (embedded > @@ -33,7 +32,6 @@ config CORESIGHT_LINK_AND_SINK_TMC > > config CORESIGHT_SINK_TPIU > bool "Coresight generic TPIU driver" > - depends on CORESIGHT_LINKS_AND_SINKS > help > This enables support for the Trace Port Interface Unit driver, > responsible for bridging the gap between the on-chip coresight > @@ -44,7 +42,6 @@ config CORESIGHT_SINK_TPIU > > config CORESIGHT_SINK_ETBV10 > bool "Coresight ETBv1.0 driver" > - depends on CORESIGHT_LINKS_AND_SINKS > help > This enables support for the Embedded Trace Buffer version 1.0 driver > that complies with the generic implementation of the component without > @@ -53,7 +50,6 @@ config CORESIGHT_SINK_ETBV10 > config CORESIGHT_SOURCE_ETM3X > bool "CoreSight Embedded Trace Macrocell 3.x driver" > depends on !ARM64 > - select CORESIGHT_LINKS_AND_SINKS > help > This driver provides support for processor ETM3.x and PTM1.x modules, > which allows tracing the instructions that a processor is executing > @@ -63,7 +59,6 @@ config CORESIGHT_SOURCE_ETM3X > config CORESIGHT_SOURCE_ETM4X > bool "CoreSight Embedded Trace Macrocell 4.x driver" > depends on ARM64 > - select CORESIGHT_LINKS_AND_SINKS > help > This driver provides support for the ETM4.x tracer module, tracing the > instructions that a processor is executing. This is primarily useful > @@ -72,7 +67,6 @@ config CORESIGHT_SOURCE_ETM4X > > config CORESIGHT_DYNAMIC_REPLICATOR > bool "CoreSight Programmable Replicator driver" > - depends on CORESIGHT_LINKS_AND_SINKS > help > This enables support for dynamic CoreSight replicator link driver. > The programmable ATB replicator allows independent filtering of the > @@ -81,7 +75,6 @@ config CORESIGHT_DYNAMIC_REPLICATOR > config CORESIGHT_STM > bool "CoreSight System Trace Macrocell driver" > depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64 > - select CORESIGHT_LINKS_AND_SINKS > select STM > help > This driver provides support for hardware assisted software > -- > 2.17.0 >
On Tue, 22 May 2018 11:31:40 -0600 Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > On Thu, May 17, 2018 at 08:20:19PM -0500, Kim Phillips wrote: > > A coresight topology doesn't need to include links, i.e., a source can > > be directly connected to a sink. As such, selecting and/or depending on > > LINKS_AND_SINKS is no longer needed. > > I'm good with this patch but now the help text for CORESIGHT_LINKS_AND_SINKS no > longer match what the config does. I see two ways to fix this: This patch doesn't change what the config does, it just changes what other config options depend on it. > 1) Rework the help text. I don't see how, given the above. Here's the text: config CORESIGHT_LINKS_AND_SINKS bool "CoreSight Link and Sink drivers" help This enables support for CoreSight link and sink drivers that are responsible for transporting and collecting the trace data respectively. Link and sinks are dynamically aggregated with a trace entity at run time to form a complete trace path. What part of that becomes invalid with this patch? > 2) Rework CORESIGHT_LINKS_AND_SINKS to be CORESIGHT_FUNNEL and move > coresight-replicator.o under CORESIGHT_DYNAMIC_REPLICATOR in the Makefile. I > really liked your idea of making the replicator driver intelligent enough to > deal with both DT and platform declaration, which merges two driver into one. > > I'm obviously favouring the second option but recognise it doesn't have to be > part of this patchet. So for this set please rework the help text for > CORESIGHT_LINKS_AND_SINKS. Once we've dealt with this topic we can refactor the > replicator driver. I'd really like to just focus on getting CoreSight to load as modules, something for which this patch isn't technically required... Thanks, Kim
On 23 May 2018 at 13:51, Kim Phillips <kim.phillips@arm.com> wrote: > On Tue, 22 May 2018 11:31:40 -0600 > Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > >> On Thu, May 17, 2018 at 08:20:19PM -0500, Kim Phillips wrote: >> > A coresight topology doesn't need to include links, i.e., a source can >> > be directly connected to a sink. As such, selecting and/or depending on >> > LINKS_AND_SINKS is no longer needed. >> >> I'm good with this patch but now the help text for CORESIGHT_LINKS_AND_SINKS no >> longer match what the config does. I see two ways to fix this: > > This patch doesn't change what the config does, it just changes what > other config options depend on it. > >> 1) Rework the help text. > > I don't see how, given the above. Here's the text: > > config CORESIGHT_LINKS_AND_SINKS > bool "CoreSight Link and Sink drivers" > help > This enables support for CoreSight link and sink drivers that are > responsible for transporting and collecting the trace data > respectively. Link and sinks are dynamically aggregated with a trace > entity at run time to form a complete trace path. > > What part of that becomes invalid with this patch? Looking at the new Kconfig, what sink component depend on CORESIGHT_LINKS_AND_SINKS? config CORESIGHT_LINKS bool "CoreSight Link drivers" help This enables support for CoreSight link drivers that are responsible for transporting trace data from source to sink. Links are dynamically aggregated with other traces entities at run time to form a complete trace path. > >> 2) Rework CORESIGHT_LINKS_AND_SINKS to be CORESIGHT_FUNNEL and move >> coresight-replicator.o under CORESIGHT_DYNAMIC_REPLICATOR in the Makefile. I >> really liked your idea of making the replicator driver intelligent enough to >> deal with both DT and platform declaration, which merges two driver into one. >> >> I'm obviously favouring the second option but recognise it doesn't have to be >> part of this patchet. So for this set please rework the help text for >> CORESIGHT_LINKS_AND_SINKS. Once we've dealt with this topic we can refactor the >> replicator driver. > > I'd really like to just focus on getting CoreSight to load as modules, > something for which this patch isn't technically required... The only thing I'm asking is that the config description and help text reflect what the Makefile does. > > Thanks, > > Kim
On Thu, 24 May 2018 09:32:48 -0600 Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > On 23 May 2018 at 13:51, Kim Phillips <kim.phillips@arm.com> wrote: > > On Tue, 22 May 2018 11:31:40 -0600 > > Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > > >> On Thu, May 17, 2018 at 08:20:19PM -0500, Kim Phillips wrote: > >> > A coresight topology doesn't need to include links, i.e., a source can > >> > be directly connected to a sink. As such, selecting and/or depending on > >> > LINKS_AND_SINKS is no longer needed. > >> > >> I'm good with this patch but now the help text for CORESIGHT_LINKS_AND_SINKS no > >> longer match what the config does. I see two ways to fix this: > > > > This patch doesn't change what the config does, it just changes what > > other config options depend on it. > > > >> 1) Rework the help text. > > > > I don't see how, given the above. Here's the text: > > > > config CORESIGHT_LINKS_AND_SINKS > > bool "CoreSight Link and Sink drivers" > > help > > This enables support for CoreSight link and sink drivers that are > > responsible for transporting and collecting the trace data > > respectively. Link and sinks are dynamically aggregated with a trace > > entity at run time to form a complete trace path. > > > > What part of that becomes invalid with this patch? > > Looking at the new Kconfig, what sink component depend on > CORESIGHT_LINKS_AND_SINKS? How does that affect the description text? The description text doesn't insinuate any implicit dependencies or non-. > config CORESIGHT_LINKS Please, not another gratuitous config name change, I've already experienced usage regressions from the CORESIGHT_QCOM_REPLICATOR => CORESIGHT_DYNAMIC_REPLICATOR change: https://patchwork.kernel.org/patch/10206023/ > bool "CoreSight Link drivers" > help > This enables support for CoreSight link drivers that are responsible > for transporting trace data from source to sink. Links are > dynamically > aggregated with other traces entities at run time to form a > complete trace > path. Oh, I see, so your point is that LINKS_AND_SINKS doesn't technically build any sink drivers? That's completely orthogonal to removing a dependency chain: that just tells me the name was a poor choice in the first place maybe? I don't see where the Makefile may have built a sink, but it may be before the move to drivers/hwtracing/coresight, or some other reorganization. > >> 2) Rework CORESIGHT_LINKS_AND_SINKS to be CORESIGHT_FUNNEL and move > >> coresight-replicator.o under CORESIGHT_DYNAMIC_REPLICATOR in the Makefile. I > >> really liked your idea of making the replicator driver intelligent enough to > >> deal with both DT and platform declaration, which merges two driver into one. > >> > >> I'm obviously favouring the second option but recognise it doesn't have to be > >> part of this patchet. So for this set please rework the help text for > >> CORESIGHT_LINKS_AND_SINKS. Once we've dealt with this topic we can refactor the > >> replicator driver. > > > > I'd really like to just focus on getting CoreSight to load as modules, > > something for which this patch isn't technically required... > > The only thing I'm asking is that the config description and help text > reflect what the Makefile does. argh, wellll, it's a completely different change, and we're now completely off the modularization topic, and I'm uncomfortable doing reorgs on things I don't understand, renaming CONFIG_s, esp. when others such as the REPLICATOR, since as far as I know, that's also a link?? Kim
On 24 May 2018 at 17:30, Kim Phillips <kim.phillips@arm.com> wrote: > On Thu, 24 May 2018 09:32:48 -0600 > Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > >> On 23 May 2018 at 13:51, Kim Phillips <kim.phillips@arm.com> wrote: >> > On Tue, 22 May 2018 11:31:40 -0600 >> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote: >> > >> >> On Thu, May 17, 2018 at 08:20:19PM -0500, Kim Phillips wrote: >> >> > A coresight topology doesn't need to include links, i.e., a source can >> >> > be directly connected to a sink. As such, selecting and/or depending on >> >> > LINKS_AND_SINKS is no longer needed. >> >> >> >> I'm good with this patch but now the help text for CORESIGHT_LINKS_AND_SINKS no >> >> longer match what the config does. I see two ways to fix this: >> > >> > This patch doesn't change what the config does, it just changes what >> > other config options depend on it. >> > >> >> 1) Rework the help text. >> > >> > I don't see how, given the above. Here's the text: >> > >> > config CORESIGHT_LINKS_AND_SINKS >> > bool "CoreSight Link and Sink drivers" >> > help >> > This enables support for CoreSight link and sink drivers that are >> > responsible for transporting and collecting the trace data >> > respectively. Link and sinks are dynamically aggregated with a trace >> > entity at run time to form a complete trace path. >> > >> > What part of that becomes invalid with this patch? >> >> Looking at the new Kconfig, what sink component depend on >> CORESIGHT_LINKS_AND_SINKS? > > How does that affect the description text? The description text > doesn't insinuate any implicit dependencies or non-. Now that the depends are gone there is no correlation between this config and sinks. > >> config CORESIGHT_LINKS > > Please, not another gratuitous config name change, I've already > experienced usage regressions from the CORESIGHT_QCOM_REPLICATOR => > CORESIGHT_DYNAMIC_REPLICATOR change: Defines within subsystems are bound to change as they evolves. Most of the CoreSight subsystem was developed on the OMAP3 based beagleboard. Since then new topologies have emerged and new IP blocks came along. It is only normal that we adjust configuration options to reflect the reality of the HW the subsystem is managing. I can guide you through the history of the replicator config name change if you want - it is quite logical. Other than that and until this patchset, we haven't modified a single configuration in the 5 years the subsystem has existed. Not bad for all the churn and new IP blocks that came in. > > https://patchwork.kernel.org/patch/10206023/ > >> bool "CoreSight Link drivers" >> help >> This enables support for CoreSight link drivers that are responsible >> for transporting trace data from source to sink. Links are >> dynamically >> aggregated with other traces entities at run time to form a >> complete trace >> path. > > Oh, I see, so your point is that LINKS_AND_SINKS doesn't technically > build any sink drivers? That's completely orthogonal to removing a > dependency chain: that just tells me the name was a poor choice in the > first place maybe? I don't see where the Makefile may have built a > sink, but it may be before the move to drivers/hwtracing/coresight, or > some other reorganization. Because of the depends property carried by the sink drivers (which we are now removing), defining CORESIGHT_LINKS_AND_SINKS was mandatory to build sink drivers. That was accurate 5 years ago with the topologies that were available at that time. Now there is no point in having the define, which is why I'm asking you to make this modification. > >> >> 2) Rework CORESIGHT_LINKS_AND_SINKS to be CORESIGHT_FUNNEL and move >> >> coresight-replicator.o under CORESIGHT_DYNAMIC_REPLICATOR in the Makefile. I >> >> really liked your idea of making the replicator driver intelligent enough to >> >> deal with both DT and platform declaration, which merges two driver into one. >> >> >> >> I'm obviously favouring the second option but recognise it doesn't have to be >> >> part of this patchet. So for this set please rework the help text for >> >> CORESIGHT_LINKS_AND_SINKS. Once we've dealt with this topic we can refactor the >> >> replicator driver. >> > >> > I'd really like to just focus on getting CoreSight to load as modules, >> > something for which this patch isn't technically required... >> >> The only thing I'm asking is that the config description and help text >> reflect what the Makefile does. > > argh, wellll, it's a completely different change, and we're now > completely off the modularization topic, and I'm uncomfortable doing I don't agree with you. This is a very simple change and I even wrote down what needed to be modified. > reorgs on things I don't understand, renaming CONFIG_s, esp. when > others such as the REPLICATOR, since as far as I know, that's also a > link?? Correct, a replicator is a link and completely removed from this conversation. If this is so hard for you then simply don't make the modification - I will do it myself, something that will take me about 10 minutes (including writing the changelog). > > Kim
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index ef9cb3c164e1..83fb78651ef9 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -23,7 +23,6 @@ config CORESIGHT_LINKS_AND_SINKS config CORESIGHT_LINK_AND_SINK_TMC bool "Coresight generic TMC driver" - depends on CORESIGHT_LINKS_AND_SINKS help This enables support for the Trace Memory Controller driver. Depending on its configuration the device can act as a link (embedded @@ -33,7 +32,6 @@ config CORESIGHT_LINK_AND_SINK_TMC config CORESIGHT_SINK_TPIU bool "Coresight generic TPIU driver" - depends on CORESIGHT_LINKS_AND_SINKS help This enables support for the Trace Port Interface Unit driver, responsible for bridging the gap between the on-chip coresight @@ -44,7 +42,6 @@ config CORESIGHT_SINK_TPIU config CORESIGHT_SINK_ETBV10 bool "Coresight ETBv1.0 driver" - depends on CORESIGHT_LINKS_AND_SINKS help This enables support for the Embedded Trace Buffer version 1.0 driver that complies with the generic implementation of the component without @@ -53,7 +50,6 @@ config CORESIGHT_SINK_ETBV10 config CORESIGHT_SOURCE_ETM3X bool "CoreSight Embedded Trace Macrocell 3.x driver" depends on !ARM64 - select CORESIGHT_LINKS_AND_SINKS help This driver provides support for processor ETM3.x and PTM1.x modules, which allows tracing the instructions that a processor is executing @@ -63,7 +59,6 @@ config CORESIGHT_SOURCE_ETM3X config CORESIGHT_SOURCE_ETM4X bool "CoreSight Embedded Trace Macrocell 4.x driver" depends on ARM64 - select CORESIGHT_LINKS_AND_SINKS help This driver provides support for the ETM4.x tracer module, tracing the instructions that a processor is executing. This is primarily useful @@ -72,7 +67,6 @@ config CORESIGHT_SOURCE_ETM4X config CORESIGHT_DYNAMIC_REPLICATOR bool "CoreSight Programmable Replicator driver" - depends on CORESIGHT_LINKS_AND_SINKS help This enables support for dynamic CoreSight replicator link driver. The programmable ATB replicator allows independent filtering of the @@ -81,7 +75,6 @@ config CORESIGHT_DYNAMIC_REPLICATOR config CORESIGHT_STM bool "CoreSight System Trace Macrocell driver" depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64 - select CORESIGHT_LINKS_AND_SINKS select STM help This driver provides support for hardware assisted software
A coresight topology doesn't need to include links, i.e., a source can be directly connected to a sink. As such, selecting and/or depending on LINKS_AND_SINKS is no longer needed. Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Signed-off-by: Kim Phillips <kim.phillips@arm.com> --- drivers/hwtracing/coresight/Kconfig | 7 ------- 1 file changed, 7 deletions(-)