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
