[PATCH v2 3/3] mailbox: add Microchip IPC support

[Jassi Brar]

On Fri, Oct 25, 2024 at 7:36 AM Valentina Fernandez
<valentina.fernandezalanis at microchip.com> wrote:

....
> +
> +enum ipc_irq_type {
> +       IPC_OPS_NOT_SUPPORTED   = 1,
> +       IPC_MP_IRQ              = 2,
> +       IPC_MC_IRQ              = 4,
> +};
totally unused.

> +
> +/**
> + * struct mchp_ipc_probe - IPC probe message format
> + *
> + * @hw_type:           IPC implementation available in the hardware
> + * @num_channels:      number of IPC channels available in the hardware
> + *
> + * Used to retrieve information on the IPC implementation
> + * using the SBI_EXT_IPC_PROBE SBI function id.
> + */
> +struct mchp_ipc_probe {
 same as the driver.probe(), so maybe call this microchip_mbox_info

......

> +struct mchp_ipc_cluster_cfg {
> +       void *buf_base;
> +       unsigned long buf_base_addr;
> +       int irq;
> +};
> +
> +struct ipc_chan_info {
 I suggest s/ipc_chan_info/microchip_sbi_chan and hooking it to
mbox_chan.con_priv

....

> +       unsigned long buf_base_tx_addr;
> +       unsigned long buf_base_rx_addr;
> +       unsigned long msg_buf_tx_addr;
> +       unsigned long msg_buf_rx_addr;
If these are __pa(), then phys_addr_t please.

> +       int chan_aggregated_irq;
> +       int mp_irq;
> +       int mc_irq;
> +       u32 id;
> +       u32 max_msg_size;
> +};
> +
> +struct microchip_ipc {
 Maybe s/microchip_ipc/microchip_sbi_mbox ?


> +       struct device *dev;
> +       struct mbox_chan *chans;
> +       struct mchp_ipc_cluster_cfg *cluster_cfg;
> +       struct ipc_chan_info *priv;
  replace this with 'struct mbox_chan *chan' and hook
     chan[i].con_priv = priv[i]
  this will help avoid having to EXPORT mchp_ipc_get_chan_id


> +       void *buf_base;
> +       unsigned long buf_base_addr;
phys_addr_t buf_base_addr ?

> +       struct mbox_controller controller;
> +       u8 num_channels;
this could be dropped by directly using 'controller.num_chans'

......

> +static int mchp_ipc_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
> +       const struct mchp_ipc_msg *msg = data;
> +       struct mchp_ipc_sbi_msg sbi_payload;
> +
> +       memcpy(chan_info->msg_buf_tx, msg->buf, msg->size);
> +       sbi_payload.buf_addr = chan_info->msg_buf_tx_addr;
> +       sbi_payload.size = msg->size;
> +       memcpy(chan_info->buf_base_tx, &sbi_payload, sizeof(sbi_payload));
How does this work? sizeof(sbi_payload) is more than
sizeof(*chan_info->buf_base_tx)
I think buf_base_tx needs to be u8 array of max{sizeof(struct
mchp_ipc_init), sizeof(struct mchp_ipc_sbi_msg)}, if there are
alignment requirements then maybe kmalloc that size.
Similarly for buf_base_rx.

...

> +static struct platform_driver mchp_ipc_driver = {
> +       .driver = {
> +               .name = "microchip_ipc",
> +               .of_match_table = mchp_ipc_of_match,
> +       },
> +       .probe = mchp_ipc_probe,
The driver could be built as a module, so please provide .remove()
even if you never intend to unload it.

cheers.