Re: [PATCH v2 12/34] media: iris: add video processing unit(VPU) specific register handling

[Konrad Dybcio]

On 12/18/23 12:32, Dikshita Agarwal wrote:
> Registers are defined differently for different VPUs.
> Define ops for VPU specific handling to accommodate
> different VPUs. Implement boot sequence of firmware and interrupt
> programming.
>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> ---
[...]

> +int write_register(struct iris_core *core, u32 reg, u32 value)
> +{
> +	void __iomem *base_addr;
> +	int ret;
> +
> +	ret = check_core_lock(core);
> +	if (ret)
> +		return ret;
> +
> +	base_addr = core->reg_base;
> +	base_addr += reg;
> +	writel_relaxed(value, base_addr);> +
> +	/* Make sure value is written into the register */
IIUC barriers only ensure the prior writes need to be submitted
before the next ones, they don't actually guarantee the value
arrives at the destination. You would probably want to read the
register back here to guarantee that.

> +	wmb();
> +
> +	return ret;
> +}
> +
> +int read_register(struct iris_core *core, u32 reg, u32 *value)
> +{
> +	void __iomem *base_addr;
> +
> +	base_addr = core->reg_base;
> +
> +	*value = readl_relaxed(base_addr + reg);
> +
> +	/* Make sure value is read correctly from the register */
> +	rmb();
You can drop _relaxed for that and simply use readl() instead of
this entire wrapper..

> +
> +	return 0;
> +}
> +
> +static const struct compat_handle compat_handle[] = {
> +	{
> +		.compat                  = "qcom,sm8550-iris",
> +		.init                    = init_iris3,
> +	},
> +};
> +
> +int init_vpu(struct iris_core *core)
> +{
> +	struct device *dev = NULL;
> +	int i, ret = 0;
> +
> +	dev = core->dev;
> +
> +	for (i = 0; i < ARRAY_SIZE(compat_handle); i++) {
> +		if (of_device_is_compatible(dev->of_node, compat_handle[i].compat)) {
> +			ret = compat_handle[i].init(core);
> +			if (ret)
> +				return ret;
> +			break;
> +		}
> +	}
> +
> +	if (i == ARRAY_SIZE(compat_handle))
> +		return -EINVAL;
> +
> +	return ret;
> +}
what's wrong with of_match_data?

[...]

> +
> +#define call_vpu_op(d, op, ...)			\
> +	(((d) && (d)->vpu_ops && (d)->vpu_ops->op) ? \
> +	((d)->vpu_ops->op(__VA_ARGS__)) : 0)
> +
> +struct compat_handle {
> +	const char *compat;
> +	int (*init)(struct iris_core *core);
> +};
> +
> +struct vpu_ops {
> +	int (*boot_firmware)(struct iris_core *core);
> +	int (*raise_interrupt)(struct iris_core *core);
> +};
or you can simply create functions like

boot_firmware(...)
raise_interrupt(...)

that call other functions as needed (unless there's no need if e.g.
the cores are so similar)

and drop this sugar (well, bitter sugar at least to my taste) syntax
[...]

> +	int ret;
> +	u32 value;
reverse-Christmas-tree, please
(Christmas is in a week, get festive! :D)

> +
> +	value = (u32)core->iface_q_table.device_addr;
> +	ret = write_register(core, UC_REGION_ADDR_IRIS3, value);
> +	if (ret)
> +		return ret;
> +
> +	value = SHARED_QSIZE;
> +	ret = write_register(core, UC_REGION_SIZE_IRIS3, value);
> +	if (ret)
> +		return ret;
> +
> +	value = (u32)core->iface_q_table.device_addr;
> +	ret = write_register(core, QTBL_ADDR_IRIS3, value);
> +	if (ret)
> +		return ret;
> +
> +	ret = write_register(core, QTBL_INFO_IRIS3, 0x01);
> +	if (ret)
> +		return ret;
> +
> +	value = (u32)((u64)core->iface_q_table.kernel_vaddr);
lower_32_bits()

> +	ret = write_register(core, CPU_CS_VCICMDARG0_IRIS3, value);
> +	if (ret)
> +		return ret;
> +
> +	value = (u32)((u64)core->iface_q_table.kernel_vaddr >> 32);
upper_32_bits()

> +	ret = write_register(core, CPU_CS_VCICMDARG1_IRIS3, value);
> +	if (ret)
> +		return ret;
> +
> +	if (core->sfr.device_addr) {
> +		value = (u32)core->sfr.device_addr + VIDEO_ARCH_LX;
> +		ret = write_register(core, SFR_ADDR_IRIS3, value);
> +		if (ret)
> +			return ret;
you're returning ret 3 lines below anyway

> +	}
> +
> +	return ret;
> +}
> +
> +static int boot_firmware_iris3(struct iris_core *core)
> +{
> +	u32 ctrl_init = 0, ctrl_status = 0, count = 0, max_tries = 1000;
> +	int ret;
> +
> +	ret = setup_ucregion_memory_map_iris3(core);
> +	if (ret)
> +		return ret;
> +
> +	ctrl_init = BIT(0);
this should be a named #define used inline

> +
> +	ret = write_register(core, CTRL_INIT_IRIS3, ctrl_init);
> +	if (ret)
> +		return ret;
> +
> +	while (!ctrl_status && count < max_tries) {
if you take the previous feedback into account, this can become
readl_poll_timeout()

> +		ret = read_register(core, CTRL_STATUS_IRIS3, &ctrl_status);
> +		if (ret)
> +			return ret;
> +
> +		if ((ctrl_status & CTRL_ERROR_STATUS__M_IRIS3) == 0x4) {
> +			dev_err(core->dev, "invalid setting for UC_REGION\n");
> +			break;
> +		}
> +
> +		usleep_range(50, 100);
> +		count++;
> +	}
> +
> +	if (count >= max_tries) {
> +		dev_err(core->dev, "Error booting up vidc firmware\n");
> +		return -ETIME;
> +	}

> +
> +	ret = write_register(core, CPU_CS_H2XSOFTINTEN_IRIS3, 0x1);
0x1? BIT(0)? probably a named BIT(0) that deserves its own #define?
> +	if (ret)
> +		return ret;
> +
> +	ret = write_register(core, CPU_CS_X2RPMH_IRIS3, 0x0);
similarly here

btw you can just return this

> +
> +	return ret;
> +}
> +
> +static int raise_interrupt_iris3(struct iris_core *core)
> +{
> +	return write_register(core, CPU_IC_SOFTINT_IRIS3, 1 << CPU_IC_SOFTINT_H2A_SHFT_IRIS3);
FIELD_PREP/GET, please

> +}
> +
> +static const struct vpu_ops iris3_ops = {
> +	.boot_firmware = boot_firmware_iris3,
> +	.raise_interrupt = raise_interrupt_iris3,
> +};
> +
> +int init_iris3(struct iris_core *core)
> +{
> +	core->vpu_ops = &iris3_ops;
> +
> +	return 0;
> +}
what is this dead function for?

Konrad