-----Original Message-----
From: Stephen Boyd [mailto:sboyd@kernel.org]
Sent: Tuesday, December 4, 2018 3:45 AM
To: linux-clk@vger.kernel.org; Aisheng DONG <aisheng.dong@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com;
shawnguo@kernel.org; Fabio Estevam [off-list ref]; dl-linux-imx
[off-list ref]; kernel@pengutronix.de; Aisheng DONG
[off-list ref]
Subject: Re: [PATCH V8 6/7] clk: imx: add lpcg clock support

Quoting Aisheng DONG (2018-11-21 06:12:38)

quoted

diff --git a/drivers/clk/imx/clk-lpcg-scu.c

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

--- /dev/null
+++ b/drivers/clk/imx/clk-lpcg-scu.c

@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 NXP
+ *     Dong Aisheng <aisheng.dong@nxp.com>
+ */
+
+#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)
+
+/*
+ * struct clk_lpcg_scu - Description of LPCG clock
+ *
+ * @hw: clk_hw of this LPCG
+ * @reg: register of this LPCG clock
+ * @bit_idx: bit index of this LPCG clock
+ * @hw_gate: HW auto gate enable
+ *
+ * This structure describes one LPCG clock  */ struct clk_lpcg_scu {
+       struct clk_hw hw;
+       void __iomem *reg;
+       u8 bit_idx;
+       bool hw_gate;
+};
+
+#define to_clk_lpcg_scu(_hw) container_of(_hw, struct clk_lpcg_scu,
+hw)
+
+static int clk_lpcg_scu_enable(struct clk_hw *hw) {
+       struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw);
+       unsigned long flags;
+       u32 reg;
+
+       spin_lock_irqsave(&imx_lpcg_scu_lock, flags);
+
+       reg = readl(clk->reg);

Use readl_relaxed() here?

Yes, we can do it.

quoted

+       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);

Write this as:

	u32 val;

	val = CLK_GATE_SCU_LPCG_SW_SEL;
	if (...)
		val |= CLK_GATE_SCU_LPCG_HW_SEL;
	reg |= val << clk->bit_idx;

Yes, a smarter way.

quoted

+       writel(reg, clk->reg);

This can stay "stronger", so no writel_relaxed().

Got it.

quoted

+
+       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;
+       u32 reg;
+
+       spin_lock_irqsave(&imx_lpcg_scu_lock, flags);
+
+       reg = readl(clk->reg);

readl_relaxed()?

Got it.

Regards
Dong Aisheng

quoted

+       reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);

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