clk: imx: add imx8qxp clock support

Add IMX8QXP SCU clock IDs.

Cc Rob Herring <[email protected]>

Cc Stephen Boyd <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc devicetree at vger.kernel.org

Cc linux-clk at vger.kernel.org

Signed-off-by Dong Aisheng <[email protected]>

---
v6: new patch, separate from driver changes to avoid a checkpatch warning
IDs are changed a lot due to SCU divider/gate/mux are merged into one general SCU clock and LPCG clocks are moved out.

---
 include/dt-bindings/clock/imx8qxp-clock.h | 136 ++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)
 create mode 100644 include/dt-bindings/clock/imx8qxp-clock.h

The Low-Power Clock Gate (LPCG) modules contain a local programming model to control the clock gates for the peripherals. An LPCG module is used to locally gate the clocks for the associated peripheral.

Note:
This level of clock gating is provided after the clocks are generated by the SCU resources and clock controls. Thus even if the clock is enabled by these control bits, it might still not be running based on the base resource.

Cc Stephen Boyd <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Rob Herring <[email protected]>

Cc devicetree at vger.kernel.org

Signed-off-by Dong Aisheng <[email protected]>

---
 .../devicetree/bindings/clock/imx8qxp-lpcg.txt     |  51 +++++++
 include/dt-bindings/clock/imx8qxp-clock.h          | 153 +++++++++++++++++++++
 2 files changed, 204 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt

drivers/clk/imx/clk-lpcg-scu.c new file +13 -0

new file mode 100644
index 0000000..54a1cb3
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP
+ *     Dong Aisheng <[email protected]>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+static DEFINE_SPINLOCK(imx_lpcg_scu_lock);

drivers/clk/imx/clk-lpcg-scu.c new file +14 -0

+       u32 reg;
+
+       spin_lock_irqsave(&imx_lpcg_scu_lock, flags);
+
+       if (clk->reg) {
+               reg = readl(clk->reg);
+               reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
+               if (clk->hw_gate)
+                       reg |= (CLK_GATE_SCU_LPCG_HW_SEL |
+                               CLK_GATE_SCU_LPCG_SW_SEL) << clk->bit_idx;
+               else
+                       reg |= (CLK_GATE_SCU_LPCG_SW_SEL << clk->bit_idx);
+               writel(reg, clk->reg);
+       }

drivers/clk/imx/clk-lpcg-scu.c new file +8 -0

+               reg = readl(clk->reg);
+               reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
+               writel(reg, clk->reg);
+       }
+
+       spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);
+}
+

-----Original Message-----
From: Stephen Boyd [mailto:sboyd at kernel.org]

Quoting A.s. Dong (2018-11-10 07:20:04)

Quoted 23 lines

(quoted diff) +0 -0

b/drivers/clk/imx/clk-lpcg-scu.c new file mode 100644 index 0000000..54a1cb3

drivers/clk/imx/clk-lpcg-scu.c new file +17 -0

@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP
+ *     Dong Aisheng <[email protected]>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+static DEFINE_SPINLOCK(imx_lpcg_scu_lock);
+
+#define CLK_GATE_SCU_LPCG_MASK         0x3
+#define CLK_GATE_SCU_LPCG_HW_SEL       BIT(0)
+#define CLK_GATE_SCU_LPCG_SW_SEL       BIT(1)

drivers/clk/imx/clk-lpcg-scu.c line 23

Quoted 6 lines

drivers/clk/imx/clk-lpcg-scu.c new file +6 -0

+
+struct clk_lpcg_scu {
+       struct clk_hw hw;
+       void __iomem *reg;
+       u8 bit_idx;
+       bool hw_gate;

What does hw_gate mean? Can you add kernel-doc to this structure?

drivers/clk/imx/clk-lpcg-scu.c line 28

Quoted 6 lines

drivers/clk/imx/clk-lpcg-scu.c new file +6 -0

+};
+
+#define to_clk_lpcg_scu(_hw) container_of(_hw, struct clk_lpcg_scu,
+hw)
+
+/* Write to the LPCG bits. */

Please remove this useless comment.

drivers/clk/imx/clk-lpcg-scu.c line 32

Quoted 3 lines

drivers/clk/imx/clk-lpcg-scu.c new file +3 -0

+static int clk_lpcg_scu_enable(struct clk_hw *hw) {
+       struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw);
+       unsigned long flags = 0;

Don't initialize flags here.

drivers/clk/imx/clk-lpcg-scu.c

Quoted 10 lines

drivers/clk/imx/clk-lpcg-scu.c new file +10 -0

+       u32 reg;
+
+       spin_lock_irqsave(&imx_lpcg_scu_lock, flags);
+
+       if (clk->reg) {
+               reg = readl(clk->reg);
+               reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
+               if (clk->hw_gate)
+                       reg |= (CLK_GATE_SCU_LPCG_HW_SEL |
+                               CLK_GATE_SCU_LPCG_SW_SEL) <<

clk->bit_idx;

drivers/clk/imx/clk-lpcg-scu.c line 44

Quoted 2 lines

drivers/clk/imx/clk-lpcg-scu.c new file +2 -0

+               else
+                       reg |= (CLK_GATE_SCU_LPCG_SW_SEL <<

clk->bit_idx);

drivers/clk/imx/clk-lpcg-scu.c line 56

Quoted 11 lines

drivers/clk/imx/clk-lpcg-scu.c new file +11 -0

+               writel(reg, clk->reg);
+       }
+
+       spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);
+
+       return 0;
+}
+
+static void clk_lpcg_scu_disable(struct clk_hw *hw) {
+       struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw);
+       unsigned long flags = 0;

Same comment.

drivers/clk/imx/clk-lpcg-scu.c line 37

Quoted 5 lines

drivers/clk/imx/clk-lpcg-scu.c new file +5 -0

+       u32 reg;
+
+       spin_lock_irqsave(&imx_lpcg_scu_lock, flags);
+
+       if (clk->reg) {

Why do we even call this function if there isn't a clk->reg value? Can't we just not assign the clk_ops in that case?

Quoted 7 lines

drivers/clk/imx/clk-lpcg-scu.c new file +7 -0

+               reg = readl(clk->reg);
+               reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
+               writel(reg, clk->reg);
+       }
+
+       spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags); }
+

Add imx8qxp lpcg driver support

Cc Stephen Boyd <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Michael Turquette <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +17 -0

+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "clk-scu.h"
+#include "clk-imx8qxp-lpcg.h"
+
+struct imx8qxp_lpcg_data {
+       int id;
+       char *name;
+       char *parent;
+       unsigned long flags;

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +6 -0

+
+static const struct imx8qxp_lpcg_data imx8qxp_lpcg_adma[] = {
+       { IMX8QXP_ADMA_LPCG_UART0_IPG_CLK, "uart0_lpcg_ipg_clk", "dma_ipg_clk_root", 0, ADMA_LPUART_0_LPCG, 16, 0, },
+       { IMX8QXP_ADMA_LPCG_UART0_BAUD_CLK, "uart0_lpcg_baud_clk", "uart0_clk", 0, ADMA_LPUART_0_LPCG, 0, 0, },
+       { IMX8QXP_ADMA_LPCG_UART1_IPG_CLK, "uart1_lpcg_ipg_clk", "dma_ipg_clk_root", 0, ADMA_LPUART_1_LPCG, 16, 0, },
+       { IMX8QXP_ADMA_LPCG_UART1_BAUD_CLK, "uart1_lpcg_baud_clk", "uart1_clk", 0, ADMA_LPUART_1_LPCG, 0, 0, },

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +8 -0

+                               GFP_KERNEL);
+       if (!clk_data)
+               return -ENOMEM;
+
+       clk_data->num = ss_lpcg->num_max;
+       clks = clk_data->hws;
+
+       for (i = 0; i < ss_lpcg->num_lpcg; i++) {

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +12 -0

+}
+
+static const struct of_device_id imx8qxp_lpcg_match[] = {
+       { .compatible = "fsl,imx8qxp-lpcg-adma", &imx8qxp_ss_adma, },
+       { .compatible = "fsl,imx8qxp-lpcg-conn", &imx8qxp_ss_conn, },
+       { .compatible = "fsl,imx8qxp-lpcg-lsio", &imx8qxp_ss_lsio, },
+       { /* sentinel */ }
+};
+
+static struct platform_driver imx8qxp_lpcg_clk_driver = {
+       .driver = {
+               .name = "imx8qxp-lpcg-clk",

-----Original Message-----
From: Stephen Boyd [mailto:sboyd at kernel.org]
Sent: Thursday, November 15, 2018 7:29 AM

Quoting A.s. Dong (2018-11-10 07:20:08)

Quoted 2 lines

Add imx8qxp lpcg driver support

Quoted 6 lines

Cc Stephen Boyd <[email protected]>

Cc Shawn Guo <[email protected]>

Cc Sascha Hauer <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Michael Turquette <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

I like it!

Quoted 8 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +1 -0

new file mode 100644
index 0000000..7b318d7
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0+

drivers/clk/imx/clk-imx8qxp-lpcg.c line 7

Quoted 6 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +6 -0

+/*
+ * Copyright 2018 NXP
+ *     Dong Aisheng <[email protected]>
+ */
+
+#include <dt-bindings/clock/imx8qxp-clock.h>

Include this after <linux/*> and before locals please.

Quoted 22 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +22 -0

+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "clk-scu.h"
+#include "clk-imx8qxp-lpcg.h"
+
+struct imx8qxp_lpcg_data {
+       int id;
+       char *name;
+       char *parent;
+       unsigned long flags;
+       u32 offset;
+       u8 bit_idx;
+       bool hw_gate;
+};
+

drivers/clk/imx/clk-imx8qxp-lpcg.c line 34

Quoted 5 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +5 -0

+struct imx8qxp_ss_lpcg {
+       const struct imx8qxp_lpcg_data *lpcg;
+       u8 num_lpcg;
+       u8 num_max;
+};

Any chance we can get kernel-doc on these structs and members of the structs?

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 3 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +3 -0

+
+static const struct imx8qxp_lpcg_data imx8qxp_lpcg_adma[] = {
+       { IMX8QXP_ADMA_LPCG_UART0_IPG_CLK, "uart0_lpcg_ipg_clk",

"dma_ipg_clk_root", 0, ADMA_LPUART_0_LPCG, 16, 0, },

drivers/clk/imx/clk-imx8qxp-lpcg.c line 38

Quoted 1 line

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +1 -0

+       { IMX8QXP_ADMA_LPCG_UART0_BAUD_CLK,

"uart0_lpcg_baud_clk", "uart0_clk", 0, ADMA_LPUART_0_LPCG, 0, 0, },

drivers/clk/imx/clk-imx8qxp-lpcg.c line 39

Quoted 1 line

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +1 -0

+       { IMX8QXP_ADMA_LPCG_UART1_IPG_CLK, "uart1_lpcg_ipg_clk",

"dma_ipg_clk_root", 0, ADMA_LPUART_1_LPCG, 16, 0, },

drivers/clk/imx/clk-imx8qxp-lpcg.c line 40

Quoted 1 line

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +1 -0

+       { IMX8QXP_ADMA_LPCG_UART1_BAUD_CLK,

"uart1_lpcg_baud_clk",

Quoted 1 line

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +1 -0

+"uart1_clk", 0, ADMA_LPUART_1_LPCG, 0, 0, },

[...]

drivers/clk/imx/clk-imx8qxp-lpcg.c line 143

Quoted 11 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +11 -0

+
+static int imx8qxp_lpcg_clk_probe(struct platform_device *pdev) {
+       struct device *dev = &pdev->dev;
+       struct device_node *np = dev->of_node;
+       struct clk_hw_onecell_data *clk_data;
+       const struct imx8qxp_ss_lpcg *ss_lpcg;
+       const struct imx8qxp_lpcg_data *lpcg;
+       struct resource *res;
+       struct clk_hw **clks;
+       void __iomem *base;
+       int i;

That's a good amount of local variables!

drivers/clk/imx/clk-imx8qxp-lpcg.c line 150

Quoted 8 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +7 -0

+
+       ss_lpcg = of_device_get_match_data(dev);
+       if (!ss_lpcg)
+               return -ENODEV;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap(dev, res->start, resource_size(res));

Why not devm_ioremap_resource()?

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 3 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +2 -0

+       if (!base)
+               return -ENOMEM;

And then return the error code out of it?

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 4 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +4 -0

+
+       clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) +
+                               sizeof(*clk_data->hws) *
+ ss_lpcg->num_max,

Can you use struct_size() here?

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 10 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +10 -0

+                               GFP_KERNEL);
+       if (!clk_data)
+               return -ENOMEM;
+
+       clk_data->num = ss_lpcg->num_max;
+       clks = clk_data->hws;
+
+       for (i = 0; i < ss_lpcg->num_lpcg; i++) {
+               lpcg = ss_lpcg->lpcg + i;
+               clks[lpcg->id] = imx_clk_lpcg_scu(lpcg->name,

lpcg->parent,

Quoted 1 line

+ lpcg->flags, base +

lpcg->offset,

Quoted 1 line

+ lpcg->bit_idx,

lpcg->hw_gate);

drivers/clk/imx/clk-imx8qxp-lpcg.c line 173

Quoted 6 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +6 -0

+       }
+
+       for (i = 0; i < clk_data->num; i++) {
+               if (IS_ERR(clks[i]))
+                       pr_err("i.MX clk %u: register failed with %ld\n",
+                               i, PTR_ERR(clks[i]));

But we're OK to continue? Alright...

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 4 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +4 -0

+       }
+
+       return of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+ clk_data);

Why not use devm_? Or can the driver never be unbound from sysfs?

Quoted 10 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +10 -0

+}
+
+static const struct of_device_id imx8qxp_lpcg_match[] = {
+       { .compatible = "fsl,imx8qxp-lpcg-adma", &imx8qxp_ss_adma, },
+       { .compatible = "fsl,imx8qxp-lpcg-conn", &imx8qxp_ss_conn, },
+       { .compatible = "fsl,imx8qxp-lpcg-lsio", &imx8qxp_ss_lsio, },
+       { /* sentinel */ }
+};
+
+static struct platform_driver imx8qxp_lpcg_clk_driver = {

drivers/clk/imx/clk-imx8qxp-lpcg.c line 197

Quoted 10 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +10 -0

+       .driver = {
+               .name = "imx8qxp-lpcg-clk",
+               .of_match_table = imx8qxp_lpcg_match,
+       },
+       .probe = imx8qxp_lpcg_clk_probe, };
+
+builtin_platform_driver(imx8qxp_lpcg_clk_driver);
+
+MODULE_AUTHOR("Dong Aisheng <[email protected]>");
+MODULE_DESCRIPTION("IMX8QXP LPCG Clock driver");

MODULE_LICENSE("GPL

Quoted 1 line

+v2");

If the module is builtin all the time because of builtin_platform_driver() I think we don't need these MODULE_ macros.

Quoted 4 lines

-----Original Message-----
From: Stephen Boyd [mailto:sboyd at kernel.org]

Quoted 9 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c line 150

Quoted 7 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +7 -0

+
+       ss_lpcg = of_device_get_match_data(dev);
+       if (!ss_lpcg)
+               return -ENODEV;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap(dev, res->start, resource_size(res));

Why not devm_ioremap_resource()?

Because LPCGs are distributed in various subsystem memory space which may be overlapped with some IP modules. So we can't use devm_ioremap_resource which has a request_mem_region() in it.
It's something like what we've done for drivers/mfd/syscon.c.

Quoted 23 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 11 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +10 -0

+                               GFP_KERNEL);
+       if (!clk_data)
+               return -ENOMEM;
+
+       clk_data->num = ss_lpcg->num_max;
+       clks = clk_data->hws;
+
+       for (i = 0; i < ss_lpcg->num_lpcg; i++) {
+               lpcg = ss_lpcg->lpcg + i;
+               clks[lpcg->id] = imx_clk_lpcg_scu(lpcg->name,

lpcg->parent,

Quoted 1 line

+ lpcg->flags, base +

lpcg->offset,

Quoted 1 line

+ lpcg->bit_idx,

lpcg->hw_gate);

Quoted 6 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +6 -0

+       }
+
+       for (i = 0; i < clk_data->num; i++) {
+               if (IS_ERR(clks[i]))
+                       pr_err("i.MX clk %u: register failed with %ld\n",
+                               i, PTR_ERR(clks[i]));

Quoted 2 lines

But we're OK to continue? Alright...

Yes, probably better to use pr_warn()?

Quoted 8 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 5 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +4 -0

+       }
+
+       return of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+ clk_data);

Why not use devm_? Or can the driver never be unbound from sysfs?

We expect the driver to be never unbound from sysfs. Probably we'd better add .suppress_bind_attrs.

-----Original Message-----
From: Stephen Boyd [mailto:sboyd at kernel.org]

Quoted 10 lines

Quoted 9 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c line 150

Quoted 7 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +7 -0

+
+       ss_lpcg = of_device_get_match_data(dev);
+       if (!ss_lpcg)
+               return -ENODEV;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap(dev, res->start, resource_size(res));

Why not devm_ioremap_resource()?

Quoted 4 lines

Because LPCGs are distributed in various subsystem memory space which may be overlapped with some IP modules. So we can't use devm_ioremap_resource which has a request_mem_region() in it. It's something like what we've done for drivers/mfd/syscon.c.

Ok. Any chance that the various subsystems that are combined with the clk controller can be split up so that we can use devm_ioremap_resource() here?

It may be better to carve up the I/O space of
this SoC into device nodes that map to the hardware IP blocks, and then have those device drivers generate sub-devices for the various logical subsystems that the hardware IP spans. Or it could be a big driver that registers with multiple subsytems/frameworks from one device driver. This way, the logical structure of the linux kernel device driver design doesn't leak into the DT description of the SoC.

Quoted 14 lines

Quoted 14 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 11 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +10 -0

+                               GFP_KERNEL);
+       if (!clk_data)
+               return -ENOMEM;
+
+       clk_data->num = ss_lpcg->num_max;
+       clks = clk_data->hws;
+
+       for (i = 0; i < ss_lpcg->num_lpcg; i++) {
+               lpcg = ss_lpcg->lpcg + i;
+               clks[lpcg->id] = imx_clk_lpcg_scu(lpcg->name,

lpcg->parent,

Quoted 2 lines

+ lpcg->flags,
+ base +

Quoted 8 lines

Quoted 8 lines

lpcg->offset,

Quoted 1 line

+ lpcg->bit_idx,

lpcg->hw_gate);

Quoted 5 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +5 -0

+       }
+
+       for (i = 0; i < clk_data->num; i++) {
+               if (IS_ERR(clks[i]))
+                       pr_err("i.MX clk %u: register failed

with %ld\n",

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

+ i, PTR_ERR(clks[i]));

But we're OK to continue? Alright...

Yes, probably better to use pr_warn()?

Yes.

Quoted 11 lines

Quoted 8 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c

Quoted 5 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +4 -0

+       }
+
+       return of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+ clk_data);

Why not use devm_? Or can the driver never be unbound from sysfs?

We expect the driver to be never unbound from sysfs. Probably we'd better add .suppress_bind_attrs.

Ok. Please add it.

Quoted 2 lines

-----Original Message-----
From: Stephen Boyd [mailto:[email protected]]

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c line 150

Quoted 7 lines

drivers/clk/imx/clk-imx8qxp-lpcg.c new file +7 -0

+
+       ss_lpcg = of_device_get_match_data(dev);
+       if (!ss_lpcg)
+               return -ENODEV;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap(dev, res->start, resource_size(res));

Why not devm_ioremap_resource()?

Quoted 8 lines

Quoted 4 lines

Because LPCGs are distributed in various subsystem memory space which may be overlapped with some IP modules. So we can't use devm_ioremap_resource which has a request_mem_region() in it. It's something like what we've done for drivers/mfd/syscon.c.

Ok. Any chance that the various subsystems that are combined with the clk controller can be split up so that we can use devm_ioremap_resource() here?

We can achieve that by describing each LPCGS node in devicetree, but that would bloat the Devicetree seriously as we have hundreds of LPCGS.

This is similar as what we did for legacy SoCs, e.g MX7D. The difference is that on MX7D LPCGS are continuous. e.g.
Table 5-19. CCGR Mapping Table
Gating Register LPCG Enable Offset
CCM_CCGR4 sim_main 0x4040
CCM_CCGR5 sim_display 0x4050
...
CCM_CCGR168 iomux 0x4A80
CCM_CCGR169 iomux_lpsr 0x4A90
CCM_CCGR170 kpp 0x4AA0

But on MX8QXP/QM, they're not continuous anymore and distributed in each SS. And we probably want to push our IC designer to re-orgnize them into a continuous memory map again for new chips in the future.

So do you think we can start with this way for MX8 as well? Then we won't block MX8QXP booting up too long or probably could improve it Later.

Quoted 9 lines

It may be better to carve up the I/O space of
this SoC into device nodes that map to the hardware IP blocks, and then have those device drivers generate sub-devices for the various logical subsystems that the hardware IP spans. Or it could be a big driver that registers with multiple subsytems/frameworks from one device driver. This way, the logical structure of the linux kernel device driver design doesn't leak into the DT description of the SoC.

Yes, we currently already describe them with each subsystems in Devicetree.
https://patchwork.kernel.org/patch/10677311/