Hi,

Thanks for the review!

On Wed, May 26, 2021, at 05:09, Stephen Boyd wrote:

Quoting Sven Peter (2021-05-24 11:27:44)

quoted

Add a simple driver for gate clocks found on Apple SoCs. These don't
have any frequency associated with them and are only used to enable
access to MMIO regions of various peripherals.

Can we figure out what frequency they are clocking at?

I don't think so. I've written some more details about how Apple
uses the clocks in a reply to Rob, but the short version is that
these clock gates are only used as on/off switches. There are
separate clocks that actually have a frequency associated with
them.

quoted

Signed-off-by: Sven Peter <redacted>

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e80918be8e9c..ac987a8cf318 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig

@@ -245,6 +245,18 @@ config CLK_TWL6040
          McPDM. McPDM module is using the external bit clock on the McPDM bus
          as functional clock.
 
+config COMMON_CLK_APPLE
+       tristate "Clock driver for Apple platforms"
+       depends on ARCH_APPLE && COMMON_CLK

The COMMON_CLK depend is redundant. Please remove.

Removed for v2.

quoted

+       default ARCH_APPLE
+       help
+         Support for clock gates on Apple SoCs such as the M1.
+
+         These clock gates do not have a frequency associated with them and
+         are only used to power on/off various peripherals. Generally, a clock
+         gate needs to be enabled before the respective MMIO region can be
+         accessed.
+
 config COMMON_CLK_AXI_CLKGEN
        tristate "AXI clkgen driver"
        depends on HAS_IOMEM || COMPILE_TEST

diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c
new file mode 100644
index 000000000000..799e9269758f
--- /dev/null
+++ b/drivers/clk/clk-apple-gate.c

@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Apple SoC clock/power gating driver
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/clk.h>

Hopefully this can be droped.

quoted

+#include <linux/clkdev.h>

And this one too. clkdev is not modern.

Okay, will remove both headers (and also fix any code that uses them).

quoted

+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+
+#define CLOCK_TARGET_MODE_MASK 0x0f

APPLE_ prefix on all these?

Fixed for v2.

quoted

+#define CLOCK_TARGET_MODE(m) (((m)&0xf))
+#define CLOCK_ACTUAL_MODE_MASK 0xf0
+#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4)
+
+#define CLOCK_MODE_ENABLE 0xf
+#define CLOCK_MODE_DISABLE 0
+
+#define CLOCK_ENDISABLE_TIMEOUT 100
+
+struct apple_clk_gate {
+       struct clk_hw hw;
+       void __iomem *reg;
+};
+
+#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw)
+
+static int apple_clk_gate_endisable(struct clk_hw *hw, int enable)
+{
+       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
+       u32 reg;
+       u32 mode;
+
+       if (enable)
+               mode = CLOCK_MODE_ENABLE;
+       else
+               mode = CLOCK_MODE_DISABLE;
+
+       reg = readl(gate->reg);
+       reg &= ~CLOCK_TARGET_MODE_MASK;
+       reg |= CLOCK_TARGET_MODE(mode);
+       writel(reg, gate->reg);
+
+       return readl_poll_timeout_atomic(gate->reg, reg,
+                                        (reg & CLOCK_ACTUAL_MODE_MASK) ==
+                                                CLOCK_ACTUAL_MODE(mode),
+                                        1, CLOCK_ENDISABLE_TIMEOUT);

Maybe this

          return readl_poll_timeout_atomic(gate->reg, reg,
		   (reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(mode),
		   1, CLOCK_ENDISABLE_TIMEOUT);

at the least please don't break the == across two lines.

Ah, sorry. I ran clang-format at the end and didn't catch that line when
I did my final review.
I'll use your suggestion for v2.

quoted

+}
+
+static int apple_clk_gate_enable(struct clk_hw *hw)
+{
+       return apple_clk_gate_endisable(hw, 1);
+}
+
+static void apple_clk_gate_disable(struct clk_hw *hw)
+{
+       apple_clk_gate_endisable(hw, 0);
+}
+
+static int apple_clk_gate_is_enabled(struct clk_hw *hw)
+{
+       struct apple_clk_gate *gate = to_apple_clk_gate(hw);
+       u32 reg;
+
+       reg = readl(gate->reg);
+
+       if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE))

Any chance we can use FIELD_GET() and friends? Please don't do bit
shifting stuff inside conditionals as it just makes life more
complicated than it needs to be.

Absolutely, thanks for the hint. I didn't know about FIELD_GET and will
use it for v2.

quoted

+               return 1;
+       return 0;

How about just return <logic>?

Good point, fixed for v2.

quoted

+}
+
+static const struct clk_ops apple_clk_gate_ops = {
+       .enable = apple_clk_gate_enable,
+       .disable = apple_clk_gate_disable,
+       .is_enabled = apple_clk_gate_is_enabled,
+};
+
+static int apple_gate_clk_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct device_node *node = dev->of_node;
+       const struct clk_parent_data parent_data[] = {
+               { .index = 0 },
+       };

Yay thanks for not doing it the old way!

:)

quoted

+       struct apple_clk_gate *data;
+       struct clk_hw *hw;
+       struct clk_init_data init;
+       struct resource *res;
+       int num_parents;
+       int ret;
+
+       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       data->reg = devm_ioremap_resource(dev, res);
+       if (IS_ERR(data->reg))
+               return PTR_ERR(data->reg);
+
+       num_parents = of_clk_get_parent_count(node);

Oh no I spoke too soon.

:(

Sorry, will fix it for v2 to use the new way.

quoted

+       if (num_parents > 1) {
+               dev_err(dev, "clock supports at most one parent\n");
+               return -EINVAL;
+       }
+
+       init.name = dev->of_node->name;
+       init.ops = &apple_clk_gate_ops;
+       init.flags = 0;
+       init.parent_names = NULL;
+       init.parent_data = parent_data;
+       init.num_parents = num_parents;
+
+       data->hw.init = &init;
+       hw = &data->hw;
+
+       ret = devm_clk_hw_register(dev, hw);
+       if (ret)
+               return ret;
+
+       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);

It looks like a one clk per DT node binding which is not how we do it. I
see Rob has started that discussion on the binding so we can keep that
there.

quoted

+}
+

Thanks,


Sven

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