Re: [U-Boot] [PATCH V4 24/32] power: Add power domain driver for i.MX8

-----Original Message----- From: Lothar Waßmann [mailto:LW@KARO-electronics.de] Sent: 2018年9月13日 23:59 To: Peng Fan peng.fan@nxp.com Cc: sbabic@denx.de; Fabio Estevam fabio.estevam@nxp.com; u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH V4 24/32] power: Add power domain driver for i.MX8

Hi,

On Wed, 5 Sep 2018 10:12:11 +0800 Peng Fan wrote:

...

Add the power domain DM driver for i.MX8, that it depends on the DTB power domain trees to generate the power domain provider devices. Users needs add power domain trees with property "compatible =

"nxp,imx8-pd";"

...

When power on one PD device, the driver will power on its ancestor PD devices in power domain tree.

When power off on PD device, the driver will check its child PD devices first, only all child PD devices are off, then power off the current PD device. Then the driver checks sibling PD devices. If sibling PD devices are off, then it will power off parent PD device.

There is no counter maintained in this driver, but a state to hold current on/off state. So the request and free functions are empty.

The power domain implementation in i.MX8 DTB set the

"#power-domain-cells"

...

to 0, so there is no ID binding with each PD device. We don't use "id" variable in struct power_domain. At same time, we have to set of_xlate to empty to bypass standard of_xlate in uclass driver.

Signed-off-by: Ye Li ye.li@nxp.com Signed-off-by: Peng Fan peng.fan@nxp.com Cc: Stefano Babic sbabic@denx.de

---

arch/arm/include/asm/arch-imx8/power-domain.h | 15 ++ drivers/power/domain/Kconfig | 6 + drivers/power/domain/Makefile | 1 + drivers/power/domain/imx8-power-domain.c | 312

++++++++++++++++++++++++++

...

4 files changed, 334 insertions(+) create mode 100644 arch/arm/include/asm/arch-imx8/power-domain.h create mode 100644 drivers/power/domain/imx8-power-domain.c

diff --git a/arch/arm/include/asm/arch-imx8/power-domain.h b/arch/arm/include/asm/arch-imx8/power-domain.h new file mode 100644 index 0000000000..1396008877 --- /dev/null +++ b/arch/arm/include/asm/arch-imx8/power-domain.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright 2017 NXP
• */

+#ifndef _ASM_ARCH_IMX8_POWER_DOMAIN_H #define +_ASM_ARCH_IMX8_POWER_DOMAIN_H

+#include <asm/arch/sci/types.h>

+struct imx8_power_domain_platdata {

• sc_rsrc_t resource_id;

+};

+#endif diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig index 7cfa761498..2a72642a26 100644 --- a/drivers/power/domain/Kconfig +++ b/drivers/power/domain/Kconfig @@ -31,4 +31,10 @@ config TEGRA186_POWER_DOMAIN Enable support for manipulating Tegra's on-SoC power domains via IPC requests to the BPMP (Boot and Power Management Processor).

+config IMX8_POWER_DOMAIN

• bool "Enable i.MX8 power domain driver"

•    depends on ARCH_IMX8
  

•    help
  

•      Enable support for manipulating NXP i.MX8 on-SoC power
  

domains via IPC

...

•      requests to the SCU.
  

endmenu diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile index 020eee2378..1ef4844c0b 100644 --- a/drivers/power/domain/Makefile +++ b/drivers/power/domain/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o obj-$(CONFIG_SANDBOX_POWER_DOMAIN) += sandbox-power-domain.o obj-$(CONFIG_SANDBOX_POWER_DOMAIN) +=

sandbox-power-domain-test.o

...

obj-$(CONFIG_TEGRA186_POWER_DOMAIN) += tegra186-power-domain.o +obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain.o diff --git a/drivers/power/domain/imx8-power-domain.c b/drivers/power/domain/imx8-power-domain.c new file mode 100644 index 0000000000..be91d626ad --- /dev/null +++ b/drivers/power/domain/imx8-power-domain.c @@ -0,0 +1,312 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright 2017 NXP
• */

+#include <common.h> +#include <dm.h> +#include <power-domain-uclass.h> +#include <asm/io.h> +#include <asm/arch/power-domain.h> +#include <dm/device-internal.h> +#include <dm/device.h> +#include <asm/arch/sci/sci.h>

+DECLARE_GLOBAL_DATA_PTR;

+struct imx8_power_domain_priv {

• bool state_on;

+};

+static int imx8_power_domain_request(struct power_domain +*power_domain) {

• debug("%s(power_domain=%p)\n", __func__, power_domain);
• return 0;

+}

+static int imx8_power_domain_free(struct power_domain *power_domain) +{

• debug("%s(power_domain=%p)\n", __func__, power_domain);
• return 0;

+}

+static int imx8_power_domain_on(struct power_domain *power_domain) {

• struct udevice *dev = power_domain->dev;
• struct imx8_power_domain_platdata *pdata;
• struct imx8_power_domain_priv *ppriv;
• sc_err_t ret;
• struct power_domain parent_domain;
• struct udevice *parent = dev_get_parent(dev);
• /* Need to power on parent node first */
• if (device_get_uclass_id(parent) == UCLASS_POWER_DOMAIN) {

• parent_domain.dev = parent;
  

• imx8_power_domain_on(&parent_domain);
  
  

What if this fails? Is it actually sensible to continue enabling the power domain, when its parent domain could not be enabled?

Actually it should not fail, if it fails, there mush be something wrong. I'll add a return value check here.

...

• pdata = (struct imx8_power_domain_platdata *)dev_get_platdata(dev);
• ppriv = (struct imx8_power_domain_priv *)dev_get_priv(dev);

useless type casts.

Drop in next version.

Thanks, Peng.

Lothar Waßmann