On 26-02-09, Sascha Hauer wrote:
> On Thu, Feb 05, 2026 at 04:45:07PM +0100, Marco Felsch wrote:
> > +#include <common.h>
> > +#include <deep-probe.h>
> > +#include <envfs.h>
> > +#include <environment.h>
> > +#include <init.h>
> > +#include <i2c/i2c.h>
> > +#include <linux/phy.h>
> > +#include <mach/imx/bbu.h>
> > +#include <mach/imx/generic.h>
> > +#include <mfd/hgs-efi.h>
> > +#include <of.h>
> > +#include <state.h>
> > +
> > +#define PHY_ID_AR8031 0x004dd074
> > +#define AR_PHY_ID_MASK 0xffffffff
> > +
> > +#define HGS_GS05_BASE_NAME "Hexagon Geosystems GS05"
> > +
> > +#define HGS_GS05_MACHINE(_revid, _compatible, _model_suffix) \
> > + HGS_MACHINE(_revid, _compatible, HGS_GS05_BASE_NAME " " _model_suffix)
> > +
> > +struct hgs_machine hgs_gs05_variants[] = {
>
> static
Sure.
> > + HGS_GS05_MACHINE(HGS_BOARD_REV_C, "hgs,gs05-rev-c", "Rev-C"),
> > + HGS_GS05_MACHINE(HGS_BOARD_REV_D, "hgs,gs05-rev-d", "Rev-D"),
> > + { /* Sentinel */ }
> > +};
> > +
> > +#define HGS_GS05_LEGACY_MACHINE(_revchar, _revid, _compatible,
> > _model_suffix) \
> > +{ \
> > + .revision = _revchar, \
> > + .machine = HGS_GS05_MACHINE(_revid, _compatible, _model_suffix) \
> > +}
> > +
> > +struct hgs_gs05_legacy_machine {
>
> static
Sure.
> > + u8 revision;
> > + struct hgs_machine machine;
> > +} hgs_gs05_legacy_variants[] = {
> > + HGS_GS05_LEGACY_MACHINE('C', HGS_BOARD_REV_C, "hgs,gs05-rev-c",
> > "Rev-C"),
> > + HGS_GS05_LEGACY_MACHINE('D', HGS_BOARD_REV_D, "hgs,gs05-rev-d",
> > "Rev-D"),
> > + { /* Sentinel */ }
> > +};
> > +
> > +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;
> > +}
> > +
> > +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;
>
> Drop sentinel and use ARRAY_SIZE?
Is this rather a personal preference or barebox coding style?
I would like to keep the sentinel-approach since we use the same
approach for other boards too which allowed us simpler code
constructions.
> > + 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;
>
> same here.
>
> [...]
>
> > +static int hgs_setup_emmc(struct hgs_machine *machine)
> > +{
> > + static const char * const mmc_commands[] = { "enh_area",
> > "write_reliability" };
> > + int partitioning_completed;
> > + char *cmd, *tmp;
> > + unsigned int i;
> > +
> > + tmp = xasprintf("%s.partitioning_completed", machine->mmc_alias);
> > + getenv_bool(tmp, &partitioning_completed);
> > + free(tmp);
> > + if (partitioning_completed)
> > + return 0;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mmc_commands); i++) {
> > + cmd = xasprintf("mmc %s /dev/%s", mmc_commands[i],
> > machine->mmc_alias);
> > + if (run_command(cmd)) {
>
> run_command("mmc %s /dev/%s", mmc_commands[i], machine->mmc_alias);
>
> Would be nice ;)
Yeah, would come in handy here, let me check this.
> > + dev_err(machine->dev,
> > + "Failed to run command: %s, skip eMMC
> > partition-complete\n", cmd);
> > + free(cmd);
> > + return -EINVAL;
> > + }
> > + free(cmd);
> > + }
> > +
> > + /* Make the final complete not part of the loop */
> > + cmd = xasprintf("mmc partition_complete /dev/%s", machine->mmc_alias);
> > + run_command(cmd);
> > + free(cmd);
> > +
> > + dev_info(machine->dev, "Initial eMMC setup completed successfully\n");
> > + return 0;
> > +}
> > +
>
> [...]
>
> > + err = imx_hab_lockdown_device(flags);
> > + if (err) {
> > + dev_err(dev, "Failed to lockdown the device\n");
> > + return err;
> > + }
> > + dev_info(dev, "Lockdown of the device succeeded\n");
> > + return 0;
> > +}
> > +postenvironment_initcall(hgs_run_first_boot_setup);
>
> Needs to be protected from running on other boards.
Guarded via the hgs_first_boot variable which is only set by
hgs_run_first_boot() which in turn gets only called via
hgs_common_boot().
>
> > +
> > +/*
> > + * Notify the PP4 at the latest point to ensure that barebox was started
> > + * properly. If this function is not reached the PP4 timeout of 10sec is
> > + * triggered which puts us into serial-downloader mode.
> > + */
> > +static int hgs_notify_pp4(void)
> > +{
> > + int ret;
> > +
> > + if (!priv)
> > + return 0;
> > +
> > + ret = gpio_request_one(priv->pp4_gpio, GPIOF_OUT_INIT_HIGH,
> > "pp4-cpu-rdy");
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +postenvironment_initcall(hgs_notify_pp4);
>
> ditto.
It's guarded by the 'if (!priv)' but this function may get removed in
v2 anyway.
> > +
> > +/*
> > + * Custom USB gadget handling since we need to get notified upon the flash
> > of
> > + * the data partition. The setup is done very late like the
> > + * usbgadget_autostart_init() since we need to ensure that the NV is loaded
> > + * properly for development case.
> > + */
> > +static int hgs_usbgadget_autostart(void)
> > +{
> > + struct usbgadget_funcs funcs = {};
> > + struct f_multi_opts *opts;
> > + int ret;
> > +
> > + if (!hgs_enable_usbgadget)
> > + return 0;
> > +
> > + /*
> > + * Some HW is missing a HW serial connecotr. Fastboot is used for file
> > + * download.
> > + */
> > + funcs.flags |= USBGADGET_EXPORT_BBU | USBGADGET_FASTBOOT |
> > USBGADGET_ACM;
> > +
> > + opts = usbgadget_prepare(&funcs);
> > + if (IS_ERR(opts))
> > + return PTR_ERR(opts);
> > +
> > + /* Custom hook to get notified once the user want to flash something */
> > + opts->fastboot_opts.cmd_flash = hgs_fastboot_cmd_flash;
> > +
> > + ret = usbgadget_register(opts);
> > + if (ret)
> > + usb_multi_opts_release(opts);
> > +
> > + return ret;
> > +}
> > +postenvironment_initcall(hgs_usbgadget_autostart);
>
> ditto.
Guarded via hgs_enable_usbgadget.
>
> > +++ b/common/boards/hgs/lib.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// SPDX-FileCopyrightText: 2025 Pengutronix
> > +// SPDX-FileCopyrightText: Leica Geosystems AG
> > +
> > +#define pr_fmt(fmt) "hgs-lib: " fmt
> > +
> > +#include <common.h>
> > +#include <linux/stddef.h>
> > +#include <linux/hex.h>
> > +
> > +#include <boards/hgs/common.h>
> > +
> > +#define HGS_BOARD_REV(_id, _str) { \
> > + .id = _id, \
> > + .str = _str, \
> > +}
> > +
> > +#define HGS_BOARD_REV_SIMPLE(_id) \
> > + HGS_BOARD_REV(HGS_BOARD_REV##_id, #_id)
> > +
> > +static const struct hgs_board_revision hgs_revision_table[] = {
> > + HGS_BOARD_REV_SIMPLE(_A),
> > + HGS_BOARD_REV_SIMPLE(_B),
> > + HGS_BOARD_REV_SIMPLE(_C),
> > + HGS_BOARD_REV_SIMPLE(_D),
> > + HGS_BOARD_REV_SIMPLE(_E),
> > +};
> > +
> > +const struct hgs_board_revision *
> > +hgs_get_rev_from_part_trace(const struct hgs_part_trace_code *code)
> > +{
> > + const struct hgs_board_revision *rev = hgs_revision_table;
> > +
> > + for (; rev->str; rev++)
>
> This iterates past the array. Use ARRAY_SIZE.
You're right! It was never noticed since we never reached the end :/
I can change it to ARRAY_SIZE() but I would like to keep the sentinel
approach and add the missing sentinel if this is fine for you.
> > + if (!memcmp(code->material_revision, rev->str,
> > + sizeof(code->material_revision)))
> > + return rev;
> > +
> > + return NULL;
> > +}
> > +
> > +const u32
> > +hgs_get_artno_from_part_trace(const struct hgs_part_trace_code *code)
> > +{
> > + __be32 res = 0;
> > + u8 tmp[8];
> > + int err;
> > +
> > + /*
> > + * Art-no. has 7-digits, add a leading '0' to align it to 8 and make
> > + * use of hex2bin
> > + */
> > + memset(tmp, '0', sizeof(tmp));
>
> tmp[0] = '0'
Sure, could also have done the intialization during the definition of
tmp.
Thanks for the review,
Marco
> > + memcpy(tmp + 1, code->art_number, sizeof(code->art_number));
> > + err = hex2bin((u8 *)&res, tmp, sizeof(res));
> > +
> > + return err ? 0 : be32_to_cpu(res);
> > +}
> > +
> > +struct hgs_machine *hgs_get_last_variant_entry(struct hgs_machine
> > *variants)
> > +{
> > + struct hgs_machine *last_entry = NULL;
> > +
> > + if (!variants->dts_compatible) {
> > + pr_warn("%s: Invalid input\n", __func__);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + for (; variants->dts_compatible; variants++)
> > + last_entry = variants;
> > +
> > + return last_entry;
> > +}
>
> Sascha
>
> --
> 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 |
>
--
#gernperDu
#CallMeByMyFirstName
Pengutronix e.K. | |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
