Re: [PATCH v1 2/2] coresight: cti: Add Qualcomm extended CTI support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/09/2024 14:18, Mao Jinlong wrote:
> The QCOM extended CTI is a heavily parameterized version of ARM’s CSCTI.
> It allows a debugger to send to trigger events to a processor or to send
> a trigger event to one or more processors when a trigger event occurs
> on another processor on the same SoC, or even between SoCs. For Qualcomm
> extended CTI, it supports up to 128 triggers.
> 
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>  .../hwtracing/coresight/coresight-cti-core.c  |  75 +++++++----
>  .../coresight/coresight-cti-platform.c        |  16 ++-
>  .../hwtracing/coresight/coresight-cti-sysfs.c | 124 ++++++++++++++----
>  drivers/hwtracing/coresight/coresight-cti.h   | 123 +++++++++++------
>  4 files changed, 239 insertions(+), 99 deletions(-)


>  
>  /*
> - * Device registers
> - * 0x000 - 0x144: CTI programming and status
> - * 0xEDC - 0xEF8: CTI integration test.
> - * 0xF00 - 0xFFC: Coresight management registers.
> + * CTI CSSoc 600 has a max of 32 trigger signals per direction.
> + * CTI CSSoc 400 has 8 IO triggers - other CTIs can be impl def.
> + * Max of in and out defined in the DEVID register.
> + * - pick up actual number used from .dts parameters if present.
>   */
> -/* CTI programming registers */
> +#define CTIINOUTEN_MAX		128
> +
>  #define CTICONTROL		0x000
> -#define CTIINTACK		0x010
> -#define CTIAPPSET		0x014
> -#define CTIAPPCLEAR		0x018
> -#define CTIAPPPULSE		0x01C
> -#define CTIINEN(n)		(0x020 + (4 * n))
> -#define CTIOUTEN(n)		(0x0A0 + (4 * n))
> -#define CTITRIGINSTATUS		0x130
> -#define CTITRIGOUTSTATUS	0x134
> -#define CTICHINSTATUS		0x138
> -#define CTICHOUTSTATUS		0x13C
> -#define CTIGATE			0x140
> -#define ASICCTL			0x144
> -/* Integration test registers */
> -#define ITCHINACK		0xEDC /* WO CTI CSSoc 400 only*/
> -#define ITTRIGINACK		0xEE0 /* WO CTI CSSoc 400 only*/
> -#define ITCHOUT			0xEE4 /* WO RW-600 */
> -#define ITTRIGOUT		0xEE8 /* WO RW-600 */
> -#define ITCHOUTACK		0xEEC /* RO CTI CSSoc 400 only*/
> -#define ITTRIGOUTACK		0xEF0 /* RO CTI CSSoc 400 only*/
> -#define ITCHIN			0xEF4 /* RO */
> -#define ITTRIGIN		0xEF8 /* RO */
> +
>  /* management registers */
>  #define CTIDEVAFF0		0xFA8
>  #define CTIDEVAFF1		0xFAC
>  
> -/*
> - * CTI CSSoc 600 has a max of 32 trigger signals per direction.
> - * CTI CSSoc 400 has 8 IO triggers - other CTIs can be impl def.
> - * Max of in and out defined in the DEVID register.
> - * - pick up actual number used from .dts parameters if present.
> - */
> -#define CTIINOUTEN_MAX		32
> +static const int cti_normal_offset[] = {

Uh? Why do you add data definitions into header? These NEVER go to
headers, for obvious reasons.

> +	0x010,		/* CTIINTACK */


>  /**
>   * Group of related trigger signals
> @@ -67,7 +109,7 @@
>   */
>  struct cti_trig_grp {
>  	int nr_sigs;
> -	u32 used_mask;
> +	DECLARE_BITMAP(used_mask, CTIINOUTEN_MAX);
>  	int sig_types[];
>  };
>  
> @@ -146,9 +188,9 @@ struct cti_config {
>  	bool hw_powered;
>  
>  	/* registered triggers and filtering */
> -	u32 trig_in_use;
> -	u32 trig_out_use;
> -	u32 trig_out_filter;
> +	DECLARE_BITMAP(trig_in_use, CTIINOUTEN_MAX);
> +	DECLARE_BITMAP(trig_out_use, CTIINOUTEN_MAX);
> +	DECLARE_BITMAP(trig_out_filter, CTIINOUTEN_MAX);
>  	bool trig_filter_enable;
>  	u8 xtrig_rchan_sel;
>  
> @@ -179,6 +221,7 @@ struct cti_drvdata {
>  	struct cti_config config;
>  	struct list_head node;
>  	void (*csdev_release)(struct device *dev);
> +	bool	is_extended_cti;

Why different indentation than everything else there? Please write code
consistent with existing style.



Best regards,
Krzysztof