Re: [PATCH v4 1/2] mailbox: marvell-mhu: add Marvell MHU

[Krzysztof Kozlowski]

On 17/01/2023 18:16, Wojciech Zmuda wrote:
> From: Wojciech Bartczak <wbartczak@xxxxxxxxxxx>
> 
> Add support for Message Handling Units on Marvell OcteonTx and OcteonTx2
> platforms. The module uses mailbox controller interface to provide
> communication between SCP and AP using SCMI protocol over a single
> shared memory communication channel.
> 
> Signed-off-by: Sujeet Baranwal <sbaranwal@xxxxxxxxxxx>
> Signed-off-by: Wojciech Bartczak <wbartczak@xxxxxxxxxxx>
> Signed-off-by: Wojciech Zmuda <wzmuda@xxxxxxxxxxx>
> ---
>  MAINTAINERS                   |   8 +
>  drivers/mailbox/Kconfig       |  10 ++
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/marvell_mhu.c | 295 ++++++++++++++++++++++++++++++++++
>  4 files changed, 315 insertions(+)
>  create mode 100644 drivers/mailbox/marvell_mhu.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7af5cd38d7c1..650a198cce24 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11322,6 +11322,14 @@ S:	Supported
>  F:	Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>  F:	drivers/mmc/host/sdhci-xenon*
>  
> +MARVELL MHU MAILBOX DRIVER
> +M:	Sujeet Baranwal <sbaranwal@xxxxxxxxxxx>
> +M:	Sunil Goutham <sgoutham@xxxxxxxxxxx>
> +M:	Wojciech Bartczak <wbartczak@xxxxxxxxxxx>
> +L:	linux-kernel@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	drivers/mailbox/mvl_mhu.c
> +
>  MATROX FRAMEBUFFER DRIVER
>  L:	linux-fbdev@xxxxxxxxxxxxxxx
>  S:	Orphan
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index c9fc06c7e685..af916007db2c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -160,6 +160,16 @@ config MAILBOX_TEST
>  	  Test client to help with testing new Controller driver
>  	  implementations.
>  
> +config MARVELL_MHU
> +	tristate "Marvell Message Handling Unit"
> +	depends on OF
> +	help
> +	  This driver adds support for Marvell MHU controller.
> +	  The MHU mailbox is used by SCMI for OcteonTX and OcteonTX2
> +	  to control various aspects of platform operation like cpufreq
> +	  or sensors. The controller supports single channel to SCP.
> +	  Say Y here to build this module. If unsure say N.
> +
>  config POLARFIRE_SOC_MAILBOX
>  	tristate "PolarFire SoC (MPFS) Mailbox"
>  	depends on HAS_IOMEM
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index c2089f04887e..50468444123b 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -41,6 +41,8 @@ obj-$(CONFIG_BCM_PDC_MBOX)	+= bcm-pdc-mailbox.o
>  
>  obj-$(CONFIG_BCM_FLEXRM_MBOX)	+= bcm-flexrm-mailbox.o
>  
> +obj-$(CONFIG_MARVELL_MHU)	+= marvell_mhu.o
> +
>  obj-$(CONFIG_POLARFIRE_SOC_MAILBOX)	+= mailbox-mpfs.o
>  
>  obj-$(CONFIG_QCOM_APCS_IPC)	+= qcom-apcs-ipc-mailbox.o
> diff --git a/drivers/mailbox/marvell_mhu.c b/drivers/mailbox/marvell_mhu.c
> new file mode 100644
> index 000000000000..82f9f9eefe55
> --- /dev/null
> +++ b/drivers/mailbox/marvell_mhu.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Marvell Message Handling Unit
> + *
> + * Copyright (c) 2023 Marvell
> + *
> + * https://spdx.org/licenses/GPL-2.0-only.html

Drop

> + */
> +
> +#define DRIVER_NAME	"marvell-mhu"
> +
> +#define pr_fmt(fmt)	DRIVER_NAME ": " fmt
> +
> +

One blank line.

> +#include <linux/pci.h>
> +#include <linux/acpi.h>
> +#include <linux/mailbox_controller.h>
> +
> +#define BAR0		0
> +#define SCP_INDEX	0x0
> +#define DEV_AP0		0x2
> +#define SCP_TO_AP_INTERRUPT	2
> +#define INT_VECS_MIN	0
> +#define INT_VECS_MAX	3
> +
> +#define XCPX_DEVY_XCP_MBOX_LINT_OFFSET	0x000E1C00
> +#define XCP_TO_DEV_XCP_MBOX_LINT(xcp_core, device_id)  \
> +	(XCPX_DEVY_XCP_MBOX_LINT_OFFSET | \
> +	((uint64_t)(xcp_core) << 36) | \
> +	((uint64_t)(device_id) << 4))
> +
> +#define AP0_TO_SCP_MBOX_LINT    XCP_TO_DEV_XCP_MBOX_LINT(SCP_INDEX, DEV_AP0)
> +
> +/*
> + * Doorbell-Register: XCP(0..1)_DEV(0..7)_XCP_MBOX
> + * Communication data from devices to XCP. When written, sets
> + * XCP(0..1)_DEV(0..7)_XCP_MBOX.
> + * PS: it doesn't matter what is written into this register,
> + * Attempting to writing 'anything' would cause an interrupt
> + * to the target!
> + */
> +
> +#define XCPX_DEVY_XCP_MBOX_OFFSET	0x000E1000
> +#define XCP_TO_DEV_XCP_MBOX(xcp_core, device_id) \
> +	(XCPX_DEVY_XCP_MBOX_OFFSET | \
> +	((uint64_t)(xcp_core) << 36) | \
> +	((uint64_t)(device_id) << 4))
> +
> +/* AP0-to-SCP doorbell */
> +#define AP0_TO_SCP_MBOX         XCP_TO_DEV_XCP_MBOX(SCP_INDEX, DEV_AP0)
> +
> +/*  Register offset: Enable interrupt from SCP to AP */
> +#define XCP0_XCP_DEV0_MBOX_RINT_ENA_W1S	0x000D1C40
> +#define XCP0_XCP_DEV1_MBOX_RINT_ENA_W1S	0x000D1C50
> +#define XCP0_XCP_DEV2_MBOX_RINT_ENA_W1S	0x000D1C60
> +#define XCP0_XCP_DEV3_MBOX_RINT_ENA_W1S	0x000D1C70
> +
> +/* Rx interrupt from SCP to Non-secure AP (linux kernel) */
> +#define XCPX_XCP_DEVY_MBOX_RINT_OFFSET 0x000D1C00
> +#define XCPX_XCP_DEVY_MBOX_RINT(xcp_core, device_id) \
> +	(XCPX_XCP_DEVY_MBOX_RINT_OFFSET | \
> +	((uint64_t)(xcp_core) << 36) | \
> +	((uint64_t)(device_id) << 4))
> +
> +/* The interrupt status register */
> +#define SCP_TO_AP0_MBOX_RINT  XCPX_XCP_DEVY_MBOX_RINT(SCP_INDEX, DEV_AP0)
> +
> +#define XCPX_XCP_DEVY_MBOX_RINT_OFFSET	0x000D1C00
> +#define XCPX_XCP_DEVY_MBOX_RINT(xcp_core, device_id) \
> +	(XCPX_XCP_DEVY_MBOX_RINT_OFFSET | \
> +	((uint64_t)(xcp_core) << 36) | \
> +	((uint64_t)(device_id) << 4))
> +
> +#define SCP_TO_AP0_MBOX_RINT  XCPX_XCP_DEVY_MBOX_RINT(SCP_INDEX, DEV_AP0)
> +#define SCP_TO_DEV0 XCPX_XCP_DEVY_MBOX_RINT(0, 0)
> +#define SCP_TO_DEV1 XCPX_XCP_DEVY_MBOX_RINT(0, 1)
> +#define SCP_TO_DEV2 XCPX_XCP_DEVY_MBOX_RINT(0, 2)
> +#define SCP_TO_DEV3 XCPX_XCP_DEVY_MBOX_RINT(0, 3)
> +
> +struct mhu {
> +	struct device *dev;
> +
> +	/* SCP link information */
> +	void __iomem *base; /* tx_reg, rx_reg */
> +	struct mbox_chan *chan;
> +};
> +
> +static irqreturn_t mhu_rx_interrupt(int irq, void *p)
> +{
> +	struct mhu *mhu = (struct mhu *)p;
> +	u64 val;
> +
> +	/* Read interrupt status register */
> +	val = readq_relaxed(mhu->base + SCP_TO_AP0_MBOX_RINT);
> +	if (val) {
> +		/* Clear the interrupt : Write on clear */
> +		writeq_relaxed(1ul, mhu->base + SCP_TO_AP0_MBOX_RINT);
> +		/* Poke mbox */
> +		mbox_chan_received_data(mhu->chan, (void *)&val);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static int mhu_send_data(struct mbox_chan *chan, void *data)
> +{
> +	const u64 doorbell = 0xff;  /* Arbitrary value, SCP requires a value */
> +	struct mhu *mhu = chan->con_priv;
> +
> +	/* signal SCP */
> +	iowrite64(doorbell, mhu->base + AP0_TO_SCP_MBOX);
> +
> +	return 0;
> +}
> +
> +static bool mhu_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct mhu *mhu = chan->con_priv;
> +	u64 status;
> +
> +	status = ioread64(mhu->base + XCPX_XCP_DEVY_MBOX_RINT(0, 2));
> +
> +	return status != 0;
> +}
> +
> +static int mhu_startup(struct mbox_chan *chan)
> +{
> +	struct mhu *mhu = chan->con_priv;
> +
> +	/* Enable interrupts only if there is client for data */
> +	writeq_relaxed(1ul, mhu->base + XCP0_XCP_DEV2_MBOX_RINT_ENA_W1S);
> +
> +	return 0;
> +}
> +
> +/* Define operations related to mbox */
> +static const struct mbox_chan_ops mhu_chan_ops = {
> +	.startup = mhu_startup,
> +	.send_data = mhu_send_data,
> +	.last_tx_done = mhu_last_tx_done,
> +};
> +
> +static struct mbox_chan mhu_chan = {};
> +
> +static struct mbox_controller mhu_mbox_ctrl = {

Why do you have static/file scope variables? This makes it unusuable
with multiple instances.

> +	.chans = &mhu_chan,
> +	.num_chans = 1,
> +	.txdone_irq = false,
> +	.txdone_poll = true,
> +	.txpoll_period = 100,
> +	.ops = &mhu_chan_ops,
> +};
> +
> +static int mhu_setup_mbox(struct device *dev)
> +{
> +	struct mhu *mhu;
> +	struct mbox_chan *chan;
> +
> +	mhu = dev_get_drvdata(dev);
> +
> +	chan = &mhu_mbox_ctrl.chans[0];
> +	chan->con_priv = mhu;
> +	mhu->chan = chan;
> +	mhu_mbox_ctrl.dev = dev;
> +
> +	return mbox_controller_register(&mhu_mbox_ctrl);
> +}
> +
> +static int mhu_setup_irq(struct pci_dev *pdev)
> +{
> +	struct device *dev;
> +	struct mhu *mhu;
> +	int irq, ret, nvec;
> +
> +	mhu = pci_get_drvdata(pdev);
> +	dev = &pdev->dev;
> +
> +	nvec = pci_alloc_irq_vectors(pdev, INT_VECS_MIN, INT_VECS_MAX, PCI_IRQ_MSIX);
> +	if (nvec < 0)
> +		return nvec;
> +
> +	irq = pci_irq_vector(pdev, SCP_TO_AP_INTERRUPT);
> +	if (irq < 0) {
> +		ret = irq;
> +		goto irq_err;
> +	}
> +
> +	ret = devm_request_irq(dev, irq, mhu_rx_interrupt, 0, "mvl-mhu", mhu);
> +	if (ret)
> +		goto irq_err;
> +
> +	return 0;
> +
> +irq_err:
> +	pci_free_irq_vectors(pdev);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id mhu_dt_ids[] = {
> +	{ .compatible = "marvell,octeontx-mhu" },
> +	{ .compatible = "marvell,octeontx2-mhu" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mhu_dt_ids);
> +
> +static const struct pci_device_id mhu_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xA067) },
> +	{ 0 },
> +};
> +MODULE_DEVICE_TABLE(pci, mhu_ids);
> +
> +

Only one blank line.

> +static int mhu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct mhu *mhu;
> +	struct device *dev;
> +	int ret;
> +
> +	/* This driver doesn't support ACPI */
> +	if (!acpi_disabled)
> +		return -ENODEV;

Why do you need this? It is the first driver doing such dance.

> +
> +	dev = &pdev->dev;

This goes to declaration.

> +	/* Enable only configured instances */

Hm, what does it mean? Can you reach here without OF bind? How the
driver would bind then? Via PCI ids?

> +	if (!dev->of_node)
> +		return -ENODEV;
> +	if (!of_match_node(mhu_dt_ids, dev->of_node))
> +		return -EINVAL;
> +
> +	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
> +	if (!mhu)
> +		return -ENOMEM;
> +
> +	/* Enable PCI device */

Drop, obvious.

> +	pci_set_drvdata(pdev, mhu);
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_request_region(pdev, BAR0, DRIVER_NAME);
> +	if (ret)
> +		return ret;
> +
> +	mhu->base = pcim_iomap(pdev, BAR0, pci_resource_len(pdev, BAR0));
> +	if (!mhu->base)
> +		return -EINVAL;
> +
> +	/* Configure IRQ from SCP to AP */
> +	ret = mhu_setup_irq(pdev);
> +	if (ret)
> +		goto irq_err;
> +	/* Configure mbox controller */
> +	ret = mhu_setup_mbox(dev);
> +	if (!ret) /* Success */

Drop comment, obvious. Anyway this is not Linux coding style. Return 0
or jump to error handling block.

> +		return 0;
> +
> +	/* In case of error, release the resources */

Drop comment, obvious.

> +	pci_free_irq_vectors(pdev);
> +irq_err:
> +	pci_release_region(pdev, BAR0);
> +
> +	return ret;
> +}
> +
> +static void mhu_remove(struct pci_dev *pdev)

Functions should have some local/driver specific name. Not generic.
> +{
> +	struct mhu *mhu;
> +
> +	mhu = pci_get_drvdata(pdev);
> +	mbox_controller_unregister(&mhu_mbox_ctrl);
> +
> +	pci_free_irq_vectors(pdev);
> +	pcim_iounmap(pdev, mhu->base);
> +	pci_release_region(pdev, BAR0);
> +}
> +
> +static struct pci_driver mhu_driver = {

Variables should have some local/driver specific name.

> +	.name = DRIVER_NAME,
> +	.id_table = mhu_ids,
> +	.probe = mhu_probe,
> +	.remove = mhu_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = mhu_dt_ids,
> +	},
> +};
> +
> +module_pci_driver(mhu_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Marvell Message Handling Unit");
> +MODULE_AUTHOR("Sujeet Baranwal <sbaranwal@xxxxxxxxxxx>");
> +MODULE_AUTHOR("Wojciech Bartczak <wbartczak@xxxxxxxxxxx>");

Best regards,
Krzysztof