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

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/8af77314... [2]: https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commit/33b09410...

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;