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; }

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

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;

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;