Re: [PATCH v6 0/4] Add Microchip IPC mailbox

[Klara Modin]

Hi,

On 2024-12-17 11:31:30 +0000, Valentina Fernandez wrote:
> Hello all,
> 
> This series adds support for the Microchip Inter-Processor Communication
> (IPC) mailbox driver.
> 
> Microchip's family of RISC-V SoCs typically has one or more clusters
> that can be configured to run in Asymmetric Multi-Processing (AMP) mode.
> 
> The Microchip IPC is used to send messages between processors using
> an interrupt signaling mechanism. The driver uses the RISC-V Supervisor
> Binary Interface (SBI) to communicate with software running in machine
> mode (M-mode) to access the IPC hardware block.
> 
> Additional details on the Microchip vendor extension and the IPC
> function IDs described in the driver can be found in the following
> documentation:
> 
> https://github.com/linux4microchip/microchip-sbi-ecall-extension
> 
> The PIC64GX MPU has a Mi-V IHC block, this will be added to the PIC64GX
> dts after the initial upstreaming [1].
> 
> [1] https://patchwork.kernel.org/project/linux-riscv/patch/20240725121609.13101-18-pierre-henry.moussay@xxxxxxxxxxxxx/
> 
> Changes in v6:
> - update bindings to explain why 'reg' is not needed with microchip,sbi-ipc compatible
> 
> Changes in v5:
> - change interrup-names property to use a list instead of pattern
> - move assitionalProperties after allOf : block
> - update reg property in dt binding example to use lowercase hex
> 
> Changes in v4:
> - specify that microchip,miv-ihc-rtl-v2 is intended for use with MIV IHC Soft IP
> - drop items array and use const value for compatible strings
> - add a contraint to microchip,ihc-chan-disabled-mask property
> - minor improvements to "'#mbox-cells' description
> 
> Changes in v3:
> - Fix incorrent formatting around '=' in dt binding examples
> - Add per compatible descriptions in dt binding
> - Add '>' in certain dt binding descriptions to keep paragraphs maintained
> - export __cpuid_to_hartid_map to compile mailbox driver as module
> - Drop unused enum ipc_irq_type
> - rename struct mchp_ipc_probe to mchp_ipc_mbox_info
> - rename struct ipc_chan_info to mchp_ipc_sbi_chan
> - rename struct microchip_ipc to mchp_ipc_sbi_mbox
> - use phys_addr_t for __pa()
> - drop mchp_ipc_get_chan_id function
> - use num_chans in mbox_controller
> - Fix buf_base_tx and buf_base_rx sizes using max and kmalloc
> 
> Changes in v2:
> - use kmalloc and __pa() instead of DMA API
> - fix size of buf_base to avoid potential buffer overflow
> - add kernel doc for exported functions (mchp_ipc_get_chan_id)
> - use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL
> - drop unnecessary blank line and fix alignment issues
> - drop of_match_ptr
> - move MODULE_DEVICE_TABLE next to the definition
> - reword subject from riscv: asm: vendorid_list to riscv: sbi: vendorid_list
> - remove the word "driver" from dt-binding commit subject
> - make interrupt-names a required property for all cases
> - add dependency on COMPILE_TEST and ARCH_MICROCHIP
> 
> Regards,
> Valentina
> 
> Valentina Fernandez (4):
>   riscv: sbi: vendorid_list: Add Microchip Technology to the vendor list
>   riscv: export __cpuid_to_hartid_map
>   dt-bindings: mailbox: add binding for Microchip IPC mailbox controller
>   mailbox: add Microchip IPC support
> 
>  .../bindings/mailbox/microchip,sbi-ipc.yaml   | 123 +++++
>  arch/riscv/include/asm/vendorid_list.h        |   1 +
>  arch/riscv/kernel/smp.c                       |   1 +
>  drivers/mailbox/Kconfig                       |  13 +
>  drivers/mailbox/Makefile                      |   2 +
>  drivers/mailbox/mailbox-mchp-ipc-sbi.c        | 504 ++++++++++++++++++
>  include/linux/mailbox/mchp-ipc.h              |  33 ++
>  7 files changed, 677 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml
>  create mode 100644 drivers/mailbox/mailbox-mchp-ipc-sbi.c
>  create mode 100644 include/linux/mailbox/mchp-ipc.h
> 
> -- 
> 2.34.1
> 

Building this driver as a module with SMP=n fails becuase the non-SMP
cpuid_to_hartid_map() depends on the non-exported boot_cpu_hartid
(discovered via randconfig).

>From the description of the driver it doesn't seem to be very useful
with SMP=n, would it make sense to have it depend on SMP rather than
exporting boot_cpu_hartid?

Regards,
Klara Modin