Re: [PATCH v2 06/15] media: intel/ipu6: add syscom interfaces between firmware and driver

[Bingbu Cao <bingbu.cao@linux.intel.com>]

Andreas,

On 11/23/23 5:33 PM, Andreas Helbech Kleist wrote:
> Hi,
> 
> On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> Syscom is an inter-process(or) communication mechanism between an IPU
>> and host. Syscom uses message queues for message exchange between IPU
>> and host. Each message queue has its consumer and producer, host
>> queue
>> messages to firmware as the producer and then firmware to dequeue the
>> messages as consumer and vice versa. IPU and host use shared
>> registers
>> or memory to reside the read and write indices which are updated by
>> consumer and producer.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> ---
>>  drivers/media/pci/intel/ipu6/ipu6-fw-com.c | 411
>> +++++++++++++++++++++
>>  drivers/media/pci/intel/ipu6/ipu6-fw-com.h |  47 +++
>>  2 files changed, 458 insertions(+)
>>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-fw-com.c
>>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-fw-com.h
> ...
>> +struct ipu6_fw_com_context {
>> +       struct ipu6_bus_device *adev;
>> +       void __iomem *dmem_addr;
>> +       int (*cell_ready)(struct ipu6_bus_device *adev);
>> +       void (*cell_start)(struct ipu6_bus_device *adev);
> 
> Why are cell_ready and cell_start function pointers? They seem to
> always be set to query_sp and start_sp.
> 
>> +
>> +       void *dma_buffer;
>> +       dma_addr_t dma_addr;
>> +       unsigned int dma_size;
>> +       unsigned long attrs;
>> +
>> +       struct ipu6_fw_sys_queue *input_queue;  /* array of host to
>> SP queues */
>> +       struct ipu6_fw_sys_queue *output_queue; /* array of SP to
>> host */
>> +
>> +       u32 config_vied_addr;
>> +
>> +       unsigned int buttress_boot_offset;
>> +       void __iomem *base_addr;
>> +};
>> +
>> +#define FW_COM_WR_REG 0
>> +#define FW_COM_RD_REG 4
>> +
>> +#define REGMEM_OFFSET 0
>> +#define TUNIT_MAGIC_PATTERN 0x5a5a5a5a
>> +
>> +enum regmem_id {
>> +       /* pass pkg_dir address to SPC in non-secure mode */
>> +       PKG_DIR_ADDR_REG = 0,
>> +       /* Tunit CFG blob for secure - provided by host.*/
>> +       TUNIT_CFG_DWR_REG = 1,
>> +       /* syscom commands - modified by the host */
>> +       SYSCOM_COMMAND_REG = 2,
>> +       /* Store interrupt status - updated by SP */
>> +       SYSCOM_IRQ_REG = 3,
>> +       /* first syscom queue pointer register */
>> +       SYSCOM_QPR_BASE_REG = 4
>> +};
>> +
>> +enum message_direction {
>> +       DIR_RECV = 0,
>> +       DIR_SEND
>> +};
> 
> Not used?

Ack, will remove.

> 
> 
> ...
>> +struct ipu6_fw_com_cfg {
>> +       unsigned int num_input_queues;
>> +       unsigned int num_output_queues;
>> +       struct ipu6_fw_syscom_queue_config *input;
>> +       struct ipu6_fw_syscom_queue_config *output;
>> +
>> +       unsigned int dmem_addr;
>> +
>> +       /* firmware-specific configuration data */
>> +       void *specific_addr;
>> +       unsigned int specific_size;
>> +       int (*cell_ready)(struct ipu6_bus_device *adev);
>> +       void (*cell_start)(struct ipu6_bus_device *adev);
>> +
>> +       unsigned int buttress_boot_offset;
> 
> This seems to always be 0 (set by ipu6-fw-isys.c), seems to be trivial
> to remove.

All these fields are all used to extend for psys driver.

> 
> /Andreas
> 

-- 
Best regards,
Bingbu Cao