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

[Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>]

On 1/27/2025 12:36 PM, Krzysztof Kozlowski wrote:
> 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
>  

Updated in V3.

> ...
>
> > +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
Updated in V3.

> > +
> > +	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.

Corrected in V3.

> > +	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?

Removed in V3.

> > +		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'

Updated in V3.

> Best regards,
> Krzysztof