| Message ID | 20230827115033.935089-9-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | ARM: qcom: apq8064: support CPU frequency scaling | expand |
On 27.08.2023 13:50, Dmitry Baryshkov wrote: > Add a simple driver that handles scaling of L2 frequency and voltages. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- [...] > +MODULE_DESCRIPTION("Qualcomm Krait L2 scaling driver"); > +MODULE_LICENSE("GPL v2"); Checkpatch? Konrad
On Sun, Aug 27, 2023 at 02:50:18PM +0300, Dmitry Baryshkov wrote: > Add a simple driver that handles scaling of L2 frequency and voltages. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- [...] > +static const struct of_device_id krait_l2_match_table[] = { > + { .compatible = "qcom,krait-l2-cache" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, krait_l2_match_table); > + > +static struct platform_driver krait_l2_driver = { > + .probe = krait_l2_probe, > + .remove = krait_l2_remove, > + .driver = { > + .name = "qcom-krait-l2", > + .of_match_table = krait_l2_match_table, > + .sync_state = icc_sync_state, > + }, > +}; As I mentioned in the other thread, cache devices already have a struct device. Specifically, they have a struct device (no subclass) on the cpu_subsys bus type. So there should be no need for a platform device and second struct device. See drivers/acpi/processor_driver.c for an example. Or grep any use of "cpu_subsys". Rob
On Wed, 11 Oct 2023 at 18:49, Rob Herring <robh@kernel.org> wrote: > > On Sun, Aug 27, 2023 at 02:50:18PM +0300, Dmitry Baryshkov wrote: > > Add a simple driver that handles scaling of L2 frequency and voltages. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > [...] > > > +static const struct of_device_id krait_l2_match_table[] = { > > + { .compatible = "qcom,krait-l2-cache" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, krait_l2_match_table); > > + > > +static struct platform_driver krait_l2_driver = { > > + .probe = krait_l2_probe, > > + .remove = krait_l2_remove, > > + .driver = { > > + .name = "qcom-krait-l2", > > + .of_match_table = krait_l2_match_table, > > + .sync_state = icc_sync_state, > > + }, > > +}; > > As I mentioned in the other thread, cache devices already have a struct > device. Specifically, they have a struct device (no subclass) on the > cpu_subsys bus type. So there should be no need for a platform device > and second struct device. > > See drivers/acpi/processor_driver.c for an example. Or grep any use of > "cpu_subsys". Most likely you mean drivers/base/cacheinfo.c. I saw this code, I don't think it makes a good fit here. The cacheinfo devices provide information only, they are not tied to DT nodes in any way. cpu_subsys doesn't provide a way to match drivers with subsys devices in the non-ACPI case, etc. Moreover, the whole cacheinfo subsys is non-existing on arm32, there is no cacheinfo implementation there, thanks to the overall variety of architectures. Thus said, I don't think cacheinfo makes a good fit for the case of scaling L2 cache.
On Wed, Oct 11, 2023 at 1:20 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 11 Oct 2023 at 18:49, Rob Herring <robh@kernel.org> wrote: > > > > On Sun, Aug 27, 2023 at 02:50:18PM +0300, Dmitry Baryshkov wrote: > > > Add a simple driver that handles scaling of L2 frequency and voltages. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > > [...] > > > > > +static const struct of_device_id krait_l2_match_table[] = { > > > + { .compatible = "qcom,krait-l2-cache" }, > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(of, krait_l2_match_table); > > > + > > > +static struct platform_driver krait_l2_driver = { > > > + .probe = krait_l2_probe, > > > + .remove = krait_l2_remove, > > > + .driver = { > > > + .name = "qcom-krait-l2", > > > + .of_match_table = krait_l2_match_table, > > > + .sync_state = icc_sync_state, > > > + }, > > > +}; > > > > As I mentioned in the other thread, cache devices already have a struct > > device. Specifically, they have a struct device (no subclass) on the > > cpu_subsys bus type. So there should be no need for a platform device > > and second struct device. > > > > See drivers/acpi/processor_driver.c for an example. Or grep any use of > > "cpu_subsys". > > Most likely you mean drivers/base/cacheinfo.c. I saw this code, I > don't think it makes a good fit here. The cacheinfo devices provide > information only, they are not tied to DT nodes in any way. They are completely tied to DT nodes beyond L1. > cpu_subsys > doesn't provide a way to match drivers with subsys devices in the > non-ACPI case, etc. That's a 2 line addition to add DT support. > Moreover, the whole cacheinfo subsys is > non-existing on arm32, there is no cacheinfo implementation there, > thanks to the overall variety of architectures. Humm, well I don't think it would be too hard to add, but I won't ask you to do that. All the info comes from DT or can come from DT, so it should be just a matter of arm32 calling the cacheinfo init. > Thus said, I don't think cacheinfo makes a good fit for the case of > scaling L2 cache. I still disagree. It's not really cacheinfo. That is what creates the devices, but it's the cpu_subsys bus type. Why do you care that it is platform bus vs. cpu_subsys? On a separate issue, I'd propose you move this to drivers/cache/ instead of the dumping ground that is drivers/soc/. It's nothing more than a location to collect cache related drivers ATM because we seem to be accumulating more of them. Rob
HI Rob, Resurrecting old thread, but I think it's better as it has context. Added driver core maintainers, see discussion points below. On Wed, 11 Oct 2023 at 21:44, Rob Herring <robh@kernel.org> wrote: > > On Wed, Oct 11, 2023 at 1:20 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Wed, 11 Oct 2023 at 18:49, Rob Herring <robh@kernel.org> wrote: > > > > > > On Sun, Aug 27, 2023 at 02:50:18PM +0300, Dmitry Baryshkov wrote: > > > > Add a simple driver that handles scaling of L2 frequency and voltages. > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > > > [...] > > > > > > > +static const struct of_device_id krait_l2_match_table[] = { > > > > + { .compatible = "qcom,krait-l2-cache" }, > > > > + {} > > > > +}; > > > > +MODULE_DEVICE_TABLE(of, krait_l2_match_table); > > > > + > > > > +static struct platform_driver krait_l2_driver = { > > > > + .probe = krait_l2_probe, > > > > + .remove = krait_l2_remove, > > > > + .driver = { > > > > + .name = "qcom-krait-l2", > > > > + .of_match_table = krait_l2_match_table, > > > > + .sync_state = icc_sync_state, > > > > + }, > > > > +}; > > > > > > As I mentioned in the other thread, cache devices already have a struct > > > device. Specifically, they have a struct device (no subclass) on the > > > cpu_subsys bus type. So there should be no need for a platform device > > > and second struct device. > > > > > > See drivers/acpi/processor_driver.c for an example. Or grep any use of > > > "cpu_subsys". > > > > Most likely you mean drivers/base/cacheinfo.c. I saw this code, I > > don't think it makes a good fit here. The cacheinfo devices provide > > information only, they are not tied to DT nodes in any way. > > They are completely tied to DT nodes beyond L1. > > > cpu_subsys > > doesn't provide a way to match drivers with subsys devices in the > > non-ACPI case, etc. > > That's a 2 line addition to add DT support. > > > Moreover, the whole cacheinfo subsys is > > non-existing on arm32, there is no cacheinfo implementation there, > > thanks to the overall variety of architectures. > > Humm, well I don't think it would be too hard to add, but I won't ask > you to do that. All the info comes from DT or can come from DT, so it > should be just a matter of arm32 calling the cacheinfo init. > > > Thus said, I don't think cacheinfo makes a good fit for the case of > > scaling L2 cache. > > I still disagree. It's not really cacheinfo. That is what creates the > devices, but it's the cpu_subsys bus type. Why do you care that it is > platform bus vs. cpu_subsys? I finally found a timeslot to look at cacheinfo. I added support for arm32 cacheinfo (which is fine) and tried using cacheinfo devices for L2 driver mapping (the RFC has been posted at [1]). But after I actually tried using it for the L2 cache driver. I stumbled upon several issues, which I'd like to discuss before rushing to code. First, you supposed that cacheinfo devices land onto the cpu_subsys bus. However only actual CPU devices end up on cpu_subsys. CPU cache devices are created using cpu_device_create(), but despite its name they don't go to cpu_subsys. Second and more important, these devices are created without any attempt to share them. So on a 4-core system I have 4 distinct devices for L2 cache even though it is shared between all cores. root@qcom-armv7a:~# stat -c "%N %i" /sys/bus/cpu/devices/cpu*/cache/index2/level /sys/bus/cpu/devices/cpu0/cache/index2/level 15537 /sys/bus/cpu/devices/cpu1/cache/index2/level 15560 /sys/bus/cpu/devices/cpu2/cache/index2/level 15583 /sys/bus/cpu/devices/cpu3/cache/index2/level 15606 I think it makes sense to rework cacheinfo to create actual CPU cache devices (maybe having a separate cache bus). In my case it should become something like: cpu0-2-unified (shared between all 4 cores) cpu0-1-icache cpu0-1-dcache cpu1-1-icache cpu1-1-dcache ... I'm not sure if it's worth supporting more than one instance of the same kind per level (e.g. I think current cacheinfo has nothing against having two I-cache or two D-cache devices) The cpuN/cache/indexM should become symlinks to those cache devices. What do you think? [1] https://lore.kernel.org/linux-arm-msm/CAA8EJppCRzknaujKFyLa_i7x4UnX31YFSyjtux+zJ0harixrbA@mail.gmail.com > On a separate issue, I'd propose you move this to drivers/cache/ > instead of the dumping ground that is drivers/soc/. It's nothing more > than a location to collect cache related drivers ATM because we seem > to be accumulating more of them. I thought about reusing drivers/devfreq, it already has the Mediatek CCI driver.
On Wed, Jan 3, 2024 at 8:02 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > HI Rob, > > Resurrecting old thread, but I think it's better as it has context. > > Added driver core maintainers, see discussion points below. > > On Wed, 11 Oct 2023 at 21:44, Rob Herring <robh@kernel.org> wrote: > > > > On Wed, Oct 11, 2023 at 1:20 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Wed, 11 Oct 2023 at 18:49, Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Sun, Aug 27, 2023 at 02:50:18PM +0300, Dmitry Baryshkov wrote: > > > > > Add a simple driver that handles scaling of L2 frequency and voltages. > > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > --- > > > > > > > > [...] > > > > > > > > > +static const struct of_device_id krait_l2_match_table[] = { > > > > > + { .compatible = "qcom,krait-l2-cache" }, > > > > > + {} > > > > > +}; > > > > > +MODULE_DEVICE_TABLE(of, krait_l2_match_table); > > > > > + > > > > > +static struct platform_driver krait_l2_driver = { > > > > > + .probe = krait_l2_probe, > > > > > + .remove = krait_l2_remove, > > > > > + .driver = { > > > > > + .name = "qcom-krait-l2", > > > > > + .of_match_table = krait_l2_match_table, > > > > > + .sync_state = icc_sync_state, > > > > > + }, > > > > > +}; > > > > > > > > As I mentioned in the other thread, cache devices already have a struct > > > > device. Specifically, they have a struct device (no subclass) on the > > > > cpu_subsys bus type. So there should be no need for a platform device > > > > and second struct device. > > > > > > > > See drivers/acpi/processor_driver.c for an example. Or grep any use of > > > > "cpu_subsys". > > > > > > Most likely you mean drivers/base/cacheinfo.c. I saw this code, I > > > don't think it makes a good fit here. The cacheinfo devices provide > > > information only, they are not tied to DT nodes in any way. > > > > They are completely tied to DT nodes beyond L1. > > > > > cpu_subsys > > > doesn't provide a way to match drivers with subsys devices in the > > > non-ACPI case, etc. > > > > That's a 2 line addition to add DT support. > > > > > Moreover, the whole cacheinfo subsys is > > > non-existing on arm32, there is no cacheinfo implementation there, > > > thanks to the overall variety of architectures. > > > > Humm, well I don't think it would be too hard to add, but I won't ask > > you to do that. All the info comes from DT or can come from DT, so it > > should be just a matter of arm32 calling the cacheinfo init. > > > > > Thus said, I don't think cacheinfo makes a good fit for the case of > > > scaling L2 cache. > > > > I still disagree. It's not really cacheinfo. That is what creates the > > devices, but it's the cpu_subsys bus type. Why do you care that it is > > platform bus vs. cpu_subsys? > > I finally found a timeslot to look at cacheinfo. I added support for > arm32 cacheinfo (which is fine) and tried using cacheinfo devices for > L2 driver mapping (the RFC has been posted at [1]). > But after I actually tried using it for the L2 cache driver. I > stumbled upon several issues, which I'd like to discuss before rushing > to code. > > First, you supposed that cacheinfo devices land onto the cpu_subsys > bus. However only actual CPU devices end up on cpu_subsys. CPU cache > devices are created using cpu_device_create(), but despite its name > they don't go to cpu_subsys. > > Second and more important, these devices are created without any > attempt to share them. So on a 4-core system I have 4 distinct devices > for L2 cache even though it is shared between all cores. I wonder if that's because things are created in CPU hotplug callbacks and there might be ordering problems if cache devices are created in another code path. Also, I think on some PowerPC systems, CPUs can move to different L2 (or L3?) caches when hot unplugged and then plugged. So hotplug rescans everything. I don't think that would be a problem with this and PowerPC does its own scanning anyways. Just wanted you to be aware of the issue. > root@qcom-armv7a:~# stat -c "%N %i" /sys/bus/cpu/devices/cpu*/cache/index2/level > /sys/bus/cpu/devices/cpu0/cache/index2/level 15537 > /sys/bus/cpu/devices/cpu1/cache/index2/level 15560 > /sys/bus/cpu/devices/cpu2/cache/index2/level 15583 > /sys/bus/cpu/devices/cpu3/cache/index2/level 15606 > > I think it makes sense to rework cacheinfo to create actual CPU cache > devices (maybe having a separate cache bus). > In my case it should become something like: > > cpu0-2-unified (shared between all 4 cores) > cpu0-1-icache > cpu0-1-dcache > cpu1-1-icache > cpu1-1-dcache > ... > > I'm not sure if it's worth supporting more than one instance of the > same kind per level (e.g. I think current cacheinfo has nothing > against having two I-cache or two D-cache devices) Probably a safe assumption. Though I think old XScale CPUs had a 1K mini I-cache and the main L1 I-cache. I guess that's really an L0 cache though. > The cpuN/cache/indexM should become symlinks to those cache devices. > > What do you think? Seems like a good improvement to me if changing the current way doesn't cause an ABI issue. > [1] https://lore.kernel.org/linux-arm-msm/CAA8EJppCRzknaujKFyLa_i7x4UnX31YFSyjtux+zJ0harixrbA@mail.gmail.com > > > On a separate issue, I'd propose you move this to drivers/cache/ > > instead of the dumping ground that is drivers/soc/. It's nothing more > > than a location to collect cache related drivers ATM because we seem > > to be accumulating more of them. > > I thought about reusing drivers/devfreq, it already has the Mediatek CCI driver. Anywhere except drivers/misc/ would be an improvement over drivers/soc/. devfreq is more tied to interconnects than caches though. Rob
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 715348869d04..1f3040ef551d 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -70,6 +70,15 @@ config QCOM_LLCC SDM845. This provides interfaces to clients that use the LLCC. Say yes here to enable LLCC slice driver. +config QCOM_KRAIT_L2_CACHE + tristate "Qualcomm Krait L2 cache scaling" + depends on ARCH_QCOM && ARM || COMPILE_TEST + select INTERCONNECT + select INTERCONNECT_CLK + default ARM_QCOM_CPUFREQ_NVMEM + help + The driver for scaling the L2 cache frequency on Qualcomm Krait platforms. + config QCOM_KRYO_L2_ACCESSORS bool depends on (ARCH_QCOM || COMPILE_TEST) && ARM64 diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index bbca2e1e55bb..4d16e5cdd334 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -28,6 +28,7 @@ obj-$(CONFIG_QCOM_STATS) += qcom_stats.o obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o obj-$(CONFIG_QCOM_APR) += apr.o obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o +obj-$(CONFIG_QCOM_KRAIT_L2_CACHE) += krait-l2-cache.o obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o qcom_ice-objs += ice.o diff --git a/drivers/soc/qcom/krait-l2-cache.c b/drivers/soc/qcom/krait-l2-cache.c new file mode 100644 index 000000000000..fb0ca9f4797c --- /dev/null +++ b/drivers/soc/qcom/krait-l2-cache.c @@ -0,0 +1,160 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2023, Linaro Ltd. + */ + +#include <linux/clk.h> +#include <linux/interconnect-clk.h> +#include <linux/interconnect-provider.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pm_opp.h> +#include <linux/regulator/consumer.h> + +#include <dt-bindings/soc/qcom,krait-l2-cache.h> + +/* Random ID that doesn't clash with main qnoc and OSM */ +#define L2_MASTER_NODE 2000 + +static int krait_l2_set_one_supply(struct device *dev, + struct regulator *reg, + struct dev_pm_opp_supply *supply) +{ + int ret; + + dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, + supply->u_volt_min, supply->u_volt, supply->u_volt_max); + + ret = regulator_set_voltage_triplet(reg, + supply->u_volt_min, + supply->u_volt, + supply->u_volt_max); + if (ret) { + dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", + __func__, supply->u_volt_min, supply->u_volt, + supply->u_volt_max, ret); + return ret; + } + + return 0; +} + +/* vdd-mem and vdd-dig */ +#define NUM_SUPPLIES 2 +static int krait_l2_config_regulators(struct device *dev, + struct dev_pm_opp *old_opp, + struct dev_pm_opp *new_opp, + struct regulator **regulators, + unsigned int count) +{ + struct dev_pm_opp_supply supplies[NUM_SUPPLIES]; + unsigned long old_freq, freq; + int ret; + + if (WARN_ON_ONCE(count != NUM_SUPPLIES)) + return -EINVAL; + + ret = dev_pm_opp_get_supplies(new_opp, supplies); + if (WARN_ON(ret)) + return ret; + + old_freq = dev_pm_opp_get_freq(old_opp); + freq = dev_pm_opp_get_freq(new_opp); + + WARN_ON(!old_freq || !freq); + if (freq > old_freq) { + ret = krait_l2_set_one_supply(dev, regulators[0], &supplies[0]); + if (ret) + return ret; + + ret = krait_l2_set_one_supply(dev, regulators[1], &supplies[1]); + if (ret) { + dev_pm_opp_get_supplies(old_opp, supplies); + krait_l2_set_one_supply(dev, regulators[0], &supplies[0]); + + return ret; + } + } else { + ret = krait_l2_set_one_supply(dev, regulators[1], &supplies[1]); + if (ret) + return ret; + + ret = krait_l2_set_one_supply(dev, regulators[0], &supplies[0]); + if (ret) { + dev_pm_opp_get_supplies(old_opp, supplies); + krait_l2_set_one_supply(dev, regulators[1], &supplies[1]); + + return ret; + } + } + + return 0; +} + +static int krait_l2_probe(struct platform_device *pdev) +{ + struct dev_pm_opp_config krait_l2_cfg = { + .clk_names = (const char * const[]) { NULL, NULL }, + .config_regulators = krait_l2_config_regulators, + .regulator_names = (const char * const[]) { "vdd-mem", "vdd-dig", NULL }, + }; + struct icc_clk_data data[] = { + { .name = "l2", .opp = true }, + }; + + struct device *dev = &pdev->dev; + struct icc_provider *provider; + struct clk *clk; + int ret; + + clk = devm_clk_get(dev, NULL); + if (IS_ERR(clk)) + return PTR_ERR(clk); + + ret = devm_pm_opp_set_config(dev, &krait_l2_cfg); + if (ret) + return ret; + + ret = devm_pm_opp_of_add_table(dev); + if (ret) + return ret; + + data[0].clk = clk; + provider = icc_clk_register(dev, L2_MASTER_NODE, ARRAY_SIZE(data), data); + if (IS_ERR(provider)) + return PTR_ERR(provider); + + platform_set_drvdata(pdev, provider); + + return 0; +} + +static int krait_l2_remove(struct platform_device *pdev) +{ + struct icc_provider *provider = platform_get_drvdata(pdev); + + icc_clk_unregister(provider); + + return 0; +} + +static const struct of_device_id krait_l2_match_table[] = { + { .compatible = "qcom,krait-l2-cache" }, + {} +}; +MODULE_DEVICE_TABLE(of, krait_l2_match_table); + +static struct platform_driver krait_l2_driver = { + .probe = krait_l2_probe, + .remove = krait_l2_remove, + .driver = { + .name = "qcom-krait-l2", + .of_match_table = krait_l2_match_table, + .sync_state = icc_sync_state, + }, +}; + +module_platform_driver(krait_l2_driver); + +MODULE_DESCRIPTION("Qualcomm Krait L2 scaling driver"); +MODULE_LICENSE("GPL v2");
Add a simple driver that handles scaling of L2 frequency and voltages. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/soc/qcom/Kconfig | 9 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/krait-l2-cache.c | 160 ++++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+) create mode 100644 drivers/soc/qcom/krait-l2-cache.c