Re: [PATCH] soc: qcom: smd-rpm: Add trace events for SMD-RPM messages

[Konrad Dybcio]

On 9/4/26 4:23 PM, Maulik Shah wrote:
> There is no visibility on what resource votes are placed via SMD RPM making
> it difficult to identify clients that do either under voting via active set
> impacting the device functions or fail to send sleep set votes and thereby
> prevent the SoC from reaching low power modes such as vmin or vlow.

[...]

> @@ -106,6 +109,7 @@ int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm,
>  		u8 payload[];
>  	} *pkt;
>  	size_t size = sizeof(*pkt) + count;
> +	const struct clk_smd_rpm_req *req = buf;

This struct is called clk_smd_rpm_req, but it seems like at least for
the current in-kernel users, everything that's passed to
qcom_rpm_smd_write() (rpmpd, regulator) is a renamed version of this
one. We should probably rename it.

Or are there requests that differ in structure that we should be
mindful of?

>  	if (le32_to_cpu(hdr->service_type) != RPM_SERVICE_TYPE_REQUEST ||
> @@ -171,6 +178,7 @@ static int qcom_smd_rpm_callback(struct rpmsg_device *rpdev,
>  		msg_length = le32_to_cpu(msg->length);
>  		switch (le32_to_cpu(msg->msg_type)) {
>  		case RPM_MSG_TYPE_MSG_ID:
> +			msg_id = msg->msg_id;
>  			break;
>  		case RPM_MSG_TYPE_ERR:
>  			len = min_t(u32, ALIGN(msg_length, 4), sizeof(msgbuf));
> @@ -187,6 +195,8 @@ static int qcom_smd_rpm_callback(struct rpmsg_device *rpdev,
>  		buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg_length, 4);
>  	}
>  
> +	trace_rpm_smd_ack_recvd(msg_id, status);

This way only the last ack/err will be traced, is that the desired behavior?

[...]

> +TRACE_EVENT(rpm_smd_send_msg,
> +
> +	TP_PROTO(__le32 msg_id, u32 state, u32 rsc_type, u32 rsc_id,
> +		 const struct clk_smd_rpm_req *req),
> +
> +	TP_ARGS(msg_id, state, rsc_type, rsc_id, req),
> +
> +	TP_STRUCT__entry(
> +		__field(u32, msg_id)
> +		__field(u32, state)
> +		__field(u32, rsc_type)
> +		__field(u32, rsc_id)
> +		__field(u32, key)
> +		__field(u32, nbytes)
> +		__field(u32, value)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->msg_id = le32_to_cpu(msg_id);
> +		__entry->state = state;
> +		__entry->rsc_type = rsc_type;
> +		__entry->rsc_id = rsc_id;
> +		__entry->key = le32_to_cpu(req->key);
> +		__entry->nbytes = le32_to_cpu(req->nbytes);
> +		__entry->value = le32_to_cpu(req->value);

GPT points out `value` can be more than just a single u32
(via nbytes)

Konrad