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 <nsaenzjulie...@suse.de> > > --- > > > > 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
signature.asc
Description: This is a digitally signed message part