Re: [PATCH 2/4] perf/arm-cmn: Add CMN-650 support

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

Hi Robin,

I need to go through your patches more carefully, but I do have a couple 
of comments already:

On Mon, 18 Apr 2022, Robin Murphy wrote:
> Add the identifiers and events for CMN-650, which slots into its
> evolutionary position between CMN-600 and the 700-series products.
> Imagine CMN-600 made bigger, and with most of the rough edges smoothed
> off, but that then balanced out by some bonkers PMU functionality for
> the new HN-P enhancement in CMN-650r2.
>
> Most of the CXG events are actually common to newer revisions of CMN-600
> too, so they're arguably a little late; oh well.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/perf/arm-cmn.c | 222 ++++++++++++++++++++++++++++++++---------
> 1 file changed, 176 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 9c1d82be7a2f..cce8516d465c 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -39,7 +39,7 @@
> #define CMN_CHILD_NODE_ADDR		GENMASK(27, 0)
> #define CMN_CHILD_NODE_EXTERNAL		BIT(31)
>
> -#define CMN_MAX_DIMENSION		8
> +#define CMN_MAX_DIMENSION		12

I wonder if it made sense to dynamically allocate the arrays later in the 
code instead of allocating them in stack, especially if mesh topologies 
keeps growing fast. That would probably avoid setting max dimension 
altogether if one could use num_xps, num_dns etc. Just for future 
thoughts...


> #define CMN_MAX_XPS			(CMN_MAX_DIMENSION * CMN_MAX_DIMENSION)
> #define CMN_MAX_DTMS			(CMN_MAX_XPS + (CMN_MAX_DIMENSION - 1) * 4)

<snip>

> @@ -1692,8 +1802,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 		cmn->num_dns += FIELD_GET(CMN_CI_CHILD_COUNT, reg);

How about checking if child count is bigger than the maximum mesh size 
before this for loop? It would help in case someone would work on enabling 
support for new, bigger models and would forget to update 
CMN_MAX_DIMENSION...

> 	}
>
> -	/* Cheeky +1 to help terminate pointer-based iteration later */
> -	dn = devm_kcalloc(cmn->dev, cmn->num_dns + 1, sizeof(*dn), GFP_KERNEL);
> +	/*
> +	 * Some nodes effectively have two separate types, which we'll handle
> +	 * by creating one of each internally. For a (very) safe initial upper
> +	 * bound, account for double the number of non-XP nodes.
> +	 */
> +	dn = devm_kcalloc(cmn->dev, cmn->num_dns * 2 - cmn->num_xps,
> +			  sizeof(*dn), GFP_KERNEL);
> 	if (!dn)
> 		return -ENOMEM;
>

<snip>

> @@ -1970,6 +2098,7 @@ static int arm_cmn_remove(struct platform_device *pdev)
> #ifdef CONFIG_OF
> static const struct of_device_id arm_cmn_of_match[] = {
> 	{ .compatible = "arm,cmn-600", .data = (void *)CMN600 },
> +	{ .compatible = "arm,cmn-650", .data = (void *)CMN650 },
> 	{ .compatible = "arm,ci-700", .data = (void *)CI700 },
> 	{}
> };
> @@ -1979,6 +2108,7 @@ MODULE_DEVICE_TABLE(of, arm_cmn_of_match);
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id arm_cmn_acpi_match[] = {
> 	{ "ARMHC600", CMN600 },
> +	{ "ARMHC650", CMN650 },

Not the great place for this comment but there probably isn't any better.

Based on DEN0093 v1.1, CMN's DSDT entries have been changed since CMN-600. 
ROOTNODEBASE seems to be specific for CMN-600. Moreover, if you compare 
the address maps in TRMs' Discovery chapters, you can see the difference.

I'm thinking, at the minimal the second platform_get_resource() call has 
to be skipped and zero returned in arm_cmn600_acpi_probe(), if the model 
is cmn650 (probably also for cmn-700)

> 	{}
> };
> MODULE_DEVICE_TABLE(acpi, arm_cmn_acpi_match);

Cheers, Ilkka

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