Re: [PATCH 5/5] ARM: i.MX8MM: add Hexagon Geosystems GS05

Ahmad Fatoum
2026-02-06T13:47:46Z

Hi,

On 2/5/26 4:45 PM, Marco Felsch wrote:
> This adds support for the Hexagon Geosystems GS05 which is part of the
> System1600 platform.
>
> Co-developed-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>

fyi, correct procedure for Linux would be for co-developed-by to be
followed by s-o-b, but I won't insist on it here.

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---

> +static int ar8031_phy_fixup(struct phy_device *phydev)
> +{
> + /* enable rgmii rxc skew and phy mode select to RGMII copper */
> + phy_write(phydev, 0x1d, 0x1f);
> + phy_write(phydev, 0x1e, 0x8);
> + phy_write(phydev, 0x1d, 0x00);
> + phy_write(phydev, 0x1e, 0x82ee);
> + phy_write(phydev, 0x1d, 0x05);
> + phy_write(phydev, 0x1e, 0x100);
> +
> + return 0;

barebox supports qca,clk-out-frequency, qca,clk-out-strength and
phy-mode properties to apply these fixups to the PHY.

This is useful if you have the same PHY elsewhere, e.g. behind a switch
as you can identify the specific PHY that should have these settings
applied. It also allows a faster boot as you could skip network probe in
barebox and do the fixups in Linux if the device tree lists them.

Just for your information. As you guard this behind your board
compatible, I can live with it.

> +}
> +
> +static struct hgs_machine *
> +hgs_gs05_get_board_from_legacy(const unsigned char *serial)
> +{
> + struct hgs_gs05_legacy_machine *machine = hgs_gs05_legacy_variants;
> +
> + for (; machine->revision; machine++)
> + if (serial[6] == machine->revision)
> + return &machine->machine;
> +
> + return ERR_PTR(-EINVAL);
> +}
> +
> +static struct hgs_machine *
> +hgs_gs05_select_board(const unsigned char *serial, bool legacy_format)
> +{
> + struct hgs_machine *machine = hgs_gs05_variants;
> + const struct hgs_board_revision *rev;
> +
> + /* TODO: Remove legacy handling if no longer required */
> + if (legacy_format)
> + return hgs_gs05_get_board_from_legacy(serial);
> +
> + rev = hgs_get_rev_from_part_trace((struct hgs_part_trace_code *)serial);
> + if (!rev)
> + return ERR_PTR(-EINVAL);
> +
> + for (; machine->dts_compatible; machine++)
> + if (rev->id == machine->revision)
> + return machine;
> +
> + return ERR_PTR(-EINVAL);
> +}
> +
> +static u64
> +hgs_gs05_set_efi_poll_intervall(struct device *efid, u64 new_polling_interval)
> +{
> + const char *old_interval_str;
> + char *new_interval;
> + u64 old_interval;
> +
> + old_interval_str = dev_get_param(efid->parent, "polling_interval");
> + kstrtoull(old_interval_str, 10, &old_interval);
> +
> + pr_debug("Update EFI UART-Rx poll interval: %llu ns -> %llu ns\n",
> + old_interval, new_polling_interval);
> +
> + new_interval = basprintf("%llu", new_polling_interval);
> + dev_set_param(efid->parent, "polling_interval", new_interval);
> + free(new_interval);
> +
> + return old_interval;
> +}
> +
> +/* '"' + sizeof(struct hgs_part_trace_code) + '"' + string delim '\0' */
> +#define HGS_GS05_SERIAL_NUMBER_CHARS \
> + (1 + sizeof(struct hgs_part_trace_code) + 1 + 1)
> +
> +static struct hgs_machine *hgs_gs05_get_board(struct device_d *dev)

Should there be a v2, you can use the occasion to replace all device_d
and driver_d with device and driver respectively.

> + chosen {
> + environment-emmc {
> + compatible = "barebox,environment";
> + device-path = &usdhc3, "partname:barebox-environment";
> + status = "disabled";
> + };
> + };

Does your board happen to have the barebox env GPT partition type UUID?
In that case, you could also control this via autoload_external_env()
or env.autoprobe instead of having to hardcode anything in DT.

> +/ {
> + /* compatible containing the correct revision and model is patched via board file */
> + compatible = "hgs,gs05", "fsl,imx8mm";
> + model = "Hexagon Geosystems GS05";
> +
> + aliases {
> + efiwdt = &efi_wdt;

Do you not have a kernel driver for the watchdog? You may want to use
watchdog0 and watchdog1 in that case to be able to identify them reliably.

> + };
> +
> + /*
> + * Prohibit OP-TEE from turning of the UART output if enabled via
> + * CFG_UART_BASE. To do so we need to specify a stdout-path which
> + * doesn't exist else OP-TEE turns off the UART.
> + */
> + secure-chosen {
> + stdout-path = "/this-path/does/not/exist";

... :/

> +&usdhc3 { /* eMMC */
> + assigned-clocks = <&clk IMX8MM_CLK_USDHC3_ROOT>;
> + assigned-clock-rates = <400000000>;
> + pinctrl-names = "default", "state_100mhz", "state_200mhz";
> + pinctrl-0 = <&pinctrl_usdhc3>;
> + pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> + pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> + bus-width = <8>;
> + non-removable;

no-sd;
no-sdio;

to skip their detect?

> + pinctrl_gpio1: gpio1grp {
> + fsl,pins = <
> + MX8MM_IOMUXC_GPIO1_IO10_GPIO1_IO10 0x100

This looks a bit unconventional. Can't the consumer select this group?

> +config BOARD_HGS
> + bool
> + select ARCH_IMX_ATF_PASS_BL_PARAMS
> + select ARM_SMCCC
> + select FIRMWARE_IMX_LPDDR4_PMU_TRAIN
> + select I2C_IMX_EARLY
> + select IMX8M_DRAM
> + select HABV4

Do you need to select this one? I think it would be better without to
allow it to be enabled in the defconfig without flipping HABv4 on for
all other boards?

> +static int hgs_console_open_fixup(struct device_node *root, void *context)
> +{
> + struct hgs_machine *machine = context;
> + struct device_node *console_np;
> + struct property *property;
> +
> + console_np = of_find_node_by_alias(root, machine->console_alias);
> + if (!console_np)
> + return -EINVAL;
> +
> + property = of_rename_property(console_np, "pinctrl-1", "pinctrl-0");

Does this not yield an invalid DT when passed to the kernel?
I think you want to either keep pinctrl-1 as is or shorten pinctrl-names
as well.

> + pinctrl = pinctrl_get_select(console->dev, "uart");

Nitpick: I think a name like "open" might be clearer?

Cheers,
Ahmad

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |