[PATCH v3 0/4]

[PATCH v3 0/4]

[Volodymyr Babchuk]

Set of pre-req patches for Qualcomm SA8155P-ADP board support.

This path series consist of generic qcom changes that may benefit
different boards. It is the part of the bigger series that adds
SA8155P-ADP support, but I am posting this limited set because there
are other developers who depend on those changes and I am not ready to
post other patches of the bigger series.


Changes in v3:
 - Replaced fdt_valid() with fdt_check_header()
 - Added "depends POWER_DOMAIN" to Kconfig (see note)
 - Use readl_poll_timeout() instead of open coded wait loop
 - Print warning if power domain can't be enabled/disabled

Changes in v2:
 - New patch in v2
 - Reworked qcom_cc_bind() function
 - Added timeout to qcom_power_set()
 - Minor fixes in register names and formatting

Volodymyr Babchuk (4):
  qcom: board: validate fdt before trying to use it
  clk: qcom: clear div mask before assigning a new divider
  clk: qcom: add support for power domains uclass
  pinctrl: qcom: pass pin number to get_function_mux callback

 arch/arm/mach-snapdragon/board.c       |   5 +-
 drivers/clk/qcom/Kconfig               |   2 +-
 drivers/clk/qcom/clock-qcom.c          | 135 ++++++++++++++++++++++---
 drivers/clk/qcom/clock-qcom.h          |   6 ++
 drivers/pinctrl/qcom/pinctrl-apq8016.c |   3 +-
 drivers/pinctrl/qcom/pinctrl-apq8096.c |   3 +-
 drivers/pinctrl/qcom/pinctrl-ipq4019.c |   3 +-
 drivers/pinctrl/qcom/pinctrl-qcom.c    |   4 +-
 drivers/pinctrl/qcom/pinctrl-qcom.h    |   3 +-
 drivers/pinctrl/qcom/pinctrl-qcs404.c  |   3 +-
 drivers/pinctrl/qcom/pinctrl-sdm845.c  |   3 +-
 11 files changed, 146 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] qcom: board: validate fdt before trying to use it

[Volodymyr Babchuk]

There are cases when previous bootloader stage leaves some seemingly
valid value in r0, which in fact does not point to valid FDT
blob. This behavior was encountered when trying to boot U-Boot as
"hyp" loader on SA8155P-ADP.

To be sure that we really got the pointer to a device tree we need to
validate it with fdt_check_header() function.

Note: This approach is not 100% fool-proof, as get_prev_bl_fdt_addr()
theoretically can return a pointer to a region that is not physically
mapped and we will get data abort exception when fdt_check_header()
will try to access it. But at this early boot stage we don't know
where RAM is anyways so there is little we can do.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

---

Changes in v3:
 - Replaced fdt_valid() with fdt_check_header()
 - Added Sumit's R-B tag

Changes in v2:
 - New patch in v2

 arch/arm/mach-snapdragon/board.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c
index f12f5791a1..6f762fc948 100644
--- a/arch/arm/mach-snapdragon/board.c
+++ b/arch/arm/mach-snapdragon/board.c
@@ -24,6 +24,7 @@
 #include <linux/sizes.h>
 #include <lmb.h>
 #include <malloc.h>
+#include <fdt_support.h>
 #include <usb.h>
 #include <sort.h>
 
@@ -93,7 +94,9 @@ void *board_fdt_blob_setup(int *err)
 	 * try and use the FDT built into U-Boot if there is one...
 	 * This avoids having a hard dependency on the previous stage bootloader
 	 */
-	if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K))) {
+
+	if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K) ||
+					       fdt_check_header((void *)fdt))) {
 		debug("%s: Using built in FDT, bootloader gave us %#llx\n", __func__, fdt);
 		return (void *)gd->fdt_blob;
 	}
-- 
2.43.0

[PATCH v3 2/4] clk: qcom: clear div mask before assigning a new divider

[Volodymyr Babchuk]

The current behaviour does a bitwise OR of the previous and new
divider values, this is wrong as some bits maybe be set already. We
need to clear all the divider bits before applying new ones.

This fixes potential issue with 1Gbit ethernet on SA8155P-ADP boards.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>

---

(no changes since v2)

Changes in v2:
 - Reworded the commit message
 - Added Caleb's R-b tag

 drivers/clk/qcom/clock-qcom.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
index 7c683e5192..729d190c54 100644
--- a/drivers/clk/qcom/clock-qcom.c
+++ b/drivers/clk/qcom/clock-qcom.c
@@ -117,7 +117,8 @@ void clk_rcg_set_rate_mnd(phys_addr_t base, const struct bcr_regs *regs,
 
 	/* setup src select and divider */
 	cfg  = readl(base + regs->cfg_rcgr);
-	cfg &= ~(CFG_SRC_SEL_MASK | CFG_MODE_MASK | CFG_HW_CLK_CTRL_MASK);
+	cfg &= ~(CFG_SRC_SEL_MASK | CFG_MODE_MASK | CFG_HW_CLK_CTRL_MASK |
+		 CFG_SRC_DIV_MASK);
 	cfg |= source & CFG_SRC_SEL_MASK; /* Select clock source */
 
 	if (div)
-- 
2.43.0

[PATCH v3 3/4] clk: qcom: add support for power domains uclass

[Volodymyr Babchuk]

Now sub-drivers for particular SoCs can register them as power domain
drivers. This is needed for upcoming SM8150 support, because it needs
to power up the Ethernet module.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---
Caleb suggested to use "imply POWER_DOMAIN", not "depends
POWER_DOMAIN" in the Kconfig, but this does not work:
$ make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm
scripts/kconfig/conf  --syncconfig Kconfig
drivers/clk/Kconfig:3:error: recursive dependency detected!
drivers/clk/Kconfig:3:  symbol CLK is selected by IMX8M_POWER_DOMAIN
drivers/power/domain/Kconfig:35:        symbol IMX8M_POWER_DOMAIN depends on POWER_DOMAIN
drivers/power/domain/Kconfig:3: symbol POWER_DOMAIN is implied by CLK_QCOM
drivers/clk/qcom/Kconfig:3:     symbol CLK_QCOM depends on CLK
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"


Changes in v3:
 - Added "depends POWER_DOMAIN" to Kconfig (see note)
 - Use readl_poll_timeout() instead of open coded wait loop
 - Print warning if power domain can't be enabled/disabled

Changes in v2:
 - Reworked qcom_cc_bind() function
 - Added timeout to qcom_power_set()
 - Minor fixes in register names and formatting

 drivers/clk/qcom/Kconfig      |   2 +-
 drivers/clk/qcom/clock-qcom.c | 132 ++++++++++++++++++++++++++++++----
 drivers/clk/qcom/clock-qcom.h |   6 ++
 3 files changed, 126 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 0df0d1881a..8dae635ac2 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -2,7 +2,7 @@ if ARCH_SNAPDRAGON || ARCH_IPQ40XX
 
 config CLK_QCOM
 	bool
-	depends on CLK && DM_RESET
+	depends on CLK && DM_RESET && POWER_DOMAIN
 	def_bool n
 
 menu "Qualcomm clock drivers"
diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
index 729d190c54..7a5938a06a 100644
--- a/drivers/clk/qcom/clock-qcom.c
+++ b/drivers/clk/qcom/clock-qcom.c
@@ -22,7 +22,9 @@
 #include <linux/bug.h>
 #include <linux/delay.h>
 #include <linux/bitops.h>
+#include <linux/iopoll.h>
 #include <reset-uclass.h>
+#include <power-domain-uclass.h>
 
 #include "clock-qcom.h"
 
@@ -30,6 +32,13 @@
 #define CBCR_BRANCH_ENABLE_BIT  BIT(0)
 #define CBCR_BRANCH_OFF_BIT     BIT(31)
 
+#define GDSC_SW_COLLAPSE_MASK		BIT(0)
+#define GDSC_POWER_DOWN_COMPLETE	BIT(15)
+#define GDSC_POWER_UP_COMPLETE		BIT(16)
+#define GDSC_PWR_ON_MASK		BIT(31)
+#define CFG_GDSCR_OFFSET		0x4
+#define GDSC_STATUS_POLL_TIMEOUT_US	1500
+
 /* Enable clock controlled by CBC soft macro */
 void clk_enable_cbc(phys_addr_t cbcr)
 {
@@ -223,7 +232,7 @@ U_BOOT_DRIVER(qcom_clk) = {
 int qcom_cc_bind(struct udevice *parent)
 {
 	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(parent);
-	struct udevice *clkdev, *rstdev;
+	struct udevice *clkdev = NULL, *rstdev = NULL, *pwrdev;
 	struct driver *drv;
 	int ret;
 
@@ -238,20 +247,41 @@ int qcom_cc_bind(struct udevice *parent)
 	if (ret)
 		return ret;
 
-	/* Bail out early if resets are not specified for this platform */
-	if (!data->resets)
-		return ret;
+	if (data->resets) {
+		/* Get a handle to the common reset handler */
+		drv = lists_driver_lookup_name("qcom_reset");
+		if (!drv) {
+			ret = -ENOENT;
+			goto unbind_clkdev;
+		}
+
+		/* Register the reset controller */
+		ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
+						   dev_ofnode(parent), &rstdev);
+		if (ret)
+			goto unbind_clkdev;
+	}
 
-	/* Get a handle to the common reset handler */
-	drv = lists_driver_lookup_name("qcom_reset");
-	if (!drv)
-		return -ENOENT;
+	if (data->power_domains) {
+		/* Get a handle to the common power domain handler */
+		drv = lists_driver_lookup_name("qcom_power");
+		if (!drv) {
+			ret = -ENOENT;
+			goto unbind_rstdev;
+		}
+		/* Register the power domain controller */
+		ret = device_bind_with_driver_data(parent, drv, "qcom_power", (ulong)data,
+						   dev_ofnode(parent), &pwrdev);
+		if (ret)
+			goto unbind_rstdev;
+	}
 
-	/* Register the reset controller */
-	ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
-					   dev_ofnode(parent), &rstdev);
-	if (ret)
-		device_unbind(clkdev);
+	return 0;
+
+unbind_rstdev:
+	device_unbind(rstdev);
+unbind_clkdev:
+	device_unbind(clkdev);
 
 	return ret;
 }
@@ -306,3 +336,79 @@ U_BOOT_DRIVER(qcom_reset) = {
 	.ops = &qcom_reset_ops,
 	.probe = qcom_reset_probe,
 };
+
+static int qcom_power_set(struct power_domain *pwr, bool on)
+{
+	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(pwr->dev);
+	void __iomem *base = dev_get_priv(pwr->dev);
+	const struct qcom_power_map *map;
+	u32 value;
+	int ret;
+
+	if (pwr->id >= data->num_power_domains)
+		return -ENODEV;
+
+	map = &data->power_domains[pwr->id];
+
+	if (!map->reg)
+		return -ENODEV;
+
+	value = readl(base + map->reg);
+
+	if (on)
+		value &= ~GDSC_SW_COLLAPSE_MASK;
+	else
+		value |= GDSC_SW_COLLAPSE_MASK;
+
+	writel(value, base + map->reg);
+
+	if (on)
+		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
+					 value,
+					 (value & GDSC_POWER_UP_COMPLETE) ||
+					 (value & GDSC_PWR_ON_MASK),
+					 GDSC_STATUS_POLL_TIMEOUT_US);
+
+	else
+		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
+					 value,
+					 (value & GDSC_POWER_DOWN_COMPLETE) ||
+					 !(value & GDSC_PWR_ON_MASK),
+					 GDSC_STATUS_POLL_TIMEOUT_US);
+
+
+	if (ret == -ETIMEDOUT)
+		printf("WARNING: GDSC %lu is stuck during power on/off\n",
+		       pwr->id);
+	return ret;
+}
+
+static int qcom_power_on(struct power_domain *pwr)
+{
+	return qcom_power_set(pwr, true);
+}
+
+static int qcom_power_off(struct power_domain *pwr)
+{
+	return qcom_power_set(pwr, false);
+}
+
+static const struct power_domain_ops qcom_power_ops = {
+	.on = qcom_power_on,
+	.off = qcom_power_off,
+};
+
+static int qcom_power_probe(struct udevice *dev)
+{
+	/* Set our priv pointer to the base address */
+	dev_set_priv(dev, (void *)dev_read_addr(dev));
+
+	return 0;
+}
+
+U_BOOT_DRIVER(qcom_power) = {
+	.name = "qcom_power",
+	.id = UCLASS_POWER_DOMAIN,
+	.ops = &qcom_power_ops,
+	.probe = qcom_power_probe,
+};
diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h
index 01088c1901..12a1eaec2b 100644
--- a/drivers/clk/qcom/clock-qcom.h
+++ b/drivers/clk/qcom/clock-qcom.h
@@ -59,9 +59,15 @@ struct qcom_reset_map {
 	u8 bit;
 };
 
+struct qcom_power_map {
+	unsigned int reg;
+};
+
 struct clk;
 
 struct msm_clk_data {
+	const struct qcom_power_map	*power_domains;
+	unsigned long			num_power_domains;
 	const struct qcom_reset_map	*resets;
 	unsigned long			num_resets;
 	const struct gate_clk		*clks;
-- 
2.43.0

[PATCH v3 4/4] pinctrl: qcom: pass pin number to get_function_mux callback

[Volodymyr Babchuk]

This patch is the preparation for SM8150 support. This new SoC
depending on the particular pin can have different numbers for the
same function. For example "rgmii" function for GPIO4 has id=2 while
for GPIO59 it has id=1. So, to support this type of SoCs,
get_function_mux() callback needs to know for which pin the function
is requested.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

---

Changes in v3:
 - Added Sumit's R-b tag

Changes in v2:
 - Added Caleb's R-b tag

 drivers/pinctrl/qcom/pinctrl-apq8016.c | 3 ++-
 drivers/pinctrl/qcom/pinctrl-apq8096.c | 3 ++-
 drivers/pinctrl/qcom/pinctrl-ipq4019.c | 3 ++-
 drivers/pinctrl/qcom/pinctrl-qcom.c    | 4 ++--
 drivers/pinctrl/qcom/pinctrl-qcom.h    | 3 ++-
 drivers/pinctrl/qcom/pinctrl-qcs404.c  | 3 ++-
 drivers/pinctrl/qcom/pinctrl-sdm845.c  | 3 ++-
 7 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-apq8016.c b/drivers/pinctrl/qcom/pinctrl-apq8016.c
index db0e212468..a9a00f4b08 100644
--- a/drivers/pinctrl/qcom/pinctrl-apq8016.c
+++ b/drivers/pinctrl/qcom/pinctrl-apq8016.c
@@ -49,7 +49,8 @@ static const char *apq8016_get_pin_name(struct udevice *dev,
 	}
 }
 
-static unsigned int apq8016_get_function_mux(unsigned int selector)
+static unsigned int apq8016_get_function_mux(__maybe_unused unsigned int pin,
+					     unsigned int selector)
 {
 	return msm_pinctrl_functions[selector].val;
 }
diff --git a/drivers/pinctrl/qcom/pinctrl-apq8096.c b/drivers/pinctrl/qcom/pinctrl-apq8096.c
index 880df8fe3c..9697cb5beb 100644
--- a/drivers/pinctrl/qcom/pinctrl-apq8096.c
+++ b/drivers/pinctrl/qcom/pinctrl-apq8096.c
@@ -44,7 +44,8 @@ static const char *apq8096_get_pin_name(struct udevice *dev,
 	}
 }
 
-static unsigned int apq8096_get_function_mux(unsigned int selector)
+static unsigned int apq8096_get_function_mux(__maybe_unused unsigned int pin,
+					     unsigned int selector)
 {
 	return msm_pinctrl_functions[selector].val;
 }
diff --git a/drivers/pinctrl/qcom/pinctrl-ipq4019.c b/drivers/pinctrl/qcom/pinctrl-ipq4019.c
index 74c04ab87c..4479230313 100644
--- a/drivers/pinctrl/qcom/pinctrl-ipq4019.c
+++ b/drivers/pinctrl/qcom/pinctrl-ipq4019.c
@@ -40,7 +40,8 @@ static const char *ipq4019_get_pin_name(struct udevice *dev,
 	return pin_name;
 }
 
-static unsigned int ipq4019_get_function_mux(unsigned int selector)
+static unsigned int ipq4019_get_function_mux(__maybe_unused unsigned int pin,
+					     unsigned int selector)
 {
 	return msm_pinctrl_functions[selector].val;
 }
diff --git a/drivers/pinctrl/qcom/pinctrl-qcom.c b/drivers/pinctrl/qcom/pinctrl-qcom.c
index ee0624df29..909e566acf 100644
--- a/drivers/pinctrl/qcom/pinctrl-qcom.c
+++ b/drivers/pinctrl/qcom/pinctrl-qcom.c
@@ -83,14 +83,14 @@ static int msm_pinmux_set(struct udevice *dev, unsigned int pin_selector,
 			  unsigned int func_selector)
 {
 	struct msm_pinctrl_priv *priv = dev_get_priv(dev);
+	u32 func = priv->data->get_function_mux(pin_selector, func_selector);
 
 	/* Always NOP for special pins, assume they're in the correct state */
 	if (qcom_is_special_pin(&priv->data->pin_data, pin_selector))
 		return 0;
 
 	clrsetbits_le32(priv->base + GPIO_CONFIG_REG(priv, pin_selector),
-			TLMM_FUNC_SEL_MASK | TLMM_GPIO_DISABLE,
-			priv->data->get_function_mux(func_selector) << 2);
+			TLMM_FUNC_SEL_MASK | TLMM_GPIO_DISABLE, func << 2);
 	return 0;
 }
 
diff --git a/drivers/pinctrl/qcom/pinctrl-qcom.h b/drivers/pinctrl/qcom/pinctrl-qcom.h
index 07f2eae9ba..49b7bfbc00 100644
--- a/drivers/pinctrl/qcom/pinctrl-qcom.h
+++ b/drivers/pinctrl/qcom/pinctrl-qcom.h
@@ -18,7 +18,8 @@ struct msm_pinctrl_data {
 	int functions_count;
 	const char *(*get_function_name)(struct udevice *dev,
 					 unsigned int selector);
-	unsigned int (*get_function_mux)(unsigned int selector);
+	unsigned int (*get_function_mux)(unsigned int pin,
+					 unsigned int selector);
 	const char *(*get_pin_name)(struct udevice *dev,
 				    unsigned int selector);
 };
diff --git a/drivers/pinctrl/qcom/pinctrl-qcs404.c b/drivers/pinctrl/qcom/pinctrl-qcs404.c
index 3a2d468599..4b7c670c90 100644
--- a/drivers/pinctrl/qcom/pinctrl-qcs404.c
+++ b/drivers/pinctrl/qcom/pinctrl-qcs404.c
@@ -94,7 +94,8 @@ static const char *qcs404_get_pin_name(struct udevice *dev,
 	}
 }
 
-static unsigned int qcs404_get_function_mux(unsigned int selector)
+static unsigned int qcs404_get_function_mux(__maybe_unused unsigned int pin,
+					    unsigned int selector)
 {
 	return msm_pinctrl_functions[selector].val;
 }
diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index 76bd8c4ef4..459a4329ec 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -70,7 +70,8 @@ static const char *sdm845_get_pin_name(struct udevice *dev,
 	return pin_name;
 }
 
-static unsigned int sdm845_get_function_mux(unsigned int selector)
+static unsigned int sdm845_get_function_mux(__maybe_unused unsigned int pin,
+					    unsigned int selector)
 {
 	return msm_pinctrl_functions[selector].val;
 }
-- 
2.43.0

Re: [PATCH v3 2/4] clk: qcom: clear div mask before assigning a new divider

[Sumit Garg]

On Tue, 12 Mar 2024 at 03:03, Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
> The current behaviour does a bitwise OR of the previous and new
> divider values, this is wrong as some bits maybe be set already. We

nit: s/maybe be/maybe/

> need to clear all the divider bits before applying new ones.
>
> This fixes potential issue with 1Gbit ethernet on SA8155P-ADP boards.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> ---
>
> (no changes since v2)
>
> Changes in v2:
>  - Reworded the commit message
>  - Added Caleb's R-b tag
>
>  drivers/clk/qcom/clock-qcom.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
> index 7c683e5192..729d190c54 100644
> --- a/drivers/clk/qcom/clock-qcom.c
> +++ b/drivers/clk/qcom/clock-qcom.c
> @@ -117,7 +117,8 @@ void clk_rcg_set_rate_mnd(phys_addr_t base, const struct bcr_regs *regs,
>
>         /* setup src select and divider */
>         cfg  = readl(base + regs->cfg_rcgr);
> -       cfg &= ~(CFG_SRC_SEL_MASK | CFG_MODE_MASK | CFG_HW_CLK_CTRL_MASK);
> +       cfg &= ~(CFG_SRC_SEL_MASK | CFG_MODE_MASK | CFG_HW_CLK_CTRL_MASK |
> +                CFG_SRC_DIV_MASK);
>         cfg |= source & CFG_SRC_SEL_MASK; /* Select clock source */
>
>         if (div)
> --
> 2.43.0

Re: [PATCH v3 3/4] clk: qcom: add support for power domains uclass

[Sumit Garg]

On Tue, 12 Mar 2024 at 03:03, Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
> Now sub-drivers for particular SoCs can register them as power domain
> drivers. This is needed for upcoming SM8150 support, because it needs
> to power up the Ethernet module.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> ---
> Caleb suggested to use "imply POWER_DOMAIN", not "depends
> POWER_DOMAIN" in the Kconfig, but this does not work:
> $ make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm
> scripts/kconfig/conf  --syncconfig Kconfig
> drivers/clk/Kconfig:3:error: recursive dependency detected!
> drivers/clk/Kconfig:3:  symbol CLK is selected by IMX8M_POWER_DOMAIN
> drivers/power/domain/Kconfig:35:        symbol IMX8M_POWER_DOMAIN depends on POWER_DOMAIN
> drivers/power/domain/Kconfig:3: symbol POWER_DOMAIN is implied by CLK_QCOM
> drivers/clk/qcom/Kconfig:3:     symbol CLK_QCOM depends on CLK
> For a resolution refer to Documentation/kbuild/kconfig-language.txt
> subsection "Kconfig recursive dependency limitations"
>
>
> Changes in v3:
>  - Added "depends POWER_DOMAIN" to Kconfig (see note)
>  - Use readl_poll_timeout() instead of open coded wait loop
>  - Print warning if power domain can't be enabled/disabled
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> Changes in v2:
>  - Reworked qcom_cc_bind() function
>  - Added timeout to qcom_power_set()
>  - Minor fixes in register names and formatting
>
>  drivers/clk/qcom/Kconfig      |   2 +-
>  drivers/clk/qcom/clock-qcom.c | 132 ++++++++++++++++++++++++++++++----
>  drivers/clk/qcom/clock-qcom.h |   6 ++
>  3 files changed, 126 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 0df0d1881a..8dae635ac2 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -2,7 +2,7 @@ if ARCH_SNAPDRAGON || ARCH_IPQ40XX
>
>  config CLK_QCOM
>         bool
> -       depends on CLK && DM_RESET
> +       depends on CLK && DM_RESET && POWER_DOMAIN
>         def_bool n
>
>  menu "Qualcomm clock drivers"
> diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
> index 729d190c54..7a5938a06a 100644
> --- a/drivers/clk/qcom/clock-qcom.c
> +++ b/drivers/clk/qcom/clock-qcom.c
> @@ -22,7 +22,9 @@
>  #include <linux/bug.h>
>  #include <linux/delay.h>
>  #include <linux/bitops.h>
> +#include <linux/iopoll.h>
>  #include <reset-uclass.h>
> +#include <power-domain-uclass.h>
>
>  #include "clock-qcom.h"
>
> @@ -30,6 +32,13 @@
>  #define CBCR_BRANCH_ENABLE_BIT  BIT(0)
>  #define CBCR_BRANCH_OFF_BIT     BIT(31)
>
> +#define GDSC_SW_COLLAPSE_MASK          BIT(0)
> +#define GDSC_POWER_DOWN_COMPLETE       BIT(15)
> +#define GDSC_POWER_UP_COMPLETE         BIT(16)
> +#define GDSC_PWR_ON_MASK               BIT(31)
> +#define CFG_GDSCR_OFFSET               0x4
> +#define GDSC_STATUS_POLL_TIMEOUT_US    1500
> +
>  /* Enable clock controlled by CBC soft macro */
>  void clk_enable_cbc(phys_addr_t cbcr)
>  {
> @@ -223,7 +232,7 @@ U_BOOT_DRIVER(qcom_clk) = {
>  int qcom_cc_bind(struct udevice *parent)
>  {
>         struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(parent);
> -       struct udevice *clkdev, *rstdev;
> +       struct udevice *clkdev = NULL, *rstdev = NULL, *pwrdev;
>         struct driver *drv;
>         int ret;
>
> @@ -238,20 +247,41 @@ int qcom_cc_bind(struct udevice *parent)
>         if (ret)
>                 return ret;
>
> -       /* Bail out early if resets are not specified for this platform */
> -       if (!data->resets)
> -               return ret;
> +       if (data->resets) {
> +               /* Get a handle to the common reset handler */
> +               drv = lists_driver_lookup_name("qcom_reset");
> +               if (!drv) {
> +                       ret = -ENOENT;
> +                       goto unbind_clkdev;
> +               }
> +
> +               /* Register the reset controller */
> +               ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> +                                                  dev_ofnode(parent), &rstdev);
> +               if (ret)
> +                       goto unbind_clkdev;
> +       }
>
> -       /* Get a handle to the common reset handler */
> -       drv = lists_driver_lookup_name("qcom_reset");
> -       if (!drv)
> -               return -ENOENT;
> +       if (data->power_domains) {
> +               /* Get a handle to the common power domain handler */
> +               drv = lists_driver_lookup_name("qcom_power");
> +               if (!drv) {
> +                       ret = -ENOENT;
> +                       goto unbind_rstdev;
> +               }
> +               /* Register the power domain controller */
> +               ret = device_bind_with_driver_data(parent, drv, "qcom_power", (ulong)data,
> +                                                  dev_ofnode(parent), &pwrdev);
> +               if (ret)
> +                       goto unbind_rstdev;
> +       }
>
> -       /* Register the reset controller */
> -       ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> -                                          dev_ofnode(parent), &rstdev);
> -       if (ret)
> -               device_unbind(clkdev);
> +       return 0;
> +
> +unbind_rstdev:
> +       device_unbind(rstdev);
> +unbind_clkdev:
> +       device_unbind(clkdev);
>
>         return ret;
>  }
> @@ -306,3 +336,79 @@ U_BOOT_DRIVER(qcom_reset) = {
>         .ops = &qcom_reset_ops,
>         .probe = qcom_reset_probe,
>  };
> +
> +static int qcom_power_set(struct power_domain *pwr, bool on)
> +{
> +       struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(pwr->dev);
> +       void __iomem *base = dev_get_priv(pwr->dev);
> +       const struct qcom_power_map *map;
> +       u32 value;
> +       int ret;
> +
> +       if (pwr->id >= data->num_power_domains)
> +               return -ENODEV;
> +
> +       map = &data->power_domains[pwr->id];
> +
> +       if (!map->reg)
> +               return -ENODEV;
> +
> +       value = readl(base + map->reg);
> +
> +       if (on)
> +               value &= ~GDSC_SW_COLLAPSE_MASK;
> +       else
> +               value |= GDSC_SW_COLLAPSE_MASK;
> +
> +       writel(value, base + map->reg);
> +
> +       if (on)
> +               ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +                                        value,
> +                                        (value & GDSC_POWER_UP_COMPLETE) ||
> +                                        (value & GDSC_PWR_ON_MASK),
> +                                        GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +       else
> +               ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +                                        value,
> +                                        (value & GDSC_POWER_DOWN_COMPLETE) ||
> +                                        !(value & GDSC_PWR_ON_MASK),
> +                                        GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +
> +       if (ret == -ETIMEDOUT)
> +               printf("WARNING: GDSC %lu is stuck during power on/off\n",
> +                      pwr->id);
> +       return ret;
> +}
> +
> +static int qcom_power_on(struct power_domain *pwr)
> +{
> +       return qcom_power_set(pwr, true);
> +}
> +
> +static int qcom_power_off(struct power_domain *pwr)
> +{
> +       return qcom_power_set(pwr, false);
> +}
> +
> +static const struct power_domain_ops qcom_power_ops = {
> +       .on = qcom_power_on,
> +       .off = qcom_power_off,
> +};
> +
> +static int qcom_power_probe(struct udevice *dev)
> +{
> +       /* Set our priv pointer to the base address */
> +       dev_set_priv(dev, (void *)dev_read_addr(dev));
> +
> +       return 0;
> +}
> +
> +U_BOOT_DRIVER(qcom_power) = {
> +       .name = "qcom_power",
> +       .id = UCLASS_POWER_DOMAIN,
> +       .ops = &qcom_power_ops,
> +       .probe = qcom_power_probe,
> +};
> diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h
> index 01088c1901..12a1eaec2b 100644
> --- a/drivers/clk/qcom/clock-qcom.h
> +++ b/drivers/clk/qcom/clock-qcom.h
> @@ -59,9 +59,15 @@ struct qcom_reset_map {
>         u8 bit;
>  };
>
> +struct qcom_power_map {
> +       unsigned int reg;
> +};
> +
>  struct clk;
>
>  struct msm_clk_data {
> +       const struct qcom_power_map     *power_domains;
> +       unsigned long                   num_power_domains;
>         const struct qcom_reset_map     *resets;
>         unsigned long                   num_resets;
>         const struct gate_clk           *clks;
> --
> 2.43.0

Re: [PATCH v3 3/4] clk: qcom: add support for power domains uclass

[Caleb Connolly]

On 11/03/2024 21:33, Volodymyr Babchuk wrote:
> Now sub-drivers for particular SoCs can register them as power domain
> drivers. This is needed for upcoming SM8150 support, because it needs
> to power up the Ethernet module.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> 
> ---
> Caleb suggested to use "imply POWER_DOMAIN", not "depends
> POWER_DOMAIN" in the Kconfig, but this does not work:
> $ make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm
> scripts/kconfig/conf  --syncconfig Kconfig
> drivers/clk/Kconfig:3:error: recursive dependency detected!
> drivers/clk/Kconfig:3:  symbol CLK is selected by IMX8M_POWER_DOMAIN
> drivers/power/domain/Kconfig:35:        symbol IMX8M_POWER_DOMAIN depends on POWER_DOMAIN
> drivers/power/domain/Kconfig:3: symbol POWER_DOMAIN is implied by CLK_QCOM
> drivers/clk/qcom/Kconfig:3:     symbol CLK_QCOM depends on CLK
> For a resolution refer to Documentation/kbuild/kconfig-language.txt
> subsection "Kconfig recursive dependency limitations"
> 
> 
> Changes in v3:
>  - Added "depends POWER_DOMAIN" to Kconfig (see note)
>  - Use readl_poll_timeout() instead of open coded wait loop
>  - Print warning if power domain can't be enabled/disabled
> 
> Changes in v2:
>  - Reworked qcom_cc_bind() function
>  - Added timeout to qcom_power_set()
>  - Minor fixes in register names and formatting
> 
>  drivers/clk/qcom/Kconfig      |   2 +-
>  drivers/clk/qcom/clock-qcom.c | 132 ++++++++++++++++++++++++++++++----
>  drivers/clk/qcom/clock-qcom.h |   6 ++
>  3 files changed, 126 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 0df0d1881a..8dae635ac2 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -2,7 +2,7 @@ if ARCH_SNAPDRAGON || ARCH_IPQ40XX
>  
>  config CLK_QCOM
>  	bool
> -	depends on CLK && DM_RESET
> +	depends on CLK && DM_RESET && POWER_DOMAIN
>  	def_bool n
>  
>  menu "Qualcomm clock drivers"
> diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
> index 729d190c54..7a5938a06a 100644
> --- a/drivers/clk/qcom/clock-qcom.c
> +++ b/drivers/clk/qcom/clock-qcom.c
> @@ -22,7 +22,9 @@
>  #include <linux/bug.h>
>  #include <linux/delay.h>
>  #include <linux/bitops.h>
> +#include <linux/iopoll.h>
>  #include <reset-uclass.h>
> +#include <power-domain-uclass.h>
>  
>  #include "clock-qcom.h"
>  
> @@ -30,6 +32,13 @@
>  #define CBCR_BRANCH_ENABLE_BIT  BIT(0)
>  #define CBCR_BRANCH_OFF_BIT     BIT(31)
>  
> +#define GDSC_SW_COLLAPSE_MASK		BIT(0)
> +#define GDSC_POWER_DOWN_COMPLETE	BIT(15)
> +#define GDSC_POWER_UP_COMPLETE		BIT(16)
> +#define GDSC_PWR_ON_MASK		BIT(31)
> +#define CFG_GDSCR_OFFSET		0x4
> +#define GDSC_STATUS_POLL_TIMEOUT_US	1500
> +
>  /* Enable clock controlled by CBC soft macro */
>  void clk_enable_cbc(phys_addr_t cbcr)
>  {
> @@ -223,7 +232,7 @@ U_BOOT_DRIVER(qcom_clk) = {
>  int qcom_cc_bind(struct udevice *parent)
>  {
>  	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(parent);
> -	struct udevice *clkdev, *rstdev;
> +	struct udevice *clkdev = NULL, *rstdev = NULL, *pwrdev;
>  	struct driver *drv;
>  	int ret;
>  
> @@ -238,20 +247,41 @@ int qcom_cc_bind(struct udevice *parent)
>  	if (ret)
>  		return ret;
>  
> -	/* Bail out early if resets are not specified for this platform */
> -	if (!data->resets)
> -		return ret;
> +	if (data->resets) {
> +		/* Get a handle to the common reset handler */
> +		drv = lists_driver_lookup_name("qcom_reset");
> +		if (!drv) {
> +			ret = -ENOENT;
> +			goto unbind_clkdev;
> +		}
> +
> +		/* Register the reset controller */
> +		ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> +						   dev_ofnode(parent), &rstdev);
> +		if (ret)
> +			goto unbind_clkdev;
> +	}
>  
> -	/* Get a handle to the common reset handler */
> -	drv = lists_driver_lookup_name("qcom_reset");
> -	if (!drv)
> -		return -ENOENT;
> +	if (data->power_domains) {
> +		/* Get a handle to the common power domain handler */
> +		drv = lists_driver_lookup_name("qcom_power");
> +		if (!drv) {
> +			ret = -ENOENT;
> +			goto unbind_rstdev;
> +		}
> +		/* Register the power domain controller */
> +		ret = device_bind_with_driver_data(parent, drv, "qcom_power", (ulong)data,
> +						   dev_ofnode(parent), &pwrdev);
> +		if (ret)
> +			goto unbind_rstdev;
> +	}
>  
> -	/* Register the reset controller */
> -	ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> -					   dev_ofnode(parent), &rstdev);
> -	if (ret)
> -		device_unbind(clkdev);
> +	return 0;
> +
> +unbind_rstdev:
> +	device_unbind(rstdev);
> +unbind_clkdev:
> +	device_unbind(clkdev);
>  
>  	return ret;
>  }
> @@ -306,3 +336,79 @@ U_BOOT_DRIVER(qcom_reset) = {
>  	.ops = &qcom_reset_ops,
>  	.probe = qcom_reset_probe,
>  };
> +
> +static int qcom_power_set(struct power_domain *pwr, bool on)
> +{
> +	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(pwr->dev);
> +	void __iomem *base = dev_get_priv(pwr->dev);
> +	const struct qcom_power_map *map;
> +	u32 value;
> +	int ret;
> +
> +	if (pwr->id >= data->num_power_domains)
> +		return -ENODEV;
> +
> +	map = &data->power_domains[pwr->id];
> +
> +	if (!map->reg)
> +		return -ENODEV;
> +
> +	value = readl(base + map->reg);
> +
> +	if (on)
> +		value &= ~GDSC_SW_COLLAPSE_MASK;
> +	else
> +		value |= GDSC_SW_COLLAPSE_MASK;
> +
> +	writel(value, base + map->reg);
> +
> +	if (on)
> +		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +					 value,
> +					 (value & GDSC_POWER_UP_COMPLETE) ||
> +					 (value & GDSC_PWR_ON_MASK),
> +					 GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +	else
> +		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +					 value,
> +					 (value & GDSC_POWER_DOWN_COMPLETE) ||
> +					 !(value & GDSC_PWR_ON_MASK),
> +					 GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +
> +	if (ret == -ETIMEDOUT)
> +		printf("WARNING: GDSC %lu is stuck during power on/off\n",
> +		       pwr->id);
> +	return ret;
> +}
> +
> +static int qcom_power_on(struct power_domain *pwr)
> +{
> +	return qcom_power_set(pwr, true);
> +}
> +
> +static int qcom_power_off(struct power_domain *pwr)
> +{
> +	return qcom_power_set(pwr, false);
> +}
> +
> +static const struct power_domain_ops qcom_power_ops = {
> +	.on = qcom_power_on,
> +	.off = qcom_power_off,
> +};
> +
> +static int qcom_power_probe(struct udevice *dev)
> +{
> +	/* Set our priv pointer to the base address */
> +	dev_set_priv(dev, (void *)dev_read_addr(dev));
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(qcom_power) = {
> +	.name = "qcom_power",
> +	.id = UCLASS_POWER_DOMAIN,
> +	.ops = &qcom_power_ops,
> +	.probe = qcom_power_probe,
> +};
> diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h
> index 01088c1901..12a1eaec2b 100644
> --- a/drivers/clk/qcom/clock-qcom.h
> +++ b/drivers/clk/qcom/clock-qcom.h
> @@ -59,9 +59,15 @@ struct qcom_reset_map {
>  	u8 bit;
>  };
>  
> +struct qcom_power_map {
> +	unsigned int reg;
> +};
> +
>  struct clk;
>  
>  struct msm_clk_data {
> +	const struct qcom_power_map	*power_domains;
> +	unsigned long			num_power_domains;
>  	const struct qcom_reset_map	*resets;
>  	unsigned long			num_resets;
>  	const struct gate_clk		*clks;

-- 
// Caleb (they/them)

Re: [PATCH v3 1/4] qcom: board: validate fdt before trying to use it

[Caleb Connolly]

On 11/03/2024 21:33, Volodymyr Babchuk wrote:
> There are cases when previous bootloader stage leaves some seemingly
> valid value in r0, which in fact does not point to valid FDT
> blob. This behavior was encountered when trying to boot U-Boot as
> "hyp" loader on SA8155P-ADP.
> 
> To be sure that we really got the pointer to a device tree we need to
> validate it with fdt_check_header() function.
> 
> Note: This approach is not 100% fool-proof, as get_prev_bl_fdt_addr()
> theoretically can return a pointer to a region that is not physically
> mapped and we will get data abort exception when fdt_check_header()
> will try to access it. But at this early boot stage we don't know
> where RAM is anyways so there is little we can do.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> 
> ---
> 
> Changes in v3:
>  - Replaced fdt_valid() with fdt_check_header()
>  - Added Sumit's R-B tag
> 
> Changes in v2:
>  - New patch in v2
> 
>  arch/arm/mach-snapdragon/board.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c
> index f12f5791a1..6f762fc948 100644
> --- a/arch/arm/mach-snapdragon/board.c
> +++ b/arch/arm/mach-snapdragon/board.c
> @@ -24,6 +24,7 @@
>  #include <linux/sizes.h>
>  #include <lmb.h>
>  #include <malloc.h>
> +#include <fdt_support.h>
>  #include <usb.h>
>  #include <sort.h>
>  
> @@ -93,7 +94,9 @@ void *board_fdt_blob_setup(int *err)
>  	 * try and use the FDT built into U-Boot if there is one...
>  	 * This avoids having a hard dependency on the previous stage bootloader
>  	 */
> -	if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K))) {
> +
> +	if (IS_ENABLED(CONFIG_OF_SEPARATE) && (!fdt || fdt != ALIGN(fdt, SZ_4K) ||
> +					       fdt_check_header((void *)fdt))) {
>  		debug("%s: Using built in FDT, bootloader gave us %#llx\n", __func__, fdt);
>  		return (void *)gd->fdt_blob;
>  	}

-- 
// Caleb (they/them)

Re: [PATCH v3 2/4] clk: qcom: clear div mask before assigning a new divider

[Volodymyr Babchuk]

Hi Sumit,

Thank you for the review.

Sumit Garg <sumit.garg@linaro.org> writes:

> On Tue, 12 Mar 2024 at 03:03, Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>> The current behaviour does a bitwise OR of the previous and new
>> divider values, this is wrong as some bits maybe be set already. We
>
> nit: s/maybe be/maybe/
>

Oops, right. Can this be applied by the committer during the merge
process? I know that committers in other open source project do this
sometimes. Or should I post a new version?

[...]

-- 
WBR, Volodymyr

Re: [PATCH v3 3/4] clk: qcom: add support for power domains uclass

[Caleb Connolly]

Hi Volodymyr,

On 11/03/2024 21:33, Volodymyr Babchuk wrote:
> Now sub-drivers for particular SoCs can register them as power domain
> drivers. This is needed for upcoming SM8150 support, because it needs
> to power up the Ethernet module.
> 
Thanks again for working on this.

I've been trying to rework my SDM845 USB support patches based on this,
and I've run into quite a few issues with CONFIG_POWER_DOMAIN.

Basically, it boils down to the RPM(h)PD, with no driver a bunch of
stuff breaks (including UART). I tried writing a no-op PD driver for it
but this didn't seem to work super well, and wouldn't scale (every soc
has it's own rpm(h)pd compatible).

In the end, I think supporting CONFIG_POWER_DOMAIN is the right approach
here. I have been able to get things working by leveraging OF_LIVE and
modifying the livetree during boot, similarly to how I plan to do for
USB. See [1].

Even with this, all the drivers we probe pre-relocation (like UART) need
the DM_FLAG_DEFAULT_PD_CTRL_OFF flag if they reference the RPM(h)PD, as
the of_fixup stuff doesn't happen until after relocation. I have a patch
which fixes that for sdm845 here [2].

I'll test this on the other boards and then re-send my USB patches
(including the two below) rebased on your series.

I'm a little worried this might come back to bite us later, but I think
by then it'll be worth trying to find a way to have U-Boot handle these
"safe to ignore" devices directly.

[1]:
https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commit/8af7731445721adab2206839d6df2d0f4f5f32d9
[2]:
https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commit/33b094103fb7e856cff6c08345a16ef8231ffaea
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> Caleb suggested to use "imply POWER_DOMAIN", not "depends
> POWER_DOMAIN" in the Kconfig, but this does not work:
> $ make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm
> scripts/kconfig/conf  --syncconfig Kconfig
> drivers/clk/Kconfig:3:error: recursive dependency detected!
> drivers/clk/Kconfig:3:  symbol CLK is selected by IMX8M_POWER_DOMAIN
> drivers/power/domain/Kconfig:35:        symbol IMX8M_POWER_DOMAIN depends on POWER_DOMAIN
> drivers/power/domain/Kconfig:3: symbol POWER_DOMAIN is implied by CLK_QCOM
> drivers/clk/qcom/Kconfig:3:     symbol CLK_QCOM depends on CLK
> For a resolution refer to Documentation/kbuild/kconfig-language.txt
> subsection "Kconfig recursive dependency limitations"
> 
> 
> Changes in v3:
>  - Added "depends POWER_DOMAIN" to Kconfig (see note)
>  - Use readl_poll_timeout() instead of open coded wait loop
>  - Print warning if power domain can't be enabled/disabled
> 
> Changes in v2:
>  - Reworked qcom_cc_bind() function
>  - Added timeout to qcom_power_set()
>  - Minor fixes in register names and formatting
> 
>  drivers/clk/qcom/Kconfig      |   2 +-
>  drivers/clk/qcom/clock-qcom.c | 132 ++++++++++++++++++++++++++++++----
>  drivers/clk/qcom/clock-qcom.h |   6 ++
>  3 files changed, 126 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 0df0d1881a..8dae635ac2 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -2,7 +2,7 @@ if ARCH_SNAPDRAGON || ARCH_IPQ40XX
>  
>  config CLK_QCOM
>  	bool
> -	depends on CLK && DM_RESET
> +	depends on CLK && DM_RESET && POWER_DOMAIN
>  	def_bool n
>  
>  menu "Qualcomm clock drivers"
> diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
> index 729d190c54..7a5938a06a 100644
> --- a/drivers/clk/qcom/clock-qcom.c
> +++ b/drivers/clk/qcom/clock-qcom.c
> @@ -22,7 +22,9 @@
>  #include <linux/bug.h>
>  #include <linux/delay.h>
>  #include <linux/bitops.h>
> +#include <linux/iopoll.h>
>  #include <reset-uclass.h>
> +#include <power-domain-uclass.h>
>  
>  #include "clock-qcom.h"
>  
> @@ -30,6 +32,13 @@
>  #define CBCR_BRANCH_ENABLE_BIT  BIT(0)
>  #define CBCR_BRANCH_OFF_BIT     BIT(31)
>  
> +#define GDSC_SW_COLLAPSE_MASK		BIT(0)
> +#define GDSC_POWER_DOWN_COMPLETE	BIT(15)
> +#define GDSC_POWER_UP_COMPLETE		BIT(16)
> +#define GDSC_PWR_ON_MASK		BIT(31)
> +#define CFG_GDSCR_OFFSET		0x4
> +#define GDSC_STATUS_POLL_TIMEOUT_US	1500
> +
>  /* Enable clock controlled by CBC soft macro */
>  void clk_enable_cbc(phys_addr_t cbcr)
>  {
> @@ -223,7 +232,7 @@ U_BOOT_DRIVER(qcom_clk) = {
>  int qcom_cc_bind(struct udevice *parent)
>  {
>  	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(parent);
> -	struct udevice *clkdev, *rstdev;
> +	struct udevice *clkdev = NULL, *rstdev = NULL, *pwrdev;
>  	struct driver *drv;
>  	int ret;
>  
> @@ -238,20 +247,41 @@ int qcom_cc_bind(struct udevice *parent)
>  	if (ret)
>  		return ret;
>  
> -	/* Bail out early if resets are not specified for this platform */
> -	if (!data->resets)
> -		return ret;
> +	if (data->resets) {
> +		/* Get a handle to the common reset handler */
> +		drv = lists_driver_lookup_name("qcom_reset");
> +		if (!drv) {
> +			ret = -ENOENT;
> +			goto unbind_clkdev;
> +		}
> +
> +		/* Register the reset controller */
> +		ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> +						   dev_ofnode(parent), &rstdev);
> +		if (ret)
> +			goto unbind_clkdev;
> +	}
>  
> -	/* Get a handle to the common reset handler */
> -	drv = lists_driver_lookup_name("qcom_reset");
> -	if (!drv)
> -		return -ENOENT;
> +	if (data->power_domains) {
> +		/* Get a handle to the common power domain handler */
> +		drv = lists_driver_lookup_name("qcom_power");
> +		if (!drv) {
> +			ret = -ENOENT;
> +			goto unbind_rstdev;
> +		}
> +		/* Register the power domain controller */
> +		ret = device_bind_with_driver_data(parent, drv, "qcom_power", (ulong)data,
> +						   dev_ofnode(parent), &pwrdev);
> +		if (ret)
> +			goto unbind_rstdev;
> +	}
>  
> -	/* Register the reset controller */
> -	ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> -					   dev_ofnode(parent), &rstdev);
> -	if (ret)
> -		device_unbind(clkdev);
> +	return 0;
> +
> +unbind_rstdev:
> +	device_unbind(rstdev);
> +unbind_clkdev:
> +	device_unbind(clkdev);
>  
>  	return ret;
>  }
> @@ -306,3 +336,79 @@ U_BOOT_DRIVER(qcom_reset) = {
>  	.ops = &qcom_reset_ops,
>  	.probe = qcom_reset_probe,
>  };
> +
> +static int qcom_power_set(struct power_domain *pwr, bool on)
> +{
> +	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(pwr->dev);
> +	void __iomem *base = dev_get_priv(pwr->dev);
> +	const struct qcom_power_map *map;
> +	u32 value;
> +	int ret;
> +
> +	if (pwr->id >= data->num_power_domains)
> +		return -ENODEV;
> +
> +	map = &data->power_domains[pwr->id];
> +
> +	if (!map->reg)
> +		return -ENODEV;
> +
> +	value = readl(base + map->reg);
> +
> +	if (on)
> +		value &= ~GDSC_SW_COLLAPSE_MASK;
> +	else
> +		value |= GDSC_SW_COLLAPSE_MASK;
> +
> +	writel(value, base + map->reg);
> +
> +	if (on)
> +		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +					 value,
> +					 (value & GDSC_POWER_UP_COMPLETE) ||
> +					 (value & GDSC_PWR_ON_MASK),
> +					 GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +	else
> +		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +					 value,
> +					 (value & GDSC_POWER_DOWN_COMPLETE) ||
> +					 !(value & GDSC_PWR_ON_MASK),
> +					 GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +
> +	if (ret == -ETIMEDOUT)
> +		printf("WARNING: GDSC %lu is stuck during power on/off\n",
> +		       pwr->id);
> +	return ret;
> +}
> +
> +static int qcom_power_on(struct power_domain *pwr)
> +{
> +	return qcom_power_set(pwr, true);
> +}
> +
> +static int qcom_power_off(struct power_domain *pwr)
> +{
> +	return qcom_power_set(pwr, false);
> +}
> +
> +static const struct power_domain_ops qcom_power_ops = {
> +	.on = qcom_power_on,
> +	.off = qcom_power_off,
> +};
> +
> +static int qcom_power_probe(struct udevice *dev)
> +{
> +	/* Set our priv pointer to the base address */
> +	dev_set_priv(dev, (void *)dev_read_addr(dev));
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(qcom_power) = {
> +	.name = "qcom_power",
> +	.id = UCLASS_POWER_DOMAIN,
> +	.ops = &qcom_power_ops,
> +	.probe = qcom_power_probe,
> +};
> diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h
> index 01088c1901..12a1eaec2b 100644
> --- a/drivers/clk/qcom/clock-qcom.h
> +++ b/drivers/clk/qcom/clock-qcom.h
> @@ -59,9 +59,15 @@ struct qcom_reset_map {
>  	u8 bit;
>  };
>  
> +struct qcom_power_map {
> +	unsigned int reg;
> +};
> +
>  struct clk;
>  
>  struct msm_clk_data {
> +	const struct qcom_power_map	*power_domains;
> +	unsigned long			num_power_domains;
>  	const struct qcom_reset_map	*resets;
>  	unsigned long			num_resets;
>  	const struct gate_clk		*clks;

-- 
// Caleb (they/them)

Re: [PATCH v3 3/4] clk: qcom: add support for power domains uclass

[Caleb Connolly]

On 11/03/2024 21:33, Volodymyr Babchuk wrote:
> Now sub-drivers for particular SoCs can register them as power domain
> drivers. This is needed for upcoming SM8150 support, because it needs
> to power up the Ethernet module.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> Caleb suggested to use "imply POWER_DOMAIN", not "depends
> POWER_DOMAIN" in the Kconfig, but this does not work:
> $ make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm
> scripts/kconfig/conf  --syncconfig Kconfig
> drivers/clk/Kconfig:3:error: recursive dependency detected!
> drivers/clk/Kconfig:3:  symbol CLK is selected by IMX8M_POWER_DOMAIN
> drivers/power/domain/Kconfig:35:        symbol IMX8M_POWER_DOMAIN depends on POWER_DOMAIN
> drivers/power/domain/Kconfig:3: symbol POWER_DOMAIN is implied by CLK_QCOM
> drivers/clk/qcom/Kconfig:3:     symbol CLK_QCOM depends on CLK
> For a resolution refer to Documentation/kbuild/kconfig-language.txt
> subsection "Kconfig recursive dependency limitations"
> 
> 
> Changes in v3:
>  - Added "depends POWER_DOMAIN" to Kconfig (see note)
>  - Use readl_poll_timeout() instead of open coded wait loop
>  - Print warning if power domain can't be enabled/disabled
> 
> Changes in v2:
>  - Reworked qcom_cc_bind() function
>  - Added timeout to qcom_power_set()
>  - Minor fixes in register names and formatting
> 
>  drivers/clk/qcom/Kconfig      |   2 +-
>  drivers/clk/qcom/clock-qcom.c | 132 ++++++++++++++++++++++++++++++----
>  drivers/clk/qcom/clock-qcom.h |   6 ++
>  3 files changed, 126 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 0df0d1881a..8dae635ac2 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -2,7 +2,7 @@ if ARCH_SNAPDRAGON || ARCH_IPQ40XX
>  
>  config CLK_QCOM
>  	bool
> -	depends on CLK && DM_RESET
> +	depends on CLK && DM_RESET && POWER_DOMAIN

This results in a similar dependency error to the one above unless
CONFIG_POWER_DOMAIN is enabled in the defconfig. I think we'll just add
"select POWER_DOMAIN" to ARCH_SNAPDRAGON, same as we have for DM_RESET.

I'll do then when picking this series up, just a note to self.
>  	def_bool n
>  
>  menu "Qualcomm clock drivers"
> diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
> index 729d190c54..7a5938a06a 100644
> --- a/drivers/clk/qcom/clock-qcom.c
> +++ b/drivers/clk/qcom/clock-qcom.c
> @@ -22,7 +22,9 @@
>  #include <linux/bug.h>
>  #include <linux/delay.h>
>  #include <linux/bitops.h>
> +#include <linux/iopoll.h>
>  #include <reset-uclass.h>
> +#include <power-domain-uclass.h>
>  
>  #include "clock-qcom.h"
>  
> @@ -30,6 +32,13 @@
>  #define CBCR_BRANCH_ENABLE_BIT  BIT(0)
>  #define CBCR_BRANCH_OFF_BIT     BIT(31)
>  
> +#define GDSC_SW_COLLAPSE_MASK		BIT(0)
> +#define GDSC_POWER_DOWN_COMPLETE	BIT(15)
> +#define GDSC_POWER_UP_COMPLETE		BIT(16)
> +#define GDSC_PWR_ON_MASK		BIT(31)
> +#define CFG_GDSCR_OFFSET		0x4
> +#define GDSC_STATUS_POLL_TIMEOUT_US	1500
> +
>  /* Enable clock controlled by CBC soft macro */
>  void clk_enable_cbc(phys_addr_t cbcr)
>  {
> @@ -223,7 +232,7 @@ U_BOOT_DRIVER(qcom_clk) = {
>  int qcom_cc_bind(struct udevice *parent)
>  {
>  	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(parent);
> -	struct udevice *clkdev, *rstdev;
> +	struct udevice *clkdev = NULL, *rstdev = NULL, *pwrdev;
>  	struct driver *drv;
>  	int ret;
>  
> @@ -238,20 +247,41 @@ int qcom_cc_bind(struct udevice *parent)
>  	if (ret)
>  		return ret;
>  
> -	/* Bail out early if resets are not specified for this platform */
> -	if (!data->resets)
> -		return ret;
> +	if (data->resets) {
> +		/* Get a handle to the common reset handler */
> +		drv = lists_driver_lookup_name("qcom_reset");
> +		if (!drv) {
> +			ret = -ENOENT;
> +			goto unbind_clkdev;
> +		}
> +
> +		/* Register the reset controller */
> +		ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> +						   dev_ofnode(parent), &rstdev);
> +		if (ret)
> +			goto unbind_clkdev;
> +	}
>  
> -	/* Get a handle to the common reset handler */
> -	drv = lists_driver_lookup_name("qcom_reset");
> -	if (!drv)
> -		return -ENOENT;
> +	if (data->power_domains) {
> +		/* Get a handle to the common power domain handler */
> +		drv = lists_driver_lookup_name("qcom_power");
> +		if (!drv) {
> +			ret = -ENOENT;
> +			goto unbind_rstdev;
> +		}
> +		/* Register the power domain controller */
> +		ret = device_bind_with_driver_data(parent, drv, "qcom_power", (ulong)data,
> +						   dev_ofnode(parent), &pwrdev);
> +		if (ret)
> +			goto unbind_rstdev;
> +	}
>  
> -	/* Register the reset controller */
> -	ret = device_bind_with_driver_data(parent, drv, "qcom_reset", (ulong)data,
> -					   dev_ofnode(parent), &rstdev);
> -	if (ret)
> -		device_unbind(clkdev);
> +	return 0;
> +
> +unbind_rstdev:
> +	device_unbind(rstdev);
> +unbind_clkdev:
> +	device_unbind(clkdev);
>  
>  	return ret;
>  }
> @@ -306,3 +336,79 @@ U_BOOT_DRIVER(qcom_reset) = {
>  	.ops = &qcom_reset_ops,
>  	.probe = qcom_reset_probe,
>  };
> +
> +static int qcom_power_set(struct power_domain *pwr, bool on)
> +{
> +	struct msm_clk_data *data = (struct msm_clk_data *)dev_get_driver_data(pwr->dev);
> +	void __iomem *base = dev_get_priv(pwr->dev);
> +	const struct qcom_power_map *map;
> +	u32 value;
> +	int ret;
> +
> +	if (pwr->id >= data->num_power_domains)
> +		return -ENODEV;
> +
> +	map = &data->power_domains[pwr->id];
> +
> +	if (!map->reg)
> +		return -ENODEV;
> +
> +	value = readl(base + map->reg);
> +
> +	if (on)
> +		value &= ~GDSC_SW_COLLAPSE_MASK;
> +	else
> +		value |= GDSC_SW_COLLAPSE_MASK;
> +
> +	writel(value, base + map->reg);
> +
> +	if (on)
> +		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +					 value,
> +					 (value & GDSC_POWER_UP_COMPLETE) ||
> +					 (value & GDSC_PWR_ON_MASK),
> +					 GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +	else
> +		ret = readl_poll_timeout(base + map->reg + CFG_GDSCR_OFFSET,
> +					 value,
> +					 (value & GDSC_POWER_DOWN_COMPLETE) ||
> +					 !(value & GDSC_PWR_ON_MASK),
> +					 GDSC_STATUS_POLL_TIMEOUT_US);
> +
> +
> +	if (ret == -ETIMEDOUT)
> +		printf("WARNING: GDSC %lu is stuck during power on/off\n",
> +		       pwr->id);
> +	return ret;
> +}
> +
> +static int qcom_power_on(struct power_domain *pwr)
> +{
> +	return qcom_power_set(pwr, true);
> +}
> +
> +static int qcom_power_off(struct power_domain *pwr)
> +{
> +	return qcom_power_set(pwr, false);
> +}
> +
> +static const struct power_domain_ops qcom_power_ops = {
> +	.on = qcom_power_on,
> +	.off = qcom_power_off,
> +};
> +
> +static int qcom_power_probe(struct udevice *dev)
> +{
> +	/* Set our priv pointer to the base address */
> +	dev_set_priv(dev, (void *)dev_read_addr(dev));
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(qcom_power) = {
> +	.name = "qcom_power",
> +	.id = UCLASS_POWER_DOMAIN,
> +	.ops = &qcom_power_ops,
> +	.probe = qcom_power_probe,
> +};
> diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h
> index 01088c1901..12a1eaec2b 100644
> --- a/drivers/clk/qcom/clock-qcom.h
> +++ b/drivers/clk/qcom/clock-qcom.h
> @@ -59,9 +59,15 @@ struct qcom_reset_map {
>  	u8 bit;
>  };
>  
> +struct qcom_power_map {
> +	unsigned int reg;
> +};
> +
>  struct clk;
>  
>  struct msm_clk_data {
> +	const struct qcom_power_map	*power_domains;
> +	unsigned long			num_power_domains;
>  	const struct qcom_reset_map	*resets;
>  	unsigned long			num_resets;
>  	const struct gate_clk		*clks;

-- 
// Caleb (they/them)

Re: [PATCH v3 0/4]

[Caleb Connolly]

On Mon, 11 Mar 2024 21:33:44 +0000, Volodymyr Babchuk wrote:
> Set of pre-req patches for Qualcomm SA8155P-ADP board support.
> 
> This path series consist of generic qcom changes that may benefit
> different boards. It is the part of the bigger series that adds
> SA8155P-ADP support, but I am posting this limited set because there
> are other developers who depend on those changes and I am not ready to
> post other patches of the bigger series.
> 
> [...]

Applied, thanks!

[1/4] qcom: board: validate fdt before trying to use it
      commit: a1ecfa2371efc68671c66d3e186743f82926a640
[2/4] clk: qcom: clear div mask before assigning a new divider
      commit: 10f402108a9063d5bc4d517e2a1197afcfabc3a4
[3/4] clk: qcom: add support for power domains uclass
      commit: 95d76bf4e9a912ee458726a59b1045ecb2eff0cc
[4/4] pinctrl: qcom: pass pin number to get_function_mux callback
      commit: 018b8ab702ce38f79e7d276186565478513880a6

Best regards,
-- 
// Caleb (they/them)

Re: [PATCH v3 0/4]

[Ilias Apalodimas]

On Sat, 20 Apr 2024 at 10:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/18/24 14:54, Ilias Apalodimas wrote:
> > Hi!
> > This is v3 of SetVariable at runtime [0]
> >
> > Nothing changed drastically from v2.
> > A few more test cases have been added, comments/suggestions have been
> > addressed and a bug where deleting a variable by setting 'attributes' to
> > 0 has been fixed.
> >
> > Changes since v2:
> > - Add more selftests checking for variable deletion as well as the
> >    VarToFile header format
> > - Use unaligned sized variables on tests
> > - Add a new function to get the variable entry length instead of
> >    repurposing efi_var_mem_compare()
> > - Converted VarToFile to RO
> > - Added a few comments requested by Heinrich
> > - Fixed a bug where SetVariable with attributes set to 0 did not delete
> >    the variable but returned EFI_INVALID_PARAMETER instead
> > - Run FWTS 'uefitests' and attach the log in patch #1
> > - Added r-b tags from Heinrich
> >
> > Changes since v1:
> > - Instead of Creating VarToFile at SetVariable, create it on GetVariable.
> >    This allows us to get rid of the preallocated RT buffer, since the
> >    address is user provided
> > - convert Set/GetVariableRT -> Set/GetVariable at runtime
> > - return EFI_INVALID_PARAM is NV is not set at runtime
> > - Heinrich sent me the efi_var_collect_mem() variant
> >
> > Changes since the RFC:
> > - Return EFI_INVALID_PARAM if attributes are not volatile
> > - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> > - Add 2 EFI variables and allow userspace to write the file
> > - Add selftests
> >
> > I also have a patch enable QueryVariableInfo [1], but I don't want to
> > introduce new patches now. This also needs a few more testcases of its
> > own so I'll send it once we finalize this one.
> >
> > [0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
> > [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0
> >
> > Regards
> > /Ilias
> >
> > Ilias Apalodimas (4):
> >    efi_loader: conditionally enable SetvariableRT
> >    efi_loader: Add OS notifications for SetVariable at runtime
> >    efi_loader: add an EFI variable with the file contents
> >    efi_selftest: add tests for setvariableRT
>
> I am missing a defconfig change which is needed to run the unit test in
> Gitlab CI. I would prefer testing this on the sandbox.
>
> Best regards

Ok I'll send a followup since you already sent a PR with these

Thanks!
/Ilias
>
> Heinrich
>
> >
> >   include/efi_loader.h                          |   4 +
> >   include/efi_variable.h                        |  16 +-
> >   lib/charset.c                                 |   2 +-
> >   lib/efi_loader/Kconfig                        |  16 ++
> >   lib/efi_loader/efi_runtime.c                  |  42 ++++
> >   lib/efi_loader/efi_var_common.c               |   6 +-
> >   lib/efi_loader/efi_var_mem.c                  | 151 ++++++++-----
> >   lib/efi_loader/efi_variable.c                 | 122 ++++++++--
> >   lib/efi_loader/efi_variable_tee.c             |   5 -
> >   .../efi_selftest_variables_runtime.c          | 211 +++++++++++++++++-
> >   10 files changed, 495 insertions(+), 80 deletions(-)
> >
> > --
> > 2.40.1
> >
>

Re: [PATCH v3 0/4]

[Heinrich Schuchardt]

On 4/18/24 14:54, Ilias Apalodimas wrote:
> Hi!
> This is v3 of SetVariable at runtime [0]
>
> Nothing changed drastically from v2.
> A few more test cases have been added, comments/suggestions have been
> addressed and a bug where deleting a variable by setting 'attributes' to
> 0 has been fixed.
>
> Changes since v2:
> - Add more selftests checking for variable deletion as well as the
>    VarToFile header format
> - Use unaligned sized variables on tests
> - Add a new function to get the variable entry length instead of
>    repurposing efi_var_mem_compare()
> - Converted VarToFile to RO
> - Added a few comments requested by Heinrich
> - Fixed a bug where SetVariable with attributes set to 0 did not delete
>    the variable but returned EFI_INVALID_PARAMETER instead
> - Run FWTS 'uefitests' and attach the log in patch #1
> - Added r-b tags from Heinrich
>
> Changes since v1:
> - Instead of Creating VarToFile at SetVariable, create it on GetVariable.
>    This allows us to get rid of the preallocated RT buffer, since the
>    address is user provided
> - convert Set/GetVariableRT -> Set/GetVariable at runtime
> - return EFI_INVALID_PARAM is NV is not set at runtime
> - Heinrich sent me the efi_var_collect_mem() variant
>
> Changes since the RFC:
> - Return EFI_INVALID_PARAM if attributes are not volatile
> - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> - Add 2 EFI variables and allow userspace to write the file
> - Add selftests
>
> I also have a patch enable QueryVariableInfo [1], but I don't want to
> introduce new patches now. This also needs a few more testcases of its
> own so I'll send it once we finalize this one.
>
> [0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
> [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0
>
> Regards
> /Ilias
>
> Ilias Apalodimas (4):
>    efi_loader: conditionally enable SetvariableRT
>    efi_loader: Add OS notifications for SetVariable at runtime
>    efi_loader: add an EFI variable with the file contents
>    efi_selftest: add tests for setvariableRT

I am missing a defconfig change which is needed to run the unit test in
Gitlab CI. I would prefer testing this on the sandbox.

Best regards

Heinrich

>
>   include/efi_loader.h                          |   4 +
>   include/efi_variable.h                        |  16 +-
>   lib/charset.c                                 |   2 +-
>   lib/efi_loader/Kconfig                        |  16 ++
>   lib/efi_loader/efi_runtime.c                  |  42 ++++
>   lib/efi_loader/efi_var_common.c               |   6 +-
>   lib/efi_loader/efi_var_mem.c                  | 151 ++++++++-----
>   lib/efi_loader/efi_variable.c                 | 122 ++++++++--
>   lib/efi_loader/efi_variable_tee.c             |   5 -
>   .../efi_selftest_variables_runtime.c          | 211 +++++++++++++++++-
>   10 files changed, 495 insertions(+), 80 deletions(-)
>
> --
> 2.40.1
>

[PATCH v3 0/4]

[Ilias Apalodimas]

Hi!
This is v3 of SetVariable at runtime [0]

Nothing changed drastically from v2.
A few more test cases have been added, comments/suggestions have been
addressed and a bug where deleting a variable by setting 'attributes' to
0 has been fixed.

Changes since v2:
- Add more selftests checking for variable deletion as well as the
  VarToFile header format
- Use unaligned sized variables on tests
- Add a new function to get the variable entry length instead of
  repurposing efi_var_mem_compare()
- Converted VarToFile to RO
- Added a few comments requested by Heinrich
- Fixed a bug where SetVariable with attributes set to 0 did not delete
  the variable but returned EFI_INVALID_PARAMETER instead
- Run FWTS 'uefitests' and attach the log in patch #1
- Added r-b tags from Heinrich

Changes since v1:
- Instead of Creating VarToFile at SetVariable, create it on GetVariable.
  This allows us to get rid of the preallocated RT buffer, since the
  address is user provided
- convert Set/GetVariableRT -> Set/GetVariable at runtime
- return EFI_INVALID_PARAM is NV is not set at runtime
- Heinrich sent me the efi_var_collect_mem() variant

Changes since the RFC:
- Return EFI_INVALID_PARAM if attributes are not volatile
- Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
- Add 2 EFI variables and allow userspace to write the file
- Add selftests

I also have a patch enable QueryVariableInfo [1], but I don't want to
introduce new patches now. This also needs a few more testcases of its
own so I'll send it once we finalize this one.

[0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
[1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0

Regards
/Ilias

Ilias Apalodimas (4):
  efi_loader: conditionally enable SetvariableRT
  efi_loader: Add OS notifications for SetVariable at runtime
  efi_loader: add an EFI variable with the file contents
  efi_selftest: add tests for setvariableRT

 include/efi_loader.h                          |   4 +
 include/efi_variable.h                        |  16 +-
 lib/charset.c                                 |   2 +-
 lib/efi_loader/Kconfig                        |  16 ++
 lib/efi_loader/efi_runtime.c                  |  42 ++++
 lib/efi_loader/efi_var_common.c               |   6 +-
 lib/efi_loader/efi_var_mem.c                  | 151 ++++++++-----
 lib/efi_loader/efi_variable.c                 | 122 ++++++++--
 lib/efi_loader/efi_variable_tee.c             |   5 -
 .../efi_selftest_variables_runtime.c          | 211 +++++++++++++++++-
 10 files changed, 495 insertions(+), 80 deletions(-)

--
2.40.1

[PATCH v3 0/4]

[Dmitry Baryshkov]

This patch concludes the conversion of display/msm schema from txt files
to YAML format.

The per-SoC compat (new addition) is required to ease migrating platform
support between mdp5 and dpu drivers.

Changes since v2:
- Fix MSM8998 compatible list: "qcom,msm8998-dpu", "msm,mdp5" to allow
  handling this device by either of the drivers.

Changes since v1:
- Renamed mdp@ to display-controller@ in the example (Krzysztof)
- Extended ports description to mention possible ports (Krzysztof)
- Fixed ports@ regexp to limit to just four ports (Krzysztof)
- Included patches adding per-SoC compat strings to the schema and to
  dtsi files.

Dmitry Baryshkov (4):
  dt-bindings: display/msm: convert MDP5 schema to YAML format
  dt-bindings: display/msm: add SoC-specific compats to qcom,mdp5.yaml
  ARM: dts: qcom-msm8974: add SoC specific compat string to mdp5 node
  arm64: dts: qcom: add SoC specific compat strings to mdp5 nodes

 .../devicetree/bindings/display/msm/mdp5.txt  | 132 ---------------
 .../bindings/display/msm/qcom,mdp5.yaml       | 154 ++++++++++++++++++
 arch/arm/boot/dts/qcom-msm8974.dtsi           |   2 +-
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |   2 +-
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   2 +-
 arch/arm64/boot/dts/qcom/sdm630.dtsi          |   2 +-
 arch/arm64/boot/dts/qcom/sdm660.dtsi          |   2 +
 7 files changed, 160 insertions(+), 136 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/msm/mdp5.txt
 create mode 100644 Documentation/devicetree/bindings/display/msm/qcom,mdp5.yaml

-- 
2.39.0

[PATCH v3 0/4]

[Dmitry Baryshkov]

This patch concludes the conversion of display/msm schema from txt files
to YAML format.

The per-SoC compat (new addition) is required to ease migrating platform
support between mdp5 and dpu drivers.

Changes since v2:
- Fix MSM8998 compatible list: "qcom,msm8998-dpu", "msm,mdp5" to allow
  handling this device by either of the drivers.

Changes since v1:
- Renamed mdp@ to display-controller@ in the example (Krzysztof)
- Extended ports description to mention possible ports (Krzysztof)
- Fixed ports@ regexp to limit to just four ports (Krzysztof)
- Included patches adding per-SoC compat strings to the schema and to
  dtsi files.

Dmitry Baryshkov (4):
  dt-bindings: display/msm: convert MDP5 schema to YAML format
  dt-bindings: display/msm: add SoC-specific compats to qcom,mdp5.yaml
  ARM: dts: qcom-msm8974: add SoC specific compat string to mdp5 node
  arm64: dts: qcom: add SoC specific compat strings to mdp5 nodes

 .../devicetree/bindings/display/msm/mdp5.txt  | 132 ---------------
 .../bindings/display/msm/qcom,mdp5.yaml       | 154 ++++++++++++++++++
 arch/arm/boot/dts/qcom-msm8974.dtsi           |   2 +-
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |   2 +-
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   2 +-
 arch/arm64/boot/dts/qcom/sdm630.dtsi          |   2 +-
 arch/arm64/boot/dts/qcom/sdm660.dtsi          |   2 +
 7 files changed, 160 insertions(+), 136 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/display/msm/mdp5.txt
 create mode 100644 Documentation/devicetree/bindings/display/msm/qcom,mdp5.yaml

-- 
2.39.0