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

Caleb Connolly caleb.connolly at linaro.org
Wed Mar 13 18:12:38 CET 2024


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 at 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)


More information about the U-Boot mailing list