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.wilczyn...@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.iq...@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
