[PATCH v2 12/13] sysinfo: Add driver for IOT2050 boards
Jan Kiszka
jan.kiszka at siemens.com
Wed Oct 23 06:14:42 CEST 2024
On 23.10.24 05:39, Simon Glass wrote:
> Hi Jan,
>
> On Tue, 22 Oct 2024 at 21:59, Jan Kiszka <jan.kiszka at siemens.com> wrote:
>>
>> On 22.10.24 19:00, Simon Glass wrote:
>>> On Tue, 22 Oct 2024 at 08:06, Jan Kiszka <jan.kiszka at siemens.com> wrote:
>>>>
>>>> From: Li Hua Qian <huaqian.li at siemens.com>
>>>>
>>>> This brings a sysinfo driver and DT entry for the IOT2050 board series.
>>>> It translates the board information passed from SE-Boot to SPL into
>>>> values that can be retrieved via the sysinfo API. Will is already used
>>>> to fill the SMBIOS table when booting via EFI.
>>>>
>>>> Signed-off-by: Li Hua Qian <huaqian.li at siemens.com>
>>>> [Jan: split-off as separate patch, cleanup]
>>>> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
>>>> ---
>>>> .../dts/k3-am65-iot2050-common-u-boot.dtsi | 18 +++
>>>> configs/iot2050_defconfig | 1 +
>>>> drivers/sysinfo/Kconfig | 7 +
>>>> drivers/sysinfo/Makefile | 1 +
>>>> drivers/sysinfo/iot2050.c | 143 ++++++++++++++++++
>>>> drivers/sysinfo/iot2050.h | 26 ++++
>>>> 6 files changed, 196 insertions(+)
>>>> create mode 100644 drivers/sysinfo/iot2050.c
>>>> create mode 100644 drivers/sysinfo/iot2050.h
>>>
>>> I think strlcpy() might be better than strncpy() for this case
>>>
>>
>> Ack.
>>
>>> The idea with sysinfo is that we use the same enum for all boards, so
>>> please add your new things to sysinfo.h
>>>
>>
>> See below.
>>
>>> Do you actually want all the sysinfo info in capitals? If so, that's
>>> fine, just checking...
>>>
>>
>> This is how it is being shipped already, yes.
>>
>>>>
>>>> diff --git a/arch/arm/dts/k3-am65-iot2050-common-u-boot.dtsi b/arch/arm/dts/k3-am65-iot2050-common-u-boot.dtsi
>>>> index b6d2c816acc..55337179f9f 100644
>>>> --- a/arch/arm/dts/k3-am65-iot2050-common-u-boot.dtsi
>>>> +++ b/arch/arm/dts/k3-am65-iot2050-common-u-boot.dtsi
>>>> @@ -14,6 +14,24 @@
>>>> spi0 = &ospi0;
>>>> };
>>>>
>>>> + sysinfo {
>>>> + compatible = "siemens,sysinfo-iot2050";
>>>> + /* TI_SRAM_SCRATCH_BOARD_EEPROM_START */
>>>> + offset = <0x40280000>;
>>>> + bootph-all;
>>>> +
>>>> + smbios {
>>>> + system {
>>>> + manufacturer = "SIEMENS AG";
>>>> + product = "SIMATIC IOT2050";
>>>> + };
>>>> +
>>>> + baseboard {
>>>> + manufacturer = "SIEMENS AG";
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> leds {
>>>> bootph-all;
>>>> status-led-red {
>>>> diff --git a/configs/iot2050_defconfig b/configs/iot2050_defconfig
>>>> index 2624f0cb573..574e232d119 100644
>>>> --- a/configs/iot2050_defconfig
>>>> +++ b/configs/iot2050_defconfig
>>>> @@ -141,6 +141,7 @@ CONFIG_SOC_TI=y
>>>> CONFIG_SPI=y
>>>> CONFIG_DM_SPI=y
>>>> CONFIG_CADENCE_QSPI=y
>>>> +CONFIG_SYSINFO=y
>>>> CONFIG_SYSRESET=y
>>>> CONFIG_SPL_SYSRESET=y
>>>> CONFIG_SYSRESET_TI_SCI=y
>>>> diff --git a/drivers/sysinfo/Kconfig b/drivers/sysinfo/Kconfig
>>>> index 2030e4babc9..df83df69ffb 100644
>>>> --- a/drivers/sysinfo/Kconfig
>>>> +++ b/drivers/sysinfo/Kconfig
>>>> @@ -31,6 +31,13 @@ config SYSINFO_RCAR3
>>>> help
>>>> Support querying SoC version information for Renesas R-Car Gen3.
>>>>
>>>> +config SYSINFO_IOT2050
>>>> + bool "Enable sysinfo driver for the Siemens IOT2050"
>>>> + depends on TARGET_IOT2050_A53
>>>> + default y if TARGET_IOT2050_A53
>>>> + help
>>>> + Support querying device information for Siemens IOT2050.
>>>> +
>>>> config SYSINFO_SANDBOX
>>>> bool "Enable sysinfo driver for the Sandbox board"
>>>> help
>>>> diff --git a/drivers/sysinfo/Makefile b/drivers/sysinfo/Makefile
>>>> index 680dde77fe8..26ca3150999 100644
>>>> --- a/drivers/sysinfo/Makefile
>>>> +++ b/drivers/sysinfo/Makefile
>>>> @@ -5,6 +5,7 @@
>>>> obj-y += sysinfo-uclass.o
>>>> obj-$(CONFIG_SYSINFO_GAZERBEAM) += gazerbeam.o
>>>> obj-$(CONFIG_SYSINFO_GPIO) += gpio.o
>>>> +obj-$(CONFIG_SYSINFO_IOT2050) += iot2050.o
>>>> obj-$(CONFIG_SYSINFO_RCAR3) += rcar3.o
>>>> obj-$(CONFIG_SYSINFO_SANDBOX) += sandbox.o
>>>> obj-$(CONFIG_SYSINFO_SMBIOS) += smbios.o
>>>> diff --git a/drivers/sysinfo/iot2050.c b/drivers/sysinfo/iot2050.c
>>>> new file mode 100644
>>>> index 00000000000..5359d6b8d62
>>>> --- /dev/null
>>>> +++ b/drivers/sysinfo/iot2050.c
>>>> @@ -0,0 +1,143 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) Siemens AG, 2024
>>>> + */
>>>> +
>>>> +#include <dm.h>
>>>> +#include <sysinfo.h>
>>>> +#include <net.h>
>>>> +#include <asm/arch/hardware.h>
>>>> +
>>>> +#include "iot2050.h"
>>>> +
>>>> +#define IOT2050_INFO_MAGIC 0x20502050
>>>> +
>>>> +struct iot2050_info {
>>>> + u32 magic;
>>>> + u16 size;
>>>> + char name[20 + 1];
>>>> + char serial[16 + 1];
>>>> + char mlfb[18 + 1];
>>>> + char uuid[32 + 1];
>>>> + char a5e[18 + 1];
>>>> + u8 mac_addr_cnt;
>>>> + u8 mac_addr[8][ARP_HLEN];
>>>> + char seboot_version[40 + 1];
>>>> + u8 padding[3];
>>>> + u32 ddr_size_mb;
>>>> +} __packed;
>>>> +
>>>> +/**
>>>> + * struct sysinfo_iot2050_priv - sysinfo private data
>>>> + * @info: iot2050 board info
>>>> + */
>>>> +struct sysinfo_iot2050_priv {
>>>> + struct iot2050_info *info;
>>>> +};
>>>> +
>>>> +static int sysinfo_iot2050_detect(struct udevice *dev)
>>>> +{
>>>> + struct sysinfo_iot2050_priv *priv = dev_get_priv(dev);
>>>> +
>>>> + if (priv->info == NULL || priv->info->magic != IOT2050_INFO_MAGIC)
>>>> + return -EFAULT;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sysinfo_iot2050_get_str(struct udevice *dev, int id, size_t size,
>>>> + char *val)
>>>> +{
>>>> + struct sysinfo_iot2050_priv *priv = dev_get_priv(dev);
>>>> + char byte_str[3] = {0};
>>>> + unsigned int n;
>>>> +
>>>> + switch (id) {
>>>> + case BOARD_NAME:
>>>> + strncpy(val, priv->info->name, size);
>>>> + break;
>>>> + case BOARD_SERIAL:
>>>> + strncpy(val, priv->info->serial, size);
>>>> + break;
>>>> + case BOARD_MLFB:
>>>> + strncpy(val, priv->info->mlfb, size);
>>>> + break;
>>>> + case BOARD_UUID:
>>>> + for (n = 0; n < min(size, (size_t)16); n++) {
>>>> + memcpy(byte_str, priv->info->uuid + n * 2, 2);
>>>> + val[n] = (char)hextoul(byte_str, NULL);
>>>> + }
>>>> + break;
>>>> + case BOARD_A5E:
>>>> + strncpy(val, priv->info->a5e, size);
>>>> + break;
>>>> + case BOARD_SEBOOT_VER:
>>>> + strncpy(val, priv->info->seboot_version, size);
>>>> + break;
>>>> + case BOARD_MAC_ADDR_1:
>>>> + case BOARD_MAC_ADDR_2:
>>>> + case BOARD_MAC_ADDR_3:
>>>> + case BOARD_MAC_ADDR_4:
>>>> + case BOARD_MAC_ADDR_5:
>>>> + case BOARD_MAC_ADDR_6:
>>>> + case BOARD_MAC_ADDR_7:
>>>> + case BOARD_MAC_ADDR_8:
>>>> + memcpy(val, priv->info->mac_addr[id - BOARD_MAC_ADDR_START],
>>>> + ARP_HLEN);
>>>
>>> For this, we really need another parameter to get_str(), i.e. the
>>> index, since we don't know how many MACs there will be. So how about
>>> adding a few new operations?
>>>
>>> int (*get_item_count)(struct udevice *dev, int id);
>>> int (*get_str_item)(struct udevice *dev, int id, int index, size_t
>>> size, char *val);
>>
>> Ok, makes sense.
>>
>>>
>>>> + return 0;
>>>> + case BOARD_DDR_SIZE:
>>>> + memcpy(val, &priv->info->ddr_size_mb,
>>>> + sizeof(priv->info->ddr_size_mb));
>>>> + return 0;
>>>> + default:
>>>> + return -EINVAL;
>>>> + };
>>>> +
>>>> + val[size - 1] = '\0';
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sysinfo_iot2050_get_int(struct udevice *dev, int id, int *val)
>>>> +{
>>>> + struct sysinfo_iot2050_priv *priv = dev_get_priv(dev);
>>>> +
>>>> + switch (id) {
>>>> + case BOARD_MAC_ADDR_CNT:
>>>> + *val = priv->info->mac_addr_cnt;
>>>> + return 0;
>>>> + default:
>>>> + return -EINVAL;
>>>> + };
>>>> +}
>>>> +
>>>> +static const struct sysinfo_ops sysinfo_iot2050_ops = {
>>>> + .detect = sysinfo_iot2050_detect,
>>>> + .get_str = sysinfo_iot2050_get_str,
>>>> + .get_int = sysinfo_iot2050_get_int,
>>>> +};
>>>> +
>>>> +static int sysinfo_iot2050_probe(struct udevice *dev)
>>>> +{
>>>> + struct sysinfo_iot2050_priv *priv = dev_get_priv(dev);
>>>> + unsigned long offset;
>>>> +
>>>> + offset = dev_read_u32_default(dev, "offset",
>>>> + TI_SRAM_SCRATCH_BOARD_EEPROM_START);
>>>> + priv->info = (struct iot2050_info *)offset;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct udevice_id sysinfo_iot2050_ids[] = {
>>>> + { .compatible = "siemens,sysinfo-iot2050" },
>>>> + { /* sentinel */ }
>>>> +};
>>>> +
>>>> +U_BOOT_DRIVER(sysinfo_iot2050) = {
>>>> + .name = "sysinfo_iot2050",
>>>> + .id = UCLASS_SYSINFO,
>>>> + .of_match = sysinfo_iot2050_ids,
>>>> + .ops = &sysinfo_iot2050_ops,
>>>> + .priv_auto = sizeof(struct sysinfo_iot2050_priv),
>>>> + .probe = sysinfo_iot2050_probe,
>>>> +};
>>>> diff --git a/drivers/sysinfo/iot2050.h b/drivers/sysinfo/iot2050.h
>>>> new file mode 100644
>>>> index 00000000000..f659a8282b5
>>>> --- /dev/null
>>>> +++ b/drivers/sysinfo/iot2050.h
>>>> @@ -0,0 +1,26 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Copyright (c) Siemens AG, 2024
>>>> + */
>>>> +
>>>> +#include <sysinfo.h>
>>>> +
>>>> +enum sysinfo_id_iot2050 {
>>>> + BOARD_MLFB = SYSINFO_ID_SMBIOS_SYSTEM_VERSION,
>>>> + BOARD_SERIAL = SYSINFO_ID_SMBIOS_SYSTEM_SERIAL,
>>>> + BOARD_UUID = SYSINFO_ID_SMBIOS_SYSTEM_UUID,
>>>> + BOARD_A5E = SYSINFO_ID_SMBIOS_BASEBOARD_PRODUCT,
>>>> + BOARD_NAME = SYSINFO_ID_SMBIOS_BASEBOARD_VERSION,
>>>
>>> Do you need to rename these? It seems better to use the standard
>>> names, although I do think they are quite long.
>>
>> We don't rename, we map pre-existing field names in EEPROM and
>> pre-existing board-specific U-Boot env vars to sysinfo fields here.
>
> Yes, that's what I mean. Can you just use the SYSINFO_ID... values
> directory, rather than creating mapping?
>
The mapping is unavoidable ("pre-existing"), and here is the most
logical point for me as it avoids that we have to do it twice: first in
the sysinfo driver (EEPROM->SYSINFO_ID) and then again in
set_board_info_env (SYSINFO_ID->env-vars).
>>
>>>
>>>> + BOARD_SEBOOT_VER = SYSINFO_ID_USER + 0,
>>>> + BOARD_MAC_ADDR_CNT = SYSINFO_ID_USER + 1,
>>>> + BOARD_MAC_ADDR_1 = SYSINFO_ID_USER + 2,
>>>> + BOARD_MAC_ADDR_START = BOARD_MAC_ADDR_1,
>>>> + BOARD_MAC_ADDR_2 = SYSINFO_ID_USER + 3,
>>>> + BOARD_MAC_ADDR_3 = SYSINFO_ID_USER + 4,
>>>> + BOARD_MAC_ADDR_4 = SYSINFO_ID_USER + 5,
>>>> + BOARD_MAC_ADDR_5 = SYSINFO_ID_USER + 6,
>>>> + BOARD_MAC_ADDR_6 = SYSINFO_ID_USER + 7,
>>>> + BOARD_MAC_ADDR_7 = SYSINFO_ID_USER + 8,
>>>> + BOARD_MAC_ADDR_8 = SYSINFO_ID_USER + 9,
>>>> + BOARD_DDR_SIZE = SYSINFO_ID_USER + 10,
>>>
>>> These are the ones which should be added as standard. Be sure to
>>> comment them as necessary.
>>
>> You mean that BOARD_MAC_ADDR and BOARD_DDR_SIZE should become
>> SYSINFO_ID_BOARD_MAC_ADDR and SYSINFO_ID_BOARD_DDR_SIZE? But
>> BOARD_SEBOOT_VER is nothing that has any meaning beyond our board.
>
> For BOARD_SEBOOT_VER, I don't know what it is, but just add a comment
> and put it in sysinfo.h. It is OK to add board-specific stufff there.
>
This still makes no sense to me. What is the point of SYSINFO_ID_USER
then? Why does it exists and is being used elsewhere when it shouldn't?
> Is BOARD_DDR_SIZE the memory size? Perhaps rename it to RAM_SIZE ?
>
Yes, and I'm also considering to append "_MB" in order to clarify the unit.
Jan
--
Siemens AG, Technology
Linux Expert Center
More information about the U-Boot
mailing list