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.
Is is it the intention to submit a psys driver as well?
/Andreas
