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

On 29.02.2024 15:21, 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

drivers/clk/qcom/clock-qcom.c | 93 ++++++++++++++++++++++++++++++++++- drivers/clk/qcom/clock-qcom.h | 6 +++ 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c index 729d190c54..986b8e4da4 100644 --- a/drivers/clk/qcom/clock-qcom.c +++ b/drivers/clk/qcom/clock-qcom.c @@ -23,6 +23,7 @@ #include <linux/delay.h> #include <linux/bitops.h> #include <reset-uclass.h> +#include <power-domain-uclass.h>

#include "clock-qcom.h"

@@ -30,6 +31,11 @@ #define CBCR_BRANCH_ENABLE_BIT BIT(0) #define CBCR_BRANCH_OFF_BIT BIT(31)

+#define GDSC_POWER_UP_COMPLETE BIT(16) +#define GDSC_POWER_DOWN_COMPLETE BIT(15)

Please keep things sorted by bit index for bitfields and register offset for registers

+#define CFG_GDSCR_OFFSET 0x4 +#define PWR_ON_MASK BIT(31)

/* Enable clock controlled by CBC soft macro */ void clk_enable_cbc(phys_addr_t cbcr) { @@ -223,7 +229,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, *rstdev, *pwrdev; struct driver *drv; int ret;

@@ -253,6 +259,20 @@ int qcom_cc_bind(struct udevice *parent) if (ret) device_unbind(clkdev);

• if (!data->power_domains)

• return ret;
  

• /* Get a handle to the common power domain handler */
• drv = lists_driver_lookup_name("qcom_power");
• if (!drv)

• return -ENOENT;
  

• /* Register the power domain controller */
• ret = device_bind_with_driver_data(parent, drv, "qcom_power", (ulong)data,

• 			   dev_ofnode(parent), &pwrdev);
  

• if (ret)

• device_unbind(pwrdev);
  

• return ret;

}

@@ -306,3 +326,74 @@ 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;
• 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 &= ~BIT(0);
  

These magic bits should be #defined.

• else

• value |= BIT(0);
  

• writel(value, base + map->reg);
• /* Wait for power on */
• while (true) {

Uhh.. I'm not sure a tight loop is the best idea here, especially since GDSCs are known to be grumpy and not wanna power on if they're missing a dependency. I'd say a timeout is definitely in order. 1500ms following Linux

Konrad