[PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support

[PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

V2:
 Fix yaml check failure

Previously there is an effort from Abel that take BLK-CTL as clock
provider, but it turns out that there is A/B lock issue and we are
not able resolve that.

Per discuss with Lucas and Jacky, we made an agreement that take BLK-CTL
as a power domain provider and use GPC's domain as parent, the consumer
node take BLK-CTL as power domain input.

This patchset has been tested on i.MX8MM EVK board, but one hack
is not included in the patchset is that the DISPMIX BLK-CTL MIPI_M/S_RESET
not implemented. Per Lucas, we will finally have a MIPI DPHY driver,
so fine to leave it.

Thanks for Lucas's suggestion, Frieder Schrempf for collecting
all the patches, Abel's previous BLK-CTL work, Jacky Bai on help
debug issues.

Peng Fan (4):
  dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains
  Documentation: bindings: clk: Add bindings for i.MX BLK_CTL
  soc: imx: Add generic blk-ctl driver
  soc: imx: Add blk-ctl driver for i.MX8MM

 .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     |  73 +++++
 drivers/soc/imx/Makefile                      |   2 +-
 drivers/soc/imx/blk-ctl-imx8mm.c              | 138 ++++++++
 drivers/soc/imx/blk-ctl.c                     | 303 ++++++++++++++++++
 drivers/soc/imx/blk-ctl.h                     |  76 +++++
 include/dt-bindings/power/imx8mm-power.h      |  11 +
 6 files changed, 602 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
 create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c
 create mode 100644 drivers/soc/imx/blk-ctl.c
 create mode 100644 drivers/soc/imx/blk-ctl.h

-- 
2.30.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 1/4] dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Adding the defines for i.MX8MM BLK-CTL power domains.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/dt-bindings/power/imx8mm-power.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/dt-bindings/power/imx8mm-power.h b/include/dt-bindings/power/imx8mm-power.h
index fc9c2e16aadc..510e383d1953 100644
--- a/include/dt-bindings/power/imx8mm-power.h
+++ b/include/dt-bindings/power/imx8mm-power.h
@@ -19,4 +19,15 @@
 #define IMX8MM_POWER_DOMAIN_DISPMIX	10
 #define IMX8MM_POWER_DOMAIN_MIPI	11
 
+#define IMX8MM_BLK_CTL_G2_PD		0
+#define IMX8MM_BLK_CTL_G1_PD		1
+#define IMX8MM_BLK_CTL_H1_PD		2
+#define IMX8MM_BLK_CTL_MAX_PD		3
+
+#define IMX8MM_BLK_CTL_DISPMIX_CSI_BRIDGE	0
+#define IMX8MM_BLK_CTL_DISPMIX_LCDIF		1
+#define IMX8MM_BLK_CTL_DISPMIX_MIPI_DSI		2
+#define IMX8MM_BLK_CTL_DISPMIX_MIPI_CSI		3
+#define IMX8MM_BLK_CTL_DISPMIX_MAX_PD		4
+
 #endif
-- 
2.30.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 2/4] Documentation: bindings: clk: Add bindings for i.MX BLK_CTL

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Document the i.MX BLK_CTL with its devicetree properties.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml

diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
new file mode 100644
index 000000000000..a491b63de50c
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/imx/fsl,imx-blk-ctl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX BLK_CTL
+
+maintainers:
+  - Peng Fan <peng.fan@nxp.com>
+
+description:
+  i.MX BLK_CTL is a conglomerate of different GPRs that are
+  dedicated to a specific subsystem. It usually contains
+  clocks and resets amongst other things. Here we take the clocks
+  and resets as virtual PDs, the reason we could not take it as
+  clock provider is there is A/B lock issue between power domain
+  and clock.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - fsl,imx8mm-dispmix-blk-ctl
+          - fsl,imx8mm-vpumix-blk-ctl
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  "#power-domain-cells":
+    const: 1
+
+  power-domains:
+    minItems: 1
+    maxItems: 32
+
+  power-domain-names:
+    minItems: 1
+    maxItems: 32
+
+  '#reset-cells':
+    const: 1
+
+  clocks:
+    minItems: 1
+    maxItems: 32
+
+  clock-names:
+    minItems: 1
+    maxItems: 32
+
+required:
+  - compatible
+  - reg
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+
+    dispmix_blk_ctl: blk-ctl@32e28000 {
+      compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
+      reg = <0x32e28000 0x100>;
+      #power-domain-cells = <1>;
+      power-domains = <&pgc_dispmix>, <&pgc_mipi>;
+      power-domain-names = "dispmix", "mipi";
+      clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
+               <&clk IMX8MM_CLK_DISP_APB_ROOT>;
+      clock-names = "disp", "axi", "apb";
+    };
-- 
2.30.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/4] Documentation: bindings: clk: Add bindings for i.MX BLK_CTL

[Rob Herring]

On Fri, Apr 30, 2021 at 01:27:44PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Document the i.MX BLK_CTL with its devicetree properties.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
> new file mode 100644
> index 000000000000..a491b63de50c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/imx/fsl,imx-blk-ctl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX BLK_CTL
> +
> +maintainers:
> +  - Peng Fan <peng.fan@nxp.com>
> +
> +description:
> +  i.MX BLK_CTL is a conglomerate of different GPRs that are
> +  dedicated to a specific subsystem. It usually contains
> +  clocks and resets amongst other things. Here we take the clocks
> +  and resets as virtual PDs, the reason we could not take it as
> +  clock provider is there is A/B lock issue between power domain
> +  and clock.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - fsl,imx8mm-dispmix-blk-ctl
> +          - fsl,imx8mm-vpumix-blk-ctl
> +      - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#power-domain-cells":
> +    const: 1
> +
> +  power-domains:
> +    minItems: 1
> +    maxItems: 32
> +
> +  power-domain-names:
> +    minItems: 1
> +    maxItems: 32

Please describe why there's a range and we don't enumerate each entry.

> +
> +  '#reset-cells':
> +    const: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 32
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 32
> +
> +required:
> +  - compatible
> +  - reg
> +  - power-domains
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8mm-clock.h>
> +
> +    dispmix_blk_ctl: blk-ctl@32e28000 {
> +      compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
> +      reg = <0x32e28000 0x100>;
> +      #power-domain-cells = <1>;
> +      power-domains = <&pgc_dispmix>, <&pgc_mipi>;
> +      power-domain-names = "dispmix", "mipi";
> +      clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
> +               <&clk IMX8MM_CLK_DISP_APB_ROOT>;
> +      clock-names = "disp", "axi", "apb";
> +    };
> -- 
> 2.30.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/4] Documentation: bindings: clk: Add bindings for i.MX BLK_CTL

[Peng Fan (OSS)]

Hi Rob,

On 2021/5/6 7:08, Rob Herring wrote:
> On Fri, Apr 30, 2021 at 01:27:44PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> Document the i.MX BLK_CTL with its devicetree properties.
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>   .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     | 73 +++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
>> new file mode 100644
>> index 000000000000..a491b63de50c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
>> @@ -0,0 +1,73 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/imx/fsl,imx-blk-ctl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: NXP i.MX BLK_CTL
>> +
>> +maintainers:
>> +  - Peng Fan <peng.fan@nxp.com>
>> +
>> +description:
>> +  i.MX BLK_CTL is a conglomerate of different GPRs that are
>> +  dedicated to a specific subsystem. It usually contains
>> +  clocks and resets amongst other things. Here we take the clocks
>> +  and resets as virtual PDs, the reason we could not take it as
>> +  clock provider is there is A/B lock issue between power domain
>> +  and clock.
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - fsl,imx8mm-dispmix-blk-ctl
>> +          - fsl,imx8mm-vpumix-blk-ctl
>> +      - const: syscon
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#power-domain-cells":
>> +    const: 1
>> +
>> +  power-domains:
>> +    minItems: 1
>> +    maxItems: 32
>> +
>> +  power-domain-names:
>> +    minItems: 1
>> +    maxItems: 32
> 
> Please describe why there's a range and we don't enumerate each entry.

Each BLK-CTL have different input power domains, they
have different names. So it is hard to write down each
power domain for each BLK-CTL.

Same to below clocks and clock-names.

Thanks,
Peng.

> 
>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +  clocks:
>> +    minItems: 1
>> +    maxItems: 32
>> +
>> +  clock-names:
>> +    minItems: 1
>> +    maxItems: 32
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - power-domains
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/imx8mm-clock.h>
>> +
>> +    dispmix_blk_ctl: blk-ctl@32e28000 {
>> +      compatible = "fsl,imx8mm-dispmix-blk-ctl", "syscon";
>> +      reg = <0x32e28000 0x100>;
>> +      #power-domain-cells = <1>;
>> +      power-domains = <&pgc_dispmix>, <&pgc_mipi>;
>> +      power-domain-names = "dispmix", "mipi";
>> +      clocks = <&clk IMX8MM_CLK_DISP_ROOT>, <&clk IMX8MM_CLK_DISP_AXI_ROOT>,
>> +               <&clk IMX8MM_CLK_DISP_APB_ROOT>;
>> +      clock-names = "disp", "axi", "apb";
>> +    };
>> -- 
>> 2.30.0
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 3/4] soc: imx: Add generic blk-ctl driver

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
some GPRs.

The GPRs has some clock bits and reset bits, but here we take it
as virtual PDs, because of the clock and power domain A/B lock issue
when taking it as a clock controller.

For some bits, it might be good to also make it as a reset controller,
but to i.MX8MM, we not add that support for now.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/soc/imx/Makefile  |   2 +-
 drivers/soc/imx/blk-ctl.c | 303 ++++++++++++++++++++++++++++++++++++++
 drivers/soc/imx/blk-ctl.h |  76 ++++++++++
 3 files changed, 380 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/imx/blk-ctl.c
 create mode 100644 drivers/soc/imx/blk-ctl.h

diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index 078dc918f4f3..d3d2b49a386c 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
 endif
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
-obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
+obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c
new file mode 100644
index 000000000000..1f764dfd308d
--- /dev/null
+++ b/drivers/soc/imx/blk-ctl.c
@@ -0,0 +1,303 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021 NXP.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/pm_domain.h>
+#include <linux/reset-controller.h>
+
+#include "blk-ctl.h"
+
+static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct generic_pm_domain *genpd)
+{
+	return container_of(genpd, struct imx_blk_ctl_domain, pd);
+}
+
+static int imx_blk_ctl_enable_hsk(struct device *dev)
+{
+	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
+	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->hw_hsk;
+	struct regmap *regmap = blk_ctl->regmap;
+	int ret;
+
+
+	if (hw->flags & IMX_BLK_CTL_PD_RESET)
+		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
+
+	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
+
+	/* Wait for handshake */
+	udelay(5);
+
+	return ret;
+}
+
+int imx_blk_ctl_power_off(struct generic_pm_domain *domain)
+{
+	struct imx_blk_ctl_domain *pd;
+	struct imx_blk_ctl *blk_ctl;
+	const struct imx_blk_ctl_hw *hw;
+	struct regmap *regmap;
+	int ret;
+
+	pd = to_imx_blk_ctl_pd(domain);
+	blk_ctl = pd->blk_ctl;
+	regmap = blk_ctl->regmap;
+	hw = &blk_ctl->dev_data->pds[pd->id];
+
+	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(regmap, hw->offset, hw->mask, 0);
+	if (ret)
+		goto hsk_fail;
+
+	if (hw->flags & IMX_BLK_CTL_PD_RESET)
+		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);
+
+	if (atomic_dec_and_test(&blk_ctl->power_count)) {
+		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
+		if (ret) {
+			dev_err(blk_ctl->dev, "Hankshake fail\n");
+			goto hsk_fail;
+		}
+	}
+
+hsk_fail:
+	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
+
+	return ret;
+}
+
+int imx_blk_ctl_power_on(struct generic_pm_domain *domain)
+{
+	struct imx_blk_ctl_domain *pd;
+	struct regmap *regmap;
+	const struct imx_blk_ctl_hw *hw;
+	int ret;
+	struct imx_blk_ctl *blk_ctl;
+
+	pd = to_imx_blk_ctl_pd(domain);
+	blk_ctl = pd->blk_ctl;
+	regmap = blk_ctl->regmap;
+	hw = &blk_ctl->dev_data->pds[pd->id];
+
+	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
+	if (ret)
+		return ret;
+
+	if ((atomic_read(&blk_ctl->power_count) == 0)) {
+		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
+		if (ret) {
+			dev_err(blk_ctl->dev, "Hankshake fail\n");
+			goto disable_clk;
+		}
+	}
+
+	if (hw->flags & IMX_BLK_CTL_PD_RESET)
+		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);
+
+	/* Wait for reset propagate */
+	udelay(5);
+
+	if (hw->flags & IMX_BLK_CTL_PD_RESET)
+		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
+
+	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
+	if (ret)
+		goto disable_clk;
+
+	atomic_inc(&blk_ctl->power_count);
+
+disable_clk:
+	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
+
+	return ret;
+}
+
+static int imx_blk_ctl_attach_pd(struct device *dev, struct device **devs, char **pd_names,
+				 u32 num_pds)
+{
+	int i, ret;
+
+	if (!pd_names)
+		return 0;
+
+	if (dev->pm_domain) {
+		devs[0] = dev;
+		pm_runtime_enable(dev);
+		return 0;
+	}
+
+	for (i = 0; i < num_pds; i++) {
+		devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
+		if (IS_ERR_OR_NULL(devs[i])) {
+			ret = PTR_ERR(devs[i]) ? : -ENODATA;
+			goto detach_pm;
+		}
+	}
+
+	return 0;
+
+detach_pm:
+	for (i--; i >= 0; i--)
+		dev_pm_domain_detach(devs[i], false);
+
+	return ret;
+}
+
+static int imx_blk_ctl_register_pd(struct device *dev)
+{
+	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
+	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
+	int num = dev_data->pds_num;
+	struct imx_blk_ctl_domain *domain;
+	int i, ret;
+
+	blk_ctl->onecell_data.num_domains = num;
+	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num,
+						     sizeof(struct generic_pm_domain *),
+						     GFP_KERNEL);
+
+	if (!blk_ctl->onecell_data.domains)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++) {
+		domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
+		if (!domain) {
+			ret = -ENOMEM;
+			goto remove_genpd;
+		}
+		domain->pd.name = dev_data->pds[i].name;
+		domain->pd.power_off = imx_blk_ctl_power_off;
+		domain->pd.power_on = imx_blk_ctl_power_on;
+		domain->blk_ctl = blk_ctl;
+		domain->id = i;
+
+		ret = pm_genpd_init(&domain->pd, NULL, true);
+		if (ret)
+			return ret;
+
+		blk_ctl->onecell_data.domains[i] = &domain->pd;
+	}
+
+	return 0;
+
+remove_genpd:
+	for (i = i - 1; i >= 0; i--)
+		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
+
+	return ret;
+}
+
+static int imx_blk_ctl_hook_pd(struct device *dev)
+{
+	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
+	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
+	const struct imx_blk_ctl_hw *pds = dev_data->pds;
+	int num_active_pd = dev_data->num_active_pd;
+	int num = dev_data->pds_num;
+	struct generic_pm_domain *genpd, *child_genpd;
+	int ret;
+	int i, j;
+
+	blk_ctl->active_pds = devm_kcalloc(dev, num_active_pd, sizeof(struct device *), GFP_KERNEL);
+	if (!blk_ctl->active_pds)
+		return -ENOMEM;
+
+	ret = imx_blk_ctl_attach_pd(dev, blk_ctl->active_pds, dev_data->active_pd_names,
+				    num_active_pd);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			return ret;
+		dev_err(dev, "Failed to attach active pd: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < num; i++) {
+		for (j = 0; j < num_active_pd; j++) {
+			genpd = pd_to_genpd(blk_ctl->active_pds[j]->pm_domain);
+			if (!strcmp(genpd->name, pds[i].parent_name))
+				break;
+		}
+
+		child_genpd = blk_ctl->onecell_data.domains[i];
+		if (pm_genpd_add_subdomain(genpd, child_genpd))
+			pr_warn("failed to add subdomain:\n");
+	}
+
+	return 0;
+}
+
+int imx_blk_ctl_register(struct device *dev)
+{
+	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
+	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
+	int num = dev_data->pds_num;
+	int i, ret;
+
+	if (!blk_ctl)
+		return -ENODEV;
+
+	ret = imx_blk_ctl_register_pd(dev);
+	if (ret)
+		return ret;
+
+	ret = imx_blk_ctl_hook_pd(dev);
+	if (ret)
+		goto unregister_pd;
+
+	ret = of_genpd_add_provider_onecell(dev->of_node, &blk_ctl->onecell_data);
+	if (ret)
+		goto detach_pd;
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	pm_runtime_put(dev);
+
+	return 0;
+
+detach_pd:
+	for (i = blk_ctl->dev_data->num_active_pd; i >= 0; i--)
+		dev_pm_domain_detach(blk_ctl->active_pds[i], false);
+unregister_pd:
+	for (i = num - 1; i >= 0; i--)
+		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
+
+static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int __maybe_unused imx_blk_ctl_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+const struct dev_pm_ops imx_blk_ctl_pm_ops = {
+	SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend,
+			   imx_blk_ctl_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+			   pm_runtime_force_resume)
+};
+EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h
new file mode 100644
index 000000000000..e736369406a1
--- /dev/null
+++ b/drivers/soc/imx/blk-ctl.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SOC_IMX_BLK_CTL_H
+#define __SOC_IMX_BLK_CTL_H
+
+enum imx_blk_ctl_pd_type {
+	BLK_CTL_PD,
+};
+
+struct imx_blk_ctl_hw {
+	int type;
+	char *name;
+	char *parent_name;
+	u32 offset;
+	u32 mask;
+	u32 flags;
+	u32 id;
+	u32 rst_offset;
+	u32 rst_mask;
+};
+
+struct imx_blk_ctl_domain {
+	struct generic_pm_domain pd;
+	struct imx_blk_ctl *blk_ctl;
+	u32 id;
+};
+
+struct imx_blk_ctl_dev_data {
+	struct regmap_config config;
+	struct imx_blk_ctl_hw *pds;
+	struct imx_blk_ctl_hw hw_hsk;
+	u32 pds_num;
+	char **active_pd_names;
+	u32 num_active_pd;
+};
+
+struct imx_blk_ctl {
+	struct device *dev;
+	struct regmap *regmap;
+	struct device **active_pds;
+	u32 pds_num;
+	u32 active_pd_count;
+	struct genpd_onecell_data onecell_data;
+	const struct imx_blk_ctl_dev_data *dev_data;
+	struct clk_bulk_data *clks;
+	u32 num_clks;
+
+	atomic_t power_count;
+};
+
+#define IMX_BLK_CTL(_type, _name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask,	\
+		    _flags)								\
+	{										\
+		.type = _type,								\
+		.name = _name,								\
+		.parent_name = _parent_name,						\
+		.id = _id,								\
+		.offset = _offset,							\
+		.mask = _mask,								\
+		.flags = _flags,							\
+		.rst_offset = _rst_offset,						\
+		.rst_mask = _rst_mask,							\
+	}
+
+#define IMX_BLK_CTL_PD(_name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask, _flags) \
+	IMX_BLK_CTL(BLK_CTL_PD, _name, _parent_name, _id, _offset, _mask, _rst_offset,		\
+		    _rst_mask, _flags)
+
+int imx_blk_ctl_register(struct device *dev);
+
+#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
+#define IMX_BLK_CTL_PD_RESET		BIT(1)
+#define IMX_BLK_CTL_PD_BUS		BIT(2)
+
+const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
+
+#endif
-- 
2.30.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 3/4] soc: imx: Add generic blk-ctl driver

[Lucas Stach]

Am Freitag, dem 30.04.2021 um 13:27 +0800 schrieb Peng Fan (OSS):
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
> some GPRs.
> 
> The GPRs has some clock bits and reset bits, but here we take it
> as virtual PDs, because of the clock and power domain A/B lock issue
> when taking it as a clock controller.
> 
> For some bits, it might be good to also make it as a reset controller,
> but to i.MX8MM, we not add that support for now.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/soc/imx/Makefile  |   2 +-
>  drivers/soc/imx/blk-ctl.c | 303 ++++++++++++++++++++++++++++++++++++++
>  drivers/soc/imx/blk-ctl.h |  76 ++++++++++
>  3 files changed, 380 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soc/imx/blk-ctl.c
>  create mode 100644 drivers/soc/imx/blk-ctl.h
> 
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> index 078dc918f4f3..d3d2b49a386c 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
>  endif
>  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c
> new file mode 100644
> index 000000000000..1f764dfd308d
> --- /dev/null
> +++ b/drivers/soc/imx/blk-ctl.c
> @@ -0,0 +1,303 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2021 NXP.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/pm_domain.h>
> +#include <linux/reset-controller.h>
> +
> +#include "blk-ctl.h"
> +
> +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct generic_pm_domain *genpd)
> +{
> +	return container_of(genpd, struct imx_blk_ctl_domain, pd);
> +}
> +
> +static int imx_blk_ctl_enable_hsk(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->hw_hsk;
> +	struct regmap *regmap = blk_ctl->regmap;
> +	int ret;
> +
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> +
> +	/* Wait for handshake */
> +	udelay(5);
> +
> +	return ret;
> +}
> +
> +int imx_blk_ctl_power_off(struct generic_pm_domain *domain)
> +{
> +	struct imx_blk_ctl_domain *pd;
> +	struct imx_blk_ctl *blk_ctl;
> +	const struct imx_blk_ctl_hw *hw;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	pd = to_imx_blk_ctl_pd(domain);
> +	blk_ctl = pd->blk_ctl;
> +	regmap = blk_ctl->regmap;
> +	hw = &blk_ctl->dev_data->pds[pd->id];
> +
> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> +	if (ret)
> +		return ret;

This looks a bit strange to me. Does that mean that you keep all the
CCM clocks going into the blkctl enabled as long as one of the blkctl
virtual power domains is up? I would have expected each virtual PD to
have a list of clock names that need to be enabled, to make this more
selective.

I haven't completely thought it through if this is a problem, but I
could see some CCM PLLs running without a reason if all CCM clocks are
kept enabled. This surely is something we want to avoid from a pwoer
consumption PoV.

> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, 0);
> +	if (ret)
> +		goto hsk_fail;
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);
> +
> +	if (atomic_dec_and_test(&blk_ctl->power_count)) {
> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> +		if (ret) {
> +			dev_err(blk_ctl->dev, "Hankshake fail\n");
> +			goto hsk_fail;
> +		}
> +	}
> +
> +hsk_fail:
> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> +
> +	return ret;
> +}
> +
> +int imx_blk_ctl_power_on(struct generic_pm_domain *domain)
> +{
> +	struct imx_blk_ctl_domain *pd;
> +	struct regmap *regmap;
> +	const struct imx_blk_ctl_hw *hw;
> +	int ret;
> +	struct imx_blk_ctl *blk_ctl;
> +
> +	pd = to_imx_blk_ctl_pd(domain);
> +	blk_ctl = pd->blk_ctl;
> +	regmap = blk_ctl->regmap;
> +	hw = &blk_ctl->dev_data->pds[pd->id];
> +
> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> +	if (ret)
> +		return ret;
> +
> +	if ((atomic_read(&blk_ctl->power_count) == 0)) {
> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> +		if (ret) {
> +			dev_err(blk_ctl->dev, "Hankshake fail\n");
> +			goto disable_clk;
> +		}
> +	}

This is bogus. The variable isn't used as a atomic, if at all this
should use atomic_inc_return, but I think even that isn't correct in
that case. imx_blk_ctl_enable_hsk() includes a wait time, so if two
paths call this function at the same time, the second caller would be
able to overtake the first one, which is still waiting for the
handshake to complete, effectively skipping the handshake. This needs a
proper lock, as this function isn't called from a performance critical
path it makes no sense to try to optimize this with an atomic counter,
rather than going with a mutex.

> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);
> +
> +	/* Wait for reset propagate */
> +	udelay(5);
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> +	if (ret)
> +		goto disable_clk;
> +
> +	atomic_inc(&blk_ctl->power_count);
> +
> +disable_clk:
> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_attach_pd(struct device *dev, struct device **devs, char **pd_names,
> +				 u32 num_pds)
> +{
> +	int i, ret;
> +
> +	if (!pd_names)
> +		return 0;
> +
> +	if (dev->pm_domain) {
> +		devs[0] = dev;
> +		pm_runtime_enable(dev);
> +		return 0;
> +	}
> +
> +	for (i = 0; i < num_pds; i++) {
> +		devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
> +		if (IS_ERR_OR_NULL(devs[i])) {
> +			ret = PTR_ERR(devs[i]) ? : -ENODATA;
> +			goto detach_pm;
> +		}
> +	}
> +
> +	return 0;
> +
> +detach_pm:
> +	for (i--; i >= 0; i--)
> +		dev_pm_domain_detach(devs[i], false);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_register_pd(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> +	int num = dev_data->pds_num;
> +	struct imx_blk_ctl_domain *domain;
> +	int i, ret;
> +
> +	blk_ctl->onecell_data.num_domains = num;
> +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num,
> +						     sizeof(struct generic_pm_domain *),
> +						     GFP_KERNEL);
> +
> +	if (!blk_ctl->onecell_data.domains)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num; i++) {
> +		domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
> +		if (!domain) {
> +			ret = -ENOMEM;
> +			goto remove_genpd;
> +		}
> +		domain->pd.name = dev_data->pds[i].name;
> +		domain->pd.power_off = imx_blk_ctl_power_off;
> +		domain->pd.power_on = imx_blk_ctl_power_on;
> +		domain->blk_ctl = blk_ctl;
> +		domain->id = i;
> +
> +		ret = pm_genpd_init(&domain->pd, NULL, true);
> +		if (ret)
> +			return ret;
> +
> +		blk_ctl->onecell_data.domains[i] = &domain->pd;
> +	}
> +
> +	return 0;
> +
> +remove_genpd:
> +	for (i = i - 1; i >= 0; i--)
> +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_hook_pd(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> +	const struct imx_blk_ctl_hw *pds = dev_data->pds;
> +	int num_active_pd = dev_data->num_active_pd;
> +	int num = dev_data->pds_num;
> +	struct generic_pm_domain *genpd, *child_genpd;
> +	int ret;
> +	int i, j;
> +
> +	blk_ctl->active_pds = devm_kcalloc(dev, num_active_pd, sizeof(struct device *), GFP_KERNEL);
> +	if (!blk_ctl->active_pds)
> +		return -ENOMEM;
> +
> +	ret = imx_blk_ctl_attach_pd(dev, blk_ctl->active_pds, dev_data->active_pd_names,
> +				    num_active_pd);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +		dev_err(dev, "Failed to attach active pd: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		for (j = 0; j < num_active_pd; j++) {
> +			genpd = pd_to_genpd(blk_ctl->active_pds[j]->pm_domain);
> +			if (!strcmp(genpd->name, pds[i].parent_name))
> +				break;
> +		}
> +
> +		child_genpd = blk_ctl->onecell_data.domains[i];
> +		if (pm_genpd_add_subdomain(genpd, child_genpd))
> +			pr_warn("failed to add subdomain:\n");
> +	}
> +
> +	return 0;
> +}
> +
> +int imx_blk_ctl_register(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> +	int num = dev_data->pds_num;
> +	int i, ret;
> +
> +	if (!blk_ctl)
> +		return -ENODEV;
> +
> +	ret = imx_blk_ctl_register_pd(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx_blk_ctl_hook_pd(dev);
> +	if (ret)
> +		goto unregister_pd;
> +
> +	ret = of_genpd_add_provider_onecell(dev->of_node, &blk_ctl->onecell_data);
> +	if (ret)
> +		goto detach_pd;
> +
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +
> +detach_pd:
> +	for (i = blk_ctl->dev_data->num_active_pd; i >= 0; i--)
> +		dev_pm_domain_detach(blk_ctl->active_pds[i], false);
> +unregister_pd:
> +	for (i = num - 1; i >= 0; i--)
> +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
> +
> +static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int __maybe_unused imx_blk_ctl_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}

Why those empty stubs? If you don't need to do anything for
suspend/resume, just don't add the functions.

> +
> +const struct dev_pm_ops imx_blk_ctl_pm_ops = {
> +	SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend,
> +			   imx_blk_ctl_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +			   pm_runtime_force_resume)
> +};
> +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
> diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h
> new file mode 100644
> index 000000000000..e736369406a1
> --- /dev/null
> +++ b/drivers/soc/imx/blk-ctl.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_IMX_BLK_CTL_H
> +#define __SOC_IMX_BLK_CTL_H
> +
> +enum imx_blk_ctl_pd_type {
> +	BLK_CTL_PD,
> +};
> +
> +struct imx_blk_ctl_hw {
> +	int type;
> +	char *name;
> +	char *parent_name;
> +	u32 offset;
> +	u32 mask;
> +	u32 flags;
> +	u32 id;
> +	u32 rst_offset;
> +	u32 rst_mask;
> +};
> +
> +struct imx_blk_ctl_domain {
> +	struct generic_pm_domain pd;
> +	struct imx_blk_ctl *blk_ctl;
> +	u32 id;
> +};
> +
> +struct imx_blk_ctl_dev_data {
> +	struct regmap_config config;
> +	struct imx_blk_ctl_hw *pds;
> +	struct imx_blk_ctl_hw hw_hsk;
> +	u32 pds_num;
> +	char **active_pd_names;
> +	u32 num_active_pd;
> +};
> +
> +struct imx_blk_ctl {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct device **active_pds;
> +	u32 pds_num;
> +	u32 active_pd_count;
> +	struct genpd_onecell_data onecell_data;
> +	const struct imx_blk_ctl_dev_data *dev_data;
> +	struct clk_bulk_data *clks;
> +	u32 num_clks;
> +
> +	atomic_t power_count;
> +};
> +
> +#define IMX_BLK_CTL(_type, _name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask,	\
> +		    _flags)								\
> +	{										\
> +		.type = _type,								\
> +		.name = _name,								\
> +		.parent_name = _parent_name,						\
> +		.id = _id,								\
> +		.offset = _offset,							\
> +		.mask = _mask,								\
> +		.flags = _flags,							\
> +		.rst_offset = _rst_offset,						\
> +		.rst_mask = _rst_mask,							\
> +	}
> +
> +#define IMX_BLK_CTL_PD(_name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask, _flags) \
> +	IMX_BLK_CTL(BLK_CTL_PD, _name, _parent_name, _id, _offset, _mask, _rst_offset,		\
> +		    _rst_mask, _flags)
> +
> +int imx_blk_ctl_register(struct device *dev);
> +
> +#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
> +#define IMX_BLK_CTL_PD_RESET		BIT(1)
> +#define IMX_BLK_CTL_PD_BUS		BIT(2)
> +
> +const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
> +
> +#endif



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 3/4] soc: imx: Add generic blk-ctl driver

[Peng Fan (OSS)]

On 2021/5/4 17:47, Lucas Stach wrote:
> Am Freitag, dem 30.04.2021 um 13:27 +0800 schrieb Peng Fan (OSS):
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
>> some GPRs.
>>
>> The GPRs has some clock bits and reset bits, but here we take it
>> as virtual PDs, because of the clock and power domain A/B lock issue
>> when taking it as a clock controller.
>>
>> For some bits, it might be good to also make it as a reset controller,
>> but to i.MX8MM, we not add that support for now.
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>   drivers/soc/imx/Makefile  |   2 +-
>>   drivers/soc/imx/blk-ctl.c | 303 ++++++++++++++++++++++++++++++++++++++
>>   drivers/soc/imx/blk-ctl.h |  76 ++++++++++
>>   3 files changed, 380 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/soc/imx/blk-ctl.c
>>   create mode 100644 drivers/soc/imx/blk-ctl.h
>>
>> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
>> index 078dc918f4f3..d3d2b49a386c 100644
>> --- a/drivers/soc/imx/Makefile
>> +++ b/drivers/soc/imx/Makefile
>> @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
>>   endif
>>   obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>>   obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
>> -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
>> +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
>> diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c
>> new file mode 100644
>> index 000000000000..1f764dfd308d
>> --- /dev/null
>> +++ b/drivers/soc/imx/blk-ctl.c
>> @@ -0,0 +1,303 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2021 NXP.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/reset-controller.h>
>> +
>> +#include "blk-ctl.h"
>> +
>> +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct generic_pm_domain *genpd)
>> +{
>> +	return container_of(genpd, struct imx_blk_ctl_domain, pd);
>> +}
>> +
>> +static int imx_blk_ctl_enable_hsk(struct device *dev)
>> +{
>> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
>> +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->hw_hsk;
>> +	struct regmap *regmap = blk_ctl->regmap;
>> +	int ret;
>> +
>> +
>> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
>> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
>> +
>> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
>> +
>> +	/* Wait for handshake */
>> +	udelay(5);
>> +
>> +	return ret;
>> +}
>> +
>> +int imx_blk_ctl_power_off(struct generic_pm_domain *domain)
>> +{
>> +	struct imx_blk_ctl_domain *pd;
>> +	struct imx_blk_ctl *blk_ctl;
>> +	const struct imx_blk_ctl_hw *hw;
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	pd = to_imx_blk_ctl_pd(domain);
>> +	blk_ctl = pd->blk_ctl;
>> +	regmap = blk_ctl->regmap;
>> +	hw = &blk_ctl->dev_data->pds[pd->id];
>> +
>> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
>> +	if (ret)
>> +		return ret;
> 
> This looks a bit strange to me. Does that mean that you keep all the
> CCM clocks going into the blkctl enabled as long as one of the blkctl
> virtual power domains is up?

No. The clocks are disable unprepared when this function finish.
The clocks are only used for setting blk ctl bits and handshake.

  I would have expected each virtual PD to
> have a list of clock names that need to be enabled, to make this more
> selective.
> 
> I haven't completely thought it through if this is a problem, but I
> could see some CCM PLLs running without a reason if all CCM clocks are
> kept enabled. This surely is something we want to avoid from a pwoer
> consumption PoV.

I thought to add selective clocks for each virtual PD, but that makes
the design completed, and the clocks(gate) are only enabled for a little
while, so I think compared with the software complexity, the little
benifit of power could be ignored?

> 
>> +
>> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, 0);
>> +	if (ret)
>> +		goto hsk_fail;
>> +
>> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
>> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);
>> +
>> +	if (atomic_dec_and_test(&blk_ctl->power_count)) {
>> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
>> +		if (ret) {
>> +			dev_err(blk_ctl->dev, "Hankshake fail\n");
>> +			goto hsk_fail;
>> +		}
>> +	}
>> +
>> +hsk_fail:
>> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
>> +
>> +	return ret;
>> +}
>> +
>> +int imx_blk_ctl_power_on(struct generic_pm_domain *domain)
>> +{
>> +	struct imx_blk_ctl_domain *pd;
>> +	struct regmap *regmap;
>> +	const struct imx_blk_ctl_hw *hw;
>> +	int ret;
>> +	struct imx_blk_ctl *blk_ctl;
>> +
>> +	pd = to_imx_blk_ctl_pd(domain);
>> +	blk_ctl = pd->blk_ctl;
>> +	regmap = blk_ctl->regmap;
>> +	hw = &blk_ctl->dev_data->pds[pd->id];
>> +
>> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if ((atomic_read(&blk_ctl->power_count) == 0)) {
>> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
>> +		if (ret) {
>> +			dev_err(blk_ctl->dev, "Hankshake fail\n");
>> +			goto disable_clk;
>> +		}
>> +	}
> 
> This is bogus. The variable isn't used as a atomic, if at all this
> should use atomic_inc_return, but I think even that isn't correct in
> that case. imx_blk_ctl_enable_hsk() includes a wait time, so if two
> paths call this function at the same time, the second caller would be
> able to overtake the first one, which is still waiting for the
> handshake to complete, effectively skipping the handshake. This needs a
> proper lock, as this function isn't called from a performance critical
> path it makes no sense to try to optimize this with an atomic counter,
> rather than going with a mutex.

I see. Will use a mutex here.

> 
>> +
>> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
>> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);
>> +
>> +	/* Wait for reset propagate */
>> +	udelay(5);
>> +
>> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
>> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);
>> +
>> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
>> +	if (ret)
>> +		goto disable_clk;
>> +
>> +	atomic_inc(&blk_ctl->power_count);
>> +
>> +disable_clk:
>> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
>> +
>> +	return ret;
>> +}
>> +
>> +static int imx_blk_ctl_attach_pd(struct device *dev, struct device **devs, char **pd_names,
>> +				 u32 num_pds)
>> +{
>> +	int i, ret;
>> +
>> +	if (!pd_names)
>> +		return 0;
>> +
>> +	if (dev->pm_domain) {
>> +		devs[0] = dev;
>> +		pm_runtime_enable(dev);
>> +		return 0;
>> +	}
>> +
>> +	for (i = 0; i < num_pds; i++) {
>> +		devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
>> +		if (IS_ERR_OR_NULL(devs[i])) {
>> +			ret = PTR_ERR(devs[i]) ? : -ENODATA;
>> +			goto detach_pm;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +detach_pm:
>> +	for (i--; i >= 0; i--)
>> +		dev_pm_domain_detach(devs[i], false);
>> +
>> +	return ret;
>> +}
>> +
>> +static int imx_blk_ctl_register_pd(struct device *dev)
>> +{
>> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
>> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
>> +	int num = dev_data->pds_num;
>> +	struct imx_blk_ctl_domain *domain;
>> +	int i, ret;
>> +
>> +	blk_ctl->onecell_data.num_domains = num;
>> +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num,
>> +						     sizeof(struct generic_pm_domain *),
>> +						     GFP_KERNEL);
>> +
>> +	if (!blk_ctl->onecell_data.domains)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < num; i++) {
>> +		domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
>> +		if (!domain) {
>> +			ret = -ENOMEM;
>> +			goto remove_genpd;
>> +		}
>> +		domain->pd.name = dev_data->pds[i].name;
>> +		domain->pd.power_off = imx_blk_ctl_power_off;
>> +		domain->pd.power_on = imx_blk_ctl_power_on;
>> +		domain->blk_ctl = blk_ctl;
>> +		domain->id = i;
>> +
>> +		ret = pm_genpd_init(&domain->pd, NULL, true);
>> +		if (ret)
>> +			return ret;
>> +
>> +		blk_ctl->onecell_data.domains[i] = &domain->pd;
>> +	}
>> +
>> +	return 0;
>> +
>> +remove_genpd:
>> +	for (i = i - 1; i >= 0; i--)
>> +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
>> +
>> +	return ret;
>> +}
>> +
>> +static int imx_blk_ctl_hook_pd(struct device *dev)
>> +{
>> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
>> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
>> +	const struct imx_blk_ctl_hw *pds = dev_data->pds;
>> +	int num_active_pd = dev_data->num_active_pd;
>> +	int num = dev_data->pds_num;
>> +	struct generic_pm_domain *genpd, *child_genpd;
>> +	int ret;
>> +	int i, j;
>> +
>> +	blk_ctl->active_pds = devm_kcalloc(dev, num_active_pd, sizeof(struct device *), GFP_KERNEL);
>> +	if (!blk_ctl->active_pds)
>> +		return -ENOMEM;
>> +
>> +	ret = imx_blk_ctl_attach_pd(dev, blk_ctl->active_pds, dev_data->active_pd_names,
>> +				    num_active_pd);
>> +	if (ret) {
>> +		if (ret == -EPROBE_DEFER)
>> +			return ret;
>> +		dev_err(dev, "Failed to attach active pd: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < num; i++) {
>> +		for (j = 0; j < num_active_pd; j++) {
>> +			genpd = pd_to_genpd(blk_ctl->active_pds[j]->pm_domain);
>> +			if (!strcmp(genpd->name, pds[i].parent_name))
>> +				break;
>> +		}
>> +
>> +		child_genpd = blk_ctl->onecell_data.domains[i];
>> +		if (pm_genpd_add_subdomain(genpd, child_genpd))
>> +			pr_warn("failed to add subdomain:\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int imx_blk_ctl_register(struct device *dev)
>> +{
>> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
>> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
>> +	int num = dev_data->pds_num;
>> +	int i, ret;
>> +
>> +	if (!blk_ctl)
>> +		return -ENODEV;
>> +
>> +	ret = imx_blk_ctl_register_pd(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = imx_blk_ctl_hook_pd(dev);
>> +	if (ret)
>> +		goto unregister_pd;
>> +
>> +	ret = of_genpd_add_provider_onecell(dev->of_node, &blk_ctl->onecell_data);
>> +	if (ret)
>> +		goto detach_pd;
>> +
>> +	pm_runtime_get_noresume(dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +
>> +	pm_runtime_put(dev);
>> +
>> +	return 0;
>> +
>> +detach_pd:
>> +	for (i = blk_ctl->dev_data->num_active_pd; i >= 0; i--)
>> +		dev_pm_domain_detach(blk_ctl->active_pds[i], false);
>> +unregister_pd:
>> +	for (i = num - 1; i >= 0; i--)
>> +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
>> +
>> +static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device *dev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused imx_blk_ctl_runtime_resume(struct device *dev)
>> +{
>> +	return 0;
>> +}
> 
> Why those empty stubs? If you don't need to do anything for
> suspend/resume, just don't add the functions.

ok, will drop in V3.

Thanks,
Peng.

> 
>> +
>> +const struct dev_pm_ops imx_blk_ctl_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend,
>> +			   imx_blk_ctl_runtime_resume, NULL)
>> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +			   pm_runtime_force_resume)
>> +};
>> +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
>> diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h
>> new file mode 100644
>> index 000000000000..e736369406a1
>> --- /dev/null
>> +++ b/drivers/soc/imx/blk-ctl.h
>> @@ -0,0 +1,76 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __SOC_IMX_BLK_CTL_H
>> +#define __SOC_IMX_BLK_CTL_H
>> +
>> +enum imx_blk_ctl_pd_type {
>> +	BLK_CTL_PD,
>> +};
>> +
>> +struct imx_blk_ctl_hw {
>> +	int type;
>> +	char *name;
>> +	char *parent_name;
>> +	u32 offset;
>> +	u32 mask;
>> +	u32 flags;
>> +	u32 id;
>> +	u32 rst_offset;
>> +	u32 rst_mask;
>> +};
>> +
>> +struct imx_blk_ctl_domain {
>> +	struct generic_pm_domain pd;
>> +	struct imx_blk_ctl *blk_ctl;
>> +	u32 id;
>> +};
>> +
>> +struct imx_blk_ctl_dev_data {
>> +	struct regmap_config config;
>> +	struct imx_blk_ctl_hw *pds;
>> +	struct imx_blk_ctl_hw hw_hsk;
>> +	u32 pds_num;
>> +	char **active_pd_names;
>> +	u32 num_active_pd;
>> +};
>> +
>> +struct imx_blk_ctl {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct device **active_pds;
>> +	u32 pds_num;
>> +	u32 active_pd_count;
>> +	struct genpd_onecell_data onecell_data;
>> +	const struct imx_blk_ctl_dev_data *dev_data;
>> +	struct clk_bulk_data *clks;
>> +	u32 num_clks;
>> +
>> +	atomic_t power_count;
>> +};
>> +
>> +#define IMX_BLK_CTL(_type, _name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask,	\
>> +		    _flags)								\
>> +	{										\
>> +		.type = _type,								\
>> +		.name = _name,								\
>> +		.parent_name = _parent_name,						\
>> +		.id = _id,								\
>> +		.offset = _offset,							\
>> +		.mask = _mask,								\
>> +		.flags = _flags,							\
>> +		.rst_offset = _rst_offset,						\
>> +		.rst_mask = _rst_mask,							\
>> +	}
>> +
>> +#define IMX_BLK_CTL_PD(_name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask, _flags) \
>> +	IMX_BLK_CTL(BLK_CTL_PD, _name, _parent_name, _id, _offset, _mask, _rst_offset,		\
>> +		    _rst_mask, _flags)
>> +
>> +int imx_blk_ctl_register(struct device *dev);
>> +
>> +#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
>> +#define IMX_BLK_CTL_PD_RESET		BIT(1)
>> +#define IMX_BLK_CTL_PD_BUS		BIT(2)
>> +
>> +const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
>> +
>> +#endif
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 3/4] soc: imx: Add generic blk-ctl driver

[Frieder Schrempf]

On 30.04.21 07:27, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
> some GPRs.
> 
> The GPRs has some clock bits and reset bits, but here we take it
> as virtual PDs, because of the clock and power domain A/B lock issue
> when taking it as a clock controller.
> 
> For some bits, it might be good to also make it as a reset controller,
> but to i.MX8MM, we not add that support for now.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   drivers/soc/imx/Makefile  |   2 +-
>   drivers/soc/imx/blk-ctl.c | 303 ++++++++++++++++++++++++++++++++++++++
>   drivers/soc/imx/blk-ctl.h |  76 ++++++++++
>   3 files changed, 380 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/soc/imx/blk-ctl.c
>   create mode 100644 drivers/soc/imx/blk-ctl.h
> 
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> index 078dc918f4f3..d3d2b49a386c 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
>   endif
>   obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>   obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c
> new file mode 100644
> index 000000000000..1f764dfd308d
> --- /dev/null
> +++ b/drivers/soc/imx/blk-ctl.c
> @@ -0,0 +1,303 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2021 NXP.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/pm_domain.h>
> +#include <linux/reset-controller.h>
> +
> +#include "blk-ctl.h"
> +
> +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct generic_pm_domain *genpd)
> +{
> +	return container_of(genpd, struct imx_blk_ctl_domain, pd);
> +}
> +
> +static int imx_blk_ctl_enable_hsk(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->hw_hsk;
> +	struct regmap *regmap = blk_ctl->regmap;
> +	int ret;
> +
> +

Only one blank line here.

> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);

The return value above gets discarded.

> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> +
> +	/* Wait for handshake */
> +	udelay(5);
> +
> +	return ret;
> +}
> +
> +int imx_blk_ctl_power_off(struct generic_pm_domain *domain)
> +{
> +	struct imx_blk_ctl_domain *pd;
> +	struct imx_blk_ctl *blk_ctl;
> +	const struct imx_blk_ctl_hw *hw;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	pd = to_imx_blk_ctl_pd(domain);
> +	blk_ctl = pd->blk_ctl;
> +	regmap = blk_ctl->regmap;
> +	hw = &blk_ctl->dev_data->pds[pd->id];

You could include the assignments above in the declarations.

> +
> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, 0);

You could use regmap_clear_bits() here.

> +	if (ret)
> +		goto hsk_fail;
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);

You could use regmap_clear_bits() here.
And the return value of regmap_update_bits() potentially gets discarded.

> +
> +	if (atomic_dec_and_test(&blk_ctl->power_count)) {
> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> +		if (ret) {
> +			dev_err(blk_ctl->dev, "Hankshake fail\n");

s/Hankshake fail/Handshake failed/

> +			goto hsk_fail;

This goto is redundant.

> +		}
> +	}
> +
> +hsk_fail:
> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> +
> +	return ret;
> +}
> +
> +int imx_blk_ctl_power_on(struct generic_pm_domain *domain)
> +{
> +	struct imx_blk_ctl_domain *pd;
> +	struct regmap *regmap;
> +	const struct imx_blk_ctl_hw *hw;
> +	int ret;
> +	struct imx_blk_ctl *blk_ctl;
> +
> +	pd = to_imx_blk_ctl_pd(domain);
> +	blk_ctl = pd->blk_ctl;
> +	regmap = blk_ctl->regmap;
> +	hw = &blk_ctl->dev_data->pds[pd->id];

You could include the assignments above in the declarations.

> +
> +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> +	if (ret)
> +		return ret;
> +
> +	if ((atomic_read(&blk_ctl->power_count) == 0)) {
> +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> +		if (ret) {
> +			dev_err(blk_ctl->dev, "Hankshake fail\n");

s/Hankshake fail/Handshake failed/

> +			goto disable_clk;
> +		}
> +	}
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, 0);

You could use regmap_clear_bits() here.
And the return value of regmap_update_bits() gets discarded.

> +
> +	/* Wait for reset propagate */
> +	udelay(5);
> +
> +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask, hw->rst_mask);

The return value above gets discarded.

> +
> +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> +	if (ret)
> +		goto disable_clk;
> +
> +	atomic_inc(&blk_ctl->power_count);
> +
> +disable_clk:
> +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_attach_pd(struct device *dev, struct device **devs, char **pd_names,
> +				 u32 num_pds)
> +{
> +	int i, ret;
> +
> +	if (!pd_names)
> +		return 0;
> +
> +	if (dev->pm_domain) {
> +		devs[0] = dev;
> +		pm_runtime_enable(dev);
> +		return 0;
> +	}
> +
> +	for (i = 0; i < num_pds; i++) {
> +		devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
> +		if (IS_ERR_OR_NULL(devs[i])) {
> +			ret = PTR_ERR(devs[i]) ? : -ENODATA;
> +			goto detach_pm;
> +		}
> +	}
> +
> +	return 0;
> +
> +detach_pm:
> +	for (i--; i >= 0; i--)
> +		dev_pm_domain_detach(devs[i], false);

It looks like you should add pm_runtime_disable() in this error path to 
not leave the pm_runtime_enable() unmatched.

> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_register_pd(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> +	int num = dev_data->pds_num;
> +	struct imx_blk_ctl_domain *domain;
> +	int i, ret;
> +
> +	blk_ctl->onecell_data.num_domains = num;
> +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num,
> +						     sizeof(struct generic_pm_domain *),
> +						     GFP_KERNEL);
> +
> +	if (!blk_ctl->onecell_data.domains)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num; i++) {
> +		domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
> +		if (!domain) {
> +			ret = -ENOMEM;
> +			goto remove_genpd;
> +		}
> +		domain->pd.name = dev_data->pds[i].name;
> +		domain->pd.power_off = imx_blk_ctl_power_off;
> +		domain->pd.power_on = imx_blk_ctl_power_on;
> +		domain->blk_ctl = blk_ctl;
> +		domain->id = i;
> +
> +		ret = pm_genpd_init(&domain->pd, NULL, true);
> +		if (ret)
> +			return ret;

Looks like you should use the error path here and "goto remove_genpd" 
instead of return.

> +
> +		blk_ctl->onecell_data.domains[i] = &domain->pd;
> +	}
> +
> +	return 0;
> +
> +remove_genpd:
> +	for (i = i - 1; i >= 0; i--)
> +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
> +
> +	return ret;
> +}
> +
> +static int imx_blk_ctl_hook_pd(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> +	const struct imx_blk_ctl_hw *pds = dev_data->pds;
> +	int num_active_pd = dev_data->num_active_pd;
> +	int num = dev_data->pds_num;
> +	struct generic_pm_domain *genpd, *child_genpd;
> +	int ret;
> +	int i, j;
> +
> +	blk_ctl->active_pds = devm_kcalloc(dev, num_active_pd, sizeof(struct device *), GFP_KERNEL);
> +	if (!blk_ctl->active_pds)
> +		return -ENOMEM;
> +
> +	ret = imx_blk_ctl_attach_pd(dev, blk_ctl->active_pds, dev_data->active_pd_names,
> +				    num_active_pd);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +		dev_err(dev, "Failed to attach active pd: %d\n", ret);
> +		return ret;

I think it would be better to do it the other way round:

                 if (ret != -EPROBE_DEFER)
	            dev_err(dev, "Failed to attach active pd: %d\n", ret);
                 return ret;

> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		for (j = 0; j < num_active_pd; j++) {
> +			genpd = pd_to_genpd(blk_ctl->active_pds[j]->pm_domain);
> +			if (!strcmp(genpd->name, pds[i].parent_name))
> +				break;
> +		}
> +
> +		child_genpd = blk_ctl->onecell_data.domains[i];
> +		if (pm_genpd_add_subdomain(genpd, child_genpd))
> +			pr_warn("failed to add subdomain:\n");

Remove the colon add the end of the warning message or add something 
after it.

> +	}
> +
> +	return 0;
> +}
> +
> +int imx_blk_ctl_register(struct device *dev)
> +{
> +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> +	int num = dev_data->pds_num;
> +	int i, ret;
> +
> +	if (!blk_ctl)
> +		return -ENODEV;
> +
> +	ret = imx_blk_ctl_register_pd(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx_blk_ctl_hook_pd(dev);
> +	if (ret)
> +		goto unregister_pd;
> +
> +	ret = of_genpd_add_provider_onecell(dev->of_node, &blk_ctl->onecell_data);
> +	if (ret)
> +		goto detach_pd;
> +
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +
> +detach_pd:
> +	for (i = blk_ctl->dev_data->num_active_pd; i >= 0; i--)
> +		dev_pm_domain_detach(blk_ctl->active_pds[i], false);
> +unregister_pd:
> +	for (i = num - 1; i >= 0; i--)
> +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
> +
> +static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int __maybe_unused imx_blk_ctl_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +const struct dev_pm_ops imx_blk_ctl_pm_ops = {
> +	SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend,
> +			   imx_blk_ctl_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +			   pm_runtime_force_resume)
> +};
> +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
> diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h
> new file mode 100644
> index 000000000000..e736369406a1
> --- /dev/null
> +++ b/drivers/soc/imx/blk-ctl.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_IMX_BLK_CTL_H
> +#define __SOC_IMX_BLK_CTL_H
> +
> +enum imx_blk_ctl_pd_type {
> +	BLK_CTL_PD,
> +};
> +
> +struct imx_blk_ctl_hw {
> +	int type;
> +	char *name;
> +	char *parent_name;
> +	u32 offset;
> +	u32 mask;
> +	u32 flags;
> +	u32 id;
> +	u32 rst_offset;
> +	u32 rst_mask;
> +};
> +
> +struct imx_blk_ctl_domain {
> +	struct generic_pm_domain pd;
> +	struct imx_blk_ctl *blk_ctl;
> +	u32 id;
> +};
> +
> +struct imx_blk_ctl_dev_data {
> +	struct regmap_config config;
> +	struct imx_blk_ctl_hw *pds;
> +	struct imx_blk_ctl_hw hw_hsk;
> +	u32 pds_num;
> +	char **active_pd_names;
> +	u32 num_active_pd;
> +};
> +
> +struct imx_blk_ctl {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct device **active_pds;
> +	u32 pds_num;
> +	u32 active_pd_count;
> +	struct genpd_onecell_data onecell_data;
> +	const struct imx_blk_ctl_dev_data *dev_data;
> +	struct clk_bulk_data *clks;
> +	u32 num_clks;
> +
> +	atomic_t power_count;
> +};
> +
> +#define IMX_BLK_CTL(_type, _name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask,	\
> +		    _flags)								\
> +	{										\
> +		.type = _type,								\
> +		.name = _name,								\
> +		.parent_name = _parent_name,						\
> +		.id = _id,								\
> +		.offset = _offset,							\
> +		.mask = _mask,								\
> +		.flags = _flags,							\
> +		.rst_offset = _rst_offset,						\
> +		.rst_mask = _rst_mask,							\
> +	}
> +
> +#define IMX_BLK_CTL_PD(_name, _parent_name, _id, _offset, _mask, _rst_offset, _rst_mask, _flags) \
> +	IMX_BLK_CTL(BLK_CTL_PD, _name, _parent_name, _id, _offset, _mask, _rst_offset,		\
> +		    _rst_mask, _flags)
> +
> +int imx_blk_ctl_register(struct device *dev);
> +
> +#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
> +#define IMX_BLK_CTL_PD_RESET		BIT(1)
> +#define IMX_BLK_CTL_PD_BUS		BIT(2)
> +
> +const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
> +
> +#endif
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2 3/4] soc: imx: Add generic blk-ctl driver

[Peng Fan (OSS)]

> Subject: Re: [PATCH V2 3/4] soc: imx: Add generic blk-ctl driver
> 
> On 30.04.21 07:27, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
> > some GPRs.
> >
> > The GPRs has some clock bits and reset bits, but here we take it as
> > virtual PDs, because of the clock and power domain A/B lock issue when
> > taking it as a clock controller.
> >
> > For some bits, it might be good to also make it as a reset controller,
> > but to i.MX8MM, we not add that support for now.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >   drivers/soc/imx/Makefile  |   2 +-
> >   drivers/soc/imx/blk-ctl.c | 303
> ++++++++++++++++++++++++++++++++++++++
> >   drivers/soc/imx/blk-ctl.h |  76 ++++++++++
> >   3 files changed, 380 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/soc/imx/blk-ctl.c
> >   create mode 100644 drivers/soc/imx/blk-ctl.h
> >
> > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> > 078dc918f4f3..d3d2b49a386c 100644
> > --- a/drivers/soc/imx/Makefile
> > +++ b/drivers/soc/imx/Makefile
> > @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
> >   endif
> >   obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> >   obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> > -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> > +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> > diff --git a/drivers/soc/imx/blk-ctl.c b/drivers/soc/imx/blk-ctl.c new
> > file mode 100644 index 000000000000..1f764dfd308d
> > --- /dev/null
> > +++ b/drivers/soc/imx/blk-ctl.c
> > @@ -0,0 +1,303 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2021 NXP.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#include "blk-ctl.h"
> > +
> > +static inline struct imx_blk_ctl_domain *to_imx_blk_ctl_pd(struct
> > +generic_pm_domain *genpd) {
> > +	return container_of(genpd, struct imx_blk_ctl_domain, pd); }
> > +
> > +static int imx_blk_ctl_enable_hsk(struct device *dev) {
> > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > +	const struct imx_blk_ctl_hw *hw = &blk_ctl->dev_data->hw_hsk;
> > +	struct regmap *regmap = blk_ctl->regmap;
> > +	int ret;
> > +
> > +
> 
> Only one blank line here.

Fix in V3.

> 
> > +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> > +hw->rst_mask);
> 
> The return value above gets discarded.

Fix in V3.

> 
> > +
> > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> > +
> > +	/* Wait for handshake */
> > +	udelay(5);
> > +
> > +	return ret;
> > +}
> > +
> > +int imx_blk_ctl_power_off(struct generic_pm_domain *domain) {
> > +	struct imx_blk_ctl_domain *pd;
> > +	struct imx_blk_ctl *blk_ctl;
> > +	const struct imx_blk_ctl_hw *hw;
> > +	struct regmap *regmap;
> > +	int ret;
> > +
> > +	pd = to_imx_blk_ctl_pd(domain);
> > +	blk_ctl = pd->blk_ctl;
> > +	regmap = blk_ctl->regmap;
> > +	hw = &blk_ctl->dev_data->pds[pd->id];
> 
> You could include the assignments above in the declarations.

Fix in V3.

> 
> > +
> > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, 0);
> 
> You could use regmap_clear_bits() here.

Fix in V3.

> 
> > +	if (ret)
> > +		goto hsk_fail;
> > +
> > +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> 0);
> 
> You could use regmap_clear_bits() here.
> And the return value of regmap_update_bits() potentially gets discarded.

Fix in V3.

> 
> > +
> > +	if (atomic_dec_and_test(&blk_ctl->power_count)) {
> > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > +		if (ret) {
> > +			dev_err(blk_ctl->dev, "Hankshake fail\n");
> 
> s/Hankshake fail/Handshake failed/

Fix in V3.

> 
> > +			goto hsk_fail;
> 
> This goto is redundant.

Fix in V3.

> 
> > +		}
> > +	}
> > +
> > +hsk_fail:
> > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > +
> > +	return ret;
> > +}
> > +
> > +int imx_blk_ctl_power_on(struct generic_pm_domain *domain) {
> > +	struct imx_blk_ctl_domain *pd;
> > +	struct regmap *regmap;
> > +	const struct imx_blk_ctl_hw *hw;
> > +	int ret;
> > +	struct imx_blk_ctl *blk_ctl;
> > +
> > +	pd = to_imx_blk_ctl_pd(domain);
> > +	blk_ctl = pd->blk_ctl;
> > +	regmap = blk_ctl->regmap;
> > +	hw = &blk_ctl->dev_data->pds[pd->id];
> 
> You could include the assignments above in the declarations.
> 
> > +
> > +	ret = clk_bulk_prepare_enable(blk_ctl->num_clks, blk_ctl->clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if ((atomic_read(&blk_ctl->power_count) == 0)) {
> > +		ret = imx_blk_ctl_enable_hsk(blk_ctl->dev);
> > +		if (ret) {
> > +			dev_err(blk_ctl->dev, "Hankshake fail\n");
> 
> s/Hankshake fail/Handshake failed/

Fix in v3.

> 
> > +			goto disable_clk;
> > +		}
> > +	}
> > +
> > +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> 0);
> 
> You could use regmap_clear_bits() here.
> And the return value of regmap_update_bits() gets discarded.

Fix in v3.

> 
> > +
> > +	/* Wait for reset propagate */
> > +	udelay(5);
> > +
> > +	if (hw->flags & IMX_BLK_CTL_PD_RESET)
> > +		ret = regmap_update_bits(regmap, hw->rst_offset, hw->rst_mask,
> > +hw->rst_mask);
> 
> The return value above gets discarded.

Fix in V3.

> 
> > +
> > +	ret = regmap_update_bits(regmap, hw->offset, hw->mask, hw->mask);
> > +	if (ret)
> > +		goto disable_clk;
> > +
> > +	atomic_inc(&blk_ctl->power_count);
> > +
> > +disable_clk:
> > +	clk_bulk_disable_unprepare(blk_ctl->num_clks, blk_ctl->clks);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx_blk_ctl_attach_pd(struct device *dev, struct device **devs,
> char **pd_names,
> > +				 u32 num_pds)
> > +{
> > +	int i, ret;
> > +
> > +	if (!pd_names)
> > +		return 0;
> > +
> > +	if (dev->pm_domain) {
> > +		devs[0] = dev;
> > +		pm_runtime_enable(dev);
> > +		return 0;
> > +	}
> > +
> > +	for (i = 0; i < num_pds; i++) {
> > +		devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
> > +		if (IS_ERR_OR_NULL(devs[i])) {
> > +			ret = PTR_ERR(devs[i]) ? : -ENODATA;
> > +			goto detach_pm;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +detach_pm:
> > +	for (i--; i >= 0; i--)
> > +		dev_pm_domain_detach(devs[i], false);
> 
> It looks like you should add pm_runtime_disable() in this error path to not
> leave the pm_runtime_enable() unmatched.

I might need to remove pm runtime, since no the ops callback here does nothing.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx_blk_ctl_register_pd(struct device *dev) {
> > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> > +	int num = dev_data->pds_num;
> > +	struct imx_blk_ctl_domain *domain;
> > +	int i, ret;
> > +
> > +	blk_ctl->onecell_data.num_domains = num;
> > +	blk_ctl->onecell_data.domains = devm_kcalloc(dev, num,
> > +						     sizeof(struct generic_pm_domain *),
> > +						     GFP_KERNEL);
> > +
> > +	if (!blk_ctl->onecell_data.domains)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
> > +		if (!domain) {
> > +			ret = -ENOMEM;
> > +			goto remove_genpd;
> > +		}
> > +		domain->pd.name = dev_data->pds[i].name;
> > +		domain->pd.power_off = imx_blk_ctl_power_off;
> > +		domain->pd.power_on = imx_blk_ctl_power_on;
> > +		domain->blk_ctl = blk_ctl;
> > +		domain->id = i;
> > +
> > +		ret = pm_genpd_init(&domain->pd, NULL, true);
> > +		if (ret)
> > +			return ret;
> 
> Looks like you should use the error path here and "goto remove_genpd"
> instead of return.

Fix in V3.

> 
> > +
> > +		blk_ctl->onecell_data.domains[i] = &domain->pd;
> > +	}
> > +
> > +	return 0;
> > +
> > +remove_genpd:
> > +	for (i = i - 1; i >= 0; i--)
> > +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx_blk_ctl_hook_pd(struct device *dev) {
> > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> > +	const struct imx_blk_ctl_hw *pds = dev_data->pds;
> > +	int num_active_pd = dev_data->num_active_pd;
> > +	int num = dev_data->pds_num;
> > +	struct generic_pm_domain *genpd, *child_genpd;
> > +	int ret;
> > +	int i, j;
> > +
> > +	blk_ctl->active_pds = devm_kcalloc(dev, num_active_pd, sizeof(struct
> device *), GFP_KERNEL);
> > +	if (!blk_ctl->active_pds)
> > +		return -ENOMEM;
> > +
> > +	ret = imx_blk_ctl_attach_pd(dev, blk_ctl->active_pds,
> dev_data->active_pd_names,
> > +				    num_active_pd);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER)
> > +			return ret;
> > +		dev_err(dev, "Failed to attach active pd: %d\n", ret);
> > +		return ret;
> 
> I think it would be better to do it the other way round:

Fix in V3.

> 
>                  if (ret != -EPROBE_DEFER)
> 	            dev_err(dev, "Failed to attach active pd: %d\n", ret);
>                  return ret;
> 
> > +	}
> > +
> > +	for (i = 0; i < num; i++) {
> > +		for (j = 0; j < num_active_pd; j++) {
> > +			genpd = pd_to_genpd(blk_ctl->active_pds[j]->pm_domain);
> > +			if (!strcmp(genpd->name, pds[i].parent_name))
> > +				break;
> > +		}
> > +
> > +		child_genpd = blk_ctl->onecell_data.domains[i];
> > +		if (pm_genpd_add_subdomain(genpd, child_genpd))
> > +			pr_warn("failed to add subdomain:\n");
> 
> Remove the colon add the end of the warning message or add something after
> it.

Fix in V3.

Thanks,
Peng.

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int imx_blk_ctl_register(struct device *dev) {
> > +	struct imx_blk_ctl *blk_ctl = dev_get_drvdata(dev);
> > +	const struct imx_blk_ctl_dev_data *dev_data = blk_ctl->dev_data;
> > +	int num = dev_data->pds_num;
> > +	int i, ret;
> > +
> > +	if (!blk_ctl)
> > +		return -ENODEV;
> > +
> > +	ret = imx_blk_ctl_register_pd(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = imx_blk_ctl_hook_pd(dev);
> > +	if (ret)
> > +		goto unregister_pd;
> > +
> > +	ret = of_genpd_add_provider_onecell(dev->of_node,
> &blk_ctl->onecell_data);
> > +	if (ret)
> > +		goto detach_pd;
> > +
> > +	pm_runtime_get_noresume(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	return 0;
> > +
> > +detach_pd:
> > +	for (i = blk_ctl->dev_data->num_active_pd; i >= 0; i--)
> > +		dev_pm_domain_detach(blk_ctl->active_pds[i], false);
> > +unregister_pd:
> > +	for (i = num - 1; i >= 0; i--)
> > +		pm_genpd_remove(blk_ctl->onecell_data.domains[i]);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(imx_blk_ctl_register);
> > +
> > +static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device
> > +*dev) {
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused imx_blk_ctl_runtime_resume(struct device
> > +*dev) {
> > +	return 0;
> > +}
> > +
> > +const struct dev_pm_ops imx_blk_ctl_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend,
> > +			   imx_blk_ctl_runtime_resume, NULL)
> > +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +			   pm_runtime_force_resume)
> > +};
> > +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
> > diff --git a/drivers/soc/imx/blk-ctl.h b/drivers/soc/imx/blk-ctl.h new
> > file mode 100644 index 000000000000..e736369406a1
> > --- /dev/null
> > +++ b/drivers/soc/imx/blk-ctl.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef __SOC_IMX_BLK_CTL_H
> > +#define __SOC_IMX_BLK_CTL_H
> > +
> > +enum imx_blk_ctl_pd_type {
> > +	BLK_CTL_PD,
> > +};
> > +
> > +struct imx_blk_ctl_hw {
> > +	int type;
> > +	char *name;
> > +	char *parent_name;
> > +	u32 offset;
> > +	u32 mask;
> > +	u32 flags;
> > +	u32 id;
> > +	u32 rst_offset;
> > +	u32 rst_mask;
> > +};
> > +
> > +struct imx_blk_ctl_domain {
> > +	struct generic_pm_domain pd;
> > +	struct imx_blk_ctl *blk_ctl;
> > +	u32 id;
> > +};
> > +
> > +struct imx_blk_ctl_dev_data {
> > +	struct regmap_config config;
> > +	struct imx_blk_ctl_hw *pds;
> > +	struct imx_blk_ctl_hw hw_hsk;
> > +	u32 pds_num;
> > +	char **active_pd_names;
> > +	u32 num_active_pd;
> > +};
> > +
> > +struct imx_blk_ctl {
> > +	struct device *dev;
> > +	struct regmap *regmap;
> > +	struct device **active_pds;
> > +	u32 pds_num;
> > +	u32 active_pd_count;
> > +	struct genpd_onecell_data onecell_data;
> > +	const struct imx_blk_ctl_dev_data *dev_data;
> > +	struct clk_bulk_data *clks;
> > +	u32 num_clks;
> > +
> > +	atomic_t power_count;
> > +};
> > +
> > +#define IMX_BLK_CTL(_type, _name, _parent_name, _id, _offset, _mask,
> _rst_offset, _rst_mask,	\
> > +		    _flags)								\
> > +	{										\
> > +		.type = _type,								\
> > +		.name = _name,								\
> > +		.parent_name = _parent_name,						\
> > +		.id = _id,								\
> > +		.offset = _offset,							\
> > +		.mask = _mask,								\
> > +		.flags = _flags,							\
> > +		.rst_offset = _rst_offset,						\
> > +		.rst_mask = _rst_mask,							\
> > +	}
> > +
> > +#define IMX_BLK_CTL_PD(_name, _parent_name, _id, _offset, _mask,
> _rst_offset, _rst_mask, _flags) \
> > +	IMX_BLK_CTL(BLK_CTL_PD, _name, _parent_name, _id, _offset, _mask,
> _rst_offset,		\
> > +		    _rst_mask, _flags)
> > +
> > +int imx_blk_ctl_register(struct device *dev);
> > +
> > +#define IMX_BLK_CTL_PD_HANDSHAKE	BIT(0)
> > +#define IMX_BLK_CTL_PD_RESET		BIT(1)
> > +#define IMX_BLK_CTL_PD_BUS		BIT(2)
> > +
> > +const extern struct dev_pm_ops imx_blk_ctl_pm_ops;
> > +
> > +#endif
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 3/4] soc: imx: Add generic blk-ctl driver

[Frieder Schrempf]

On 04.05.21 13:22, Peng Fan (OSS) wrote:
>> Subject: Re: [PATCH V2 3/4] soc: imx: Add generic blk-ctl driver
>>
>> On 30.04.21 07:27, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> The i.MX8MM introduces an IP named BLK_CTL and usually is comprised of
>>> some GPRs.
>>>
>>> The GPRs has some clock bits and reset bits, but here we take it as
>>> virtual PDs, because of the clock and power domain A/B lock issue when
>>> taking it as a clock controller.
>>>
>>> For some bits, it might be good to also make it as a reset controller,
>>> but to i.MX8MM, we not add that support for now.
>>>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>   drivers/soc/imx/Makefile  |   2 +-
>>>   drivers/soc/imx/blk-ctl.c | 303
>> ++++++++++++++++++++++++++++++++++++++
>>>   drivers/soc/imx/blk-ctl.h |  76 ++++++++++
>>>   3 files changed, 380 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/soc/imx/blk-ctl.c
>>>   create mode 100644 drivers/soc/imx/blk-ctl.h
[...]
>>> +
>>> +static int imx_blk_ctl_attach_pd(struct device *dev, struct device **devs,
>> char **pd_names,
>>> +				 u32 num_pds)
>>> +{
>>> +	int i, ret;
>>> +
>>> +	if (!pd_names)
>>> +		return 0;
>>> +
>>> +	if (dev->pm_domain) {
>>> +		devs[0] = dev;
>>> +		pm_runtime_enable(dev);
>>> +		return 0;
>>> +	}
>>> +
>>> +	for (i = 0; i < num_pds; i++) {
>>> +		devs[i] = dev_pm_domain_attach_by_name(dev, pd_names[i]);
>>> +		if (IS_ERR_OR_NULL(devs[i])) {
>>> +			ret = PTR_ERR(devs[i]) ? : -ENODATA;
>>> +			goto detach_pm;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +detach_pm:
>>> +	for (i--; i >= 0; i--)
>>> +		dev_pm_domain_detach(devs[i], false);
>>
>> It looks like you should add pm_runtime_disable() in this error path to not
>> leave the pm_runtime_enable() unmatched.
> 
> I might need to remove pm runtime, since no the ops callback here does nothing.

Anyway, my comment is nonsense as you return success right after pm_runtime_enable().

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 4/4] soc: imx: Add blk-ctl driver for i.MX8MM

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The i.MX8MM SoC has dispmix BLK-CTL and vpumix BLK-CTL, so we add
that support in this driver.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/soc/imx/Makefile         |   2 +-
 drivers/soc/imx/blk-ctl-imx8mm.c | 138 +++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c

diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index d3d2b49a386c..c260b962f495 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
 endif
 obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
-obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
+obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o blk-ctl-imx8mm.o
diff --git a/drivers/soc/imx/blk-ctl-imx8mm.c b/drivers/soc/imx/blk-ctl-imx8mm.c
new file mode 100644
index 000000000000..77e0a5d3fdac
--- /dev/null
+++ b/drivers/soc/imx/blk-ctl-imx8mm.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021 NXP
+ */
+
+#include <dt-bindings/clock/imx8mm-clock.h>
+#include <dt-bindings/power/imx8mm-power.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+
+#include "blk-ctl.h"
+
+#define MEDIA_BLK_BUS_RSTN_BLK_SYNC_SFT_EN			BIT(6)
+#define MEDIA_BLK_MIPI_DSI_I_PRESETN_SFT_EN			BIT(5)
+#define MEDIA_BLK_MIPI_CSI_I_PRESETN_SFT_EN			BIT(4)
+#define MEDIA_BLK_CAMERA_PIXEL_RESET_N_SFT_EN			BIT(3)
+#define MEDIA_BLK_CSI_BRIDGE_SFT_EN				GENMASK(2, 0)
+
+#define MEDIA_BLK_BUS_PD_MASK					BIT(12)
+#define MEDIA_BLK_MIPI_CSI_PD_MASK				GENMASK(11, 10)
+#define MEDIA_BLK_MIPI_DSI_PD_MASK				GENMASK(9, 8)
+#define MEDIA_BLK_LCDIF_PD_MASK					GENMASK(7, 6)
+#define MEDIA_BLK_CSI_BRIDGE_PD_MASK				GENMASK(5, 0)
+
+static struct imx_blk_ctl_hw imx8mm_dispmix_blk_ctl_pds[] = {
+	IMX_BLK_CTL_PD("CSI_BRIDGE", "dispmix", IMX8MM_BLK_CTL_DISPMIX_CSI_BRIDGE, 0x4,
+		       MEDIA_BLK_CSI_BRIDGE_PD_MASK, 0, MEDIA_BLK_CSI_BRIDGE_SFT_EN,
+		       IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("LCDIF", "dispmix", IMX8MM_BLK_CTL_DISPMIX_LCDIF, 0x4,
+		       MEDIA_BLK_LCDIF_PD_MASK, -1, -1, 0),
+	IMX_BLK_CTL_PD("MIPI_DSI", "mipi", IMX8MM_BLK_CTL_DISPMIX_MIPI_DSI, 0x4,
+		       MEDIA_BLK_MIPI_DSI_PD_MASK, 0, MEDIA_BLK_MIPI_DSI_I_PRESETN_SFT_EN,
+		       IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("MIPI_CSI", "mipi", IMX8MM_BLK_CTL_DISPMIX_MIPI_CSI, 0x4,
+		       MEDIA_BLK_MIPI_CSI_PD_MASK, 0,
+		       MEDIA_BLK_MIPI_CSI_I_PRESETN_SFT_EN | MEDIA_BLK_CAMERA_PIXEL_RESET_N_SFT_EN,
+		       IMX_BLK_CTL_PD_RESET)
+};
+
+static struct imx_blk_ctl_hw imx8mm_vpumix_blk_ctl_pds[] = {
+	IMX_BLK_CTL_PD("VPU_BLK_CTL_G2", "vpu-g2", IMX8MM_BLK_CTL_G2_PD, 0x4,
+		       BIT(0), 0, BIT(0), IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("VPU_BLK_CTL_G1", "vpu-g1", IMX8MM_BLK_CTL_G1_PD, 0x4,
+		       BIT(1), 0, BIT(1), IMX_BLK_CTL_PD_RESET),
+	IMX_BLK_CTL_PD("VPU_BLK_CTL_H1", "vpu-h1", IMX8MM_BLK_CTL_H1_PD, 0x4,
+		       BIT(2), 0, BIT(2), IMX_BLK_CTL_PD_HANDSHAKE | IMX_BLK_CTL_PD_RESET),
+};
+
+static const struct regmap_config imx8mm_blk_ctl_regmap_config = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+	.max_register		= 0x30,
+	.fast_io		= true,
+};
+
+static const struct imx_blk_ctl_dev_data imx8mm_vpumix_blk_ctl_dev_data = {
+	.pds = imx8mm_vpumix_blk_ctl_pds,
+	.pds_num = ARRAY_SIZE(imx8mm_vpumix_blk_ctl_pds),
+	.hw_hsk = IMX_BLK_CTL_PD(NULL, NULL, IMX8MM_BLK_CTL_H1_PD, 0x4, BIT(2), 0, BIT(2),
+				 IMX_BLK_CTL_PD_HANDSHAKE),
+	.config = imx8mm_blk_ctl_regmap_config,
+	.active_pd_names = (char*[]){"vpumix", "g1", "g2", "h1"},
+	.num_active_pd = 4,
+};
+
+static const struct imx_blk_ctl_dev_data imx8mm_dispmix_blk_ctl_dev_data = {
+	.pds = imx8mm_dispmix_blk_ctl_pds,
+	.pds_num = ARRAY_SIZE(imx8mm_dispmix_blk_ctl_pds),
+	.hw_hsk = IMX_BLK_CTL_PD(NULL, NULL, -1, 0x4, MEDIA_BLK_BUS_PD_MASK, 0,
+				 MEDIA_BLK_BUS_RSTN_BLK_SYNC_SFT_EN,
+				 IMX_BLK_CTL_PD_HANDSHAKE | IMX_BLK_CTL_PD_RESET),
+	.config = imx8mm_blk_ctl_regmap_config,
+	.active_pd_names = (char*[]){"dispmix", "mipi"},
+	.num_active_pd = 2,
+};
+
+static int imx8mm_blk_ctl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev);
+	struct regmap *regmap;
+	struct resource *res;
+	struct imx_blk_ctl *ctl;
+	void __iomem *base;
+
+	ctl = devm_kzalloc(dev, sizeof(*ctl), GFP_KERNEL);
+	if (!ctl)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &dev_data->config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ctl->regmap = regmap;
+	ctl->dev = dev;
+	atomic_set(&ctl->power_count, 0);
+
+	ctl->num_clks = devm_clk_bulk_get_all(dev, &ctl->clks);
+	if (ctl->num_clks < 0)
+		return ctl->num_clks;
+
+	dev_set_drvdata(dev, ctl);
+	ctl->dev_data = of_device_get_match_data(dev);
+
+	return imx_blk_ctl_register(dev);
+}
+
+static const struct of_device_id imx_blk_ctl_of_match[] = {
+	{ .compatible = "fsl,imx8mm-vpumix-blk-ctl", .data = &imx8mm_vpumix_blk_ctl_dev_data },
+	{ .compatible = "fsl,imx8mm-dispmix-blk-ctl", .data = &imx8mm_dispmix_blk_ctl_dev_data },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_blk_ctl_of_match);
+
+static struct platform_driver imx_blk_ctl_driver = {
+	.probe = imx8mm_blk_ctl_probe,
+	.driver = {
+		.name = "imx8mm-blk-ctl",
+		.of_match_table = of_match_ptr(imx_blk_ctl_of_match),
+		.pm = &imx_blk_ctl_pm_ops,
+	},
+};
+module_platform_driver(imx_blk_ctl_driver);
-- 
2.30.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 4/4] soc: imx: Add blk-ctl driver for i.MX8MM

[Frieder Schrempf]

On 30.04.21 07:27, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX8MM SoC has dispmix BLK-CTL and vpumix BLK-CTL, so we add
> that support in this driver.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   drivers/soc/imx/Makefile         |   2 +-
>   drivers/soc/imx/blk-ctl-imx8mm.c | 138 +++++++++++++++++++++++++++++++
>   2 files changed, 139 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c
> 
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> index d3d2b49a386c..c260b962f495 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -4,4 +4,4 @@ obj-$(CONFIG_ARCH_MXC) += soc-imx.o
>   endif
>   obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>   obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> -obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o
> +obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o blk-ctl.o blk-ctl-imx8mm.o
> diff --git a/drivers/soc/imx/blk-ctl-imx8mm.c b/drivers/soc/imx/blk-ctl-imx8mm.c
> new file mode 100644
> index 000000000000..77e0a5d3fdac
> --- /dev/null
> +++ b/drivers/soc/imx/blk-ctl-imx8mm.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2021 NXP
> + */
> +
> +#include <dt-bindings/clock/imx8mm-clock.h>
> +#include <dt-bindings/power/imx8mm-power.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regmap.h>
> +
> +#include "blk-ctl.h"
> +
> +#define MEDIA_BLK_BUS_RSTN_BLK_SYNC_SFT_EN			BIT(6)
> +#define MEDIA_BLK_MIPI_DSI_I_PRESETN_SFT_EN			BIT(5)
> +#define MEDIA_BLK_MIPI_CSI_I_PRESETN_SFT_EN			BIT(4)
> +#define MEDIA_BLK_CAMERA_PIXEL_RESET_N_SFT_EN			BIT(3)
> +#define MEDIA_BLK_CSI_BRIDGE_SFT_EN				GENMASK(2, 0)
> +
> +#define MEDIA_BLK_BUS_PD_MASK					BIT(12)
> +#define MEDIA_BLK_MIPI_CSI_PD_MASK				GENMASK(11, 10)
> +#define MEDIA_BLK_MIPI_DSI_PD_MASK				GENMASK(9, 8)
> +#define MEDIA_BLK_LCDIF_PD_MASK					GENMASK(7, 6)
> +#define MEDIA_BLK_CSI_BRIDGE_PD_MASK				GENMASK(5, 0)
> +
> +static struct imx_blk_ctl_hw imx8mm_dispmix_blk_ctl_pds[] = {
> +	IMX_BLK_CTL_PD("CSI_BRIDGE", "dispmix", IMX8MM_BLK_CTL_DISPMIX_CSI_BRIDGE, 0x4,
> +		       MEDIA_BLK_CSI_BRIDGE_PD_MASK, 0, MEDIA_BLK_CSI_BRIDGE_SFT_EN,
> +		       IMX_BLK_CTL_PD_RESET),
> +	IMX_BLK_CTL_PD("LCDIF", "dispmix", IMX8MM_BLK_CTL_DISPMIX_LCDIF, 0x4,
> +		       MEDIA_BLK_LCDIF_PD_MASK, -1, -1, 0),
> +	IMX_BLK_CTL_PD("MIPI_DSI", "mipi", IMX8MM_BLK_CTL_DISPMIX_MIPI_DSI, 0x4,
> +		       MEDIA_BLK_MIPI_DSI_PD_MASK, 0, MEDIA_BLK_MIPI_DSI_I_PRESETN_SFT_EN,
> +		       IMX_BLK_CTL_PD_RESET),
> +	IMX_BLK_CTL_PD("MIPI_CSI", "mipi", IMX8MM_BLK_CTL_DISPMIX_MIPI_CSI, 0x4,
> +		       MEDIA_BLK_MIPI_CSI_PD_MASK, 0,
> +		       MEDIA_BLK_MIPI_CSI_I_PRESETN_SFT_EN | MEDIA_BLK_CAMERA_PIXEL_RESET_N_SFT_EN,
> +		       IMX_BLK_CTL_PD_RESET)
> +};
> +
> +static struct imx_blk_ctl_hw imx8mm_vpumix_blk_ctl_pds[] = {
> +	IMX_BLK_CTL_PD("VPU_BLK_CTL_G2", "vpu-g2", IMX8MM_BLK_CTL_G2_PD, 0x4,
> +		       BIT(0), 0, BIT(0), IMX_BLK_CTL_PD_RESET),
> +	IMX_BLK_CTL_PD("VPU_BLK_CTL_G1", "vpu-g1", IMX8MM_BLK_CTL_G1_PD, 0x4,
> +		       BIT(1), 0, BIT(1), IMX_BLK_CTL_PD_RESET),
> +	IMX_BLK_CTL_PD("VPU_BLK_CTL_H1", "vpu-h1", IMX8MM_BLK_CTL_H1_PD, 0x4,
> +		       BIT(2), 0, BIT(2), IMX_BLK_CTL_PD_HANDSHAKE | IMX_BLK_CTL_PD_RESET),
> +};
> +
> +static const struct regmap_config imx8mm_blk_ctl_regmap_config = {
> +	.reg_bits		= 32,
> +	.reg_stride		= 4,
> +	.val_bits		= 32,
> +	.max_register		= 0x30,
> +	.fast_io		= true,
> +};
> +
> +static const struct imx_blk_ctl_dev_data imx8mm_vpumix_blk_ctl_dev_data = {
> +	.pds = imx8mm_vpumix_blk_ctl_pds,
> +	.pds_num = ARRAY_SIZE(imx8mm_vpumix_blk_ctl_pds),
> +	.hw_hsk = IMX_BLK_CTL_PD(NULL, NULL, IMX8MM_BLK_CTL_H1_PD, 0x4, BIT(2), 0, BIT(2),
> +				 IMX_BLK_CTL_PD_HANDSHAKE),
> +	.config = imx8mm_blk_ctl_regmap_config,
> +	.active_pd_names = (char*[]){"vpumix", "g1", "g2", "h1"},
> +	.num_active_pd = 4,
> +};
> +
> +static const struct imx_blk_ctl_dev_data imx8mm_dispmix_blk_ctl_dev_data = {
> +	.pds = imx8mm_dispmix_blk_ctl_pds,
> +	.pds_num = ARRAY_SIZE(imx8mm_dispmix_blk_ctl_pds),
> +	.hw_hsk = IMX_BLK_CTL_PD(NULL, NULL, -1, 0x4, MEDIA_BLK_BUS_PD_MASK, 0,
> +				 MEDIA_BLK_BUS_RSTN_BLK_SYNC_SFT_EN,
> +				 IMX_BLK_CTL_PD_HANDSHAKE | IMX_BLK_CTL_PD_RESET),
> +	.config = imx8mm_blk_ctl_regmap_config,
> +	.active_pd_names = (char*[]){"dispmix", "mipi"},
> +	.num_active_pd = 2,
> +};
> +
> +static int imx8mm_blk_ctl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev);
> +	struct regmap *regmap;
> +	struct resource *res;
> +	struct imx_blk_ctl *ctl;
> +	void __iomem *base;
> +
> +	ctl = devm_kzalloc(dev, sizeof(*ctl), GFP_KERNEL);
> +	if (!ctl)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);

This could be simplified by using devm_platform_ioremap_resource().

> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap = devm_regmap_init_mmio(dev, base, &dev_data->config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ctl->regmap = regmap;
> +	ctl->dev = dev;
> +	atomic_set(&ctl->power_count, 0);
> +
> +	ctl->num_clks = devm_clk_bulk_get_all(dev, &ctl->clks);
> +	if (ctl->num_clks < 0)
> +		return ctl->num_clks;
> +
> +	dev_set_drvdata(dev, ctl);
> +	ctl->dev_data = of_device_get_match_data(dev);
> +
> +	return imx_blk_ctl_register(dev);
> +}
> +
> +static const struct of_device_id imx_blk_ctl_of_match[] = {
> +	{ .compatible = "fsl,imx8mm-vpumix-blk-ctl", .data = &imx8mm_vpumix_blk_ctl_dev_data },
> +	{ .compatible = "fsl,imx8mm-dispmix-blk-ctl", .data = &imx8mm_dispmix_blk_ctl_dev_data },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_blk_ctl_of_match);
> +
> +static struct platform_driver imx_blk_ctl_driver = {
> +	.probe = imx8mm_blk_ctl_probe,
> +	.driver = {
> +		.name = "imx8mm-blk-ctl",
> +		.of_match_table = of_match_ptr(imx_blk_ctl_of_match),
> +		.pm = &imx_blk_ctl_pm_ops,
> +	},
> +};
> +module_platform_driver(imx_blk_ctl_driver);
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 4/4] soc: imx: Add blk-ctl driver for i.MX8MM

[Fabio Estevam]

Hi Peng,

On Fri, Apr 30, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> +static int imx8mm_blk_ctl_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       const struct imx_blk_ctl_dev_data *dev_data = of_device_get_match_data(dev);
> +       struct regmap *regmap;
> +       struct resource *res;
> +       struct imx_blk_ctl *ctl;
> +       void __iomem *base;
> +
> +       ctl = devm_kzalloc(dev, sizeof(*ctl), GFP_KERNEL);
> +       if (!ctl)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       regmap = devm_regmap_init_mmio(dev, base, &dev_data->config);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       ctl->regmap = regmap;
> +       ctl->dev = dev;
> +       atomic_set(&ctl->power_count, 0);
> +
> +       ctl->num_clks = devm_clk_bulk_get_all(dev, &ctl->clks);
> +       if (ctl->num_clks < 0)
> +               return ctl->num_clks;
> +
> +       dev_set_drvdata(dev, ctl);
> +       ctl->dev_data = of_device_get_match_data(dev);

No need to call of_device_get_match_data() twice. You could do:

ctl->dev_data = dev_data;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2 4/4] soc: imx: Add blk-ctl driver for i.MX8MM

[Peng Fan]

> Subject: Re: [PATCH V2 4/4] soc: imx: Add blk-ctl driver for i.MX8MM
> 
> Hi Peng,
> 
> On Fri, Apr 30, 2021 at 1:56 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> 
> > +static int imx8mm_blk_ctl_probe(struct platform_device *pdev) {
> > +       struct device *dev = &pdev->dev;
> > +       const struct imx_blk_ctl_dev_data *dev_data =
> of_device_get_match_data(dev);
> > +       struct regmap *regmap;
> > +       struct resource *res;
> > +       struct imx_blk_ctl *ctl;
> > +       void __iomem *base;
> > +
> > +       ctl = devm_kzalloc(dev, sizeof(*ctl), GFP_KERNEL);
> > +       if (!ctl)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       base = devm_ioremap_resource(dev, res);
> > +       if (IS_ERR(base))
> > +               return PTR_ERR(base);
> > +
> > +       regmap = devm_regmap_init_mmio(dev, base,
> &dev_data->config);
> > +       if (IS_ERR(regmap))
> > +               return PTR_ERR(regmap);
> > +
> > +       ctl->regmap = regmap;
> > +       ctl->dev = dev;
> > +       atomic_set(&ctl->power_count, 0);
> > +
> > +       ctl->num_clks = devm_clk_bulk_get_all(dev, &ctl->clks);
> > +       if (ctl->num_clks < 0)
> > +               return ctl->num_clks;
> > +
> > +       dev_set_drvdata(dev, ctl);
> > +       ctl->dev_data = of_device_get_match_data(dev);
> 
> No need to call of_device_get_match_data() twice. You could do:
> 
> ctl->dev_data = dev_data;

Fix in V3.

Thanks,
Peng.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support

[Peng Fan]

> Subject: [PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support
> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> V2:
>  Fix yaml check failure

Forget the address Jacky's comments on patch 3,4 in V1.
Will send V3 to add fix.

Thanks,
Peng.

> 
> Previously there is an effort from Abel that take BLK-CTL as clock provider, but
> it turns out that there is A/B lock issue and we are not able resolve that.
> 
> Per discuss with Lucas and Jacky, we made an agreement that take BLK-CTL
> as a power domain provider and use GPC's domain as parent, the consumer
> node take BLK-CTL as power domain input.
> 
> This patchset has been tested on i.MX8MM EVK board, but one hack is not
> included in the patchset is that the DISPMIX BLK-CTL MIPI_M/S_RESET not
> implemented. Per Lucas, we will finally have a MIPI DPHY driver, so fine to
> leave it.
> 
> Thanks for Lucas's suggestion, Frieder Schrempf for collecting all the patches,
> Abel's previous BLK-CTL work, Jacky Bai on help debug issues.
> 
> Peng Fan (4):
>   dt-bindings: power: Add defines for i.MX8MM BLK-CTL power domains
>   Documentation: bindings: clk: Add bindings for i.MX BLK_CTL
>   soc: imx: Add generic blk-ctl driver
>   soc: imx: Add blk-ctl driver for i.MX8MM
> 
>  .../bindings/soc/imx/fsl,imx-blk-ctl.yaml     |  73 +++++
>  drivers/soc/imx/Makefile                      |   2 +-
>  drivers/soc/imx/blk-ctl-imx8mm.c              | 138 ++++++++
>  drivers/soc/imx/blk-ctl.c                     | 303
> ++++++++++++++++++
>  drivers/soc/imx/blk-ctl.h                     |  76 +++++
>  include/dt-bindings/power/imx8mm-power.h      |  11 +
>  6 files changed, 602 insertions(+), 1 deletion(-)  create mode 100644
> Documentation/devicetree/bindings/soc/imx/fsl,imx-blk-ctl.yaml
>  create mode 100644 drivers/soc/imx/blk-ctl-imx8mm.c  create mode
> 100644 drivers/soc/imx/blk-ctl.c  create mode 100644
> drivers/soc/imx/blk-ctl.h
> 
> --
> 2.30.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support

[Frieder Schrempf]

Hi Peng,

On 30.04.21 07:27, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> V2:
>   Fix yaml check failure
> 
> Previously there is an effort from Abel that take BLK-CTL as clock
> provider, but it turns out that there is A/B lock issue and we are
> not able resolve that.
> 
> Per discuss with Lucas and Jacky, we made an agreement that take BLK-CTL
> as a power domain provider and use GPC's domain as parent, the consumer
> node take BLK-CTL as power domain input.
> 
> This patchset has been tested on i.MX8MM EVK board, but one hack
> is not included in the patchset is that the DISPMIX BLK-CTL MIPI_M/S_RESET
> not implemented. Per Lucas, we will finally have a MIPI DPHY driver,
> so fine to leave it.

Thanks for your work. I would like to test this together with the DSIM 
and PHY driver by Michael and Marek. So far the boot hangs when probing 
the DSIM, but I'm not even sure if my DT is correct.

With the DSIM PHY driver (see [1]) in place, the GPR_MIPI_M_RESETN 
should be set correctly, right?

Would you mind sharing your imx8mm.dtsi, that you used for testing?

Thanks
Frieder

[1] 
https://patchwork.kernel.org/project/linux-samsung-soc/patch/20201003225020.164358-1-marex@denx.de/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/4] soc: imx: add i.MX BLK-CTL support

[Frieder Schrempf]

On 03.05.21 16:57, Frieder Schrempf wrote:
> Hi Peng,
> 
> On 30.04.21 07:27, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> V2:
>>   Fix yaml check failure
>>
>> Previously there is an effort from Abel that take BLK-CTL as clock
>> provider, but it turns out that there is A/B lock issue and we are
>> not able resolve that.
>>
>> Per discuss with Lucas and Jacky, we made an agreement that take BLK-CTL
>> as a power domain provider and use GPC's domain as parent, the consumer
>> node take BLK-CTL as power domain input.
>>
>> This patchset has been tested on i.MX8MM EVK board, but one hack
>> is not included in the patchset is that the DISPMIX BLK-CTL 
>> MIPI_M/S_RESET
>> not implemented. Per Lucas, we will finally have a MIPI DPHY driver,
>> so fine to leave it.
> 
> Thanks for your work. I would like to test this together with the DSIM 
> and PHY driver by Michael and Marek. So far the boot hangs when probing 
> the DSIM, but I'm not even sure if my DT is correct.
> 
> With the DSIM PHY driver (see [1]) in place, the GPR_MIPI_M_RESETN 
> should be set correctly, right?

So I found out, that with the hack below applied (taken from Marek's WIP 
patches for DSIM support) it seems to work properly.

Therefore I guess this is an issue with the DSIM driver, that somehow 
needs to make sure that the proper clocks are enabled at probe time.

diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 04564017bfe9..5a9bca805b0c 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -9,6 +9,7 @@
   */

  #include <linux/clk.h>
+#include <linux/clk-provider.h>
  #include <linux/of_device.h>
  #include <linux/platform_device.h>
  #include <linux/pm_domain.h>
@@ -193,6 +194,7 @@ to_imx_pgc_domain(struct generic_pm_domain *genpd)
  static int imx_pgc_power_up(struct generic_pm_domain *genpd)
  {
         struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd);
+       unsigned int i;
         u32 reg_val;
         int ret;

@@ -254,7 +256,14 @@ static int imx_pgc_power_up(struct 
generic_pm_domain *genpd)
         }

         /* Disable reset clocks for all devices in the domain */
-       clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+       //clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
+       for (i = 0; i < domain->num_clks; i++) {
+               /* Keep the DISPMIX active, it is needed by both LCDIF 
and MIPI */
+               if (strcmp(__clk_get_name(domain->clks[i].clk), 
"disp_root_clk") &&
+                   strcmp(__clk_get_name(domain->clks[i].clk), 
"disp_axi_root_clk") &&
+                   strcmp(__clk_get_name(domain->clks[i].clk), 
"disp_apb_root_clk"))
+                       clk_disable_unprepare(domain->clks[i].clk);
+       }

         return 0;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel