Re: [PATCH v4 2/2] interconnect: qcom: add MSM8x60 NoC driver

[Konrad Dybcio]

On 6/6/26 3:00 PM, Herman van Hazendonk wrote:
> Add a Qualcomm interconnect driver for the MSM8x60 family modelling the
> four NoC fabrics (APPSS, System, MMSS, Daytona) that connect masters
> and slaves on these Scorpion-class SoCs.  The driver implements the
> interconnect-provider API to manage bandwidth between specific masters
> and slaves via the RPM arbitration tables.

[...]

I'll skip the bulk of this for now, as Dmitry asked you to drop the
clock interface (and do rpm writes directly from this driver, see
icc-rpm-clocks.c and smd-rpm.c)

> +/*
> + * Minimum fabric clock rate to prevent bus starvation.
> + *
> + * When no consumers request bandwidth, the rate calculation yields 0,
> + * causing fabric clocks to drop to minimum. This creates bimodal
> + * performance: fast when other subsystems (like display) happen to
> + * request bandwidth, slow otherwise.
> + *
> + * 384 MHz keeps fabric fast during concurrent MDP display scanout
> + * and USB gadget traffic. legacy vendor kernel docs: "AXI bus frequency needs to be
> + * kept at maximum value while USB data transfers are happening."
> + * 266 MHz was insufficient - USB crashed during display activity.
> + */
> +#define MSM8660_FABRIC_MIN_RATE		384000000UL	/* 384 MHz */

So, are there any issues if the USB controller is offline, can we go to
the aforementioned 266 MHz rate then?

> +
> +/*
> + * Maximum RPM ARB buffer size across all fabrics.
> + * MM fabric is largest at 23 u32 words.
> + */
> +#define MSM8660_MAX_RPM_BUF		23
> +
> +/*
> + * RPM fabric arbitration data format (from legacy vendor kernel msm_bus_fabric.c):
> + *
> + * Each u16 arb entry: bit 15 = tier (1=TIER1 high priority), bits 14-0 = BW
> + * Bandwidth is in 128KB units (bytes >> 17).
> + * Two u16 values are packed into each u32 RPM register word.
> + *
> + * Buffer layout: [bwsum pairs] [arb pairs]
> + * bwsum[slave_port] = total bandwidth to that slave
> + * arb[(tier-1)*nmasters + master_port] = per-master arbitration entry
> + */
> +#define ARB_BWMASK		0x7FFF

GENMASK(14, 0)

> +#define ARB_TIERMASK		0x8000

BIT(15)

[...]

> +/*
> + * =========================================================================

Let's drop the =s

[...]

> +/*
> + * Pack bwsum[] and arb[] arrays into the u32 RPM buffer.
> + *
> + * Two u16 values are packed per u32 word: lower 16 bits first, upper 16 next.
> + * Layout: [bwsum pairs] then [arb pairs], handling odd boundaries.
> + *
> + * This matches the legacy vendor kernel msm_bus_fabric_rpm_commit() packing algorithm.
> + */
> +static void msm8660_pack_rpm_data(const u16 *bwsum, int nslaves,
> +				  const u16 *arb, int arb_size,
> +				  u32 *buf)
> +{
> +	int i, index = 0;
> +
> +	/* Pack bwsum pairs */
> +	for (i = 0; i + 1 < nslaves; i += 2) {
> +		buf[index] = ((u32)bwsum[i + 1] << 16) | bwsum[i];
> +		index++;
> +	}

FIELD_PREP(GENMASK(31, 16), bwmon[i + 1])) |
FIELD_PREP(GENMASK(15, 0), bwsum[i]);


> +
> +	/*
> +	 * Handle boundary between bwsum and arb for odd nslaves. When the
> +	 * fabric has no master ports (arb_size == 0) the arb[0] access would
> +	 * read out of bounds; pad the lone bwsum into the low half of the
> +	 * word instead.
> +	 */
> +	if (nslaves & 1) {

& BIT (0)

> +		if (arb_size > 0) {
> +			buf[index] = ((u32)arb[0] << 16) | bwsum[i];
> +			i = 1;
> +		} else {
> +			buf[index] = bwsum[i];
> +			i = 0;
> +		}

similarly as above

[...]

> +static int msm8660_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
> +{
> +	*avg = 0;
> +	*peak = 0;
> +	return 0;
> +}

I think we can drop this

Konrad