On 2 September 2014 20:28, Geert Uytterhoeven [off-list ref] wrote:

Hi Ulf, Tomasz,

On Thu, Aug 28, 2014 at 10:38 AM, Ulf Hansson [off-list ref] wrote:

quoted

From: Tomasz Figa <redacted>

This patch introduces generic code to perform power domain look-up using

Should "power domain" be replaced by "PM domain" here (and everywhere else
in this patch), too, cfr. Rafael's earlier comment?

Hi Geert,

Thanks for reviewing!

I changed some of the other patches according to Rafael's earlier
comment, but decided to keep this as is. The reason is that in
drivers/base/power/domain.c there are already a mix between using
"power domain" and "PM domain", thus I couldn't decide which way to
go.

I have no strong opinion of what terminology we use, I am happy to
change this patch. Since I anyway will be sending new version, I will
update to "PM domain".

quoted

+++ b/Documentation/devicetree/bindings/power/power_domain.txt

@@ -0,0 +1,51 @@
+* Generic power domains
+
+System on chip designs are often divided into multiple power domains that
+can be used for power gating of selected IP blocks for power saving by
+reduced leakage current.
+
+This device tree binding can be used to bind power domain consumer devices
+with their power domains provided by power domain providers. A power domain
+provider can be represented by any node in the device tree and can provide
+one or more power domains. A consumer node can refer to the provider by
+a phandle and a set of phandle arguments (so called power domain specifier)

specifiers

quoted

+of length specified by #power-domain-cells property in the power domain

the #power-domain-cells property

quoted

+provider node.
+
+==Power domain providers==
+
+Required properties:
+ - #power-domain-cells : Number of cells in a power domain specifier;
+   Typically 0 for nodes representing a single power domain and 1 for nodes
+   providing multiple power domains (e.g. power controllers), but can be
+   any value as specified by device tree binding documentation of particular
+   provider.
+
+Example:
+
+       power: power-controller@12340000 {
+               compatible = "foo,power-controller";
+               reg = <0x12340000 0x1000>;
+               #power-domain-cells = <1>;
+       };
+
+The node above defines a power controller that is a power domain provider
+and expects one cell as its phandle argument.
+
+==Power domain consumers==
+
+Required properties:
+ - power-domains : A phandle and power domain specifier as defined by bindings
+                   of power controller specified by phandle.

the power controller

quoted

+
+Example:
+
+       leaky-device@12350000 {
+               compatible = "foo,i-leak-current";
+               reg = <0x12350000 0x1000>;
+               power-domains = <&power 0>;
+       };
+
+The node above defines a typical power domain consumer device, which is located
+inside power domain with index 0 of power controller represented by node with

the power domain ... the power controller ... the node

quoted

+label "power".

quoted

--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c

quoted

+#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+/*
+ * Device Tree based power domain providers.
+ *
+ * The code below implements generic device tree based power domain providers
+ * that bind device tree nodes with generic power domains registered in the
+ * system.
+ *
+ * Any driver that registers generic power domains and need to support binding

needs

quoted

+ * of devices to these domains is supposed to register a power domain provider,
+ * which maps a power domain specifier retrieved from device tree to a power

the device tree

quoted

+/**
+ * of_genpd_xlate_onecell() - Xlate function for providers using single index.

a single index

quoted

+ * @genpdspec: OF phandle args to map into a power domain
+ * @data: xlate function private data - pointer to struct genpd_onecell_data
+ *
+ * This is a generic xlate function that can be used to model simple power
+ * domain controllers that have one device tree node and provide multiple
+ * power domains. A single cell is used as an index to an array of power

an index into

quoted

+ * domains specified in genpd_onecell_data struct when registering the

the genpd_onecell_data struct

quoted

+ * provider.
+ */
+struct generic_pm_domain *of_genpd_xlate_onecell(
+                                       struct of_phandle_args *genpdspec,
+                                       void *data)
+{
+       struct genpd_onecell_data *genpd_data = data;
+       unsigned int idx = genpdspec->args[0];
+
+       if (genpdspec->args_count != 1)
+               return ERR_PTR(-EINVAL);
+
+       if (idx >= genpd_data->domain_num) {
+               pr_err("%s: invalid domain index %d\n", __func__, idx);

%u for unsigned int idx

quoted

+               return ERR_PTR(-EINVAL);
+       }
+
+       return genpd_data->domains[idx];

This assumes ->domains[] is a contiguous array.

I'd add a NULL check here, to translate NULL to ERR_PTR(-Esomething),
so it can support sparse power domain spaces.
E.g. indexed by the "bit_shift" value of struct rmobile_pm_domain, which
is the actual bit number to use to power up/down an R-Mobile domain.
Different members of the R-Mobile family have different numbers of
domains, and use more or less bits in the same power up/down registers.
This would be similar to the sparse clock-indices handling in
renesas,cpg-mstp-clocks.

OK, will do!

quoted

+}
+EXPORT_SYMBOL_GPL(of_genpd_xlate_onecell);
+
+/**
+ * of_genpd_add_provider() - Register a domain provider for a node
+ * @np: Device node pointer associated with domain provider.

the domain provider

quoted

+/**
+ * of_genpd_del_provider() - Remove a previously registered domain provider
+ * @np: Device node pointer associated with domain provider

the domain provider

quoted

+/**
+ * of_genpd_get_from_provider() - Look-up power domain
+ * @genpdspec: OF phandle args to use for look-up
+ *
+ * Looks for domain provider under node specified by @genpdspec and if found

a domain provider ... under the node

quoted

+ * uses xlate function of the provider to map phandle args to a power domain.

quoted

+/**
+ * genpd_dev_pm_attach - Attach a device to it's power domain using DT.

its

quoted

+ * @dev: Device to attach.

quoted

+/**
+ * genpd_dev_pm_detach - Detach a device from it's power domain.

its

quoted

+ * @dev: Device to attach.
+ *
+ * Try to locate a corresponding generic power domain, which the device
+ * then previously were attached to. If found the device is detached from

"was attached to previously"?

quoted

+ * the power domain.

quoted

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 7c1d252..5989758 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h

@@ -310,4 +310,50 @@ static inline void pm_genpd_syscore_poweron(struct device *dev)
        pm_genpd_syscore_switch(dev, false);
 }

+/* OF power domain providers */
+struct of_device_id;
+
+struct genpd_onecell_data {
+       struct generic_pm_domain **domains;
+       unsigned int domain_num;

This is the number of domains: "num_domains"?

OK, that describes it better.

quoted

+};
+
+typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
+                                                  void *data);
+
+#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+int of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
+                         void *data);
+void of_genpd_del_provider(struct device_node *np);
+
+struct generic_pm_domain *of_genpd_xlate_simple(
+                                       struct of_phandle_args *genpdspec,
+                                       void *data);
+struct generic_pm_domain *of_genpd_xlate_onecell(
+                                       struct of_phandle_args *genpdspec,
+                                       void *data);
+
+int genpd_dev_pm_attach(struct device *dev);
+int genpd_dev_pm_detach(struct device *dev);
+#else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
+static inline int of_genpd_add_provider(struct device_node *np,
+                                       genpd_xlate_t xlate, void *data)
+{
+       return 0;
+}
+static inline void of_genpd_del_provider(struct device_node *np) {}
+
+#define of_genpd_xlate_simple          NULL
+#define of_genpd_xlate_onecell         NULL
+
+static inline int genpd_dev_pm_attach(struct device *dev)
+{
+       return -ENODEV;
+}
+static inline int genpd_dev_pm_detach(struct device *dev)
+{
+       return -ENODEV;
+}
+#endif /* CONFIG_PM_GENERIC_DOMAINS_OF */

As of_genpd_add_provider() provides no compile-time type-checking for
"void *data", I propose to prepend a double underscore to
of_genpd_add_provider(), and to the xlate helpers of_genpd_xlate_simple()
and of_genpd_xlate_onecell(), and provide type-checking wrappers:

Good idea, I will give it a try in next version.

static inline int of_genpd_add_provider_simple(struct device_node *np,
                                               struct generic_pm_domain *genpd)
{
        return __of_genpd_add_provider(np, __of_genpd_xlate_simple, genpd);
}
static inline int of_genpd_add_provider_onecell(struct device_node *np,
                                                struct genpd_onecell_data *data)
{
        return __of_genpd_add_provider(np, __of_genpd_xlate_onecell, data);
}

Thanks for all the grammar/English corrections as well. Will update.

Kind regards
Uffe