| Message ID | 20210524182745.22923-3-sven@svenpeter.dev |
|---|---|
| State | New |
| Headers | show |
| Series | Apple M1 clock gate driver | expand |
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. > +} > +
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 --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);
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