On Fri, Oct 25, 2024 at 7:36 AM Valentina Fernandez
<valentina.fernandezalanis@xxxxxxxxxxxxx> 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.
