Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220714121215.22931-1-wbartczak@marvell.com> <20220714121215.22931-3-wbartczak@marvell.com>
In-Reply-To: <20220714121215.22931-3-wbartczak@marvell.com>
From:   Jassi Brar <jassisinghbrar@gmail.com>
Date:   Sun, 17 Jul 2022 10:17:46 -0500
Message-ID: <CABb+yY1fO-QPZ_YNsvHxnrhZ5+8NGa6yoZUPkvjihVaiRJcTnw@mail.gmail.com>
Subject: Re: [PATCH 2/2] mailbox: mvl_mhu: Add support for Marvell Message
 Handling Unit
To:     Wojciech Bartczak <wbartczak@marvell.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        wbartczak@gmail.com, Piyush Malgujar <pmalgujar@marvell.com>,
        Sunil Goutham <sgoutham@marvell.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jul 14, 2022 at 7:13 AM Wojciech Bartczak <wbartczak@marvell.com> wrote:

...
> +config MVL_MHU
> +       tristate "Marvell MHU Mailbox" if ARM_SCMI_PROTOCOL
> +       depends on OF && ARM64
You want to make it depend on your ARCH or COMPILE_TEST

....
> diff --git a/drivers/mailbox/mvl_mhu.c b/drivers/mailbox/mvl_mhu.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Marvell Message Handling Unit driver
This doesn't seem to be related to ARM-MHU. Please consider calling it
something else.

...
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/acpi.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/spinlock.h>
> +
please consider dropping unnecessary includes

...
> +/*
> + * 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 DONT_CARE_DATA                 0xFF
likely only one platform-specific bit needs to be set. you may want to
get that from dt/client ?

....
> +#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
maybe use more compact defines?

....
> +
> +#define MHU_CHANNEL_INDEX(mhu, chan) (chan - &mhu->chan[0])
this is unused

....
> +
> +/* Secures static data processed in the irq handler */
> +DEFINE_SPINLOCK(mhu_irq_spinlock);
Please move this in 'struct mhu'

...
> +static irqreturn_t mhu_rx_interrupt_thread(int irq, void *p)
> +{
> +       struct mhu *mhu = (struct mhu *)p;
> +       struct int_src_data_s *data = (struct int_src_data_s *)mhu->payload;
> +       u64 val, scmi_tx_cnt;
> +
> +       /*
> +        * Local copy of event counters. A mismatch of received
> +        * count value and the local copy means additional events
> +        * are being flagged that needs to be attended by AP
> +        */
> +       static u64 event_counter[INDEX_INT_SRC_NONE] = {0};
Please move this in 'struct mhu'

....
> +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));
> +       pr_debug("last_tx_done status: %#llx\n", status);
please use dev_XXX instead of pr_debug, here and elsewhere

thanks