| Message ID | 20211011165707.138157-7-marcan@marcan.st (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Apple SoC CPU P-state switching | expand |
On 11/10/2021 18:57, Hector Martin wrote: > This driver binds to the memory controller hardware in Apple SoCs such > as the Apple M1, and provides a power domain that downstream devices can > use to change the performance state of the memory controller. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/memory/Kconfig | 9 +++ > drivers/memory/Makefile | 1 + > drivers/memory/apple-mcc.c | 130 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 140 insertions(+) > create mode 100644 drivers/memory/apple-mcc.c > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig > index 72c0df129d5c..48ef3d563a1c 100644 > --- a/drivers/memory/Kconfig > +++ b/drivers/memory/Kconfig > @@ -30,6 +30,15 @@ config ARM_PL172_MPMC > If you have an embedded system with an AMBA bus and a PL172 > controller, say Y or M here. > > +config APPLE_MCC > + tristate "Apple SoC MCC driver" > + default y if ARCH_APPLE > + select PM_GENERIC_DOMAINS > + depends on ARCH_APPLE || COMPILE_TEST > + help > + This driver manages performance tuning for the memory controller in > + Apple SoCs, such as the Apple M1. > + > config ATMEL_SDRAMC > bool "Atmel (Multi-port DDR-)SDRAM Controller" > default y if ARCH_AT91 > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile > index bc7663ed1c25..947840cbd2d4 100644 > --- a/drivers/memory/Makefile > +++ b/drivers/memory/Makefile > @@ -8,6 +8,7 @@ ifeq ($(CONFIG_DDR),y) > obj-$(CONFIG_OF) += of_memory.o > endif > obj-$(CONFIG_ARM_PL172_MPMC) += pl172.o > +obj-$(CONFIG_APPLE_MCC) += apple-mcc.o > obj-$(CONFIG_ATMEL_SDRAMC) += atmel-sdramc.o > obj-$(CONFIG_ATMEL_EBI) += atmel-ebi.o > obj-$(CONFIG_BRCMSTB_DPFE) += brcmstb_dpfe.o > diff --git a/drivers/memory/apple-mcc.c b/drivers/memory/apple-mcc.c > new file mode 100644 > index 000000000000..55959f034b9a > --- /dev/null > +++ b/drivers/memory/apple-mcc.c > @@ -0,0 +1,130 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SoC MCC memory controller performance control driver > + * > + * Copyright The Asahi Linux Contributors Copyright date? > + */ > + > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_domain.h> > +#include <linux/pm_opp.h> > + > +#define APPLE_MCC_PERF_CONFIG1 0xdc4 > +#define APPLE_MCC_PERF_CONFIG2 0xdbc > +#define APPLE_MCC_CHANNEL(x) ((x) * 0x40000) > + > +struct apple_mcc { > + struct device *dev; > + struct generic_pm_domain genpd; > + void __iomem *reg_base; > + u32 num_channels; > +}; > + > +#define to_apple_mcc(_genpd) container_of(_genpd, struct apple_mcc, genpd) > + > +static int apple_mcc_set_performance_state(struct generic_pm_domain *genpd, unsigned int state) > +{ > + struct apple_mcc *mcc = to_apple_mcc(genpd); > + struct dev_pm_opp *opp; > + struct device_node *np; > + u32 perf_config[2]; > + unsigned int i; > + > + dev_dbg(mcc->dev, "switching to perf state %d\n", state); > + > + opp = dev_pm_opp_find_level_exact(&mcc->genpd.dev, state); > + if (IS_ERR(opp)) > + return PTR_ERR(opp); > + > + np = dev_pm_opp_get_of_node(opp); > + if (of_property_read_u32_array(np, "apple,memory-perf-config", > + perf_config, ARRAY_SIZE(perf_config))) { > + dev_err(mcc->dev, "missing apple,memory-perf-config property"); > + of_node_put(np); > + return -EINVAL; > + } > + of_node_put(np); > + > + for (i = 0; i < mcc->num_channels; i++) { > + writel_relaxed(perf_config[0], > + mcc->reg_base + APPLE_MCC_CHANNEL(i) + APPLE_MCC_PERF_CONFIG1); > + writel_relaxed(perf_config[1], > + mcc->reg_base + APPLE_MCC_CHANNEL(i) + APPLE_MCC_PERF_CONFIG2); > + } > + > + return 0; > +} > + > +static unsigned int apple_mcc_opp_to_performance_state(struct generic_pm_domain *genpd, > + struct dev_pm_opp *opp) > +{ > + return dev_pm_opp_get_level(opp); > +} > + > +static int apple_mcc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; By convention mostly we call the variable "np". > + struct apple_mcc *mcc; > + int ret; > + > + mcc = devm_kzalloc(dev, sizeof(*mcc), GFP_KERNEL); > + if (!mcc) > + return -ENOMEM; > + > + mcc->reg_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(mcc->reg_base)) > + return PTR_ERR(mcc->reg_base); > + > + if (of_property_read_u32(node, "apple,num-channels", &mcc->num_channels)) { Don't you have a limit of supported channels? It cannot be any uint32... > + dev_err(dev, "missing apple,num-channels property\n"); Use almost everywhere dev_err_probe - less code and you get error msg printed. > + return -ENOENT; > + } > + > + mcc->dev = dev; > + mcc->genpd.name = "apple-mcc-perf"; > + mcc->genpd.opp_to_performance_state = apple_mcc_opp_to_performance_state; > + mcc->genpd.set_performance_state = apple_mcc_set_performance_state; > + > + ret = pm_genpd_init(&mcc->genpd, NULL, false); > + if (ret < 0) { > + dev_err(dev, "pm_genpd_init failed\n"); > + return ret; > + } > + > + ret = of_genpd_add_provider_simple_noclk(node, &mcc->genpd); > + if (ret < 0) { > + dev_err(dev, "of_genpd_add_provider_simple failed\n"); > + return ret; > + } > + > + dev_info(dev, "Apple MCC performance driver initialized\n"); Please skip it, or at least make it a dev_dbg, you don't print any valuable information here. > + > + return 0; > +} > + > +static const struct of_device_id apple_mcc_of_match[] = { > + { .compatible = "apple,mcc" }, > + {} > +}; > + > +MODULE_DEVICE_TABLE(of, apple_mcc_of_match); > + > +static struct platform_driver apple_mcc_driver = { > + .probe = apple_mcc_probe, > + .driver = { > + .name = "apple-mcc", > + .of_match_table = apple_mcc_of_match, > + }, > +}; module_platform_driver() goes here. > + > +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); > +MODULE_DESCRIPTION("MCC memory controller performance tuning driver for Apple SoCs"); > +MODULE_LICENSE("GPL v2"); I think this will be "Dual MIT/GPL", based on your SPDX. > + > +module_platform_driver(apple_mcc_driver); > Best regards, Krzysztof
On 12/10/2021 18.19, Krzysztof Kozlowski wrote: >> +// SPDX-License-Identifier: GPL-2.0-only OR MIT >> +/* >> + * Apple SoC MCC memory controller performance control driver >> + * >> + * Copyright The Asahi Linux Contributors > > Copyright date? We've gone over this one a few times already; most copyright dates quickly become outdated and meaningless :) See: https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/ >> +static int apple_mcc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *node = dev->of_node; > > By convention mostly we call the variable "np". Ack, I'll change it for v2. >> + mcc->reg_base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(mcc->reg_base)) >> + return PTR_ERR(mcc->reg_base); >> + >> + if (of_property_read_u32(node, "apple,num-channels", &mcc->num_channels)) { > > Don't you have a limit of supported channels? It cannot be any uint32... Today, it's max 8. But if come Monday we find out Apple's new chips have 16 channels and otherwise the same register layout, I'd much rather not have to change the driver... >> + dev_err(dev, "missing apple,num-channels property\n"); > > Use almost everywhere dev_err_probe - less code and you get error msg > printed. Heh, I didn't know about that one. Thanks! >> + >> + dev_info(dev, "Apple MCC performance driver initialized\n"); > > Please skip it, or at least make it a dev_dbg, you don't print any > valuable information here. Ack, I'll remove this. >> +static struct platform_driver apple_mcc_driver = { >> + .probe = apple_mcc_probe, >> + .driver = { >> + .name = "apple-mcc", >> + .of_match_table = apple_mcc_of_match, >> + }, >> +}; > > module_platform_driver() goes here. Ack, will fix for v2. > >> + >> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); >> +MODULE_DESCRIPTION("MCC memory controller performance tuning driver for Apple SoCs"); >> +MODULE_LICENSE("GPL v2"); > > I think this will be "Dual MIT/GPL", based on your SPDX. Ah, I didn't realize that was a valid option for MODULE_LICENSE. I guess anything containing "GPL" works with EXPORT_SYMBOL_GPL? Thanks for the review!
On 14/10/2021 08:59, Hector Martin wrote: > On 12/10/2021 18.19, Krzysztof Kozlowski wrote: >>> +// SPDX-License-Identifier: GPL-2.0-only OR MIT >>> +/* >>> + * Apple SoC MCC memory controller performance control driver >>> + * >>> + * Copyright The Asahi Linux Contributors >> >> Copyright date? > > We've gone over this one a few times already; most copyright dates > quickly become outdated and meaningless :) > > See: > https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/ > >>> +static int apple_mcc_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct device_node *node = dev->of_node; >> >> By convention mostly we call the variable "np". > > Ack, I'll change it for v2. > >>> + mcc->reg_base = devm_platform_ioremap_resource(pdev, 0); >>> + if (IS_ERR(mcc->reg_base)) >>> + return PTR_ERR(mcc->reg_base); >>> + >>> + if (of_property_read_u32(node, "apple,num-channels", &mcc->num_channels)) { >> >> Don't you have a limit of supported channels? It cannot be any uint32... > > Today, it's max 8. But if come Monday we find out Apple's new chips have > 16 channels and otherwise the same register layout, I'd much rather not > have to change the driver... OK, however if the driver ever receives different DT with a different value, it will accept it unconditionally and go via address space. I am just saying that being conservative on received values is safer, but I am fine with skipping this problem. At the end we trust DT that it will always match the kernel, don't we? Oh wait, someone can use DT from other kernel in this one... > >>> + dev_err(dev, "missing apple,num-channels property\n"); >> >> Use almost everywhere dev_err_probe - less code and you get error msg >> printed. > > Heh, I didn't know about that one. Thanks! > >>> + >>> + dev_info(dev, "Apple MCC performance driver initialized\n"); >> >> Please skip it, or at least make it a dev_dbg, you don't print any >> valuable information here. > > Ack, I'll remove this. > >>> +static struct platform_driver apple_mcc_driver = { >>> + .probe = apple_mcc_probe, >>> + .driver = { >>> + .name = "apple-mcc", >>> + .of_match_table = apple_mcc_of_match, >>> + }, >>> +}; >> >> module_platform_driver() goes here. > > Ack, will fix for v2. > >> >>> + >>> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); >>> +MODULE_DESCRIPTION("MCC memory controller performance tuning driver for Apple SoCs"); >>> +MODULE_LICENSE("GPL v2"); >> >> I think this will be "Dual MIT/GPL", based on your SPDX. > > Ah, I didn't realize that was a valid option for MODULE_LICENSE. I guess > anything containing "GPL" works with EXPORT_SYMBOL_GPL? I don't think exporting symbols is related to how you license your code. Best regards, Krzysztof
On 14/10/2021 16.36, Krzysztof Kozlowski wrote: > On 14/10/2021 08:59, Hector Martin wrote: >>> Don't you have a limit of supported channels? It cannot be any uint32... >> >> Today, it's max 8. But if come Monday we find out Apple's new chips have >> 16 channels and otherwise the same register layout, I'd much rather not >> have to change the driver... > > OK, however if the driver ever receives different DT with a different > value, it will accept it unconditionally and go via address space. I am > just saying that being conservative on received values is safer, but I > am fine with skipping this problem. At the end we trust DT that it will > always match the kernel, don't we? Oh wait, someone can use DT from > other kernel in this one... DTs using these compatibles should have the same register layout, and should work with this driver; if a new chip comes out that has a different register layout we will change the compatibles (both) and therefore older kernels won't bind at all. If it has the same layout we'll keep the base compatible, `reg` will grow as needed to accomodate the extra channels, and e.g. num-channels=16 will then just work on older kernels with no changes. Obviously a broken DT with an insane value here would crash the driver, but so would any other number of crazy DT things; however, I don't expect that to ever happen. There's also the case where we end up with multiple memory controllers at discrete offsets (e.g. rumored multi-die configurations); in that case we'll end up with multiple genpd parents and have to add code to support that, and in the meantime older kernels will just have broken cpufreq on the p-cores. But I think that is ~acceptable as long as the system boots; we don't expect to be able to *fully* support newer SoCs on older kernels with no code changes. What I'm aiming for is just making the system work, hopefully with NVMe and USB and a dumb framebuffer, so that distro installers can run and then users can later install a proper up to date kernel will full support for the new SoC. >> Ah, I didn't realize that was a valid option for MODULE_LICENSE. I guess >> anything containing "GPL" works with EXPORT_SYMBOL_GPL? > > I don't think exporting symbols is related to how you license your code. It is; only modules with a GPL-compatible MODULE_LICENSE get to use symbols exported via EXPORT_SYMBOL_GPL. See kernel/module.c for the symbol lookup logic and include/linux/license.h for the logic to check the string (seems like "Dual MIT/GPL" is explicitly whitelisted there). Of course, this is a futile effort, as ~every time I see a proprietary module in some embedded device, it either falsely declares itself to be GPL, or they have a shim module that re-exports GPL symbols as non-GPL.
On 14/10/2021 09:52, Hector Martin wrote: > On 14/10/2021 16.36, Krzysztof Kozlowski wrote: (...) > >>> Ah, I didn't realize that was a valid option for MODULE_LICENSE. I guess >>> anything containing "GPL" works with EXPORT_SYMBOL_GPL? >> >> I don't think exporting symbols is related to how you license your code. > > It is; only modules with a GPL-compatible MODULE_LICENSE get to use > symbols exported via EXPORT_SYMBOL_GPL. Although there might be such correlation but it's not a rule. You can have a GPL module exporting symbols without GPL requirement (EXPORT_SYMBOLS). You can have a GPL+MIT module exporting symbols as GPL. Obviously you cannot have a non-GPL module, as we do not accept these and there is no such choice. So answering your question that "GPL" works with EXPORT_SYMBOL_GPL - everything is GPL but it works with both EXPORT_SYMBOL and EXPORT_SYMBOL_GPL. > > See kernel/module.c for the symbol lookup logic and > include/linux/license.h for the logic to check the string (seems like > "Dual MIT/GPL" is explicitly whitelisted there). Not related to export symbol. It is used for determining the tainted kernel via other licenses. > > Of course, this is a futile effort, as ~every time I see a proprietary > module in some embedded device, it either falsely declares itself to be > GPL, or they have a shim module that re-exports GPL symbols as non-GPL. > This is being removed soon (or already). Best regards, Krzysztof
On 14/10/2021 17.04, Krzysztof Kozlowski wrote: >> It is; only modules with a GPL-compatible MODULE_LICENSE get to use >> symbols exported via EXPORT_SYMBOL_GPL. > > Although there might be such correlation but it's not a rule. You can > have a GPL module exporting symbols without GPL requirement > (EXPORT_SYMBOLS). You can have a GPL+MIT module exporting symbols as > GPL. Obviously you cannot have a non-GPL module, as we do not accept > these and there is no such choice. What I mean is that modules can only import GPL symbols if they themselves are GPL compatible. What I didn't know is that "Dual MIT/GPL" is a valid string for MODULE_LICENSE to qualify as such. >> See kernel/module.c for the symbol lookup logic and >> include/linux/license.h for the logic to check the string (seems like >> "Dual MIT/GPL" is explicitly whitelisted there). > > Not related to export symbol. It is used for determining the tainted > kernel via other licenses. > Not just that; that module taint is used as a filter so that non-GPL-compatible modules are technically prevented from resolving EXPORT_SYMBOL_GPL symbols. >> Of course, this is a futile effort, as ~every time I see a proprietary >> module in some embedded device, it either falsely declares itself to be >> GPL, or they have a shim module that re-exports GPL symbols as non-GPL. >> > > This is being removed soon (or already). ? Good luck getting proprietary embedded vendors to start following licenses... :)
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig index 72c0df129d5c..48ef3d563a1c 100644 --- a/drivers/memory/Kconfig +++ b/drivers/memory/Kconfig @@ -30,6 +30,15 @@ config ARM_PL172_MPMC If you have an embedded system with an AMBA bus and a PL172 controller, say Y or M here. +config APPLE_MCC + tristate "Apple SoC MCC driver" + default y if ARCH_APPLE + select PM_GENERIC_DOMAINS + depends on ARCH_APPLE || COMPILE_TEST + help + This driver manages performance tuning for the memory controller in + Apple SoCs, such as the Apple M1. + config ATMEL_SDRAMC bool "Atmel (Multi-port DDR-)SDRAM Controller" default y if ARCH_AT91 diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile index bc7663ed1c25..947840cbd2d4 100644 --- a/drivers/memory/Makefile +++ b/drivers/memory/Makefile @@ -8,6 +8,7 @@ ifeq ($(CONFIG_DDR),y) obj-$(CONFIG_OF) += of_memory.o endif obj-$(CONFIG_ARM_PL172_MPMC) += pl172.o +obj-$(CONFIG_APPLE_MCC) += apple-mcc.o obj-$(CONFIG_ATMEL_SDRAMC) += atmel-sdramc.o obj-$(CONFIG_ATMEL_EBI) += atmel-ebi.o obj-$(CONFIG_BRCMSTB_DPFE) += brcmstb_dpfe.o diff --git a/drivers/memory/apple-mcc.c b/drivers/memory/apple-mcc.c new file mode 100644 index 000000000000..55959f034b9a --- /dev/null +++ b/drivers/memory/apple-mcc.c @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Apple SoC MCC memory controller performance control driver + * + * Copyright The Asahi Linux Contributors + */ + +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_domain.h> +#include <linux/pm_opp.h> + +#define APPLE_MCC_PERF_CONFIG1 0xdc4 +#define APPLE_MCC_PERF_CONFIG2 0xdbc +#define APPLE_MCC_CHANNEL(x) ((x) * 0x40000) + +struct apple_mcc { + struct device *dev; + struct generic_pm_domain genpd; + void __iomem *reg_base; + u32 num_channels; +}; + +#define to_apple_mcc(_genpd) container_of(_genpd, struct apple_mcc, genpd) + +static int apple_mcc_set_performance_state(struct generic_pm_domain *genpd, unsigned int state) +{ + struct apple_mcc *mcc = to_apple_mcc(genpd); + struct dev_pm_opp *opp; + struct device_node *np; + u32 perf_config[2]; + unsigned int i; + + dev_dbg(mcc->dev, "switching to perf state %d\n", state); + + opp = dev_pm_opp_find_level_exact(&mcc->genpd.dev, state); + if (IS_ERR(opp)) + return PTR_ERR(opp); + + np = dev_pm_opp_get_of_node(opp); + if (of_property_read_u32_array(np, "apple,memory-perf-config", + perf_config, ARRAY_SIZE(perf_config))) { + dev_err(mcc->dev, "missing apple,memory-perf-config property"); + of_node_put(np); + return -EINVAL; + } + of_node_put(np); + + for (i = 0; i < mcc->num_channels; i++) { + writel_relaxed(perf_config[0], + mcc->reg_base + APPLE_MCC_CHANNEL(i) + APPLE_MCC_PERF_CONFIG1); + writel_relaxed(perf_config[1], + mcc->reg_base + APPLE_MCC_CHANNEL(i) + APPLE_MCC_PERF_CONFIG2); + } + + return 0; +} + +static unsigned int apple_mcc_opp_to_performance_state(struct generic_pm_domain *genpd, + struct dev_pm_opp *opp) +{ + return dev_pm_opp_get_level(opp); +} + +static int apple_mcc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *node = dev->of_node; + struct apple_mcc *mcc; + int ret; + + mcc = devm_kzalloc(dev, sizeof(*mcc), GFP_KERNEL); + if (!mcc) + return -ENOMEM; + + mcc->reg_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(mcc->reg_base)) + return PTR_ERR(mcc->reg_base); + + if (of_property_read_u32(node, "apple,num-channels", &mcc->num_channels)) { + dev_err(dev, "missing apple,num-channels property\n"); + return -ENOENT; + } + + mcc->dev = dev; + mcc->genpd.name = "apple-mcc-perf"; + mcc->genpd.opp_to_performance_state = apple_mcc_opp_to_performance_state; + mcc->genpd.set_performance_state = apple_mcc_set_performance_state; + + ret = pm_genpd_init(&mcc->genpd, NULL, false); + if (ret < 0) { + dev_err(dev, "pm_genpd_init failed\n"); + return ret; + } + + ret = of_genpd_add_provider_simple_noclk(node, &mcc->genpd); + if (ret < 0) { + dev_err(dev, "of_genpd_add_provider_simple failed\n"); + return ret; + } + + dev_info(dev, "Apple MCC performance driver initialized\n"); + + return 0; +} + +static const struct of_device_id apple_mcc_of_match[] = { + { .compatible = "apple,mcc" }, + {} +}; + +MODULE_DEVICE_TABLE(of, apple_mcc_of_match); + +static struct platform_driver apple_mcc_driver = { + .probe = apple_mcc_probe, + .driver = { + .name = "apple-mcc", + .of_match_table = apple_mcc_of_match, + }, +}; + +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); +MODULE_DESCRIPTION("MCC memory controller performance tuning driver for Apple SoCs"); +MODULE_LICENSE("GPL v2"); + +module_platform_driver(apple_mcc_driver);
This driver binds to the memory controller hardware in Apple SoCs such as the Apple M1, and provides a power domain that downstream devices can use to change the performance state of the memory controller. Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/memory/Kconfig | 9 +++ drivers/memory/Makefile | 1 + drivers/memory/apple-mcc.c | 130 +++++++++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+) create mode 100644 drivers/memory/apple-mcc.c