Andreas,
On 1/3/24 5:25 PM, Andreas Helbech Kleist wrote:
> Hi,
>
> On Thu, 2023-12-28 at 14:39 +0800, Bingbu Cao wrote:
>> Andreas,
>>
>> On 11/23/23 5:33 PM, Andreas Helbech Kleist wrote:
>>> Hi,
>>>
>>> On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@xxxxxxxxx wrote:
>>>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxx>
>>>> ---
>>>> 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.
>
> Then I suppose they can be added if a psys driver is submitted upstream
> at some point.
For IPU subsystem, the fwcom interface (IPU6 driver) is a common
interface for both isys and psys. It makes sense that making the
common interface scalable even it is only set by isys driver now.
>
> Is is it the intention to submit a psys driver as well?
Yes, we are going to submit a IPU6 psys driver, but it will take more
time to get ready.
>
> /Andreas
>
>
--
Best regards,
Bingbu Cao
