[PATCH v2 0/7] Qualcomm: add support for SC7280 and the RB3 Gen 2
Introduce support for the SC7280 platform and specifically the RB3 Gen 2 development board.
The RB3 Gen 2 is the latest robotics board from Qualcomm, featuring a QCM6490 SoC (the same SoC found in the Fairphone 5), 6GB of RAM, and 128GB of UFS storage, as well as a whole lot of I/O.
Most of the I/O is attached via a custom PCIe bridge chip, and as such is not yet supported. However it is possible to boot from USB via the Type-C port, which is hardcoded to host mode for this initial support.
USB support additionally requires this patch: https://lore.kernel.org/u-boot/20240423141551.2410199-1-caleb.connolly@linar...
--- Changes in v2: - Add rb3gen2 docs to index - Add qcm6490_defconfig to MAINTAINERS - Link to v1: https://lore.kernel.org/r/20240809-b4-rb3gen2-v1-0-7c73fe05b37a@linaro.org
--- Caleb Connolly (7): clk/qcom: add initial clock driver for sc7280 dts: qcs6490-rb3gen2-u-boot: add override dtsi dts: qcs6490-rb3gen2-u-boot: USB host mode iommu: qcom-smmu: add sc7280-smmu-500 compatible qcom_defconfig: enable SC7280 clocks configs: add qcm6490_defconfig doc: board/qualcomm: document rb3gen2 building/flashing
MAINTAINERS | 1 + arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi | 28 +++++++ configs/qcm6490_defconfig | 18 +++++ configs/qcom_defconfig | 1 + doc/board/qualcomm/index.rst | 1 + doc/board/qualcomm/rb3gen2.rst | 53 +++++++++++++ drivers/clk/qcom/Kconfig | 8 ++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clock-qcom.h | 1 + drivers/clk/qcom/clock-sc7280.c | 132 +++++++++++++++++++++++++++++++ drivers/iommu/qcom-hyp-smmu.c | 1 + 11 files changed, 245 insertions(+) --- change-id: 20240809-b4-rb3gen2-87bd866abde1 base-commit: c6222132b64faf0ab083330f208b1400ee47ccc9
// Caleb (they/them)
We don't actually need any clocks to get UFS up and running, resets are useful though.
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- drivers/clk/qcom/Kconfig | 8 +++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clock-qcom.h | 1 + drivers/clk/qcom/clock-sc7280.c | 132 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+)
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index 45d63c6d6dbf..0d2c0ac225c5 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -85,7 +85,15 @@ config CLK_QCOM_SM8650 Say Y here to enable support for the Global Clock Controller on the Snapdragon SM8650 SoC. This driver supports the clocks and resets exposed by the GCC hardware block.
+config CLK_QCOM_SC7280 + bool "Qualcomm SC7280 GCC" + select CLK_QCOM + help + Say Y here to enable support for the Global Clock Controller + on the Snapdragon SC7280 SoC. This driver supports the clocks + and resets exposed by the GCC hardware block. + endmenu
endif diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile index dec20e4b5943..e223c131ee4d 100644 --- a/drivers/clk/qcom/Makefile +++ b/drivers/clk/qcom/Makefile @@ -8,8 +8,9 @@ obj-$(CONFIG_CLK_QCOM_APQ8016) += clock-apq8016.o obj-$(CONFIG_CLK_QCOM_APQ8096) += clock-apq8096.o obj-$(CONFIG_CLK_QCOM_IPQ4019) += clock-ipq4019.o obj-$(CONFIG_CLK_QCOM_QCM2290) += clock-qcm2290.o obj-$(CONFIG_CLK_QCOM_QCS404) += clock-qcs404.o +obj-$(CONFIG_CLK_QCOM_SC7280) += clock-sc7280.o obj-$(CONFIG_CLK_QCOM_SM6115) += clock-sm6115.o obj-$(CONFIG_CLK_QCOM_SM8250) += clock-sm8250.o obj-$(CONFIG_CLK_QCOM_SM8550) += clock-sm8550.o obj-$(CONFIG_CLK_QCOM_SM8650) += clock-sm8650.o diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h index f6445c8f566f..7aa6ca59aad5 100644 --- a/drivers/clk/qcom/clock-qcom.h +++ b/drivers/clk/qcom/clock-qcom.h @@ -10,8 +10,9 @@ #define CFG_CLK_SRC_CXO (0 << 8) #define CFG_CLK_SRC_GPLL0 (1 << 8) #define CFG_CLK_SRC_GPLL0_AUX2 (2 << 8) #define CFG_CLK_SRC_GPLL9 (2 << 8) +#define CFG_CLK_SRC_GPLL0_ODD (3 << 8) #define CFG_CLK_SRC_GPLL6 (4 << 8) #define CFG_CLK_SRC_GPLL7 (3 << 8) #define CFG_CLK_SRC_GPLL4 (5 << 8) #define CFG_CLK_SRC_GPLL0_EVEN (6 << 8) diff --git a/drivers/clk/qcom/clock-sc7280.c b/drivers/clk/qcom/clock-sc7280.c new file mode 100644 index 000000000000..5d343f120519 --- /dev/null +++ b/drivers/clk/qcom/clock-sc7280.c @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Clock drivers for Qualcomm sc7280 + * + * (C) Copyright 2024 Linaro Ltd. + */ + +#include <linux/types.h> +#include <clk-uclass.h> +#include <dm.h> +#include <linux/delay.h> +#include <asm/io.h> +#include <linux/bug.h> +#include <linux/bitops.h> +#include <dt-bindings/clock/qcom,gcc-sc7280.h> + +#include "clock-qcom.h" + +#define USB30_PRIM_MOCK_UTMI_CLK_CMD_RCGR 0xf038 +#define USB30_PRIM_MASTER_CLK_CMD_RCGR 0xf020 + +static ulong sc7280_set_rate(struct clk *clk, ulong rate) +{ + struct msm_clk_priv *priv = dev_get_priv(clk->dev); + + if (clk->id < priv->data->num_clks) + debug("%s: %s, requested rate=%ld\n", __func__, priv->data->clks[clk->id].name, rate); + + switch (clk->id) { + case GCC_USB30_PRIM_MOCK_UTMI_CLK: + WARN(rate != 19200000, "Unexpected rate for USB30_PRIM_MOCK_UTMI_CLK: %lu\n", rate); + clk_rcg_set_rate(priv->base, USB30_PRIM_MASTER_CLK_CMD_RCGR, 0, CFG_CLK_SRC_CXO); + return rate; + case GCC_USB30_PRIM_MASTER_CLK: + WARN(rate != 200000000, "Unexpected rate for USB30_PRIM_MASTER_CLK: %lu\n", rate); + clk_rcg_set_rate_mnd(priv->base, USB30_PRIM_MASTER_CLK_CMD_RCGR, + 1, 0, 0, CFG_CLK_SRC_GPLL0_ODD, 8); + clk_rcg_set_rate(priv->base, 0xf064, 0, 0); + return rate; + default: + return 0; + } +} + +static const struct gate_clk sc7280_clks[] = { + GATE_CLK(GCC_CFG_NOC_USB3_PRIM_AXI_CLK, 0xf07c, 1), + GATE_CLK(GCC_USB30_PRIM_MASTER_CLK, 0xf010, 1), + GATE_CLK(GCC_AGGRE_USB3_PRIM_AXI_CLK, 0xf080, 1), + GATE_CLK(GCC_USB30_PRIM_SLEEP_CLK, 0xf018, 1), + GATE_CLK(GCC_USB30_PRIM_MOCK_UTMI_CLK, 0xf01c, 1), + GATE_CLK(GCC_USB3_PRIM_PHY_AUX_CLK, 0xf054, 1), + GATE_CLK(GCC_USB3_PRIM_PHY_COM_AUX_CLK, 0xf058, 1), +}; + +static int sc7280_enable(struct clk *clk) +{ + struct msm_clk_priv *priv = dev_get_priv(clk->dev); + + if (priv->data->num_clks < clk->id) { + debug("%s: unknown clk id %lu\n", __func__, clk->id); + return 0; + } + + debug("%s: clk %ld: %s\n", __func__, clk->id, sc7280_clks[clk->id].name); + + switch (clk->id) { + case GCC_AGGRE_USB3_PRIM_AXI_CLK: + qcom_gate_clk_en(priv, GCC_USB30_PRIM_MASTER_CLK); + fallthrough; + case GCC_USB30_PRIM_MASTER_CLK: + qcom_gate_clk_en(priv, GCC_USB3_PRIM_PHY_AUX_CLK); + qcom_gate_clk_en(priv, GCC_USB3_PRIM_PHY_COM_AUX_CLK); + break; + } + + qcom_gate_clk_en(priv, clk->id); + + return 0; +} + +static const struct qcom_reset_map sc7280_gcc_resets[] = { + [GCC_PCIE_0_BCR] = { 0x6b000 }, + [GCC_PCIE_0_PHY_BCR] = { 0x6c01c }, + [GCC_PCIE_1_BCR] = { 0x8d000 }, + [GCC_PCIE_1_PHY_BCR] = { 0x8e01c }, + [GCC_QUSB2PHY_PRIM_BCR] = { 0x12000 }, + [GCC_QUSB2PHY_SEC_BCR] = { 0x12004 }, + [GCC_SDCC1_BCR] = { 0x75000 }, + [GCC_SDCC2_BCR] = { 0x14000 }, + [GCC_SDCC4_BCR] = { 0x16000 }, + [GCC_UFS_PHY_BCR] = { 0x77000 }, + [GCC_USB30_PRIM_BCR] = { 0xf000 }, + [GCC_USB30_SEC_BCR] = { 0x9e000 }, + [GCC_USB3_DP_PHY_PRIM_BCR] = { 0x50008 }, + [GCC_USB3_PHY_PRIM_BCR] = { 0x50000 }, + [GCC_USB3PHY_PHY_PRIM_BCR] = { 0x50004 }, + [GCC_USB_PHY_CFG_AHB2PHY_BCR] = { 0x6a000 }, +}; + +static const struct qcom_power_map sc7280_gdscs[] = { + [GCC_UFS_PHY_GDSC] = { 0x77004 }, + [GCC_USB30_PRIM_GDSC] = { 0xf004 }, +}; + +static struct msm_clk_data qcs404_gcc_data = { + .resets = sc7280_gcc_resets, + .num_resets = ARRAY_SIZE(sc7280_gcc_resets), + .clks = sc7280_clks, + .num_clks = ARRAY_SIZE(sc7280_clks), + + .power_domains = sc7280_gdscs, + .num_power_domains = ARRAY_SIZE(sc7280_gdscs), + + .enable = sc7280_enable, + .set_rate = sc7280_set_rate, +}; + +static const struct udevice_id gcc_sc7280_of_match[] = { + { + .compatible = "qcom,gcc-sc7280", + .data = (ulong)&qcs404_gcc_data, + }, + { } +}; + +U_BOOT_DRIVER(gcc_sc7280) = { + .name = "gcc_sc7280", + .id = UCLASS_NOP, + .of_match = gcc_sc7280_of_match, + .bind = qcom_cc_bind, + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, +};
Hi Caleb,
On Wed, 21 Aug 2024 at 07:42, Caleb Connolly caleb.connolly@linaro.org wrote:
We don't actually need any clocks to get UFS up and running, resets are useful though.
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
drivers/clk/qcom/Kconfig | 8 +++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clock-qcom.h | 1 + drivers/clk/qcom/clock-sc7280.c | 132 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+)
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index 45d63c6d6dbf..0d2c0ac225c5 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -85,7 +85,15 @@ config CLK_QCOM_SM8650 Say Y here to enable support for the Global Clock Controller on the Snapdragon SM8650 SoC. This driver supports the clocks and resets exposed by the GCC hardware block.
+config CLK_QCOM_SC7280
bool "Qualcomm SC7280 GCC"select CLK_QCOMhelpSay Y here to enable support for the Global Clock Controlleron the Snapdragon SC7280 SoC. This driver supports the clocksand resets exposed by the GCC hardware block.endmenu
endif diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile index dec20e4b5943..e223c131ee4d 100644 --- a/drivers/clk/qcom/Makefile +++ b/drivers/clk/qcom/Makefile @@ -8,8 +8,9 @@ obj-$(CONFIG_CLK_QCOM_APQ8016) += clock-apq8016.o obj-$(CONFIG_CLK_QCOM_APQ8096) += clock-apq8096.o obj-$(CONFIG_CLK_QCOM_IPQ4019) += clock-ipq4019.o obj-$(CONFIG_CLK_QCOM_QCM2290) += clock-qcm2290.o obj-$(CONFIG_CLK_QCOM_QCS404) += clock-qcs404.o +obj-$(CONFIG_CLK_QCOM_SC7280) += clock-sc7280.o obj-$(CONFIG_CLK_QCOM_SM6115) += clock-sm6115.o obj-$(CONFIG_CLK_QCOM_SM8250) += clock-sm8250.o obj-$(CONFIG_CLK_QCOM_SM8550) += clock-sm8550.o obj-$(CONFIG_CLK_QCOM_SM8650) += clock-sm8650.o diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h index f6445c8f566f..7aa6ca59aad5 100644 --- a/drivers/clk/qcom/clock-qcom.h +++ b/drivers/clk/qcom/clock-qcom.h @@ -10,8 +10,9 @@ #define CFG_CLK_SRC_CXO (0 << 8) #define CFG_CLK_SRC_GPLL0 (1 << 8) #define CFG_CLK_SRC_GPLL0_AUX2 (2 << 8) #define CFG_CLK_SRC_GPLL9 (2 << 8) +#define CFG_CLK_SRC_GPLL0_ODD (3 << 8) #define CFG_CLK_SRC_GPLL6 (4 << 8) #define CFG_CLK_SRC_GPLL7 (3 << 8) #define CFG_CLK_SRC_GPLL4 (5 << 8) #define CFG_CLK_SRC_GPLL0_EVEN (6 << 8) diff --git a/drivers/clk/qcom/clock-sc7280.c b/drivers/clk/qcom/clock-sc7280.c new file mode 100644 index 000000000000..5d343f120519 --- /dev/null +++ b/drivers/clk/qcom/clock-sc7280.c @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Clock drivers for Qualcomm sc7280
- (C) Copyright 2024 Linaro Ltd.
- */
+#include <linux/types.h> +#include <clk-uclass.h> +#include <dm.h> +#include <linux/delay.h> +#include <asm/io.h> +#include <linux/bug.h> +#include <linux/bitops.h> +#include <dt-bindings/clock/qcom,gcc-sc7280.h>
+#include "clock-qcom.h"
+#define USB30_PRIM_MOCK_UTMI_CLK_CMD_RCGR 0xf038 +#define USB30_PRIM_MASTER_CLK_CMD_RCGR 0xf020
+static ulong sc7280_set_rate(struct clk *clk, ulong rate) +{
struct msm_clk_priv *priv = dev_get_priv(clk->dev);if (clk->id < priv->data->num_clks)debug("%s: %s, requested rate=%ld\n", __func__, priv->data->clks[clk->id].name, rate);switch (clk->id) {case GCC_USB30_PRIM_MOCK_UTMI_CLK:WARN(rate != 19200000, "Unexpected rate for USB30_PRIM_MOCK_UTMI_CLK: %lu\n", rate);clk_rcg_set_rate(priv->base, USB30_PRIM_MASTER_CLK_CMD_RCGR, 0, CFG_CLK_SRC_CXO);return rate;case GCC_USB30_PRIM_MASTER_CLK:WARN(rate != 200000000, "Unexpected rate for USB30_PRIM_MASTER_CLK: %lu\n", rate);clk_rcg_set_rate_mnd(priv->base, USB30_PRIM_MASTER_CLK_CMD_RCGR,1, 0, 0, CFG_CLK_SRC_GPLL0_ODD, 8);clk_rcg_set_rate(priv->base, 0xf064, 0, 0);return rate;default:return 0;}+}
+static const struct gate_clk sc7280_clks[] = {
GATE_CLK(GCC_CFG_NOC_USB3_PRIM_AXI_CLK, 0xf07c, 1),GATE_CLK(GCC_USB30_PRIM_MASTER_CLK, 0xf010, 1),GATE_CLK(GCC_AGGRE_USB3_PRIM_AXI_CLK, 0xf080, 1),GATE_CLK(GCC_USB30_PRIM_SLEEP_CLK, 0xf018, 1),GATE_CLK(GCC_USB30_PRIM_MOCK_UTMI_CLK, 0xf01c, 1),GATE_CLK(GCC_USB3_PRIM_PHY_AUX_CLK, 0xf054, 1),GATE_CLK(GCC_USB3_PRIM_PHY_COM_AUX_CLK, 0xf058, 1),+};
+static int sc7280_enable(struct clk *clk) +{
struct msm_clk_priv *priv = dev_get_priv(clk->dev);if (priv->data->num_clks < clk->id) {debug("%s: unknown clk id %lu\n", __func__, clk->id);return 0;}debug("%s: clk %ld: %s\n", __func__, clk->id, sc7280_clks[clk->id].name);switch (clk->id) {case GCC_AGGRE_USB3_PRIM_AXI_CLK:qcom_gate_clk_en(priv, GCC_USB30_PRIM_MASTER_CLK);fallthrough;case GCC_USB30_PRIM_MASTER_CLK:qcom_gate_clk_en(priv, GCC_USB3_PRIM_PHY_AUX_CLK);qcom_gate_clk_en(priv, GCC_USB3_PRIM_PHY_COM_AUX_CLK);break;}qcom_gate_clk_en(priv, clk->id);return 0;+}
+static const struct qcom_reset_map sc7280_gcc_resets[] = {
[GCC_PCIE_0_BCR] = { 0x6b000 },[GCC_PCIE_0_PHY_BCR] = { 0x6c01c },[GCC_PCIE_1_BCR] = { 0x8d000 },[GCC_PCIE_1_PHY_BCR] = { 0x8e01c },[GCC_QUSB2PHY_PRIM_BCR] = { 0x12000 },[GCC_QUSB2PHY_SEC_BCR] = { 0x12004 },[GCC_SDCC1_BCR] = { 0x75000 },[GCC_SDCC2_BCR] = { 0x14000 },[GCC_SDCC4_BCR] = { 0x16000 },[GCC_UFS_PHY_BCR] = { 0x77000 },[GCC_USB30_PRIM_BCR] = { 0xf000 },[GCC_USB30_SEC_BCR] = { 0x9e000 },[GCC_USB3_DP_PHY_PRIM_BCR] = { 0x50008 },[GCC_USB3_PHY_PRIM_BCR] = { 0x50000 },[GCC_USB3PHY_PHY_PRIM_BCR] = { 0x50004 },[GCC_USB_PHY_CFG_AHB2PHY_BCR] = { 0x6a000 },+};
+static const struct qcom_power_map sc7280_gdscs[] = {
[GCC_UFS_PHY_GDSC] = { 0x77004 },[GCC_USB30_PRIM_GDSC] = { 0xf004 },+};
+static struct msm_clk_data qcs404_gcc_data = {
.resets = sc7280_gcc_resets,.num_resets = ARRAY_SIZE(sc7280_gcc_resets),.clks = sc7280_clks,.num_clks = ARRAY_SIZE(sc7280_clks),.power_domains = sc7280_gdscs,.num_power_domains = ARRAY_SIZE(sc7280_gdscs),.enable = sc7280_enable,.set_rate = sc7280_set_rate,+};
+static const struct udevice_id gcc_sc7280_of_match[] = {
{.compatible = "qcom,gcc-sc7280",.data = (ulong)&qcs404_gcc_data,},{ }+};
+U_BOOT_DRIVER(gcc_sc7280) = {
.name = "gcc_sc7280",.id = UCLASS_NOP,.of_match = gcc_sc7280_of_match,.bind = qcom_cc_bind,.flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF,+};
This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Regards, Simon
Hi Simon,
+U_BOOT_DRIVER(gcc_sc7280) = {
.name = "gcc_sc7280",.id = UCLASS_NOP,.of_match = gcc_sc7280_of_match,.bind = qcom_cc_bind,.flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF,+};
This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Please refer to qcom_cc_bind() which binds the clock, reset, and power domain drivers.
Kind regards,
Regards, Simon
Hi Caleb,
On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
+U_BOOT_DRIVER(gcc_sc7280) = {
.name = "gcc_sc7280",.id = UCLASS_NOP,.of_match = gcc_sc7280_of_match,.bind = qcom_cc_bind,.flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF,+};
This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Please refer to qcom_cc_bind() which binds the clock, reset, and power domain drivers.
Gosh, why? Are these devices not in the devicetree?
Regards, Simon
On 21/08/2024 16:37, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
+U_BOOT_DRIVER(gcc_sc7280) = {
.name = "gcc_sc7280",.id = UCLASS_NOP,.of_match = gcc_sc7280_of_match,.bind = qcom_cc_bind,.flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF,+};
This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Please refer to qcom_cc_bind() which binds the clock, reset, and power domain drivers.
Gosh, why? Are these devices not in the devicetree?
They are, the gcc block contains clock, reset, and pd parts. On Linux this is not an issue because a single device can be multiple different classes (e..g when you register a reset you do it for a device) whereas U-Boot requires a device per class.
e.g. see devm_reset_controller_register() in Linux, it populates a struct reset_controller_dev which references the struct device created for the node. In U-Boot you have to create a new device which _is_ the reset controller.
Regards, Simon
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 16:37, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
+U_BOOT_DRIVER(gcc_sc7280) = {
.name = "gcc_sc7280",.id = UCLASS_NOP,.of_match = gcc_sc7280_of_match,.bind = qcom_cc_bind,.flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF,+};
This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Please refer to qcom_cc_bind() which binds the clock, reset, and power domain drivers.
Gosh, why? Are these devices not in the devicetree?
They are, the gcc block contains clock, reset, and pd parts. On Linux this is not an issue because a single device can be multiple different classes (e..g when you register a reset you do it for a device) whereas U-Boot requires a device per class.
e.g. see devm_reset_controller_register() in Linux, it populates a struct reset_controller_dev which references the struct device created for the node. In U-Boot you have to create a new device which _is_ the reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
1. Allow DM to bind multiple devices to each devicetree node, perhaps as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
2. As 1 but have DM create a parent 'UCLASS_MULTI' device automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
3. Implement devices which are in more than one uclass. There would still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
What do you think?
Regards, Simon
On 21/08/2024 18:16, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 16:37, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
+U_BOOT_DRIVER(gcc_sc7280) = {
.name = "gcc_sc7280",.id = UCLASS_NOP,.of_match = gcc_sc7280_of_match,.bind = qcom_cc_bind,.flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF,+};
This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Please refer to qcom_cc_bind() which binds the clock, reset, and power domain drivers.
Gosh, why? Are these devices not in the devicetree?
They are, the gcc block contains clock, reset, and pd parts. On Linux this is not an issue because a single device can be multiple different classes (e..g when you register a reset you do it for a device) whereas U-Boot requires a device per class.
e.g. see devm_reset_controller_register() in Linux, it populates a struct reset_controller_dev which references the struct device created for the node. In U-Boot you have to create a new device which _is_ the reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this.
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
- Allow DM to bind multiple devices to each devicetree node, perhaps
as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
- As 1 but have DM create a parent 'UCLASS_MULTI' device
automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
- Implement devices which are in more than one uclass. There would
still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
Kind regards,
Regards, Simon
Hi Caleb,
On Wed, 21 Aug 2024 at 10:47, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 18:16, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 16:37, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
> +U_BOOT_DRIVER(gcc_sc7280) = { > + .name = "gcc_sc7280", > + .id = UCLASS_NOP, > + .of_match = gcc_sc7280_of_match, > + .bind = qcom_cc_bind, > + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, > +};
This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Please refer to qcom_cc_bind() which binds the clock, reset, and power domain drivers.
Gosh, why? Are these devices not in the devicetree?
They are, the gcc block contains clock, reset, and pd parts. On Linux this is not an issue because a single device can be multiple different classes (e..g when you register a reset you do it for a device) whereas U-Boot requires a device per class.
e.g. see devm_reset_controller_register() in Linux, it populates a struct reset_controller_dev which references the struct device created for the node. In U-Boot you have to create a new device which _is_ the reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this.
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
I'm only really involved in mainline and don't really see vendor trees much. An example is where pinctrl has a GPIO controller but it is not mentioned in the devicetree. It would be better for U-Boot to add a subnode for each GPIO controller. In general, if the SoC has a device, it should be in the devicetree.
Part of this difference (between U-Boot and Linux) comes about because Linux device setup is fairly manual, whereas U-Boot tries to put all of that in common DM code. Whenever you are including dm/device-internal.h that is often a sign that the binding is causing issues.
I'm happy for you to change my mind.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
- Allow DM to bind multiple devices to each devicetree node, perhaps
as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
You would have to repeat the same compatible string in each driver.
For generic compatible strings we could a) worry about it later or b) restrict this technique to only nodes with a single compatible string, or c) use the driver flag as mentioned
- As 1 but have DM create a parent 'UCLASS_MULTI' device
automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
How come? What is qcom_cc_bind() doing which DM couldn't?
- Implement devices which are in more than one uclass. There would
still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
In U-Boot the uclass is a stronger concept, e.g. you can generically iterate through all devices in a uclass, and all devices have one.
What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
What is the Linux model, in this sense? Whenever I see barebox I wonder why we can't fold whatever new features it needs into U-Boot...perhaps the code bases have converged too much...?
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
Well I would like to tidy this up in DM, so let's figure out which option makes the most sense...once I understand what you mean by 'Linux model' above.
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
So far as I can tell, you are always going to have a flat tree, even if only before relocation or in SPL. How would we get around that? Also, what don't you like about ofnode?
Given that Qualcomm is only using U-Boot as a second-stage loader so far, (please, not for long!!) everything looks quite different. But most platforms use U-Boot from SoC-boot-ROM handoff, so the constraints are different (tighter).
Anyway, certainly OF_LIVE being the default would be good. I have often wondered if we can (at build-time) convert the devicetree into a 'live' version, where the pointers are replaced by integers, such that the early U-Boot code can easily compute the pointer value for each node. It should make the unflattening much faster. For pre-relocation and SPL, since we know the load address, we can (I am pretty sure) have Binman put a full, live tree in the image and avoid the unflattening code at all. Relocating a livetree is fairly easy too.
Regards, Simon
On 21/08/2024 20:27, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 10:47, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 18:16, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 16:37, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon, >> +U_BOOT_DRIVER(gcc_sc7280) = { >> + .name = "gcc_sc7280", >> + .id = UCLASS_NOP, >> + .of_match = gcc_sc7280_of_match, >> + .bind = qcom_cc_bind, >> + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, >> +}; > > This should use driver model, with a UCLASS_CLK and UCLASS_RESET
Please refer to qcom_cc_bind() which binds the clock, reset, and power domain drivers.
Gosh, why? Are these devices not in the devicetree?
They are, the gcc block contains clock, reset, and pd parts. On Linux this is not an issue because a single device can be multiple different classes (e..g when you register a reset you do it for a device) whereas U-Boot requires a device per class.
e.g. see devm_reset_controller_register() in Linux, it populates a struct reset_controller_dev which references the struct device created for the node. In U-Boot you have to create a new device which _is_ the reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this.
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
I'm only really involved in mainline and don't really see vendor trees much. An example is where pinctrl has a GPIO controller but it is not mentioned in the devicetree. It would be better for U-Boot to add a subnode for each GPIO controller. In general, if the SoC has a device, it should be in the devicetree.
The concept of a device is an OS one. DT is not "telling the OS how to use the hardware", it is describing the hardware.
This distinction is important because it's the only way to ensure that future OS changes can be done regardless of the DT. And also of course because different OS's will have different ideas of how to model devices (case in point).
The GCC block on Qualcomm platforms is a single hardware block. The datasheets and hardware programming guides describe it as such. The clocks, resets, and GDSCs are all entwined at the hardware level. They also have overlapping register addresses.
The further away DT gets from describing the hardware in favour of simplifying the OS, the more likely we are to start running into issues with fitting hardware changes into our arbitrary model.
Part of this difference (between U-Boot and Linux) comes about because Linux device setup is fairly manual, whereas U-Boot tries to put all of that in common DM code. Whenever you are including dm/device-internal.h that is often a sign that the binding is causing issues.
To me this indicates an inability for U-Boot's DM to handle complicated devices. I don't think U-Boot should dictate the design of devicetree.
I don't know how else to describe this. This issue has been litigated over and over again on the kernel mailing list. Every time someone suggests changing DT because of a limitation in Linux they are (rightfully) shut down. If these changes were accepted then it would be impossible for DT to be an OS agnostic hardware description, because it would be full of OS specific hacks.
I'm happy for you to change my mind.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
- Allow DM to bind multiple devices to each devicetree node, perhaps
as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
You would have to repeat the same compatible string in each driver.
For generic compatible strings we could a) worry about it later or b) restrict this technique to only nodes with a single compatible string, or c) use the driver flag as mentioned
- As 1 but have DM create a parent 'UCLASS_MULTI' device
automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
How come? What is qcom_cc_bind() doing which DM couldn't?
Maybe DM could look at the "#clock-cells", "#reset-cells", and "#power-domain-cells" properties and match on compatible string + uclass? I think that could work.
- Implement devices which are in more than one uclass. There would
still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
In U-Boot the uclass is a stronger concept, e.g. you can generically iterate through all devices in a uclass, and all devices have one.
What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
What is the Linux model, in this sense? Whenever I see barebox I wonder why we can't fold whatever new features it needs into U-Boot...perhaps the code bases have converged too much...?
barebox is livetree only (like Linux) as I understand it, and I think it aims to be literally copy/paste compatible with Linux. I would love for U-Boot to adopt this approach.
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
Well I would like to tidy this up in DM, so let's figure out which option makes the most sense...once I understand what you mean by 'Linux model' above.
I mean where a single struct device can be multiple classes, so you create a device and then create a reset controller and associate the device with it.
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
So far as I can tell, you are always going to have a flat tree, even if only before relocation or in SPL. How would we get around that? Also, what don't you like about ofnode?
Yeah, you start with a flat tree and then should build a live one as early as possible.
The problem with ofnode is that it is more or less restricted to the subset of operations fdt supports. It lacks some of the fancier features of a live tree. It also means that every use of the of_* API has to be preceded by a check that we're actually running the live tree, otherwise some kind of hard bail.
Would be great to use of_* API by default, for capable platforms anyway.
Given that Qualcomm is only using U-Boot as a second-stage loader so far, (please, not for long!!) everything looks quite different. But most platforms use U-Boot from SoC-boot-ROM handoff, so the constraints are different (tighter).
For U-Boot SPL yeah I expect the constraints to be different. We're getting a bit closer to the metal on the rb3gen2 (now it's just bootROM -> SBL1 -> (tz -> hyp) -> U-Boot, without going through edk2 first haha).
I hope we'll get to do U-Boot SPL on some Qualcomm platforms eventually.
Anyway, certainly OF_LIVE being the default would be good. I have often wondered if we can (at build-time) convert the devicetree into a 'live' version, where the pointers are replaced by integers, such that the early U-Boot code can easily compute the pointer value for each node. It should make the unflattening much faster. For pre-relocation and SPL, since we know the load address, we can (I am pretty sure) have Binman put a full, live tree in the image and avoid the unflattening code at all. Relocating a livetree is fairly easy too.
This might be a nice optimisation? I think it would be acceptable to just read the memory layout -> enable the MMU -> build the live tree. This would probably be fast enough.
And of course, the great thing we have on Qualcomm platforms is that we can run the same U-Boot binary on every sdm845 and newer platform.
Regards, Simon
Hi Caleb,
On Wed, 21 Aug 2024 at 14:33, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 20:27, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 10:47, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 18:16, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 16:37, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote: > > Hi Simon, >>> +U_BOOT_DRIVER(gcc_sc7280) = { >>> + .name = "gcc_sc7280", >>> + .id = UCLASS_NOP, >>> + .of_match = gcc_sc7280_of_match, >>> + .bind = qcom_cc_bind, >>> + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, >>> +}; >> >> This should use driver model, with a UCLASS_CLK and UCLASS_RESET > > Please refer to qcom_cc_bind() which binds the clock, reset, and power > domain drivers.
Gosh, why? Are these devices not in the devicetree?
They are, the gcc block contains clock, reset, and pd parts. On Linux this is not an issue because a single device can be multiple different classes (e..g when you register a reset you do it for a device) whereas U-Boot requires a device per class.
e.g. see devm_reset_controller_register() in Linux, it populates a struct reset_controller_dev which references the struct device created for the node. In U-Boot you have to create a new device which _is_ the reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this.
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
I'm only really involved in mainline and don't really see vendor trees much. An example is where pinctrl has a GPIO controller but it is not mentioned in the devicetree. It would be better for U-Boot to add a subnode for each GPIO controller. In general, if the SoC has a device, it should be in the devicetree.
The concept of a device is an OS one. DT is not "telling the OS how to use the hardware", it is describing the hardware.
This distinction is important because it's the only way to ensure that future OS changes can be done regardless of the DT. And also of course because different OS's will have different ideas of how to model devices (case in point).
The GCC block on Qualcomm platforms is a single hardware block. The datasheets and hardware programming guides describe it as such. The clocks, resets, and GDSCs are all entwined at the hardware level. They also have overlapping register addresses.
The further away DT gets from describing the hardware in favour of simplifying the OS, the more likely we are to start running into issues with fitting hardware changes into our arbitrary model.
I completely agree with everything you are saying, but you don't go far enough. We should additionally require that all hardware has a description in the devicetree. See for example the GPIO controller I mentioned. When Linux wants it, it gets it, when it doesn't, it isn't there. Sorry to have to say it, but that's not right.
Part of this difference (between U-Boot and Linux) comes about because Linux device setup is fairly manual, whereas U-Boot tries to put all of that in common DM code. Whenever you are including dm/device-internal.h that is often a sign that the binding is causing issues.
To me this indicates an inability for U-Boot's DM to handle complicated devices. I don't think U-Boot should dictate the design of devicetree.
I don't know how else to describe this. This issue has been litigated over and over again on the kernel mailing list. Every time someone suggests changing DT because of a limitation in Linux they are (rightfully) shut down. If these changes were accepted then it would be impossible for DT to be an OS agnostic hardware description, because it would be full of OS specific hacks.
Given that an OS has no size limitations so can afford to do just about anything to deal with one-off cases in each SoC, sure. But let's face it, it isn't project-agnostic. I could provide dozens of examples where the bindings are a pain for U-Boot. Even the use of strings in some of the SoCs' pinctrl bindings is painful. My point is that there are many ways to model and describe the hardware and taking more account of all users would be a big step forward.
I'm happy for you to change my mind.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
- Allow DM to bind multiple devices to each devicetree node, perhaps
as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
You would have to repeat the same compatible string in each driver.
For generic compatible strings we could a) worry about it later or b) restrict this technique to only nodes with a single compatible string, or c) use the driver flag as mentioned
- As 1 but have DM create a parent 'UCLASS_MULTI' device
automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
How come? What is qcom_cc_bind() doing which DM couldn't?
Maybe DM could look at the "#clock-cells", "#reset-cells", and "#power-domain-cells" properties and match on compatible string + uclass? I think that could work.
Hmmm you mean when it sees those in the node it knows the additional uclasses it is allowed to use?
- Implement devices which are in more than one uclass. There would
still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
In U-Boot the uclass is a stronger concept, e.g. you can generically iterate through all devices in a uclass, and all devices have one.
What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
What is the Linux model, in this sense? Whenever I see barebox I wonder why we can't fold whatever new features it needs into U-Boot...perhaps the code bases have converged too much...?
barebox is livetree only (like Linux) as I understand it, and I think it aims to be literally copy/paste compatible with Linux. I would love for U-Boot to adopt this approach.
OK. We do have code-size restrictions though.
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
Well I would like to tidy this up in DM, so let's figure out which option makes the most sense...once I understand what you mean by 'Linux model' above.
I mean where a single struct device can be multiple classes, so you create a device and then create a reset controller and associate the device with it.
It's the bit about having one driver manually creating devices that I very much want to avoid. If the devicetree really does (fully) describe hardware, it shouldn't be necessary.
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
So far as I can tell, you are always going to have a flat tree, even if only before relocation or in SPL. How would we get around that? Also, what don't you like about ofnode?
Yeah, you start with a flat tree and then should build a live one as early as possible.
The problem with ofnode is that it is more or less restricted to the subset of operations fdt supports. It lacks some of the fancier features of a live tree. It also means that every use of the of_* API has to be preceded by a check that we're actually running the live tree, otherwise some kind of hard bail.
Indeed. In principle, ofnode could support anything, but the cost might be high when implemented in the flat tree. People do add new functions from time to time.
Would be great to use of_* API by default, for capable platforms anyway.
Given that Qualcomm is only using U-Boot as a second-stage loader so far, (please, not for long!!) everything looks quite different. But most platforms use U-Boot from SoC-boot-ROM handoff, so the constraints are different (tighter).
For U-Boot SPL yeah I expect the constraints to be different. We're getting a bit closer to the metal on the rb3gen2 (now it's just bootROM -> SBL1 -> (tz -> hyp) -> U-Boot, without going through edk2 first haha).
You should teach rpi to do that too! At the moment I think it boots edk2 before U-Boot.
I hope we'll get to do U-Boot SPL on some Qualcomm platforms eventually.
Anyway, certainly OF_LIVE being the default would be good. I have often wondered if we can (at build-time) convert the devicetree into a 'live' version, where the pointers are replaced by integers, such that the early U-Boot code can easily compute the pointer value for each node. It should make the unflattening much faster. For pre-relocation and SPL, since we know the load address, we can (I am pretty sure) have Binman put a full, live tree in the image and avoid the unflattening code at all. Relocating a livetree is fairly easy too.
This might be a nice optimisation? I think it would be acceptable to just read the memory layout -> enable the MMU -> build the live tree. This would probably be fast enough.
And of course, the great thing we have on Qualcomm platforms is that we can run the same U-Boot binary on every sdm845 and newer platform.
That's nice, and a good demo of devicetree done right.
If I don't hear any strong preference from you I am likely to have a go (in the fullness of time) at implementing the easiest DM-based solution to get rid of that binding function you have. We can iterate on it in the review in any case.
Regards, Simon
On 22/08/2024 04:59, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 14:33, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 20:27, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 10:47, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 18:16, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 16:37, Simon Glass wrote: > Hi Caleb, > > On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote: >> >> Hi Simon, >>>> +U_BOOT_DRIVER(gcc_sc7280) = { >>>> + .name = "gcc_sc7280", >>>> + .id = UCLASS_NOP, >>>> + .of_match = gcc_sc7280_of_match, >>>> + .bind = qcom_cc_bind, >>>> + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, >>>> +}; >>> >>> This should use driver model, with a UCLASS_CLK and UCLASS_RESET >> >> Please refer to qcom_cc_bind() which binds the clock, reset, and power >> domain drivers. > > Gosh, why? Are these devices not in the devicetree?
They are, the gcc block contains clock, reset, and pd parts. On Linux this is not an issue because a single device can be multiple different classes (e..g when you register a reset you do it for a device) whereas U-Boot requires a device per class.
e.g. see devm_reset_controller_register() in Linux, it populates a struct reset_controller_dev which references the struct device created for the node. In U-Boot you have to create a new device which _is_ the reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this.
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
I'm only really involved in mainline and don't really see vendor trees much. An example is where pinctrl has a GPIO controller but it is not mentioned in the devicetree. It would be better for U-Boot to add a subnode for each GPIO controller. In general, if the SoC has a device, it should be in the devicetree.
The concept of a device is an OS one. DT is not "telling the OS how to use the hardware", it is describing the hardware.
This distinction is important because it's the only way to ensure that future OS changes can be done regardless of the DT. And also of course because different OS's will have different ideas of how to model devices (case in point).
The GCC block on Qualcomm platforms is a single hardware block. The datasheets and hardware programming guides describe it as such. The clocks, resets, and GDSCs are all entwined at the hardware level. They also have overlapping register addresses.
The further away DT gets from describing the hardware in favour of simplifying the OS, the more likely we are to start running into issues with fitting hardware changes into our arbitrary model.
I completely agree with everything you are saying, but you don't go far enough. We should additionally require that all hardware has a description in the devicetree. See for example the GPIO controller I mentioned. When Linux wants it, it gets it, when it doesn't, it isn't there. Sorry to have to say it, but that's not right.
If it's only used in U-Boot, that's justification enough to add the node upstream?
Part of this difference (between U-Boot and Linux) comes about because Linux device setup is fairly manual, whereas U-Boot tries to put all of that in common DM code. Whenever you are including dm/device-internal.h that is often a sign that the binding is causing issues.
To me this indicates an inability for U-Boot's DM to handle complicated devices. I don't think U-Boot should dictate the design of devicetree.
I don't know how else to describe this. This issue has been litigated over and over again on the kernel mailing list. Every time someone suggests changing DT because of a limitation in Linux they are (rightfully) shut down. If these changes were accepted then it would be impossible for DT to be an OS agnostic hardware description, because it would be full of OS specific hacks.
Given that an OS has no size limitations so can afford to do just about anything to deal with one-off cases in each SoC, sure. But let's face it, it isn't project-agnostic. I could provide dozens of examples where the bindings are a pain for U-Boot. Even the use of strings in some of the SoCs' pinctrl bindings is painful. My point is that there are many ways to model and describe the hardware and taking more account of all users would be a big step forward.
Sure, I can understand wanting to prioritize size/speed. I think livetree would help a lot here.
I'm happy for you to change my mind.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
- Allow DM to bind multiple devices to each devicetree node, perhaps
as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
You would have to repeat the same compatible string in each driver.
For generic compatible strings we could a) worry about it later or b) restrict this technique to only nodes with a single compatible string, or c) use the driver flag as mentioned
- As 1 but have DM create a parent 'UCLASS_MULTI' device
automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
How come? What is qcom_cc_bind() doing which DM couldn't?
Maybe DM could look at the "#clock-cells", "#reset-cells", and "#power-domain-cells" properties and match on compatible string + uclass? I think that could work.
Hmmm you mean when it sees those in the node it knows the additional uclasses it is allowed to use?
Yes, the "#reset-cells" property literally means "this is a reset controller".
- Implement devices which are in more than one uclass. There would
still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
In U-Boot the uclass is a stronger concept, e.g. you can generically iterate through all devices in a uclass, and all devices have one.
What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
What is the Linux model, in this sense? Whenever I see barebox I wonder why we can't fold whatever new features it needs into U-Boot...perhaps the code bases have converged too much...?
barebox is livetree only (like Linux) as I understand it, and I think it aims to be literally copy/paste compatible with Linux. I would love for U-Boot to adopt this approach.
OK. We do have code-size restrictions though.
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
Well I would like to tidy this up in DM, so let's figure out which option makes the most sense...once I understand what you mean by 'Linux model' above.
I mean where a single struct device can be multiple classes, so you create a device and then create a reset controller and associate the device with it.
It's the bit about having one driver manually creating devices that I very much want to avoid. If the devicetree really does (fully) describe hardware, it shouldn't be necessary.
For sure
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
So far as I can tell, you are always going to have a flat tree, even if only before relocation or in SPL. How would we get around that? Also, what don't you like about ofnode?
Yeah, you start with a flat tree and then should build a live one as early as possible.
The problem with ofnode is that it is more or less restricted to the subset of operations fdt supports. It lacks some of the fancier features of a live tree. It also means that every use of the of_* API has to be preceded by a check that we're actually running the live tree, otherwise some kind of hard bail.
Indeed. In principle, ofnode could support anything, but the cost might be high when implemented in the flat tree. People do add new functions from time to time.
Would be great to use of_* API by default, for capable platforms anyway.
Given that Qualcomm is only using U-Boot as a second-stage loader so far, (please, not for long!!) everything looks quite different. But most platforms use U-Boot from SoC-boot-ROM handoff, so the constraints are different (tighter).
For U-Boot SPL yeah I expect the constraints to be different. We're getting a bit closer to the metal on the rb3gen2 (now it's just bootROM -> SBL1 -> (tz -> hyp) -> U-Boot, without going through edk2 first haha).
You should teach rpi to do that too! At the moment I think it boots edk2 before U-Boot.
I hope we'll get to do U-Boot SPL on some Qualcomm platforms eventually.
Anyway, certainly OF_LIVE being the default would be good. I have often wondered if we can (at build-time) convert the devicetree into a 'live' version, where the pointers are replaced by integers, such that the early U-Boot code can easily compute the pointer value for each node. It should make the unflattening much faster. For pre-relocation and SPL, since we know the load address, we can (I am pretty sure) have Binman put a full, live tree in the image and avoid the unflattening code at all. Relocating a livetree is fairly easy too.
This might be a nice optimisation? I think it would be acceptable to just read the memory layout -> enable the MMU -> build the live tree. This would probably be fast enough.
And of course, the great thing we have on Qualcomm platforms is that we can run the same U-Boot binary on every sdm845 and newer platform.
That's nice, and a good demo of devicetree done right.
If I don't hear any strong preference from you I am likely to have a go (in the fullness of time) at implementing the easiest DM-based solution to get rid of that binding function you have. We can iterate on it in the review in any case.
That makes sense. I assume in the mean time it's ok for me to take this patch?
Kind regards,
Regards, Simon
On 22/08/2024 14:12, Caleb Connolly wrote:
On 22/08/2024 04:59, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 14:33, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 20:27, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 10:47, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 18:16, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote: > > > > On 21/08/2024 16:37, Simon Glass wrote: >> Hi Caleb, >> >> On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote: >>> >>> Hi Simon, >>>>> +U_BOOT_DRIVER(gcc_sc7280) = { >>>>> +      .name          = "gcc_sc7280", >>>>> +      .id            = UCLASS_NOP, >>>>> +      .of_match      = gcc_sc7280_of_match, >>>>> +      .bind          = qcom_cc_bind, >>>>> +      .flags         = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, >>>>> +}; >>>> >>>> This should use driver model, with a UCLASS_CLK and UCLASS_RESET >>> >>> Please refer to qcom_cc_bind() which binds the clock, reset, and power >>> domain drivers. >> >> Gosh, why? Are these devices not in the devicetree? > > They are, the gcc block contains clock, reset, and pd parts. On Linux > this is not an issue because a single device can be multiple different > classes (e..g when you register a reset you do it for a device) whereas > U-Boot requires a device per class. > > e.g. see devm_reset_controller_register() in Linux, it populates a > struct reset_controller_dev which references the struct device created > for the node. In U-Boot you have to create a new device which _is_ the > reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this.
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
I'm only really involved in mainline and don't really see vendor trees much. An example is where pinctrl has a GPIO controller but it is not mentioned in the devicetree. It would be better for U-Boot to add a subnode for each GPIO controller. In general, if the SoC has a device, it should be in the devicetree.
The concept of a device is an OS one. DT is not "telling the OS how to use the hardware", it is describing the hardware.
This distinction is important because it's the only way to ensure that future OS changes can be done regardless of the DT. And also of course because different OS's will have different ideas of how to model devices (case in point).
The GCC block on Qualcomm platforms is a single hardware block. The datasheets and hardware programming guides describe it as such. The clocks, resets, and GDSCs are all entwined at the hardware level. They also have overlapping register addresses.
The further away DT gets from describing the hardware in favour of simplifying the OS, the more likely we are to start running into issues with fitting hardware changes into our arbitrary model.
I completely agree with everything you are saying, but you don't go far enough. We should additionally require that all hardware has a description in the devicetree. See for example the GPIO controller I mentioned. When Linux wants it, it gets it, when it doesn't, it isn't there. Sorry to have to say it, but that's not right.
If it's only used in U-Boot, that's justification enough to add the node upstream?
Part of this difference (between U-Boot and Linux) comes about because Linux device setup is fairly manual, whereas U-Boot tries to put all of that in common DM code. Whenever you are including dm/device-internal.h that is often a sign that the binding is causing issues.
To me this indicates an inability for U-Boot's DM to handle complicated devices. I don't think U-Boot should dictate the design of devicetree.
I don't know how else to describe this. This issue has been litigated over and over again on the kernel mailing list. Every time someone suggests changing DT because of a limitation in Linux they are (rightfully) shut down. If these changes were accepted then it would be impossible for DT to be an OS agnostic hardware description, because it would be full of OS specific hacks.
Given that an OS has no size limitations so can afford to do just about anything to deal with one-off cases in each SoC, sure. But let's face it, it isn't project-agnostic. I could provide dozens of examples where the bindings are a pain for U-Boot. Even the use of strings in some of the SoCs' pinctrl bindings is painful. My point is that there are many ways to model and describe the hardware and taking more account of all users would be a big step forward.
Sure, I can understand wanting to prioritize size/speed. I think livetree would help a lot here.
I'm happy for you to change my mind.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
- Allow DM to bind multiple devices to each devicetree node, perhaps
as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
You would have to repeat the same compatible string in each driver.
For generic compatible strings we could a) worry about it later or b) restrict this technique to only nodes with a single compatible string, or c) use the driver flag as mentioned
- As 1 but have DM create a parent 'UCLASS_MULTI' device
automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
How come? What is qcom_cc_bind() doing which DM couldn't?
Maybe DM could look at the "#clock-cells", "#reset-cells", and "#power-domain-cells" properties and match on compatible string + uclass? I think that could work.
Hmmm you mean when it sees those in the node it knows the additional uclasses it is allowed to use?
Yes, the "#reset-cells" property literally means "this is a reset controller".
Well technically it's more "here is the number of parameters I expect for phandles to my node as a reset controller", but since it's required to support phandles for reset handles, having the #<>-cells can be an hint that the node is supposed to act as a reset controller since some other nodes references the current node via a reset phandle.
Since won't solve node that can act as provides _and_ other services, like it's common to have like a mmc controller that can also act as clock controller, you'll have the #clock-cells, but no #mmc-cells since no other nodes depends on it.
You also have the case for firmware nodes, where the link between users and consumers is not described in DT but only in code, like the qcom scm or the amlogic securemonitor.
So I won't base myself on the #<>-cells property.
Neil
- Implement devices which are in more than one uclass. There would
still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
In U-Boot the uclass is a stronger concept, e.g. you can generically iterate through all devices in a uclass, and all devices have one.
What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
What is the Linux model, in this sense? Whenever I see barebox I wonder why we can't fold whatever new features it needs into U-Boot...perhaps the code bases have converged too much...?
barebox is livetree only (like Linux) as I understand it, and I think it aims to be literally copy/paste compatible with Linux. I would love for U-Boot to adopt this approach.
OK. We do have code-size restrictions though.
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
Well I would like to tidy this up in DM, so let's figure out which option makes the most sense...once I understand what you mean by 'Linux model' above.
I mean where a single struct device can be multiple classes, so you create a device and then create a reset controller and associate the device with it.
It's the bit about having one driver manually creating devices that I very much want to avoid. If the devicetree really does (fully) describe hardware, it shouldn't be necessary.
For sure
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
So far as I can tell, you are always going to have a flat tree, even if only before relocation or in SPL. How would we get around that? Also, what don't you like about ofnode?
Yeah, you start with a flat tree and then should build a live one as early as possible.
The problem with ofnode is that it is more or less restricted to the subset of operations fdt supports. It lacks some of the fancier features of a live tree. It also means that every use of the of_* API has to be preceded by a check that we're actually running the live tree, otherwise some kind of hard bail.
Indeed. In principle, ofnode could support anything, but the cost might be high when implemented in the flat tree. People do add new functions from time to time.
Would be great to use of_* API by default, for capable platforms anyway.
Given that Qualcomm is only using U-Boot as a second-stage loader so far, (please, not for long!!) everything looks quite different. But most platforms use U-Boot from SoC-boot-ROM handoff, so the constraints are different (tighter).
For U-Boot SPL yeah I expect the constraints to be different. We're getting a bit closer to the metal on the rb3gen2 (now it's just bootROM -> SBL1 -> (tz -> hyp) -> U-Boot, without going through edk2 first haha).
You should teach rpi to do that too! At the moment I think it boots edk2 before U-Boot.
I hope we'll get to do U-Boot SPL on some Qualcomm platforms eventually.
Anyway, certainly OF_LIVE being the default would be good. I have often wondered if we can (at build-time) convert the devicetree into a 'live' version, where the pointers are replaced by integers, such that the early U-Boot code can easily compute the pointer value for each node. It should make the unflattening much faster. For pre-relocation and SPL, since we know the load address, we can (I am pretty sure) have Binman put a full, live tree in the image and avoid the unflattening code at all. Relocating a livetree is fairly easy too.
This might be a nice optimisation? I think it would be acceptable to just read the memory layout -> enable the MMU -> build the live tree. This would probably be fast enough.
And of course, the great thing we have on Qualcomm platforms is that we can run the same U-Boot binary on every sdm845 and newer platform.
That's nice, and a good demo of devicetree done right.
If I don't hear any strong preference from you I am likely to have a go (in the fullness of time) at implementing the easiest DM-based solution to get rid of that binding function you have. We can iterate on it in the review in any case.
That makes sense. I assume in the mean time it's ok for me to take this patch?
Kind regards,
Regards, Simon
Hi Caleb,
On Thu, 22 Aug 2024 at 06:12, Caleb Connolly caleb.connolly@linaro.org wrote:
On 22/08/2024 04:59, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 14:33, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 20:27, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 10:47, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 18:16, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote: > > > > On 21/08/2024 16:37, Simon Glass wrote: >> Hi Caleb, >> >> On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote: >>> >>> Hi Simon, >>>>> +U_BOOT_DRIVER(gcc_sc7280) = { >>>>> + .name = "gcc_sc7280", >>>>> + .id = UCLASS_NOP, >>>>> + .of_match = gcc_sc7280_of_match, >>>>> + .bind = qcom_cc_bind, >>>>> + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, >>>>> +}; >>>> >>>> This should use driver model, with a UCLASS_CLK and UCLASS_RESET >>> >>> Please refer to qcom_cc_bind() which binds the clock, reset, and power >>> domain drivers. >> >> Gosh, why? Are these devices not in the devicetree? > > They are, the gcc block contains clock, reset, and pd parts. On Linux > this is not an issue because a single device can be multiple different > classes (e..g when you register a reset you do it for a device) whereas > U-Boot requires a device per class. > > e.g. see devm_reset_controller_register() in Linux, it populates a > struct reset_controller_dev which references the struct device created > for the node. In U-Boot you have to create a new device which _is_ the > reset controller.
OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon to access registers. The clock driver 'owns' the node in U-Boot and it manually binds a reset driver. It isn't great, either.
Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
Re devm_reset_controller_register(), yes the U-Boot driver model is a lot more regular, e.g. we don't really want drivers creating their own struct udevice. We want all devices to be created automatically by the devicetree and most memory allocations to be done automatically. This helps to reduce code size and execution time. You probably know all this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this.
To a significant degree, the devicetree bindings are created without much thought to efficient operation in U-Boot. I hope that eventually this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
I'm only really involved in mainline and don't really see vendor trees much. An example is where pinctrl has a GPIO controller but it is not mentioned in the devicetree. It would be better for U-Boot to add a subnode for each GPIO controller. In general, if the SoC has a device, it should be in the devicetree.
The concept of a device is an OS one. DT is not "telling the OS how to use the hardware", it is describing the hardware.
This distinction is important because it's the only way to ensure that future OS changes can be done regardless of the DT. And also of course because different OS's will have different ideas of how to model devices (case in point).
The GCC block on Qualcomm platforms is a single hardware block. The datasheets and hardware programming guides describe it as such. The clocks, resets, and GDSCs are all entwined at the hardware level. They also have overlapping register addresses.
The further away DT gets from describing the hardware in favour of simplifying the OS, the more likely we are to start running into issues with fitting hardware changes into our arbitrary model.
I completely agree with everything you are saying, but you don't go far enough. We should additionally require that all hardware has a description in the devicetree. See for example the GPIO controller I mentioned. When Linux wants it, it gets it, when it doesn't, it isn't there. Sorry to have to say it, but that's not right.
If it's only used in U-Boot, that's justification enough to add the node upstream?
Haven't you just defeated your assertion though? By saying that if 'only U-Boot uses' then the binding should not be included, it confirms in my mind that there are two different rules here.
Is the devicetree just for Linux, or not?
To repeat, the rule should be that all hardware has a description in the devicetree. That would help U-Boot a lot.
Part of this difference (between U-Boot and Linux) comes about because Linux device setup is fairly manual, whereas U-Boot tries to put all of that in common DM code. Whenever you are including dm/device-internal.h that is often a sign that the binding is causing issues.
To me this indicates an inability for U-Boot's DM to handle complicated devices. I don't think U-Boot should dictate the design of devicetree.
I don't know how else to describe this. This issue has been litigated over and over again on the kernel mailing list. Every time someone suggests changing DT because of a limitation in Linux they are (rightfully) shut down. If these changes were accepted then it would be impossible for DT to be an OS agnostic hardware description, because it would be full of OS specific hacks.
Given that an OS has no size limitations so can afford to do just about anything to deal with one-off cases in each SoC, sure. But let's face it, it isn't project-agnostic. I could provide dozens of examples where the bindings are a pain for U-Boot. Even the use of strings in some of the SoCs' pinctrl bindings is painful. My point is that there are many ways to model and describe the hardware and taking more account of all users would be a big step forward.
Sure, I can understand wanting to prioritize size/speed. I think livetree would help a lot here.
I'm happy for you to change my mind.
Anyway, with the devicetree we have, I wonder how we could do this better?
Some ideas:
- Allow DM to bind multiple devices to each devicetree node, perhaps
as a Kconfig option (to avoid time penalty for all boards), or by requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
You would have to repeat the same compatible string in each driver.
For generic compatible strings we could a) worry about it later or b) restrict this technique to only nodes with a single compatible string, or c) use the driver flag as mentioned
- As 1 but have DM create a parent 'UCLASS_MULTI' device
automatically. I am thinking that we should have a new uclass for these things, or rename the NOP uclass. This option would allow easy access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
How come? What is qcom_cc_bind() doing which DM couldn't?
Maybe DM could look at the "#clock-cells", "#reset-cells", and "#power-domain-cells" properties and match on compatible string + uclass? I think that could work.
Hmmm you mean when it sees those in the node it knows the additional uclasses it is allowed to use?
Yes, the "#reset-cells" property literally means "this is a reset controller".
But we know that anyway, don't we, since the reset controller is in the DT and would have the same compatible string?
- Implement devices which are in more than one uclass. There would
still be a primary uclass, but others can be added too. This would involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() macro. We would then have a struct dmtag_node for each secondary uclass, containing the ID and the uclass pointer. Certain functions would need to be updated to support this, and again it could be behind a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
In U-Boot the uclass is a stronger concept, e.g. you can generically iterate through all devices in a uclass, and all devices have one.
What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
What is the Linux model, in this sense? Whenever I see barebox I wonder why we can't fold whatever new features it needs into U-Boot...perhaps the code bases have converged too much...?
barebox is livetree only (like Linux) as I understand it, and I think it aims to be literally copy/paste compatible with Linux. I would love for U-Boot to adopt this approach.
OK. We do have code-size restrictions though.
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
Well I would like to tidy this up in DM, so let's figure out which option makes the most sense...once I understand what you mean by 'Linux model' above.
I mean where a single struct device can be multiple classes, so you create a device and then create a reset controller and associate the device with it.
It's the bit about having one driver manually creating devices that I very much want to avoid. If the devicetree really does (fully) describe hardware, it shouldn't be necessary.
For sure
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
So far as I can tell, you are always going to have a flat tree, even if only before relocation or in SPL. How would we get around that? Also, what don't you like about ofnode?
Yeah, you start with a flat tree and then should build a live one as early as possible.
The problem with ofnode is that it is more or less restricted to the subset of operations fdt supports. It lacks some of the fancier features of a live tree. It also means that every use of the of_* API has to be preceded by a check that we're actually running the live tree, otherwise some kind of hard bail.
Indeed. In principle, ofnode could support anything, but the cost might be high when implemented in the flat tree. People do add new functions from time to time.
Would be great to use of_* API by default, for capable platforms anyway.
Given that Qualcomm is only using U-Boot as a second-stage loader so far, (please, not for long!!) everything looks quite different. But most platforms use U-Boot from SoC-boot-ROM handoff, so the constraints are different (tighter).
For U-Boot SPL yeah I expect the constraints to be different. We're getting a bit closer to the metal on the rb3gen2 (now it's just bootROM -> SBL1 -> (tz -> hyp) -> U-Boot, without going through edk2 first haha).
You should teach rpi to do that too! At the moment I think it boots edk2 before U-Boot.
I hope we'll get to do U-Boot SPL on some Qualcomm platforms eventually.
Anyway, certainly OF_LIVE being the default would be good. I have often wondered if we can (at build-time) convert the devicetree into a 'live' version, where the pointers are replaced by integers, such that the early U-Boot code can easily compute the pointer value for each node. It should make the unflattening much faster. For pre-relocation and SPL, since we know the load address, we can (I am pretty sure) have Binman put a full, live tree in the image and avoid the unflattening code at all. Relocating a livetree is fairly easy too.
This might be a nice optimisation? I think it would be acceptable to just read the memory layout -> enable the MMU -> build the live tree. This would probably be fast enough.
And of course, the great thing we have on Qualcomm platforms is that we can run the same U-Boot binary on every sdm845 and newer platform.
That's nice, and a good demo of devicetree done right.
If I don't hear any strong preference from you I am likely to have a go (in the fullness of time) at implementing the easiest DM-based solution to get rid of that binding function you have. We can iterate on it in the review in any case.
That makes sense. I assume in the mean time it's ok for me to take this patch?
Yes, thanks for the discussion and for helping with my understanding. It might be a while before I get to doing a patch, but I would like to figure out how to deal with these multi-uclass devices as it happens on other SoCs too.
Reviewed-by: Simon Glass sjg@chromium.org
Regards, Simon
On 22/08/2024 18:54, Simon Glass wrote:
Hi Caleb,
On Thu, 22 Aug 2024 at 06:12, Caleb Connolly caleb.connolly@linaro.org wrote:
On 22/08/2024 04:59, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 14:33, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 20:27, Simon Glass wrote:
Hi Caleb,
On Wed, 21 Aug 2024 at 10:47, Caleb Connolly caleb.connolly@linaro.org wrote:
On 21/08/2024 18:16, Simon Glass wrote: > Hi Caleb, > > On Wed, 21 Aug 2024 at 08:49, Caleb Connolly caleb.connolly@linaro.org wrote: >> >> >> >> On 21/08/2024 16:37, Simon Glass wrote: >>> Hi Caleb, >>> >>> On Wed, 21 Aug 2024 at 08:11, Caleb Connolly caleb.connolly@linaro.org wrote: >>>> >>>> Hi Simon, >>>>>> +U_BOOT_DRIVER(gcc_sc7280) = { >>>>>> + .name = "gcc_sc7280", >>>>>> + .id = UCLASS_NOP, >>>>>> + .of_match = gcc_sc7280_of_match, >>>>>> + .bind = qcom_cc_bind, >>>>>> + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF, >>>>>> +}; >>>>> >>>>> This should use driver model, with a UCLASS_CLK and UCLASS_RESET >>>> >>>> Please refer to qcom_cc_bind() which binds the clock, reset, and power >>>> domain drivers. >>> >>> Gosh, why? Are these devices not in the devicetree? >> >> They are, the gcc block contains clock, reset, and pd parts. On Linux >> this is not an issue because a single device can be multiple different >> classes (e..g when you register a reset you do it for a device) whereas >> U-Boot requires a device per class. >> >> e.g. see devm_reset_controller_register() in Linux, it populates a >> struct reset_controller_dev which references the struct device created >> for the node. In U-Boot you have to create a new device which _is_ the >> reset controller. > > OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon > to access registers. The clock driver 'owns' the node in U-Boot and it > manually binds a reset driver. It isn't great, either. > > Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't > actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a > binding. Can you please point me to the node?
It's in dts/upstream/src/arm64/qcom/sdm845.dtsi
> > Re devm_reset_controller_register(), yes the U-Boot driver model is a > lot more regular, e.g. we don't really want drivers creating their own > struct udevice. We want all devices to be created automatically by the > devicetree and most memory allocations to be done automatically. This > helps to reduce code size and execution time. You probably know all > this :-)
Yeah, U-Boot's model is simpler for most cases. This makes sense. But it doesn't reflect the reality of DT so well in cases like this. > > To a significant degree, the devicetree bindings are created without > much thought to efficient operation in U-Boot. I hope that eventually > this might change.
I strongly disagree with this mental model. This is the approach I see vendors take in their BSP sources and the result is not pleasant.
DT should (within reason) never be written with the OS in mind. It is an agnostic structure to describe the hardware. I think the new power sequencing subsystem in Linux does a good job at embodying how we should approach consuming DT.
I'm only really involved in mainline and don't really see vendor trees much. An example is where pinctrl has a GPIO controller but it is not mentioned in the devicetree. It would be better for U-Boot to add a subnode for each GPIO controller. In general, if the SoC has a device, it should be in the devicetree.
The concept of a device is an OS one. DT is not "telling the OS how to use the hardware", it is describing the hardware.
This distinction is important because it's the only way to ensure that future OS changes can be done regardless of the DT. And also of course because different OS's will have different ideas of how to model devices (case in point).
The GCC block on Qualcomm platforms is a single hardware block. The datasheets and hardware programming guides describe it as such. The clocks, resets, and GDSCs are all entwined at the hardware level. They also have overlapping register addresses.
The further away DT gets from describing the hardware in favour of simplifying the OS, the more likely we are to start running into issues with fitting hardware changes into our arbitrary model.
I completely agree with everything you are saying, but you don't go far enough. We should additionally require that all hardware has a description in the devicetree. See for example the GPIO controller I mentioned. When Linux wants it, it gets it, when it doesn't, it isn't there. Sorry to have to say it, but that's not right.
If it's only used in U-Boot, that's justification enough to add the node upstream?
Haven't you just defeated your assertion though? By saying that if 'only U-Boot uses' then the binding should not be included, it confirms in my mind that there are two different rules here.
No, my issue was with the idea that we change DT or architect it to serve U-Boot. Obviously if a controller is missing that U-Boot needs it makes sense to add it.
Is the devicetree just for Linux, or not?
To repeat, the rule should be that all hardware has a description in the devicetree. That would help U-Boot a lot.
Part of this difference (between U-Boot and Linux) comes about because Linux device setup is fairly manual, whereas U-Boot tries to put all of that in common DM code. Whenever you are including dm/device-internal.h that is often a sign that the binding is causing issues.
To me this indicates an inability for U-Boot's DM to handle complicated devices. I don't think U-Boot should dictate the design of devicetree.
I don't know how else to describe this. This issue has been litigated over and over again on the kernel mailing list. Every time someone suggests changing DT because of a limitation in Linux they are (rightfully) shut down. If these changes were accepted then it would be impossible for DT to be an OS agnostic hardware description, because it would be full of OS specific hacks.
Given that an OS has no size limitations so can afford to do just about anything to deal with one-off cases in each SoC, sure. But let's face it, it isn't project-agnostic. I could provide dozens of examples where the bindings are a pain for U-Boot. Even the use of strings in some of the SoCs' pinctrl bindings is painful. My point is that there are many ways to model and describe the hardware and taking more account of all users would be a big step forward.
Sure, I can understand wanting to prioritize size/speed. I think livetree would help a lot here.
I'm happy for you to change my mind.
> > Anyway, with the devicetree we have, I wonder how we could do this better? > > Some ideas: > > 1. Allow DM to bind multiple devices to each devicetree node, perhaps > as a Kconfig option (to avoid time penalty for all boards), or by > requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be > independent, with no common parent device
This would make sharing match data hard, and likely cause issues with generic compatible strings.
You would have to repeat the same compatible string in each driver.
For generic compatible strings we could a) worry about it later or b) restrict this technique to only nodes with a single compatible string, or c) use the driver flag as mentioned
> > 2. As 1 but have DM create a parent 'UCLASS_MULTI' device > automatically. I am thinking that we should have a new uclass for > these things, or rename the NOP uclass. This option would allow easy > access to the parent device, if needed.
This is the current approach, we just bind the clk/reset/pd drivers explicitly, allowing us to create and share common data. I don't believe there is a sensible way to do this generically.
How come? What is qcom_cc_bind() doing which DM couldn't?
Maybe DM could look at the "#clock-cells", "#reset-cells", and "#power-domain-cells" properties and match on compatible string + uclass? I think that could work.
Hmmm you mean when it sees those in the node it knows the additional uclasses it is allowed to use?
Yes, the "#reset-cells" property literally means "this is a reset controller".
But we know that anyway, don't we, since the reset controller is in the DT and would have the same compatible string?
> > 3. Implement devices which are in more than one uclass. There would > still be a primary uclass, but others can be added too. This would > involve declaring a list of secondary uclasses in the U_BOOT_DRIVER() > macro. We would then have a struct dmtag_node for each secondary > uclass, containing the ID and the uclass pointer. Certain functions > would need to be updated to support this, and again it could be behind > a Kconfig.
Many device classes in U-Boot rely on going from a struct udevice to some uclass specific data or ops. I have always found this to be a bit odd, though simpler to deal with than Linux.
In U-Boot the uclass is a stronger concept, e.g. you can generically iterate through all devices in a uclass, and all devices have one.
> > What do you think?
I think if we're to try and solve this at all, the Linux model is by far the most sensible. It is already tried and tested, and would have the huge bonus of simplifying driver porting.
barebox went with this approach and it seems to have worked out quite well for them.
What is the Linux model, in this sense? Whenever I see barebox I wonder why we can't fold whatever new features it needs into U-Boot...perhaps the code bases have converged too much...?
barebox is livetree only (like Linux) as I understand it, and I think it aims to be literally copy/paste compatible with Linux. I would love for U-Boot to adopt this approach.
OK. We do have code-size restrictions though.
All that being said, while it's taken me some time to get my head around "the U-Boot way", I think there is still value in the simplicity of U-Boot's approach. I also think the solution we've ended up with (after many iterations I might add) in clock-qcom is clean, simple, and easy to understand; though I do agree that U-Boot's DM is definitely hitting the limit of what complexity it can handle.
Well I would like to tidy this up in DM, so let's figure out which option makes the most sense...once I understand what you mean by 'Linux model' above.
I mean where a single struct device can be multiple classes, so you create a device and then create a reset controller and associate the device with it.
It's the bit about having one driver manually creating devices that I very much want to avoid. If the devicetree really does (fully) describe hardware, it shouldn't be necessary.
For sure
I would honestly be much more interested in seeing early init get cleaned up, OF_LIVE becoming the default, and the ofnode abstraction going away.
So far as I can tell, you are always going to have a flat tree, even if only before relocation or in SPL. How would we get around that? Also, what don't you like about ofnode?
Yeah, you start with a flat tree and then should build a live one as early as possible.
The problem with ofnode is that it is more or less restricted to the subset of operations fdt supports. It lacks some of the fancier features of a live tree. It also means that every use of the of_* API has to be preceded by a check that we're actually running the live tree, otherwise some kind of hard bail.
Indeed. In principle, ofnode could support anything, but the cost might be high when implemented in the flat tree. People do add new functions from time to time.
Would be great to use of_* API by default, for capable platforms anyway.
Given that Qualcomm is only using U-Boot as a second-stage loader so far, (please, not for long!!) everything looks quite different. But most platforms use U-Boot from SoC-boot-ROM handoff, so the constraints are different (tighter).
For U-Boot SPL yeah I expect the constraints to be different. We're getting a bit closer to the metal on the rb3gen2 (now it's just bootROM -> SBL1 -> (tz -> hyp) -> U-Boot, without going through edk2 first haha).
You should teach rpi to do that too! At the moment I think it boots edk2 before U-Boot.
I hope we'll get to do U-Boot SPL on some Qualcomm platforms eventually.
Anyway, certainly OF_LIVE being the default would be good. I have often wondered if we can (at build-time) convert the devicetree into a 'live' version, where the pointers are replaced by integers, such that the early U-Boot code can easily compute the pointer value for each node. It should make the unflattening much faster. For pre-relocation and SPL, since we know the load address, we can (I am pretty sure) have Binman put a full, live tree in the image and avoid the unflattening code at all. Relocating a livetree is fairly easy too.
This might be a nice optimisation? I think it would be acceptable to just read the memory layout -> enable the MMU -> build the live tree. This would probably be fast enough.
And of course, the great thing we have on Qualcomm platforms is that we can run the same U-Boot binary on every sdm845 and newer platform.
That's nice, and a good demo of devicetree done right.
If I don't hear any strong preference from you I am likely to have a go (in the fullness of time) at implementing the easiest DM-based solution to get rid of that binding function you have. We can iterate on it in the review in any case.
That makes sense. I assume in the mean time it's ok for me to take this patch?
Yes, thanks for the discussion and for helping with my understanding. It might be a while before I get to doing a patch, but I would like to figure out how to deal with these multi-uclass devices as it happens on other SoCs too.
Reviewed-by: Simon Glass sjg@chromium.org
Thanks and regards,
Regards, Simon
For running U-Boot as primary bootloader we must define the memory layout statically.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi b/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi new file mode 100644 index 000000000000..c3ec4a317f7c --- /dev/null +++ b/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Copyright (c) 2024 Linaro Ltd. + */ +/ { + /* When running as the primary bootloader there is no prior + * stage to populate the memory layout for us. We *should* + * have two nodes here, but ABL does NOT like that. + * sooo we're stuck with this. + */ + memory@80000000 { + device_type = "memory"; + reg = <0 0x80000000 0 0x3A800000>, + <0 0xC0000000 0 0x01800000>, + <0 0xC3400000 0 0x3CC00000>, + <1 0x00000000 1 0x00000000>; + }; +}; + +// RAM Entry 0 : Base 0x0080000000 Size 0x003A800000 +// RAM Entry 1 : Base 0x00C0000000 Size 0x0001800000 +// RAM Entry 2 : Base 0x00C3400000 Size 0x003CC00000 +// RAM Entry 3 : Base 0x0100000000 Size 0x0100000000
Hi Caleb,
On Wed, 21 Aug 2024 at 07:42, Caleb Connolly caleb.connolly@linaro.org wrote:
For running U-Boot as primary bootloader we must define the memory layout statically.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi b/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi new file mode 100644 index 000000000000..c3ec4a317f7c --- /dev/null +++ b/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: BSD-3-Clause +/*
- Copyright (c) 2024 Linaro Ltd.
- */
+/ {
/* When running as the primary bootloader there is no prior
We normally have an empty /* on the first line
* stage to populate the memory layout for us. We *should** have two nodes here, but ABL does NOT like that.* sooo we're stuck with this.*/memory@80000000 {device_type = "memory";reg = <0 0x80000000 0 0x3A800000>,<0 0xC0000000 0 0x01800000>,<0 0xC3400000 0 0x3CC00000>,<1 0x00000000 1 0x00000000>;};+};
+// RAM Entry 0 : Base 0x0080000000 Size 0x003A800000 +// RAM Entry 1 : Base 0x00C0000000 Size 0x0001800000 +// RAM Entry 2 : Base 0x00C3400000 Size 0x003CC00000 +// RAM Entry 3 : Base 0x0100000000 Size 0x0100000000
-- 2.46.0
Can you please use lower-case hex?
Regards, Simon
Adjust DTS so USB runs in host mode. The type-c port is the only supported port (since the others need PCIe). Booting from USB is possible with a powered type-c dock.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi b/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi index c3ec4a317f7c..fbe72595f5ac 100644 --- a/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi +++ b/arch/arm/dts/qcs6490-rb3gen2-u-boot.dtsi @@ -16,8 +16,13 @@ <1 0x00000000 1 0x00000000>; }; };
+&usb_1_dwc3 { + dr_mode = "host"; + /delete-property/ usb-role-switch; +}; + // RAM Entry 0 : Base 0x0080000000 Size 0x003A800000 // RAM Entry 1 : Base 0x00C0000000 Size 0x0001800000 // RAM Entry 2 : Base 0x00C3400000 Size 0x003CC00000 // RAM Entry 3 : Base 0x0100000000 Size 0x0100000000
This soc doesn't have the generic compatible.
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- drivers/iommu/qcom-hyp-smmu.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/qcom-hyp-smmu.c b/drivers/iommu/qcom-hyp-smmu.c index 7b646d840dd4..1b5a09bb7b39 100644 --- a/drivers/iommu/qcom-hyp-smmu.c +++ b/drivers/iommu/qcom-hyp-smmu.c @@ -380,8 +380,9 @@ static struct iommu_ops qcom_smmu_ops = { };
static const struct udevice_id qcom_smmu500_ids[] = { { .compatible = "qcom,sdm845-smmu-500" }, + { .compatible = "qcom,sc7280-smmu-500" }, { .compatible = "qcom,smmu-500", }, { /* sentinel */ } };
On Wed, 21 Aug 2024 at 07:42, Caleb Connolly caleb.connolly@linaro.org wrote:
This soc doesn't have the generic compatible.
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
drivers/iommu/qcom-hyp-smmu.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/drivers/iommu/qcom-hyp-smmu.c b/drivers/iommu/qcom-hyp-smmu.c index 7b646d840dd4..1b5a09bb7b39 100644 --- a/drivers/iommu/qcom-hyp-smmu.c +++ b/drivers/iommu/qcom-hyp-smmu.c @@ -380,8 +380,9 @@ static struct iommu_ops qcom_smmu_ops = { };
static const struct udevice_id qcom_smmu500_ids[] = { { .compatible = "qcom,sdm845-smmu-500" },
{ .compatible = "qcom,sc7280-smmu-500" }, { .compatible = "qcom,smmu-500", }, { /* sentinel */ }};
-- 2.46.0
Enable clocks on SC7280
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- configs/qcom_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig index 24b71ba7be29..1a079264a554 100644 --- a/configs/qcom_defconfig +++ b/configs/qcom_defconfig @@ -44,8 +44,9 @@ CONFIG_CLK=y CONFIG_CLK_QCOM_APQ8016=y CONFIG_CLK_QCOM_APQ8096=y CONFIG_CLK_QCOM_QCM2290=y CONFIG_CLK_QCOM_QCS404=y +CONFIG_CLK_QCOM_SC7280=y CONFIG_CLK_QCOM_SDM845=y CONFIG_CLK_QCOM_SM6115=y CONFIG_CLK_QCOM_SM8250=y CONFIG_CLK_QCOM_SM8550=y
On Wed, 21 Aug 2024 at 07:42, Caleb Connolly caleb.connolly@linaro.org wrote:
Enable clocks on SC7280
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
configs/qcom_defconfig | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig index 24b71ba7be29..1a079264a554 100644 --- a/configs/qcom_defconfig +++ b/configs/qcom_defconfig @@ -44,8 +44,9 @@ CONFIG_CLK=y CONFIG_CLK_QCOM_APQ8016=y CONFIG_CLK_QCOM_APQ8096=y CONFIG_CLK_QCOM_QCM2290=y CONFIG_CLK_QCOM_QCS404=y +CONFIG_CLK_QCOM_SC7280=y CONFIG_CLK_QCOM_SDM845=y CONFIG_CLK_QCOM_SM6115=y CONFIG_CLK_QCOM_SM8250=y CONFIG_CLK_QCOM_SM8550=y
-- 2.46.0
Introduce a defconfig for the RB3 Gen 2 and other QCM6490 boards with a dedicated uefi partition. These can replace EDK2 entirely with U-Boot.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- MAINTAINERS | 1 + configs/qcm6490_defconfig | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS index daaf0345d0e8..7ab39d91a553 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -616,8 +616,9 @@ M: Neil Armstrong neil.armstrong@linaro.org R: Sumit Garg sumit.garg@linaro.org L: u-boot-qcom@groups.io S: Maintained T: git https://source.denx.de/u-boot/custodians/u-boot-snapdragon.git +F: configs/qcm6490_defconfig F: drivers/*/*/pm8???-* F: drivers/gpio/msm_gpio.c F: drivers/mmc/msm_sdhci.c F: drivers/phy/msm8916-usbh-phy.c diff --git a/configs/qcm6490_defconfig b/configs/qcm6490_defconfig new file mode 100644 index 000000000000..25413ac9ed5a --- /dev/null +++ b/configs/qcm6490_defconfig @@ -0,0 +1,18 @@ +# Configuration for building U-Boot to be flashed +# to the uefi partition of QCM6490 dev boards with +# the "Linux Embedded" partition layout (which have +# a dedicated "uefi" partition for edk2/U-Boot) + +#include "qcom_defconfig" + +CONFIG_DEBUG_UART=y +CONFIG_DEBUG_UART_ANNOUNCE=y +CONFIG_DEBUG_UART_BASE=0x994000 +CONFIG_DEBUG_UART_MSM_GENI=y +CONFIG_DEBUG_UART_CLOCK=14745600 + +# Address where U-Boot will be loaded +CONFIG_TEXT_BASE=0x9fc00000 +CONFIG_REMAKE_ELF=y + +CONFIG_DEFAULT_DEVICE_TREE="qcom/qcs6490-rb3gen2"
On Wed, 21 Aug 2024 at 07:43, Caleb Connolly caleb.connolly@linaro.org wrote:
Introduce a defconfig for the RB3 Gen 2 and other QCM6490 boards with a dedicated uefi partition. These can replace EDK2 entirely with U-Boot.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
MAINTAINERS | 1 + configs/qcm6490_defconfig | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/MAINTAINERS b/MAINTAINERS index daaf0345d0e8..7ab39d91a553 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -616,8 +616,9 @@ M: Neil Armstrong neil.armstrong@linaro.org R: Sumit Garg sumit.garg@linaro.org L: u-boot-qcom@groups.io S: Maintained T: git https://source.denx.de/u-boot/custodians/u-boot-snapdragon.git +F: configs/qcm6490_defconfig F: drivers/*/*/pm8???-* F: drivers/gpio/msm_gpio.c F: drivers/mmc/msm_sdhci.c F: drivers/phy/msm8916-usbh-phy.c diff --git a/configs/qcm6490_defconfig b/configs/qcm6490_defconfig new file mode 100644 index 000000000000..25413ac9ed5a --- /dev/null +++ b/configs/qcm6490_defconfig @@ -0,0 +1,18 @@ +# Configuration for building U-Boot to be flashed +# to the uefi partition of QCM6490 dev boards with +# the "Linux Embedded" partition layout (which have +# a dedicated "uefi" partition for edk2/U-Boot)
+#include "qcom_defconfig"
+CONFIG_DEBUG_UART=y +CONFIG_DEBUG_UART_ANNOUNCE=y +CONFIG_DEBUG_UART_BASE=0x994000 +CONFIG_DEBUG_UART_MSM_GENI=y +CONFIG_DEBUG_UART_CLOCK=14745600
+# Address where U-Boot will be loaded +CONFIG_TEXT_BASE=0x9fc00000 +CONFIG_REMAKE_ELF=y
+CONFIG_DEFAULT_DEVICE_TREE="qcom/qcs6490-rb3gen2"
-- 2.46.0
The process here is almost identical to the Dragonboard 410c, we've come full circle!
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- doc/board/qualcomm/index.rst | 1 + doc/board/qualcomm/rb3gen2.rst | 53 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)
diff --git a/doc/board/qualcomm/index.rst b/doc/board/qualcomm/index.rst index 4955274a39bc..8c7969987a97 100644 --- a/doc/board/qualcomm/index.rst +++ b/doc/board/qualcomm/index.rst @@ -6,6 +6,7 @@ Qualcomm .. toctree:: :maxdepth: 2
dragonboard410c + rb3gen2 board debugging diff --git a/doc/board/qualcomm/rb3gen2.rst b/doc/board/qualcomm/rb3gen2.rst new file mode 100644 index 000000000000..4240606224f5 --- /dev/null +++ b/doc/board/qualcomm/rb3gen2.rst @@ -0,0 +1,53 @@ +.. SPDX-License-Identifier: GPL-2.0+ +.. sectionauthor:: Caleb Connolly caleb.connolly@linaro.org + +Qualcomm Robotics RB3 Gen 2 +=========================== + +The RB3 Gen 2 is a development board based on the Qualcomm QCM6490 SoC (a derivative +of SC7280). More information can be found on `Qualcomm's product page`_. + +U-Boot can be used as a replacement for Qualcomm's original EDK2 bootloader by +flashing it directly to the uefi_a (or _b) partition. + +.. _Qualcomm's product page: https://www.qualcomm.com/developer/hardware/rb3-gen-2-development-kit + +Installation +------------ +First, setup ``CROSS_COMPILE`` for aarch64. Then, build U-Boot for ``qcm6490``:: + + $ export CROSS_COMPILE=<aarch64 toolchain prefix> + $ make qcm6490_defconfig + $ make -j8 + +This will build ``u-boot.elf`` in the configured output directory. + +Although the RB3 Gen 2 does not have secure boot set up by default, +the firmware still expects firmware ELF images to be "signed". The signature +does not provide any security in this case, but it provides the firmware with +some required metadata. + +To "sign" ``u-boot.elf`` you can use e.g. `qtestsign`_:: + + $ qtestsign -v6 aboot -o u-boot.mbn u-boot.elf + +Then install the resulting ``u-boot.mbn`` to the ``uefi_a`` partition +on your device with ``fastboot flash uefi_a u-boot.mbn``. + +U-Boot should be running after a reboot (``fastboot reboot``). + +Note that fastboot is not yet supported in U-Boot on this board, as a result, +to flash back the original firmware, or new versoins of the U-Boot, EDL mode +must be used. This can be accessed by pressing the EDL mode button as described +in the Qualcomm Linux documentation. A tool like bkerler's `edl`_ can be used +for flashing with the firehose loader binary appropriate for the board. + +.. _qtestsign: https://github.com/msm8916-mainline/qtestsign +.. _edl: https://github.com/bkerler/edl + +Usage +----- + +The USB Type-A ports are connected via a PCIe USB hub, which is not supported yet. +However, the Type-C port can be used with a powered USB dock to connect peripherals +like a USB stick.
On Wed, 21 Aug 2024 at 07:43, Caleb Connolly caleb.connolly@linaro.org wrote:
The process here is almost identical to the Dragonboard 410c, we've come full circle!
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
doc/board/qualcomm/index.rst | 1 + doc/board/qualcomm/rb3gen2.rst | 53 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
participants (3)
-
Caleb Connolly -
Neil Armstrong -
Simon Glass