Hi all,
this series aims at adding cpufreq support to the Raspberry Pi family of
boards.
The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431
The series first factors out 'pllb' from clk-bcm2385 and creates a new
clk driver that operates it over RPi's firmware interface[1]. We are
forced to do so as the firmware 'owns' the pll and we're not allowed to
change through the register interface directly as we might race with the
over-temperature and under-voltage protections provided by the firmware.
Next it creates a minimal cpufreq driver that populates the CPU's opp
table, and registers cpufreq-dt. Which is needed as the firmware
controls the max and min frequencies available.
This was tested on a RPi3b+ and RPI2b which are the boards I have access
to. Until this is tested broadly the cpufreq driver takes care of
filtering out the rest of boards.
That's all,
kind regards,
Nicolas
[1] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
---
Changes since RFC:
- Addressed Viresh's comments in cpufreq driver
- Addressed Stefan's comments in both cpufreq & clk drivers
- Moved all firmware clk operations into it's own driver
Nicolas Saenz Julienne (4):
clk: bcm2835: remove pllb
clk: bcm283x: add driver interfacing with Raspberry Pi's firmware
clk: bcm2835: register Raspberry Pi's firmware clk device
cpufreq: add driver for Raspbery Pi
drivers/clk/bcm/Makefile | 1 +
drivers/clk/bcm/clk-bcm2835.c | 40 ++--
drivers/clk/bcm/clk-raspberrypi.c | 316 ++++++++++++++++++++++++++
drivers/cpufreq/Kconfig.arm | 8 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/raspberrypi-cpufreq.c | 84 +++++++
6 files changed, 423 insertions(+), 27 deletions(-)
create mode 100644 drivers/clk/bcm/clk-raspberrypi.c
create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c
--
2.21.0
Raspberry Pi's firmware controls this pll, we should use the firmware
interface to access it.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
drivers/clk/bcm/clk-bcm2835.c | 25 -------------------------
1 file changed, 25 deletions(-)
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index 770bb01f523e..ccb0319fc2e9 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -1651,31 +1651,6 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
.fixed_divider = 1,
.flags = CLK_SET_RATE_PARENT),
- /* PLLB is used for the ARM's clock. */
- [BCM2835_PLLB] = REGISTER_PLL(
- .name = "pllb",
- .cm_ctrl_reg = CM_PLLB,
- .a2w_ctrl_reg = A2W_PLLB_CTRL,
- .frac_reg = A2W_PLLB_FRAC,
- .ana_reg_base = A2W_PLLB_ANA0,
- .reference_enable_mask = A2W_XOSC_CTRL_PLLB_ENABLE,
- .lock_mask = CM_LOCK_FLOCKB,
-
- .ana = &bcm2835_ana_default,
-
- .min_rate = 600000000u,
- .max_rate = 3000000000u,
- .max_fb_rate = BCM2835_MAX_FB_RATE),
- [BCM2835_PLLB_ARM] = REGISTER_PLL_DIV(
- .name = "pllb_arm",
- .source_pll = "pllb",
- .cm_reg = CM_PLLB,
- .a2w_reg = A2W_PLLB_ARM,
- .load_mask = CM_PLLB_LOADARM,
- .hold_mask = CM_PLLB_HOLDARM,
- .fixed_divider = 1,
- .flags = CLK_SET_RATE_PARENT),
-
/*
* PLLC is the core PLL, used to drive the core VPU clock.
*
--
2.21.0
Registers clk-raspberrypi as a platform device as part of the driver's
probe sequence.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
drivers/clk/bcm/clk-bcm2835.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index ccb0319fc2e9..6f370e6bafed 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -2098,6 +2098,7 @@ static int bcm2835_mark_sdc_parent_critical(struct clk *sdc)
static int bcm2835_clk_probe(struct platform_device *pdev)
{
+ struct platform_device *rpi_fw_clk;
struct device *dev = &pdev->dev;
struct clk_hw **hws;
struct bcm2835_cprman *cprman;
@@ -2150,8 +2151,18 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
if (ret)
return ret;
- return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
- &cprman->onecell);
+ ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
+ &cprman->onecell);
+ if (ret)
+ return ret;
+
+ rpi_fw_clk = platform_device_register_data(NULL, "raspberrypi-clk", -1,
+ NULL, 0);
+ ret = PTR_ERR_OR_ZERO(rpi_fw_clk);
+ if (ret)
+ dev_err(dev, "Failed to create platform device, %d\n", ret);
+
+ return ret;
}
static const struct of_device_id bcm2835_clk_of_match[] = {
--
2.21.0
Raspberry Pi's firmware offers and interface though which update it's
clock's frequencies. This is specially useful in order to change the CPU
clock (pllb_arm) which is 'owned' by the firmware and we're unable to
scale using the register interface.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
Changes since RFC:
- Moved firmware interface into own driver
- Use of_find_compatible_node()
- Remove error message on rpi_firmware_get() failure
- Ratelimit messages on set_rate() failure
- Use __le32 on firmware interface definition
drivers/clk/bcm/Makefile | 1 +
drivers/clk/bcm/clk-raspberrypi.c | 316 ++++++++++++++++++++++++++++++
2 files changed, 317 insertions(+)
create mode 100644 drivers/clk/bcm/clk-raspberrypi.c
diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
index 002661d39128..07abe92df9d1 100644
--- a/drivers/clk/bcm/Makefile
+++ b/drivers/clk/bcm/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm21664.o
obj-$(CONFIG_COMMON_CLK_IPROC) += clk-iproc-armpll.o clk-iproc-pll.o clk-iproc-asiu.o
obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835-aux.o
+obj-$(CONFIG_ARCH_BCM2835) += clk-raspberrypi.o
obj-$(CONFIG_ARCH_BCM_53573) += clk-bcm53573-ilp.o
obj-$(CONFIG_CLK_BCM_CYGNUS) += clk-cygnus.o
obj-$(CONFIG_CLK_BCM_HR2) += clk-hr2.o
diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
new file mode 100644
index 000000000000..485c00288414
--- /dev/null
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Nicolas Saenz Julienne
+ */
+
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+#define RPI_FIRMWARE_ARM_CLK_ID 0x000000003
+
+#define RPI_FIRMWARE_STATE_ENABLE_BIT 0x1
+#define RPI_FIRMWARE_STATE_WAIT_BIT 0x2
+
+/*
+ * Even though the firmware interface alters 'pllb' the frequencies are
+ * provided as per 'pllb_arm'. We need to scale before passing them trough.
+ */
+#define RPI_FIRMWARE_PLLB_ARM_DIV_RATE 2
+
+#define A2W_PLL_FRAC_BITS 20
+
+struct raspberrypi_clk {
+ struct device *dev;
+ struct rpi_firmware *firmware;
+
+ unsigned long min_rate;
+ unsigned long max_rate;
+
+ struct clk_hw pllb;
+ struct clk_hw *pllb_arm;
+ struct clk_lookup *pllb_arm_lookup;
+};
+
+/*
+ * Structure of the message passed to Raspberry Pi's firmware in order to
+ * change clock rates. The 'disable_turbo' option is only available to the ARM
+ * clock (pllb) which we enable by default as turbo mode will alter multiple
+ * clocks at once.
+ *
+ * Even though we're able to access the clock registers directly we're bound to
+ * use the firmware interface as the firmware ultimately takes care of
+ * mitigating overheating/undervoltage situations and we would be changing
+ * frequencies behind his back.
+ *
+ * For more information on the firmware interface check:
+ * https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
+ */
+struct raspberrypi_firmware_prop {
+ __le32 id;
+ __le32 val;
+ __le32 disable_turbo;
+} __packed;
+
+static int raspberrypi_clock_property(struct rpi_firmware *firmware, u32 tag,
+ u32 clk, u32 *val)
+{
+ struct raspberrypi_firmware_prop msg = {
+ .id = clk,
+ .val = *val,
+ .disable_turbo = 1,
+ };
+ int ret;
+
+ ret = rpi_firmware_property(firmware, tag, &msg, sizeof(msg));
+ if (ret)
+ return ret;
+
+ *val = msg.val;
+
+ return 0;
+}
+
+static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
+{
+ struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
+ pllb);
+ u32 val = 0;
+ int ret;
+
+ ret = raspberrypi_clock_property(rpi->firmware,
+ RPI_FIRMWARE_GET_CLOCK_STATE,
+ RPI_FIRMWARE_ARM_CLK_ID, &val);
+ if (ret)
+ return 0;
+
+ return !!(val & RPI_FIRMWARE_STATE_ENABLE_BIT);
+}
+
+
+static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
+ pllb);
+ u32 val = 0;
+ int ret;
+
+ ret = raspberrypi_clock_property(rpi->firmware,
+ RPI_FIRMWARE_GET_CLOCK_RATE,
+ RPI_FIRMWARE_ARM_CLK_ID,
+ &val);
+ if (ret)
+ return ret;
+
+ return val * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+}
+
+static int raspberrypi_fw_pll_on(struct clk_hw *hw)
+{
+ struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
+ pllb);
+ u32 val;
+ int ret;
+
+ val = RPI_FIRMWARE_STATE_ENABLE_BIT | RPI_FIRMWARE_STATE_WAIT_BIT;
+
+ ret = raspberrypi_clock_property(rpi->firmware,
+ RPI_FIRMWARE_SET_CLOCK_STATE,
+ RPI_FIRMWARE_ARM_CLK_ID, &val);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
+ pllb);
+ u32 new_rate = rate / RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+ int ret;
+
+ ret = raspberrypi_clock_property(rpi->firmware,
+ RPI_FIRMWARE_SET_CLOCK_RATE,
+ RPI_FIRMWARE_ARM_CLK_ID,
+ &new_rate);
+ if (ret)
+ dev_err_ratelimited(rpi->dev, "Failed to change %s frequency: %d",
+ clk_hw_get_name(hw), ret);
+
+ return ret;
+}
+
+/*
+ * Sadly there is no firmware rate rounding interface. We borred it from
+ * clk-bcm2835.
+ */
+static long raspberrypi_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
+ pllb);
+ u64 div, final_rate;
+ u32 ndiv, fdiv;
+
+ rate = clamp(rate, rpi->min_rate, rpi->max_rate);
+
+ div = (u64)rate << A2W_PLL_FRAC_BITS;
+ do_div(div, *parent_rate);
+
+ ndiv = div >> A2W_PLL_FRAC_BITS;
+ fdiv = div & ((1 << A2W_PLL_FRAC_BITS) - 1);
+
+ /* We can't use rate directly as it would overflow */
+ final_rate = ((u64)*parent_rate * ((ndiv << A2W_PLL_FRAC_BITS) + fdiv));
+
+ return final_rate >> A2W_PLL_FRAC_BITS;
+}
+
+static void raspberrypi_fw_pll_off(struct clk_hw *hw)
+{
+ struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
+ pllb);
+ u32 val = RPI_FIRMWARE_STATE_WAIT_BIT;
+
+ raspberrypi_clock_property(rpi->firmware,
+ RPI_FIRMWARE_SET_CLOCK_STATE,
+ RPI_FIRMWARE_ARM_CLK_ID, &val);
+}
+
+static const struct clk_ops raspberrypi_firmware_pll_clk_ops = {
+ .is_prepared = raspberrypi_fw_pll_is_on,
+ .prepare = raspberrypi_fw_pll_on,
+ .unprepare = raspberrypi_fw_pll_off,
+ .recalc_rate = raspberrypi_fw_pll_get_rate,
+ .set_rate = raspberrypi_fw_pll_set_rate,
+ .round_rate = raspberrypi_pll_round_rate,
+};
+
+static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
+{
+ u32 min_rate = 0, max_rate = 0;
+ struct clk_init_data init;
+ int ret;
+
+ /* Get min & max rates set by the firmware */
+ ret = raspberrypi_clock_property(rpi->firmware,
+ RPI_FIRMWARE_GET_MIN_CLOCK_RATE,
+ RPI_FIRMWARE_ARM_CLK_ID,
+ &min_rate);
+ if (ret) {
+ dev_err(rpi->dev, "Failed to get %s min freq: %d\n",
+ init.name, ret);
+ return ret;
+ }
+
+ ret = raspberrypi_clock_property(rpi->firmware,
+ RPI_FIRMWARE_GET_MAX_CLOCK_RATE,
+ RPI_FIRMWARE_ARM_CLK_ID,
+ &max_rate);
+ if (ret) {
+ dev_err(rpi->dev, "Failed to get %s max freq: %d\n",
+ init.name, ret);
+ return ret;
+ }
+
+ dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n",
+ min_rate, max_rate);
+
+ rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+ rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+
+ memset(&init, 0, sizeof(init));
+
+ /* All of the PLLs derive from the external oscillator. */
+ init.parent_names = (const char *[]){ "osc" };
+ init.num_parents = 1;
+ init.name = "pllb";
+ init.ops = &raspberrypi_firmware_pll_clk_ops;
+ init.flags = CLK_GET_RATE_NOCACHE | CLK_IGNORE_UNUSED;
+
+ rpi->pllb.init = &init;
+
+ return devm_clk_hw_register(rpi->dev, &rpi->pllb);
+}
+
+static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
+{
+ rpi->pllb_arm = clk_hw_register_fixed_factor(rpi->dev,
+ "pllb_arm", "pllb",
+ CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
+ 1, 2);
+ if (IS_ERR(rpi->pllb_arm)) {
+ dev_err(rpi->dev, "Failed to initialize pllb_arm\n");
+ return PTR_ERR(rpi->pllb_arm);
+ }
+
+ rpi->pllb_arm_lookup = clkdev_hw_create(rpi->pllb_arm, NULL, "cpu0");
+ if (!rpi->pllb_arm_lookup) {
+ dev_err(rpi->dev, "Failed to initialize pllb_arm_lookup\n");
+ clk_hw_unregister_fixed_factor(rpi->pllb_arm);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int raspberrypi_clk_probe(struct platform_device *pdev)
+{
+ struct device_node *firmware_node;
+ struct device *dev = &pdev->dev;
+ struct rpi_firmware *firmware;
+ struct raspberrypi_clk *rpi;
+ int ret;
+
+ firmware_node = of_find_compatible_node(NULL, NULL,
+ "raspberrypi,bcm2835-firmware");
+ if (!firmware_node) {
+ dev_err(dev, "Missing firmware node\n");
+ return -ENOENT;
+ }
+
+ firmware = rpi_firmware_get(firmware_node);
+ of_node_put(firmware_node);
+ if (!firmware)
+ return -EPROBE_DEFER;
+
+ rpi = devm_kzalloc(dev, sizeof(*rpi), GFP_KERNEL);
+ if (!rpi)
+ return -ENOMEM;
+
+ rpi->dev = dev;
+ rpi->firmware = firmware;
+
+ ret = raspberrypi_register_pllb(rpi);
+ if (ret) {
+ dev_err(dev, "Failed to initialize pllb, %d\n", ret);
+ return ret;
+ }
+
+ ret = raspberrypi_register_pllb_arm(rpi);
+ if (ret) {
+ dev_err(dev, "Failed to initialize pllb_arm, %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct platform_driver raspberrypi_clk_driver = {
+ .driver = {
+ .name = "raspberrypi-clk",
+ },
+ .probe = raspberrypi_clk_probe,
+};
+builtin_platform_driver(raspberrypi_clk_driver);
+
+MODULE_AUTHOR("Nicolas Saenz Julienne <[email protected]>");
+MODULE_DESCRIPTION("Raspberry Pi firmware clock driver");
+MODULE_LICENSE("GPLv2");
--
2.21.0
Raspberry Pi's firmware offers and interface though which update it's
performance requirements. It allows us to request for specific runtime
frequencies, which the firmware might or might not respect, depending on
the firmware configuration and thermals.
As the maximum and minimum frequencies are configurable in the firmware
there is no way to know in advance their values. So the Raspberry Pi
cpufreq driver queries them, builds an opp frequency table to then
launch cpufreq-dt.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
Changes since RFC:
- Alphabetically ordered relevant stuff
- Updated Kconfig to select firmware interface
- Correctly unref clk_dev after use
- Remove all opps on failure
- Remove use of dev_pm_opp_set_sharing_cpus()
drivers/cpufreq/Kconfig.arm | 8 +++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/raspberrypi-cpufreq.c | 84 +++++++++++++++++++++++++++
3 files changed, 93 insertions(+)
create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index f8129edc145e..556d432cc826 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -133,6 +133,14 @@ config ARM_QCOM_CPUFREQ_HW
The driver implements the cpufreq interface for this HW engine.
Say Y if you want to support CPUFreq HW.
+config ARM_RASPBERRYPI_CPUFREQ
+ tristate "Raspberry Pi cpufreq support"
+ select RASPBERRYPI_FIRMWARE
+ help
+ This adds the CPUFreq driver for Raspberry Pi
+
+ If in doubt, say N.
+
config ARM_S3C_CPUFREQ
bool
help
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 689b26c6f949..121c1acb66c0 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW) += qcom-cpufreq-hw.o
obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o
+obj-$(CONFIG_ARM_RASPBERRYPI_CPUFREQ) += raspberrypi-cpufreq.o
obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c b/drivers/cpufreq/raspberrypi-cpufreq.c
new file mode 100644
index 000000000000..2b3a195a9d37
--- /dev/null
+++ b/drivers/cpufreq/raspberrypi-cpufreq.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Raspberry Pi cpufreq driver
+ *
+ * Copyright (C) 2019, Nicolas Saenz Julienne <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+
+static const struct of_device_id machines[] __initconst = {
+ { .compatible = "raspberrypi,3-model-b-plus" },
+ { .compatible = "raspberrypi,3-model-b" },
+ { .compatible = "raspberrypi,2-model-b" },
+ { /* sentinel */ }
+};
+
+static int __init raspberrypi_cpufreq_driver_init(void)
+{
+ struct platform_device *pdev;
+ struct device *cpu_dev;
+ unsigned long min, max;
+ unsigned long rate;
+ struct clk *clk;
+ int ret;
+
+ if (!of_match_node(machines, of_root))
+ return -ENODEV;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev) {
+ pr_err("Cannot get CPU for cpufreq driver\n");
+ return -ENODEV;
+ }
+
+ clk = clk_get(cpu_dev, 0);
+ if (IS_ERR(clk)) {
+ dev_err(cpu_dev, "Cannot get clock for CPU0\n");
+ return PTR_ERR(clk);
+ }
+
+ /*
+ * The max and min frequencies are configurable in the Raspberry Pi
+ * firmware, so we query them at runtime
+ */
+ min = clk_round_rate(clk, 0);
+ max = clk_round_rate(clk, ULONG_MAX);
+ clk_put(clk);
+
+ for (rate = min; rate < max; rate += 100000000) {
+ ret = dev_pm_opp_add(cpu_dev, rate, 0);
+ if (ret)
+ goto remove_opp;
+ }
+
+ ret = dev_pm_opp_add(cpu_dev, max, 0);
+ if (ret)
+ goto remove_opp;
+
+ pdev = platform_device_register_data(NULL, "cpufreq-dt", -1, NULL, 0);
+ ret = PTR_ERR_OR_ZERO(pdev);
+ if (ret) {
+ dev_err(cpu_dev, "Failed to create platform device, %d\n", ret);
+ goto remove_opp;
+ }
+
+ return 0;
+
+remove_opp:
+ dev_pm_opp_remove_all_dynamic(cpu_dev);
+
+ return ret;
+}
+
+late_initcall(raspberrypi_cpufreq_driver_init);
+
+MODULE_AUTHOR("Nicolas Saenz Julienne <[email protected]");
+MODULE_DESCRIPTION("Raspberry Pi cpufreq driver");
+MODULE_LICENSE("GPL v2");
--
2.21.0
Nicolas Saenz Julienne <[email protected]> writes:
> Registers clk-raspberrypi as a platform device as part of the driver's
> probe sequence.
Similar to how we have VCHI register platform devices for the services
VCHI provides, shouldn't we have the firmware driver register the device
for clk_raspberrypi? Or put the clk provider in the fw driver instead
of a separate driver (no opinion on my part).
Nicolas Saenz Julienne <[email protected]> writes:
> Raspberry Pi's firmware controls this pll, we should use the firmware
> interface to access it.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Acked-by: Eric Anholt <[email protected]>
If someone ever has a non-rpi 2835 to support, they can resurrect this.
Nicolas Saenz Julienne <[email protected]> writes:
> Raspberry Pi's firmware offers and interface though which update it's
> performance requirements. It allows us to request for specific runtime
> frequencies, which the firmware might or might not respect, depending on
> the firmware configuration and thermals.
>
> As the maximum and minimum frequencies are configurable in the firmware
> there is no way to know in advance their values. So the Raspberry Pi
> cpufreq driver queries them, builds an opp frequency table to then
> launch cpufreq-dt.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
>
> Changes since RFC:
> - Alphabetically ordered relevant stuff
> - Updated Kconfig to select firmware interface
> - Correctly unref clk_dev after use
> - Remove all opps on failure
> - Remove use of dev_pm_opp_set_sharing_cpus()
>
> drivers/cpufreq/Kconfig.arm | 8 +++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/raspberrypi-cpufreq.c | 84 +++++++++++++++++++++++++++
> 3 files changed, 93 insertions(+)
> create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index f8129edc145e..556d432cc826 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -133,6 +133,14 @@ config ARM_QCOM_CPUFREQ_HW
> The driver implements the cpufreq interface for this HW engine.
> Say Y if you want to support CPUFreq HW.
>
> +config ARM_RASPBERRYPI_CPUFREQ
> + tristate "Raspberry Pi cpufreq support"
> + select RASPBERRYPI_FIRMWARE
> + help
> + This adds the CPUFreq driver for Raspberry Pi
> +
> + If in doubt, say N.
> +
> config ARM_S3C_CPUFREQ
> bool
> help
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 689b26c6f949..121c1acb66c0 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW) += qcom-cpufreq-hw.o
> obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o
> +obj-$(CONFIG_ARM_RASPBERRYPI_CPUFREQ) += raspberrypi-cpufreq.o
> obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
> obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
> diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c b/drivers/cpufreq/raspberrypi-cpufreq.c
> new file mode 100644
> index 000000000000..2b3a195a9d37
> --- /dev/null
> +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Raspberry Pi cpufreq driver
> + *
> + * Copyright (C) 2019, Nicolas Saenz Julienne <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +
> +static const struct of_device_id machines[] __initconst = {
> + { .compatible = "raspberrypi,3-model-b-plus" },
> + { .compatible = "raspberrypi,3-model-b" },
> + { .compatible = "raspberrypi,2-model-b" },
> + { /* sentinel */ }
> +};
I think I'd skip the compatible string check here. The firmware's
clock-management should be well-tested by folks playing with clocking in
the downstream tree. There aren't any firmware differences in the
processing of these clock management packets, to my recollection.
Other than that, I'm happy with the series and would give it my
acked-by.
On Tue, 2019-06-04 at 17:00 -0700, Eric Anholt wrote:
> Nicolas Saenz Julienne <[email protected]> writes:
>
> > Registers clk-raspberrypi as a platform device as part of the driver's
> > probe sequence.
>
> Similar to how we have VCHI register platform devices for the services
> VCHI provides, shouldn't we have the firmware driver register the device
> for clk_raspberrypi? Or put the clk provider in the fw driver instead
> of a separate driver (no opinion on my part).
Makes sense to me, I'll move the platform driver registration into the firmware
driver.
Hi Eric,
On Tue, 2019-06-04 at 17:18 -0700, Eric Anholt wrote:
> Nicolas Saenz Julienne <[email protected]> writes:
>
> > Raspberry Pi's firmware offers and interface though which update it's
> > performance requirements. It allows us to request for specific runtime
> > frequencies, which the firmware might or might not respect, depending on
> > the firmware configuration and thermals.
> >
> > As the maximum and minimum frequencies are configurable in the firmware
> > there is no way to know in advance their values. So the Raspberry Pi
> > cpufreq driver queries them, builds an opp frequency table to then
> > launch cpufreq-dt.
> >
> > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > ---
> >
> > Changes since RFC:
> > - Alphabetically ordered relevant stuff
> > - Updated Kconfig to select firmware interface
> > - Correctly unref clk_dev after use
> > - Remove all opps on failure
> > - Remove use of dev_pm_opp_set_sharing_cpus()
> >
> > drivers/cpufreq/Kconfig.arm | 8 +++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/raspberrypi-cpufreq.c | 84 +++++++++++++++++++++++++++
> > 3 files changed, 93 insertions(+)
> > create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c
> >
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index f8129edc145e..556d432cc826 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -133,6 +133,14 @@ config ARM_QCOM_CPUFREQ_HW
> > The driver implements the cpufreq interface for this HW engine.
> > Say Y if you want to support CPUFreq HW.
> >
> > +config ARM_RASPBERRYPI_CPUFREQ
> > + tristate "Raspberry Pi cpufreq support"
> > + select RASPBERRYPI_FIRMWARE
> > + help
> > + This adds the CPUFreq driver for Raspberry Pi
> > +
> > + If in doubt, say N.
> > +
> > config ARM_S3C_CPUFREQ
> > bool
> > help
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index 689b26c6f949..121c1acb66c0 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -64,6 +64,7 @@ obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> > obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> > obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW) += qcom-cpufreq-hw.o
> > obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o
> > +obj-$(CONFIG_ARM_RASPBERRYPI_CPUFREQ) += raspberrypi-cpufreq.o
> > obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> > obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
> > obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
> > diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c
> > b/drivers/cpufreq/raspberrypi-cpufreq.c
> > new file mode 100644
> > index 000000000000..2b3a195a9d37
> > --- /dev/null
> > +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Raspberry Pi cpufreq driver
> > + *
> > + * Copyright (C) 2019, Nicolas Saenz Julienne <[email protected]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +
> > +static const struct of_device_id machines[] __initconst = {
> > + { .compatible = "raspberrypi,3-model-b-plus" },
> > + { .compatible = "raspberrypi,3-model-b" },
> > + { .compatible = "raspberrypi,2-model-b" },
> > + { /* sentinel */ }
> > +};
>
> I think I'd skip the compatible string check here. The firmware's
> clock-management should be well-tested by folks playing with clocking in
> the downstream tree. There aren't any firmware differences in the
> processing of these clock management packets, to my recollection.
Fair enough, I'll remove it.
> Other than that, I'm happy with the series and would give it my
> acked-by.
Thanks!
Regads,
Nicolas
Hi Nicolas,
Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> Hi all,
> this series aims at adding cpufreq support to the Raspberry Pi family of
> boards.
>
> The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431
>
> The series first factors out 'pllb' from clk-bcm2385 and creates a new
> clk driver that operates it over RPi's firmware interface[1]. We are
> forced to do so as the firmware 'owns' the pll and we're not allowed to
> change through the register interface directly as we might race with the
> over-temperature and under-voltage protections provided by the firmware.
it would be nice to preserve such design decision in the driver as a
comment, because the cover letter usually get lost.
>
> Next it creates a minimal cpufreq driver that populates the CPU's opp
> table, and registers cpufreq-dt. Which is needed as the firmware
> controls the max and min frequencies available.
I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
manually enable this drivers. During boot with Raspbian rootfs i'm
getting the following:
[ 1.177009] cpu cpu0: failed to get clock: -2
[ 1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
[ 1.192117] sdhci: Secure Digital Host Controller Interface driver
[ 1.198725] sdhci: Copyright(c) Pierre Ossman
[ 1.207005] Synopsys Designware Multimedia Card Interface Driver
[ 1.319936] sdhost-bcm2835 3f202000.mmc: loaded - DMA enabled (>1)
[ 1.326641] sdhci-pltfm: SDHCI platform and OF driver helper
[ 1.336568] ledtrig-cpu: registered to indicate activity on CPUs
[ 1.343713] usbcore: registered new interface driver usbhid
[ 1.350275] usbhid: USB HID core driver
[ 1.357639] bcm2835-mbox 3f00b880.mailbox: mailbox enabled
[ 1.367490] NET: Registered protocol family 10
[ 1.375013] Segment Routing with IPv6
[ 1.381696] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
[ 1.388980] NET: Registered protocol family 17
[ 1.395624] can: controller area network core (rev 20170425 abi 9)
[ 1.402358] NET: Registered protocol family 29
[ 1.408997] can: raw protocol (rev 20170425)
[ 1.415599] can: broadcast manager protocol (rev 20170425 t)
[ 1.422219] can: netlink gateway (rev 20170425) max_hops=1
[ 1.429369] Key type dns_resolver registered
[ 1.437190] Registering SWP/SWPB emulation handler
[ 1.444443] Loading compiled-in X.509 certificates
[ 1.455693] 3f201000.serial: ttyAMA0 at MMIO 0x3f201000 (irq = 81,
base_baud = 0) is a PL011 rev2
[ 1.462768] serial serial0: tty port ttyAMA0 registered
[ 1.478755] mmc0: host does not support reading read-only switch,
assuming write-enable
[ 1.488792] mmc0: new high speed SDHC card at address 0007
[ 1.495766] raspberrypi-firmware soc:firmware: Attached to firmware
from 2019-03-27 15:45
[ 1.496862] mmcblk0: mmc0:0007 SDCIT 14.6 GiB
[ 1.512768] raspberrypi-clk raspberrypi-clk: CPU frequency range: min
600000000, max 1400000000
[ 1.513012] mmcblk0: p1 p2
[ 1.558085] dwc2 3f980000.usb: 3f980000.usb supply vusb_d not found,
using dummy regulator
[ 1.565355] dwc2 3f980000.usb: 3f980000.usb supply vusb_a not found,
using dummy regulator
[ 1.623246] dwc2 3f980000.usb: DWC OTG Controller
[ 1.630318] dwc2 3f980000.usb: new USB bus registered, assigned bus
number 1
[ 1.637439] dwc2 3f980000.usb: irq 33, io mem 0x3f980000
[ 1.645268] hub 1-0:1.0: USB hub found
[ 1.652317] hub 1-0:1.0: 1 port detected
[ 1.665867] sdhci-iproc 3f300000.sdhci: allocated mmc-pwrseq
[ 1.704788] mmc1: SDHCI controller on 3f300000.sdhci [3f300000.sdhci]
using PIO
[ 1.717694] hctosys: unable to open rtc device (rtc0)
[ 1.724967] sysfs: cannot create duplicate filename
'/devices/platform/cpufreq-dt'
[ 1.732120] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
5.2.0-rc1-00004-g5aa6d98-dirty #2
[ 1.739288] Hardware name: BCM2835
[ 1.746421] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[ 1.753636] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[ 1.760840] [<c0e7d358>] (dump_stack) from [<c0503b64>]
(sysfs_warn_dup+0x58/0x64)
[ 1.768105] [<c0503b64>] (sysfs_warn_dup) from [<c0503c8c>]
(sysfs_create_dir_ns+0xd8/0xe8)
[ 1.775481] [<c0503c8c>] (sysfs_create_dir_ns) from [<c0e82520>]
(kobject_add_internal+0xb0/0x2fc)
[ 1.782958] [<c0e82520>] (kobject_add_internal) from [<c0e827c8>]
(kobject_add+0x5c/0xc0)
[ 1.790534] [<c0e827c8>] (kobject_add) from [<c096b1cc>]
(device_add+0xf8/0x608)
[ 1.798180] [<c096b1cc>] (device_add) from [<c0971098>]
(platform_device_add+0x110/0x214)
[ 1.805945] [<c0971098>] (platform_device_add) from [<c0971ae4>]
(platform_device_register_full+0x130/0x148)
[ 1.813866] [<c0971ae4>] (platform_device_register_full) from
[<c15a7a30>] (raspberrypi_cpufreq_driver_init+0x128/0x178)
[ 1.821916] [<c15a7a30>] (raspberrypi_cpufreq_driver_init) from
[<c0302eec>] (do_one_initcall+0x54/0x21c)
[ 1.830099] [<c0302eec>] (do_one_initcall) from [<c15010f8>]
(kernel_init_freeable+0x244/0x2e0)
[ 1.838312] [<c15010f8>] (kernel_init_freeable) from [<c0e949d4>]
(kernel_init+0x8/0x10c)
[ 1.846541] [<c0e949d4>] (kernel_init) from [<c03010e8>]
(ret_from_fork+0x14/0x2c)
[ 1.854783] Exception stack(0xea89dfb0 to 0xea89dff8)
[ 1.863036] dfa0: 00000000
00000000 00000000 00000000
[ 1.871450] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 1.879860] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 1.888251] kobject_add_internal failed for cpufreq-dt with -EEXIST,
don't try to register things with the same name in the same directory.
[ 1.896910] cpu cpu0: Failed to create platform device, -17
>
> This was tested on a RPi3b+ and RPI2b which are the boards I have access
> to. Until this is tested broadly the cpufreq driver takes care of
> filtering out the rest of boards.
Unfortunately this makes it harder to test on other boards. So i welcome
your decision to remove it.
>
> That's all,
> kind regards,
> Nicolas
>
> [1] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
>
> ---
>
> Changes since RFC:
> - Addressed Viresh's comments in cpufreq driver
> - Addressed Stefan's comments in both cpufreq & clk drivers
Just a note for the future, this make it hard for other reviewers to
follow. I don't really consist of the credits, it is more important to
mention what has changed. But it's not necessary to mention every single
change.
Am 05.06.19 um 11:11 schrieb Nicolas Saenz Julienne:
> On Tue, 2019-06-04 at 17:00 -0700, Eric Anholt wrote:
>> Nicolas Saenz Julienne <[email protected]> writes:
>>
>>> Registers clk-raspberrypi as a platform device as part of the driver's
>>> probe sequence.
>> Similar to how we have VCHI register platform devices for the services
>> VCHI provides, shouldn't we have the firmware driver register the device
>> for clk_raspberrypi? Or put the clk provider in the fw driver instead
>> of a separate driver (no opinion on my part).
> Makes sense to me, I'll move the platform driver registration into the firmware
> driver.
Fine. Please keep in mind that you might need to add a MODULE_ALIAS
otherwise autoload won't work.
Hi Nicolas,
Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> Raspberry Pi's firmware offers and interface though which update it's
> clock's frequencies. This is specially useful in order to change the CPU
> clock (pllb_arm) which is 'owned' by the firmware and we're unable to
> scale using the register interface.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
>
> Changes since RFC:
> - Moved firmware interface into own driver
> - Use of_find_compatible_node()
> - Remove error message on rpi_firmware_get() failure
> - Ratelimit messages on set_rate() failure
> - Use __le32 on firmware interface definition
>
> drivers/clk/bcm/Makefile | 1 +
> drivers/clk/bcm/clk-raspberrypi.c | 316 ++++++++++++++++++++++++++++++
> 2 files changed, 317 insertions(+)
> create mode 100644 drivers/clk/bcm/clk-raspberrypi.c
>
> diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
> index 002661d39128..07abe92df9d1 100644
> --- a/drivers/clk/bcm/Makefile
> +++ b/drivers/clk/bcm/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm21664.o
> obj-$(CONFIG_COMMON_CLK_IPROC) += clk-iproc-armpll.o clk-iproc-pll.o clk-iproc-asiu.o
> obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o
> obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835-aux.o
> +obj-$(CONFIG_ARCH_BCM2835) += clk-raspberrypi.o
Hm, on the one side it would be nice to avoid building this driver in
case the firmware driver is disabled on the other side it would be good
to keep compile test.
> obj-$(CONFIG_ARCH_BCM_53573) += clk-bcm53573-ilp.o
> obj-$(CONFIG_CLK_BCM_CYGNUS) += clk-cygnus.o
> obj-$(CONFIG_CLK_BCM_HR2) += clk-hr2.o
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> new file mode 100644
> index 000000000000..485c00288414
> --- /dev/null
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -0,0 +1,316 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Nicolas Saenz Julienne
> + */
> +
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define RPI_FIRMWARE_ARM_CLK_ID 0x000000003
> +
> +#define RPI_FIRMWARE_STATE_ENABLE_BIT 0x1
> +#define RPI_FIRMWARE_STATE_WAIT_BIT 0x2
how about using the BIT() macro?
> +
> +/*
> + * Even though the firmware interface alters 'pllb' the frequencies are
> + * provided as per 'pllb_arm'. We need to scale before passing them trough.
> + */
> +#define RPI_FIRMWARE_PLLB_ARM_DIV_RATE 2
> +
> +#define A2W_PLL_FRAC_BITS 20
> +
> +struct raspberrypi_clk {
> + struct device *dev;
> + struct rpi_firmware *firmware;
> +
> + unsigned long min_rate;
> + unsigned long max_rate;
> +
> + struct clk_hw pllb;
> + struct clk_hw *pllb_arm;
> + struct clk_lookup *pllb_arm_lookup;
> +};
> +
> +/*
> + * Structure of the message passed to Raspberry Pi's firmware in order to
> + * change clock rates. The 'disable_turbo' option is only available to the ARM
> + * clock (pllb) which we enable by default as turbo mode will alter multiple
> + * clocks at once.
> + *
> + * Even though we're able to access the clock registers directly we're bound to
> + * use the firmware interface as the firmware ultimately takes care of
> + * mitigating overheating/undervoltage situations and we would be changing
> + * frequencies behind his back.
> + *
> + * For more information on the firmware interface check:
> + * https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
> + */
> +struct raspberrypi_firmware_prop {
> + __le32 id;
> + __le32 val;
> + __le32 disable_turbo;
> +} __packed;
> +
> +static int raspberrypi_clock_property(struct rpi_firmware *firmware, u32 tag,
> + u32 clk, u32 *val)
> +{
> + struct raspberrypi_firmware_prop msg = {
> + .id = clk,
> + .val = *val,
> + .disable_turbo = 1,
> + };
> + int ret;
> +
> + ret = rpi_firmware_property(firmware, tag, &msg, sizeof(msg));
> + if (ret)
> + return ret;
> +
> + *val = msg.val;
> +
> + return 0;
> +}
> +
> +static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
> +{
> + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> + pllb);
> + u32 val = 0;
> + int ret;
> +
> + ret = raspberrypi_clock_property(rpi->firmware,
> + RPI_FIRMWARE_GET_CLOCK_STATE,
> + RPI_FIRMWARE_ARM_CLK_ID, &val);
> + if (ret)
> + return 0;
> +
> + return !!(val & RPI_FIRMWARE_STATE_ENABLE_BIT);
> +}
> +
> +
> +static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> + pllb);
> + u32 val = 0;
> + int ret;
> +
> + ret = raspberrypi_clock_property(rpi->firmware,
> + RPI_FIRMWARE_GET_CLOCK_RATE,
> + RPI_FIRMWARE_ARM_CLK_ID,
> + &val);
> + if (ret)
> + return ret;
> +
> + return val * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
> +}
> +
> +static int raspberrypi_fw_pll_on(struct clk_hw *hw)
> +{
> + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> + pllb);
> + u32 val;
> + int ret;
> +
> + val = RPI_FIRMWARE_STATE_ENABLE_BIT | RPI_FIRMWARE_STATE_WAIT_BIT;
> +
> + ret = raspberrypi_clock_property(rpi->firmware,
> + RPI_FIRMWARE_SET_CLOCK_STATE,
> + RPI_FIRMWARE_ARM_CLK_ID, &val);
> + if (ret)
> + return ret;
> +
> + return 0;
return ret;
> +}
> +
> +static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> + pllb);
> + u32 new_rate = rate / RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
> + int ret;
> +
> + ret = raspberrypi_clock_property(rpi->firmware,
> + RPI_FIRMWARE_SET_CLOCK_RATE,
> + RPI_FIRMWARE_ARM_CLK_ID,
> + &new_rate);
> + if (ret)
> + dev_err_ratelimited(rpi->dev, "Failed to change %s frequency: %d",
> + clk_hw_get_name(hw), ret);
> +
> + return ret;
> +}
> +
> +/*
> + * Sadly there is no firmware rate rounding interface. We borred it from
borrowed?
> + * clk-bcm2835.
> + */
> +static long raspberrypi_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> + pllb);
> + u64 div, final_rate;
> + u32 ndiv, fdiv;
> +
> + rate = clamp(rate, rpi->min_rate, rpi->max_rate);
> +
> + div = (u64)rate << A2W_PLL_FRAC_BITS;
> + do_div(div, *parent_rate);
> +
> + ndiv = div >> A2W_PLL_FRAC_BITS;
> + fdiv = div & ((1 << A2W_PLL_FRAC_BITS) - 1);
> +
> + /* We can't use rate directly as it would overflow */
> + final_rate = ((u64)*parent_rate * ((ndiv << A2W_PLL_FRAC_BITS) + fdiv));
> +
> + return final_rate >> A2W_PLL_FRAC_BITS;
> +}
> +
> +static void raspberrypi_fw_pll_off(struct clk_hw *hw)
> +{
> + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> + pllb);
> + u32 val = RPI_FIRMWARE_STATE_WAIT_BIT;
> +
> + raspberrypi_clock_property(rpi->firmware,
> + RPI_FIRMWARE_SET_CLOCK_STATE,
> + RPI_FIRMWARE_ARM_CLK_ID, &val);
> +}
I'm not sure. Does this operation really make sense?
> +
> +static const struct clk_ops raspberrypi_firmware_pll_clk_ops = {
> + .is_prepared = raspberrypi_fw_pll_is_on,
> + .prepare = raspberrypi_fw_pll_on,
> + .unprepare = raspberrypi_fw_pll_off,
> + .recalc_rate = raspberrypi_fw_pll_get_rate,
> + .set_rate = raspberrypi_fw_pll_set_rate,
> + .round_rate = raspberrypi_pll_round_rate,
> +};
> +
> +static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
> +{
> + u32 min_rate = 0, max_rate = 0;
> + struct clk_init_data init;
> + int ret;
> +
> + /* Get min & max rates set by the firmware */
> + ret = raspberrypi_clock_property(rpi->firmware,
> + RPI_FIRMWARE_GET_MIN_CLOCK_RATE,
> + RPI_FIRMWARE_ARM_CLK_ID,
> + &min_rate);
> + if (ret) {
> + dev_err(rpi->dev, "Failed to get %s min freq: %d\n",
> + init.name, ret);
at this point init isn't initialized
> + return ret;
> + }
> +
> + ret = raspberrypi_clock_property(rpi->firmware,
> + RPI_FIRMWARE_GET_MAX_CLOCK_RATE,
> + RPI_FIRMWARE_ARM_CLK_ID,
> + &max_rate);
> + if (ret) {
> + dev_err(rpi->dev, "Failed to get %s max freq: %d\n",
> + init.name, ret);
> + return ret;
ditto
> + }
> +
> + dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n",
> + min_rate, max_rate);
> +
> + rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
> + rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
I know we have to trust the firmware, but i would at least check that
min_rate and max_rate are greater than zero.
> +
> + memset(&init, 0, sizeof(init));
> +
> + /* All of the PLLs derive from the external oscillator. */
> + init.parent_names = (const char *[]){ "osc" };
> + init.num_parents = 1;
> + init.name = "pllb";
> + init.ops = &raspberrypi_firmware_pll_clk_ops;
> + init.flags = CLK_GET_RATE_NOCACHE | CLK_IGNORE_UNUSED;
> +
> + rpi->pllb.init = &init;
> +
> + return devm_clk_hw_register(rpi->dev, &rpi->pllb);
> +}
> +
> +static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
> +{
> + rpi->pllb_arm = clk_hw_register_fixed_factor(rpi->dev,
> + "pllb_arm", "pllb",
> + CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
> + 1, 2);
> + if (IS_ERR(rpi->pllb_arm)) {
> + dev_err(rpi->dev, "Failed to initialize pllb_arm\n");
> + return PTR_ERR(rpi->pllb_arm);
> + }
> +
> + rpi->pllb_arm_lookup = clkdev_hw_create(rpi->pllb_arm, NULL, "cpu0");
> + if (!rpi->pllb_arm_lookup) {
> + dev_err(rpi->dev, "Failed to initialize pllb_arm_lookup\n");
> + clk_hw_unregister_fixed_factor(rpi->pllb_arm);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int raspberrypi_clk_probe(struct platform_device *pdev)
> +{
> + struct device_node *firmware_node;
> + struct device *dev = &pdev->dev;
> + struct rpi_firmware *firmware;
> + struct raspberrypi_clk *rpi;
> + int ret;
> +
> + firmware_node = of_find_compatible_node(NULL, NULL,
> + "raspberrypi,bcm2835-firmware");
> + if (!firmware_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + firmware = rpi_firmware_get(firmware_node);
> + of_node_put(firmware_node);
> + if (!firmware)
> + return -EPROBE_DEFER;
> +
> + rpi = devm_kzalloc(dev, sizeof(*rpi), GFP_KERNEL);
> + if (!rpi)
> + return -ENOMEM;
> +
> + rpi->dev = dev;
> + rpi->firmware = firmware;
> +
> + ret = raspberrypi_register_pllb(rpi);
> + if (ret) {
> + dev_err(dev, "Failed to initialize pllb, %d\n", ret);
> + return ret;
> + }
> +
> + ret = raspberrypi_register_pllb_arm(rpi);
> + if (ret) {
> + dev_err(dev, "Failed to initialize pllb_arm, %d\n", ret);
I think the error messages in raspberrypi_register_pllb_arm() are enough.
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver raspberrypi_clk_driver = {
> + .driver = {
> + .name = "raspberrypi-clk",
> + },
> + .probe = raspberrypi_clk_probe,
> +};
> +builtin_platform_driver(raspberrypi_clk_driver);
> +
> +MODULE_AUTHOR("Nicolas Saenz Julienne <[email protected]>");
> +MODULE_DESCRIPTION("Raspberry Pi firmware clock driver");
> +MODULE_LICENSE("GPLv2");
Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> Raspberry Pi's firmware controls this pll, we should use the firmware
> interface to access it.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
> drivers/clk/bcm/clk-bcm2835.c | 25 -------------------------
> 1 file changed, 25 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 770bb01f523e..ccb0319fc2e9 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1651,31 +1651,6 @@ static const struct bcm2835_clk_desc clk_desc_array[] = {
> .fixed_divider = 1,
> .flags = CLK_SET_RATE_PARENT),
>
> - /* PLLB is used for the ARM's clock. */
How about leaving a short comment, that these clocks are now handled by
a different driver?
Hi Stefan,
thanks for the review, I took note of your code comments.
On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
> Hi Nicolas,
>
> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> > Hi all,
> > this series aims at adding cpufreq support to the Raspberry Pi family of
> > boards.
> >
> > The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431
> >
> > The series first factors out 'pllb' from clk-bcm2385 and creates a new
> > clk driver that operates it over RPi's firmware interface[1]. We are
> > forced to do so as the firmware 'owns' the pll and we're not allowed to
> > change through the register interface directly as we might race with the
> > over-temperature and under-voltage protections provided by the firmware.
> it would be nice to preserve such design decision in the driver as a
> comment, because the cover letter usually get lost.
> > Next it creates a minimal cpufreq driver that populates the CPU's opp
> > table, and registers cpufreq-dt. Which is needed as the firmware
> > controls the max and min frequencies available.
>
> I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
> manually enable this drivers. During boot with Raspbian rootfs i'm
> getting the following:
>
> [ 1.177009] cpu cpu0: failed to get clock: -2
> [ 1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
This is surprising, who could be creating a platform_device for cpufreq-dt
apart from raspberrypi-cpufreq? Just to make things clear, you're using the
device tree from v5.2-rc1 (as opposed to the Raspbian one)?
> [ 1.192117] sdhci: Secure Digital Host Controller Interface driver
> [ 1.198725] sdhci: Copyright(c) Pierre Ossman
> [ 1.207005] Synopsys Designware Multimedia Card Interface Driver
> [ 1.319936] sdhost-bcm2835 3f202000.mmc: loaded - DMA enabled (>1)
> [ 1.326641] sdhci-pltfm: SDHCI platform and OF driver helper
> [ 1.336568] ledtrig-cpu: registered to indicate activity on CPUs
> [ 1.343713] usbcore: registered new interface driver usbhid
> [ 1.350275] usbhid: USB HID core driver
> [ 1.357639] bcm2835-mbox 3f00b880.mailbox: mailbox enabled
> [ 1.367490] NET: Registered protocol family 10
> [ 1.375013] Segment Routing with IPv6
> [ 1.381696] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
> [ 1.388980] NET: Registered protocol family 17
> [ 1.395624] can: controller area network core (rev 20170425 abi 9)
> [ 1.402358] NET: Registered protocol family 29
> [ 1.408997] can: raw protocol (rev 20170425)
> [ 1.415599] can: broadcast manager protocol (rev 20170425 t)
> [ 1.422219] can: netlink gateway (rev 20170425) max_hops=1
> [ 1.429369] Key type dns_resolver registered
> [ 1.437190] Registering SWP/SWPB emulation handler
> [ 1.444443] Loading compiled-in X.509 certificates
> [ 1.455693] 3f201000.serial: ttyAMA0 at MMIO 0x3f201000 (irq = 81,
> base_baud = 0) is a PL011 rev2
> [ 1.462768] serial serial0: tty port ttyAMA0 registered
> [ 1.478755] mmc0: host does not support reading read-only switch,
> assuming write-enable
> [ 1.488792] mmc0: new high speed SDHC card at address 0007
> [ 1.495766] raspberrypi-firmware soc:firmware: Attached to firmware
> from 2019-03-27 15:45
> [ 1.496862] mmcblk0: mmc0:0007 SDCIT 14.6 GiB
> [ 1.512768] raspberrypi-clk raspberrypi-clk: CPU frequency range: min
> 600000000, max 1400000000
> [ 1.513012] mmcblk0: p1 p2
> [ 1.558085] dwc2 3f980000.usb: 3f980000.usb supply vusb_d not found,
> using dummy regulator
> [ 1.565355] dwc2 3f980000.usb: 3f980000.usb supply vusb_a not found,
> using dummy regulator
> [ 1.623246] dwc2 3f980000.usb: DWC OTG Controller
> [ 1.630318] dwc2 3f980000.usb: new USB bus registered, assigned bus
> number 1
> [ 1.637439] dwc2 3f980000.usb: irq 33, io mem 0x3f980000
> [ 1.645268] hub 1-0:1.0: USB hub found
> [ 1.652317] hub 1-0:1.0: 1 port detected
> [ 1.665867] sdhci-iproc 3f300000.sdhci: allocated mmc-pwrseq
> [ 1.704788] mmc1: SDHCI controller on 3f300000.sdhci [3f300000.sdhci]
> using PIO
> [ 1.717694] hctosys: unable to open rtc device (rtc0)
> [ 1.724967] sysfs: cannot create duplicate filename
> '/devices/platform/cpufreq-dt'
For the record, this is raspberrypi-cpufreq creating the platform device for
cpufreq-dt.
> [ 1.732120] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> 5.2.0-rc1-00004-g5aa6d98-dirty #2
> [ 1.739288] Hardware name: BCM2835
> [ 1.746421] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
> (show_stack+0x10/0x14)
> [ 1.753636] [<c030cc08>] (show_stack) from [<c0e7d358>]
> (dump_stack+0xb4/0xc8)
> [ 1.760840] [<c0e7d358>] (dump_stack) from [<c0503b64>]
> (sysfs_warn_dup+0x58/0x64)
> [ 1.768105] [<c0503b64>] (sysfs_warn_dup) from [<c0503c8c>]
> (sysfs_create_dir_ns+0xd8/0xe8)
> [ 1.775481] [<c0503c8c>] (sysfs_create_dir_ns) from [<c0e82520>]
> (kobject_add_internal+0xb0/0x2fc)
> [ 1.782958] [<c0e82520>] (kobject_add_internal) from [<c0e827c8>]
> (kobject_add+0x5c/0xc0)
> [ 1.790534] [<c0e827c8>] (kobject_add) from [<c096b1cc>]
> (device_add+0xf8/0x608)
> [ 1.798180] [<c096b1cc>] (device_add) from [<c0971098>]
> (platform_device_add+0x110/0x214)
> [ 1.805945] [<c0971098>] (platform_device_add) from [<c0971ae4>]
> (platform_device_register_full+0x130/0x148)
> [ 1.813866] [<c0971ae4>] (platform_device_register_full) from
> [<c15a7a30>] (raspberrypi_cpufreq_driver_init+0x128/0x178)
> [ 1.821916] [<c15a7a30>] (raspberrypi_cpufreq_driver_init) from
> [<c0302eec>] (do_one_initcall+0x54/0x21c)
> [ 1.830099] [<c0302eec>] (do_one_initcall) from [<c15010f8>]
> (kernel_init_freeable+0x244/0x2e0)
> [ 1.838312] [<c15010f8>] (kernel_init_freeable) from [<c0e949d4>]
> (kernel_init+0x8/0x10c)
> [ 1.846541] [<c0e949d4>] (kernel_init) from [<c03010e8>]
> (ret_from_fork+0x14/0x2c)
> [ 1.854783] Exception stack(0xea89dfb0 to 0xea89dff8)
> [ 1.863036] dfa0: 00000000
> 00000000 00000000 00000000
> [ 1.871450] dfc0: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [ 1.879860] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 1.888251] kobject_add_internal failed for cpufreq-dt with -EEXIST,
> don't try to register things with the same name in the same directory.
> [ 1.896910] cpu cpu0: Failed to create platform device, -17
Regards,
Nicolas
Hi Nicolas,
Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> Raspberry Pi's firmware offers and interface though which update it's
> performance requirements. It allows us to request for specific runtime
> frequencies, which the firmware might or might not respect, depending on
> the firmware configuration and thermals.
>
> As the maximum and minimum frequencies are configurable in the firmware
> there is no way to know in advance their values. So the Raspberry Pi
> cpufreq driver queries them, builds an opp frequency table to then
> launch cpufreq-dt.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
>
> Changes since RFC:
> - Alphabetically ordered relevant stuff
> - Updated Kconfig to select firmware interface
> - Correctly unref clk_dev after use
> - Remove all opps on failure
> - Remove use of dev_pm_opp_set_sharing_cpus()
>
> drivers/cpufreq/Kconfig.arm | 8 +++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/raspberrypi-cpufreq.c | 84 +++++++++++++++++++++++++++
> 3 files changed, 93 insertions(+)
> create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index f8129edc145e..556d432cc826 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -133,6 +133,14 @@ config ARM_QCOM_CPUFREQ_HW
> The driver implements the cpufreq interface for this HW engine.
> Say Y if you want to support CPUFreq HW.
>
> +config ARM_RASPBERRYPI_CPUFREQ
> + tristate "Raspberry Pi cpufreq support"
> + select RASPBERRYPI_FIRMWARE
> + help
> + This adds the CPUFreq driver for Raspberry Pi
For the next version please add one patch which enables the driver and
its dependencies for bcm2835_defconfig and multi_v7_defconfig and
another patch to build as a module for arm64/defconfig.
> +
> + If in doubt, say N.
> +
> config ARM_S3C_CPUFREQ
> bool
> help
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 689b26c6f949..121c1acb66c0 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW) += qcom-cpufreq-hw.o
> obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o
> +obj-$(CONFIG_ARM_RASPBERRYPI_CPUFREQ) += raspberrypi-cpufreq.o
> obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
> obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
> diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c b/drivers/cpufreq/raspberrypi-cpufreq.c
> new file mode 100644
> index 000000000000..2b3a195a9d37
> --- /dev/null
> +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Raspberry Pi cpufreq driver
> + *
> + * Copyright (C) 2019, Nicolas Saenz Julienne <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +
> +static const struct of_device_id machines[] __initconst = {
> + { .compatible = "raspberrypi,3-model-b-plus" },
> + { .compatible = "raspberrypi,3-model-b" },
> + { .compatible = "raspberrypi,2-model-b" },
> + { /* sentinel */ }
> +};
> +
> +static int __init raspberrypi_cpufreq_driver_init(void)
> +{
> + struct platform_device *pdev;
> + struct device *cpu_dev;
> + unsigned long min, max;
> + unsigned long rate;
> + struct clk *clk;
> + int ret;
> +
> + if (!of_match_node(machines, of_root))
> + return -ENODEV;
> +
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev) {
> + pr_err("Cannot get CPU for cpufreq driver\n");
> + return -ENODEV;
> + }
> +
> + clk = clk_get(cpu_dev, 0);
Please use NULL in this context
Thanks
Hi,
Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
> Hi Stefan,
> thanks for the review, I took note of your code comments.
>
> On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
>> Hi Nicolas,
>>
>> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
>>> Hi all,
>>> this series aims at adding cpufreq support to the Raspberry Pi family of
>>> boards.
>>>
>>> The previous revision can be found at: https://lkml.org/lkml/2019/5/20/431
>>>
>>> The series first factors out 'pllb' from clk-bcm2385 and creates a new
>>> clk driver that operates it over RPi's firmware interface[1]. We are
>>> forced to do so as the firmware 'owns' the pll and we're not allowed to
>>> change through the register interface directly as we might race with the
>>> over-temperature and under-voltage protections provided by the firmware.
>> it would be nice to preserve such design decision in the driver as a
>> comment, because the cover letter usually get lost.
>>> Next it creates a minimal cpufreq driver that populates the CPU's opp
>>> table, and registers cpufreq-dt. Which is needed as the firmware
>>> controls the max and min frequencies available.
>> I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
>> manually enable this drivers. During boot with Raspbian rootfs i'm
>> getting the following:
>>
>> [ 1.177009] cpu cpu0: failed to get clock: -2
>> [ 1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
> This is surprising, who could be creating a platform_device for cpufreq-dt
> apart from raspberrypi-cpufreq? Just to make things clear, you're using the
> device tree from v5.2-rc1 (as opposed to the Raspbian one)?
sorry my fault, i thought it already has been replaced. The behavior in
this unexpected case is fine, since it doesn't crash.
I replaced the the DTB with the mainline one, but now i'm getting this:
[ 4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
600000 KHz
[ 4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
[ 4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22
[ 4.608413] ------------[ cut here ]------------
[ 4.620203] kernel BUG at drivers/cpufreq/cpufreq.c:1348!
[ 4.632787] Internal error: Oops - BUG: 0 [#1] SMP ARM
[ 4.645062] Modules linked in:
[ 4.655147] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G W
5.2.0-rc1-00004-g5aa6d98-dirty #2
[ 4.671891] Hardware name: BCM2835
[ 4.682549] PC is at cpufreq_online+0x690/0x6b0
[ 4.694409] LR is at __wake_up_common_lock+0xa0/0xc8
[ 4.706744] pc : [<c0c696c0>] lr : [<c03860d4>] psr: a0000013
[ 4.720518] sp : ea89dce8 ip : eaa1ace0 fp : c1704d54
[ 4.733288] r10: eaa1acb4 r9 : c1673440 r8 : c1705038
[ 4.746085] r7 : c18e6728 r6 : 000927c0 r5 : 00000000 r4 : eaa1ac00
[ 4.760288] r3 : d89db0bd r2 : d89db0bd r1 : 60000013 r0 : ffffffea
[ 4.774541] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM
Segment none
[ 4.789431] Control: 10c5383d Table: 0020406a DAC: 00000051
[ 4.802897] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[ 4.816661] Stack: (0xea89dce8 to 0xea89e000)
[ 4.828744] dce0: 00000000 d89db0bd eaa1ac04
00000001 ea89dd28 00000000
[ 4.844876] dd00: c184539c c1704c48 00000000 c186de1c 0000000f
00000000 c15dc83c c0c6975c
[ 4.861033] dd20: c186d9e4 c096dbbc ea891358 ea9a5db4 00000000
d89db0bd c186de68 c18e6728
[ 4.877227] dd40: c186d99c c0c66ffc eaff1800 c0c6bda0 00000000
eb80c050 eaff1800 c0c6c148
[ 4.893461] dd60: eaff1810 00000000 c186de1c c097126c c18e16a4
eaff1810 c18e16a8 c096f314
[ 4.909769] dd80: eaff1810 c186de1c c096f800 c1704c48 00000001
00000000 c1667b7c c096f64c
[ 4.926077] dda0: c186de1c ea89ddfc eaff1810 00000000 ea89ddfc
c096f800 c1704c48 00000001
[ 4.942410] ddc0: 00000000 c1667b7c c15dc83c c096d864 00000000
ea89126c eaf85138 d89db0bd
[ 4.958798] dde0: eaff1810 eaff1810 c1704c48 eaff1854 c1704c48
c096f1b0 c1667b7c eaff1810
[ 4.975236] de00: 00000001 d89db0bd eaff1810 eaff1810 c1845308
c1704c48 00000000 c096e574
[ 4.991705] de20: eaff1810 c1845138 00000000 c096b490 00000000
c0e822d8 c1704c48 ea89dea0
[ 5.008233] de40: eaff1800 d89db0bd ea89de60 eaff1800 00000000
eaff1800 eb80c050 00000000
[ 5.024770] de60: eaff1810 c0971098 eaff1800 ea89dea0 c1704c48
eb80c050 00000000 05f5e100
[ 5.041302] de80: c1667b7c c0971ae4 00000000 23c34600 c1704c48
eb80c050 00000000 c15a7a30
[ 5.057796] dea0: 00000000 00000000 00000000 c1361770 ffffffff
00000000 00000000 00000000
[ 5.074269] dec0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 d89db0bd
[ 5.090693] dee0: 00000167 c1704c48 c188d420 ffffe000 00000000
c15a7908 00000167 c0302eec
[ 5.107119] df00: 00000167 c0365a5c c13ff448 c130e700 00000000
00000007 00000007 c124057c
[ 5.123539] df20: 00000000 c1704c48 c1251c04 c12405f0 00000000
ebfffc7d 00000000 d89db0bd
[ 5.139941] df40: 00000000 c188d420 00000008 d89db0bd c188d420
00000008 c1899800 c1899800
[ 5.156355] df60: c15dc838 c15010f8 00000007 00000007 00000000
c150067c c0e949cc 00000000
[ 5.172779] df80: 00000000 00000000 c0e949cc 00000000 00000000
00000000 00000000 00000000
[ 5.189192] dfa0: 00000000 c0e949d4 00000000 c03010e8 00000000
00000000 00000000 00000000
[ 5.205577] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 5.221915] dfe0: 00000000 00000000 00000000 00000000 00000013
00000000 00000000 00000000
[ 5.238205] [<c0c696c0>] (cpufreq_online) from [<c0c6975c>]
(cpufreq_add_dev+0x6c/0x78)
[ 5.254372] [<c0c6975c>] (cpufreq_add_dev) from [<c096dbbc>]
(subsys_interface_register+0xa0/0xec)
[ 5.271557] [<c096dbbc>] (subsys_interface_register) from
[<c0c66ffc>] (cpufreq_register_driver+0x14c/0x20c)
[ 5.289700] [<c0c66ffc>] (cpufreq_register_driver) from [<c0c6c148>]
(dt_cpufreq_probe+0x94/0x114)
[ 5.307002] [<c0c6c148>] (dt_cpufreq_probe) from [<c097126c>]
(platform_drv_probe+0x48/0x98)
[ 5.323802] [<c097126c>] (platform_drv_probe) from [<c096f314>]
(really_probe+0xf0/0x2c8)
[ 5.340390] [<c096f314>] (really_probe) from [<c096f64c>]
(driver_probe_device+0x60/0x164)
[ 5.357113] [<c096f64c>] (driver_probe_device) from [<c096d864>]
(bus_for_each_drv+0x58/0xb8)
[ 5.374142] [<c096d864>] (bus_for_each_drv) from [<c096f1b0>]
(__device_attach+0xd0/0x13c)
[ 5.390937] [<c096f1b0>] (__device_attach) from [<c096e574>]
(bus_probe_device+0x84/0x8c)
[ 5.407684] [<c096e574>] (bus_probe_device) from [<c096b490>]
(device_add+0x3bc/0x608)
[ 5.424208] [<c096b490>] (device_add) from [<c0971098>]
(platform_device_add+0x110/0x214)
[ 5.440983] [<c0971098>] (platform_device_add) from [<c0971ae4>]
(platform_device_register_full+0x130/0x148)
[ 5.459471] [<c0971ae4>] (platform_device_register_full) from
[<c15a7a30>] (raspberrypi_cpufreq_driver_init+0x128/0x178)
[ 5.479053] [<c15a7a30>] (raspberrypi_cpufreq_driver_init) from
[<c0302eec>] (do_one_initcall+0x54/0x21c)
[ 5.497336] [<c0302eec>] (do_one_initcall) from [<c15010f8>]
(kernel_init_freeable+0x244/0x2e0)
[ 5.514735] [<c15010f8>] (kernel_init_freeable) from [<c0e949d4>]
(kernel_init+0x8/0x10c)
[ 5.531590] [<c0e949d4>] (kernel_init) from [<c03010e8>]
(ret_from_fork+0x14/0x2c)
[ 5.547768] Exception stack(0xea89dfb0 to 0xea89dff8)
[ 5.561323] dfa0: 00000000
00000000 00000000 00000000
[ 5.578116] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 5.594962] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 5.610133] Code: e59f1024 e34c0136 ebdcbce4 eaffff63 (e7f001f2)
[ 5.624789] ---[ end trace 7aaf0f77e232247e ]---
[ 5.637981] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b
[ 5.654308] CPU3: stopping
[ 5.665609] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G D W
5.2.0-rc1-00004-g5aa6d98-dirty #2
[ 5.683854] Hardware name: BCM2835
[ 5.695927] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[ 5.712502] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[ 5.728537] [<c0e7d358>] (dump_stack) from [<c03107c8>]
(handle_IPI+0x3bc/0x3dc)
[ 5.744710] [<c03107c8>] (handle_IPI) from [<c0301a8c>]
(__irq_svc+0x6c/0x90)
[ 5.760601] Exception stack(0xea8d7f60 to 0xea8d7fa8)
[ 5.774348] 7f60: 00000000 000004e4 eb854ae0 c031d840 ffffe000
c1704c6c c1704cac 00000008
[ 5.791381] 7f80: 00000000 c1704c48 c1673568 00000000 00000002
ea8d7fb0 c0309164 c0309168
[ 5.808421] 7fa0: 60000013 ffffffff
[ 5.820668] [<c0301a8c>] (__irq_svc) from [<c0309168>]
(arch_cpu_idle+0x38/0x3c)
[ 5.836950] [<c0309168>] (arch_cpu_idle) from [<c03747c4>]
(do_idle+0x1bc/0x298)
[ 5.853198] [<c03747c4>] (do_idle) from [<c0374b3c>]
(cpu_startup_entry+0x18/0x1c)
[ 5.869667] [<c0374b3c>] (cpu_startup_entry) from [<003025cc>]
(0x3025cc)
[ 5.885362] CPU1: stopping
[ 5.896857] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G D W
5.2.0-rc1-00004-g5aa6d98-dirty #2
[ 5.915295] Hardware name: BCM2835
[ 5.927623] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[ 5.944490] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[ 5.960861] [<c0e7d358>] (dump_stack) from [<c03107c8>]
(handle_IPI+0x3bc/0x3dc)
[ 5.977364] [<c03107c8>] (handle_IPI) from [<c0301a8c>]
(__irq_svc+0x6c/0x90)
[ 5.993562] Exception stack(0xea8d3f60 to 0xea8d3fa8)
[ 6.007632] 3f60: 00000000 00002930 eb82cae0 c031d840 ffffe000
c1704c6c c1704cac 00000002
[ 6.024974] 3f80: 00000000 c1704c48 c1673568 00000000 00000000
ea8d3fb0 c0309164 c0309168
[ 6.042285] 3fa0: 60000013 ffffffff
[ 6.054772] [<c0301a8c>] (__irq_svc) from [<c0309168>]
(arch_cpu_idle+0x38/0x3c)
[ 6.071321] [<c0309168>] (arch_cpu_idle) from [<c03747c4>]
(do_idle+0x1bc/0x298)
[ 6.087880] [<c03747c4>] (do_idle) from [<c0374b3c>]
(cpu_startup_entry+0x18/0x1c)
[ 6.104610] [<c0374b3c>] (cpu_startup_entry) from [<003025cc>]
(0x3025cc)
[ 6.120535] CPU0: stopping
[ 6.132240] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G D W
5.2.0-rc1-00004-g5aa6d98-dirty #2
[ 6.150859] Hardware name: BCM2835
[ 6.163308] [<c0312304>] (unwind_backtrace) from [<c030cc08>]
(show_stack+0x10/0x14)
[ 6.180258] [<c030cc08>] (show_stack) from [<c0e7d358>]
(dump_stack+0xb4/0xc8)
[ 6.196660] [<c0e7d358>] (dump_stack) from [<c03107c8>]
(handle_IPI+0x3bc/0x3dc)
[ 6.213214] [<c03107c8>] (handle_IPI) from [<c0301a8c>]
(__irq_svc+0x6c/0x90)
[ 6.229479] Exception stack(0xc1701f10 to 0xc1701f58)
[ 6.243636] 1f00: 00000000
0000190c eb818ae0 c031d840
[ 6.261073] 1f20: ffffe000 c1704c6c c1704cac 00000001 00000000
c1704c48 c1673568 00000000
[ 6.278565] 1f40: 00000002 c1701f60 c0309164 c0309168 60000013 ffffffff
[ 6.294466] [<c0301a8c>] (__irq_svc) from [<c0309168>]
(arch_cpu_idle+0x38/0x3c)
[ 6.311211] [<c0309168>] (arch_cpu_idle) from [<c03747c4>]
(do_idle+0x1bc/0x298)
[ 6.327979] [<c03747c4>] (do_idle) from [<c0374b3c>]
(cpu_startup_entry+0x18/0x1c)
[ 6.344934] [<c0374b3c>] (cpu_startup_entry) from [<c1500e88>]
(start_kernel+0x460/0x48c)
[ 6.362527] [<c1500e88>] (start_kernel) from [<00000000>] (0x0)
[ 6.377824] ---[ end Kernel panic - not syncing: Attempted to kill
init! exitcode=0x0000000b ]---
Hi Stefan,
thanks for your review.
On Wed, 2019-06-05 at 12:44 +0200, Stefan Wahren wrote:
> Hi Nicolas,
>
> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> > Raspberry Pi's firmware offers and interface though which update it's
> > clock's frequencies. This is specially useful in order to change the CPU
> > clock (pllb_arm) which is 'owned' by the firmware and we're unable to
> > scale using the register interface.
> >
> > Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> > ---
> >
> > Changes since RFC:
> > - Moved firmware interface into own driver
> > - Use of_find_compatible_node()
> > - Remove error message on rpi_firmware_get() failure
> > - Ratelimit messages on set_rate() failure
> > - Use __le32 on firmware interface definition
> >
> > drivers/clk/bcm/Makefile | 1 +
> > drivers/clk/bcm/clk-raspberrypi.c | 316 ++++++++++++++++++++++++++++++
> > 2 files changed, 317 insertions(+)
> > create mode 100644 drivers/clk/bcm/clk-raspberrypi.c
> >
> > diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
> > index 002661d39128..07abe92df9d1 100644
> > --- a/drivers/clk/bcm/Makefile
> > +++ b/drivers/clk/bcm/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm21664.o
> > obj-$(CONFIG_COMMON_CLK_IPROC) += clk-iproc-armpll.o clk-iproc-pll.o
> > clk-iproc-asiu.o
> > obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o
> > obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835-aux.o
> > +obj-$(CONFIG_ARCH_BCM2835) += clk-raspberrypi.o
> Hm, on the one side it would be nice to avoid building this driver in
> case the firmware driver is disabled on the other side it would be good
> to keep compile test.
> > obj-$(CONFIG_ARCH_BCM_53573) += clk-bcm53573-ilp.o
> > obj-$(CONFIG_CLK_BCM_CYGNUS) += clk-cygnus.o
> > obj-$(CONFIG_CLK_BCM_HR2) += clk-hr2.o
> > diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-
> > raspberrypi.c
> > new file mode 100644
> > index 000000000000..485c00288414
> > --- /dev/null
> > +++ b/drivers/clk/bcm/clk-raspberrypi.c
> > @@ -0,0 +1,316 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2019 Nicolas Saenz Julienne
> > + */
> > +
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> > +#define RPI_FIRMWARE_ARM_CLK_ID 0x000000003
> > +
> > +#define RPI_FIRMWARE_STATE_ENABLE_BIT 0x1
> > +#define RPI_FIRMWARE_STATE_WAIT_BIT 0x2
> how about using the BIT() macro?
> > +
> > +/*
> > + * Even though the firmware interface alters 'pllb' the frequencies are
> > + * provided as per 'pllb_arm'. We need to scale before passing them trough.
> > + */
> > +#define RPI_FIRMWARE_PLLB_ARM_DIV_RATE 2
> > +
> > +#define A2W_PLL_FRAC_BITS 20
> > +
> > +struct raspberrypi_clk {
> > + struct device *dev;
> > + struct rpi_firmware *firmware;
> > +
> > + unsigned long min_rate;
> > + unsigned long max_rate;
> > +
> > + struct clk_hw pllb;
> > + struct clk_hw *pllb_arm;
> > + struct clk_lookup *pllb_arm_lookup;
> > +};
> > +
> > +/*
> > + * Structure of the message passed to Raspberry Pi's firmware in order to
> > + * change clock rates. The 'disable_turbo' option is only available to the
> > ARM
> > + * clock (pllb) which we enable by default as turbo mode will alter
> > multiple
> > + * clocks at once.
> > + *
> > + * Even though we're able to access the clock registers directly we're
> > bound to
> > + * use the firmware interface as the firmware ultimately takes care of
> > + * mitigating overheating/undervoltage situations and we would be changing
> > + * frequencies behind his back.
> > + *
> > + * For more information on the firmware interface check:
> > + * https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
> > + */
> > +struct raspberrypi_firmware_prop {
> > + __le32 id;
> > + __le32 val;
> > + __le32 disable_turbo;
> > +} __packed;
> > +
> > +static int raspberrypi_clock_property(struct rpi_firmware *firmware, u32
> > tag,
> > + u32 clk, u32 *val)
> > +{
> > + struct raspberrypi_firmware_prop msg = {
> > + .id = clk,
> > + .val = *val,
> > + .disable_turbo = 1,
> > + };
> > + int ret;
> > +
> > + ret = rpi_firmware_property(firmware, tag, &msg, sizeof(msg));
> > + if (ret)
> > + return ret;
> > +
> > + *val = msg.val;
> > +
> > + return 0;
> > +}
> > +
> > +static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
> > +{
> > + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> > + pllb);
> > + u32 val = 0;
> > + int ret;
> > +
> > + ret = raspberrypi_clock_property(rpi->firmware,
> > + RPI_FIRMWARE_GET_CLOCK_STATE,
> > + RPI_FIRMWARE_ARM_CLK_ID, &val);
> > + if (ret)
> > + return 0;
> > +
> > + return !!(val & RPI_FIRMWARE_STATE_ENABLE_BIT);
> > +}
> > +
> > +
> > +static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> > + pllb);
> > + u32 val = 0;
> > + int ret;
> > +
> > + ret = raspberrypi_clock_property(rpi->firmware,
> > + RPI_FIRMWARE_GET_CLOCK_RATE,
> > + RPI_FIRMWARE_ARM_CLK_ID,
> > + &val);
> > + if (ret)
> > + return ret;
> > +
> > + return val * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
> > +}
> > +
> > +static int raspberrypi_fw_pll_on(struct clk_hw *hw)
> > +{
> > + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> > + pllb);
> > + u32 val;
> > + int ret;
> > +
> > + val = RPI_FIRMWARE_STATE_ENABLE_BIT | RPI_FIRMWARE_STATE_WAIT_BIT;
> > +
> > + ret = raspberrypi_clock_property(rpi->firmware,
> > + RPI_FIRMWARE_SET_CLOCK_STATE,
> > + RPI_FIRMWARE_ARM_CLK_ID, &val);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> return ret;
> > +}
> > +
> > +static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long
> > rate,
> > + unsigned long parent_rate)
> > +{
> > + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> > + pllb);
> > + u32 new_rate = rate / RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
> > + int ret;
> > +
> > + ret = raspberrypi_clock_property(rpi->firmware,
> > + RPI_FIRMWARE_SET_CLOCK_RATE,
> > + RPI_FIRMWARE_ARM_CLK_ID,
> > + &new_rate);
> > + if (ret)
> > + dev_err_ratelimited(rpi->dev, "Failed to change %s frequency:
> > %d",
> > + clk_hw_get_name(hw), ret);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * Sadly there is no firmware rate rounding interface. We borred it from
> borrowed?
Yes
> > + * clk-bcm2835.
> > + */
> > +static long raspberrypi_pll_round_rate(struct clk_hw *hw, unsigned long
> > rate,
> > + unsigned long *parent_rate)
> > +{
> > + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> > + pllb);
> > + u64 div, final_rate;
> > + u32 ndiv, fdiv;
> > +
> > + rate = clamp(rate, rpi->min_rate, rpi->max_rate);
> > +
> > + div = (u64)rate << A2W_PLL_FRAC_BITS;
> > + do_div(div, *parent_rate);
> > +
> > + ndiv = div >> A2W_PLL_FRAC_BITS;
> > + fdiv = div & ((1 << A2W_PLL_FRAC_BITS) - 1);
> > +
> > + /* We can't use rate directly as it would overflow */
> > + final_rate = ((u64)*parent_rate * ((ndiv << A2W_PLL_FRAC_BITS) + fdiv));
> > +
> > + return final_rate >> A2W_PLL_FRAC_BITS;
> > +}
> > +
> > +static void raspberrypi_fw_pll_off(struct clk_hw *hw)
> > +{
> > + struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
> > + pllb);
> > + u32 val = RPI_FIRMWARE_STATE_WAIT_BIT;
> > +
> > + raspberrypi_clock_property(rpi->firmware,
> > + RPI_FIRMWARE_SET_CLOCK_STATE,
> > + RPI_FIRMWARE_ARM_CLK_ID, &val);
> > +}
> I'm not sure. Does this operation really make sense?
You're right, I implemented it mindlessly as I saw the API available in the
firmware interface. I'll remove both prepare and unprepare as one is redundant
and the other harmful (though I wonder what whould happen if called).
Regards,
Nicolas
Hi Stefan,
On Wed, 2019-06-05 at 13:34 +0200, Stefan Wahren wrote:
> Hi,
>
> Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
> > Hi Stefan,
> > thanks for the review, I took note of your code comments.
> >
> > On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
> > > Hi Nicolas,
> > >
> > > Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> > > > Hi all,
> > > > this series aims at adding cpufreq support to the Raspberry Pi family of
> > > > boards.
> > > >
> > > > The previous revision can be found at:
> > > > https://lkml.org/lkml/2019/5/20/431
> > > >
> > > > The series first factors out 'pllb' from clk-bcm2385 and creates a new
> > > > clk driver that operates it over RPi's firmware interface[1]. We are
> > > > forced to do so as the firmware 'owns' the pll and we're not allowed to
> > > > change through the register interface directly as we might race with the
> > > > over-temperature and under-voltage protections provided by the firmware.
> > > it would be nice to preserve such design decision in the driver as a
> > > comment, because the cover letter usually get lost.
> > > > Next it creates a minimal cpufreq driver that populates the CPU's opp
> > > > table, and registers cpufreq-dt. Which is needed as the firmware
> > > > controls the max and min frequencies available.
> > > I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
> > > manually enable this drivers. During boot with Raspbian rootfs i'm
> > > getting the following:
> > >
> > > [ 1.177009] cpu cpu0: failed to get clock: -2
> > > [ 1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
> > This is surprising, who could be creating a platform_device for cpufreq-dt
> > apart from raspberrypi-cpufreq? Just to make things clear, you're using the
> > device tree from v5.2-rc1 (as opposed to the Raspbian one)?
>
> sorry my fault, i thought it already has been replaced. The behavior in
> this unexpected case is fine, since it doesn't crash.
>
> I replaced the the DTB with the mainline one, but now i'm getting this:
>
> [ 4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
> 600000 KHz
> [ 4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
> [ 4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22
Ok, this looks more more like my fault, probably an overflow error somewhere. I
saw something similar while testing it on RPI2b. Which board & config was this
run with? Could you confirm the clk-raspberrypi.c message verifying the max and
min frequencies showed up and was correct.
Regards,
Nicolas
Am 05.06.19 um 14:27 schrieb Nicolas Saenz Julienne:
> Ok, this looks more more like my fault, probably an overflow error somewhere. I
> saw something similar while testing it on RPI2b. Which board & config was this
> run with?
It's an RPi 3B+ with multi_v7_defconfig
> Could you confirm the clk-raspberrypi.c message verifying the max and
> min frequencies showed up and was correct.
[ 4.253294] raspberrypi-firmware soc:firmware: Attached to firmware
from 2019-03-27 15:45
[ 4.269727] mmcblk0: mmc0:0007 SDCIT 14.6 GiB
[ 4.282464] raspberrypi-clk raspberrypi-clk: CPU frequency range: min
600000000, max 1400000000
>
> Regards,
> Nicolas
>
On Wed, 2019-06-05 at 13:34 +0200, Stefan Wahren wrote:
> Hi,
>
> Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
> > Hi Stefan,
> > thanks for the review, I took note of your code comments.
> >
> > On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
> > > Hi Nicolas,
> > >
> > > Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
> > > > Hi all,
> > > > this series aims at adding cpufreq support to the Raspberry Pi family of
> > > > boards.
> > > >
> > > > The previous revision can be found at:
> > > > https://lkml.org/lkml/2019/5/20/431
> > > >
> > > > The series first factors out 'pllb' from clk-bcm2385 and creates a new
> > > > clk driver that operates it over RPi's firmware interface[1]. We are
> > > > forced to do so as the firmware 'owns' the pll and we're not allowed to
> > > > change through the register interface directly as we might race with the
> > > > over-temperature and under-voltage protections provided by the firmware.
> > > it would be nice to preserve such design decision in the driver as a
> > > comment, because the cover letter usually get lost.
> > > > Next it creates a minimal cpufreq driver that populates the CPU's opp
> > > > table, and registers cpufreq-dt. Which is needed as the firmware
> > > > controls the max and min frequencies available.
> > > I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
> > > manually enable this drivers. During boot with Raspbian rootfs i'm
> > > getting the following:
> > >
> > > [ 1.177009] cpu cpu0: failed to get clock: -2
> > > [ 1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
> > This is surprising, who could be creating a platform_device for cpufreq-dt
> > apart from raspberrypi-cpufreq? Just to make things clear, you're using the
> > device tree from v5.2-rc1 (as opposed to the Raspbian one)?
>
> sorry my fault, i thought it already has been replaced. The behavior in
> this unexpected case is fine, since it doesn't crash.
>
> I replaced the the DTB with the mainline one, but now i'm getting this:
>
> [ 4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
> 600000 KHz
> [ 4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
> [ 4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22
For the record this fixes it:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756fd4d6..edb71eefe9cf 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1293,7 +1293,7 @@ static int clk_core_determine_round_nolock(struct clk_core
*core,
} else if (core->ops->round_rate) {
rate = core->ops->round_rate(core->hw, req->rate,
&req->best_parent_rate);
- if (rate < 0)
+ if (IS_ERR_VALUE(rate))
return rate;
req->rate = rate;
round_rate() returns a 'long' value, yet 'pllb' in rpi3b+ goes as high as
2.8GHz, which only fits in an 'unsigned long'. This explains why I didn't see
this issue with RPI2b.
I'll add the patch to the series.
Regards,
Nicolas
Hi Nicolas,
>> Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
>>> Hi Stefan,
>>> thanks for the review, I took note of your code comments.
>>>
>>> On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
>>>> Hi Nicolas,
>>>>
>>>> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
>>>>> Hi all,
>>>>> this series aims at adding cpufreq support to the Raspberry Pi family of
>>>>> boards.
>>>>>
>>>>> The previous revision can be found at:
>>>>> https://lkml.org/lkml/2019/5/20/431
>>>>>
>>>>> The series first factors out 'pllb' from clk-bcm2385 and creates a new
>>>>> clk driver that operates it over RPi's firmware interface[1]. We are
>>>>> forced to do so as the firmware 'owns' the pll and we're not allowed to
>>>>> change through the register interface directly as we might race with the
>>>>> over-temperature and under-voltage protections provided by the firmware.
>>>> it would be nice to preserve such design decision in the driver as a
>>>> comment, because the cover letter usually get lost.
>>>>> Next it creates a minimal cpufreq driver that populates the CPU's opp
>>>>> table, and registers cpufreq-dt. Which is needed as the firmware
>>>>> controls the max and min frequencies available.
>>>> I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
>>>> manually enable this drivers. During boot with Raspbian rootfs i'm
>>>> getting the following:
>>>>
>>>> [ 1.177009] cpu cpu0: failed to get clock: -2
>>>> [ 1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
>>> This is surprising, who could be creating a platform_device for cpufreq-dt
>>> apart from raspberrypi-cpufreq? Just to make things clear, you're using the
>>> device tree from v5.2-rc1 (as opposed to the Raspbian one)?
>> sorry my fault, i thought it already has been replaced. The behavior in
>> this unexpected case is fine, since it doesn't crash.
>>
>> I replaced the the DTB with the mainline one, but now i'm getting this:
>>
>> [ 4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
>> 600000 KHz
>> [ 4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
>> [ 4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22
> For the record this fixes it:
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index aa51756fd4d6..edb71eefe9cf 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1293,7 +1293,7 @@ static int clk_core_determine_round_nolock(struct clk_core
> *core,
> } else if (core->ops->round_rate) {
> rate = core->ops->round_rate(core->hw, req->rate,
> &req->best_parent_rate);
> - if (rate < 0)
> + if (IS_ERR_VALUE(rate))
> return rate;
>
> req->rate = rate;
>
> round_rate() returns a 'long' value, yet 'pllb' in rpi3b+ goes as high as
> 2.8GHz, which only fits in an 'unsigned long'. This explains why I didn't see
> this issue with RPI2b.
Okay, i understand the problem. But the patch doesn't look like the
proper fix to me.
Maybe the better way is to implement determine_rate instead of
round_rate in the clock driver. AFAICS this provides the necessary
unsigned long.
>
> I'll add the patch to the series.
>
> Regards,
> Nicolas