| Message ID | 3028e04887c7b8a6ffc150c016aa63281461b434.1582533919.git-series.maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Support BCM2711 Display Pipeline | expand |
Hi Maxime, Am 24.02.20 um 10:06 schrieb Maxime Ripard: > The driver has really only supported one clock so far and has hardcoded the > ID used in communications with the firmware in all the functions > implementing the clock framework hooks. Let's store that in the clock data > structure so that we can support more clocks later on. thank you for this series. I looked through it but i couldn't find an explanation why we need to expose firmware clocks via DT instead of extending clk-bcm2835. The whole pllb / clk-raspberrypi stuff was an exception to get cpufreq working. I prefer to keep it an exception. Regards Stefan
Hi Stefan, On Mon, Feb 24, 2020 at 08:25:46PM +0100, Stefan Wahren wrote: > Hi Maxime, > > Am 24.02.20 um 10:06 schrieb Maxime Ripard: > > The driver has really only supported one clock so far and has hardcoded the > > ID used in communications with the firmware in all the functions > > implementing the clock framework hooks. Let's store that in the clock data > > structure so that we can support more clocks later on. > > thank you for this series. I looked through it but i couldn't find an > explanation why we need to expose firmware clocks via DT instead of > extending clk-bcm2835. The whole pllb / clk-raspberrypi stuff was an > exception to get cpufreq working. I prefer to keep it an exception. Thanks for pointing this out, I indeed forgot to address it in my cover letter or my commit log. I'm not quite sure what the situation was with the previous RaspberryPi, but the RPi4 firmware does a bunch of things under the hood to make sure that everything works as expected: - The HSM (and V3D) clocks will be reparented to multiple PLLs depending on the rate being asked for. - Still depending on the rate, the firmware will adjust the voltage of the various PLLs. - Depending on the temperature of the CPU and GPU, the firmware will change the rate of clocks to throttle in case of the cores overheating, with all the fallout that might happen to clocks deriving from it. - No matter what we choose to do in Linux, this will happen so whether or not we want to do it, so doing it behind the firmware's back (or the firmware doing it behind Linux's back) will only result in troubles, with voltages too low, or the firmware trying to access the same register at the same time than the Linux driver would, etc. So all in all, it just seems much easier and safer to use the firmware clocks. Maxime
Hi Maxime, On Tue, 2020-02-25 at 10:54 +0100, Maxime Ripard wrote: > Hi Stefan, > > On Mon, Feb 24, 2020 at 08:25:46PM +0100, Stefan Wahren wrote: > > Hi Maxime, > > > > Am 24.02.20 um 10:06 schrieb Maxime Ripard: > > > The driver has really only supported one clock so far and has hardcoded > > > the > > > ID used in communications with the firmware in all the functions > > > implementing the clock framework hooks. Let's store that in the clock data > > > structure so that we can support more clocks later on. > > > > thank you for this series. I looked through it but i couldn't find an > > explanation why we need to expose firmware clocks via DT instead of > > extending clk-bcm2835. The whole pllb / clk-raspberrypi stuff was an > > exception to get cpufreq working. I prefer to keep it an exception. > > Thanks for pointing this out, I indeed forgot to address it in my > cover letter or my commit log. > > I'm not quite sure what the situation was with the previous > RaspberryPi, but the RPi4 firmware does a bunch of things under the > hood to make sure that everything works as expected: > > - The HSM (and V3D) clocks will be reparented to multiple PLLs > depending on the rate being asked for. > - Still depending on the rate, the firmware will adjust the voltage > of the various PLLs. > - Depending on the temperature of the CPU and GPU, the firmware will > change the rate of clocks to throttle in case of the cores > overheating, with all the fallout that might happen to clocks > deriving from it. > - No matter what we choose to do in Linux, this will happen so > whether or not we want to do it, so doing it behind the firmware's > back (or the firmware doing it behind Linux's back) will only > result in troubles, with voltages too low, or the firmware trying > to access the same register at the same time than the Linux driver > would, etc. > > So all in all, it just seems much easier and safer to use the firmware > clocks. I agree with your assesment. Both DVFS and overheating/overvoltage protections will cause trouble, if not, make a Linux solution impossible while using the Foundation's firmware. Please note that, as Stefan says, it'd be nice to keep track of those arguments somewhere in the commit messages. Regards, Nicolas
On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote: > The driver has really only supported one clock so far and has hardcoded the > ID used in communications with the firmware in all the functions > implementing the clock framework hooks. Let's store that in the clock data > structure so that we can support more clocks later on. > > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: linux-clk@vger.kernel.org > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Thanks! Nicolas
Quoting Maxime Ripard (2020-02-24 01:06:18) > The driver has really only supported one clock so far and has hardcoded the > ID used in communications with the firmware in all the functions > implementing the clock framework hooks. Let's store that in the clock data > structure so that we can support more clocks later on. > > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: linux-clk@vger.kernel.org > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- Reviewed-by: Stephen Boyd <sboyd@kernel.org>
diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index 964fc8f792cc..e796dabbc641 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -39,6 +39,7 @@ struct raspberrypi_clk { struct raspberrypi_clk_data { struct clk_hw hw; + unsigned id; unsigned long min_rate; unsigned long max_rate; @@ -95,7 +96,7 @@ static int raspberrypi_fw_pll_is_on(struct clk_hw *hw) ret = raspberrypi_clock_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCK_STATE, - RPI_FIRMWARE_ARM_CLK_ID, &val); + data->id, &val); if (ret) return 0; @@ -114,8 +115,7 @@ static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw, ret = raspberrypi_clock_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCK_RATE, - RPI_FIRMWARE_ARM_CLK_ID, - &val); + data->id, &val); if (ret) return ret; @@ -133,8 +133,7 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate, ret = raspberrypi_clock_property(rpi->firmware, RPI_FIRMWARE_SET_CLOCK_RATE, - RPI_FIRMWARE_ARM_CLK_ID, - &new_rate); + data->id, &new_rate); if (ret) dev_err_ratelimited(rpi->dev, "Failed to change %s frequency: %d", clk_hw_get_name(hw), ret); @@ -189,6 +188,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi) if (!data) return -ENOMEM; data->rpi = rpi; + data->id = RPI_FIRMWARE_ARM_CLK_ID; /* All of the PLLs derive from the external oscillator. */ init.parent_names = (const char *[]){ "osc" }; @@ -200,8 +200,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi) /* 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); + data->id, &min_rate); if (ret) { dev_err(rpi->dev, "Failed to get %s min freq: %d\n", init.name, ret); @@ -210,8 +209,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi) ret = raspberrypi_clock_property(rpi->firmware, RPI_FIRMWARE_GET_MAX_CLOCK_RATE, - RPI_FIRMWARE_ARM_CLK_ID, - &max_rate); + data->id, &max_rate); if (ret) { dev_err(rpi->dev, "Failed to get %s max freq: %d\n", init.name, ret);
The driver has really only supported one clock so far and has hardcoded the ID used in communications with the firmware in all the functions implementing the clock framework hooks. Let's store that in the clock data structure so that we can support more clocks later on. Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@kernel.org> Cc: linux-clk@vger.kernel.org Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/bcm/clk-raspberrypi.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)