| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f20b98fb-1cbc-4211-b616-5421bf43301f@quicinc.com>
Date: Mon, 3 Mar 2025 18:17:09 +0530
From: Viken Dadhaniya <quic_vdadhani@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, <andi.shyti@...nel.org>,
<robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<gregkh@...uxfoundation.org>, <jirislaby@...nel.org>,
<broonie@...nel.or>, <andersson@...nel.org>, <konradybcio@...nel.org>,
<johan+linaro@...nel.org>, <dianders@...omium.org>,
<agross@...nel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-serial@...r.kernel.org>,
<linux-spi@...r.kernel.org>
CC: <quic_msavaliy@...cinc.com>, <quic_anupkulk@...cinc.com>
Subject: Re: [PATCH v2 5/8] soc: qcom: geni-se: Add support to load QUP SE
Firmware via Linux subsystem
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
Powered by blists - more mailing lists