Re: [PATCH v3 4/5] coresight: tools: Add config file write and reader tools

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

[...]

> > > diff --git a/tools/include/uapi/coresight-config-uapi.h b/tools/include/uapi/coresight-config-uapi.h
> > > new file mode 100644
> > > index 000000000000..d051c01ea982
> > > --- /dev/null
> > > +++ b/tools/include/uapi/coresight-config-uapi.h
> > > @@ -0,0 +1,76 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (c) 2020 Linaro Limited, All rights reserved.
> > > + * Author: Mike Leach <mike.leach@linaro.org>
> > > + */
> > > +
> > > +#ifndef _CORESIGHT_CORESIGHT_CONFIG_UAPI_H
> > > +#define _CORESIGHT_CORESIGHT_CONFIG_UAPI_H
> > > +
> > > +#include <linux/types.h>
> > > +#include <asm-generic/errno-base.h>
> > > +
> > > +#include "coresight-config.h"
> > > +
> > > +/*
> > > + * Userspace versions of the configuration and feature descriptors.
> > > + * Used in the tools/coresight programs.
> > > + *
> > > + * Compatible with structures in coresight-config.h for use in
> > > + * coresight-config-file.c common reader source file.
> > > + */
> > > +
> > > +/**
> > > + * Device feature descriptor - combination of registers and parameters to
> > > + * program a device to implement a specific complex function.
> > > + *
> > > + * UAPI version - removed kernel constructs.
> > > + *
> > > + * @name:     feature name.
> > > + * @description: brief description of the feature.
> > > + * @match_flags: matching information if loading into a device
> > > + * @nr_params:   number of parameters used.
> > > + * @params_desc: array of parameters used.
> > > + * @nr_regs:  number of registers used.
> > > + * @regs_desc:        array of registers used.
> > > + */
> > > +struct cscfg_feature_desc {
> > > +     const char *name;
> > > +     const char *description;
> > > +     u32 match_flags;
> > > +     int nr_params;
> > > +     struct cscfg_parameter_desc *params_desc;
> > > +     int nr_regs;
> > > +     struct cscfg_regval_desc *regs_desc;
> > > +};
> > > +
> > > +/**
> > > + * Configuration descriptor - describes selectable system configuration.
> > > + *
> > > + * A configuration describes device features in use, and may provide preset
> > > + * values for the parameters in those features.
> > > + *
> > > + * A single set of presets is the sum of the parameters declared by
> > > + * all the features in use - this value is @nr_total_params.
> > > + *
> > > + * UAPI version - removed kernel constructs.
> > > + *
> > > + * @name:            name of the configuration - used for selection.
> > > + * @description:     description of the purpose of the configuration.
> > > + * @nr_feat_refs:    Number of features used in this configuration.
> > > + * @feat_ref_names:  references to features used in this configuration.
> > > + * @nr_presets:              Number of sets of presets supplied by this configuration.
> > > + * @nr_total_params: Sum of all parameters declared by used features
> > > + * @presets:         Array of preset values.
> > > + */
> > > +struct cscfg_config_desc {
> > > +     const char *name;
> > > +     const char *description;
> > > +     int nr_feat_refs;
> > > +     const char **feat_ref_names;
> > > +     int nr_presets;
> > > +     int nr_total_params;
> > > +     const u64 *presets; /* nr_presets * nr_total_params */
> > > +};
> >
> > I would call the above cscfg_feature_fs_desc and cscfg_config_fs_desc to make
> > sure they don't get confused with the kernel's internal structures of the same name.
> >
> 
> The issue here is that the common reader code expects structs of these names.
> 
> The alternative was to put multiple #if _KERNEL__ defines in the
> middle of the structures in the kernel headers to eliminate kernel
> only elements- which you pointed out in your comments to v2 of this
> set was a maintenence issue.
> 
> This is a least worst alternative. We have common reader code, there
> are minimal changes to the kernel headers - some of the structures in
> coresight-config.h are backeted by a __KERNEL__  define but those
> without kernel specific elements are used in full.
> 
> The cost is maintaining these two structures to be the same as the
> kernel versions - which I believe to be minimal as I do not expect the
> data format to change going forwards.
>

I agree with you - the current implementation is the least intrusive and easiest
to maintain.  Unless someone finds an alternative it is better to keep the
current solution.

> > Moreover, I would keep this file private to tools/coresight/ and rename it
> > coresight-config.h.
> >
> 
> I can and have moved it.
> Howver this file includes the kernel coresight-config.h, so renaming
> is a non-starter.
> 

Yes, you are correct.

> Thanks and Regards
> 
> Mike
> 
> > > +
> > > +#endif /* _CORESIGHT_CORESIGHT_CONFIG_UAPI_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