Re: [RFC PATCH v3 0/9] CoreSight complex config support; ETM strobing

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Good day Mike,

On Thu, Dec 24, 2020 at 07:20:57PM +0000, Mike Leach wrote:
> Hi MAthieu,
> 
> Thanks for the review as ever. Sorry its taken me a bit of time to
> respond, but there has been a bit of reworking to do.
> 

Ok

> I certainly agree with the _desc / _csdev convention, and the follow
> up set should address that.

Ok

> The locking is also reconfigured as mentioned  - so that it is simpler.
> 

Ok

> The general rules have been tightened. I was trying to allow features
> to be enabled independently of configurations when driving the system
> from configfs / sysfs rather than perf. But in the end there is little
> value in that approach.
>

Ok
 
> As mentioned I have been working on follow up patches - which should
> be able to be split into smaller changesets. One is the dynamic
> loading of configurations, which will be either by loadable module, or
> configfs. The latter will allow the user to mkdir in the configs
> directory, and then load a binary blob via a "load" file. I do not
> believe that it is practicable to define an entire set of features
> with registers and configs via configfs - given that it is relatively
> inflexible in terms of adding files dynamically from a kernel
> perspective.

My view of the changes you are making is likely incomplete and as such it is
probably a good idea to wait for another revision before providing further
comments.  

Let me know if there are things you need clarifications on that I have
missed in your replies.

Thanks,
Mathieu

> 
> That said, it may be possible later to add in a way to defne a
> configuration via configfs using previously defined named features
> only.
> 
> Regards
> 
> Mike
> 
> 
> 
> On Thu, 26 Nov 2020 at 18:52, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > Hi Mike,
> >
> > On Fri, Oct 30, 2020 at 05:56:46PM +0000, Mike Leach wrote:
> > > This patchset introduces initial concepts in CoreSight complex system
> > > configuration support.
> > >
> > > Configurations consist of 2 elements:-
> > > 1) Features - programming combinations for devices, applied to a class of
> > > device on the system (all ETMv4), or individual devices.
> > > 2) Configurations - a set of programmed features used when the named
> > > configuration is selected.
> > >
> > > Features and configurations are declared as a data table, a set of register,
> > > resource and parameter requirements. Features and configurations are loaded
> > > into the system by the virtual cs_syscfg device. This then matches features
> > > to any registered devices and loads the feature into them.
> > >
> > > Individual device classes that support feature and configuration register
> > > with cs_syscfg.
> > >
> > > Once loaded a configuration can be enabled for a specific trace run.
> > > Configurations are registered with the perf cs_etm event as entries in
> > > cs_etm/cs_config. These can be selected on the perf command line as follows:-
> > >
> > > perf record -e cs_etm/<config_name>/ ...
> > >
> > > This patch set has one pre-loaded configuration and feature.
> > > A named "strobing" feature is provided for ETMv4.
> > > A named "autofdo" configuration is provided. This configuration enables
> > > strobing on any ETM in used.
> > >
> > > Thus the command:
> > > perf record -e cs_etm/autofdo/ ...
> > >
> > > will trace the supplied application while enabling the "autofdo" configuation
> > > on each ETM as it is enabled by perf. This in turn will enable strobing for
> > > the ETM - with default parameters. Parameters can be adjusted using configfs.
> > >
> > > The sink used in the trace run will be automatically selected.
> > >
> > > A configuation can supply up to 15 of preset parameter values, which will
> > > subsitute in parameter values for any feature used in the configuration.
> > >
> > > Selection of preset values as follows
> > > perf record -e cs_etm/autofdo,preset=1/ ...
> > >
> > > (valid presets 1-N, where N is the number supplied in the configuration, not
> > > exceeding 15. preset=0 is the same as not selecting a preset.)
> > >
> > > Applies to coresight/next (5.10-rc1 base)
> >
> > I am done reviewing this third revision.  I commented on a fair amount of things
> > but that isn't unusual with this much code, especially when it is new code that
> > doesn't have a precedent.
> >
> > As I stated before I think the general architecture of the feature is sound.  The
> > concepts of XYZ_desc and XYZ_csdev have now sunk in and I quite like it.  It
> > will be really good when the naming convention is straighten out.
> >
> > I spent a long time thinking about creating configuration and features on the fly from
> > configfs.  It is definitely not something that needs to be part of this set, nor
> > have to be implemented any time soon, but is this something that is on your
> > radar?
> >
> > Lastly the "RFC" should be dropped for the fourth iteration, we are way passed
> > that point now.
> >
> > Thanks,
> > Mathieu
> >
> > >
> > > Changes since v2:
> > > 1) Added documentation file.
> > > 2) Altered cs_syscfg driver to no longer be coresight_device based, and moved
> > > to its own custom bus to remove it from the main coresight bus. (Mathieu)
> > > 3) Added configfs support to inspect and control loaded configurations and
> > > features. Allows listing of preset values (Yabin Cui)
> > > 4) Dropped sysfs support for adjusting feature parameters on the per device basis,
> > > in favour of a single point adjustment in configfs that is pushed to all device
> > > instances.
> > > 5) Altered how the config and preset command line options are handled in perf and
> > > the drivers. (Mathieu and Suzuki).
> > > 6) Fixes for various issues and technical points (Mathieu, Yabin)
> > >
> > > Changes since v1:
> > > 1) Moved preloaded configurations and features out of individual drivers.
> > > 2) Added cs_syscfg driver to manage configurations and features. Individual
> > > drivers register with cs_syscfg indicating support for config, and provide
> > > matching information that the system uses to load features into the drivers.
> > > This allows individual drivers to be updated on an as needed basis - and
> > > removes the need to consider devices that cannot benefit from configuration -
> > > static replicators, funnels, tpiu.
> > > 3) Added perf selection of configuarations.
> > > 4) Rebased onto the coresight module loading set.
> > >
> > >
> > > To follow in future revisions / sets:-
> > > a) load of additional config and features by loadable module.
> > > b) load of additional config and features by configfs
> > > c) enhanced resource management for ETMv4 and checking features have sufficient
> > > resources to be enabled.
> > > d) ECT and CTI support for configuration and features.
> > >
> > > Mike Leach (9):
> > >   coresight: syscfg: Initial coresight system configuration
> > >   coresight: syscfg: Add registration and feature loading for cs devices
> > >   coresight: config: Add configuration and feature generic functions
> > >   coresight: etm-perf: update to handle configuration selection
> > >   coresight: etm4x: Add complex configuration handlers to etmv4
> > >   coresight: config: Add preloaded configurations
> > >   coresight: syscfg: Add initial configfs support.
> > >   coresight: syscfg: Allow update of feature params from configfs
> > >   coresight: docs: Add documentation for CoreSight config.
> > >
> > >  .../trace/coresight/coresight-config.rst      | 230 ++++++
> > >  Documentation/trace/coresight/coresight.rst   |  16 +
> > >  drivers/hwtracing/coresight/Makefile          |   6 +-
> > >  .../coresight/coresight-cfg-preload.c         | 160 +++++
> > >  .../hwtracing/coresight/coresight-config.c    | 392 +++++++++++
> > >  .../hwtracing/coresight/coresight-config.h    | 311 +++++++++
> > >  drivers/hwtracing/coresight/coresight-core.c  |  18 +-
> > >  .../hwtracing/coresight/coresight-etm-perf.c  | 166 ++++-
> > >  .../hwtracing/coresight/coresight-etm-perf.h  |  10 +-
> > >  .../hwtracing/coresight/coresight-etm4x-cfg.c | 181 +++++
> > >  .../hwtracing/coresight/coresight-etm4x-cfg.h |  29 +
> > >  .../coresight/coresight-etm4x-core.c          |  36 +-
> > >  .../coresight/coresight-etm4x-sysfs.c         |   3 +
> > >  .../coresight/coresight-syscfg-configfs.c     | 421 +++++++++++
> > >  .../coresight/coresight-syscfg-configfs.h     |  47 ++
> > >  .../hwtracing/coresight/coresight-syscfg.c    | 656 ++++++++++++++++++
> > >  .../hwtracing/coresight/coresight-syscfg.h    |  83 +++
> > >  include/linux/coresight.h                     |   7 +
> > >  18 files changed, 2743 insertions(+), 29 deletions(-)
> > >  create mode 100644 Documentation/trace/coresight/coresight-config.rst
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-cfg-preload.c
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-config.c
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-config.h
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-etm4x-cfg.h
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-syscfg-configfs.c
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-syscfg-configfs.h
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.c
> > >  create mode 100644 drivers/hwtracing/coresight/coresight-syscfg.h
> > >
> > > --
> > > 2.17.1
> > >
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel