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

Viken Dadhaniya
2025-03-03T12:47:24Z

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