diff mbox series

[2/3] clk: add support for gate clocks on Apple SoCs

Message ID 20210524182745.22923-3-sven@svenpeter.dev (mailing list archive)
State New, archived
Headers show
Series Apple M1 clock gate driver | expand

Commit Message

Sven Peter May 24, 2021, 6:27 p.m. UTC
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.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 MAINTAINERS                  |   1 +
 drivers/clk/Kconfig          |  12 +++
 drivers/clk/Makefile         |   1 +
 drivers/clk/clk-apple-gate.c | 152 +++++++++++++++++++++++++++++++++++
 4 files changed, 166 insertions(+)
 create mode 100644 drivers/clk/clk-apple-gate.c

Comments

Stephen Boyd May 26, 2021, 3:09 a.m. UTC | #1
Quoting Sven Peter (2021-05-24 11:27:44)
> 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?

> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> 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.

> +       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.

> +#include <linux/clkdev.h>

And this one too. clkdev is not modern.

> +#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?

> +#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.

> +}
> +
> +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.

> +               return 1;
> +       return 0;

How about just return <logic>?

> +}
> +
> +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!

> +       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.

> +       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.

> +}
> +
Sven Peter May 30, 2021, 11:17 a.m. UTC | #2
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)
> > 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.


> > 
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > 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.

> 
> > +       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.
> 
> > +#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).

> 
> > +#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.

> 
> > +#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.

> 
> > +}
> > +
> > +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.

> 
> > +               return 1;
> > +       return 0;
> 
> How about just return <logic>?

Good point, fixed for v2.

> 
> > +}
> > +
> > +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!

:)

> 
> > +       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.

> 
> > +       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.
> 
> > +}
> > +
> 

Thanks,


Sven
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 59c026ce4d73..4b5d8e7a0fbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1657,6 +1657,7 @@  F:	Documentation/devicetree/bindings/arm/apple.yaml
 F:	Documentation/devicetree/bindings/clock/apple,gate-clock.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	arch/arm64/boot/dts/apple/
+F:	drivers/clk/clk-apple-gate.c
 F:	drivers/irqchip/irq-apple-aic.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 
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
+	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/Makefile b/drivers/clk/Makefile
index 5f06879d7fe9..ba73960694e3 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -18,6 +18,7 @@  endif
 
 # hardware specific clock types
 # please keep this section sorted lexicographically by file path name
+obj-$(CONFIG_COMMON_CLK_APPLE)		+= clk-apple-gate.o
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
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>
+#include <linux/clkdev.h>
+#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
+#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);
+}
+
+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))
+		return 1;
+	return 0;
+}
+
+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 },
+	};
+	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);
+	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);
+}
+
+static const struct of_device_id apple_gate_clk_of_match[] = {
+	{ .compatible = "apple,t8103-gate-clock" },
+	{ .compatible = "apple,gate-clock" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, apple_gate_clk_of_match);
+
+static struct platform_driver apple_gate_clkdriver = {
+	.probe = apple_gate_clk_probe,
+	.driver = {
+		.name = "apple-gate-clock",
+		.of_match_table = apple_gate_clk_of_match,
+	},
+};
+
+MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>");
+MODULE_DESCRIPTION("Clock gating driver for Apple SoCs");
+MODULE_LICENSE("GPL v2");
+
+module_platform_driver(apple_gate_clkdriver);