Re: [RFC PATCH v2 05/19] firmware: thead: Add AON firmware protocol driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/12/2024 13:55, Michal Wilczynski wrote:
> The T-Head TH1520 SoC uses an E902 co-processor running Always-On (AON)
> firmware to manage power, clock, and other system resources [1]. This
> patch introduces a driver implementing the AON firmware protocol,
> allowing the Linux kernel to communicate with the firmware via mailbox
> channels.  Through an RPC-based interface, the kernel can initiate power
> state transitions, update resource configurations, and perform other
> AON-related tasks.
> 
> Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS                                   |   2 +
>  drivers/firmware/Kconfig                      |   9 +
>  drivers/firmware/Makefile                     |   1 +
>  drivers/firmware/thead,th1520-aon.c           | 203 ++++++++++++++++++
>  .../linux/firmware/thead/thead,th1520-aon.h   | 186 ++++++++++++++++
>  5 files changed, 401 insertions(+)
>  create mode 100644 drivers/firmware/thead,th1520-aon.c
>  create mode 100644 include/linux/firmware/thead/thead,th1520-aon.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7705d1b6dd7a..42aef66bd257 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20196,10 +20196,12 @@ F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
>  F:	Documentation/devicetree/bindings/power/thead,th1520-power.yaml
>  F:	arch/riscv/boot/dts/thead/
>  F:	drivers/clk/thead/clk-th1520-ap.c
> +F:	drivers/firmware/thead,th1520-aon.c
>  F:	drivers/mailbox/mailbox-th1520.c
>  F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
>  F:	drivers/pinctrl/pinctrl-th1520.c
>  F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
> +F:	include/linux/firmware/thead/thead,th1520-aon.h
>  
>  RNBD BLOCK DRIVERS
>  M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 71d8b26c4103..e08e01de3ee3 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -212,6 +212,15 @@ config SYSFB_SIMPLEFB
>  
>  	  If unsure, say Y.
>  
> +config TH1520_AON_PROTOCOL
> +	tristate "Always-On firmware protocol"
> +	depends on THEAD_TH1520_MBOX

Would:
	|| COMPILE_TEST
work?

What sort of dependency is this? Build time? Runtime? If runtime, this
should be just dependency on ARCH_THEAD (or whatever is there)

> +	help
> +	  Power, clock, and resource management capabilities on the TH1520 SoC are
> +	  managed by the E902 core. Firmware running on this core communicates with
> +	  the kernel through the Always-On protocol, using hardware mailbox as a medium.
> +	  Say yes if you need such capabilities.
> +


...

> +static void th1520_aon_rx_callback(struct mbox_client *c, void *rx_msg)
> +{
> +	struct th1520_aon_chan *aon_chan =
> +		container_of(c, struct th1520_aon_chan, cl);
> +	struct th1520_aon_rpc_msg_hdr *hdr =
> +		(struct th1520_aon_rpc_msg_hdr *)rx_msg;
> +	u8 recv_size = sizeof(struct th1520_aon_rpc_msg_hdr) + hdr->size;
> +
> +	if (recv_size != sizeof(struct th1520_aon_rpc_ack_common)) {
> +		dev_err(c->dev, "Invalid ack size, not completing\n");
> +		return;
> +	}
> +
> +	memcpy(&aon_chan->ack_msg, rx_msg, recv_size);
> +	complete(&aon_chan->done);
> +}
> +

You need proper (and useful) kerneldoc for all exported functions.

> +int th1520_aon_call_rpc(struct th1520_aon_chan *aon_chan, void *msg)
> +{
> +	struct th1520_aon_rpc_msg_hdr *hdr = msg;
> +	int ret;
> +
> +	mutex_lock(&aon_chan->transaction_lock);
> +	reinit_completion(&aon_chan->done);
> +
> +	RPC_SET_VER(hdr, TH1520_AON_RPC_VERSION);
> +	RPC_SET_SVC_ID(hdr, hdr->svc);
> +	RPC_SET_SVC_FLAG_MSG_TYPE(hdr, RPC_SVC_MSG_TYPE_DATA);
> +	RPC_SET_SVC_FLAG_ACK_TYPE(hdr, RPC_SVC_MSG_NEED_ACK);
> +
> +	ret = mbox_send_message(aon_chan->ch, msg);
> +	if (ret < 0) {
> +		dev_err(aon_chan->cl.dev, "RPC send msg failed: %d\n", ret);
> +		goto out;
> +	}
> +
> +	if (!wait_for_completion_timeout(&aon_chan->done, MAX_RX_TIMEOUT)) {
> +		dev_err(aon_chan->cl.dev, "RPC send msg timeout\n");
> +		mutex_unlock(&aon_chan->transaction_lock);
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = aon_chan->ack_msg.err_code;
> +
> +out:
> +	mutex_unlock(&aon_chan->transaction_lock);
> +
> +	return th1520_aon_to_linux_errno(ret);
> +}
> +EXPORT_SYMBOL_GPL(th1520_aon_call_rpc);
> +

Here as well.

> +int th1520_aon_power_update(struct th1520_aon_chan *aon_chan, u16 rsrc,
> +			    bool power_on)
> +{
> +	struct th1520_aon_msg_req_set_resource_power_mode msg = {};
> +	struct th1520_aon_rpc_msg_hdr *hdr = &msg.hdr;
> +	int ret;
> +
> +	hdr->svc = TH1520_AON_RPC_SVC_PM;
> +	hdr->func = TH1520_AON_PM_FUNC_SET_RESOURCE_POWER_MODE;
> +	hdr->size = TH1520_AON_RPC_MSG_NUM;
> +
> +	RPC_SET_BE16(&msg.resource, 0, rsrc);
> +	RPC_SET_BE16(&msg.resource, 2,
> +		     (power_on ? TH1520_AON_PM_PW_MODE_ON :
> +				 TH1520_AON_PM_PW_MODE_OFF));
> +
> +	ret = th1520_aon_call_rpc(aon_chan, &msg);
> +	if (ret)
> +		dev_err(aon_chan->cl.dev, "failed to power %s resource %d ret %d\n",
> +			power_on ? "up" : "off", rsrc, ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(th1520_aon_power_update);
> +
> +static int th1520_aon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct th1520_aon_chan *aon_chan;
> +	struct mbox_client *cl;
> +	int ret;
> +
> +	aon_chan = devm_kzalloc(dev, sizeof(*aon_chan), GFP_KERNEL);
> +	if (!aon_chan)
> +		return -ENOMEM;
> +
> +	cl = &aon_chan->cl;
> +	cl->dev = dev;
> +	cl->tx_block = true;
> +	cl->tx_tout = MAX_TX_TIMEOUT;
> +	cl->rx_callback = th1520_aon_rx_callback;
> +
> +	aon_chan->ch = mbox_request_channel_byname(cl, "aon");
> +	if (IS_ERR(aon_chan->ch)) {
> +		ret = PTR_ERR(aon_chan->ch);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to request aon mbox chan ret %d\n",
> +				ret);

You just open-coded dev_err_probe. Syntax is:

return dev_err_probe()

> +		return ret;
> +	}
> +
> +	mutex_init(&aon_chan->transaction_lock);
> +	init_completion(&aon_chan->done);
> +
> +	platform_set_drvdata(pdev, aon_chan);
> +
> +	return devm_of_platform_populate(dev);
> +}
> +

No remove() callback to free mbox channel? Looks like a leak.



Best regards,
Krzysztof