[PATCH v2 5/9] misc: qcom_geni: Add minicore support

[Simon Glass]

Hi Varadarajan,

On 2026-04-10T09:11:45, Varadarajan Narayanan
<varadarajan.narayanan at oss.qualcomm.com> wrote:
> misc: qcom_geni: Add minicore support
>
> The qcom_geni driver reads an ELF from storage and configures a set of
> registers and programs the firmware to the GENI Serial Engine (GENI-SE)
> wrapper device for the expected functionality.
>
> Unlike the GENI-SE wrapper found in MSM SoCs, the IPQ5210's GENI-SE
> wrapper is pre-configured for one of the functions defined in 'enum
> geni_se_protocol_type'. Hence, the firmware download is not needed.
> Only the register configuration part is needed.
>
> Earlier, the boot stages before U-Boot would configure the GENI-SE (to
> access UART/SPI etc). Since for IPQ5210 U-Boot SPL, the previous stage
> (i.e. boot ROM) doesn't do that modify the driver to do the register
> configuration part alone without reading an ELF from the storage.
>
> Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan at oss.qualcomm.com>
>
> drivers/misc/Kconfig              |   6 +++
>  drivers/misc/Makefile             |   1 +
>  drivers/misc/qcom_geni-minicore.c | 102 ++++++++++++++++++++++++++++++++++++++
>  drivers/misc/qcom_geni.c          |  90 ++++++++++++++++++++++++++++-----
>  include/soc/qcom/geni-se.h        |   2 +
>  include/soc/qcom/qup-fw-load.h    |  15 ++++++
>  6 files changed, 204 insertions(+), 12 deletions(-)

> diff --git a/drivers/misc/qcom_geni.c b/drivers/misc/qcom_geni.c
> @@ -163,16 +170,44 @@ static void geni_config_common_control(...)
> +static int load_se_firmware(struct qup_se_rsc *rsc, bool elf, void *info)
> +{
> +     ...
> +     if (elf) {
> +             hdr = info;
> +             ...
> +     } else if (IS_ENABLED(CONFIG_QCOM_GENI_MINICORE)) {

If elf is false and CONFIG_QCOM_GENI_MINICORE is disabled some
variables are used uninitialised. Please add an else clause that
returns an error.

> diff --git a/drivers/misc/qcom_geni.c b/drivers/misc/qcom_geni.c
> @@ -377,15 +413,22 @@ int qcom_geni_load_firmware(...)
> +     if (IS_ELF(*(Elf32_Ehdr *)fw)) {
> +             ...
> +             elf = 1;
> +             info = hdr;
> +     } else {
> +             elf = 0;
> +             info = fw;
> +     }

Using 1/0 for a bool is a bit odd. Please use true/false.

> diff --git a/drivers/misc/qcom_geni.c b/drivers/misc/qcom_geni.c
> @@ -492,7 +540,11 @@ static int probe_children_load_firmware(...)
> +#if CONFIG_XPL_BUILD
> +int qcom_geni_fw_initialise(struct udevice *dev)
> +#else
> +int qcom_geni_fw_initialise(void)
> +#endif

Having two different function signatures based on a preprocessor
conditional makes future maintenance harder. Consider always passing
dev and ignoring it when unused.

Regards,
Simon