Re: [PATCH v2 5/8] soc: qcom: geni-se: Add support to load QUP SE Firmware via Linux subsystem

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/01/2025 11:53, Viken Dadhaniya wrote:
>  /* Common SE registers */
> @@ -891,6 +896,445 @@ int geni_icc_disable(struct geni_se *se)
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_disable);
>  
> +/**
> + * elf_phdr_valid: Function to validate elf header.
> + * @phdr: A pointer to a elf header.
> + *
> + * This function validates elf header by comparing fields

Drop "This function" and use imperative. It's redundant and you keep
using it everywherre here


...

> +static int qup_fw_load(struct qup_se_rsc *rsc, const char *fw_name)
> +{
> +	int ret;
> +	const struct firmware *fw;
> +	struct device *dev = rsc->se->dev;
> +
> +	ret = request_firmware(&fw, fw_name, dev);
> +	if (ret) {
> +		dev_err(dev, "request_firmware failed for %d: %d\n", rsc->protocol, ret);
> +		return ret;
> +	}
> +
> +	ret = (rsc->protocol != GENI_SE_NONE) ? geni_load_se_fw(rsc, fw) : -EINVAL;

Drop ternary operator. Not easy to read.

> +
> +	release_firmware(fw);
> +
> +	return ret;
> +}

> +
> +/**
> + * geni_load_se_firmware: Function to initiate firmware loading.
> + * @se: Serial engine details.
> + * @protocol: protocol from spi, i2c or uart for which firmware to
> + * be loaded
> + *
> + * This function is called from the probe function of protocol driver.
> + * if dtsi properties are configured to load QUP firmware and firmware
> + * is already not loaded, it will start firmware loading. if dtsi
> + * properties are not defined,it will skip loading firmware assuming
> + * it is already loaded by TZ.
> + *
> + * return: Return 0 if no error, else return error value.
> + */
> +int geni_load_se_firmware(struct geni_se *se,
> +			  enum geni_se_protocol_type protocol)
> +{
> +	struct qup_se_rsc rsc;
> +	const char *fw_name;
> +	int ret;
> +
> +	ret = device_property_read_string(se->wrapper->dev, "firmware-name", &fw_name);
> +	if (ret)
> +		return  -EINVAL;
> +
> +	rsc.se = se;
> +	rsc.protocol = protocol;
> +
> +	/* Set default xfer mode to FIFO*/
> +	rsc.mode = GENI_SE_FIFO;
> +	of_property_read_u32(se->dev->of_node, "qcom,xfer-mode", &rsc.mode);
> +	switch (rsc.mode) {
> +	case GENI_SE_FIFO:
> +	case GENI_SE_DMA:

How value of 2 is acceptable? Your bindings said it is not.


> +	case GENI_GPI_DMA:
> +		break;
> +	default:
> +		dev_err(se->dev, "Invalid xfer mode specified: %d\n", rsc.mode);
> +		return -EINVAL;
> +	}
> +
> +	ret = qup_fw_load(&rsc, fw_name);
> +	if (ret) {
> +		dev_err(se->dev,  "Firmware Loading failed for proto: %s Error: %d\n",
> +			protocol_name[rsc.protocol], ret);

Aren't you printing same error multiple times?

> +		return ret;
> +	}
> +
> +	dev_dbg(se->dev, "Firmware load for %s protocol is Success for xfer mode %d\n",
> +		protocol_name[rsc.protocol], rsc.mode);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(geni_load_se_firmware);
> +
>  static int geni_se_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
> index 2996a3c28ef3..289fa6675d2b 100644
> --- a/include/linux/soc/qcom/geni-se.h
> +++ b/include/linux/soc/qcom/geni-se.h
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /*
>   * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #ifndef _LINUX_QCOM_GENI_SE
> @@ -72,6 +73,19 @@ struct geni_se {
>  	struct geni_icc_path icc_paths[3];
>  };
>  
> +/**
> + * struct qup_se_rsc - Structure containing se details protocol and xfer mode
> + *
> + * @mode: transfer mode se fifo, dma or gsi.
> + * @protocol: Protocol spi or i2c or serial.
> + * @se: Pointer to the concerned serial engine.
> + */
> +struct qup_se_rsc {
> +	struct geni_se *se;
> +	enum geni_se_xfer_mode mode;
> +	enum geni_se_protocol_type protocol;
> +};
> +
>  /* Common SE registers */
>  #define GENI_FORCE_DEFAULT_REG		0x20
>  #define GENI_OUTPUT_CTRL		0x24
> @@ -531,5 +545,8 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag);
>  int geni_icc_enable(struct geni_se *se);
>  
>  int geni_icc_disable(struct geni_se *se);
> +
> +int geni_load_se_firmware(struct geni_se *se,
> +			  enum geni_se_protocol_type protocol);
>  #endif
>  #endif
> diff --git a/include/linux/soc/qcom/qup-fw-load.h b/include/linux/soc/qcom/qup-fw-load.h
> new file mode 100644
> index 000000000000..b9b58e81f5cb
> --- /dev/null
> +++ b/include/linux/soc/qcom/qup-fw-load.h
> @@ -0,0 +1,179 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef _LINUX_QCOM_QUP_FW_LOAD
> +#define _LINUX_QCOM_QUP_FW_LOAD
> +
> +#include <linux/device.h>
> +#include <linux/elf.h>
> +#include <linux/firmware.h>
> +#include <linux/kernel.h>
> +
> +/*Magic numbers*/
> +#define MAGIC_NUM_SE			0x57464553
> +
> +/* Common SE registers*/
> +#define GENI_INIT_CFG_REVISION		0x0
> +#define GENI_S_INIT_CFG_REVISION	0x4
> +#define GENI_FORCE_DEFAULT_REG		0x20
> +#define GENI_CGC_CTRL			0x28
> +#define GENI_CFG_REG0			0x100
> +
> +#define	QUPV3_SE_HW_PARAM_1		0xE28

Drop indentation after 'define'


Best regards,
Krzysztof